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