[U-Boot] [PATCH v3 19/58] clk: sunxi: Implement direct MMC clocks

[Maxime Ripard]

On Mon, Aug 27, 2018 at 03:34:20PM +0530, Jagan Teki wrote:
> >> diff --git a/drivers/clk/sunxi/clk_a10.c b/drivers/clk/sunxi/clk_a10.c
> >> index fb11231dd1..55176bc174 100644
> >> --- a/drivers/clk/sunxi/clk_a10.c
> >> +++ b/drivers/clk/sunxi/clk_a10.c
> >> @@ -23,6 +23,11 @@ static struct ccu_clk_map a10_clks[] = {
> >>       [CLK_AHB_MMC2]          = { 0x060, BIT(10), NULL },
> >>       [CLK_AHB_MMC3]          = { 0x060, BIT(11), NULL },
> >>
> >> +     [CLK_MMC0]              = { 0x088, BIT(31), &mmc_clk_set_rate },
> >> +     [CLK_MMC1]              = { 0x08c, BIT(31), &mmc_clk_set_rate },
> >> +     [CLK_MMC2]              = { 0x090, BIT(31), &mmc_clk_set_rate },
> >> +     [CLK_MMC3]              = { 0x094, BIT(31), &mmc_clk_set_rate },
> >> +
> >>       [CLK_USB_OHCI0]         = { 0x0cc, BIT(6), NULL },
> >>       [CLK_USB_OHCI1]         = { 0x0cc, BIT(7), NULL },
> >>       [CLK_USB_PHY]           = { 0x0cc, BIT(8), NULL },
> >> diff --git a/drivers/clk/sunxi/clk_a10s.c b/drivers/clk/sunxi/clk_a10s.c
> >> index bc4ae7352b..fbac0ad751 100644
> >> --- a/drivers/clk/sunxi/clk_a10s.c
> >> +++ b/drivers/clk/sunxi/clk_a10s.c
> >> @@ -20,6 +20,12 @@ static struct ccu_clk_map a10s_clks[] = {
> >>       [CLK_AHB_MMC1]          = { 0x060, BIT(9), NULL },
> >>       [CLK_AHB_MMC2]          = { 0x060, BIT(10), NULL },
> >>
> >> +#ifdef CONFIG_MMC
> >> +     [CLK_MMC0]              = { 0x088, BIT(31), &mmc_clk_set_rate },
> >> +     [CLK_MMC1]              = { 0x08c, BIT(31), &mmc_clk_set_rate },
> >> +     [CLK_MMC2]              = { 0x090, BIT(31), &mmc_clk_set_rate },
> >> +#endif
> >> +
> >
> > I'm not too sure about the ifdef here. Or at least, we should be
> > consistent, and if we do it for the MMC, we should do it for all the
> > SoCs (including the A10), and for all the controllers (including USB,
> > for example).
> 
> because few of sun5i boards not using MMC, example CHIP and CHIP_pro
> otherwise we need to use intermediate wrapper to call
> mmc_clk_set_rate.

Well, yes, but you can make the same argument for other SoCs, and
other features. So really, I think this is a good idea, but if we
remain consistent between SoCs and features.

> >
> >> diff --git a/drivers/mmc/sunxi_mmc.c b/drivers/mmc/sunxi_mmc.c
> >> index 39f15eb423..bf82014a64 100644
> >> --- a/drivers/mmc/sunxi_mmc.c
> >> +++ b/drivers/mmc/sunxi_mmc.c
> >> @@ -13,6 +13,7 @@
> >>  #include <malloc.h>
> >>  #include <mmc.h>
> >>  #include <asm/io.h>
> >> +#include <asm/arch/ccu.h>
> >>  #include <asm/arch/clock.h>
> >>  #include <asm/arch/cpu.h>
> >>  #include <asm/arch/gpio.h>
> >> @@ -34,6 +35,8 @@ struct sunxi_mmc_priv {
> >>       struct mmc_config cfg;
> >>  };
> >>
> >> +bool new_mode;
> >> +
> >>  #if !CONFIG_IS_ENABLED(DM_MMC)
> >>  /* support 4 mmc hosts */
> >>  struct sunxi_mmc_priv mmc_host[4];
> >> @@ -95,23 +98,19 @@ static int mmc_resource_init(int sdc_no)
> >>  }
> >>  #endif
> >>
> >> -static int mmc_set_mod_clk(struct sunxi_mmc_priv *priv, unsigned int hz)
> >> +int mmc_clk_set_rate(void *base, u32 bit, ulong rate)
> >>  {
> >>       unsigned int pll, pll_hz, div, n, oclk_dly, sclk_dly;
> >> -     bool new_mode = false;
> >>       u32 val = 0;
> >>
> >> -     if (IS_ENABLED(CONFIG_MMC_SUNXI_HAS_NEW_MODE) && (priv->mmc_no == 2))
> >> -             new_mode = true;
> >> -
> >
> > [..]
> >
> >> +static int mmc_set_mod_clk(struct sunxi_mmc_priv *priv, unsigned int hz)
> >> +{
> >> +#if CONFIG_IS_ENABLED(DM_MMC) && CONFIG_IS_ENABLED(CLK)
> >> +#else
> >> +     if (IS_ENABLED(CONFIG_MMC_SUNXI_HAS_NEW_MODE) && (priv->mmc_no == 2))
> >> +             new_mode = true;
> >> +
> >
> > I'm not sure why you need the global variable.
> >
> > The A83t emmc case you have below is caught in this condition, and
> > therefore, the scope doesn't need to be global.
> 
> Since mmc_set_rate calling with base and reg bits, which doesn't have
> any possibility know private data of driver we need a global to check
> to update the same in dm and non-dm.

Wouldn't that lead to issues if we have two controllers being active,
one with the new mode enabled, the other without, and you call
mmc_set_rate on the one without?

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180828/f6b82b74/attachment.sig>