Re: [PATCH 2/2] clk: mvebu: use firmware SiP service for accessing dfx register set

From: Grzegorz Jaszczyk <jaz@semihalf.com>
To: Stephen Boyd <sboyd@kernel.org>
Cc: linux-clk@vger.kernel.org, linux-pm@vger.kernel.org,
	Michael Turquette <mturquette@baylibre.com>,
	Miquel Raynal <miquel.raynal@bootlin.com>,
	rui.zhang@intel.com, daniel.lezcano@linaro.org, amitk@kernel.org,
	Marcin Wojtas <mw@semihalf.com>,
	Nadav Haklai <nadavh@marvell.com>, Ben Peled <bpeled@marvell.com>,
	Stefan Chulski <stefanc@marvell.com>,
	Konstantin Porotchkin <kostap@marvell.com>
Subject: Re: [PATCH 2/2] clk: mvebu: use firmware SiP service for accessing dfx register set
Date: Tue, 29 Dec 2020 23:51:16 +0100	[thread overview]
Message-ID: <CAH76GKOQoroW4p3qTffXtFJLbhvSRfOWOnm0JPysqiy8K2wT_g@mail.gmail.com> (raw)
In-Reply-To: <160842340492.1580929.1761349313009973921@swboyd.mtv.corp.google.com>

niedz., 20 gru 2020 o 01:16 Stephen Boyd <sboyd@kernel.org> napisał(a):
>
> Quoting kostap@marvell.com (2020-12-17 09:46:02)
> > diff --git a/drivers/clk/mvebu/ap-cpu-clk.c b/drivers/clk/mvebu/ap-cpu-clk.c
> > index b4259b60dcfd..52721ef2d7d9 100644
> > --- a/drivers/clk/mvebu/ap-cpu-clk.c
> > +++ b/drivers/clk/mvebu/ap-cpu-clk.c
> > @@ -139,8 +141,107 @@ struct ap_cpu_clk {
> >         struct clk_hw hw;
> >         struct regmap *pll_cr_base;
> >         const struct cpu_dfs_regs *pll_regs;
> > +       phys_addr_t phys;
> >  };
> >
> > +static int dfx_sread_smc(unsigned long addr, unsigned int *reg)
> > +{
> > +       struct arm_smccc_res res;
> > +
> > +       arm_smccc_smc(MV_SIP_DFX, MV_SIP_DFX_SREAD, addr, 0, 0, 0, 0, 0, &res);
> > +
> > +       if (res.a0 == 0 && reg != NULL)
> > +               *reg = res.a1;
> > +
> > +       return res.a0;
> > +}
> > +
> > +static int dfx_swrite_smc(unsigned long addr, unsigned long val)
> > +{
> > +       struct arm_smccc_res res;
> > +
> > +       arm_smccc_smc(MV_SIP_DFX, MV_SIP_DFX_SWRITE, addr, val,
> > +                     0, 0, 0, 0, &res);
> > +
> > +       return res.a0;
> > +}
>
> Can this be implemented as a regmap, similar to the regmap-mmio? And
> then the cpu clks use this instead of mmio regmap?

Yes, I think it is possible. Just to be sure: you suggest to provide a
custom regmap reg_read/reg_write, based on current
dfx_sread_smc/dfx_swrite_smc, only if it is needed (after some check
in probe - please see also my answer below)?

>
> > +
> > +static int ap_clk_regmap_read(struct ap_cpu_clk *clk, unsigned int reg,
> > +                             unsigned int *val)
> > +{
> > +       int ret;
> > +
> > +       ret = dfx_sread_smc(clk->phys + reg, val);
> > +       if (ret != SMCCC_RET_SUCCESS)
> > +               ret = regmap_read(clk->pll_cr_base, reg, val);
> > +
> > +       return ret;
> > +}
> > +
> > +static int ap_clk_regmap_write(struct ap_cpu_clk *clk, unsigned int reg,
> > +                         unsigned int val)
> > +{
> > +       int ret;
> > +
> > +       ret = dfx_swrite_smc(clk->phys + reg, val);
> > +       if (ret != SMCCC_RET_SUCCESS)
> > +               ret = regmap_write(clk->pll_cr_base, reg, val);
> > +
> > +       return ret;
> > +}
> > +
> > +static int ap_clk_regmap_update_bits(struct ap_cpu_clk *clk, unsigned int reg,
> > +                                    unsigned int mask, unsigned int val)
> > +{
> > +       int ret;
> > +       unsigned int tmp;
> > +
> > +       ret = dfx_sread_smc(clk->phys + reg, &tmp);
> > +       if (ret != SMCCC_RET_SUCCESS)
> > +               goto try_legacy;
>
> Can we try the legacy path at boot to read something and then if it
> fails plug in the old regmap? If it works then use the new regmap. Let's
> not keep both paths all the time if we can runtime detect it at boot.

Trying legacy in case of a secured dfx region will result with an
exception. Actually doing the opposite will do the job: during driver
probe read something with the use of smc, in case of success we will
know that we deal with updated fw and we can switch to custom, based
on smc, regmap reg_read/reg_write (as described above). In case of
failure we continue with "legacy" default regmap reg_read/reg_write.

>
> > +
> > +       tmp &= ~mask;
> > +       tmp |= val & mask;
> > +
> > +       ret = dfx_swrite_smc(clk->phys + reg, tmp);
> > +       if (ret == SMCCC_RET_SUCCESS)
> > +               return ret;
> > +
> > +try_legacy:
> > +       return regmap_update_bits(clk->pll_cr_base, reg, mask, val);
> > +}
> > +
> > +static int ap_clk_regmap_read_poll_timeout(struct ap_cpu_clk *clk,
> > +                                           unsigned int reg,
> > +                                           unsigned int stable_bit)
> > +{
> > +       int ret;
> > +       u32 val;
> > +       ktime_t timeout;
> > +
> > +       timeout = ktime_add_us(ktime_get(), STATUS_POLL_TIMEOUT_US);
> > +       do {
> > +               ret = dfx_sread_smc(clk->phys + reg, &val);
> > +               if (ret || (val & stable_bit))
> > +                       break;
> > +
> > +               usleep_range((STATUS_POLL_PERIOD_US >> 2) + 1,
> > +                            STATUS_POLL_PERIOD_US);
> > +
> > +       } while (ktime_before(ktime_get(), timeout));
> > +
> > +       if (ret == SMCCC_RET_SUCCESS)
> > +               return (val & stable_bit) ? 0 : -ETIMEDOUT;
> > +
> > +       /* If above fail, try legacy */
> > +       ret = regmap_read_poll_timeout(clk->pll_cr_base,
> > +                                      reg, val,
> > +                                      val & stable_bit, STATUS_POLL_PERIOD_US,
> > +                                      STATUS_POLL_TIMEOUT_US);
> > +
> > +       return ret;
> > +}
> > +
> >  static unsigned long ap_cpu_clk_recalc_rate(struct clk_hw *hw,
> >                                             unsigned long parent_rate)
> >  {
> > @@ -150,7 +251,7 @@ static unsigned long ap_cpu_clk_recalc_rate(struct clk_hw *hw,
> >
> >         cpu_clkdiv_reg = clk->pll_regs->divider_reg +
> >                 (clk->cluster * clk->pll_regs->cluster_offset);
> > -       regmap_read(clk->pll_cr_base, cpu_clkdiv_reg, &cpu_clkdiv_ratio);
> > +       ap_clk_regmap_read(clk, cpu_clkdiv_reg, &cpu_clkdiv_ratio);
> >         cpu_clkdiv_ratio &= clk->pll_regs->divider_mask;
> >         cpu_clkdiv_ratio >>= clk->pll_regs->divider_offset;
> >
> > @@ -171,7 +272,7 @@ static int ap_cpu_clk_set_rate(struct clk_hw *hw, unsigned long rate,
> >         cpu_ratio_reg = clk->pll_regs->ratio_reg +
> >                 (clk->cluster * clk->pll_regs->cluster_offset);
> >
> > -       regmap_read(clk->pll_cr_base, cpu_clkdiv_reg, &reg);
> > +       ap_clk_regmap_read(clk, cpu_clkdiv_reg, &reg);
> >         reg &= ~(clk->pll_regs->divider_mask);
> >         reg |= (divider << clk->pll_regs->divider_offset);
> >
> > @@ -184,29 +285,26 @@ static int ap_cpu_clk_set_rate(struct clk_hw *hw, unsigned long rate,
> >                 reg |= ((divider * clk->pll_regs->divider_ratio) <<
> >                                 AP807_PLL_CR_1_CPU_CLK_DIV_RATIO_OFFSET);
> >         }
> > -       regmap_write(clk->pll_cr_base, cpu_clkdiv_reg, reg);
> > -
> > +       ap_clk_regmap_write(clk, cpu_clkdiv_reg, reg);
> >
> > -       regmap_update_bits(clk->pll_cr_base, cpu_force_reg,
> > -                          clk->pll_regs->force_mask,
> > -                          clk->pll_regs->force_mask);
> > +       ap_clk_regmap_update_bits(clk, cpu_force_reg, clk->pll_regs->force_mask,
> > +                                 clk->pll_regs->force_mask);
> >
> > -       regmap_update_bits(clk->pll_cr_base, cpu_ratio_reg,
> > -                          BIT(clk->pll_regs->ratio_offset),
> > -                          BIT(clk->pll_regs->ratio_offset));
> > +       ap_clk_regmap_update_bits(clk, cpu_ratio_reg,
> > +                                 BIT(clk->pll_regs->ratio_offset),
> > +                                 BIT(clk->pll_regs->ratio_offset));
> >
> >         stable_bit = BIT(clk->pll_regs->ratio_state_offset +
> >                          clk->cluster *
> >                          clk->pll_regs->ratio_state_cluster_offset);
> > -       ret = regmap_read_poll_timeout(clk->pll_cr_base,
> > -                                      clk->pll_regs->ratio_state_reg, reg,
> > -                                      reg & stable_bit, STATUS_POLL_PERIOD_US,
> > -                                      STATUS_POLL_TIMEOUT_US);
> > +       ret = ap_clk_regmap_read_poll_timeout(clk,
> > +                                             clk->pll_regs->ratio_state_reg,
> > +                                             stable_bit);
> >         if (ret)
> >                 return ret;
> >
> > -       regmap_update_bits(clk->pll_cr_base, cpu_ratio_reg,
> > -                          BIT(clk->pll_regs->ratio_offset), 0);
> > +       ap_clk_regmap_update_bits(clk, cpu_ratio_reg,
> > +                                 BIT(clk->pll_regs->ratio_offset), 0);
> >
> >         return 0;
> >  }
> > @@ -235,6 +333,11 @@ static int ap_cpu_clock_probe(struct platform_device *pdev)
> >         struct clk_hw_onecell_data *ap_cpu_data;
> >         struct ap_cpu_clk *ap_cpu_clk;
> >         struct regmap *regmap;
> > +       struct resource res;
> > +
> > +       ret = of_address_to_resource(np->parent, 0, &res);
> > +       if (ret)
> > +               return ret;
> >
> >         regmap = syscon_node_to_regmap(np->parent);
> >         if (IS_ERR(regmap)) {
> > @@ -313,6 +416,12 @@ static int ap_cpu_clock_probe(struct platform_device *pdev)
> >                 ap_cpu_clk[cluster_index].dev = dev;
> >                 ap_cpu_clk[cluster_index].pll_regs = of_device_get_match_data(&pdev->dev);
> >
> > +               /*
> > +                * Hack to retrieve a physical addr that will be given to the
>
> I'd rather not enshrine "hack" in the code. "Get a physicall address to
> hand to the firmware"? Presumably this address will work and isn't some
> intermediate PA (IPA)?

You are correct, let's update the comment as you suggested.

Thank you for all your comments and suggestions,
Grzegorz

>
> > +                * firmware.
> > +                */
> > +               ap_cpu_clk[cluster_index].phys = res.start;
> > +
> >                 init.name = ap_cpu_clk[cluster_index].clk_name;
> >                 init.ops = &ap_cpu_clk_ops;
> >                 init.num_parents = 1;