| Message ID | 20201107081420.60325-2-damien.lemoal@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | RISC-V Kendryte K210 support improvments | expand |
On Sat, Nov 07, 2020 at 05:13:49PM +0900, Damien Le Moal wrote: > The DesignWare GPIO driver gpio-dwapb ("snps,dw-apb-gpio" or > "apm,xgene-gpio-v2" compatible string) defines the property > "snps,nr-gpios" for the user to specify the number of GPIOs available > on a port. The "-gpios" suffix of this property name ends up being > interpreted as a cell reference when properties are parsed in > of_link_to_suppliers(), leading to 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 property, skipping the search for the supplier and thus avoiding > the device tree parsing error. That's why I have introduced the "ngpios" property support and marked the "snps,nr-gpios" as deprecated here: https://lkml.org/lkml/2020/7/22/1298 to encourage the later one from being used in favor of the first one. So I suggest for you to convert your dts'es (if you have ones) to using the "ngpios" property anyway. > > Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> > --- > drivers/of/property.c | 17 ++++++++++++++++- > 1 file changed, 16 insertions(+), 1 deletion(-) > > diff --git a/drivers/of/property.c b/drivers/of/property.c > index 408a7b5f06a9..d16111c0d6da 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,22 @@ 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) > +{ > + /* > + * Quirck for the DesignWare gpio-dwapb GPIO driver which defines ^ | Quirk? > + * the "snps,nr-gpios" property to indicate the total number of GPIOs > + * available. As this conflict with "xx-gpios" reference properties, > + * ignore it. > + */ > + if (strcmp(prop_name, "snps,nr-gpios") == 0) > + return NULL; > + > + return parse_suffix_prop_cells(np, prop_name, index, > + "-gpios", "#gpio-cells"); > +} > + Personally I'd prefer to convert all the dts-es to using the "ngpios' instead of the vendor-specific property. That's why I haven't fixed the problem the way you suggest in the first place, to encourage people to send the patches with such fixes. Anyway it's up to the OF-subsystem maintainers to decide whether to accept this quirk. -Sergey > static const struct supplier_bindings of_supplier_bindings[] = { > { .parse_prop = parse_clocks, }, > { .parse_prop = parse_interconnects, }, > -- > 2.28.0 >
On Sat, Nov 7, 2020 at 10:14 AM Damien Le Moal <damien.lemoal@wdc.com> wrote: > @@ -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") Sorry, but the above doesn't sound right to me. It's a generic code and you may imagine how many systems you broke by this change.
Hello Andy, On Mon, Nov 09, 2020 at 05:14:21PM +0200, Andy Shevchenko wrote: > On Sat, Nov 7, 2020 at 10:14 AM Damien Le Moal <damien.lemoal@wdc.com> wrote: > > > @@ -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") > > Sorry, but the above doesn't sound right to me. > It's a generic code and you may imagine how many systems you broke by > this change. Damien replaced the macro above with the code below (your removed it from your message): +static struct device_node *parse_gpios(struct device_node *np, + const char *prop_name, int index) +{ + /* + * Quirck for the DesignWare gpio-dwapb GPIO driver which defines + * the "snps,nr-gpios" property to indicate the total number of GPIOs + * available. As this conflict with "xx-gpios" reference properties, + * ignore it. + */ + if (strcmp(prop_name, "snps,nr-gpios") == 0) + return NULL; + + return parse_suffix_prop_cells(np, prop_name, index, + "-gpios", "#gpio-cells"); +} So AFAICS removing the macro shouldn't cause any problem. My concern was whether the quirk has been really needed. As I said the "snps,nr-gpios" property has been marked as deprecated in favor of the standard "ngpios" one. Due to the problem noted by Damien any deprecated property utilization will cause the DW APB SSI DT-nodes probe malfunction. That though implicitly but is supposed to encourage people to provide fixes for the dts-files with the deprecated property replaced with "ngpios". On the other hand an encouragement based on breaking the kernel doesn't seem a good solution. So as I see it either we should accept the solution provided by Damien, or replace it with a series of fixes for all dts-es with DW APB SSI DT-node defined. I suggest to hear the OF-subsystem maintainers out what solution would they prefer. -Sergey > > -- > With Best Regards, > Andy Shevchenko
On Mon, Nov 9, 2020 at 11:45 AM Serge Semin <fancer.lancer@gmail.com> wrote: > > Hello Andy, > > On Mon, Nov 09, 2020 at 05:14:21PM +0200, Andy Shevchenko wrote: > > On Sat, Nov 7, 2020 at 10:14 AM Damien Le Moal <damien.lemoal@wdc.com> wrote: > > > > > @@ -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") > > > > Sorry, but the above doesn't sound right to me. > > It's a generic code and you may imagine how many systems you broke by > > this change. > > Damien replaced the macro above with the code below (your removed it from your > message): > > +static struct device_node *parse_gpios(struct device_node *np, > + const char *prop_name, int index) > +{ > + /* > + * Quirck for the DesignWare gpio-dwapb GPIO driver which defines > + * the "snps,nr-gpios" property to indicate the total number of GPIOs > + * available. As this conflict with "xx-gpios" reference properties, > + * ignore it. > + */ > + if (strcmp(prop_name, "snps,nr-gpios") == 0) > + return NULL; > + > + return parse_suffix_prop_cells(np, prop_name, index, > + "-gpios", "#gpio-cells"); > +} > > So AFAICS removing the macro shouldn't cause any problem. > > My concern was whether the quirk has been really needed. As I said the > "snps,nr-gpios" property has been marked as deprecated in favor of the standard > "ngpios" one. Due to the problem noted by Damien any deprecated property > utilization will cause the DW APB SSI DT-nodes probe malfunction. That > though implicitly but is supposed to encourage people to provide fixes for > the dts-files with the deprecated property replaced with "ngpios". May be deprecated, but we've still got 53 cases in upstream dts files. Plus changing doesn't work well for new DT with old kernels unless we backport 'ngpios' support. > On the other hand an encouragement based on breaking the kernel doesn't seem a > good solution. So as I see it either we should accept the solution provided by > Damien, or replace it with a series of fixes for all dts-es with DW APB SSI > DT-node defined. I suggest to hear the OF-subsystem maintainers out what > solution would they prefer. I'd suggest making failing to parse a warning rather than an error. The devlink stuff is off by default anyways and is still changing. Rob
On 2020/11/10 2:45, Serge Semin wrote: > Hello Andy, > > On Mon, Nov 09, 2020 at 05:14:21PM +0200, Andy Shevchenko wrote: >> On Sat, Nov 7, 2020 at 10:14 AM Damien Le Moal <damien.lemoal@wdc.com> wrote: >> >>> @@ -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") >> >> Sorry, but the above doesn't sound right to me. >> It's a generic code and you may imagine how many systems you broke by >> this change. > > Damien replaced the macro above with the code below (your removed it from your > message): > > +static struct device_node *parse_gpios(struct device_node *np, > + const char *prop_name, int index) > +{ > + /* > + * Quirck for the DesignWare gpio-dwapb GPIO driver which defines > + * the "snps,nr-gpios" property to indicate the total number of GPIOs > + * available. As this conflict with "xx-gpios" reference properties, > + * ignore it. > + */ > + if (strcmp(prop_name, "snps,nr-gpios") == 0) > + return NULL; > + > + return parse_suffix_prop_cells(np, prop_name, index, > + "-gpios", "#gpio-cells"); > +} > > So AFAICS removing the macro shouldn't cause any problem. > > My concern was whether the quirk has been really needed. As I said the > "snps,nr-gpios" property has been marked as deprecated in favor of the standard > "ngpios" one. Due to the problem noted by Damien any deprecated property > utilization will cause the DW APB SSI DT-nodes probe malfunction. That > though implicitly but is supposed to encourage people to provide fixes for > the dts-files with the deprecated property replaced with "ngpios". > > On the other hand an encouragement based on breaking the kernel doesn't seem a > good solution. So as I see it either we should accept the solution provided by > Damien, or replace it with a series of fixes for all dts-es with DW APB SSI > DT-node defined. I suggest to hear the OF-subsystem maintainers out what > solution would they prefer. As Rob mentioned, there are still a lot of DTS out there using "snps,nr-gpios", so I think the fix is needed, albeit with an added warning as Rob suggested so that board maintainers can notice and update their DT. And I can send a patch for the DW gpio apb driver to first try the default "ngpios" property, and if it is not defined, fallback to the legacy "snps,nr-gpios". With that, these new RISC-V boards will not add another use case of the deprecated "snsps,nr-gpios". Does that sound like a good plan ? > > -Sergey > >> >> -- >> With Best Regards, >> Andy Shevchenko >
On Mon, Nov 16, 2020 at 07:30:15AM +0000, Damien Le Moal wrote: > On 2020/11/10 2:45, Serge Semin wrote: > > Hello Andy, > > > > On Mon, Nov 09, 2020 at 05:14:21PM +0200, Andy Shevchenko wrote: > >> On Sat, Nov 7, 2020 at 10:14 AM Damien Le Moal <damien.lemoal@wdc.com> wrote: > >> > >>> @@ -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") > >> > >> Sorry, but the above doesn't sound right to me. > >> It's a generic code and you may imagine how many systems you broke by > >> this change. > > > > Damien replaced the macro above with the code below (your removed it from your > > message): > > > > +static struct device_node *parse_gpios(struct device_node *np, > > + const char *prop_name, int index) > > +{ > > + /* > > + * Quirck for the DesignWare gpio-dwapb GPIO driver which defines > > + * the "snps,nr-gpios" property to indicate the total number of GPIOs > > + * available. As this conflict with "xx-gpios" reference properties, > > + * ignore it. > > + */ > > + if (strcmp(prop_name, "snps,nr-gpios") == 0) > > + return NULL; > > + > > + return parse_suffix_prop_cells(np, prop_name, index, > > + "-gpios", "#gpio-cells"); > > +} > > > > So AFAICS removing the macro shouldn't cause any problem. > > > > My concern was whether the quirk has been really needed. As I said the > > "snps,nr-gpios" property has been marked as deprecated in favor of the standard > > "ngpios" one. Due to the problem noted by Damien any deprecated property > > utilization will cause the DW APB SSI DT-nodes probe malfunction. That > > though implicitly but is supposed to encourage people to provide fixes for > > the dts-files with the deprecated property replaced with "ngpios". > > > > On the other hand an encouragement based on breaking the kernel doesn't seem a > > good solution. So as I see it either we should accept the solution provided by > > Damien, or replace it with a series of fixes for all dts-es with DW APB SSI > > DT-node defined. I suggest to hear the OF-subsystem maintainers out what > > solution would they prefer. > > As Rob mentioned, there are still a lot of DTS out there using "snps,nr-gpios", > so I think the fix is needed, Yes. > albeit with an added warning as Rob suggested so > that board maintainers can notice and update their DT. Yes. > And I can send a patch > for the DW gpio apb driver to first try the default "ngpios" property, and if it > is not defined, fallback to the legacy "snps,nr-gpios". With that, these new > RISC-V boards will not add another use case of the deprecated "snsps,nr-gpios". > Does that sound like a good plan ? It has already been added in 5.10: https://elixir.bootlin.com/linux/v5.10-rc4/source/drivers/gpio/gpio-dwapb.c#L585 so there is no need in sending a patch for the gpio-dwapb.c driver. -Sergey > > > > > > -Sergey > > > >> > >> -- > >> With Best Regards, > >> Andy Shevchenko > > > > > -- > Damien Le Moal > Western Digital Research
diff --git a/drivers/of/property.c b/drivers/of/property.c index 408a7b5f06a9..d16111c0d6da 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,22 @@ 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) +{ + /* + * Quirck for the DesignWare gpio-dwapb GPIO driver which defines + * the "snps,nr-gpios" property to indicate the total number of GPIOs + * available. As this conflict with "xx-gpios" reference properties, + * 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, },
The DesignWare GPIO driver gpio-dwapb ("snps,dw-apb-gpio" or "apm,xgene-gpio-v2" compatible string) defines the property "snps,nr-gpios" for the user to specify the number of GPIOs available on a port. The "-gpios" suffix of this property name ends up being interpreted as a cell reference when properties are parsed in of_link_to_suppliers(), leading to 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 property, 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 | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)