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? [..] > + 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? [..] > +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. > + > + 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