Messages in this thread | | | From | Baolin Wang <> | Date | Thu, 16 Apr 2020 21:49:01 +0800 | 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@arndb.de> wrote: > > On Thu, Apr 16, 2020 at 5:49 AM Baolin Wang <baolin.wang7@gmail.com> wrote: > > > > 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. > > 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@20210000 { 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
| |