Re: [PATCH 0/4] arm: bcmbca: add basic support for more Broadcom BCA SoCs

[William Zhang]

Hi Florian,

Thanks for the review!  Please see my comments inline blow,

On 08/08/2022 08:29 PM, Florian Fainelli wrote:

On 8/5/2022 6:33 PM, William Zhang wrote:

Broadcom BCA (Broadband Carrier Access origin) chipset family includes
DSL, PON and WLAN access point and gateway SoC. Now that the ARCH_BCMBCA
architecture and its first SoC BCM47622 are supported in u-boot 2022.07,
this patch series add the basic support for following BCA chips under
ARCH_BCMBCA: BCM4908, BCM4912, BCM63146 and BCM6813.

This patch series applies on top of the my previous patch [1].

[1] https://lists.denx.de/pipermail/u-boot/2022-August/491060.html

Looks good to me, thanks William! On the mmu_table.c implementation maybe just a few nits: - should not we do an early parsing of the memory node for the given board(s) to ensure that we map no more than the amount of available DRAM?

Yes there will be a patch after all these SoC patches to set the ddr size during the dram_init based on the actually memory size.

- the exact same file is currently being re-used, so it would make sense to make it a common object

For these initial soc support patches, I just include the ddr and periph block and they happen to be the same range. But different SoC has different ip block address as we add more more blocks/drivers late. To avoid many ifdef, I would prefer to have one file per chip.

- you could create a memory mapping for the AXI bus region right away to avoid forgetting about it later if you start bringing up drivers that make use of that peripheral region

Yeah I could but we won't forget either because system will crash if we miss that entry in the mmu table. IMHO it is better to limit the access than opening a wide range that we don't use. We can catch invalid/unintended access by only allowing the regions that we need to access.

Thanks!

smime.p7s
Description: S/MIME Cryptographic Signature