Re: [U-Boot] [PATCH] arm: imx6: configure NoC on i.MX6DQP

[Christian Gmeiner]

Hi Filip

Is there any update on this patch? I am looking into imx6qp bring-up
and this looks like
a nice patch.

2016-10-04 10:51 GMT+02:00 Stefano Babic :
> Hi Filip,
>
> On 14/09/2016 13:36, Filip Brozovic wrote:
>> The i.MX6DP and i.MX6QP incorporate NoC interconnect logic
>> which needs to be configured in order to use external DDR memory.
>>
>> This patch enables the SPL to configure the necessary registers
>> in accordance with the NXP engineering bulletin EB828.
>>
>> Signed-off-by: Filip Brozovic 
>> ---
>>  arch/arm/cpu/armv7/mx6/ddr.c| 69 
>> +
>>  arch/arm/include/asm/arch-mx6/mx6-ddr.h | 19 +
>>  2 files changed, 88 insertions(+)
>>
>> diff --git a/arch/arm/cpu/armv7/mx6/ddr.c b/arch/arm/cpu/armv7/mx6/ddr.c
>> index 7beb7ea..60bfd08 100644
>> --- a/arch/arm/cpu/armv7/mx6/ddr.c
>> +++ b/arch/arm/cpu/armv7/mx6/ddr.c
>> @@ -1181,6 +1181,8 @@ void mx6_ddr3_cfg(const struct mx6_ddr_sysinfo 
>> *sysinfo,
>>  {
>>   volatile struct mmdc_p_regs *mmdc0;
>>   volatile struct mmdc_p_regs *mmdc1;
>> + struct src *src_regs = (struct src *)SRC_BASE_ADDR;
>> + u8 soc_boot_cfg3 = ((readl(_regs->sbmr1) & 0xff) >> 16) & 0xff;
>>   u32 val;
>>   u8 tcke, tcksrx, tcksre, txpdll, taofpd, taonpd, trrd;
>>   u8 todtlon, taxpd, tanpd, tcwl, txp, tfaw, tcl;
>> @@ -1473,6 +1475,73 @@ void mx6_ddr3_cfg(const struct mx6_ddr_sysinfo 
>> *sysinfo,
>>   /* Step 12: Configure and activate periodic refresh */
>>   mmdc0->mdref = (sysinfo->refsel << 14) | (sysinfo->refr << 11);
>>
>> + /*
>> +  * i.MX6DQP only: If the NoC scheduler is enabled,
>> +  * configure it and disable MMDC arbitration/reordering (see EB828)
>> +  */
>> + if (is_mx6dqp() && ((soc_boot_cfg3 & 0x30) == 0x00 ||
>> + (soc_boot_cfg3 & 0x33) == 0x31))
>
> I would suggest you add defines for the value of the "DDR Memory Map
> default config" bit, because this is not very readable. And you add in
> the comment a reference to the manual, i.e. a reference to the table
> (5.5 ?) where BOOT_CFG3 is explained.
>
>  {
>> + struct mx6dqp_noc_sched_regs *noc_sched =
>> + (struct mx6dqp_noc_sched_regs *)MX6DQP_NOC_SCHED_BASE;
>> +
>> + /* These values are fixed based on integration parameters and
>> +  * should not be modified */
>
> Codestyle: Multi-line comment must be fixed
>
>> + noc_sched->rlat = 0x0040;
>> + noc_sched->ipu1 = 0x0020;
>> + noc_sched->ipu2 = 0x0020;
>> +
>> + noc_sched->activate = (1 << 10) |   /* FawBank */
>> +   (tfaw << 4) |
>> +   (trrd << 0);
>> + noc_sched->ddrtiming = (((sysinfo->dsize == 1) ? 1 : 0) << 31) 
>> |
>> +((tcwl + twtr) << 26) |
>> +((tcl - tcwl + 2) << 21) |
>> +(4 << 18) |  /* Burst Length = 8 */
>> +((tcwl + twr + trp + trcd) << 12) |
>> +((trtp + trp + trcd - 4) << 6) |
>> +(trc << 0);
>> +
>
> Same here: what about to add defines for shifting and seting into the
> register ? This is not very nice to read it.
>
>
>> + if (sysinfo->dsize == 2) {
>> + if (ddr3_cfg->coladdr == 10) {
>> + if (ddr3_cfg->rowaddr == 15 &&
>> + sysinfo->ncs == 2)
>> + noc_sched->ddrconf = 4;
>> + else
>> + noc_sched->ddrconf = 0;
>> + } else if (ddr3_cfg->coladdr == 11) {
>> + noc_sched->ddrconf = 1;
>> + }
>> + } else {
>> + if (ddr3_cfg->coladdr == 9) {
>> + if (ddr3_cfg->rowaddr == 13)
>> + noc_sched->ddrconf = 2;
>> + else if (ddr3_cfg->rowaddr == 14)
>> + noc_sched->ddrconf = 15;
>> + } else if (ddr3_cfg->coladdr == 10) {
>> + if (ddr3_cfg->rowaddr == 14 &&
>> + sysinfo->ncs == 2)
>> + noc_sched->ddrconf = 14;
>> + else if (ddr3_cfg->rowaddr == 15 &&
>> +  sysinfo->ncs == 2)
>> + noc_sched->ddrconf = 9;
>> + else
>> + noc_sched->ddrconf = 3;
>> + } else if (ddr3_cfg->coladdr == 11) {
>> + if (ddr3_cfg->rowaddr == 15 &&
>> +   

Re: [U-Boot] [PATCH] arm: imx6: configure NoC on i.MX6DQP

[Stefano Babic]

Hi Filip,

On 14/09/2016 13:36, Filip Brozovic wrote:
> The i.MX6DP and i.MX6QP incorporate NoC interconnect logic
> which needs to be configured in order to use external DDR memory.
> 
> This patch enables the SPL to configure the necessary registers
> in accordance with the NXP engineering bulletin EB828.
> 
> Signed-off-by: Filip Brozovic 
> ---
>  arch/arm/cpu/armv7/mx6/ddr.c| 69 
> +
>  arch/arm/include/asm/arch-mx6/mx6-ddr.h | 19 +
>  2 files changed, 88 insertions(+)
> 
> diff --git a/arch/arm/cpu/armv7/mx6/ddr.c b/arch/arm/cpu/armv7/mx6/ddr.c
> index 7beb7ea..60bfd08 100644
> --- a/arch/arm/cpu/armv7/mx6/ddr.c
> +++ b/arch/arm/cpu/armv7/mx6/ddr.c
> @@ -1181,6 +1181,8 @@ void mx6_ddr3_cfg(const struct mx6_ddr_sysinfo *sysinfo,
>  {
>   volatile struct mmdc_p_regs *mmdc0;
>   volatile struct mmdc_p_regs *mmdc1;
> + struct src *src_regs = (struct src *)SRC_BASE_ADDR;
> + u8 soc_boot_cfg3 = ((readl(_regs->sbmr1) & 0xff) >> 16) & 0xff;
>   u32 val;
>   u8 tcke, tcksrx, tcksre, txpdll, taofpd, taonpd, trrd;
>   u8 todtlon, taxpd, tanpd, tcwl, txp, tfaw, tcl;
> @@ -1473,6 +1475,73 @@ void mx6_ddr3_cfg(const struct mx6_ddr_sysinfo 
> *sysinfo,
>   /* Step 12: Configure and activate periodic refresh */
>   mmdc0->mdref = (sysinfo->refsel << 14) | (sysinfo->refr << 11);
>  
> + /*
> +  * i.MX6DQP only: If the NoC scheduler is enabled,
> +  * configure it and disable MMDC arbitration/reordering (see EB828)
> +  */
> + if (is_mx6dqp() && ((soc_boot_cfg3 & 0x30) == 0x00 ||
> + (soc_boot_cfg3 & 0x33) == 0x31))

I would suggest you add defines for the value of the "DDR Memory Map
default config" bit, because this is not very readable. And you add in
the comment a reference to the manual, i.e. a reference to the table
(5.5 ?) where BOOT_CFG3 is explained.

 {
> + struct mx6dqp_noc_sched_regs *noc_sched =
> + (struct mx6dqp_noc_sched_regs *)MX6DQP_NOC_SCHED_BASE;
> +
> + /* These values are fixed based on integration parameters and
> +  * should not be modified */

Codestyle: Multi-line comment must be fixed

> + noc_sched->rlat = 0x0040;
> + noc_sched->ipu1 = 0x0020;
> + noc_sched->ipu2 = 0x0020;
> +
> + noc_sched->activate = (1 << 10) |   /* FawBank */
> +   (tfaw << 4) |
> +   (trrd << 0);
> + noc_sched->ddrtiming = (((sysinfo->dsize == 1) ? 1 : 0) << 31) |
> +((tcwl + twtr) << 26) |
> +((tcl - tcwl + 2) << 21) |
> +(4 << 18) |  /* Burst Length = 8 */
> +((tcwl + twr + trp + trcd) << 12) |
> +((trtp + trp + trcd - 4) << 6) |
> +(trc << 0);
> +

Same here: what about to add defines for shifting and seting into the
register ? This is not very nice to read it.


> + if (sysinfo->dsize == 2) {
> + if (ddr3_cfg->coladdr == 10) {
> + if (ddr3_cfg->rowaddr == 15 &&
> + sysinfo->ncs == 2)
> + noc_sched->ddrconf = 4;
> + else
> + noc_sched->ddrconf = 0;
> + } else if (ddr3_cfg->coladdr == 11) {
> + noc_sched->ddrconf = 1;
> + }
> + } else {
> + if (ddr3_cfg->coladdr == 9) {
> + if (ddr3_cfg->rowaddr == 13)
> + noc_sched->ddrconf = 2;
> + else if (ddr3_cfg->rowaddr == 14)
> + noc_sched->ddrconf = 15;
> + } else if (ddr3_cfg->coladdr == 10) {
> + if (ddr3_cfg->rowaddr == 14 &&
> + sysinfo->ncs == 2)
> + noc_sched->ddrconf = 14;
> + else if (ddr3_cfg->rowaddr == 15 &&
> +  sysinfo->ncs == 2)
> + noc_sched->ddrconf = 9;
> + else
> + noc_sched->ddrconf = 3;
> + } else if (ddr3_cfg->coladdr == 11) {
> + if (ddr3_cfg->rowaddr == 15 &&
> + sysinfo->ncs == 2)
> + noc_sched->ddrconf = 4;
> + else
> + noc_sched->ddrconf = 0;
> + } else if (ddr3_cfg->coladdr == 12) {
> +