On 22.02.2023 12:15, Jerome Brunet wrote:
>
> On Sat 18 Feb 2023 at 21:07, Heiner Kallweit <hkallweit1@xxxxxxxxx> 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@xxxxxxxxx>
>> ---
>> 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);
>
