| Message ID | 0f78b654-86d9-3bbe-9fa5-003479b0cdbe@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] mmc: meson-gx: improve clock privisioning | expand |
On Sat 18 Feb 2023 at 21:07, Heiner Kallweit <hkallweit1@gmail.com> wrote: > Motivation for dealing with this code is that the driver doesn't work > on my SC2-based test system (HK1 RBOX X4S, based on ah212 board). > > The current code makes the assumption that clkin0 is set to XTAL rate > (24MHz) or less, otherwise the initial frequency of 400kHz can't be set, > considering that the maximum divider value is 63. > Currently there's no code for changing the rate of clkin0. Because it was expected the bootloader would leave clkin0 (the MMC clk) on the XTAL. This has holded true on all the SoC so far. It is a fairly weak and unsafe assumption for sure. > > On my system clkin0 is set to 1Ghz (fclkdiv2) when meson-gx mmc driver > is loaded. Therefore the driver doesn't work for me as-is. > > Further facts to consider: > > The MMC block internal divider isn't strictly needed for clock generation > because the clkin0 hierarchy includes a better (wider) divider that > we could use. It's primary purpose is supporting resampling. The bigger > the divider value the more granularity we have for resampling. I already tried to get rid of clkin1 in the past. You may indeed get fdiv2 and other clocks through the mmc clock of the main clock tree. However, getting fdiv2 (or another clock) through this path caused problem under various conditions with high speed modes (such as HS200 and SDR). It should have been equivalent, but it was not. Revisiting this is a good idea but it will require a *LOT* of tests on all the supported HW. > > clkin1 is fclkdiv2, and this clock is one parent of clkin0 anyway. > Therefore the MMC block internal mux isn't strictly needed. > In theory, it is not needed - In practice, it is needed. > What the proposed change does: > - Ignore the MMC block internal mux and use clkin0 only. > - Before setting rate of mmc_clk, set clkin0 to the rate closest to > 63 (max_div) * requested_rate. This allows for maximum divider value > and therefore most granularity for resampling. > > The changed driver works fine on my system. Initializing clkin0 to force it back on XTAL after devm_clk_get() would solve your problem too. It would be far simpler without any risk for the other supported HW in the short term. > > I have limited insight in the other Amlogic families supported by this > driver. Therefore patch comes as RFC. > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > --- > drivers/mmc/host/meson-gx-mmc.c | 77 +++++++++++++-------------------- > 1 file changed, 30 insertions(+), 47 deletions(-) > > diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c > index 2b963a81c..83d849db6 100644 > --- a/drivers/mmc/host/meson-gx-mmc.c > +++ b/drivers/mmc/host/meson-gx-mmc.c > @@ -32,6 +32,7 @@ > > #define SD_EMMC_CLOCK 0x0 > #define CLK_DIV_MASK GENMASK(5, 0) > +#define CLK_DIV_MASK_WIDTH __builtin_popcountl(CLK_DIV_MASK) > #define CLK_SRC_MASK GENMASK(7, 6) > #define CLK_CORE_PHASE_MASK GENMASK(9, 8) > #define CLK_TX_PHASE_MASK GENMASK(11, 10) > @@ -131,8 +132,6 @@ > #define SD_EMMC_PRE_REQ_DONE BIT(0) > #define SD_EMMC_DESC_CHAIN_MODE BIT(1) > > -#define MUX_CLK_NUM_PARENTS 2 > - > struct meson_mmc_data { > unsigned int tx_delay_mask; > unsigned int rx_delay_mask; > @@ -155,7 +154,7 @@ struct meson_host { > struct mmc_command *cmd; > > void __iomem *regs; > - struct clk *mux_clk; > + struct clk *clkin; > struct clk *mmc_clk; > unsigned long req_rate; > bool ddr; > @@ -203,6 +202,21 @@ struct meson_host { > #define CMD_RESP_MASK GENMASK(31, 1) > #define CMD_RESP_SRAM BIT(0) > > +static int meson_mmc_clk_set_rate(struct meson_host *host, unsigned long rate) > +{ > + unsigned long max_div; > + int ret; > + > + /* maximize divider value, this improves resampling granularity */ > + max_div = min(ULONG_MAX / rate, (1UL << CLK_DIV_MASK_WIDTH) - 1); > + > + ret = clk_set_rate(host->clkin, rate * max_div); > + if (ret) > + return ret; > + > + return clk_set_rate(host->mmc_clk, rate); > +} > + > static unsigned int meson_mmc_get_timeout_msecs(struct mmc_data *data) > { > unsigned int timeout = data->timeout_ns / NSEC_PER_MSEC; > @@ -386,7 +400,7 @@ static int meson_mmc_clk_set(struct meson_host *host, unsigned long rate, > writel(cfg, host->regs + SD_EMMC_CFG); > host->ddr = ddr; > > - ret = clk_set_rate(host->mmc_clk, rate); > + ret = meson_mmc_clk_set_rate(host, rate); > if (ret) { > dev_err(host->dev, "Unable to set cfg_div_clk to %lu. ret=%d\n", > rate, ret); > @@ -420,11 +434,9 @@ static int meson_mmc_clk_set(struct meson_host *host, unsigned long rate, > static int meson_mmc_clk_init(struct meson_host *host) > { > struct clk_init_data init; > - struct clk_mux *mux; > struct clk_divider *div; > char clk_name[32]; > - int i, ret = 0; > - const char *mux_parent_names[MUX_CLK_NUM_PARENTS]; > + int ret = 0; > const char *clk_parent[1]; > u32 clk_reg; > > @@ -438,40 +450,10 @@ static int meson_mmc_clk_init(struct meson_host *host) > clk_reg |= CLK_IRQ_SDIO_SLEEP(host); > writel(clk_reg, host->regs + SD_EMMC_CLOCK); > > - /* get the mux parents */ > - for (i = 0; i < MUX_CLK_NUM_PARENTS; i++) { > - struct clk *clk; > - char name[16]; > - > - snprintf(name, sizeof(name), "clkin%d", i); > - clk = devm_clk_get(host->dev, name); > - if (IS_ERR(clk)) > - return dev_err_probe(host->dev, PTR_ERR(clk), > - "Missing clock %s\n", name); > - > - mux_parent_names[i] = __clk_get_name(clk); => Here you could init clkin0 Another solution is use 'assigned-rate' and 'assigned-parent' in DT to properly set the MMC clock coming from the main clock tree. Maybe it would be better. > - } > - > - /* create the mux */ > - mux = devm_kzalloc(host->dev, sizeof(*mux), GFP_KERNEL); > - if (!mux) > - return -ENOMEM; > - > - snprintf(clk_name, sizeof(clk_name), "%s#mux", dev_name(host->dev)); > - init.name = clk_name; > - init.ops = &clk_mux_ops; > - init.flags = 0; > - init.parent_names = mux_parent_names; > - init.num_parents = MUX_CLK_NUM_PARENTS; > - > - mux->reg = host->regs + SD_EMMC_CLOCK; > - mux->shift = __ffs(CLK_SRC_MASK); > - mux->mask = CLK_SRC_MASK >> mux->shift; > - mux->hw.init = &init; > - > - host->mux_clk = devm_clk_register(host->dev, &mux->hw); > - if (WARN_ON(IS_ERR(host->mux_clk))) > - return PTR_ERR(host->mux_clk); > + host->clkin = devm_clk_get(host->dev, "clkin0"); > + if (IS_ERR(host->clkin)) > + return dev_err_probe(host->dev, PTR_ERR(host->clkin), > + "Missing clkin0\n"); > > /* create the divider */ > div = devm_kzalloc(host->dev, sizeof(*div), GFP_KERNEL); > @@ -481,14 +463,14 @@ static int meson_mmc_clk_init(struct meson_host *host) > snprintf(clk_name, sizeof(clk_name), "%s#div", dev_name(host->dev)); > init.name = clk_name; > init.ops = &clk_divider_ops; > - init.flags = CLK_SET_RATE_PARENT; > - clk_parent[0] = __clk_get_name(host->mux_clk); > + init.flags = 0; > + clk_parent[0] = __clk_get_name(host->clkin); > init.parent_names = clk_parent; > init.num_parents = 1; > > div->reg = host->regs + SD_EMMC_CLOCK; > div->shift = __ffs(CLK_DIV_MASK); > - div->width = __builtin_popcountl(CLK_DIV_MASK); > + div->width = CLK_DIV_MASK_WIDTH; > div->hw.init = &init; > div->flags = CLK_DIVIDER_ONE_BASED; > > @@ -497,11 +479,12 @@ static int meson_mmc_clk_init(struct meson_host *host) > return PTR_ERR(host->mmc_clk); > > /* init SD_EMMC_CLOCK to sane defaults w/min clock rate */ > - host->mmc->f_min = clk_round_rate(host->mmc_clk, 400000); > - ret = clk_set_rate(host->mmc_clk, host->mmc->f_min); > + ret = meson_mmc_clk_set_rate(host, 400000); > if (ret) > return ret; > > + host->mmc->f_min = clk_get_rate(host->mmc_clk); > + This diff actually changes nothing > return clk_prepare_enable(host->mmc_clk); > } > > @@ -531,7 +514,7 @@ static int meson_mmc_resampling_tuning(struct mmc_host *mmc, u32 opcode) > int ret; > > /* Resampling is done using the source clock */ > - max_dly = DIV_ROUND_UP(clk_get_rate(host->mux_clk), > + max_dly = DIV_ROUND_UP(clk_get_rate(host->clkin), > clk_get_rate(host->mmc_clk)); > > val = readl(host->regs + host->data->adjust);
On 22.02.2023 12:15, Jerome Brunet wrote: > > On Sat 18 Feb 2023 at 21:07, Heiner Kallweit <hkallweit1@gmail.com> wrote: > >> Motivation for dealing with this code is that the driver doesn't work >> on my SC2-based test system (HK1 RBOX X4S, based on ah212 board). >> >> The current code makes the assumption that clkin0 is set to XTAL rate >> (24MHz) or less, otherwise the initial frequency of 400kHz can't be set, >> considering that the maximum divider value is 63. >> Currently there's no code for changing the rate of clkin0. > > Because it was expected the bootloader would leave clkin0 (the MMC clk) > on the XTAL. This has holded true on all the SoC so far. It is a fairly > weak and unsafe assumption for sure. > >> >> On my system clkin0 is set to 1Ghz (fclkdiv2) when meson-gx mmc driver >> is loaded. Therefore the driver doesn't work for me as-is. >> >> Further facts to consider: >> >> The MMC block internal divider isn't strictly needed for clock generation >> because the clkin0 hierarchy includes a better (wider) divider that >> we could use. It's primary purpose is supporting resampling. The bigger >> the divider value the more granularity we have for resampling. > > I already tried to get rid of clkin1 in the past. > You may indeed get fdiv2 and other clocks through the mmc clock of the > main clock tree. However, getting fdiv2 (or another clock) through this > path caused problem under various conditions with high speed modes (such > as HS200 and SDR). > > It should have been equivalent, but it was not. Revisiting this is a > good idea but it will require a *LOT* of tests on all the > supported HW. > That's the type of feedback I was hoping for when sending the patch as RFC. Thanks. I didn't notice any problems with HS200 and the patch on my system. >> >> clkin1 is fclkdiv2, and this clock is one parent of clkin0 anyway. >> Therefore the MMC block internal mux isn't strictly needed. >> > > In theory, it is not needed - In practice, it is needed. > >> What the proposed change does: >> - Ignore the MMC block internal mux and use clkin0 only. >> - Before setting rate of mmc_clk, set clkin0 to the rate closest to >> 63 (max_div) * requested_rate. This allows for maximum divider value >> and therefore most granularity for resampling. >> >> The changed driver works fine on my system. > > Initializing clkin0 to force it back on XTAL after devm_clk_get() would > solve your problem too. It would be far simpler without any risk for the > other supported HW in the short term. > That's what I did in the first place to make it work on my system. >> >> I have limited insight in the other Amlogic families supported by this >> driver. Therefore patch comes as RFC. >> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >> --- >> drivers/mmc/host/meson-gx-mmc.c | 77 +++++++++++++-------------------- >> 1 file changed, 30 insertions(+), 47 deletions(-) >> >> diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c >> index 2b963a81c..83d849db6 100644 >> --- a/drivers/mmc/host/meson-gx-mmc.c >> +++ b/drivers/mmc/host/meson-gx-mmc.c >> @@ -32,6 +32,7 @@ >> >> #define SD_EMMC_CLOCK 0x0 >> #define CLK_DIV_MASK GENMASK(5, 0) >> +#define CLK_DIV_MASK_WIDTH __builtin_popcountl(CLK_DIV_MASK) >> #define CLK_SRC_MASK GENMASK(7, 6) >> #define CLK_CORE_PHASE_MASK GENMASK(9, 8) >> #define CLK_TX_PHASE_MASK GENMASK(11, 10) >> @@ -131,8 +132,6 @@ >> #define SD_EMMC_PRE_REQ_DONE BIT(0) >> #define SD_EMMC_DESC_CHAIN_MODE BIT(1) >> >> -#define MUX_CLK_NUM_PARENTS 2 >> - >> struct meson_mmc_data { >> unsigned int tx_delay_mask; >> unsigned int rx_delay_mask; >> @@ -155,7 +154,7 @@ struct meson_host { >> struct mmc_command *cmd; >> >> void __iomem *regs; >> - struct clk *mux_clk; >> + struct clk *clkin; >> struct clk *mmc_clk; >> unsigned long req_rate; >> bool ddr; >> @@ -203,6 +202,21 @@ struct meson_host { >> #define CMD_RESP_MASK GENMASK(31, 1) >> #define CMD_RESP_SRAM BIT(0) >> >> +static int meson_mmc_clk_set_rate(struct meson_host *host, unsigned long rate) >> +{ >> + unsigned long max_div; >> + int ret; >> + >> + /* maximize divider value, this improves resampling granularity */ >> + max_div = min(ULONG_MAX / rate, (1UL << CLK_DIV_MASK_WIDTH) - 1); >> + >> + ret = clk_set_rate(host->clkin, rate * max_div); >> + if (ret) >> + return ret; >> + >> + return clk_set_rate(host->mmc_clk, rate); >> +} >> + >> static unsigned int meson_mmc_get_timeout_msecs(struct mmc_data *data) >> { >> unsigned int timeout = data->timeout_ns / NSEC_PER_MSEC; >> @@ -386,7 +400,7 @@ static int meson_mmc_clk_set(struct meson_host *host, unsigned long rate, >> writel(cfg, host->regs + SD_EMMC_CFG); >> host->ddr = ddr; >> >> - ret = clk_set_rate(host->mmc_clk, rate); >> + ret = meson_mmc_clk_set_rate(host, rate); >> if (ret) { >> dev_err(host->dev, "Unable to set cfg_div_clk to %lu. ret=%d\n", >> rate, ret); >> @@ -420,11 +434,9 @@ static int meson_mmc_clk_set(struct meson_host *host, unsigned long rate, >> static int meson_mmc_clk_init(struct meson_host *host) >> { >> struct clk_init_data init; >> - struct clk_mux *mux; >> struct clk_divider *div; >> char clk_name[32]; >> - int i, ret = 0; >> - const char *mux_parent_names[MUX_CLK_NUM_PARENTS]; >> + int ret = 0; >> const char *clk_parent[1]; >> u32 clk_reg; >> >> @@ -438,40 +450,10 @@ static int meson_mmc_clk_init(struct meson_host *host) >> clk_reg |= CLK_IRQ_SDIO_SLEEP(host); >> writel(clk_reg, host->regs + SD_EMMC_CLOCK); >> >> - /* get the mux parents */ >> - for (i = 0; i < MUX_CLK_NUM_PARENTS; i++) { >> - struct clk *clk; >> - char name[16]; >> - >> - snprintf(name, sizeof(name), "clkin%d", i); >> - clk = devm_clk_get(host->dev, name); >> - if (IS_ERR(clk)) >> - return dev_err_probe(host->dev, PTR_ERR(clk), >> - "Missing clock %s\n", name); >> - >> - mux_parent_names[i] = __clk_get_name(clk); > > => Here you could init clkin0 > > Another solution is use 'assigned-rate' and 'assigned-parent' in DT > to properly set the MMC clock coming from the main clock tree. Maybe it > would be better. > I think you mean assigned-clocks and assigned-clock-rates. Yes, this would be another option. Thanks for the hint. >> - } >> - >> - /* create the mux */ >> - mux = devm_kzalloc(host->dev, sizeof(*mux), GFP_KERNEL); >> - if (!mux) >> - return -ENOMEM; >> - >> - snprintf(clk_name, sizeof(clk_name), "%s#mux", dev_name(host->dev)); >> - init.name = clk_name; >> - init.ops = &clk_mux_ops; >> - init.flags = 0; >> - init.parent_names = mux_parent_names; >> - init.num_parents = MUX_CLK_NUM_PARENTS; >> - >> - mux->reg = host->regs + SD_EMMC_CLOCK; >> - mux->shift = __ffs(CLK_SRC_MASK); >> - mux->mask = CLK_SRC_MASK >> mux->shift; >> - mux->hw.init = &init; >> - >> - host->mux_clk = devm_clk_register(host->dev, &mux->hw); >> - if (WARN_ON(IS_ERR(host->mux_clk))) >> - return PTR_ERR(host->mux_clk); >> + host->clkin = devm_clk_get(host->dev, "clkin0"); >> + if (IS_ERR(host->clkin)) >> + return dev_err_probe(host->dev, PTR_ERR(host->clkin), >> + "Missing clkin0\n"); >> >> /* create the divider */ >> div = devm_kzalloc(host->dev, sizeof(*div), GFP_KERNEL); >> @@ -481,14 +463,14 @@ static int meson_mmc_clk_init(struct meson_host *host) >> snprintf(clk_name, sizeof(clk_name), "%s#div", dev_name(host->dev)); >> init.name = clk_name; >> init.ops = &clk_divider_ops; >> - init.flags = CLK_SET_RATE_PARENT; >> - clk_parent[0] = __clk_get_name(host->mux_clk); >> + init.flags = 0; >> + clk_parent[0] = __clk_get_name(host->clkin); >> init.parent_names = clk_parent; >> init.num_parents = 1; >> >> div->reg = host->regs + SD_EMMC_CLOCK; >> div->shift = __ffs(CLK_DIV_MASK); >> - div->width = __builtin_popcountl(CLK_DIV_MASK); >> + div->width = CLK_DIV_MASK_WIDTH; >> div->hw.init = &init; >> div->flags = CLK_DIVIDER_ONE_BASED; >> >> @@ -497,11 +479,12 @@ static int meson_mmc_clk_init(struct meson_host *host) >> return PTR_ERR(host->mmc_clk); >> >> /* init SD_EMMC_CLOCK to sane defaults w/min clock rate */ >> - host->mmc->f_min = clk_round_rate(host->mmc_clk, 400000); >> - ret = clk_set_rate(host->mmc_clk, host->mmc->f_min); >> + ret = meson_mmc_clk_set_rate(host, 400000); >> if (ret) >> return ret; >> >> + host->mmc->f_min = clk_get_rate(host->mmc_clk); >> + > > This diff actually changes nothing > >> return clk_prepare_enable(host->mmc_clk); >> } >> >> @@ -531,7 +514,7 @@ static int meson_mmc_resampling_tuning(struct mmc_host *mmc, u32 opcode) >> int ret; >> >> /* Resampling is done using the source clock */ >> - max_dly = DIV_ROUND_UP(clk_get_rate(host->mux_clk), >> + max_dly = DIV_ROUND_UP(clk_get_rate(host->clkin), >> clk_get_rate(host->mmc_clk)); >> >> val = readl(host->regs + host->data->adjust); >
diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c index 2b963a81c..83d849db6 100644 --- a/drivers/mmc/host/meson-gx-mmc.c +++ b/drivers/mmc/host/meson-gx-mmc.c @@ -32,6 +32,7 @@ #define SD_EMMC_CLOCK 0x0 #define CLK_DIV_MASK GENMASK(5, 0) +#define CLK_DIV_MASK_WIDTH __builtin_popcountl(CLK_DIV_MASK) #define CLK_SRC_MASK GENMASK(7, 6) #define CLK_CORE_PHASE_MASK GENMASK(9, 8) #define CLK_TX_PHASE_MASK GENMASK(11, 10) @@ -131,8 +132,6 @@ #define SD_EMMC_PRE_REQ_DONE BIT(0) #define SD_EMMC_DESC_CHAIN_MODE BIT(1) -#define MUX_CLK_NUM_PARENTS 2 - struct meson_mmc_data { unsigned int tx_delay_mask; unsigned int rx_delay_mask; @@ -155,7 +154,7 @@ struct meson_host { struct mmc_command *cmd; void __iomem *regs; - struct clk *mux_clk; + struct clk *clkin; struct clk *mmc_clk; unsigned long req_rate; bool ddr; @@ -203,6 +202,21 @@ struct meson_host { #define CMD_RESP_MASK GENMASK(31, 1) #define CMD_RESP_SRAM BIT(0) +static int meson_mmc_clk_set_rate(struct meson_host *host, unsigned long rate) +{ + unsigned long max_div; + int ret; + + /* maximize divider value, this improves resampling granularity */ + max_div = min(ULONG_MAX / rate, (1UL << CLK_DIV_MASK_WIDTH) - 1); + + ret = clk_set_rate(host->clkin, rate * max_div); + if (ret) + return ret; + + return clk_set_rate(host->mmc_clk, rate); +} + static unsigned int meson_mmc_get_timeout_msecs(struct mmc_data *data) { unsigned int timeout = data->timeout_ns / NSEC_PER_MSEC; @@ -386,7 +400,7 @@ static int meson_mmc_clk_set(struct meson_host *host, unsigned long rate, writel(cfg, host->regs + SD_EMMC_CFG); host->ddr = ddr; - ret = clk_set_rate(host->mmc_clk, rate); + ret = meson_mmc_clk_set_rate(host, rate); if (ret) { dev_err(host->dev, "Unable to set cfg_div_clk to %lu. ret=%d\n", rate, ret); @@ -420,11 +434,9 @@ static int meson_mmc_clk_set(struct meson_host *host, unsigned long rate, static int meson_mmc_clk_init(struct meson_host *host) { struct clk_init_data init; - struct clk_mux *mux; struct clk_divider *div; char clk_name[32]; - int i, ret = 0; - const char *mux_parent_names[MUX_CLK_NUM_PARENTS]; + int ret = 0; const char *clk_parent[1]; u32 clk_reg; @@ -438,40 +450,10 @@ static int meson_mmc_clk_init(struct meson_host *host) clk_reg |= CLK_IRQ_SDIO_SLEEP(host); writel(clk_reg, host->regs + SD_EMMC_CLOCK); - /* get the mux parents */ - for (i = 0; i < MUX_CLK_NUM_PARENTS; i++) { - struct clk *clk; - char name[16]; - - snprintf(name, sizeof(name), "clkin%d", i); - clk = devm_clk_get(host->dev, name); - if (IS_ERR(clk)) - return dev_err_probe(host->dev, PTR_ERR(clk), - "Missing clock %s\n", name); - - mux_parent_names[i] = __clk_get_name(clk); - } - - /* create the mux */ - mux = devm_kzalloc(host->dev, sizeof(*mux), GFP_KERNEL); - if (!mux) - return -ENOMEM; - - snprintf(clk_name, sizeof(clk_name), "%s#mux", dev_name(host->dev)); - init.name = clk_name; - init.ops = &clk_mux_ops; - init.flags = 0; - init.parent_names = mux_parent_names; - init.num_parents = MUX_CLK_NUM_PARENTS; - - mux->reg = host->regs + SD_EMMC_CLOCK; - mux->shift = __ffs(CLK_SRC_MASK); - mux->mask = CLK_SRC_MASK >> mux->shift; - mux->hw.init = &init; - - host->mux_clk = devm_clk_register(host->dev, &mux->hw); - if (WARN_ON(IS_ERR(host->mux_clk))) - return PTR_ERR(host->mux_clk); + host->clkin = devm_clk_get(host->dev, "clkin0"); + if (IS_ERR(host->clkin)) + return dev_err_probe(host->dev, PTR_ERR(host->clkin), + "Missing clkin0\n"); /* create the divider */ div = devm_kzalloc(host->dev, sizeof(*div), GFP_KERNEL); @@ -481,14 +463,14 @@ static int meson_mmc_clk_init(struct meson_host *host) snprintf(clk_name, sizeof(clk_name), "%s#div", dev_name(host->dev)); init.name = clk_name; init.ops = &clk_divider_ops; - init.flags = CLK_SET_RATE_PARENT; - clk_parent[0] = __clk_get_name(host->mux_clk); + init.flags = 0; + clk_parent[0] = __clk_get_name(host->clkin); init.parent_names = clk_parent; init.num_parents = 1; div->reg = host->regs + SD_EMMC_CLOCK; div->shift = __ffs(CLK_DIV_MASK); - div->width = __builtin_popcountl(CLK_DIV_MASK); + div->width = CLK_DIV_MASK_WIDTH; div->hw.init = &init; div->flags = CLK_DIVIDER_ONE_BASED; @@ -497,11 +479,12 @@ static int meson_mmc_clk_init(struct meson_host *host) return PTR_ERR(host->mmc_clk); /* init SD_EMMC_CLOCK to sane defaults w/min clock rate */ - host->mmc->f_min = clk_round_rate(host->mmc_clk, 400000); - ret = clk_set_rate(host->mmc_clk, host->mmc->f_min); + ret = meson_mmc_clk_set_rate(host, 400000); if (ret) return ret; + host->mmc->f_min = clk_get_rate(host->mmc_clk); + return clk_prepare_enable(host->mmc_clk); } @@ -531,7 +514,7 @@ static int meson_mmc_resampling_tuning(struct mmc_host *mmc, u32 opcode) int ret; /* Resampling is done using the source clock */ - max_dly = DIV_ROUND_UP(clk_get_rate(host->mux_clk), + max_dly = DIV_ROUND_UP(clk_get_rate(host->clkin), clk_get_rate(host->mmc_clk)); val = readl(host->regs + host->data->adjust);
Motivation for dealing with this code is that the driver doesn't work on my SC2-based test system (HK1 RBOX X4S, based on ah212 board). The current code makes the assumption that clkin0 is set to XTAL rate (24MHz) or less, otherwise the initial frequency of 400kHz can't be set, considering that the maximum divider value is 63. Currently there's no code for changing the rate of clkin0. On my system clkin0 is set to 1Ghz (fclkdiv2) when meson-gx mmc driver is loaded. Therefore the driver doesn't work for me as-is. Further facts to consider: The MMC block internal divider isn't strictly needed for clock generation because the clkin0 hierarchy includes a better (wider) divider that we could use. It's primary purpose is supporting resampling. The bigger the divider value the more granularity we have for resampling. clkin1 is fclkdiv2, and this clock is one parent of clkin0 anyway. Therefore the MMC block internal mux isn't strictly needed. What the proposed change does: - Ignore the MMC block internal mux and use clkin0 only. - Before setting rate of mmc_clk, set clkin0 to the rate closest to 63 (max_div) * requested_rate. This allows for maximum divider value and therefore most granularity for resampling. The changed driver works fine on my system. I have limited insight in the other Amlogic families supported by this driver. Therefore patch comes as RFC. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/mmc/host/meson-gx-mmc.c | 77 +++++++++++++-------------------- 1 file changed, 30 insertions(+), 47 deletions(-)