| Message ID | 96d444cd73239e0166316bd8f44082031cf72491.1587088646.git.baolin.wang7@gmail.com |
|---|---|
| State | New, archived |
| Headers | show |
| Series |
|
| Related | show |
On Fri, 17 Apr 2020, Baolin Wang wrote: > 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 | 23 +++++++++++++++++++++-- > include/linux/mfd/syscon.h | 7 +++++++ > 2 files changed, 28 insertions(+), 2 deletions(-) > > diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c > index 3a97816d0cba..92bfe87038ca 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,25 @@ 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") && Please find a more generic way of supporting your use-case. This is a generic driver, and as such I am vehemently against adding any sort of vendor specific code in here. > + syscon_phy_regmap_bus) > + regmap = regmap_init(NULL, syscon_phy_regmap_bus, base, > + &syscon_config); > + else > + regmap = regmap_init_mmio(NULL, base, &syscon_config);
On Fri, Apr 24, 2020 at 10:11 AM Lee Jones <lee.jones@linaro.org> wrote: > On Fri, 17 Apr 2020, Baolin Wang wrote: > > @@ -106,14 +107,25 @@ 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") && > > Please find a more generic way of supporting your use-case. This is a > generic driver, and as such I am vehemently against adding any sort of > vendor specific code in here. I suggested doing it this way, as all alternatives seemed worse than this. Arnd
On Fri, 24 Apr 2020, Arnd Bergmann wrote: > On Fri, Apr 24, 2020 at 10:11 AM Lee Jones <lee.jones@linaro.org> wrote: > > On Fri, 17 Apr 2020, Baolin Wang wrote: > > > @@ -106,14 +107,25 @@ 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") && > > > > Please find a more generic way of supporting your use-case. This is a > > generic driver, and as such I am vehemently against adding any sort of > > vendor specific code in here. > > I suggested doing it this way, as all alternatives seemed worse than this. If we're using a registration function (could probably be swapped out for or accompanied by a Device Tree property) anyway, then why conduct the vendor platform checks?
On Fri, Apr 24, 2020 at 4:32 PM Lee Jones <lee.jones@linaro.org> wrote: > > On Fri, 24 Apr 2020, Arnd Bergmann wrote: > > > On Fri, Apr 24, 2020 at 10:11 AM Lee Jones <lee.jones@linaro.org> wrote: > > > On Fri, 17 Apr 2020, Baolin Wang wrote: > > > > @@ -106,14 +107,25 @@ 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") && > > > > > > Please find a more generic way of supporting your use-case. This is a > > > generic driver, and as such I am vehemently against adding any sort of > > > vendor specific code in here. > > > > I suggested doing it this way, as all alternatives seemed worse than this. > > If we're using a registration function (could probably be swapped out > for or accompanied by a Device Tree property) anyway, then why conduct > the vendor platform checks? Actually I've send out the v3 patch according to Arnd's suggestion. In v3 patch, I removed the registration function, but we agreed that adding the vendor specific support in the syscon driver seems a better way than others.
On Fri, Apr 24, 2020 at 4:42 PM Baolin Wang <baolin.wang7@gmail.com> wrote: > > On Fri, Apr 24, 2020 at 4:32 PM Lee Jones <lee.jones@linaro.org> wrote: > > > > On Fri, 24 Apr 2020, Arnd Bergmann wrote: > > > > > On Fri, Apr 24, 2020 at 10:11 AM Lee Jones <lee.jones@linaro.org> wrote: > > > > On Fri, 17 Apr 2020, Baolin Wang wrote: > > > > > @@ -106,14 +107,25 @@ 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") && > > > > > > > > Please find a more generic way of supporting your use-case. This is a > > > > generic driver, and as such I am vehemently against adding any sort of > > > > vendor specific code in here. > > > > > > I suggested doing it this way, as all alternatives seemed worse than this. > > > > If we're using a registration function (could probably be swapped out > > for or accompanied by a Device Tree property) anyway, then why conduct > > the vendor platform checks? > > Actually I've send out the v3 patch according to Arnd's suggestion. In > v3 patch, I removed the registration function, but we agreed that > adding the vendor specific support in the syscon driver seems a better > way than others. Sorry, I forgot the link: https://lore.kernel.org/patchwork/patch/1228160/
diff --git a/drivers/mfd/syscon.c b/drivers/mfd/syscon.c index 3a97816d0cba..92bfe87038ca 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,25 @@ 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") && + 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 +184,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__ */
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 | 23 +++++++++++++++++++++-- include/linux/mfd/syscon.h | 7 +++++++ 2 files changed, 28 insertions(+), 2 deletions(-)