Hi Ryder, On 25 October 2018 at 03:38, Ryder Lee <ryder....@mediatek.com> wrote: > 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.
OK we can leave it. > >> [..] >> >> > + 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. We often end up worrying about delays later so any comments about how you came to those values are useful. > >> [..] >> >> > +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 . OK > > Also I don't understand why we cannot use clk_set_defaults() in > pre-relocate state? I don't know either. It should be possible. Regards, Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot