[U-Boot] [linux-sunxi] Re: [RFC PATCH 2/6] sunxi: Rename bus-width related macros in H3 DRAM code

[Icenowy Zheng]

12.02.2017, 01:00, "Jens Kuske" <jenskuske at gmail.com>:
> Hi,
>
> renaming is not quite enough, see the comments below.
>
> On 11.02.2017 16:08, Icenowy Zheng wrote:
>>  The DesignWare DRAM controller used by H3 and newer SoCs use a bit to
>>  identify whether the DRAM is half-width.
>>
>>  As H3 itself come with 32-bit DRAM, the two modes of the bit used to be
>>  named "MCTL_CR_32BIT" and "MCTL_CR_16BIT", but for SoCs with 16-bit DRAM
>>  they're really 8-bit and 16-bit.
>>
>>  Rename the bit's macro, and also rename the variable name in
>>  dram_sun8i_h3.c.
>>
>>  Signed-off-by: Icenowy Zheng <icenowy at aosc.xyz>
>>  ---
>>   arch/arm/include/asm/arch-sunxi/dram_sun8i_h3.h | 6 +++---
>>   arch/arm/mach-sunxi/dram_sunxi_dw.c | 11 ++++++-----
>>   2 files changed, 9 insertions(+), 8 deletions(-)
>>
>>  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 25d07d9863..48bd6f7c0f 100644
>>  --- a/arch/arm/include/asm/arch-sunxi/dram_sun8i_h3.h
>>  +++ b/arch/arm/include/asm/arch-sunxi/dram_sun8i_h3.h
>>  @@ -52,9 +52,9 @@ struct sunxi_mctl_com_reg {
>>   #define MCTL_CR_SEQUENTIAL (0x1 << 15)
>>   #define MCTL_CR_INTERLEAVED (0x0 << 15)
>>
>>  -#define MCTL_CR_32BIT (0x1 << 12)
>>  -#define MCTL_CR_16BIT (0x0 << 12)
>>  -#define MCTL_CR_BUS_WIDTH(x) ((x) == 32 ? MCTL_CR_32BIT : MCTL_CR_16BIT)
>>  +#define MCTL_CR_FULL_WIDTH (0x1 << 12)
>>  +#define MCTL_CR_HALF_WIDTH (0x0 << 12)
>>  +#define MCTL_CR_BUS_FULL_WIDTH(x) ((x) << 12)
>>
>>   #define MCTL_CR_PAGE_SIZE(x) ((fls(x) - 4) << 8)
>>   #define MCTL_CR_ROW_BITS(x) (((x) - 1) << 4)
>>  diff --git a/arch/arm/mach-sunxi/dram_sunxi_dw.c b/arch/arm/mach-sunxi/dram_sunxi_dw.c
>>  index 9f7cc7fd4c..0c73a43075 100644
>>  --- a/arch/arm/mach-sunxi/dram_sunxi_dw.c
>>  +++ b/arch/arm/mach-sunxi/dram_sunxi_dw.c
>>  @@ -28,7 +28,7 @@
>>   #define LINES_PER_BYTE_LANE (BITS_PER_BYTE + 3)
>>   struct dram_para {
>>           u16 page_size;
>>  - u8 bus_width;
>>  + u8 bus_full_width;
>>           u8 dual_rank;
>>           u8 row_bits;
>>           const u8 dx_read_delays[NR_OF_BYTE_LANES][LINES_PER_BYTE_LANE];
>>  @@ -358,7 +358,8 @@ static void mctl_set_cr(struct dram_para *para)
>>                           (struct sunxi_mctl_com_reg *)SUNXI_DRAM_COM_BASE;
>>
>>           writel(MCTL_CR_BL8 | MCTL_CR_2T | MCTL_CR_DDR3 | MCTL_CR_INTERLEAVED |
>>  - MCTL_CR_EIGHT_BANKS | MCTL_CR_BUS_WIDTH(para->bus_width) |
>>  + MCTL_CR_EIGHT_BANKS |
>>  + MCTL_CR_BUS_FULL_WIDTH(para->bus_full_width) |
>>                  (para->dual_rank ? MCTL_CR_DUAL_RANK : MCTL_CR_SINGLE_RANK) |
>>                  MCTL_CR_PAGE_SIZE(para->page_size) |
>>                  MCTL_CR_ROW_BITS(para->row_bits), &mctl_com->cr);
>>  @@ -471,7 +472,7 @@ static int mctl_channel_init(uint16_t socid, struct dram_para *para)
>>           }
>>
>>           /* set half DQ */
>>  - if (para->bus_width != 32) {
>>  + if (!para->bus_full_width) {
>>                   writel(0x0, &mctl_ctl->dx[2].gcr);
>>                   writel(0x0, &mctl_ctl->dx[3].gcr);
>
> This is not correct, it still disables byte 2 and 3, which don't even
> exist on 16bit bus devices. On a 16bit device dx[1] would have do be
> disabled for half width.

Yes, verified on dram_sun8i_a33.c .

>
>>           }
>>  @@ -509,7 +510,7 @@ static int mctl_channel_init(uint16_t socid, struct dram_para *para)
>>                       ((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;
>>  + para->bus_full_width = 0;
>
> Same here, it only detects that byte 2 and 3 are missing, to detect half
> width on 16bit devices byte 1 would have to be checked in the if above.
> Also, the rank detection above must not check byte 1 on 16bit devices.
>
>>                   }
>>
>>                   mctl_set_cr(para);
>>  @@ -613,7 +614,7 @@ unsigned long sunxi_dram_init(void)
>>
>>           struct dram_para para = {
>>                   .dual_rank = 0,
>>  - .bus_width = 32,
>>  + .bus_full_width = 1,
>>                   .row_bits = 15,
>>                   .page_size = 4096,
>
> --
> You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe at googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.