[PATCH v2 0/2] Fix dwapb gpio snps,nr-gpios property handling

[PATCH v2 0/2] Fix dwapb gpio snps,nr-gpios property handling

[Damien Le Moal]

Two patches in this series to fix handling of the "snps,nr-gpios"
property of the Synopsis DW apb gpio controller. Parsing of this
deprecated property triggers an device tree parsing error in
of_link_to_suppliers(). This is fixed in patch 1.

Patch 2 adds a warning message in the dwapb gpio driver to signal that a
device tree still uses this deprecated property.

The first patch is extracted from the series "RISC-V Kendryte K210
support improvments".

Damien Le Moal (2):
  of: Fix property supplier parsing
  gpio: dwapb: warn about deprecated property use

 drivers/gpio/gpio-dwapb.c | 14 +++++++++++---
 drivers/of/property.c     | 16 +++++++++++++++-
 2 files changed, 26 insertions(+), 4 deletions(-)

-- 
2.28.0

[PATCH v2 1/2] of: Fix property supplier parsing

[Damien Le Moal]

The DesignWare gpio-dwapb GPIO driver ("snps,dw-apb-gpio" or
"apm,xgene-gpio-v2" compatible string) defines the now deprecated
property "snps,nr-gpios" to specify the number of GPIOs available
on a port. However, if this property is used in a device tree, its
"-gpios" suffix causes the generic property supplier parsing code to
interpret it as a cell reference when properties are parsed in
of_link_to_suppliers(), leading to an error messages such as:

OF: /soc/bus@50200000/gpio-controller@50200000/gpio-port@0: could not
find phandle

Fix this by manually defining a parse_gpios() function which ignores
this deprecated property that is still present in many device trees,
skipping the search for the supplier and thus avoiding the device tree
parsing error.

Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
---
 drivers/of/property.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/of/property.c b/drivers/of/property.c
index 408a7b5f06a9..4eefe8efc2fe 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -1308,7 +1308,6 @@ DEFINE_SIMPLE_PROP(pinctrl7, "pinctrl-7", NULL)
 DEFINE_SIMPLE_PROP(pinctrl8, "pinctrl-8", NULL)
 DEFINE_SUFFIX_PROP(regulators, "-supply", NULL)
 DEFINE_SUFFIX_PROP(gpio, "-gpio", "#gpio-cells")
-DEFINE_SUFFIX_PROP(gpios, "-gpios", "#gpio-cells")
 
 static struct device_node *parse_iommu_maps(struct device_node *np,
 					    const char *prop_name, int index)
@@ -1319,6 +1318,21 @@ static struct device_node *parse_iommu_maps(struct device_node *np,
 	return of_parse_phandle(np, prop_name, (index * 4) + 1);
 }
 
+static struct device_node *parse_gpios(struct device_node *np,
+				       const char *prop_name, int index)
+{
+	/*
+	 * Quirk for the deprecated "snps,nr-gpios" property of the
+	 * DesignWare gpio-dwapb GPIO driver: as this property parsing
+	 * conflicts with the "xx-gpios" phandle reference property, ignore it.
+	 */
+	if (strcmp(prop_name, "snps,nr-gpios") == 0)
+		return NULL;
+
+	return parse_suffix_prop_cells(np, prop_name, index,
+				       "-gpios", "#gpio-cells");
+}
+
 static const struct supplier_bindings of_supplier_bindings[] = {
 	{ .parse_prop = parse_clocks, },
 	{ .parse_prop = parse_interconnects, },
-- 
2.28.0

[PATCH v2 2/2] gpio: dwapb: warn about deprecated property use

[Damien Le Moal]

In dwapb_gpio_get_pdata(), add a warning to signal the fact that a
device tree is using the deprecated "snps,nr-gpios" property instead of
the standard "ngpios" property.

Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
---
 drivers/gpio/gpio-dwapb.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 2a9046c0fb16..242b058e6630 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -553,7 +553,7 @@ static struct dwapb_platform_data *dwapb_gpio_get_pdata(struct device *dev)
 	struct dwapb_platform_data *pdata;
 	struct dwapb_port_property *pp;
 	int nports;
-	int i;
+	int i, ret;
 
 	nports = device_get_child_node_count(dev);
 	if (nports == 0)
@@ -582,8 +582,16 @@ static struct dwapb_platform_data *dwapb_gpio_get_pdata(struct device *dev)
 			return ERR_PTR(-EINVAL);
 		}
 
-		if (fwnode_property_read_u32(fwnode, "ngpios", &pp->ngpio) &&
-		    fwnode_property_read_u32(fwnode, "snps,nr-gpios", &pp->ngpio)) {
+		ret = fwnode_property_read_u32(fwnode, "ngpios", &pp->ngpio);
+		if (ret) {
+			ret = fwnode_property_read_u32(fwnode, "snps,nr-gpios",
+						       &pp->ngpio);
+			if (!ret) {
+				dev_warn(dev,
+					 "deprecated \"snps,nr-gpios\" property, update device tree to use \"ngpios\".\n");
+			}
+		}
+		if (ret) {
 			dev_info(dev,
 				 "failed to get number of gpios for port%d\n",
 				 i);
-- 
2.28.0

Re: [PATCH v2 2/2] gpio: dwapb: warn about deprecated property use

[Serge Semin]

Hello Damien

On Thu, Nov 19, 2020 at 03:09:21PM +0900, Damien Le Moal wrote:
> In dwapb_gpio_get_pdata(), add a warning to signal the fact that a
> device tree is using the deprecated "snps,nr-gpios" property instead of
> the standard "ngpios" property.

Instead of printing the warning from the driver I'd suggest to do that
from the quirk. That'd be better from maintainability point of view.
So when all the snps,nr-gpios properties are removed from dts'es,
we'll need to discard the quirk only. Otherwise if the warning and
quirk are separated, we may forget to remove the later.

-Sergey

> 
> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
> ---
>  drivers/gpio/gpio-dwapb.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
> index 2a9046c0fb16..242b058e6630 100644
> --- a/drivers/gpio/gpio-dwapb.c
> +++ b/drivers/gpio/gpio-dwapb.c
> @@ -553,7 +553,7 @@ static struct dwapb_platform_data *dwapb_gpio_get_pdata(struct device *dev)
>  	struct dwapb_platform_data *pdata;
>  	struct dwapb_port_property *pp;
>  	int nports;
> -	int i;
> +	int i, ret;
>  
>  	nports = device_get_child_node_count(dev);
>  	if (nports == 0)
> @@ -582,8 +582,16 @@ static struct dwapb_platform_data *dwapb_gpio_get_pdata(struct device *dev)
>  			return ERR_PTR(-EINVAL);
>  		}
>  
> -		if (fwnode_property_read_u32(fwnode, "ngpios", &pp->ngpio) &&
> -		    fwnode_property_read_u32(fwnode, "snps,nr-gpios", &pp->ngpio)) {
> +		ret = fwnode_property_read_u32(fwnode, "ngpios", &pp->ngpio);
> +		if (ret) {
> +			ret = fwnode_property_read_u32(fwnode, "snps,nr-gpios",
> +						       &pp->ngpio);
> +			if (!ret) {
> +				dev_warn(dev,
> +					 "deprecated \"snps,nr-gpios\" property, update device tree to use \"ngpios\".\n");
> +			}
> +		}
> +		if (ret) {
>  			dev_info(dev,
>  				 "failed to get number of gpios for port%d\n",
>  				 i);
> -- 
> 2.28.0
> 

Re: [PATCH v2 1/2] of: Fix property supplier parsing

[Serge Semin]

On Thu, Nov 19, 2020 at 03:09:20PM +0900, Damien Le Moal wrote:
> The DesignWare gpio-dwapb GPIO driver ("snps,dw-apb-gpio" or
> "apm,xgene-gpio-v2" compatible string) defines the now deprecated
> property "snps,nr-gpios" to specify the number of GPIOs available
> on a port. However, if this property is used in a device tree, its
> "-gpios" suffix causes the generic property supplier parsing code to
> interpret it as a cell reference when properties are parsed in
> of_link_to_suppliers(), leading to an error messages such as:
> 
> OF: /soc/bus@50200000/gpio-controller@50200000/gpio-port@0: could not
> find phandle
> 
> Fix this by manually defining a parse_gpios() function which ignores
> this deprecated property that is still present in many device trees,
> skipping the search for the supplier and thus avoiding the device tree
> parsing error.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
> ---
>  drivers/of/property.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index 408a7b5f06a9..4eefe8efc2fe 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -1308,7 +1308,6 @@ DEFINE_SIMPLE_PROP(pinctrl7, "pinctrl-7", NULL)
>  DEFINE_SIMPLE_PROP(pinctrl8, "pinctrl-8", NULL)
>  DEFINE_SUFFIX_PROP(regulators, "-supply", NULL)
>  DEFINE_SUFFIX_PROP(gpio, "-gpio", "#gpio-cells")
> -DEFINE_SUFFIX_PROP(gpios, "-gpios", "#gpio-cells")
>  
>  static struct device_node *parse_iommu_maps(struct device_node *np,
>  					    const char *prop_name, int index)
> @@ -1319,6 +1318,21 @@ static struct device_node *parse_iommu_maps(struct device_node *np,
>  	return of_parse_phandle(np, prop_name, (index * 4) + 1);
>  }
>  
> +static struct device_node *parse_gpios(struct device_node *np,
> +				       const char *prop_name, int index)
> +{
> +	/*
> +	 * Quirk for the deprecated "snps,nr-gpios" property of the
> +	 * DesignWare gpio-dwapb GPIO driver: as this property parsing
> +	 * conflicts with the "xx-gpios" phandle reference property, ignore it.
> +	 */

> +	if (strcmp(prop_name, "snps,nr-gpios") == 0)
> +		return NULL;

What about printing the warning from instead of doing that from the driver?
Like this:

+	if (strcmp(prop_name, "snps,nr-gpios") == 0) {
+		pr_warn("%pOF: %s is deprecated in favor of ngpios\n");
+		return NULL;
+	}

So when the property is removed from all dts'es we wouldn't
forget to discard the quirk?

-Sergey

> +
> +	return parse_suffix_prop_cells(np, prop_name, index,
> +				       "-gpios", "#gpio-cells");
> +}
> +
>  static const struct supplier_bindings of_supplier_bindings[] = {
>  	{ .parse_prop = parse_clocks, },
>  	{ .parse_prop = parse_interconnects, },
> -- 
> 2.28.0
> 

Re: [PATCH v2 1/2] of: Fix property supplier parsing

[Rob Herring]

On Wed, Nov 25, 2020 at 2:06 PM Serge Semin <fancer.lancer@gmail.com> wrote:
>
> On Thu, Nov 19, 2020 at 03:09:20PM +0900, Damien Le Moal wrote:
> > The DesignWare gpio-dwapb GPIO driver ("snps,dw-apb-gpio" or
> > "apm,xgene-gpio-v2" compatible string) defines the now deprecated
> > property "snps,nr-gpios" to specify the number of GPIOs available
> > on a port. However, if this property is used in a device tree, its
> > "-gpios" suffix causes the generic property supplier parsing code to
> > interpret it as a cell reference when properties are parsed in
> > of_link_to_suppliers(), leading to an error messages such as:
> >
> > OF: /soc/bus@50200000/gpio-controller@50200000/gpio-port@0: could not
> > find phandle
> >
> > Fix this by manually defining a parse_gpios() function which ignores
> > this deprecated property that is still present in many device trees,
> > skipping the search for the supplier and thus avoiding the device tree
> > parsing error.
> >
> > Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
> > ---
> >  drivers/of/property.c | 16 +++++++++++++++-
> >  1 file changed, 15 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/of/property.c b/drivers/of/property.c
> > index 408a7b5f06a9..4eefe8efc2fe 100644
> > --- a/drivers/of/property.c
> > +++ b/drivers/of/property.c
> > @@ -1308,7 +1308,6 @@ DEFINE_SIMPLE_PROP(pinctrl7, "pinctrl-7", NULL)
> >  DEFINE_SIMPLE_PROP(pinctrl8, "pinctrl-8", NULL)
> >  DEFINE_SUFFIX_PROP(regulators, "-supply", NULL)
> >  DEFINE_SUFFIX_PROP(gpio, "-gpio", "#gpio-cells")
> > -DEFINE_SUFFIX_PROP(gpios, "-gpios", "#gpio-cells")
> >
> >  static struct device_node *parse_iommu_maps(struct device_node *np,
> >                                           const char *prop_name, int index)
> > @@ -1319,6 +1318,21 @@ static struct device_node *parse_iommu_maps(struct device_node *np,
> >       return of_parse_phandle(np, prop_name, (index * 4) + 1);
> >  }
> >
> > +static struct device_node *parse_gpios(struct device_node *np,
> > +                                    const char *prop_name, int index)
> > +{
> > +     /*
> > +      * Quirk for the deprecated "snps,nr-gpios" property of the
> > +      * DesignWare gpio-dwapb GPIO driver: as this property parsing
> > +      * conflicts with the "xx-gpios" phandle reference property, ignore it.
> > +      */
>
> > +     if (strcmp(prop_name, "snps,nr-gpios") == 0)
> > +             return NULL;
>
> What about printing the warning from instead of doing that from the driver?
> Like this:
>
> +       if (strcmp(prop_name, "snps,nr-gpios") == 0) {
> +               pr_warn("%pOF: %s is deprecated in favor of ngpios\n");
> +               return NULL;
> +       }
>
> So when the property is removed from all dts'es we wouldn't
> forget to discard the quirk?

Why do we need this change at all? We've already got a message printed
and devlink is still default off. If someone cares about devlink and
the error message, then they should fix their dts file.

Now if there's a stable/mature platform using "snps,nr-gpios" and we
enable devlink by default (which needs to happen at some point), then
I'd feel differently and we'll need to handle this. But until then, I
don't want to carry this quirk.

Rob

Re: [PATCH v2 1/2] of: Fix property supplier parsing

[Damien Le Moal]

On 2020/12/01 3:22, Rob Herring wrote:
> On Wed, Nov 25, 2020 at 2:06 PM Serge Semin <fancer.lancer@gmail.com> wrote:
>>
>> On Thu, Nov 19, 2020 at 03:09:20PM +0900, Damien Le Moal wrote:
>>> The DesignWare gpio-dwapb GPIO driver ("snps,dw-apb-gpio" or
>>> "apm,xgene-gpio-v2" compatible string) defines the now deprecated
>>> property "snps,nr-gpios" to specify the number of GPIOs available
>>> on a port. However, if this property is used in a device tree, its
>>> "-gpios" suffix causes the generic property supplier parsing code to
>>> interpret it as a cell reference when properties are parsed in
>>> of_link_to_suppliers(), leading to an error messages such as:
>>>
>>> OF: /soc/bus@50200000/gpio-controller@50200000/gpio-port@0: could not
>>> find phandle
>>>
>>> Fix this by manually defining a parse_gpios() function which ignores
>>> this deprecated property that is still present in many device trees,
>>> skipping the search for the supplier and thus avoiding the device tree
>>> parsing error.
>>>
>>> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
>>> ---
>>>  drivers/of/property.c | 16 +++++++++++++++-
>>>  1 file changed, 15 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/of/property.c b/drivers/of/property.c
>>> index 408a7b5f06a9..4eefe8efc2fe 100644
>>> --- a/drivers/of/property.c
>>> +++ b/drivers/of/property.c
>>> @@ -1308,7 +1308,6 @@ DEFINE_SIMPLE_PROP(pinctrl7, "pinctrl-7", NULL)
>>>  DEFINE_SIMPLE_PROP(pinctrl8, "pinctrl-8", NULL)
>>>  DEFINE_SUFFIX_PROP(regulators, "-supply", NULL)
>>>  DEFINE_SUFFIX_PROP(gpio, "-gpio", "#gpio-cells")
>>> -DEFINE_SUFFIX_PROP(gpios, "-gpios", "#gpio-cells")
>>>
>>>  static struct device_node *parse_iommu_maps(struct device_node *np,
>>>                                           const char *prop_name, int index)
>>> @@ -1319,6 +1318,21 @@ static struct device_node *parse_iommu_maps(struct device_node *np,
>>>       return of_parse_phandle(np, prop_name, (index * 4) + 1);
>>>  }
>>>
>>> +static struct device_node *parse_gpios(struct device_node *np,
>>> +                                    const char *prop_name, int index)
>>> +{
>>> +     /*
>>> +      * Quirk for the deprecated "snps,nr-gpios" property of the
>>> +      * DesignWare gpio-dwapb GPIO driver: as this property parsing
>>> +      * conflicts with the "xx-gpios" phandle reference property, ignore it.
>>> +      */
>>
>>> +     if (strcmp(prop_name, "snps,nr-gpios") == 0)
>>> +             return NULL;
>>
>> What about printing the warning from instead of doing that from the driver?
>> Like this:
>>
>> +       if (strcmp(prop_name, "snps,nr-gpios") == 0) {
>> +               pr_warn("%pOF: %s is deprecated in favor of ngpios\n");
>> +               return NULL;
>> +       }
>>
>> So when the property is removed from all dts'es we wouldn't
>> forget to discard the quirk?
> 
> Why do we need this change at all? We've already got a message printed
> and devlink is still default off. If someone cares about devlink and
> the error message, then they should fix their dts file.
> 
> Now if there's a stable/mature platform using "snps,nr-gpios" and we
> enable devlink by default (which needs to happen at some point), then
> I'd feel differently and we'll need to handle this. But until then, I
> don't want to carry this quirk.

I have the device tree fixed for my use case, so there is no problem anymore.
The improvement this patch brings is a clearer error message. The one that
shows up without the patch is fairly obscure and it took me a while to figure
out what was wrong. But again, no problems for me since the DT is fixed. So
sure, we can drop this patch.

> 
> Rob
> 


-- 
Damien Le Moal
Western Digital Research