On Wed, 2018-10-24 at 21:30 -0600, Simon Glass wrote:
> On 12 October 2018 at 01:01, Ryder Lee <ryder....@mediatek.com> wrote:
> > This patch adds a DDR3 driver for MT7629 SoC.
> >
> > Signed-off-by: Wu Zou <wu....@mediatek.com>
> > Signed-off-by: Ryder Lee <ryder....@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 <s...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
