[U-Boot] [PATCH v2 15/18] ram: MediaTek: add DDR3 driver for MT7629 SoC

[Ryder Lee]

On Wed, 2018-10-24 at 21:30 -0600, Simon Glass wrote:
> On 12 October 2018 at 01:01, Ryder Lee <ryder.lee at mediatek.com> wrote:
> > This patch adds a DDR3 driver for MT7629 SoC.
> >
> > Signed-off-by: Wu Zou <wu.zou at mediatek.com>
> > Signed-off-by: Ryder Lee <ryder.lee at mediatek.com>
> > ---
> >  drivers/ram/Makefile               |   1 +
> >  drivers/ram/mediatek/Makefile      |   7 +
> >  drivers/ram/mediatek/ddr3-mt7629.c | 766 +++++++++++++++++++++++++++++++++++++
> >  3 files changed, 774 insertions(+)
> >  create mode 100644 drivers/ram/mediatek/Makefile
> >  create mode 100644 drivers/ram/mediatek/ddr3-mt7629.c
> 
> Reviewed-by: Simon Glass <sjg at chromium.org>
> 
> Thoughts below.
> 
> [..]
> 
> > +#define DDRPHY_PLL1                    0x0000
> > +#define DDRPHY_PLL2                    0x0004
> 
> Why not use a C struct for these registers?

These are copy-n-paste from our SDK. I've considered converting these
into C struct but it will take me a while. Just let it be.

But, I could do that if you insist.

> [..]
> 
> > +       writel(0x400, priv->dramc_ao + DRAMC_MRS);
> > +       writel(0x1d7000, priv->dramc_ao + DRAMC_MRS);
> > +       writel(0x1, priv->dramc_ao + DRAMC_SPCMD);
> > +       writel(0x0, priv->dramc_ao + DRAMC_SPCMD);
> > +       udelay(100);
> 
> Are these delays specified in a datasheet? Why did you chose 100?
> Perhaps add an enum for this value? Is there a way to check for when
> the hardware is ready, e.g. by reading a registers in a loop?

Rule of thumb. There is no registers to read here, but I can try to
reduce the delay time.

> [..]
> 
> > +static int mtk_ddr3_probe(struct udevice *dev)
> > +{
> > +       struct mtk_ddr3_priv *priv = dev_get_priv(dev);
> > +
> > +       priv->emi = dev_read_addr_index(dev, 0);
> > +       if (priv->emi == FDT_ADDR_T_NONE)
> > +               return -EINVAL;
> > +
> > +       priv->ddrphy = dev_read_addr_index(dev, 1);
> > +       if (priv->ddrphy == FDT_ADDR_T_NONE)
> > +               return -EINVAL;
> > +
> > +       priv->dramc_ao = dev_read_addr_index(dev, 2);
> > +       if (priv->dramc_ao == FDT_ADDR_T_NONE)
> > +               return -EINVAL;
> > +
> > +#ifdef CONFIG_SPL_BUILD
> > +       int ret;
> > +
> > +       ret = clk_get_by_index(dev, 0, &priv->phy);
> > +       if (ret)
> > +               return ret;
> > +
> > +       ret = clk_get_by_index(dev, 1, &priv->phy_mux);
> > +       if (ret)
> > +               return ret;
> > +
> > +       ret = clk_get_by_index(dev, 2, &priv->mem);
> > +       if (ret)
> > +               return ret;
> > +
> > +       ret = clk_get_by_index(dev, 3, &priv->mem_mux);
> > +       if (ret)
> > +               return ret;
> 
> Do you have phandles for these clocks? I only worry that it is a bit
> brittle to have them numbered.

Yes. I choose clk_get_by_index() here as I stripped the 'clock-names'
via CONFIG_OF_SPL_REMOVE_PROPS to reduce the size .

Also I don't understand why we cannot use clk_set_defaults() in
pre-relocate state?


Ryder

> > +
> > +       ret = mtk_ddr3_init(dev);
> > +       if (ret)
> > +               return ret;
> > +#endif
> > +       return 0;
> > +}
> > +
> 
> Regards,
> Simon