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@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 <[email protected]>
> >>+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
signature.asc
Description: PGP signature
_______________________________________________ U-Boot mailing list [email protected] https://lists.denx.de/listinfo/u-boot
