[U-Boot] [PATCH v5] ARM: am335x: Add phyCORE AM335x R2 support

[Tom Rini]

On Tue, May 07, 2019 at 11:39:12AM +0200, Niel Fourie wrote:
> Hi Tom,
> 
> On 5/6/19 7:24 PM, Tom Rini wrote:
> >On Mon, May 06, 2019 at 06:44:48PM +0200, Niel Fourie wrote:
> >>Hi Tom,
> >>
> >>On 5/6/19 4:18 PM, Tom Rini wrote:
> >>>On Mon, May 06, 2019 at 04:02:53PM +0200, Niel Fourie wrote:
> >>>
> >>>>Support for Phytech phyCORE AM335x R2 SOM (PCL060) on the Phytec
> >>>>phyBOARD-Wega AM335x.
> >>>>
> >>>>CPU  : AM335X-GP rev 2.1
> >>>>Model: Phytec AM335x phyBOARD-WEGA
> >>>>DRAM:  256 MiB
> >>>>NAND:  256 MiB
> >>>>MMC:   OMAP SD/MMC: 0
> >>>>eth0: ethernet at 4a100000
> >>>>
> >>>>Working:
> >>>>  - Eth0
> >>>>  - i2C
> >>>>  - MMC/SD
> >>>>  - NAND
> >>>>  - UART
> >>>>  - USB (host)
> >>>>
> >>>>Device trees were taken from Linux mainline:
> >>>>commit 37624b58542f ("Linux 5.1-rc7")
> >>>>
> >>>>Signed-off-by: Niel Fourie <lusus at denx.de>
> [snip]
> >>
> >>void sdram_init(void)
> >>{
> >>	/* Configure memory to maximum supported size for detection */
> >>	int ram_type_index = PHYCORE_R2_MT41K512M16HA125IT_1024MB;
> >>	config_ddr(DDR_CLK_MHZ, &ioregs,
> >>		   &physom_timings[ram_type_index].ddr3_data,
> >>		   &ddr3_cmd_ctrl_data,
> >>		   &physom_timings[ram_type_index].ddr3_emif_reg_data,
> >>		   0);
> >>
> >>	/* Detect memory physically present */
> >>	gd->ram_size = get_ram_size((void *)CONFIG_SYS_SDRAM_BASE,
> >>				    CONFIG_MAX_RAM_BANK_SIZE);
> >>
> >>	/* Reconfigure memory for actual detected size */
> >>	switch (gd->ram_size) {
> >>	case SZ_1G:
> >>		ram_type_index = PHYCORE_R2_MT41K512M16HA125IT_1024MB;
> >>		break;
> >>	case SZ_512M:
> >>		ram_type_index = PHYCORE_R2_MT41K256M16TW107IT_512MB;
> >>		break;
> >>	case SZ_256M:
> >>	default:
> >>		ram_type_index = PHYCORE_R2_MT41K128M16JT_256MB;
> >>		break;
> >>	}
> >>	config_ddr(DDR_CLK_MHZ, &ioregs,
> >>		   &physom_timings[ram_type_index].ddr3_data,
> >>		   &ddr3_cmd_ctrl_data,
> >>		   &physom_timings[ram_type_index].ddr3_emif_reg_data,
> >>		   0);
> >>}
> >>
> >>The ugliest part of this is, as you pointed out, that directly after this is
> >>called, get_ram_size() will be called again from sdram_init(). But it at
> >>least noninvasive, and no longer requires the device tree.
> >
> >I don't think it's safe to call config_ddr twice, especially with the
> >possibly wrong parameters.  What's barebox doing in this case, being
> >told the presumably correct DDR size in the device tree?
> 
> Good point. Barebox uses the above mechanism to detect the memory size, and
> I could find no equivalent memory size specified in its internal device
> tree.

Configure for 1GB and then see how much we can actually talk to?

> Marek originally proposed using the memory size specified in the device tree
> as an improvement over specifying the size in the defconfig (as in v1 of the
> patch).

But then you aren't populating 3 device trees nor making it clear / easy
to say which module you're on, and then still need to change the config
for which DT you're picking up.  These SOMs really don't provide any
run-time method to see which one you're on?  There's no GPIOs to poke?

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190507/30419d0d/attachment.sig>