lkml.org 
[lkml]   [2020]   [Apr]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [RFC PATCH v2 3/3] soc: sprd: Add Spreadtrum special bits updating support
On Wed, Apr 15, 2020 at 11:36 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Mon, Apr 13, 2020 at 8:14 AM Baolin Wang <baolin.wang7@gmail.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@gmail.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.

>
> > +#define SPRD_REG_SET_OFFSET 0x1000
> > +#define SPRD_REG_CLR_OFFSET 0x2000
> > +
> > +/*
> > + * The Spreadtrum platform defines a special set/clear method to update
> > + * registers' bits, which means it can write values to the register's SET
> > + * address (offset is 0x1000) to set bits, and write values to the register's
> > + * CLEAR address (offset is 0x2000) to clear bits.
> > + *
> > + * This set/clear method can help to remove the race of accessing the global
> > + * registers between the multiple subsystems instead of using hardware
> > + * spinlocks.
> > + */
> > +static int sprd_syscon_update_bits(void *context, unsigned int reg,
> > + unsigned int mask, unsigned int val)
> > +{
> > + 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.

> > +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);

--
Baolin Wang

\
 
 \ /
  Last update: 2020-04-16 05:50    [W:0.382 / U:0.056 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site