[RFC PATCH v2 0/3] Add new reg_update_bits() support

[RFC PATCH v2 0/3] Add new reg_update_bits() support

[Baolin Wang]

The Spreadtrum platform uses a special set/clear method to update
registers' bits, thus this patch set registers a physical regmap
bus into syscon core to support this feature instead of using the
MMIO bus, which is not a physical regmap bus.

Any comments are welcome. Thanks.

Changes from RFC v1:
 - Add new helper to registers a physical regmap bus instead of
 using the MMIO bus.

Baolin Wang (3):
  mfd: syscon: Support physical regmap bus
  regmap: Add bus reg_update_bits() support
  soc: sprd: Add Spreadtrum special bits updating support

 drivers/base/regmap/regmap.c   |  1 +
 drivers/mfd/syscon.c           | 16 ++++++-
 drivers/soc/Kconfig            |  1 +
 drivers/soc/Makefile           |  1 +
 drivers/soc/sprd/Kconfig       | 16 +++++++
 drivers/soc/sprd/Makefile      |  2 +
 drivers/soc/sprd/sprd_syscon.c | 76 ++++++++++++++++++++++++++++++++++
 include/linux/mfd/syscon.h     |  7 ++++
 8 files changed, 118 insertions(+), 2 deletions(-)
 create mode 100644 drivers/soc/sprd/Kconfig
 create mode 100644 drivers/soc/sprd/Makefile
 create mode 100644 drivers/soc/sprd/sprd_syscon.c

-- 
2.17.1

[RFC PATCH v2 1/3] mfd: syscon: Support physical regmap bus

[Baolin Wang]

Some platforms such as Spreadtrum platform, define a special method to
update bits of the registers instead of reading and writing, which means
we should use a physical regmap bus to define the reg_update_bits()
operation instead of the MMIO regmap bus.

Thus add a new helper for the syscon driver to allow to register a physical
regmap bus to support this new requirement.

Signed-off-by: Baolin Wang <baolin.wang7@gmail.com>
---
 drivers/mfd/syscon.c       | 16 ++++++++++++++--
 include/linux/mfd/syscon.h |  7 +++++++
 2 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
index 3a97816d0cba..5de74e1b6634 100644
--- a/drivers/mfd/syscon.c
+++ b/drivers/mfd/syscon.c
@@ -24,6 +24,7 @@
 #include <linux/slab.h>
 
 static struct platform_driver syscon_driver;
+static struct regmap_bus *syscon_phy_regmap_bus;
 
 static DEFINE_SPINLOCK(syscon_list_slock);
 static LIST_HEAD(syscon_list);
@@ -106,14 +107,18 @@ static struct syscon *of_syscon_register(struct device_node *np, bool check_clk)
 	syscon_config.val_bits = reg_io_width * 8;
 	syscon_config.max_register = resource_size(&res) - reg_io_width;
 
-	regmap = regmap_init_mmio(NULL, base, &syscon_config);
+	if (syscon_phy_regmap_bus)
+		regmap = regmap_init(NULL, syscon_phy_regmap_bus, base,
+				     &syscon_config);
+	else
+		regmap = regmap_init_mmio(NULL, base, &syscon_config);
 	if (IS_ERR(regmap)) {
 		pr_err("regmap init failed\n");
 		ret = PTR_ERR(regmap);
 		goto err_regmap;
 	}
 
-	if (check_clk) {
+	if (check_clk && !syscon_phy_regmap_bus) {
 		clk = of_clk_get(np, 0);
 		if (IS_ERR(clk)) {
 			ret = PTR_ERR(clk);
@@ -172,6 +177,13 @@ static struct regmap *device_node_get_regmap(struct device_node *np,
 	return syscon->regmap;
 }
 
+void syscon_register_phys_regmap_bus(struct regmap_bus *phy_regmap_bus)
+{
+	if (phy_regmap_bus)
+		syscon_phy_regmap_bus = phy_regmap_bus;
+}
+EXPORT_SYMBOL_GPL(syscon_register_phys_regmap_bus);
+
 struct regmap *device_node_to_regmap(struct device_node *np)
 {
 	return device_node_get_regmap(np, false);
diff --git a/include/linux/mfd/syscon.h b/include/linux/mfd/syscon.h
index 7f20e9b502a5..fbc2a2f0f414 100644
--- a/include/linux/mfd/syscon.h
+++ b/include/linux/mfd/syscon.h
@@ -13,6 +13,7 @@
 
 #include <linux/err.h>
 #include <linux/errno.h>
+#include <linux/regmap.h>
 
 struct device_node;
 
@@ -28,6 +29,7 @@ extern struct regmap *syscon_regmap_lookup_by_phandle_args(
 					const char *property,
 					int arg_count,
 					unsigned int *out_args);
+extern void syscon_register_phys_regmap_bus(struct regmap_bus *phy_regmap_bus);
 #else
 static inline struct regmap *device_node_to_regmap(struct device_node *np)
 {
@@ -59,6 +61,11 @@ static inline struct regmap *syscon_regmap_lookup_by_phandle_args(
 {
 	return ERR_PTR(-ENOTSUPP);
 }
+
+static inline void
+syscon_register_phys_regmap_bus(struct regmap_bus *phy_regmap_bus)
+{
+}
 #endif
 
 #endif /* __LINUX_MFD_SYSCON_H__ */
-- 
2.17.1

[RFC PATCH v2 2/3] regmap: Add bus reg_update_bits() support

[Baolin Wang]

Add reg_update_bits() support in case some platforms use a special method
to update bits of registers.

Signed-off-by: Baolin Wang <baolin.wang7@gmail.com>
---
 drivers/base/regmap/regmap.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index 59f911e57719..553d92aa0c68 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -827,6 +827,7 @@ struct regmap *__regmap_init(struct device *dev,
 	} else if (!bus->read || !bus->write) {
 		map->reg_read = _regmap_bus_reg_read;
 		map->reg_write = _regmap_bus_reg_write;
+		map->reg_update_bits = bus->reg_update_bits;
 
 		map->defer_caching = false;
 		goto skip_format_initialization;
-- 
2.17.1

Applied "regmap: Add bus reg_update_bits() support" to the regmap tree

[Mark Brown]

The patch

   regmap: Add bus reg_update_bits() support

has been applied to the regmap tree at

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

From 80215f133d59310fdfce5ee4398aeb7076c2e99f Mon Sep 17 00:00:00 2001
From: Baolin Wang <baolin.wang7@gmail.com>
Date: Mon, 13 Apr 2020 14:13:20 +0800
Subject: [PATCH] regmap: Add bus reg_update_bits() support

Add reg_update_bits() support in case some platforms use a special method
to update bits of registers.

Signed-off-by: Baolin Wang <baolin.wang7@gmail.com>
Link: https://lore.kernel.org/r/df32fd0529957d1e7e26ba1465723f16cfbe92c8.1586757922.git.baolin.wang7@gmail.com
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/base/regmap/regmap.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index 59f911e57719..553d92aa0c68 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -827,6 +827,7 @@ struct regmap *__regmap_init(struct device *dev,
 	} else if (!bus->read || !bus->write) {
 		map->reg_read = _regmap_bus_reg_read;
 		map->reg_write = _regmap_bus_reg_write;
+		map->reg_update_bits = bus->reg_update_bits;
 
 		map->defer_caching = false;
 		goto skip_format_initialization;
-- 
2.20.1

Re: [RFC PATCH v2 2/3] regmap: Add bus reg_update_bits() support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 978 bytes --]

On Mon, Apr 13, 2020 at 02:13:20PM +0800, Baolin Wang wrote:
> Add reg_update_bits() support in case some platforms use a special method
> to update bits of registers.

The following changes since commit 8f3d9f354286745c751374f5f1fcafee6b3f3136:

  Linux 5.7-rc1 (2020-04-12 12:35:55 -0700)

are available in the Git repository at:

  https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git tags/regmap-add-bus-update-bits

for you to fetch changes up to 80215f133d59310fdfce5ee4398aeb7076c2e99f:

  regmap: Add bus reg_update_bits() support (2020-04-14 16:05:35 +0100)

----------------------------------------------------------------
regmap: Allow bus update_bits() operation

Allow buses that provide custom operations to provide a custom
update_bits() operation.

----------------------------------------------------------------
Baolin Wang (1):
      regmap: Add bus reg_update_bits() support

 drivers/base/regmap/regmap.c | 1 +
 1 file changed, 1 insertion(+)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Baolin Wang]

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>
---
 drivers/soc/Kconfig            |  1 +
 drivers/soc/Makefile           |  1 +
 drivers/soc/sprd/Kconfig       | 16 +++++++
 drivers/soc/sprd/Makefile      |  2 +
 drivers/soc/sprd/sprd_syscon.c | 76 ++++++++++++++++++++++++++++++++++
 5 files changed, 96 insertions(+)
 create mode 100644 drivers/soc/sprd/Kconfig
 create mode 100644 drivers/soc/sprd/Makefile
 create mode 100644 drivers/soc/sprd/sprd_syscon.c

diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig
index 425ab6f7e375..8cfbf2dc518d 100644
--- a/drivers/soc/Kconfig
+++ b/drivers/soc/Kconfig
@@ -23,5 +23,6 @@ source "drivers/soc/versatile/Kconfig"
 source "drivers/soc/xilinx/Kconfig"
 source "drivers/soc/zte/Kconfig"
 source "drivers/soc/kendryte/Kconfig"
+source "drivers/soc/sprd/Kconfig"
 
 endmenu
diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
index 36452bed86ef..7d156a6dd536 100644
--- a/drivers/soc/Makefile
+++ b/drivers/soc/Makefile
@@ -29,3 +29,4 @@ obj-$(CONFIG_PLAT_VERSATILE)	+= versatile/
 obj-y				+= xilinx/
 obj-$(CONFIG_ARCH_ZX)		+= zte/
 obj-$(CONFIG_SOC_KENDRYTE)	+= kendryte/
+obj-$(CONFIG_ARCH_SPRD)		+= sprd/
diff --git a/drivers/soc/sprd/Kconfig b/drivers/soc/sprd/Kconfig
new file mode 100644
index 000000000000..38d1f5971a28
--- /dev/null
+++ b/drivers/soc/sprd/Kconfig
@@ -0,0 +1,16 @@
+# SPDX-License-Identifier: GPL-2.0-only
+#
+# SPRD SoC drivers
+#
+
+menu "Spreadtrum SoC drivers"
+	depends on ARCH_SPRD || COMPILE_TEST
+
+config SPRD_SYSCON
+	tristate "Spreadtrum syscon support"
+	depends on ARCH_SPRD || COMPILE_TEST
+	help
+	  Say yes here to add support for the Spreadtrum syscon driver,
+	  which is used to implement the atomic method of bits updating.
+
+endmenu
diff --git a/drivers/soc/sprd/Makefile b/drivers/soc/sprd/Makefile
new file mode 100644
index 000000000000..4d7715553cf6
--- /dev/null
+++ b/drivers/soc/sprd/Makefile
@@ -0,0 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0-only
+obj-$(CONFIG_SPRD_SYSCON) += sprd_syscon.o
diff --git a/drivers/soc/sprd/sprd_syscon.c b/drivers/soc/sprd/sprd_syscon.c
new file mode 100644
index 000000000000..0cfd43afeaff
--- /dev/null
+++ b/drivers/soc/sprd/sprd_syscon.c
@@ -0,0 +1,76 @@
+//SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2020 Spreadtrum Communications Inc.
+ */
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mfd/syscon.h>
+#include <linux/regmap.h>
+
+#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;
+}
+
+static int sprd_syscon_read(void *context, unsigned int reg, unsigned int *val)
+{
+	void __iomem *base = context;
+
+	*val = readl(base + reg);
+	return 0;
+}
+
+static int sprd_syscon_write(void *context, unsigned int reg, unsigned int val)
+{
+	void __iomem *base = context;
+
+	writel(val, base + reg);
+	return 0;
+}
+
+static struct regmap_bus sprd_syscon_regmap = {
+	.fast_io = true,
+	.reg_write = sprd_syscon_write,
+	.reg_read = sprd_syscon_read,
+	.reg_update_bits = sprd_syscon_update_bits,
+	.val_format_endian_default = REGMAP_ENDIAN_LITTLE,
+};
+
+static int sprd_syscon_init(void)
+{
+	syscon_register_phys_regmap_bus(&sprd_syscon_regmap);
+
+	return 0;
+}
+core_initcall_sync(sprd_syscon_init);
+
+MODULE_DESCRIPTION("Spreadtrum syscon support");
+MODULE_AUTHOR("Baolin Wang <baolin.wang@unisoc.com>");
+MODULE_LICENSE("GPL v2");
-- 
2.17.1

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

[Arnd Bergmann]

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

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

[Baolin Wang]

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

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

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

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

[Baolin Wang]

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

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

[Arnd Bergmann]

On Thu, Apr 16, 2020 at 3:49 PM Baolin Wang <baolin.wang7@gmail.com> wrote:

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

Ok, sounds good. Maybe also define another compatible string that
is more generic than "sprd,sc9860-syscon" (but less generic than
"syscon") so you can still identify the chip specific syscon area if
necessary, while not having to list each future chip individually.

Something like

compatible = "sprd,sc9860-syscon", "sprd,atomic-syscon", "syscon";

Also I'd add an IS_ENABLED() check so it gets the 'else' path
at compile-time when CONFIG_ARCH_SPRD is disabled.

        Arnd

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

[Baolin Wang]

On Thu, Apr 16, 2020 at 10:38 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Thu, Apr 16, 2020 at 3:49 PM Baolin Wang <baolin.wang7@gmail.com> wrote:
>
> >
> > 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);
>
> Ok, sounds good. Maybe also define another compatible string that
> is more generic than "sprd,sc9860-syscon" (but less generic than
> "syscon") so you can still identify the chip specific syscon area if
> necessary, while not having to list each future chip individually.

OK.

>
> Something like
>
> compatible = "sprd,sc9860-syscon", "sprd,atomic-syscon", "syscon";
>
> Also I'd add an IS_ENABLED() check so it gets the 'else' path
> at compile-time when CONFIG_ARCH_SPRD is disabled.

Sure. Will do in next version. Thanks for your good suggestion.

-- 
Baolin Wang

Re: [RFC PATCH v2 0/3] Add new reg_update_bits() support

[Baolin Wang]

Hi Arnd,

On Mon, Apr 13, 2020 at 2:13 PM Baolin Wang <baolin.wang7@gmail.com> wrote:
>
> The Spreadtrum platform uses a special set/clear method to update
> registers' bits, thus this patch set registers a physical regmap
> bus into syscon core to support this feature instead of using the
> MMIO bus, which is not a physical regmap bus.
>
> Any comments are welcome. Thanks.

I saw Mark had applied patch 2 already. Thanks Mark.

How do you think the syscon changes? As Mark pointed out, we can not
add update_bits() for MMIO regmap bus, which is not a real physical
regmap bus, so I introduced a new helper to allow a  physical regmap
bus to be registered.

And I wonder if you can apply the SPRD SoC driver through your tree if
no objections from your side? Thanks.

> Changes from RFC v1:
>  - Add new helper to registers a physical regmap bus instead of
>  using the MMIO bus.
>
> Baolin Wang (3):
>   mfd: syscon: Support physical regmap bus
>   regmap: Add bus reg_update_bits() support
>   soc: sprd: Add Spreadtrum special bits updating support
>
>  drivers/base/regmap/regmap.c   |  1 +
>  drivers/mfd/syscon.c           | 16 ++++++-
>  drivers/soc/Kconfig            |  1 +
>  drivers/soc/Makefile           |  1 +
>  drivers/soc/sprd/Kconfig       | 16 +++++++
>  drivers/soc/sprd/Makefile      |  2 +
>  drivers/soc/sprd/sprd_syscon.c | 76 ++++++++++++++++++++++++++++++++++
>  include/linux/mfd/syscon.h     |  7 ++++
>  8 files changed, 118 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/soc/sprd/Kconfig
>  create mode 100644 drivers/soc/sprd/Makefile
>  create mode 100644 drivers/soc/sprd/sprd_syscon.c
>
> --
> 2.17.1
>


-- 
Baolin Wang