Re: [PATCH 2/9] soc: samsung: exynos-pmu: Add exynos_pmu_update/read/write APIs and SoC quirks

[Sam Protsenko]

On Wed, Jan 24, 2024 at 4:02 AM Peter Griffin <peter.griffin@linaroorg> wrote:
>
> Hi Sam,
>
> Thanks for the review feedback.
>
> On Tue, 23 Jan 2024 at 18:56, Sam Protsenko <semen.protsenko@xxxxxxxxxx> wrote:
> >
> > On Mon, Jan 22, 2024 at 4:57 PM Peter Griffin <peter.griffin@xxxxxxxxxx> wrote:
> > >
> > > Newer Exynos SoCs have atomic set/clear bit hardware for PMU registers as
> > > these registers can be accessed by multiple masters. Some platforms also
> > > protect the PMU registers for security hardening reasons so they can't be
> > > written by normal world and are only write acessible in el3 via a SMC call.
> > >
> > > Add support for both of these usecases using SoC specific quirks that are
> > > determined from the DT compatible string.
> > >
> > > Drivers which need to read and write PMU registers should now use these
> > > new exynos_pmu_*() APIs instead of obtaining a regmap using
> > > syscon_regmap_lookup_by_phandle()
> > >
> > > Depending on the SoC specific quirks, the exynos_pmu_*() APIs will access
> > > the PMU register in the appropriate way.
> > >
> > > Signed-off-by: Peter Griffin <peter.griffin@xxxxxxxxxx>
> > > ---
> > >  drivers/soc/samsung/exynos-pmu.c       | 209 ++++++++++++++++++++++++-
> > >  drivers/soc/samsung/exynos-pmu.h       |   4 +
> > >  include/linux/soc/samsung/exynos-pmu.h |  28 ++++
> > >  3 files changed, 234 insertions(+), 7 deletions(-)
> > >
> >
> > [snip]
> >
> > > +
> > > +int exynos_pmu_update_bits(unsigned int offset, unsigned int mask,
> > > +                          unsigned int val)
> > > +{
> > > +       if (pmu_context->pmu_data &&
> > > +           pmu_context->pmu_data->quirks & QUIRK_PMU_ALIVE_WRITE_SEC)
> > > +               return rmw_priv_reg(pmu_context->pmu_base_pa + offset,
> > > +                                   mask, val);
> > > +
> > > +       return regmap_update_bits(pmu_context->pmureg, offset, mask, val);
> > > +}
> > > +EXPORT_SYMBOL(exynos_pmu_update_bits);
> > > +
> >
> > This seems a bit hacky, from the design perspective. This way the user
> > will have to worry about things like driver dependencies, making sure
> > everything is instantiated in a correct order, etc. It also hides the
> > details otherwise visible through "syscon-phandle" property in the
> > device tree.
>
> In v2 I will keep the phandle to pmu_system_controller in DT, and add
> some -EPROBE_DEFER logic (See my email with Krzysztof).
>
> > Can we instead rework it by overriding regmap
> > implementation for Exynos specifics, and then continue to use it in
> > the leaf drivers via "syscon-phandle" property?
>
> I did look at that possibility first, as like you say it would avoid
> updating the leaf drivers to use the new API. Unfortunately a SMC
> backend to regmap was already tried and nacked upstream pretty hard.
> See here https://lore.kernel.org/lkml/20210723163759.GI5221@xxxxxxxxxxxxx/T/
>

Oh, I didn't mean creating a new regmap implementation :) To
illustrate what I meant, please look at these:

  - drivers/mfd/altera-sysmgr.c
  - altr_sysmgr_regmap_lookup_by_phandle()
  - arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi
  - drivers/mmc/host/dw_mmc-pltfm.c

They basically implement their own regmap operations (with smcc too)
in their syscon implementation. So they can actually reference that
syscon as phandle in device tree and avoid exporting and calling
read/write operations (which I think looks hacky). Instead they use
altr_sysmgr_regmap_lookup_by_phandle() to get their regmap (which
performs smcc), and then they just use regular regmap_read() /
regmap_write or whatever functions to operate on their regmap object.
That's what I meant by "overriding" the regmap.

Do you think this approach would be clearer and more "productizable"
so to speak? Just a thought.

> regards,
>
> Peter.