[U-Boot] [PATCH v2 13/23] sunxi: H3: add and rename some DRAM contoller registers
André Przywara
andre.przywara at arm.com
Sat Dec 17 03:30:17 CET 2016
On 05/12/16 06:26, Simon Glass wrote:
Hi Simon,
> On 4 December 2016 at 18:52, Andre Przywara <andre.przywara at arm.com> wrote:
>> From: Jens Kuske <jenskuske at gmail.com>
>>
>> The IOCR registers got renamed to BDLR to match the public
>> documentation of similar controllers.
>>
>> Signed-off-by: Jens Kuske <jenskuske at gmail.com>
>> Signed-off-by: Andre Przywara <andre.przywara at arm.com>
>> ---
>> arch/arm/include/asm/arch-sunxi/dram_sun8i_h3.h | 43 ++++++++++++++-----------
>> arch/arm/mach-sunxi/dram_sun8i_h3.c | 34 +++++++++----------
>> 2 files changed, 41 insertions(+), 36 deletions(-)
>
> Reviewed-by: Simon Glass <sjg at chromium.org>
>
> Some suggestions below if you have the energy.
>
>>
>> diff --git a/arch/arm/include/asm/arch-sunxi/dram_sun8i_h3.h b/arch/arm/include/asm/arch-sunxi/dram_sun8i_h3.h
>> index d0f2b8a..867fd12 100644
>> --- a/arch/arm/include/asm/arch-sunxi/dram_sun8i_h3.h
>> +++ b/arch/arm/include/asm/arch-sunxi/dram_sun8i_h3.h
>> @@ -81,7 +81,7 @@ struct sunxi_mctl_ctl_reg {
>> u32 rfshtmg; /* 0x90 refresh timing */
>> u32 rfshctl1; /* 0x94 */
>> u32 pwrtmg; /* 0x98 */
>> - u8 res3[0x20]; /* 0x9c */
>> + u8 res3[0x20]; /* 0x9c */
>> u32 dqsgmr; /* 0xbc */
>> u32 dtcr; /* 0xc0 */
>> u32 dtar[4]; /* 0xc4 */
>> @@ -106,20 +106,23 @@ struct sunxi_mctl_ctl_reg {
>> u32 perfhpr[2]; /* 0x1c4 */
>> u32 perflpr[2]; /* 0x1cc */
>> u32 perfwr[2]; /* 0x1d4 */
>> - u8 res8[0x2c]; /* 0x1dc */
>> - u32 aciocr; /* 0x208 */
>> - u8 res9[0xf4]; /* 0x20c */
>> + u8 res8[0x24]; /* 0x1dc */
>> + u32 acmdlr; /* 0x200 AC master delay line register */
>> + u32 aclcdlr; /* 0x204 AC local calibrated delay line register */
>> + u32 aciocr; /* 0x208 AC I/O configuration register */
>> + u8 res9[0x4]; /* 0x20c */
>> + u32 acbdlr[31]; /* 0x210 AC bit delay line registers */
>> + u8 res10[0x74]; /* 0x28c */
>> struct { /* 0x300 DATX8 modules*/
>> - u32 mdlr; /* 0x00 */
>> - u32 lcdlr[3]; /* 0x04 */
>> - u32 iocr[11]; /* 0x10 IO configuration register */
>> - u32 bdlr6; /* 0x3c */
>> - u32 gtr; /* 0x40 */
>> - u32 gcr; /* 0x44 */
>> - u32 gsr[3]; /* 0x48 */
>> + u32 mdlr; /* 0x00 master delay line register */
>> + u32 lcdlr[3]; /* 0x04 local calibrated delay line registers */
>> + u32 bdlr[12]; /* 0x10 bit delay line registers */
>> + u32 gtr; /* 0x40 general timing register */
>> + u32 gcr; /* 0x44 general configuration register */
>> + u32 gsr[3]; /* 0x48 general status registers */
>> u8 res0[0x2c]; /* 0x54 */
>> - } datx[4];
>> - u8 res10[0x388]; /* 0x500 */
>> + } dx[4];
>> + u8 res11[0x388]; /* 0x500 */
>> u32 upd2; /* 0x888 */
>> };
>>
>> @@ -174,12 +177,14 @@ struct sunxi_mctl_ctl_reg {
>>
>> #define ZQCR_PWRDOWN (0x1 << 31) /* ZQ power down */
>
> 1U << 31
>
>>
>> -#define DATX_IOCR_DQ(x) (x) /* DQ0-7 IOCR index */
>> -#define DATX_IOCR_DM (8) /* DM IOCR index */
>> -#define DATX_IOCR_DQS (9) /* DQS IOCR index */
>> -#define DATX_IOCR_DQSN (10) /* DQSN IOCR index */
>> +#define ACBDLR_WRITE_DELAY(x) ((x) << 8)
>
> Better to have
>
> #define ACBDLR_WRITE_DELAY_SHIFT 8
> #define ACBDLR_WRITE_DELAY_MASK (0xff << ACBDLR_WRITE_DELAY_SHIFT)
>
> and then use that in the code. Similarly with other accessors.
I first agreed, but actually doing this makes it more hideous, IMHO.
These defines are rather long now (but can't be really shorter), so they
break 80 lines, also add more brackets.
Also existing accessors in this file use the same approach, so I'd
rather keep it this way for now.
>>
>> -#define DATX_IOCR_WRITE_DELAY(x) ((x) << 8)
>> -#define DATX_IOCR_READ_DELAY(x) ((x) << 0)
>> +#define DXBDLR_DQ(x) (x) /* DQ0-7 BDLR index */
>> +#define DXBDLR_DM (8) /* DM BDLR index */
>
> Can we drop the unnecessary brackets around constants?
Yup.
>> +#define DXBDLR_DQS (9) /* DQS BDLR index */
>> +#define DXBDLR_DQSN (10) /* DQSN BDLR index */
>> +
>> +#define DXBDLR_WRITE_DELAY(x) ((x) << 8)
>> +#define DXBDLR_READ_DELAY(x) ((x) << 0)
>>
>> #endif /* _SUNXI_DRAM_SUN8I_H3_H */
>> diff --git a/arch/arm/mach-sunxi/dram_sun8i_h3.c b/arch/arm/mach-sunxi/dram_sun8i_h3.c
>> index b08b8e6..3dd6803 100644
>> --- a/arch/arm/mach-sunxi/dram_sun8i_h3.c
>> +++ b/arch/arm/mach-sunxi/dram_sun8i_h3.c
>> @@ -72,21 +72,21 @@ static void mctl_dq_delay(u32 read, u32 write)
>> u32 val;
>>
>> for (i = 0; i < 4; i++) {
>> - val = DATX_IOCR_WRITE_DELAY((write >> (i * 4)) & 0xf) |
>> - DATX_IOCR_READ_DELAY(((read >> (i * 4)) & 0xf) * 2);
>> + val = DXBDLR_WRITE_DELAY((write >> (i * 4)) & 0xf) |
>> + DXBDLR_READ_DELAY(((read >> (i * 4)) & 0xf) * 2);
>>
>> - for (j = DATX_IOCR_DQ(0); j <= DATX_IOCR_DM; j++)
>> - writel(val, &mctl_ctl->datx[i].iocr[j]);
>> + for (j = DXBDLR_DQ(0); j <= DXBDLR_DM; j++)
>> + writel(val, &mctl_ctl->dx[i].bdlr[j]);
>> }
>>
>> clrbits_le32(&mctl_ctl->pgcr[0], 1 << 26);
>>
>> for (i = 0; i < 4; i++) {
>> - val = DATX_IOCR_WRITE_DELAY((write >> (16 + i * 4)) & 0xf) |
>> - DATX_IOCR_READ_DELAY((read >> (16 + i * 4)) & 0xf);
>> + val = DXBDLR_WRITE_DELAY((write >> (16 + i * 4)) & 0xf) |
>> + DXBDLR_READ_DELAY((read >> (16 + i * 4)) & 0xf);
>>
>> - writel(val, &mctl_ctl->datx[i].iocr[DATX_IOCR_DQS]);
>> - writel(val, &mctl_ctl->datx[i].iocr[DATX_IOCR_DQSN]);
>> + writel(val, &mctl_ctl->dx[i].bdlr[DXBDLR_DQS]);
>> + writel(val, &mctl_ctl->dx[i].bdlr[DXBDLR_DQSN]);
>> }
>>
>> setbits_le32(&mctl_ctl->pgcr[0], 1 << 26);
>> @@ -344,7 +344,7 @@ static int mctl_channel_init(struct dram_para *para)
>>
>> /* set dramc odt */
>> for (i = 0; i < 4; i++)
>> - clrsetbits_le32(&mctl_ctl->datx[i].gcr, (0x3 << 4) |
>> + clrsetbits_le32(&mctl_ctl->dx[i].gcr, (0x3 << 4) |
>> (0x1 << 1) | (0x3 << 2) | (0x3 << 12) |
>> (0x3 << 14),
>> IS_ENABLED(CONFIG_DRAM_ODT_EN) ? 0x0 : 0x2);
>> @@ -364,8 +364,8 @@ static int mctl_channel_init(struct dram_para *para)
>>
>> /* set half DQ */
>> if (para->bus_width != 32) {
>> - writel(0x0, &mctl_ctl->datx[2].gcr);
>> - writel(0x0, &mctl_ctl->datx[3].gcr);
>> + writel(0x0, &mctl_ctl->dx[2].gcr);
>> + writel(0x0, &mctl_ctl->dx[3].gcr);
>> }
>>
>> /* data training configuration */
>> @@ -386,17 +386,17 @@ static int mctl_channel_init(struct dram_para *para)
>> /* detect ranks and bus width */
>> if (readl(&mctl_ctl->pgsr[0]) & (0xfe << 20)) {
>> /* only one rank */
>> - if (((readl(&mctl_ctl->datx[0].gsr[0]) >> 24) & 0x2) ||
>> - ((readl(&mctl_ctl->datx[1].gsr[0]) >> 24) & 0x2)) {
>> + if (((readl(&mctl_ctl->dx[0].gsr[0]) >> 24) & 0x2) ||
>
> Looks like these fields should have #defines also.
They should, only I don't know their meaning.
Cheers,
Andre.
>> + ((readl(&mctl_ctl->dx[1].gsr[0]) >> 24) & 0x2)) {
>> clrsetbits_le32(&mctl_ctl->dtcr, 0xf << 24, 0x1 << 24);
>> para->dual_rank = 0;
>> }
>>
>> /* only half DQ width */
>> - if (((readl(&mctl_ctl->datx[2].gsr[0]) >> 24) & 0x1) ||
>> - ((readl(&mctl_ctl->datx[3].gsr[0]) >> 24) & 0x1)) {
>> - writel(0x0, &mctl_ctl->datx[2].gcr);
>> - writel(0x0, &mctl_ctl->datx[3].gcr);
>> + if (((readl(&mctl_ctl->dx[2].gsr[0]) >> 24) & 0x1) ||
>> + ((readl(&mctl_ctl->dx[3].gsr[0]) >> 24) & 0x1)) {
>> + writel(0x0, &mctl_ctl->dx[2].gcr);
>> + writel(0x0, &mctl_ctl->dx[3].gcr);
>> para->bus_width = 16;
>> }
>>
>> --
>> 2.8.2
>>
>
> Regards,
> Simon
>
More information about the U-Boot
mailing list