Hi Ryder,
On 25 October 2018 at 03:38, Ryder Lee ryder.lee@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@mediatek.com wrote:
This patch adds a DDR3 driver for MT7629 SoC.
Signed-off-by: Wu Zou wu.zou@mediatek.com Signed-off-by: Ryder Lee ryder.lee@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@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