Introducing the Broadcom bcm281xx Architecture

The Kona architecture is present on a number of Broadcom mobile SoCs including the bcm281xx family of chips.

arch/arm/cpu/armv7/kona-common/lowlevel_init.S new file +5 -0

new file mode 100644
index 0000000..a03afcc
@@ -0,0 +1,15 @@
+/*****************************************************************************
+*
+* Copyright 2013 Broadcom Corporation.  All rights reserved.
+*
+* SPDX-License-Identifier:      GPL-2.0+

arch/arm/cpu/armv7/kona-common/proc.c new file +8 -0

new file mode 100644
index 0000000..92fb39b
@@ -0,0 +1,20 @@
+/*****************************************************************************
+*
+* Copyright 2013 Broadcom Corporation.  All rights reserved.
+*
+* SPDX-License-Identifier:      GPL-2.0+
+*
+*****************************************************************************/
+

Quoted 2 lines · Darwin Rambo

The Kona architecture is present on a number of Broadcom mobile SoCs including the bcm281xx family of chips.

[snip]

arch/arm/cpu/armv7/kona-common/clk-stubs.c line 22

Quoted 8 lines

arch/arm/cpu/armv7/kona-common/clk-stubs.c new file +8 -0

+int __weak clk_sdio_enable(void *base, u32 rate, u32 *actual_ratep)
+{
+	return 0;
+}
+int __weak clk_bsc_enable(void *base, u32 rate, u32 *actual_ratep)
+{
+	return 0;
+}

Blank lines between functions please.

Quoted 16 lines

arch/arm/cpu/armv7/kona-common/lowlevel_init.S new file +9 -0

new file mode 100644
index 0000000..a03afcc
@@ -0,0 +1,15 @@
+/*****************************************************************************
+*
+* Copyright 2013 Broadcom Corporation.  All rights reserved.
+*
+* SPDX-License-Identifier:      GPL-2.0+
+*
+*****************************************************************************/
+
+//#include <asm-offsets.h>

arch/arm/cpu/armv7/kona-common/lowlevel_init.S line 15

Quoted 6 lines

arch/arm/cpu/armv7/kona-common/lowlevel_init.S new file +6 -0

+//#include <config.h>
+//#include <linux/linkage.h>
+
+.globl lowlevel_init
+lowlevel_init:
+	mov	pc, lr

(a) no commented out include lines

(b) This is empty, which seems wrong. I strongly suspect you want to make use of arch/arm/cpu/armv7/lowlevel_init.S and have an s_init function, ala omap*/tegra/rmobile/etc.

Quoted 19 lines

arch/arm/cpu/armv7/kona-common/proc.c new file +12 -0

new file mode 100644
index 0000000..92fb39b
@@ -0,0 +1,20 @@
+/*****************************************************************************
+*
+* Copyright 2013 Broadcom Corporation.  All rights reserved.
+*
+* SPDX-License-Identifier:      GPL-2.0+
+*
+*****************************************************************************/
+
+#include <common.h>
+#include <asm/io.h>
+#include <asm/kona-common/misc.h>
+#include <asm/armv7.h>

arch/arm/cpu/armv7/kona-common/proc.c line 18

Quoted 6 lines

arch/arm/cpu/armv7/kona-common/proc.c new file +6 -0

+
+void do_proc_wfe(int forever)
+{
+	do {
+		asm volatile ("wfe	@ wait for event\n");
+		/*printf("wake up from wfe\n"); */

No commented out printf, thanks.

Add bcm281xx architecture support code including a clock framework and chip reset. Define register block base addresses for the bcm281xx architecture and create an empty gpio header file required when CONFIG_CMD_GPIO is set.

arch/arm/cpu/armv7/bcm281xx/clk-core.c new file +11 -0

+/* Bitfield operations */
+
+/* Produces a mask of set bits covering a range of a 32-bit value */
+static inline u32 bitfield_mask(u32 shift, u32 width)
+{
+	return ((1 << width) - 1) << shift;
+}
+
+/* Extract the value of a bitfield found within a given register value */
+static inline u32 bitfield_extract(u32 reg_val, u32 shift, u32 width)
+{

Quoted 4 lines · Darwin Rambo

Add bcm281xx architecture support code including a clock framework and chip reset. Define register block base addresses for the bcm281xx architecture and create an empty gpio header file required when CONFIG_CMD_GPIO is set.

[snip]

Quoted 17 lines

arch/arm/cpu/armv7/bcm281xx/clk-core.c new file +17 -0

+/* Bitfield operations */
+
+/* Produces a mask of set bits covering a range of a 32-bit value */
+static inline u32 bitfield_mask(u32 shift, u32 width)
+{
+	return ((1 << width) - 1) << shift;
+}
+
+/* Extract the value of a bitfield found within a given register value */
+static inline u32 bitfield_extract(u32 reg_val, u32 shift, u32 width)
+{
+	return (reg_val & bitfield_mask(shift, width)) >> shift;
+}
+
+/* Replace the value of a bitfield found within a given register value */
+static inline u32 bitfield_replace(u32 reg_val, u32 shift, u32 width, u32 val)
+{

arch/arm/cpu/armv7/bcm281xx/clk-core.c line 67

Quoted 4 lines

arch/arm/cpu/armv7/bcm281xx/clk-core.c new file +4 -0

+	u32 mask = bitfield_mask(shift, width);
+
+	return (reg_val & ~mask) | (val << shift);
+}

This all feels horribly generic, isn't there some linux header we've already got that I can't think off of the top of my head that gives us these kind of functions?

Quoted 24 lines · Tom Rini

Quoted 4 lines · Darwin Rambo

Add bcm281xx architecture support code including a clock framework and chip reset. Define register block base addresses for the bcm281xx architecture and create an empty gpio header file required when CONFIG_CMD_GPIO is set.

[snip]

Quoted 17 lines

arch/arm/cpu/armv7/bcm281xx/clk-core.c new file +17 -0

+/* Bitfield operations */
+
+/* Produces a mask of set bits covering a range of a 32-bit value */
+static inline u32 bitfield_mask(u32 shift, u32 width)
+{
+	return ((1 << width) - 1) << shift;
+}
+
+/* Extract the value of a bitfield found within a given register value */
+static inline u32 bitfield_extract(u32 reg_val, u32 shift, u32 width)
+{
+	return (reg_val & bitfield_mask(shift, width)) >> shift;
+}
+
+/* Replace the value of a bitfield found within a given register value */
+static inline u32 bitfield_replace(u32 reg_val, u32 shift, u32 width, u32 val)
+{

Quoted 8 lines

arch/arm/cpu/armv7/bcm281xx/clk-core.c line 67

Quoted 4 lines

arch/arm/cpu/armv7/bcm281xx/clk-core.c new file +4 -0

+	u32 mask = bitfield_mask(shift, width);
+
+	return (reg_val & ~mask) | (val << shift);
+}

This all feels horribly generic, isn't there some linux header we've already got that I can't think off of the top of my head that gives us these kind of functions?

Hi Tom,

I had a similar feeling. There are files such as include/linux/bitops.h and arch/arm/include/asm/bitops.h and a host of others, but these seem single bit oriented, and don't provide the functionality here. The bcm281xx clock registers are a myriad of bit fields of different widths and positions, and the driver code is simpler because it uses these generic bitfield functions and data tables to describe the bitfields. Perhaps the bcm281xx clock register hardware has revealed the need for more functions like this now. I've searched through the tree for equivalent functions and they don't seem to exist, but I could be wrong. We could create include/bitfield.h with functions specifically for bitfield operations if it were warranted. But if it only ever got used by one driver, it might be wrong to make it generic. But my gut feel is that if we did create include/bitfield.h it probably would be used by others who wanted to take a similar data-driven approach to register fields. We would also have to make it non-u32 specific I imagine, possibly just 'int' types. Thanks.

Quoted 37 lines · Darwin Rambo

Quoted 24 lines · Tom Rini

Quoted 4 lines · Darwin Rambo

Add bcm281xx architecture support code including a clock framework and chip reset. Define register block base addresses for the bcm281xx architecture and create an empty gpio header file required when CONFIG_CMD_GPIO is set.

[snip]

Quoted 17 lines

arch/arm/cpu/armv7/bcm281xx/clk-core.c new file +17 -0

+/* Bitfield operations */
+
+/* Produces a mask of set bits covering a range of a 32-bit value */
+static inline u32 bitfield_mask(u32 shift, u32 width)
+{
+	return ((1 << width) - 1) << shift;
+}
+
+/* Extract the value of a bitfield found within a given register value */
+static inline u32 bitfield_extract(u32 reg_val, u32 shift, u32 width)
+{
+	return (reg_val & bitfield_mask(shift, width)) >> shift;
+}
+
+/* Replace the value of a bitfield found within a given register value */
+static inline u32 bitfield_replace(u32 reg_val, u32 shift, u32 width, u32 val)
+{

Quoted 8 lines

arch/arm/cpu/armv7/bcm281xx/clk-core.c line 67

Quoted 4 lines

arch/arm/cpu/armv7/bcm281xx/clk-core.c new file +4 -0

+	u32 mask = bitfield_mask(shift, width);
+
+	return (reg_val & ~mask) | (val << shift);
+}

This all feels horribly generic, isn't there some linux header we've already got that I can't think off of the top of my head that gives us these kind of functions?

Hi Tom,

Quoted 16 lines

I had a similar feeling. There are files such as include/linux/bitops.h and arch/arm/include/asm/bitops.h and a host of others, but these seem single bit oriented, and don't provide the functionality here. The bcm281xx clock registers are a myriad of bit fields of different widths and positions, and the driver code is simpler because it uses these generic bitfield functions and data tables to describe the bitfields. Perhaps the bcm281xx clock register hardware has revealed the need for more functions like this now. I've searched through the tree for equivalent functions and they don't seem to exist, but I could be wrong. We could create include/bitfield.h with functions specifically for bitfield operations if it were warranted. But if it only ever got used by one driver, it might be wrong to make it generic. But my gut feel is that if we did create include/bitfield.h it probably would be used by others who wanted to take a similar data-driven approach to register fields. We would also have to make it non-u32 specific I imagine, possibly just 'int' types. Thanks.

With Matt chiming in on where this is within the kernel, lets go with creating a include/bitfield.h here. Thanks!

Quoted 4 lines · Darwin Rambo

Add bcm281xx architecture support code including a clock framework and chip reset. Define register block base addresses for the bcm281xx architecture and create an empty gpio header file required when CONFIG_CMD_GPIO is set.

[snip]

Quoted 17 lines

arch/arm/cpu/armv7/bcm281xx/clk-core.c new file +17 -0

+/* Bitfield operations */
+
+/* Produces a mask of set bits covering a range of a 32-bit value */
+static inline u32 bitfield_mask(u32 shift, u32 width)
+{
+	return ((1 << width) - 1) << shift;
+}
+
+/* Extract the value of a bitfield found within a given register value */
+static inline u32 bitfield_extract(u32 reg_val, u32 shift, u32 width)
+{
+	return (reg_val & bitfield_mask(shift, width)) >> shift;
+}
+
+/* Replace the value of a bitfield found within a given register value */
+static inline u32 bitfield_replace(u32 reg_val, u32 shift, u32 width, u32 val)
+{

arch/arm/cpu/armv7/bcm281xx/clk-core.c line 67

Quoted 4 lines

arch/arm/cpu/armv7/bcm281xx/clk-core.c new file +4 -0

+	u32 mask = bitfield_mask(shift, width);
+
+	return (reg_val & ~mask) | (val << shift);
+}

This all feels horribly generic, isn't there some linux header we've already got that I can't think off of the top of my head that gives us these kind of functions?

Add support for the bcm28155_ap reference board.

Signed-off-by Darwin Rambo <[email protected]>

Reviewed-by Steve Rae <[email protected]>

Reviewed-by Tim Kryger <[email protected]>

+#define CONFIG_SYS_PROMPT_HUSH_PS2 "> "

Quoted 5 lines · Darwin Rambo

Add support for the bcm28155_ap reference board.

Signed-off-by Darwin Rambo <[email protected]>

Reviewed-by Steve Rae <[email protected]>

Reviewed-by Tim Kryger <[email protected]>

[snip]

board/broadcom/bcm28155_ap/bcm28155_ap.c line 60

Quoted 1 line

board/broadcom/bcm28155_ap/bcm28155_ap.c new file +1 -0

+	gd->ram_size = CONFIG_SYS_SDRAM_SIZE;

Please use get_ram_size(...);

include/configs/bcm28155_ap.h line 76

Quoted 4 lines

include/configs/bcm28155_ap.h new file +3 -0

+/* Memory testing range */
+#define CONFIG_SYS_MEMTEST_START	CONFIG_SYS_SDRAM_BASE
+#define CONFIG_SYS_MEMTEST_END		(CONFIG_SYS_MEMTEST_START + SZ_1M)

Please see doc/README.memory-test

include/configs/bcm28155_ap.h line 96

Quoted 3 lines

include/configs/bcm28155_ap.h new file +2 -0

+#define CONFIG_SYS_BAUDRATE_TABLE	{9600, 19200, 38400, 57600, \
+					115200, 230400, 460800, 921600}

Unless you really use those higher values in U-Boot, please use the default table.

Quoted 2 lines

+#define CONFIG_SYS_PROMPT_HUSH_PS2 "> "

Unneeded now.

Quoted 8 lines · Tom Rini

Quoted 5 lines · Darwin Rambo

Add support for the bcm28155_ap reference board.

Signed-off-by Darwin Rambo <[email protected]> Reviewed-by:

Steve Rae <[email protected]> Reviewed-by: Tim Kryger <[email protected]>

[snip]

Quoted 3 lines

board/broadcom/bcm28155_ap/bcm28155_ap.c line 60

Quoted 1 line

board/broadcom/bcm28155_ap/bcm28155_ap.c new file +1 -0

+	gd->ram_size = CONFIG_SYS_SDRAM_SIZE;

Please use get_ram_size(...);

We tried using this on our reference board and it hangs accessing memory regions that are not populated. Our memory controller doesn't appear to properly support accessing regions that are not backed by physical sdram. So I think it's best to keep this code as is and consider this approach for future designs.

Quoted 5 lines · Darwin Rambo

We tried using this on our reference board and it hangs accessing memory regions that are not populated. Our memory controller doesn't appear to properly support accessing regions that are not backed by physical sdram. So I think it's best to keep this code as is and consider this approach for future designs.

Wait, what did you do? get_ram_size(base, max) must work and return CONFIG_SYS_SDRAM_SIZE, or you haven't properly configured your controller (as get_ram_size just pokes parts of the specified range).

Quoted 10 lines · Tom Rini

Quoted 5 lines · Darwin Rambo

We tried using this on our reference board and it hangs accessing memory regions that are not populated. Our memory controller doesn't appear to properly support accessing regions that are not backed by physical sdram. So I think it's best to keep this code as is and consider this approach for future designs.

Wait, what did you do? get_ram_size(base, max) must work and return CONFIG_SYS_SDRAM_SIZE, or you haven't properly configured your controller (as get_ram_size just pokes parts of the specified range).

Tom,

In my experience, there are three general ways memory controllers behave when an access is made to an address that could be backed by RAM but isn't on a particular board.

1. All reads return a fixed pattern and writes are ignored
2. A bus error is triggered that the CPU sees as a data abort
3. No response to the transaction so the bus and CPU lock up

I believe the Broadcom memory controller can only behave in the third, least desirable way.

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 22 lines · Tim Kryger

Quoted 10 lines · Tom Rini

Quoted 5 lines · Darwin Rambo

We tried using this on our reference board and it hangs accessing memory regions that are not populated. Our memory controller doesn't appear to properly support accessing regions that are not backed by physical sdram. So I think it's best to keep this code as is and consider this approach for future designs.

Wait, what did you do? get_ram_size(base, max) must work and return CONFIG_SYS_SDRAM_SIZE, or you haven't properly configured your controller (as get_ram_size just pokes parts of the specified range).

Tom,

In my experience, there are three general ways memory controllers behave when an access is made to an address that could be backed by RAM but isn't on a particular board.

1. All reads return a fixed pattern and writes are ignored
2. A bus error is triggered that the CPU sees as a data abort
3. No response to the transaction so the bus and CPU lock up

Quoted 2 lines

I believe the Broadcom memory controller can only behave in the third, least desirable way.

That's fine. You should still use get_ram_size(base, CONFIG_SYS_SDRAM_SIZE) like other platforms use as this go above the max size it's given. If this hangs, you have a problem with your configuration of the memory controller. Make sense?

- --
Tom
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJS69oUAAoJENk4IS6UOR1Wf/AP/2ApLwjAffxJ7LmEFFEK7hlr +dq1FW9ZRppfLRbfUrZz8Q/muyrx1ZuF9pIjjrFxLkNSLS3t8LZrJmfeswePqGC+ dq6qLl+ovk0VNYsxEhS77j1QQ/LLri67QCiGEZuS8vDB7qxiyuuCXkzyyW8hRFZ7 yfKpbfdRYpzWTJhAYnB0b1OaBi1jamsnjhXNU5XNa/H3PfDDofNA2fFQQfKbleOH BVOka1LP/+28gqKksEziM9cbcjPYJdE/FfY75nkbqKl9PWl+qIUZli90QO3Qdbgy uJJB1d3v6amfHZYbJymR3r8FoRjjCKxnCza5rCLpwFLxAKCUIIGHBFUGpvdcFoVa YctuEu2Dx2SgQ9dEGbLT1d2bsbfxOATEr5CyCpD7vD/9e/Ze7XaXALVKocfQmDbs 77s0YvHuXLGQPudPzYrEqPvgxf/NlNjeLj5IWqF++AE3j5U1sE26x95qk8jobMSn KjYGwzsxnd/gErqiYBawkrHxS2f4beAucsoprCh0WmSXiqSh6Wzc/+XlAIVSV7vm UE969gAzP1F2j37QfWR2/B0ILP27Pk3Ano8Dj2MIVKCAVkkpSQ5SCqAA7wvMzHej 801O52kArhG3/G/WrAbeLoQxKsdUyNI92+T0UaquTI4hRZk5GSvHQNCaZAcx6fCa hZ1o6zG9nMsp/yzVe7VZ
=0yIj
-----END PGP SIGNATURE-----

This patchset introduces the Broadcom bcm281xx family of mobile SoC chips. Broadcom kona hardware blocks are often found in Broadcom mobile SoC chips including the bcm281xx family, so support for some of these kona blocks is also provided here. These patches work on u-boot master as well as the u-boot-arm custodian tree.

Darwin Rambo (6):
arch: kona: Initial commit of kona-common architecture code
arch: bcm281xx: Initial commit of bcm281xx architecture code
gpio: kona: Add Kona gpio driver
i2c: kona: Add Kona I2C driver
mmc: kona: Add Kona mmc driver
board: bcm28155_ap: Add board files

Quoted 6 lines · Darwin Rambo

This patchset introduces the Broadcom bcm281xx family of mobile SoC chips. Broadcom kona hardware blocks are often found in Broadcom mobile SoC chips including the bcm281xx family, so support for some of these kona blocks is also provided here. These patches work on u-boot master as well as the u-boot-arm custodian tree.

Quoted 7 lines

Darwin Rambo (6):
arch: kona: Initial commit of kona-common architecture code
arch: bcm281xx: Initial commit of bcm281xx architecture code
gpio: kona: Add Kona gpio driver
i2c: kona: Add Kona I2C driver
mmc: kona: Add Kona mmc driver
board: bcm28155_ap: Add board files

Thanks for posting. Globally, please don't introduce another style for the file header,
/*
* This works
* just fine.
*
* Including what the file is about
*/

And I also see the incorrect style sprinkled throughout.

I saw some '#define<tab>' mixed in with '#define<space>', and some other things I'd have sworn checkpatch complains about, so please make sure things are checkpatch clean (except for very long printed strings, which are an exception to line length rules).

Finally, please CC the custodians for i2c/mmc for those drivers as they may have further comments than the above list.

Thanks!

On 14-01-29 02:32 PM, Tom Rini wrote:

Quoted 2 lines

Finally, please CC the custodians for i2c/mmc for those drivers as they may have further comments than the above list.

OK. There are mmc, i2c and gpio drivers. I assume each driver gets a different CC:?