Messages in this thread | | | From | Arnd Bergmann <> | Date | Wed, 15 Apr 2020 17:36:07 +0200 | Subject | Re: [RFC PATCH v2 3/3] soc: sprd: Add Spreadtrum special bits updating support |
| |
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?
> +#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.
> +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.
Arnd
| |