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

[Tom Rini]

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>
> >>+void sdram_init(void)
> >>+{
> >>+	int ram_type_index = PHYCORE_R2_MT41K128M16JT_256MB;
> >>+
> >>+	if (fdtdec_setup_mem_size_base())
> >>+		gd->ram_size = SZ_256M;
> >>+
> >>+	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);
> >>+}
> >
> >This is wrong.  sdram_init() is called by
> >arch/arm/mach-omap2/am33xx/board.c::dram_init() which then sets
> >gd->ram_size based on what get_ram_size() determines.  So this is all
> >just a wrapper around how the various parts of the am33xx generations
> >call some form of config_ddr().  And what you have here is a lot of
> >unused code about which module provides how much memory.  I assume
> >there's some run-time method to determine which module you're on and
> >thus determine that correct parameters to pass in for the chip that's in
> >use.  If you're not there yet then just make sdram_init() call
> >config_ddr(...) with the correct enum for the 256M chip and then update
> >this when you have real detection.
> 
> Thanks for that input, you are right. I could not find any documented way to
> detect the exact module we are running on, but as you pointed out we can use
> get_ram_size() to find the size of the installed RAM. This is in fact
> exactly what barebox did, I just missed it. How is this for a replacement of
> the above?
> 
> 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?

-- 
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/20190506/bf66167d/attachment.sig>