Hi Wolfram,
clk: renesas: rcar-gen3: ...
Thanks for your patch!
On Tue, Sep 28, 2021 at 10:08 PM Wolfram Sang
<wsa+renesas@xxxxxxxxxxxxxxxxxxxx> wrote:
> The old SD handling code was huge and could not handle all the details
> which showed up on R-Car Gen3 SoCs meanwhile. It is time to switch to
> another design. Have SDnH a seperate clock, use the existing divider
separate (as checkpatch told me ;-)
> clocks and move the errata handling from the clock driver to the SDHI
> driver where it belongs.
>
> This patch removes the old SD handling code and switch to the new one.
> This updates the SDHI driver at the same time. Because the SDHI driver
> can only communitcate with the clock driver via clk_set_rate(), I don't
communicate (as Gmail composer told me ;-)
> see an alternative to this flag-day-approach, so we cross subsystems
> here.
>
> The patch sadly looks messy for the CPG lib, but it is basically a huge
> chunk of code removed and smaller chunks added. It looks much better
> when you just view the resulting source file.
Indeed, the end result is pretty neat.
BTW, the diff looks much better with the --histogram option of
git diff/show.
> Signed-off-by: Wolfram Sang <wsa+renesas@xxxxxxxxxxxxxxxxxxxx>
> --- a/drivers/clk/renesas/rcar-cpg-lib.c
> +++ b/drivers/clk/renesas/rcar-cpg-lib.c
> -static int cpg_sd_clock_set_rate(struct clk_hw *hw, unsigned long rate,
> - unsigned long parent_rate)
> +struct clk * __init cpg_sdh_clk_register(const char *name,
> + void __iomem *sdnckcr, const char *parent_name,
> + struct raw_notifier_head *notifiers)
> {
> - struct sd_clock *clock = to_sd_clock(hw);
> - unsigned int i;
> -
> - for (i = 0; i < clock->div_num; i++)
> - if (rate == DIV_ROUND_CLOSEST(parent_rate,
> - clock->div_table[i].div))
> - break;
> + struct cpg_simple_notifier *csn;
> + struct clk *clk;
>
> - if (i >= clock->div_num)
> - return -EINVAL;
> + csn = kzalloc(sizeof(*csn), GFP_KERNEL);
> + if (!csn)
> + return ERR_PTR(-ENOMEM);
>
> - clock->cur_div_idx = i;
> + csn->reg = sdnckcr;
>
> - cpg_reg_modify(clock->csn.reg, CPG_SD_STP_MASK | CPG_SD_FC_MASK,
> - clock->div_table[i].val &
> - (CPG_SD_STP_MASK | CPG_SD_FC_MASK));
> + clk = clk_register_divider_table(NULL, name, parent_name, 0, sdnckcr,
> + SDnSRCFC_SHIFT, 8, 0, cpg_sdh_div_table,
> + &cpg_lock);
> + if (IS_ERR(clk))
> + return clk;
Missing "kfree(csn)".
>
> - return 0;
> + cpg_simple_notifier_register(notifiers, csn);
> + return clk;
> }
> struct clk * __init cpg_sd_clk_register(const char *name,
> - void __iomem *base, unsigned int offset, const char *parent_name,
> - struct raw_notifier_head *notifiers, bool skip_first)
> + void __iomem *sdnckcr, const char *parent_name)
> {
> - struct clk_init_data init = {};
> - struct sd_clock *clock;
> - struct clk *clk;
> - u32 val;
> -
> - clock = kzalloc(sizeof(*clock), GFP_KERNEL);
> - if (!clock)
> - return ERR_PTR(-ENOMEM);
> -
> - init.name = name;
> - init.ops = &cpg_sd_clock_ops;
> - init.flags = CLK_SET_RATE_PARENT;
> - init.parent_names = &parent_name;
> - init.num_parents = 1;
> -
> - clock->csn.reg = base + offset;
> - clock->hw.init = &init;
> - clock->div_table = cpg_sd_div_table;
> - clock->div_num = ARRAY_SIZE(cpg_sd_div_table);
> -
> - if (skip_first) {
> - clock->div_table++;
> - clock->div_num--;
> - }
> -
> - val = readl(clock->csn.reg) & ~CPG_SD_FC_MASK;
> - val |= CPG_SD_STP_MASK | (clock->div_table[0].val & CPG_SD_FC_MASK);
> - writel(val, clock->csn.reg);
> -
> - clk = clk_register(NULL, &clock->hw);
> - if (IS_ERR(clk))
> - goto free_clock;
> -
> - cpg_simple_notifier_register(notifiers, &clock->csn);
> - return clk;
> -
> -free_clock:
> - kfree(clock);
> - return clk;
> + return clk_register_divider_table(NULL, name, parent_name, 0, sdnckcr,
> + 0, 2, 0, cpg_sd_div_table, &cpg_lock);
So the SDn clock can no longer be disabled, as CPG_SD_STP_CK
handling is gone?
> }
> --- a/drivers/mmc/host/renesas_sdhi_core.c
> +++ b/drivers/mmc/host/renesas_sdhi_core.c
> @@ -141,6 +143,16 @@ static unsigned int renesas_sdhi_clk_update(struct tmio_mmc_host *host,
> if (!(host->pdata->flags & TMIO_MMC_MIN_RCAR2) || mmc_doing_tune(host->mmc))
> return clk_get_rate(priv->clk);
>
> + if (priv->clkh) {
> + bool use_4tap = priv->quirks && priv->quirks->hs400_4taps;
> + bool need_slow_clkh = (host->mmc->ios.timing == MMC_TIMING_UHS_SDR104) ||
> + (host->mmc->ios.timing == MMC_TIMING_MMC_HS400);
> + clkh_mult = use_4tap && need_slow_clkh ? 2 : 4;
> + ref_clk = priv->clkh;
> + }
> +
> + new_clock = wanted_clock * clkh_mult;
> +
> /*
> * We want the bus clock to be as close as possible to, but no
> * greater than, new_clock. As we can divide by 1 << i for
> @@ -148,11 +160,10 @@ static unsigned int renesas_sdhi_clk_update(struct tmio_mmc_host *host,
> * possible, but no greater than, new_clock << i.
> */
> for (i = min(9, ilog2(UINT_MAX / new_clock)); i >= 0; i--) {
> - freq = clk_round_rate(priv->clk, new_clock << i);
> + freq = clk_round_rate(ref_clk, new_clock << i);
> if (freq > (new_clock << i)) {
> /* Too fast; look for a slightly slower option */
> - freq = clk_round_rate(priv->clk,
> - (new_clock << i) / 4 * 3);
> + freq = clk_round_rate(ref_clk, (new_clock << i) / 4 * 3);
> if (freq > (new_clock << i))
> continue;
> }
> @@ -164,7 +175,10 @@ static unsigned int renesas_sdhi_clk_update(struct tmio_mmc_host *host,
> }
> }
>
> - clk_set_rate(priv->clk, best_freq);
> + clk_set_rate(ref_clk, best_freq);
> +
> + if (ref_clk == priv->clkh)
"if (priv->clkh)", for consistency with above?
> + clk_set_rate(priv->clk, best_freq / clkh_mult);
>
> return clk_get_rate(priv->clk);
> }
> @@ -908,6 +922,10 @@ int renesas_sdhi_probe(struct platform_device *pdev,
> return ret;
> }
>
> + /* Fallback for old DTs */
> + if (of_device_is_compatible(pdev->dev.of_node, "renesas,rcar-gen3-sdhi"))
I think it would be cleaner to check a flag in struct
renesas_sdhi_of_data instead.
> + priv->clkh = clk_get_parent(clk_get_parent(priv->clk));
> +
> /*
> * Some controllers provide a 2nd clock just to run the internal card
> * detection logic. Unfortunately, the existing driver architecture does
The core looks good to me, but I have to admit I'm no expert on the
SDHn/SDn clock relations and the various SDHI transfer modes.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
