Date:   Thu, 16 Apr 2020 21:49:01 +0800
From:   Baolin Wang <baolin.wang7@...il.com>
To:     Arnd Bergmann <arnd@...db.de>
Cc:     Lee Jones <lee.jones@...aro.org>, Mark Brown <broonie@...nel.org>,
        Orson Zhai <orsonzhai@...il.com>,
        Lyra Zhang <zhang.lyra@...il.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH v2 3/3] soc: sprd: Add Spreadtrum special bits
 updating support

On Thu, Apr 16, 2020 at 8:55 PM Arnd Bergmann <arnd@...db.de> wrote:
>
> On Thu, Apr 16, 2020 at 5:49 AM Baolin Wang <baolin.wang7@...il.com> wrote:
> >
> > On Wed, Apr 15, 2020 at 11:36 PM Arnd Bergmann <arnd@...db.de> wrote:
> > >
> > > On Mon, Apr 13, 2020 at 8:14 AM Baolin Wang <baolin.wang7@...il.com> wrote:
> > > >
> > > > The spreadtrum platform uses a special set/clear method to update
> > > > registers' bits, which can remove the race of updating the global
> > > > registers between the multiple subsystems. Thus we can register
> > > > a physical regmap bus into syscon core to support this.
> > > >
> > > > Signed-off-by: Baolin Wang <baolin.wang7@...il.com>
> > >
> > > I'd hope to avoid complicating the syscon driver further for this.
> > > Have you tried to use something other than syscon here to
> > > provide the regmap?
> >
> > I did not figure out other better solutions, since we still want to
> > use the common syscon driver with related APIs and node properties.
> >
> > Otherwise, I am afraid I should copy the common syscon driver into the
> > Spreadtrum SoC syscon driver with providing a new regmap bus, and
> > invent other similar APIs for users, but I think this is not good. We
> > still want to use the standard syscon APIs to keep consistent.
>
> Right, that is certainly a problem.
>
> One option would be modifying the syscon driver itself, making it support
> the spreadtrum specific update_bits function natively when a matching
> syscon node is used and CONFIG_ARCH_SPRD is enabled.
>
> We do support endianess properties and hwspinlocksin syscon, so adding
> another variant of regmap there isn't too much of a stretch.

I still prefer to use the compatible string as you suggested in this mail.

>
> > > > +       void __iomem *base = context;
> > > > +       unsigned int set, clr;
> > > > +
> > > > +       set = val & mask;
> > > > +       clr = ~set & mask;
> > > > +
> > > > +       if (set)
> > > > +               writel(set, base + reg + SPRD_REG_SET_OFFSET);
> > > > +
> > > > +       if (clr)
> > > > +               writel(clr, base + reg + SPRD_REG_CLR_OFFSET);
> > > > +
> > > > +       return 0;
> > > > +}
> > >
> > > Regarding the implementation: Doesn't this introduce a new race
> > > between setting and clearing bits if you do both at the same time?
> > >
> > > This may not be a problem if you never do.
> >
> > I think this is not a issue, we just make sure the set bits updating
> > and clear bits updating both are atomic operation, which is safe to
> > update bits, right?
> > If user want to protect a series of bits updating operation between
> > the multiple subsystems, ( such as including several bits setting and
> > bit clearing operations), you still need use hwlocks. But that's
> > another topic, which is not set/clr method can solve.
>
> One thing that breaks is setting a multi-bit field atomically. We have
> other drivers doing for instance
>
> static void cdce925_clk_set_pdiv(struct clk_cdce925_output *data, u16 pdiv)
> {
>         switch (data->index) {
>         case 0:
>                 regmap_update_bits(data->chip->regmap,
>                         CDCE925_REG_Y1SPIPDIVH,
>                         0x03, (pdiv >> 8) & 0x03);
>                 regmap_write(data->chip->regmap, 0x03, pdiv & 0xFF);
>                 break;
>         case 1:
>                 regmap_update_bits(data->chip->regmap, 0x16, 0x7F, pdiv);
>                 break;
>         case 2:
>                 regmap_update_bits(data->chip->regmap, 0x17, 0x7F, pdiv);
>                 break;
>         case 3:
>                 regmap_update_bits(data->chip->regmap, 0x26, 0x7F, pdiv);
>                 break;
> ...
> }
>
> This works with the read-modify-write method under a lock, but
> it would risk setting a dangerous (i.e. crashing the system or
> damaging the hardware) clock divider value if we first enable some
> bits and then disable some others.

Ah, I undersood your concern, yes, this is a potential risk. But we
have not met this issue with using set/clr method on our platform
until now.
I can add some comments in this file to describe this potential risk.
Thanks for pointing this out.

>
> Hardware registers only have bits you set or clear independently
> it is not a problem.
>
> > > > +static int sprd_syscon_init(void)
> > > > +{
> > > > +       syscon_register_phys_regmap_bus(&sprd_syscon_regmap);
> > > > +
> > > > +       return 0;
> > > > +}
> > > > +core_initcall_sync(sprd_syscon_init);
> > >
> > > I don't think this part can be done at all: If you load the module on a
> > > generic kernel running on a random other platform, it will break as
> > > there is no check at all to ensure the platform is compatible.
> > >
> > > The same thing happens on a platform that may have multiple
> > > syscon nodes, when not all of them use the same register layout.
> > >
> > > The only sane way that I can see would be to do it based on
> > > properties of the syscon node itself.
> >
> > OK, so what about adding a new property for the syscon node? and we
> > can check if need to register a new physical regmap bus from the
> > syscon node.
> >
> > if (of_property_read_bool(np, "physical-regmap-bus") && syscon_phy_regmap_bus)
> >         regmap = regmap_init(NULL, syscon_phy_regmap_bus, base, &syscon_config);
> > else
> >         regmap = regmap_init_mmio(NULL, base, &syscon_config);
>
> The property also needs to encode which implementation is used,
> either describing the way that spreadtrum does the bit set/clear,
> or just naming it something with spreadtrum.
>
> This could be either in the compatible string as a more specific
> identifier, or it could be a separate property.

OK, I think adding a Spreadtrum compatible string will be an easy and
clear way, so what about below sample code?

DT:
ap_ahb_regs: syscon@...10000 {
         compatible = "sprd,sc9860-syscon", "syscon";
         reg = <0 0x20210000 0 0x10000>;
};

/* The Spreadtrum syscon need register a real physical regmap bus with
new bits updating method. */
if (of_device_is_compatible(np, "sprd,sc9860-syscon") && syscon_phy_regmap_bus)
       regmap = regmap_init(NULL, syscon_phy_regmap_bus, base, &syscon_config);
else
       regmap = regmap_init_mmio(NULL, base, &syscon_config);

-- 
Baolin Wang

Powered by blists - more mailing lists