Messages in this thread | | | From | Baolin Wang <> | Date | Thu, 16 Apr 2020 11:49:03 +0800 | Subject | Re: [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
| |