[PATCH 7/8] sunxi: Parameterize bit delay code in H616 DRAM driver
Jernej Škrabec
jernej.skrabec at gmail.com
Wed Jan 4 22:28:00 CET 2023
Dne sreda, 04. januar 2023 ob 01:37:47 CET je Andre Przywara napisal(a):
> On Sun, 11 Dec 2022 17:32:12 +0100
> Jernej Skrabec <jernej.skrabec at gmail.com> wrote:
>
> Hi Jernej,
>
> > These values are highly board specific and thus make sense to add
> > parameter for them. To ease adding support for new boards, let's make
> > them same as in vendor DRAM settings.
>
> So scrolling up and down: does this patch miss the TPR11 and TPR12
> values in the OPi-Zero2 defconfig?
No, because 0 (which is default) is correct here.
> And should we not default to 0 in
> Kconfig to help spotting this omission more easily for new boards?
Not all boards need to set all the values. I set default values for symbols
which seem to have same value for multiple boards.
> If I pieced the bits together correctly, we end up with the same values
> in the register with TPR11=0xfffedddb and TPR12=0xeddca998, and ODT_EN
> being irrelevant.
>
> > Signed-off-by: Jernej Skrabec <jernej.skrabec at gmail.com>
> > ---
> >
> > .../include/asm/arch-sunxi/dram_sun50i_h616.h | 4 +
> > arch/arm/mach-sunxi/Kconfig | 18 ++
> > arch/arm/mach-sunxi/dram_sun50i_h616.c | 189 +++++++++++++-----
> > 3 files changed, 162 insertions(+), 49 deletions(-)
> >
> > diff --git a/arch/arm/include/asm/arch-sunxi/dram_sun50i_h616.h
> > b/arch/arm/include/asm/arch-sunxi/dram_sun50i_h616.h index
> > b5140c79b70e..c7890c83391f 100644
> > --- a/arch/arm/include/asm/arch-sunxi/dram_sun50i_h616.h
> > +++ b/arch/arm/include/asm/arch-sunxi/dram_sun50i_h616.h
> > @@ -145,6 +145,7 @@ check_member(sunxi_mctl_ctl_reg, unk_0x4240, 0x4240);
> >
> > #define TPR10_READ_CALIBRATION BIT(21)
> > #define TPR10_READ_TRAINING BIT(22)
> > #define TPR10_WRITE_TRAINING BIT(23)
> >
> > +#define TPR10_UNKNOWN_FEAT3 BIT(30)
>
> As mentioned in the other patch: if we don't know the meaning of this
> bit, I'd prefer using BIT(30) directly, or at least encode BIT30
> in the name.
>
> > struct dram_para {
> >
> > u32 clk;
> >
> > @@ -156,7 +157,10 @@ struct dram_para {
> >
> > u32 dx_odt;
> > u32 dx_dri;
> > u32 ca_dri;
> >
> > + u32 odt_en;
> >
> > u32 tpr10;
> >
> > + u32 tpr11;
> > + u32 tpr12;
> >
> > };
> >
> > diff --git a/arch/arm/mach-sunxi/Kconfig b/arch/arm/mach-sunxi/Kconfig
> > index 778304b77e26..b050f0a56971 100644
> > --- a/arch/arm/mach-sunxi/Kconfig
> > +++ b/arch/arm/mach-sunxi/Kconfig
> > @@ -67,11 +67,29 @@ config DRAM_SUN50I_H616_CA_DRI
> >
> > help
> >
> > CA DRI value from vendor DRAM settings.
> >
> > +config DRAM_SUN50I_H616_ODT_EN
> > + hex "H616 DRAM ODT EN parameter"
> > + default 0x1
> > + help
> > + ODT EN value from vendor DRAM settings.
> > +
> >
> > config DRAM_SUN50I_H616_TPR10
> >
> > hex "H616 DRAM TPR10 parameter"
> > help
> >
> > TPR10 value from vendor DRAM settings. It tells which features
> > should be configured, like write leveling, read calibration,
etc.
> >
> > +
> > +config DRAM_SUN50I_H616_TPR11
> > + hex "H616 DRAM TPR11 parameter"
> > + default 0x0
> > + help
> > + TPR11 value from vendor DRAM settings.
> > +
> > +config DRAM_SUN50I_H616_TPR12
> > + hex "H616 DRAM TPR12 parameter"
> > + default 0x0
> > + help
> > + TPR12 value from vendor DRAM settings.
> >
> > endif
> >
> > config SUN6I_PRCM
> >
> > diff --git a/arch/arm/mach-sunxi/dram_sun50i_h616.c
> > b/arch/arm/mach-sunxi/dram_sun50i_h616.c index 3b2ba168498c..df06cea42464
> > 100644
> > --- a/arch/arm/mach-sunxi/dram_sun50i_h616.c
> > +++ b/arch/arm/mach-sunxi/dram_sun50i_h616.c
> > @@ -574,7 +574,7 @@ static bool mctl_phy_write_training(struct dram_para
> > *para)>
> > static void mctl_phy_bit_delay_compensation(struct dram_para *para)
> > {
> >
> > - u32 *ptr;
> > + u32 *ptr, val;
> >
> > int i;
> >
> > if (para->tpr10 & TPR10_UNKNOWN_FEAT2) {
> >
> > @@ -582,49 +582,93 @@ static void mctl_phy_bit_delay_compensation(struct
> > dram_para *para)>
> > setbits_le32(SUNXI_DRAM_PHY0_BASE + 8, 8);
> > clrbits_le32(SUNXI_DRAM_PHY0_BASE + 0x190, 0x10);
> >
> > + if (para->tpr10 & TPR10_UNKNOWN_FEAT3)
> > + val = para->tpr11 & 0x3f;
> > + else
> > + val = (para->tpr11 & 0xf) << 1;
> > +
> >
> > ptr = (u32 *)(SUNXI_DRAM_PHY0_BASE + 0x484);
> > for (i = 0; i < 9; i++) {
> >
> > - writel_relaxed(0x16, ptr);
> > - writel_relaxed(0x16, ptr + 0x30);
> > + writel_relaxed(val, ptr);
> > + writel_relaxed(val, ptr + 0x30);
> >
> > ptr += 2;
> >
> > }
> >
> > - writel_relaxed(0x1c, SUNXI_DRAM_PHY0_BASE + 0x4d0);
> > - writel_relaxed(0x1c, SUNXI_DRAM_PHY0_BASE + 0x590);
> > - writel_relaxed(0x1c, SUNXI_DRAM_PHY0_BASE + 0x4cc);
> > - writel_relaxed(0x1c, SUNXI_DRAM_PHY0_BASE + 0x58c);
> > +
> > + if (para->tpr10 & TPR10_UNKNOWN_FEAT3)
> > + val = (para->odt_en >> 15) & 0x1e;
>
> So I guess odt_en stands for "ODT enable". Looking at the D1 DRAM
> driver, they have a boot0 parameter odt_en which is either 0x0 or 0x1,
> so it looks like a boolean value. This seems to be also the case here?
Nope. While I only ever see odt_en being 1, based on vendor code it can have
additional bits set, as it can be seen from above code.
> In the D1 driver, this seems to gate the ZQ value being used, which
> provides the actual timing values.
> So is TPR10_UNKNOWN_FEAT3 actually this ODT_EN switch, and the variable
> containing the timing bits should be zq_value or CONFIG_DRAM_ZQ?
No idea.
>
> > + else
> > + val = (para->tpr11 >> 15) & 0x1e;
>
> So I am wondering if it's less confusing to mask first, then shift?
> Granted, this here is shift from [19:16] into [5:1], so it's not easy
> either way, but I think it's easier to match the mask with the provided
> TPR11 value.
I did it this way because mask is much smaller and easier to read, but neither
are straightforward to understand. I guess some macro could be crafted for
such purpose.
>
> > +
> > + writel_relaxed(val, SUNXI_DRAM_PHY0_BASE + 0x4d0);
> > + writel_relaxed(val, SUNXI_DRAM_PHY0_BASE + 0x590);
> > + writel_relaxed(val, SUNXI_DRAM_PHY0_BASE + 0x4cc);
> > + writel_relaxed(val, SUNXI_DRAM_PHY0_BASE + 0x58c);
> > +
> > + if (para->tpr10 & TPR10_UNKNOWN_FEAT3)
> > + val = (para->tpr11 >> 8) & 0x3f;
> > + else
> > + val = (para->tpr11 >> 3) & 0x1e;
> >
> > ptr = (u32 *)(SUNXI_DRAM_PHY0_BASE + 0x4d8);
> > for (i = 0; i < 9; i++) {
> >
> > - writel_relaxed(0x1a, ptr);
> > - writel_relaxed(0x1a, ptr + 0x30);
> > + writel_relaxed(val, ptr);
> > + writel_relaxed(val, ptr + 0x30);
> >
> > ptr += 2;
> >
> > }
> >
> > - writel_relaxed(0x1e, SUNXI_DRAM_PHY0_BASE + 0x524);
> > - writel_relaxed(0x1e, SUNXI_DRAM_PHY0_BASE + 0x5e4);
> > - writel_relaxed(0x1e, SUNXI_DRAM_PHY0_BASE + 0x520);
> > - writel_relaxed(0x1e, SUNXI_DRAM_PHY0_BASE + 0x5e0);
> > +
> > + if (para->tpr10 & TPR10_UNKNOWN_FEAT3)
> > + val = (para->odt_en >> 19) & 0x1e;
>
> Is this really odt_en as a base, while *most* of the other bits use
> TPR11 as well? Just asking, since it doesn't fit into my naive pattern
> matching ;-)
odt_en is name from device tree. It doesn't seem to match actual purpose, but
there is not much we can do without documentation.
Best regards,
Jernej
>
> The rest looks correct (minus the repeat of the above mentioned
> issues), I compared the register offsets between - and +.
>
> Cheers,
> Andre
>
> > + else
> > + val = (para->tpr11 >> 19) & 0x1e;
> > +
> > + writel_relaxed(val, SUNXI_DRAM_PHY0_BASE + 0x524);
> > + writel_relaxed(val, SUNXI_DRAM_PHY0_BASE + 0x5e4);
> > + writel_relaxed(val, SUNXI_DRAM_PHY0_BASE + 0x520);
> > + writel_relaxed(val, SUNXI_DRAM_PHY0_BASE + 0x5e0);
> > +
> > + if (para->tpr10 & TPR10_UNKNOWN_FEAT3)
> > + val = (para->tpr11 >> 16) & 0x3f;
> > + else
> > + val = (para->tpr11 >> 7) & 0x1e;
> >
> > ptr = (u32 *)(SUNXI_DRAM_PHY0_BASE + 0x604);
> > for (i = 0; i < 9; i++) {
> >
> > - writel_relaxed(0x1a, ptr);
> > - writel_relaxed(0x1a, ptr + 0x30);
> > + writel_relaxed(val, ptr);
> > + writel_relaxed(val, ptr + 0x30);
> >
> > ptr += 2;
> >
> > }
> >
> > - writel_relaxed(0x1e, SUNXI_DRAM_PHY0_BASE + 0x650);
> > - writel_relaxed(0x1e, SUNXI_DRAM_PHY0_BASE + 0x710);
> > - writel_relaxed(0x1e, SUNXI_DRAM_PHY0_BASE + 0x64c);
> > - writel_relaxed(0x1e, SUNXI_DRAM_PHY0_BASE + 0x70c);
> > +
> > + if (para->tpr10 & TPR10_UNKNOWN_FEAT3)
> > + val = (para->odt_en >> 23) & 0x1e;
> > + else
> > + val = (para->tpr11 >> 23) & 0x1e;
> > +
> > + writel_relaxed(val, SUNXI_DRAM_PHY0_BASE + 0x650);
> > + writel_relaxed(val, SUNXI_DRAM_PHY0_BASE + 0x710);
> > + writel_relaxed(val, SUNXI_DRAM_PHY0_BASE + 0x64c);
> > + writel_relaxed(val, SUNXI_DRAM_PHY0_BASE + 0x70c);
> > +
> > + if (para->tpr10 & TPR10_UNKNOWN_FEAT3)
> > + val = (para->tpr11 >> 24) & 0x3f;
> > + else
> > + val = (para->tpr11 >> 11) & 0x1e;
> >
> > ptr = (u32 *)(SUNXI_DRAM_PHY0_BASE + 0x658);
> > for (i = 0; i < 9; i++) {
> >
> > - writel_relaxed(0x1a, ptr);
> > - writel_relaxed(0x1a, ptr + 0x30);
> > + writel_relaxed(val, ptr);
> > + writel_relaxed(val, ptr + 0x30);
> >
> > ptr += 2;
> >
> > }
> >
> > - writel_relaxed(0x1e, SUNXI_DRAM_PHY0_BASE + 0x6a4);
> > - writel_relaxed(0x1e, SUNXI_DRAM_PHY0_BASE + 0x764);
> > - writel_relaxed(0x1e, SUNXI_DRAM_PHY0_BASE + 0x6a0);
> > - writel_relaxed(0x1e, SUNXI_DRAM_PHY0_BASE + 0x760);
> > +
> > + if (para->tpr10 & TPR10_UNKNOWN_FEAT3)
> > + val = (para->odt_en >> 27) & 0x1e;
> > + else
> > + val = (para->tpr11 >> 27) & 0x1e;
> > +
> > + writel_relaxed(val, SUNXI_DRAM_PHY0_BASE + 0x6a4);
> > + writel_relaxed(val, SUNXI_DRAM_PHY0_BASE + 0x764);
> > + writel_relaxed(val, SUNXI_DRAM_PHY0_BASE + 0x6a0);
> > + writel_relaxed(val, SUNXI_DRAM_PHY0_BASE + 0x760);
> >
> > dmb();
> >
> > @@ -635,49 +679,93 @@ static void mctl_phy_bit_delay_compensation(struct
> > dram_para *para)>
> > clrbits_le32(SUNXI_DRAM_PHY0_BASE + 0x54, 0x80);
> > clrbits_le32(SUNXI_DRAM_PHY0_BASE + 0x190, 4);
> >
> > + if (para->tpr10 & TPR10_UNKNOWN_FEAT3)
> > + val = para->tpr12 & 0x3f;
> > + else
> > + val = (para->tpr12 & 0xf) << 1;
> > +
> >
> > ptr = (u32 *)(SUNXI_DRAM_PHY0_BASE + 0x480);
> > for (i = 0; i < 9; i++) {
> >
> > - writel_relaxed(0x10, ptr);
> > - writel_relaxed(0x10, ptr + 0x30);
> > + writel_relaxed(val, ptr);
> > + writel_relaxed(val, ptr + 0x30);
> >
> > ptr += 2;
> >
> > }
> >
> > - writel_relaxed(0x18, SUNXI_DRAM_PHY0_BASE + 0x528);
> > - writel_relaxed(0x18, SUNXI_DRAM_PHY0_BASE + 0x5e8);
> > - writel_relaxed(0x18, SUNXI_DRAM_PHY0_BASE + 0x4c8);
> > - writel_relaxed(0x18, SUNXI_DRAM_PHY0_BASE + 0x588);
> > +
> > + if (para->tpr10 & TPR10_UNKNOWN_FEAT3)
> > + val = (para->odt_en << 1) & 0x1e;
> > + else
> > + val = (para->tpr12 >> 15) & 0x1e;
> > +
> > + writel_relaxed(val, SUNXI_DRAM_PHY0_BASE + 0x528);
> > + writel_relaxed(val, SUNXI_DRAM_PHY0_BASE + 0x5e8);
> > + writel_relaxed(val, SUNXI_DRAM_PHY0_BASE + 0x4c8);
> > + writel_relaxed(val, SUNXI_DRAM_PHY0_BASE + 0x588);
> > +
> > + if (para->tpr10 & TPR10_UNKNOWN_FEAT3)
> > + val = (para->tpr12 >> 8) & 0x3f;
> > + else
> > + val = (para->tpr12 >> 3) & 0x1e;
> >
> > ptr = (u32 *)(SUNXI_DRAM_PHY0_BASE + 0x4d4);
> > for (i = 0; i < 9; i++) {
> >
> > - writel_relaxed(0x12, ptr);
> > - writel_relaxed(0x12, ptr + 0x30);
> > + writel_relaxed(val, ptr);
> > + writel_relaxed(val, ptr + 0x30);
> >
> > ptr += 2;
> >
> > }
> >
> > - writel_relaxed(0x1a, SUNXI_DRAM_PHY0_BASE + 0x52c);
> > - writel_relaxed(0x1a, SUNXI_DRAM_PHY0_BASE + 0x5ec);
> > - writel_relaxed(0x1a, SUNXI_DRAM_PHY0_BASE + 0x51c);
> > - writel_relaxed(0x1a, SUNXI_DRAM_PHY0_BASE + 0x5dc);
> > +
> > + if (para->tpr10 & TPR10_UNKNOWN_FEAT3)
> > + val = (para->odt_en >> 3) & 0x1e;
> > + else
> > + val = (para->tpr12 >> 19) & 0x1e;
> > +
> > + writel_relaxed(val, SUNXI_DRAM_PHY0_BASE + 0x52c);
> > + writel_relaxed(val, SUNXI_DRAM_PHY0_BASE + 0x5ec);
> > + writel_relaxed(val, SUNXI_DRAM_PHY0_BASE + 0x51c);
> > + writel_relaxed(val, SUNXI_DRAM_PHY0_BASE + 0x5dc);
> > +
> > + if (para->tpr10 & TPR10_UNKNOWN_FEAT3)
> > + val = (para->tpr12 >> 16) & 0x3f;
> > + else
> > + val = (para->tpr12 >> 7) & 0x1e;
> >
> > ptr = (u32 *)(SUNXI_DRAM_PHY0_BASE + 0x600);
> > for (i = 0; i < 9; i++) {
> >
> > - writel_relaxed(0x12, ptr);
> > - writel_relaxed(0x12, ptr + 0x30);
> > + writel_relaxed(val, ptr);
> > + writel_relaxed(val, ptr + 0x30);
> >
> > ptr += 2;
> >
> > }
> >
> > - writel_relaxed(0x1a, SUNXI_DRAM_PHY0_BASE + 0x6a8);
> > - writel_relaxed(0x1a, SUNXI_DRAM_PHY0_BASE + 0x768);
> > - writel_relaxed(0x1a, SUNXI_DRAM_PHY0_BASE + 0x648);
> > - writel_relaxed(0x1a, SUNXI_DRAM_PHY0_BASE + 0x708);
> > +
> > + if (para->tpr10 & TPR10_UNKNOWN_FEAT3)
> > + val = (para->odt_en >> 7) & 0x1e;
> > + else
> > + val = (para->tpr12 >> 23) & 0x1e;
> > +
> > + writel_relaxed(val, SUNXI_DRAM_PHY0_BASE + 0x6a8);
> > + writel_relaxed(val, SUNXI_DRAM_PHY0_BASE + 0x768);
> > + writel_relaxed(val, SUNXI_DRAM_PHY0_BASE + 0x648);
> > + writel_relaxed(val, SUNXI_DRAM_PHY0_BASE + 0x708);
> > +
> > + if (para->tpr10 & TPR10_UNKNOWN_FEAT3)
> > + val = (para->tpr12 >> 24) & 0x3f;
> > + else
> > + val = (para->tpr12 >> 11) & 0x1e;
> >
> > ptr = (u32 *)(SUNXI_DRAM_PHY0_BASE + 0x654);
> > for (i = 0; i < 9; i++) {
> >
> > - writel_relaxed(0x14, ptr);
> > - writel_relaxed(0x14, ptr + 0x30);
> > + writel_relaxed(val, ptr);
> > + writel_relaxed(val, ptr + 0x30);
> >
> > ptr += 2;
> >
> > }
> >
> > - writel_relaxed(0x1c, SUNXI_DRAM_PHY0_BASE + 0x6ac);
> > - writel_relaxed(0x1c, SUNXI_DRAM_PHY0_BASE + 0x76c);
> > - writel_relaxed(0x1c, SUNXI_DRAM_PHY0_BASE + 0x69c);
> > - writel_relaxed(0x1c, SUNXI_DRAM_PHY0_BASE + 0x75c);
> > +
> > + if (para->tpr10 & TPR10_UNKNOWN_FEAT3)
> > + val = (para->odt_en >> 11) & 0x1e;
> > + else
> > + val = (para->tpr12 >> 27) & 0x1e;
> > +
> > + writel_relaxed(val, SUNXI_DRAM_PHY0_BASE + 0x6ac);
> > + writel_relaxed(val, SUNXI_DRAM_PHY0_BASE + 0x76c);
> > + writel_relaxed(val, SUNXI_DRAM_PHY0_BASE + 0x69c);
> > + writel_relaxed(val, SUNXI_DRAM_PHY0_BASE + 0x75c);
> >
> > dmb();
> >
> > @@ -1021,7 +1109,10 @@ unsigned long sunxi_dram_init(void)
> >
> > .dx_odt = CONFIG_DRAM_SUN50I_H616_DX_ODT,
> > .dx_dri = CONFIG_DRAM_SUN50I_H616_DX_DRI,
> > .ca_dri = CONFIG_DRAM_SUN50I_H616_CA_DRI,
> >
> > + .odt_en = CONFIG_DRAM_SUN50I_H616_ODT_EN,
> >
> > .tpr10 = CONFIG_DRAM_SUN50I_H616_TPR10,
> >
> > + .tpr11 = CONFIG_DRAM_SUN50I_H616_TPR11,
> > + .tpr12 = CONFIG_DRAM_SUN50I_H616_TPR12,
> >
> > };
> > unsigned long size;
More information about the U-Boot
mailing list