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

[Simon Glass]

Hi Ryder,

On 25 October 2018 at 03:38, Ryder Lee <ryder.lee at 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.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.

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