Re: [RFC PATCH v2 3/3] soc: sprd: Add Spreadtrum special bits updating support

[Arnd Bergmann]

On Thu, Apr 16, 2020 at 5:49 AM Baolin Wang <baolin.wang7@xxxxxxxxx> wrote:
>
> On Wed, Apr 15, 2020 at 11:36 PM Arnd Bergmann <arnd@xxxxxxxx> wrote:
> >
> > On Mon, Apr 13, 2020 at 8:14 AM Baolin Wang <baolin.wang7@xxxxxxxxx> 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@xxxxxxxxx>
> >
> > 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.

> > > +       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.

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.

          Arnd