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

[Simon Glass]

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?

[..]

> +       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