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

[Niel Fourie]

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.

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).

Best regards,
Niel Fourie

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-21 Fax: +49-8142-66989-80  Email: lusus at denx.de