Re: [RFC PATCH 4/9] clk: renesas: gen3: switch to new SD clock handling

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux Memonry Technology]     [Linux USB Devel]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux