Some platforms such as Spreadtrum platform, define a special method to
update bits of the registers instead of read-modify-write, which means
we should use a physical regmap bus to define the reg_update_bits()
operation instead of the MMIO regmap bus. Thus we can register a new
physical regmap bus into syscon core to support this.
Signed-off-by: Baolin Wang <[email protected]>
---
Changes from v2:
- Fix building errors without enabling CONFIG_ARCH_SPRD.
Changes from v1:
- Add WARN_ONCE() for seting bits and clearing bits at the same time.
- Remove the Spreadtrum SoC syscon driver, instead moving the regmap_bus
instance into syscon.c driver.
Changes from RFC v2:
- Drop regmap change, which was applied by Mark.
- Add more information about how to use set/clear.
- Add checking to ensure the platform is compatible with
using a new physical regmap bus.
Changes from RFC v1:
- Add new helper to registers a physical regmap bus instead of
using the MMIO bus.
---
drivers/mfd/syscon.c | 83 ++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 81 insertions(+), 2 deletions(-)
diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
index 3a97816d0cba..ca91b7770e1a 100644
--- a/drivers/mfd/syscon.c
+++ b/drivers/mfd/syscon.c
@@ -40,6 +40,72 @@ static const struct regmap_config syscon_regmap_config = {
.reg_stride = 4,
};
+#if IS_ENABLED(CONFIG_ARCH_SPRD)
+#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.
+ *
+ * Note: there is a potential risk when users want to set and clear bits
+ * at the same time, since the set/clear method will always do bits setting
+ * before bits clearing, which may cause some unexpected results if the
+ * operation sequence is strict. Thus we recommend that do not set and
+ * clear bits at the same time if you are not sure about the results.
+ */
+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);
+
+ WARN_ONCE(set && clr, "%s: non-atomic update", __func__);
+ 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;
+}
+#endif
+
+static struct regmap_bus sprd_syscon_regmap_bus = {
+#if IS_ENABLED(CONFIG_ARCH_SPRD)
+ .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,
+#endif
+};
+
static struct syscon *of_syscon_register(struct device_node *np, bool check_clk)
{
struct clk *clk;
@@ -50,6 +116,7 @@ static struct syscon *of_syscon_register(struct device_node *np, bool check_clk)
int ret;
struct regmap_config syscon_config = syscon_regmap_config;
struct resource res;
+ bool use_phy_regmap_bus = false;
syscon = kzalloc(sizeof(*syscon), GFP_KERNEL);
if (!syscon)
@@ -106,14 +173,26 @@ 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);
+ /*
+ * The Spreadtrum syscon need register a real physical regmap bus
+ * with new atomic bits updating operation instead of using
+ * read-modify-write.
+ */
+ if (IS_ENABLED(CONFIG_ARCH_SPRD) &&
+ of_device_is_compatible(np, "sprd,atomic-syscon")) {
+ use_phy_regmap_bus = true;
+ regmap = regmap_init(NULL, &sprd_syscon_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 (!use_phy_regmap_bus && check_clk) {
clk = of_clk_get(np, 0);
if (IS_ERR(clk)) {
ret = PTR_ERR(clk);
--
2.17.1
Hi Arnd and Lee,
On Tue, Apr 21, 2020 at 10:13 PM Baolin Wang <[email protected]> wrote:
>
> Some platforms such as Spreadtrum platform, define a special method to
> update bits of the registers instead of read-modify-write, which means
> we should use a physical regmap bus to define the reg_update_bits()
> operation instead of the MMIO regmap bus. Thus we can register a new
> physical regmap bus into syscon core to support this.
>
> Signed-off-by: Baolin Wang <[email protected]>
Do you have any comments for this patch? Thanks.
> ---
> Changes from v2:
> - Fix building errors without enabling CONFIG_ARCH_SPRD.
>
> Changes from v1:
> - Add WARN_ONCE() for seting bits and clearing bits at the same time.
> - Remove the Spreadtrum SoC syscon driver, instead moving the regmap_bus
> instance into syscon.c driver.
>
> Changes from RFC v2:
> - Drop regmap change, which was applied by Mark.
> - Add more information about how to use set/clear.
> - Add checking to ensure the platform is compatible with
> using a new physical regmap bus.
>
> Changes from RFC v1:
> - Add new helper to registers a physical regmap bus instead of
> using the MMIO bus.
> ---
> drivers/mfd/syscon.c | 83 ++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 81 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c
> index 3a97816d0cba..ca91b7770e1a 100644
> --- a/drivers/mfd/syscon.c
> +++ b/drivers/mfd/syscon.c
> @@ -40,6 +40,72 @@ static const struct regmap_config syscon_regmap_config = {
> .reg_stride = 4,
> };
>
> +#if IS_ENABLED(CONFIG_ARCH_SPRD)
> +#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.
> + *
> + * Note: there is a potential risk when users want to set and clear bits
> + * at the same time, since the set/clear method will always do bits setting
> + * before bits clearing, which may cause some unexpected results if the
> + * operation sequence is strict. Thus we recommend that do not set and
> + * clear bits at the same time if you are not sure about the results.
> + */
> +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);
> +
> + WARN_ONCE(set && clr, "%s: non-atomic update", __func__);
> + 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;
> +}
> +#endif
> +
> +static struct regmap_bus sprd_syscon_regmap_bus = {
> +#if IS_ENABLED(CONFIG_ARCH_SPRD)
> + .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,
> +#endif
> +};
> +
> static struct syscon *of_syscon_register(struct device_node *np, bool check_clk)
> {
> struct clk *clk;
> @@ -50,6 +116,7 @@ static struct syscon *of_syscon_register(struct device_node *np, bool check_clk)
> int ret;
> struct regmap_config syscon_config = syscon_regmap_config;
> struct resource res;
> + bool use_phy_regmap_bus = false;
>
> syscon = kzalloc(sizeof(*syscon), GFP_KERNEL);
> if (!syscon)
> @@ -106,14 +173,26 @@ 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);
> + /*
> + * The Spreadtrum syscon need register a real physical regmap bus
> + * with new atomic bits updating operation instead of using
> + * read-modify-write.
> + */
> + if (IS_ENABLED(CONFIG_ARCH_SPRD) &&
> + of_device_is_compatible(np, "sprd,atomic-syscon")) {
> + use_phy_regmap_bus = true;
> + regmap = regmap_init(NULL, &sprd_syscon_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 (!use_phy_regmap_bus && check_clk) {
> clk = of_clk_get(np, 0);
> if (IS_ERR(clk)) {
> ret = PTR_ERR(clk);
> --
> 2.17.1
>
--
Baolin Wang
On Mon, 27 Apr 2020, Baolin Wang wrote:
> Hi Arnd and Lee,
>
> On Tue, Apr 21, 2020 at 10:13 PM Baolin Wang <[email protected]> wrote:
> >
> > Some platforms such as Spreadtrum platform, define a special method to
> > update bits of the registers instead of read-modify-write, which means
> > we should use a physical regmap bus to define the reg_update_bits()
> > operation instead of the MMIO regmap bus. Thus we can register a new
> > physical regmap bus into syscon core to support this.
> >
> > Signed-off-by: Baolin Wang <[email protected]>
>
> Do you have any comments for this patch? Thanks.
Yes. I'm not accepting it, sorry.
I'd rather you duplicate the things you need from of_syscon_register()
in your own driver than taint this one.
> > ---
> > Changes from v2:
> > - Fix building errors without enabling CONFIG_ARCH_SPRD.
> >
> > Changes from v1:
> > - Add WARN_ONCE() for seting bits and clearing bits at the same time.
> > - Remove the Spreadtrum SoC syscon driver, instead moving the regmap_bus
> > instance into syscon.c driver.
> >
> > Changes from RFC v2:
> > - Drop regmap change, which was applied by Mark.
> > - Add more information about how to use set/clear.
> > - Add checking to ensure the platform is compatible with
> > using a new physical regmap bus.
> >
> > Changes from RFC v1:
> > - Add new helper to registers a physical regmap bus instead of
> > using the MMIO bus.
> > ---
> > drivers/mfd/syscon.c | 83 ++++++++++++++++++++++++++++++++++++++++++--
> > 1 file changed, 81 insertions(+), 2 deletions(-)
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Mon, Apr 27, 2020 at 5:05 PM Lee Jones <[email protected]> wrote:
>
> On Mon, 27 Apr 2020, Baolin Wang wrote:
>
> > Hi Arnd and Lee,
> >
> > On Tue, Apr 21, 2020 at 10:13 PM Baolin Wang <[email protected]> wrote:
> > >
> > > Some platforms such as Spreadtrum platform, define a special method to
> > > update bits of the registers instead of read-modify-write, which means
> > > we should use a physical regmap bus to define the reg_update_bits()
> > > operation instead of the MMIO regmap bus. Thus we can register a new
> > > physical regmap bus into syscon core to support this.
> > >
> > > Signed-off-by: Baolin Wang <[email protected]>
> >
> > Do you have any comments for this patch? Thanks.
>
> Yes. I'm not accepting it, sorry.
>
> I'd rather you duplicate the things you need from of_syscon_register()
> in your own driver than taint this one.
Thanks for your comments and I can understand your concern. But we
still want to use the standard syscon APIs in syscon.c, which means we
still need insert an callback or registration or other similar methods
to support vendor specific regmap bus. Otherwise we should invent some
similar syscon APIs in our vendor syscon driver, like
sprd_syscon_regmap_lookup_by_phandle/sprd_syscon_regmap_lookup_by_compatible.
Arnd, what do you think? Thanks.
> > > ---
> > > Changes from v2:
> > > - Fix building errors without enabling CONFIG_ARCH_SPRD.
> > >
> > > Changes from v1:
> > > - Add WARN_ONCE() for seting bits and clearing bits at the same time.
> > > - Remove the Spreadtrum SoC syscon driver, instead moving the regmap_bus
> > > instance into syscon.c driver.
> > >
> > > Changes from RFC v2:
> > > - Drop regmap change, which was applied by Mark.
> > > - Add more information about how to use set/clear.
> > > - Add checking to ensure the platform is compatible with
> > > using a new physical regmap bus.
> > >
> > > Changes from RFC v1:
> > > - Add new helper to registers a physical regmap bus instead of
> > > using the MMIO bus.
> > > ---
> > > drivers/mfd/syscon.c | 83 ++++++++++++++++++++++++++++++++++++++++++--
> > > 1 file changed, 81 insertions(+), 2 deletions(-)
>
> --
> Lee Jones [李琼斯]
> Linaro Services Technical Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog
--
Baolin Wang
On Tue, 28 Apr 2020, Baolin Wang wrote:
> On Mon, Apr 27, 2020 at 5:05 PM Lee Jones <[email protected]> wrote:
> >
> > On Mon, 27 Apr 2020, Baolin Wang wrote:
> >
> > > Hi Arnd and Lee,
> > >
> > > On Tue, Apr 21, 2020 at 10:13 PM Baolin Wang <[email protected]> wrote:
> > > >
> > > > Some platforms such as Spreadtrum platform, define a special method to
> > > > update bits of the registers instead of read-modify-write, which means
> > > > we should use a physical regmap bus to define the reg_update_bits()
> > > > operation instead of the MMIO regmap bus. Thus we can register a new
> > > > physical regmap bus into syscon core to support this.
> > > >
> > > > Signed-off-by: Baolin Wang <[email protected]>
> > >
> > > Do you have any comments for this patch? Thanks.
> >
> > Yes. I'm not accepting it, sorry.
> >
> > I'd rather you duplicate the things you need from of_syscon_register()
> > in your own driver than taint this one.
>
> Thanks for your comments and I can understand your concern. But we
> still want to use the standard syscon APIs in syscon.c, which means we
> still need insert an callback or registration or other similar methods
> to support vendor specific regmap bus. Otherwise we should invent some
> similar syscon APIs in our vendor syscon driver, like
> sprd_syscon_regmap_lookup_by_phandle/sprd_syscon_regmap_lookup_by_compatible.
So long as the generic driver stays generic. Providing a registration
function sounds cleaner than tainting the code with vendor specifics.
> Arnd, what do you think? Thanks.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Tue, Apr 28, 2020 at 3:14 PM Lee Jones <[email protected]> wrote:
>
> On Tue, 28 Apr 2020, Baolin Wang wrote:
>
> > On Mon, Apr 27, 2020 at 5:05 PM Lee Jones <[email protected]> wrote:
> > >
> > > On Mon, 27 Apr 2020, Baolin Wang wrote:
> > >
> > > > Hi Arnd and Lee,
> > > >
> > > > On Tue, Apr 21, 2020 at 10:13 PM Baolin Wang <[email protected]> wrote:
> > > > >
> > > > > Some platforms such as Spreadtrum platform, define a special method to
> > > > > update bits of the registers instead of read-modify-write, which means
> > > > > we should use a physical regmap bus to define the reg_update_bits()
> > > > > operation instead of the MMIO regmap bus. Thus we can register a new
> > > > > physical regmap bus into syscon core to support this.
> > > > >
> > > > > Signed-off-by: Baolin Wang <[email protected]>
> > > >
> > > > Do you have any comments for this patch? Thanks.
> > >
> > > Yes. I'm not accepting it, sorry.
> > >
> > > I'd rather you duplicate the things you need from of_syscon_register()
> > > in your own driver than taint this one.
> >
> > Thanks for your comments and I can understand your concern. But we
> > still want to use the standard syscon APIs in syscon.c, which means we
> > still need insert an callback or registration or other similar methods
> > to support vendor specific regmap bus. Otherwise we should invent some
> > similar syscon APIs in our vendor syscon driver, like
> > sprd_syscon_regmap_lookup_by_phandle/sprd_syscon_regmap_lookup_by_compatible.
>
> So long as the generic driver stays generic. Providing a registration
> function sounds cleaner than tainting the code with vendor specifics.
So seems my V1 patch set [1] was on the direction as you suggested,
but Arnd did not like that.
[1]
https://lore.kernel.org/patchwork/patch/1226161/
https://lore.kernel.org/patchwork/patch/1226162/
--
Baolin Wang
On Tue, 28 Apr 2020, Baolin Wang wrote:
> On Tue, Apr 28, 2020 at 3:14 PM Lee Jones <[email protected]> wrote:
> >
> > On Tue, 28 Apr 2020, Baolin Wang wrote:
> >
> > > On Mon, Apr 27, 2020 at 5:05 PM Lee Jones <[email protected]> wrote:
> > > >
> > > > On Mon, 27 Apr 2020, Baolin Wang wrote:
> > > >
> > > > > Hi Arnd and Lee,
> > > > >
> > > > > On Tue, Apr 21, 2020 at 10:13 PM Baolin Wang <[email protected]> wrote:
> > > > > >
> > > > > > Some platforms such as Spreadtrum platform, define a special method to
> > > > > > update bits of the registers instead of read-modify-write, which means
> > > > > > we should use a physical regmap bus to define the reg_update_bits()
> > > > > > operation instead of the MMIO regmap bus. Thus we can register a new
> > > > > > physical regmap bus into syscon core to support this.
> > > > > >
> > > > > > Signed-off-by: Baolin Wang <[email protected]>
> > > > >
> > > > > Do you have any comments for this patch? Thanks.
> > > >
> > > > Yes. I'm not accepting it, sorry.
> > > >
> > > > I'd rather you duplicate the things you need from of_syscon_register()
> > > > in your own driver than taint this one.
> > >
> > > Thanks for your comments and I can understand your concern. But we
> > > still want to use the standard syscon APIs in syscon.c, which means we
> > > still need insert an callback or registration or other similar methods
> > > to support vendor specific regmap bus. Otherwise we should invent some
> > > similar syscon APIs in our vendor syscon driver, like
> > > sprd_syscon_regmap_lookup_by_phandle/sprd_syscon_regmap_lookup_by_compatible.
> >
> > So long as the generic driver stays generic. Providing a registration
> > function sounds cleaner than tainting the code with vendor specifics.
>
> So seems my V1 patch set [1] was on the direction as you suggested,
> but Arnd did not like that.
>
> [1]
> https://lore.kernel.org/patchwork/patch/1226161/
> https://lore.kernel.org/patchwork/patch/1226162/
I don't often disagree with Arnd, but in this instance I think a
registration function which allows vendor spin-offs to use the generic
API is better than tainting the generic driver by adding vendor
specific #ifery/code to it.
Your original idea seems more palatable to me.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Tue, Apr 28, 2020 at 4:19 PM Lee Jones <[email protected]> wrote:
>
> On Tue, 28 Apr 2020, Baolin Wang wrote:
>
> > On Tue, Apr 28, 2020 at 3:14 PM Lee Jones <[email protected]> wrote:
> > >
> > > On Tue, 28 Apr 2020, Baolin Wang wrote:
> > >
> > > > On Mon, Apr 27, 2020 at 5:05 PM Lee Jones <[email protected]> wrote:
> > > > >
> > > > > On Mon, 27 Apr 2020, Baolin Wang wrote:
> > > > >
> > > > > > Hi Arnd and Lee,
> > > > > >
> > > > > > On Tue, Apr 21, 2020 at 10:13 PM Baolin Wang <[email protected]> wrote:
> > > > > > >
> > > > > > > Some platforms such as Spreadtrum platform, define a special method to
> > > > > > > update bits of the registers instead of read-modify-write, which means
> > > > > > > we should use a physical regmap bus to define the reg_update_bits()
> > > > > > > operation instead of the MMIO regmap bus. Thus we can register a new
> > > > > > > physical regmap bus into syscon core to support this.
> > > > > > >
> > > > > > > Signed-off-by: Baolin Wang <[email protected]>
> > > > > >
> > > > > > Do you have any comments for this patch? Thanks.
> > > > >
> > > > > Yes. I'm not accepting it, sorry.
> > > > >
> > > > > I'd rather you duplicate the things you need from of_syscon_register()
> > > > > in your own driver than taint this one.
> > > >
> > > > Thanks for your comments and I can understand your concern. But we
> > > > still want to use the standard syscon APIs in syscon.c, which means we
> > > > still need insert an callback or registration or other similar methods
> > > > to support vendor specific regmap bus. Otherwise we should invent some
> > > > similar syscon APIs in our vendor syscon driver, like
> > > > sprd_syscon_regmap_lookup_by_phandle/sprd_syscon_regmap_lookup_by_compatible.
> > >
> > > So long as the generic driver stays generic. Providing a registration
> > > function sounds cleaner than tainting the code with vendor specifics.
> >
> > So seems my V1 patch set [1] was on the direction as you suggested,
> > but Arnd did not like that.
> >
> > [1]
> > https://lore.kernel.org/patchwork/patch/1226161/
> > https://lore.kernel.org/patchwork/patch/1226162/
>
> I don't often disagree with Arnd, but in this instance I think a
> registration function which allows vendor spin-offs to use the generic
> API is better than tainting the generic driver by adding vendor
> specific #ifery/code to it.
>
> Your original idea seems more palatable to me.
OK, thanks for sharing your opinion. Let's see what Arnd's opinion
before I send out new version.
--
Baolin Wang
Hi Arnd
On Tue, Apr 28, 2020 at 4:41 PM Baolin Wang <[email protected]> wrote:
>
> On Tue, Apr 28, 2020 at 4:19 PM Lee Jones <[email protected]> wrote:
> >
> > On Tue, 28 Apr 2020, Baolin Wang wrote:
> >
> > > On Tue, Apr 28, 2020 at 3:14 PM Lee Jones <[email protected]> wrote:
> > > >
> > > > On Tue, 28 Apr 2020, Baolin Wang wrote:
> > > >
> > > > > On Mon, Apr 27, 2020 at 5:05 PM Lee Jones <[email protected]> wrote:
> > > > > >
> > > > > > On Mon, 27 Apr 2020, Baolin Wang wrote:
> > > > > >
> > > > > > > Hi Arnd and Lee,
> > > > > > >
> > > > > > > On Tue, Apr 21, 2020 at 10:13 PM Baolin Wang <[email protected]> wrote:
> > > > > > > >
> > > > > > > > Some platforms such as Spreadtrum platform, define a special method to
> > > > > > > > update bits of the registers instead of read-modify-write, which means
> > > > > > > > we should use a physical regmap bus to define the reg_update_bits()
> > > > > > > > operation instead of the MMIO regmap bus. Thus we can register a new
> > > > > > > > physical regmap bus into syscon core to support this.
> > > > > > > >
> > > > > > > > Signed-off-by: Baolin Wang <[email protected]>
> > > > > > >
> > > > > > > Do you have any comments for this patch? Thanks.
> > > > > >
> > > > > > Yes. I'm not accepting it, sorry.
> > > > > >
> > > > > > I'd rather you duplicate the things you need from of_syscon_register()
> > > > > > in your own driver than taint this one.
> > > > >
> > > > > Thanks for your comments and I can understand your concern. But we
> > > > > still want to use the standard syscon APIs in syscon.c, which means we
> > > > > still need insert an callback or registration or other similar methods
> > > > > to support vendor specific regmap bus. Otherwise we should invent some
> > > > > similar syscon APIs in our vendor syscon driver, like
> > > > > sprd_syscon_regmap_lookup_by_phandle/sprd_syscon_regmap_lookup_by_compatible.
> > > >
> > > > So long as the generic driver stays generic. Providing a registration
> > > > function sounds cleaner than tainting the code with vendor specifics.
> > >
> > > So seems my V1 patch set [1] was on the direction as you suggested,
> > > but Arnd did not like that.
> > >
> > > [1]
> > > https://lore.kernel.org/patchwork/patch/1226161/
> > > https://lore.kernel.org/patchwork/patch/1226162/
> >
> > I don't often disagree with Arnd, but in this instance I think a
> > registration function which allows vendor spin-offs to use the generic
> > API is better than tainting the generic driver by adding vendor
> > specific #ifery/code to it.
> >
> > Your original idea seems more palatable to me.
>
> OK, thanks for sharing your opinion. Let's see what Arnd's opinion
> before I send out new version.
Do yo have any comments about how to add new bits updating method? Can
I re-send my v1 patch set [1]? Thanks.
[1]:
https://lore.kernel.org/patchwork/patch/1226161/
https://lore.kernel.org/patchwork/patch/1226162/
--
Baolin Wang
On Mon, 04 May 2020, Baolin Wang wrote:
> Hi Arnd
>
> On Tue, Apr 28, 2020 at 4:41 PM Baolin Wang <[email protected]> wrote:
> >
> > On Tue, Apr 28, 2020 at 4:19 PM Lee Jones <[email protected]> wrote:
> > >
> > > On Tue, 28 Apr 2020, Baolin Wang wrote:
> > >
> > > > On Tue, Apr 28, 2020 at 3:14 PM Lee Jones <[email protected]> wrote:
> > > > >
> > > > > On Tue, 28 Apr 2020, Baolin Wang wrote:
> > > > >
> > > > > > On Mon, Apr 27, 2020 at 5:05 PM Lee Jones <[email protected]> wrote:
> > > > > > >
> > > > > > > On Mon, 27 Apr 2020, Baolin Wang wrote:
> > > > > > >
> > > > > > > > Hi Arnd and Lee,
> > > > > > > >
> > > > > > > > On Tue, Apr 21, 2020 at 10:13 PM Baolin Wang <[email protected]> wrote:
> > > > > > > > >
> > > > > > > > > Some platforms such as Spreadtrum platform, define a special method to
> > > > > > > > > update bits of the registers instead of read-modify-write, which means
> > > > > > > > > we should use a physical regmap bus to define the reg_update_bits()
> > > > > > > > > operation instead of the MMIO regmap bus. Thus we can register a new
> > > > > > > > > physical regmap bus into syscon core to support this.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Baolin Wang <[email protected]>
> > > > > > > >
> > > > > > > > Do you have any comments for this patch? Thanks.
> > > > > > >
> > > > > > > Yes. I'm not accepting it, sorry.
> > > > > > >
> > > > > > > I'd rather you duplicate the things you need from of_syscon_register()
> > > > > > > in your own driver than taint this one.
> > > > > >
> > > > > > Thanks for your comments and I can understand your concern. But we
> > > > > > still want to use the standard syscon APIs in syscon.c, which means we
> > > > > > still need insert an callback or registration or other similar methods
> > > > > > to support vendor specific regmap bus. Otherwise we should invent some
> > > > > > similar syscon APIs in our vendor syscon driver, like
> > > > > > sprd_syscon_regmap_lookup_by_phandle/sprd_syscon_regmap_lookup_by_compatible.
> > > > >
> > > > > So long as the generic driver stays generic. Providing a registration
> > > > > function sounds cleaner than tainting the code with vendor specifics.
> > > >
> > > > So seems my V1 patch set [1] was on the direction as you suggested,
> > > > but Arnd did not like that.
> > > >
> > > > [1]
> > > > https://lore.kernel.org/patchwork/patch/1226161/
> > > > https://lore.kernel.org/patchwork/patch/1226162/
> > >
> > > I don't often disagree with Arnd, but in this instance I think a
> > > registration function which allows vendor spin-offs to use the generic
> > > API is better than tainting the generic driver by adding vendor
> > > specific #ifery/code to it.
> > >
> > > Your original idea seems more palatable to me.
> >
> > OK, thanks for sharing your opinion. Let's see what Arnd's opinion
> > before I send out new version.
>
> Do yo have any comments about how to add new bits updating method? Can
> I re-send my v1 patch set [1]? Thanks.
Just resend and we'll review.
> [1]:
> https://lore.kernel.org/patchwork/patch/1226161/
> https://lore.kernel.org/patchwork/patch/1226162/
>
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Tue, May 19, 2020 at 6:35 PM Lee Jones <[email protected]> wrote:
>
> On Mon, 04 May 2020, Baolin Wang wrote:
>
> > Hi Arnd
> >
> > On Tue, Apr 28, 2020 at 4:41 PM Baolin Wang <[email protected]> wrote:
> > >
> > > On Tue, Apr 28, 2020 at 4:19 PM Lee Jones <[email protected]> wrote:
> > > >
> > > > On Tue, 28 Apr 2020, Baolin Wang wrote:
> > > >
> > > > > On Tue, Apr 28, 2020 at 3:14 PM Lee Jones <[email protected]> wrote:
> > > > > >
> > > > > > On Tue, 28 Apr 2020, Baolin Wang wrote:
> > > > > >
> > > > > > > On Mon, Apr 27, 2020 at 5:05 PM Lee Jones <[email protected]> wrote:
> > > > > > > >
> > > > > > > > On Mon, 27 Apr 2020, Baolin Wang wrote:
> > > > > > > >
> > > > > > > > > Hi Arnd and Lee,
> > > > > > > > >
> > > > > > > > > On Tue, Apr 21, 2020 at 10:13 PM Baolin Wang <[email protected]> wrote:
> > > > > > > > > >
> > > > > > > > > > Some platforms such as Spreadtrum platform, define a special method to
> > > > > > > > > > update bits of the registers instead of read-modify-write, which means
> > > > > > > > > > we should use a physical regmap bus to define the reg_update_bits()
> > > > > > > > > > operation instead of the MMIO regmap bus. Thus we can register a new
> > > > > > > > > > physical regmap bus into syscon core to support this.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Baolin Wang <[email protected]>
> > > > > > > > >
> > > > > > > > > Do you have any comments for this patch? Thanks.
> > > > > > > >
> > > > > > > > Yes. I'm not accepting it, sorry.
> > > > > > > >
> > > > > > > > I'd rather you duplicate the things you need from of_syscon_register()
> > > > > > > > in your own driver than taint this one.
> > > > > > >
> > > > > > > Thanks for your comments and I can understand your concern. But we
> > > > > > > still want to use the standard syscon APIs in syscon.c, which means we
> > > > > > > still need insert an callback or registration or other similar methods
> > > > > > > to support vendor specific regmap bus. Otherwise we should invent some
> > > > > > > similar syscon APIs in our vendor syscon driver, like
> > > > > > > sprd_syscon_regmap_lookup_by_phandle/sprd_syscon_regmap_lookup_by_compatible.
> > > > > >
> > > > > > So long as the generic driver stays generic. Providing a registration
> > > > > > function sounds cleaner than tainting the code with vendor specifics.
> > > > >
> > > > > So seems my V1 patch set [1] was on the direction as you suggested,
> > > > > but Arnd did not like that.
> > > > >
> > > > > [1]
> > > > > https://lore.kernel.org/patchwork/patch/1226161/
> > > > > https://lore.kernel.org/patchwork/patch/1226162/
> > > >
> > > > I don't often disagree with Arnd, but in this instance I think a
> > > > registration function which allows vendor spin-offs to use the generic
> > > > API is better than tainting the generic driver by adding vendor
> > > > specific #ifery/code to it.
> > > >
> > > > Your original idea seems more palatable to me.
> > >
> > > OK, thanks for sharing your opinion. Let's see what Arnd's opinion
> > > before I send out new version.
> >
> > Do yo have any comments about how to add new bits updating method? Can
> > I re-send my v1 patch set [1]? Thanks.
>
> Just resend and we'll review.
Yes, I already sent out the v4 patch set.
https://lore.kernel.org/patchwork/patch/1242814/
https://lore.kernel.org/patchwork/patch/1242815/
--
Baolin Wang