Re: [PATCH 5/8] sunxi: Always configure ODT on H616 DRAM

Jernej Škrabec
2023-01-04T21:12:10Z

Dne sreda, 04. januar 2023 ob 01:37:17 CET je Andre Przywara napisal(a):
> On Sun, 11 Dec 2022 17:32:10 +0100
>
> Jernej Skrabec <jernej.skrabec@gmail.com> wrote:
> > Vendor H616 DRAM code always configure part which we call ODT
> > configuration. Let's reflect that here too.
>
> I wonder if we need this patch at all. "depends on !H616" looks
> counter-intuitive, since this suggests it's always off.

I don't see it that way. It just means it's unused for H616.

> As it stands, it doesn't hurt. "default y" does the right thing, and if
> people want to shoot themselves in the foot: fine by me.
>
> At least I would like to keep the Kconfig part. We could change the
> condition in the code into an explaining comment, if you still want to
> force this on.
>
> And coming back from patch 7/8: how does this correspond to
> DRAM_SUN50I_H616_ODT_EN?

The thing is that I can't give you good explanation for anything above due to
nature of reverse engineering. It's just how vendor code is done and I would
argue that my original assumption when I was writing this driver was wrong and
I shouldn't use this symbol at all in first place. I'm not even sure if I named
mctl_phy_configure_odt() completely correct. It has to do something with ODT,
but I don't know if it gets enabled here or not. To me, it looks more like
that just some parameters are set here.

Best regards,
Jernej

>
> Cheers,
> Andre
>
> > Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> > ---
> >
> > arch/arm/mach-sunxi/Kconfig | 2 +-
> > arch/arm/mach-sunxi/dram_sun50i_h616.c | 3 +--
> > 2 files changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/arm/mach-sunxi/Kconfig b/arch/arm/mach-sunxi/Kconfig
> > index abcbd0fb9061..778304b77e26 100644
> > --- a/arch/arm/mach-sunxi/Kconfig
> > +++ b/arch/arm/mach-sunxi/Kconfig
> > @@ -488,12 +488,12 @@ config DRAM_ZQ
> >
> > config DRAM_ODT_EN
> >
> > bool "sunxi dram odt enable"
> >
> > + depends on !MACH_SUN50I_H616
> >
> > default y if MACH_SUN8I_A23
> > default y if MACH_SUNXI_H3_H5
> > default y if MACH_SUN8I_R40
> > default y if MACH_SUN50I
> > default y if MACH_SUN50I_H6
> >
> > - default y if MACH_SUN50I_H616
> >
> > ---help---
> > Select this to enable dram odt (on die termination).
> >
> > diff --git a/arch/arm/mach-sunxi/dram_sun50i_h616.c
> > b/arch/arm/mach-sunxi/dram_sun50i_h616.c index 14a01a3c4e54..bf5b4ddfb5c2
> > 100644
> > --- a/arch/arm/mach-sunxi/dram_sun50i_h616.c
> > +++ b/arch/arm/mach-sunxi/dram_sun50i_h616.c
> > @@ -736,8 +736,7 @@ static bool mctl_phy_init(struct dram_para *para)
> >
> > writel(0x80, SUNXI_DRAM_PHY0_BASE + 0x3dc);
> > writel(0x80, SUNXI_DRAM_PHY0_BASE + 0x45c);
> >
> > - if (IS_ENABLED(CONFIG_DRAM_ODT_EN))
> > - mctl_phy_configure_odt(para);
> > + mctl_phy_configure_odt(para);
> >
> > clrsetbits_le32(SUNXI_DRAM_PHY0_BASE + 4, 7, 0xa);