* fw_devlink on will break all snps,dw-apb-gpio users
@ 2020-10-14 11:12 Jisheng Zhang
2020-10-14 17:29 ` Saravana Kannan
0 siblings, 1 reply; 10+ messages in thread
From: Jisheng Zhang @ 2020-10-14 11:12 UTC (permalink / raw)
To: Saravana Kannan, Greg Kroah-Hartman, Rafael J. Wysocki,
linux-kernel, Rob Herring, Frank Rowand, devicetree,
linux-arm-kernel
Hi,
If set fw_devlink as on, any consumers of dw apb gpio won't probe.
The related dts looks like:
gpio0: gpio@2400 {
compatible = "snps,dw-apb-gpio";
#address-cells = <1>;
#size-cells = <0>;
porta: gpio-port@0 {
compatible = "snps,dw-apb-gpio-port";
gpio-controller;
#gpio-cells = <2>;
ngpios = <32>;
reg = <0>;
};
};
device_foo {
status = "okay"
...;
reset-gpio = <&porta, 0, GPIO_ACTIVE_HIGH>;
};
If I change the reset-gpio property to use another kind of gpio phandle,
e.g gpio expander, then device_foo can be probed successfully.
The gpio expander dt node looks like:
expander3: gpio@44 {
compatible = "fcs,fxl6408";
pinctrl-names = "default";
pinctrl-0 = <&expander3_pmux>;
reg = <0x44>;
gpio-controller;
#gpio-cells = <2>;
interrupt-parent = <&portb>;
interrupts = <23 IRQ_TYPE_NONE>;
interrupt-controller;
#interrupt-cells = <2>;
};
The common pattern looks like the devlink can't cope with suppliers from
child dt node.
Any suggestions?
Thanks in advance,
Jisheng
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: fw_devlink on will break all snps,dw-apb-gpio users 2020-10-14 11:12 fw_devlink on will break all snps,dw-apb-gpio users Jisheng Zhang @ 2020-10-14 17:29 ` Saravana Kannan 2020-10-15 4:02 ` Jisheng Zhang 0 siblings, 1 reply; 10+ messages in thread From: Saravana Kannan @ 2020-10-14 17:29 UTC (permalink / raw) To: Jisheng Zhang Cc: Greg Kroah-Hartman, Rafael J. Wysocki, LKML, Rob Herring, Frank Rowand, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, linux-arm-kernel On Wed, Oct 14, 2020 at 4:12 AM Jisheng Zhang <Jisheng.Zhang@synaptics.com> wrote: > > Hi, > > If set fw_devlink as on, any consumers of dw apb gpio won't probe. > > The related dts looks like: > > gpio0: gpio@2400 { > compatible = "snps,dw-apb-gpio"; > #address-cells = <1>; > #size-cells = <0>; > > porta: gpio-port@0 { > compatible = "snps,dw-apb-gpio-port"; > gpio-controller; > #gpio-cells = <2>; > ngpios = <32>; > reg = <0>; > }; > }; > > device_foo { > status = "okay" > ...; > reset-gpio = <&porta, 0, GPIO_ACTIVE_HIGH>; > }; > > If I change the reset-gpio property to use another kind of gpio phandle, > e.g gpio expander, then device_foo can be probed successfully. > > The gpio expander dt node looks like: > > expander3: gpio@44 { > compatible = "fcs,fxl6408"; > pinctrl-names = "default"; > pinctrl-0 = <&expander3_pmux>; > reg = <0x44>; > gpio-controller; > #gpio-cells = <2>; > interrupt-parent = <&portb>; > interrupts = <23 IRQ_TYPE_NONE>; > interrupt-controller; > #interrupt-cells = <2>; > }; > > The common pattern looks like the devlink can't cope with suppliers from > child dt node. fw_devlink doesn't have any problem dealing with child devices being suppliers. The problem with your case is that the drivers/gpio/gpio-dwapb.c driver directly parses the child nodes and never creates struct devices for them. If you have a node with compatible string, fw_devlink expects you to create and probe a struct device for it. So change your driver to add the child devices as devices instead of just parsing the node directly and doing stuff with it. Either that, or stop putting "compatible" string in a node if you don't plan to actually treat it as a device -- but that's too late for this driver (it needs to be backward compatible). So change the driver to add of_platform_populate() and write a driver that probes "snps,dw-apb-gpio-port". -Saravana ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: fw_devlink on will break all snps,dw-apb-gpio users 2020-10-14 17:29 ` Saravana Kannan @ 2020-10-15 4:02 ` Jisheng Zhang 2020-10-15 5:04 ` Saravana Kannan 0 siblings, 1 reply; 10+ messages in thread From: Jisheng Zhang @ 2020-10-15 4:02 UTC (permalink / raw) To: Saravana Kannan Cc: Greg Kroah-Hartman, Rafael J. Wysocki, LKML, Rob Herring, Frank Rowand, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, linux-arm-kernel On Wed, 14 Oct 2020 10:29:36 -0700 Saravana Kannan <saravanak@google.com> wrote: > CAUTION: Email originated externally, do not click links or open attachments unless you recognize the sender and know the content is safe. > > > On Wed, Oct 14, 2020 at 4:12 AM Jisheng Zhang > <Jisheng.Zhang@synaptics.com> wrote: > > > > Hi, > > > > If set fw_devlink as on, any consumers of dw apb gpio won't probe. > > > > The related dts looks like: > > > > gpio0: gpio@2400 { > > compatible = "snps,dw-apb-gpio"; > > #address-cells = <1>; > > #size-cells = <0>; > > > > porta: gpio-port@0 { > > compatible = "snps,dw-apb-gpio-port"; > > gpio-controller; > > #gpio-cells = <2>; > > ngpios = <32>; > > reg = <0>; > > }; > > }; > > > > device_foo { > > status = "okay" > > ...; > > reset-gpio = <&porta, 0, GPIO_ACTIVE_HIGH>; > > }; > > > > If I change the reset-gpio property to use another kind of gpio phandle, > > e.g gpio expander, then device_foo can be probed successfully. > > > > The gpio expander dt node looks like: > > > > expander3: gpio@44 { > > compatible = "fcs,fxl6408"; > > pinctrl-names = "default"; > > pinctrl-0 = <&expander3_pmux>; > > reg = <0x44>; > > gpio-controller; > > #gpio-cells = <2>; > > interrupt-parent = <&portb>; > > interrupts = <23 IRQ_TYPE_NONE>; > > interrupt-controller; > > #interrupt-cells = <2>; > > }; > > > > The common pattern looks like the devlink can't cope with suppliers from > > child dt node. > > fw_devlink doesn't have any problem dealing with child devices being > suppliers. The problem with your case is that the > drivers/gpio/gpio-dwapb.c driver directly parses the child nodes and > never creates struct devices for them. If you have a node with > compatible string, fw_devlink expects you to create and probe a struct > device for it. So change your driver to add the child devices as > devices instead of just parsing the node directly and doing stuff with > it. > > Either that, or stop putting "compatible" string in a node if you > don't plan to actually treat it as a device -- but that's too late for > this driver (it needs to be backward compatible). So change the driver > to add of_platform_populate() and write a driver that probes > "snps,dw-apb-gpio-port". > Thanks for the information. The "snps,dw-apb-gpio-port" is never used, so I just sent out a series to remove it. Thanks ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: fw_devlink on will break all snps,dw-apb-gpio users 2020-10-15 4:02 ` Jisheng Zhang @ 2020-10-15 5:04 ` Saravana Kannan 2020-10-15 8:14 ` Jisheng Zhang 0 siblings, 1 reply; 10+ messages in thread From: Saravana Kannan @ 2020-10-15 5:04 UTC (permalink / raw) To: Jisheng Zhang Cc: Greg Kroah-Hartman, Rafael J. Wysocki, LKML, Rob Herring, Frank Rowand, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, linux-arm-kernel On Wed, Oct 14, 2020 at 9:02 PM Jisheng Zhang <Jisheng.Zhang@synaptics.com> wrote: > > On Wed, 14 Oct 2020 10:29:36 -0700 > Saravana Kannan <saravanak@google.com> wrote: > > > CAUTION: Email originated externally, do not click links or open attachments unless you recognize the sender and know the content is safe. > > > > > > On Wed, Oct 14, 2020 at 4:12 AM Jisheng Zhang > > <Jisheng.Zhang@synaptics.com> wrote: > > > > > > Hi, > > > > > > If set fw_devlink as on, any consumers of dw apb gpio won't probe. > > > > > > The related dts looks like: > > > > > > gpio0: gpio@2400 { > > > compatible = "snps,dw-apb-gpio"; > > > #address-cells = <1>; > > > #size-cells = <0>; > > > > > > porta: gpio-port@0 { > > > compatible = "snps,dw-apb-gpio-port"; > > > gpio-controller; > > > #gpio-cells = <2>; > > > ngpios = <32>; > > > reg = <0>; > > > }; > > > }; > > > > > > device_foo { > > > status = "okay" > > > ...; > > > reset-gpio = <&porta, 0, GPIO_ACTIVE_HIGH>; > > > }; > > > > > > If I change the reset-gpio property to use another kind of gpio phandle, > > > e.g gpio expander, then device_foo can be probed successfully. > > > > > > The gpio expander dt node looks like: > > > > > > expander3: gpio@44 { > > > compatible = "fcs,fxl6408"; > > > pinctrl-names = "default"; > > > pinctrl-0 = <&expander3_pmux>; > > > reg = <0x44>; > > > gpio-controller; > > > #gpio-cells = <2>; > > > interrupt-parent = <&portb>; > > > interrupts = <23 IRQ_TYPE_NONE>; > > > interrupt-controller; > > > #interrupt-cells = <2>; > > > }; > > > > > > The common pattern looks like the devlink can't cope with suppliers from > > > child dt node. > > > > fw_devlink doesn't have any problem dealing with child devices being > > suppliers. The problem with your case is that the > > drivers/gpio/gpio-dwapb.c driver directly parses the child nodes and > > never creates struct devices for them. If you have a node with > > compatible string, fw_devlink expects you to create and probe a struct > > device for it. So change your driver to add the child devices as > > devices instead of just parsing the node directly and doing stuff with > > it. > > > > Either that, or stop putting "compatible" string in a node if you > > don't plan to actually treat it as a device -- but that's too late for > > this driver (it needs to be backward compatible). So change the driver > > to add of_platform_populate() and write a driver that probes > > "snps,dw-apb-gpio-port". > > > > Thanks for the information. The "snps,dw-apb-gpio-port" is never used, > so I just sent out a series to remove it. I'd actually prefer that you fix the kernel code to actually use it. So that fw_devlink can be backward compatible (Older DT + new kernel). The change is pretty trivial (I just have time to do it for you). -Saravana ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: fw_devlink on will break all snps,dw-apb-gpio users 2020-10-15 5:04 ` Saravana Kannan @ 2020-10-15 8:14 ` Jisheng Zhang 2020-10-15 8:48 ` Saravana Kannan 0 siblings, 1 reply; 10+ messages in thread From: Jisheng Zhang @ 2020-10-15 8:14 UTC (permalink / raw) To: Saravana Kannan, Rob Herring, Linus Walleij Cc: Greg Kroah-Hartman, Rafael J. Wysocki, LKML, Frank Rowand, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, linux-arm-kernel On Wed, 14 Oct 2020 22:04:24 -0700 Saravana Kannan wrote: > > > On Wed, Oct 14, 2020 at 9:02 PM Jisheng Zhang > <Jisheng.Zhang@synaptics.com> wrote: > > > > On Wed, 14 Oct 2020 10:29:36 -0700 > > Saravana Kannan <saravanak@google.com> wrote: > > > > > > > > > > > On Wed, Oct 14, 2020 at 4:12 AM Jisheng Zhang > > > <Jisheng.Zhang@synaptics.com> wrote: > > > > > > > > Hi, > > > > > > > > If set fw_devlink as on, any consumers of dw apb gpio won't probe. > > > > > > > > The related dts looks like: > > > > > > > > gpio0: gpio@2400 { > > > > compatible = "snps,dw-apb-gpio"; > > > > #address-cells = <1>; > > > > #size-cells = <0>; > > > > > > > > porta: gpio-port@0 { > > > > compatible = "snps,dw-apb-gpio-port"; > > > > gpio-controller; > > > > #gpio-cells = <2>; > > > > ngpios = <32>; > > > > reg = <0>; > > > > }; > > > > }; > > > > > > > > device_foo { > > > > status = "okay" > > > > ...; > > > > reset-gpio = <&porta, 0, GPIO_ACTIVE_HIGH>; > > > > }; > > > > > > > > If I change the reset-gpio property to use another kind of gpio phandle, > > > > e.g gpio expander, then device_foo can be probed successfully. > > > > > > > > The gpio expander dt node looks like: > > > > > > > > expander3: gpio@44 { > > > > compatible = "fcs,fxl6408"; > > > > pinctrl-names = "default"; > > > > pinctrl-0 = <&expander3_pmux>; > > > > reg = <0x44>; > > > > gpio-controller; > > > > #gpio-cells = <2>; > > > > interrupt-parent = <&portb>; > > > > interrupts = <23 IRQ_TYPE_NONE>; > > > > interrupt-controller; > > > > #interrupt-cells = <2>; > > > > }; > > > > > > > > The common pattern looks like the devlink can't cope with suppliers from > > > > child dt node. > > > > > > fw_devlink doesn't have any problem dealing with child devices being > > > suppliers. The problem with your case is that the > > > drivers/gpio/gpio-dwapb.c driver directly parses the child nodes and > > > never creates struct devices for them. If you have a node with > > > compatible string, fw_devlink expects you to create and probe a struct > > > device for it. So change your driver to add the child devices as > > > devices instead of just parsing the node directly and doing stuff with > > > it. > > > > > > Either that, or stop putting "compatible" string in a node if you > > > don't plan to actually treat it as a device -- but that's too late for > > > this driver (it needs to be backward compatible). So change the driver > > > to add of_platform_populate() and write a driver that probes > > > "snps,dw-apb-gpio-port". > > > > > > > Thanks for the information. The "snps,dw-apb-gpio-port" is never used, > > so I just sent out a series to remove it. > > I'd actually prefer that you fix the kernel code to actually use it. > So that fw_devlink can be backward compatible (Older DT + new kernel). > The change is pretty trivial (I just have time to do it for you). > I agree the change is trivial, but it will add some useless LoCs like below. I'm not sure whether this is acceptable.So add GPIO and DT maintainers to comment. Hi Linus, Rob, Could you please comment? A simple introduction of the problem: As pointed out by Saravana, "gpio-dwapb.c driver directly parses the child nodes and never creates struct devices for them. If you have a node with compatible string, fw_devlink expects you to create and probe a struct device for it", so once we set fw_devlink=on, then any users of gpio-dwapb as below won't be probed. device_foo { status = "okay" ...; reset-gpio = <&porta, 0, GPIO_ACTIVE_HIGH>; }; The compatible string "snps,dw-apb-gpio-port" is never used, but it's in the dt-binding since the dw gpio mainlined. I believe the every dw apb users just copy the compatible string in to soc dtsi. So I submit a series to remove the unused "snps,dw-apb-gpio-port" https://lkml.org/lkml/2020/10/14/1186 But this will break Older DT + new kernel with fw_devlink on. Which solution is better? If the following patch is acceptable, I can submit it once 5.10-rc1 is out. thanks diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c index 1d8d55bd63aa..b8e012e48b59 100644 --- a/drivers/gpio/gpio-dwapb.c +++ b/drivers/gpio/gpio-dwapb.c @@ -19,6 +19,7 @@ #include <linux/of_address.h> #include <linux/of_device.h> #include <linux/of_irq.h> +#include <linux/of_platform.h> #include <linux/platform_device.h> #include <linux/property.h> #include <linux/reset.h> @@ -694,6 +695,10 @@ static int dwapb_gpio_probe(struct platform_device *pdev) } platform_set_drvdata(pdev, gpio); + err = devm_of_platform_populate(dev); + if (err) + goto out_unregister; + return 0; out_unregister: @@ -820,6 +825,25 @@ static struct platform_driver dwapb_gpio_driver = { module_platform_driver(dwapb_gpio_driver); +static const struct of_device_id dwapb_port_of_match[] = { + { .compatible = "snps,dw-apb-gpio-port" }, + { /* Sentinel */ } +}; + +static int dwapb_gpio_port_probe(struct platform_device *pdev) +{ + return 0; +} + +static struct platform_driver dwapb_gpio_port_driver = { + .driver = { + .name = "gpio-dwapb-port", + .of_match_table = dwapb_port_of_match, + }, + .probe = dwapb_gpio_port_probe, +}; +module_platform_driver(dwapb_gpio_port_driver); + MODULE_LICENSE("GPL"); MODULE_AUTHOR("Jamie Iles"); MODULE_DESCRIPTION("Synopsys DesignWare APB GPIO driver"); ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: fw_devlink on will break all snps,dw-apb-gpio users 2020-10-15 8:14 ` Jisheng Zhang @ 2020-10-15 8:48 ` Saravana Kannan 2020-10-15 9:52 ` Jisheng Zhang 0 siblings, 1 reply; 10+ messages in thread From: Saravana Kannan @ 2020-10-15 8:48 UTC (permalink / raw) To: Jisheng Zhang Cc: Rob Herring, Linus Walleij, Greg Kroah-Hartman, Rafael J. Wysocki, LKML, Frank Rowand, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, linux-arm-kernel On Thu, Oct 15, 2020 at 1:15 AM Jisheng Zhang <Jisheng.Zhang@synaptics.com> wrote: > > On Wed, 14 Oct 2020 22:04:24 -0700 Saravana Kannan wrote: > > > > > > > On Wed, Oct 14, 2020 at 9:02 PM Jisheng Zhang > > <Jisheng.Zhang@synaptics.com> wrote: > > > > > > On Wed, 14 Oct 2020 10:29:36 -0700 > > > Saravana Kannan <saravanak@google.com> wrote: > > > > > > > > > > > > > > > On Wed, Oct 14, 2020 at 4:12 AM Jisheng Zhang > > > > <Jisheng.Zhang@synaptics.com> wrote: > > > > > > > > > > Hi, > > > > > > > > > > If set fw_devlink as on, any consumers of dw apb gpio won't probe. > > > > > > > > > > The related dts looks like: > > > > > > > > > > gpio0: gpio@2400 { > > > > > compatible = "snps,dw-apb-gpio"; > > > > > #address-cells = <1>; > > > > > #size-cells = <0>; > > > > > > > > > > porta: gpio-port@0 { > > > > > compatible = "snps,dw-apb-gpio-port"; > > > > > gpio-controller; > > > > > #gpio-cells = <2>; > > > > > ngpios = <32>; > > > > > reg = <0>; > > > > > }; > > > > > }; > > > > > > > > > > device_foo { > > > > > status = "okay" > > > > > ...; > > > > > reset-gpio = <&porta, 0, GPIO_ACTIVE_HIGH>; > > > > > }; > > > > > > > > > > If I change the reset-gpio property to use another kind of gpio phandle, > > > > > e.g gpio expander, then device_foo can be probed successfully. > > > > > > > > > > The gpio expander dt node looks like: > > > > > > > > > > expander3: gpio@44 { > > > > > compatible = "fcs,fxl6408"; > > > > > pinctrl-names = "default"; > > > > > pinctrl-0 = <&expander3_pmux>; > > > > > reg = <0x44>; > > > > > gpio-controller; > > > > > #gpio-cells = <2>; > > > > > interrupt-parent = <&portb>; > > > > > interrupts = <23 IRQ_TYPE_NONE>; > > > > > interrupt-controller; > > > > > #interrupt-cells = <2>; > > > > > }; > > > > > > > > > > The common pattern looks like the devlink can't cope with suppliers from > > > > > child dt node. > > > > > > > > fw_devlink doesn't have any problem dealing with child devices being > > > > suppliers. The problem with your case is that the > > > > drivers/gpio/gpio-dwapb.c driver directly parses the child nodes and > > > > never creates struct devices for them. If you have a node with > > > > compatible string, fw_devlink expects you to create and probe a struct > > > > device for it. So change your driver to add the child devices as > > > > devices instead of just parsing the node directly and doing stuff with > > > > it. > > > > > > > > Either that, or stop putting "compatible" string in a node if you > > > > don't plan to actually treat it as a device -- but that's too late for > > > > this driver (it needs to be backward compatible). So change the driver > > > > to add of_platform_populate() and write a driver that probes > > > > "snps,dw-apb-gpio-port". > > > > > > > > > > Thanks for the information. The "snps,dw-apb-gpio-port" is never used, > > > so I just sent out a series to remove it. > > > > I'd actually prefer that you fix the kernel code to actually use it. > > So that fw_devlink can be backward compatible (Older DT + new kernel). > > The change is pretty trivial (I just have time to do it for you). > > > > I agree the change is trivial, but it will add some useless LoCs like below. It's not useless if it preserves backward compatibility with DT. > I'm not sure whether this is acceptable.So add GPIO and DT maintainers to comment. > > Hi Linus, Rob, > > Could you please comment? A simple introduction of the problem: > > As pointed out by Saravana, "gpio-dwapb.c driver directly parses the child > nodes and never creates struct devices for them. If you have a node with > compatible string, fw_devlink expects you to create and probe a struct > device for it", so once we set fw_devlink=on, then any users of gpio-dwapb > as below won't be probed. > > device_foo { > status = "okay" > ...; > reset-gpio = <&porta, 0, GPIO_ACTIVE_HIGH>; > }; > > The compatible string "snps,dw-apb-gpio-port" is never used, but it's in > the dt-binding since the dw gpio mainlined. I believe the every dw apb > users just copy the compatible string in to soc dtsi. So I submit a series > to remove the unused "snps,dw-apb-gpio-port" https://lkml.org/lkml/2020/10/14/1186 > But this will break Older DT + new kernel with fw_devlink on. Which solution > is better? > > If the following patch is acceptable, I can submit it once 5.10-rc1 is out. > > thanks > > diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c > index 1d8d55bd63aa..b8e012e48b59 100644 > --- a/drivers/gpio/gpio-dwapb.c > +++ b/drivers/gpio/gpio-dwapb.c > @@ -19,6 +19,7 @@ > #include <linux/of_address.h> > #include <linux/of_device.h> > #include <linux/of_irq.h> > +#include <linux/of_platform.h> > #include <linux/platform_device.h> > #include <linux/property.h> > #include <linux/reset.h> > @@ -694,6 +695,10 @@ static int dwapb_gpio_probe(struct platform_device *pdev) > } > platform_set_drvdata(pdev, gpio); > > + err = devm_of_platform_populate(dev); > + if (err) > + goto out_unregister; > + > return 0; > > out_unregister: > @@ -820,6 +825,25 @@ static struct platform_driver dwapb_gpio_driver = { > > module_platform_driver(dwapb_gpio_driver); > > +static const struct of_device_id dwapb_port_of_match[] = { > + { .compatible = "snps,dw-apb-gpio-port" }, > + { /* Sentinel */ } > +}; > + > +static int dwapb_gpio_port_probe(struct platform_device *pdev) > +{ > + return 0; No, I'm not asking to do a stub/dummy probe. Move the stuff you do inside device_for_each_child_node{} and dwapb_gpio_add_port() into this probe function. Those two pieces of code together are effectively "probing" a separate gpio controller for each of the child nodes. So just create a real struct device (like we do for every other "compatible" DT node) and probe each of them properly using the device driver core. -Saravana > +} > + > +static struct platform_driver dwapb_gpio_port_driver = { > + .driver = { > + .name = "gpio-dwapb-port", > + .of_match_table = dwapb_port_of_match, > + }, > + .probe = dwapb_gpio_port_probe, > +}; > +module_platform_driver(dwapb_gpio_port_driver); > + > MODULE_LICENSE("GPL"); > MODULE_AUTHOR("Jamie Iles"); > MODULE_DESCRIPTION("Synopsys DesignWare APB GPIO driver"); > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: fw_devlink on will break all snps,dw-apb-gpio users 2020-10-15 8:48 ` Saravana Kannan @ 2020-10-15 9:52 ` Jisheng Zhang 2020-10-15 14:08 ` Robin Murphy 0 siblings, 1 reply; 10+ messages in thread From: Jisheng Zhang @ 2020-10-15 9:52 UTC (permalink / raw) To: Saravana Kannan Cc: Rob Herring, Linus Walleij, Greg Kroah-Hartman, Rafael J. Wysocki, LKML, Frank Rowand, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, linux-arm-kernel On Thu, 15 Oct 2020 01:48:13 -0700 Saravana Kannan <saravanak@google.com> wrote: > On Thu, Oct 15, 2020 at 1:15 AM Jisheng Zhang > <Jisheng.Zhang@synaptics.com> wrote: > > > > On Wed, 14 Oct 2020 22:04:24 -0700 Saravana Kannan wrote: > > > > > > > > > > > On Wed, Oct 14, 2020 at 9:02 PM Jisheng Zhang > > > <Jisheng.Zhang@synaptics.com> wrote: > > > > > > > > On Wed, 14 Oct 2020 10:29:36 -0700 > > > > Saravana Kannan <saravanak@google.com> wrote: > > > > > > > > > > > > > > > > > > > On Wed, Oct 14, 2020 at 4:12 AM Jisheng Zhang > > > > > <Jisheng.Zhang@synaptics.com> wrote: > > > > > > > > > > > > Hi, > > > > > > > > > > > > If set fw_devlink as on, any consumers of dw apb gpio won't probe. > > > > > > > > > > > > The related dts looks like: > > > > > > > > > > > > gpio0: gpio@2400 { > > > > > > compatible = "snps,dw-apb-gpio"; > > > > > > #address-cells = <1>; > > > > > > #size-cells = <0>; > > > > > > > > > > > > porta: gpio-port@0 { > > > > > > compatible = "snps,dw-apb-gpio-port"; > > > > > > gpio-controller; > > > > > > #gpio-cells = <2>; > > > > > > ngpios = <32>; > > > > > > reg = <0>; > > > > > > }; > > > > > > }; > > > > > > > > > > > > device_foo { > > > > > > status = "okay" > > > > > > ...; > > > > > > reset-gpio = <&porta, 0, GPIO_ACTIVE_HIGH>; > > > > > > }; > > > > > > > > > > > > If I change the reset-gpio property to use another kind of gpio phandle, > > > > > > e.g gpio expander, then device_foo can be probed successfully. > > > > > > > > > > > > The gpio expander dt node looks like: > > > > > > > > > > > > expander3: gpio@44 { > > > > > > compatible = "fcs,fxl6408"; > > > > > > pinctrl-names = "default"; > > > > > > pinctrl-0 = <&expander3_pmux>; > > > > > > reg = <0x44>; > > > > > > gpio-controller; > > > > > > #gpio-cells = <2>; > > > > > > interrupt-parent = <&portb>; > > > > > > interrupts = <23 IRQ_TYPE_NONE>; > > > > > > interrupt-controller; > > > > > > #interrupt-cells = <2>; > > > > > > }; > > > > > > > > > > > > The common pattern looks like the devlink can't cope with suppliers from > > > > > > child dt node. > > > > > > > > > > fw_devlink doesn't have any problem dealing with child devices being > > > > > suppliers. The problem with your case is that the > > > > > drivers/gpio/gpio-dwapb.c driver directly parses the child nodes and > > > > > never creates struct devices for them. If you have a node with > > > > > compatible string, fw_devlink expects you to create and probe a struct > > > > > device for it. So change your driver to add the child devices as > > > > > devices instead of just parsing the node directly and doing stuff with > > > > > it. > > > > > > > > > > Either that, or stop putting "compatible" string in a node if you > > > > > don't plan to actually treat it as a device -- but that's too late for > > > > > this driver (it needs to be backward compatible). So change the driver > > > > > to add of_platform_populate() and write a driver that probes > > > > > "snps,dw-apb-gpio-port". > > > > > > > > > > > > > Thanks for the information. The "snps,dw-apb-gpio-port" is never used, > > > > so I just sent out a series to remove it. > > > > > > I'd actually prefer that you fix the kernel code to actually use it. > > > So that fw_devlink can be backward compatible (Older DT + new kernel). > > > The change is pretty trivial (I just have time to do it for you). > > > > > > > I agree the change is trivial, but it will add some useless LoCs like below. > > It's not useless if it preserves backward compatibility with DT. > > > I'm not sure whether this is acceptable.So add GPIO and DT maintainers to comment. > > > > Hi Linus, Rob, > > > > Could you please comment? A simple introduction of the problem: > > > > As pointed out by Saravana, "gpio-dwapb.c driver directly parses the child > > nodes and never creates struct devices for them. If you have a node with > > compatible string, fw_devlink expects you to create and probe a struct > > device for it", so once we set fw_devlink=on, then any users of gpio-dwapb > > as below won't be probed. > > > > device_foo { > > status = "okay" > > ...; > > reset-gpio = <&porta, 0, GPIO_ACTIVE_HIGH>; > > }; > > > > The compatible string "snps,dw-apb-gpio-port" is never used, but it's in > > the dt-binding since the dw gpio mainlined. I believe the every dw apb > > users just copy the compatible string in to soc dtsi. So I submit a series > > to remove the unused "snps,dw-apb-gpio-port" https://lkml.org/lkml/2020/10/14/1186 > > But this will break Older DT + new kernel with fw_devlink on. Which solution > > is better? > > > > If the following patch is acceptable, I can submit it once 5.10-rc1 is out. > > > > thanks > > > > diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c > > index 1d8d55bd63aa..b8e012e48b59 100644 > > --- a/drivers/gpio/gpio-dwapb.c > > +++ b/drivers/gpio/gpio-dwapb.c > > @@ -19,6 +19,7 @@ > > #include <linux/of_address.h> > > #include <linux/of_device.h> > > #include <linux/of_irq.h> > > +#include <linux/of_platform.h> > > #include <linux/platform_device.h> > > #include <linux/property.h> > > #include <linux/reset.h> > > @@ -694,6 +695,10 @@ static int dwapb_gpio_probe(struct platform_device *pdev) > > } > > platform_set_drvdata(pdev, gpio); > > > > + err = devm_of_platform_populate(dev); > > + if (err) > > + goto out_unregister; > > + > > return 0; > > > > out_unregister: > > @@ -820,6 +825,25 @@ static struct platform_driver dwapb_gpio_driver = { > > > > module_platform_driver(dwapb_gpio_driver); > > > > +static const struct of_device_id dwapb_port_of_match[] = { > > + { .compatible = "snps,dw-apb-gpio-port" }, > > + { /* Sentinel */ } > > +}; > > + > > +static int dwapb_gpio_port_probe(struct platform_device *pdev) > > +{ > > + return 0; > > No, I'm not asking to do a stub/dummy probe. Move the stuff you do > inside device_for_each_child_node{} and dwapb_gpio_add_port() into > this probe function. Those two pieces of code together are effectively > "probing" a separate gpio controller for each of the child nodes. So > just create a real struct device (like we do for every other > "compatible" DT node) and probe each of them properly using the device > driver core. Then I believe the modifications are non-trivial. Maybe Linus and Rob can comment which way is better, fix the dts or modify the gpio-dwapb.c. Personally, I prefer fixing dts, because this doesn't remove or modify any used properties or compatible string, it just removes the unused compatible string. Thanks > > > +} > > + > > +static struct platform_driver dwapb_gpio_port_driver = { > > + .driver = { > > + .name = "gpio-dwapb-port", > > + .of_match_table = dwapb_port_of_match, > > + }, > > + .probe = dwapb_gpio_port_probe, > > +}; > > +module_platform_driver(dwapb_gpio_port_driver); > > + > > MODULE_LICENSE("GPL"); > > MODULE_AUTHOR("Jamie Iles"); > > MODULE_DESCRIPTION("Synopsys DesignWare APB GPIO driver"); > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: fw_devlink on will break all snps,dw-apb-gpio users 2020-10-15 9:52 ` Jisheng Zhang @ 2020-10-15 14:08 ` Robin Murphy 2020-10-16 3:39 ` Jisheng Zhang 0 siblings, 1 reply; 10+ messages in thread From: Robin Murphy @ 2020-10-15 14:08 UTC (permalink / raw) To: Jisheng Zhang, Saravana Kannan Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Rafael J. Wysocki, Greg Kroah-Hartman, Linus Walleij, LKML, Rob Herring, Frank Rowand, linux-arm-kernel On 2020-10-15 10:52, Jisheng Zhang wrote: > On Thu, 15 Oct 2020 01:48:13 -0700 > Saravana Kannan <saravanak@google.com> wrote: > >> On Thu, Oct 15, 2020 at 1:15 AM Jisheng Zhang >> <Jisheng.Zhang@synaptics.com> wrote: >>> >>> On Wed, 14 Oct 2020 22:04:24 -0700 Saravana Kannan wrote: >>> >>>> >>>> >>>> On Wed, Oct 14, 2020 at 9:02 PM Jisheng Zhang >>>> <Jisheng.Zhang@synaptics.com> wrote: >>>>> >>>>> On Wed, 14 Oct 2020 10:29:36 -0700 >>>>> Saravana Kannan <saravanak@google.com> wrote: >>>>> >>>>>> >>>>>> >>>>>> On Wed, Oct 14, 2020 at 4:12 AM Jisheng Zhang >>>>>> <Jisheng.Zhang@synaptics.com> wrote: >>>>>>> >>>>>>> Hi, >>>>>>> >>>>>>> If set fw_devlink as on, any consumers of dw apb gpio won't probe. >>>>>>> >>>>>>> The related dts looks like: >>>>>>> >>>>>>> gpio0: gpio@2400 { >>>>>>> compatible = "snps,dw-apb-gpio"; >>>>>>> #address-cells = <1>; >>>>>>> #size-cells = <0>; >>>>>>> >>>>>>> porta: gpio-port@0 { >>>>>>> compatible = "snps,dw-apb-gpio-port"; >>>>>>> gpio-controller; >>>>>>> #gpio-cells = <2>; >>>>>>> ngpios = <32>; >>>>>>> reg = <0>; >>>>>>> }; >>>>>>> }; >>>>>>> >>>>>>> device_foo { >>>>>>> status = "okay" >>>>>>> ...; >>>>>>> reset-gpio = <&porta, 0, GPIO_ACTIVE_HIGH>; >>>>>>> }; >>>>>>> >>>>>>> If I change the reset-gpio property to use another kind of gpio phandle, >>>>>>> e.g gpio expander, then device_foo can be probed successfully. >>>>>>> >>>>>>> The gpio expander dt node looks like: >>>>>>> >>>>>>> expander3: gpio@44 { >>>>>>> compatible = "fcs,fxl6408"; >>>>>>> pinctrl-names = "default"; >>>>>>> pinctrl-0 = <&expander3_pmux>; >>>>>>> reg = <0x44>; >>>>>>> gpio-controller; >>>>>>> #gpio-cells = <2>; >>>>>>> interrupt-parent = <&portb>; >>>>>>> interrupts = <23 IRQ_TYPE_NONE>; >>>>>>> interrupt-controller; >>>>>>> #interrupt-cells = <2>; >>>>>>> }; >>>>>>> >>>>>>> The common pattern looks like the devlink can't cope with suppliers from >>>>>>> child dt node. >>>>>> >>>>>> fw_devlink doesn't have any problem dealing with child devices being >>>>>> suppliers. The problem with your case is that the >>>>>> drivers/gpio/gpio-dwapb.c driver directly parses the child nodes and >>>>>> never creates struct devices for them. If you have a node with >>>>>> compatible string, fw_devlink expects you to create and probe a struct >>>>>> device for it. So change your driver to add the child devices as >>>>>> devices instead of just parsing the node directly and doing stuff with >>>>>> it. >>>>>> >>>>>> Either that, or stop putting "compatible" string in a node if you >>>>>> don't plan to actually treat it as a device -- but that's too late for >>>>>> this driver (it needs to be backward compatible). So change the driver >>>>>> to add of_platform_populate() and write a driver that probes >>>>>> "snps,dw-apb-gpio-port". >>>>>> >>>>> >>>>> Thanks for the information. The "snps,dw-apb-gpio-port" is never used, >>>>> so I just sent out a series to remove it. >>>> >>>> I'd actually prefer that you fix the kernel code to actually use it. >>>> So that fw_devlink can be backward compatible (Older DT + new kernel). >>>> The change is pretty trivial (I just have time to do it for you). >>>> >>> >>> I agree the change is trivial, but it will add some useless LoCs like below. >> >> It's not useless if it preserves backward compatibility with DT. >> >>> I'm not sure whether this is acceptable.So add GPIO and DT maintainers to comment. >>> >>> Hi Linus, Rob, >>> >>> Could you please comment? A simple introduction of the problem: >>> >>> As pointed out by Saravana, "gpio-dwapb.c driver directly parses the child >>> nodes and never creates struct devices for them. If you have a node with >>> compatible string, fw_devlink expects you to create and probe a struct >>> device for it", so once we set fw_devlink=on, then any users of gpio-dwapb >>> as below won't be probed. >>> >>> device_foo { >>> status = "okay" >>> ...; >>> reset-gpio = <&porta, 0, GPIO_ACTIVE_HIGH>; >>> }; >>> >>> The compatible string "snps,dw-apb-gpio-port" is never used, but it's in >>> the dt-binding since the dw gpio mainlined. I believe the every dw apb >>> users just copy the compatible string in to soc dtsi. So I submit a series >>> to remove the unused "snps,dw-apb-gpio-port" https://lkml.org/lkml/2020/10/14/1186 >>> But this will break Older DT + new kernel with fw_devlink on. Which solution >>> is better? >>> >>> If the following patch is acceptable, I can submit it once 5.10-rc1 is out. >>> >>> thanks >>> >>> diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c >>> index 1d8d55bd63aa..b8e012e48b59 100644 >>> --- a/drivers/gpio/gpio-dwapb.c >>> +++ b/drivers/gpio/gpio-dwapb.c >>> @@ -19,6 +19,7 @@ >>> #include <linux/of_address.h> >>> #include <linux/of_device.h> >>> #include <linux/of_irq.h> >>> +#include <linux/of_platform.h> >>> #include <linux/platform_device.h> >>> #include <linux/property.h> >>> #include <linux/reset.h> >>> @@ -694,6 +695,10 @@ static int dwapb_gpio_probe(struct platform_device *pdev) >>> } >>> platform_set_drvdata(pdev, gpio); >>> >>> + err = devm_of_platform_populate(dev); >>> + if (err) >>> + goto out_unregister; >>> + >>> return 0; >>> >>> out_unregister: >>> @@ -820,6 +825,25 @@ static struct platform_driver dwapb_gpio_driver = { >>> >>> module_platform_driver(dwapb_gpio_driver); >>> >>> +static const struct of_device_id dwapb_port_of_match[] = { >>> + { .compatible = "snps,dw-apb-gpio-port" }, >>> + { /* Sentinel */ } >>> +}; >>> + >>> +static int dwapb_gpio_port_probe(struct platform_device *pdev) >>> +{ >>> + return 0; >> >> No, I'm not asking to do a stub/dummy probe. Move the stuff you do >> inside device_for_each_child_node{} and dwapb_gpio_add_port() into >> this probe function. Those two pieces of code together are effectively >> "probing" a separate gpio controller for each of the child nodes. So >> just create a real struct device (like we do for every other >> "compatible" DT node) and probe each of them properly using the device >> driver core. > > Then I believe the modifications are non-trivial. Maybe Linus and Rob > can comment which way is better, fix the dts or modify the gpio-dwapb.c. > Personally, I prefer fixing dts, because this doesn't remove or modify > any used properties or compatible string, it just removes the unused > compatible string. You appear to be assuming that: A) There a no consumers of DTBs and DT bindings other than Linux. B) No Linux user ever updates their kernel image without also updating their DTB. I can assure you that, in general, neither of those hold true. Hacking DTs to work around internal implementation details in Linux is rarely if ever a good or even viable idea. Robin. > > Thanks > > >> >>> +} >>> + >>> +static struct platform_driver dwapb_gpio_port_driver = { >>> + .driver = { >>> + .name = "gpio-dwapb-port", >>> + .of_match_table = dwapb_port_of_match, >>> + }, >>> + .probe = dwapb_gpio_port_probe, >>> +}; >>> +module_platform_driver(dwapb_gpio_port_driver); >>> + >>> MODULE_LICENSE("GPL"); >>> MODULE_AUTHOR("Jamie Iles"); >>> MODULE_DESCRIPTION("Synopsys DesignWare APB GPIO driver"); >>> > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: fw_devlink on will break all snps,dw-apb-gpio users 2020-10-15 14:08 ` Robin Murphy @ 2020-10-16 3:39 ` Jisheng Zhang 2020-10-17 0:44 ` Saravana Kannan 0 siblings, 1 reply; 10+ messages in thread From: Jisheng Zhang @ 2020-10-16 3:39 UTC (permalink / raw) To: Robin Murphy Cc: Saravana Kannan, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Rafael J. Wysocki, Greg Kroah-Hartman, Linus Walleij, LKML, Rob Herring, Frank Rowand, linux-arm-kernel On Thu, 15 Oct 2020 15:08:33 +0100 Robin Murphy <robin.murphy@arm.com> wrote: > > > On 2020-10-15 10:52, Jisheng Zhang wrote: > > On Thu, 15 Oct 2020 01:48:13 -0700 > > Saravana Kannan <saravanak@google.com> wrote: > > > >> On Thu, Oct 15, 2020 at 1:15 AM Jisheng Zhang > >> <Jisheng.Zhang@synaptics.com> wrote: > >>> > >>> On Wed, 14 Oct 2020 22:04:24 -0700 Saravana Kannan wrote: > >>> > >>>> > >>>> > >>>> On Wed, Oct 14, 2020 at 9:02 PM Jisheng Zhang > >>>> <Jisheng.Zhang@synaptics.com> wrote: > >>>>> > >>>>> On Wed, 14 Oct 2020 10:29:36 -0700 > >>>>> Saravana Kannan <saravanak@google.com> wrote: > >>>>> > >>>>>> > >>>>>> > >>>>>> On Wed, Oct 14, 2020 at 4:12 AM Jisheng Zhang > >>>>>> <Jisheng.Zhang@synaptics.com> wrote: > >>>>>>> > >>>>>>> Hi, > >>>>>>> > >>>>>>> If set fw_devlink as on, any consumers of dw apb gpio won't probe. > >>>>>>> > >>>>>>> The related dts looks like: > >>>>>>> > >>>>>>> gpio0: gpio@2400 { > >>>>>>> compatible = "snps,dw-apb-gpio"; > >>>>>>> #address-cells = <1>; > >>>>>>> #size-cells = <0>; > >>>>>>> > >>>>>>> porta: gpio-port@0 { > >>>>>>> compatible = "snps,dw-apb-gpio-port"; > >>>>>>> gpio-controller; > >>>>>>> #gpio-cells = <2>; > >>>>>>> ngpios = <32>; > >>>>>>> reg = <0>; > >>>>>>> }; > >>>>>>> }; > >>>>>>> > >>>>>>> device_foo { > >>>>>>> status = "okay" > >>>>>>> ...; > >>>>>>> reset-gpio = <&porta, 0, GPIO_ACTIVE_HIGH>; > >>>>>>> }; > >>>>>>> > >>>>>>> If I change the reset-gpio property to use another kind of gpio phandle, > >>>>>>> e.g gpio expander, then device_foo can be probed successfully. > >>>>>>> > >>>>>>> The gpio expander dt node looks like: > >>>>>>> > >>>>>>> expander3: gpio@44 { > >>>>>>> compatible = "fcs,fxl6408"; > >>>>>>> pinctrl-names = "default"; > >>>>>>> pinctrl-0 = <&expander3_pmux>; > >>>>>>> reg = <0x44>; > >>>>>>> gpio-controller; > >>>>>>> #gpio-cells = <2>; > >>>>>>> interrupt-parent = <&portb>; > >>>>>>> interrupts = <23 IRQ_TYPE_NONE>; > >>>>>>> interrupt-controller; > >>>>>>> #interrupt-cells = <2>; > >>>>>>> }; > >>>>>>> > >>>>>>> The common pattern looks like the devlink can't cope with suppliers from > >>>>>>> child dt node. > >>>>>> > >>>>>> fw_devlink doesn't have any problem dealing with child devices being > >>>>>> suppliers. The problem with your case is that the > >>>>>> drivers/gpio/gpio-dwapb.c driver directly parses the child nodes and > >>>>>> never creates struct devices for them. If you have a node with > >>>>>> compatible string, fw_devlink expects you to create and probe a struct > >>>>>> device for it. So change your driver to add the child devices as > >>>>>> devices instead of just parsing the node directly and doing stuff with > >>>>>> it. > >>>>>> > >>>>>> Either that, or stop putting "compatible" string in a node if you > >>>>>> don't plan to actually treat it as a device -- but that's too late for > >>>>>> this driver (it needs to be backward compatible). So change the driver > >>>>>> to add of_platform_populate() and write a driver that probes > >>>>>> "snps,dw-apb-gpio-port". > >>>>>> > >>>>> > >>>>> Thanks for the information. The "snps,dw-apb-gpio-port" is never used, > >>>>> so I just sent out a series to remove it. > >>>> > >>>> I'd actually prefer that you fix the kernel code to actually use it. > >>>> So that fw_devlink can be backward compatible (Older DT + new kernel). > >>>> The change is pretty trivial (I just have time to do it for you). > >>>> > >>> > >>> I agree the change is trivial, but it will add some useless LoCs like below. > >> > >> It's not useless if it preserves backward compatibility with DT. > >> > >>> I'm not sure whether this is acceptable.So add GPIO and DT maintainers to comment. > >>> > >>> Hi Linus, Rob, > >>> > >>> Could you please comment? A simple introduction of the problem: > >>> > >>> As pointed out by Saravana, "gpio-dwapb.c driver directly parses the child > >>> nodes and never creates struct devices for them. If you have a node with > >>> compatible string, fw_devlink expects you to create and probe a struct > >>> device for it", so once we set fw_devlink=on, then any users of gpio-dwapb > >>> as below won't be probed. > >>> > >>> device_foo { > >>> status = "okay" > >>> ...; > >>> reset-gpio = <&porta, 0, GPIO_ACTIVE_HIGH>; > >>> }; > >>> > >>> The compatible string "snps,dw-apb-gpio-port" is never used, but it's in > >>> the dt-binding since the dw gpio mainlined. I believe the every dw apb > >>> users just copy the compatible string in to soc dtsi. So I submit a series > >>> to remove the unused "snps,dw-apb-gpio-port" > >>> But this will break Older DT + new kernel with fw_devlink on. Which solution > >>> is better? > >>> > >>> If the following patch is acceptable, I can submit it once 5.10-rc1 is out. > >>> > >>> thanks > >>> > >>> diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c > >>> index 1d8d55bd63aa..b8e012e48b59 100644 > >>> --- a/drivers/gpio/gpio-dwapb.c > >>> +++ b/drivers/gpio/gpio-dwapb.c > >>> @@ -19,6 +19,7 @@ > >>> #include <linux/of_address.h> > >>> #include <linux/of_device.h> > >>> #include <linux/of_irq.h> > >>> +#include <linux/of_platform.h> > >>> #include <linux/platform_device.h> > >>> #include <linux/property.h> > >>> #include <linux/reset.h> > >>> @@ -694,6 +695,10 @@ static int dwapb_gpio_probe(struct platform_device *pdev) > >>> } > >>> platform_set_drvdata(pdev, gpio); > >>> > >>> + err = devm_of_platform_populate(dev); > >>> + if (err) > >>> + goto out_unregister; > >>> + > >>> return 0; > >>> > >>> out_unregister: > >>> @@ -820,6 +825,25 @@ static struct platform_driver dwapb_gpio_driver = { > >>> > >>> module_platform_driver(dwapb_gpio_driver); > >>> > >>> +static const struct of_device_id dwapb_port_of_match[] = { > >>> + { .compatible = "snps,dw-apb-gpio-port" }, > >>> + { /* Sentinel */ } > >>> +}; > >>> + > >>> +static int dwapb_gpio_port_probe(struct platform_device *pdev) > >>> +{ > >>> + return 0; > >> > >> No, I'm not asking to do a stub/dummy probe. Move the stuff you do > >> inside device_for_each_child_node{} and dwapb_gpio_add_port() into > >> this probe function. Those two pieces of code together are effectively > >> "probing" a separate gpio controller for each of the child nodes. So > >> just create a real struct device (like we do for every other > >> "compatible" DT node) and probe each of them properly using the device > >> driver core. > > > > Then I believe the modifications are non-trivial. Maybe Linus and Rob > > can comment which way is better, fix the dts or modify the gpio-dwapb.c. > > Personally, I prefer fixing dts, because this doesn't remove or modify > > any used properties or compatible string, it just removes the unused > > compatible string. > > You appear to be assuming that: > > A) There a no consumers of DTBs and DT bindings other than Linux. > B) No Linux user ever updates their kernel image without also updating > their DTB. > > I can assure you that, in general, neither of those hold true. Hacking Just my humble opinion, this is fixing rather than hacking DTs. > DTs to work around internal implementation details in Linux is rarely if > ever a good or even viable idea. > I got your opinion. So it looks like modify the dwapb gpio driver is avoidable. I will submit patch to do so once 5.10-rc1 is out. But the device link also introduces below warning for all dw-apb-gpio users: [ 0.016113] OF: /soc/apb@f7e80000/gpio@0800/gpio-port@1: could not find phandle [ 0.016197] OF: /soc/apb@f7e80000/gpio@0c00/gpio-port@1: could not find phandle [ 0.016464] OF: /soc/apb@f7e80000/gpio@2400/gpio-port@0: could not find phandle [ 0.016697] OF: /soc/apb@f7fc0000/gpio@8000/gpio-port@4: could not find phandle [ 0.017054] OF: /soc/apb@f7e80000/gpio@0800/gpio-port@1: could not find phandle [ 0.017128] OF: /soc/apb@f7e80000/gpio@0c00/gpio-port@1: could not find phandle Previously, it seems that the solution would be "let's mark the "snps,nr-gpios" property as deprecated and add the generic "ngpios" property support with the same purpose as the deprecated one. That and the errors log above shall motivate the platform developer to *convert* the DW APB GPIO DT-nodes to using the standard number of GPIOs property" as commit 7569486d79ae8ec4 ("gpio: dwapb: Add ngpios DT-property support") said, the "can't break old DTs" also apply here, it means we need to fix the warning in device link code rather than fix DTs. Any comments? Thanks ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: fw_devlink on will break all snps,dw-apb-gpio users 2020-10-16 3:39 ` Jisheng Zhang @ 2020-10-17 0:44 ` Saravana Kannan 0 siblings, 0 replies; 10+ messages in thread From: Saravana Kannan @ 2020-10-17 0:44 UTC (permalink / raw) To: Jisheng Zhang Cc: Robin Murphy, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Rafael J. Wysocki, Greg Kroah-Hartman, Linus Walleij, LKML, Rob Herring, Frank Rowand, linux-arm-kernel On Thu, Oct 15, 2020 at 8:39 PM Jisheng Zhang <Jisheng.Zhang@synaptics.com> wrote: > > On Thu, 15 Oct 2020 15:08:33 +0100 > Robin Murphy <robin.murphy@arm.com> wrote: > > > > > > > On 2020-10-15 10:52, Jisheng Zhang wrote: > > > On Thu, 15 Oct 2020 01:48:13 -0700 > > > Saravana Kannan <saravanak@google.com> wrote: > > > > > >> On Thu, Oct 15, 2020 at 1:15 AM Jisheng Zhang > > >> <Jisheng.Zhang@synaptics.com> wrote: > > >>> > > >>> On Wed, 14 Oct 2020 22:04:24 -0700 Saravana Kannan wrote: > > >>> > > >>>> > > >>>> > > >>>> On Wed, Oct 14, 2020 at 9:02 PM Jisheng Zhang > > >>>> <Jisheng.Zhang@synaptics.com> wrote: > > >>>>> > > >>>>> On Wed, 14 Oct 2020 10:29:36 -0700 > > >>>>> Saravana Kannan <saravanak@google.com> wrote: > > >>>>> > > >>>>>> > > >>>>>> > > >>>>>> On Wed, Oct 14, 2020 at 4:12 AM Jisheng Zhang > > >>>>>> <Jisheng.Zhang@synaptics.com> wrote: > > >>>>>>> > > >>>>>>> Hi, > > >>>>>>> > > >>>>>>> If set fw_devlink as on, any consumers of dw apb gpio won't probe. > > >>>>>>> > > >>>>>>> The related dts looks like: > > >>>>>>> > > >>>>>>> gpio0: gpio@2400 { > > >>>>>>> compatible = "snps,dw-apb-gpio"; > > >>>>>>> #address-cells = <1>; > > >>>>>>> #size-cells = <0>; > > >>>>>>> > > >>>>>>> porta: gpio-port@0 { > > >>>>>>> compatible = "snps,dw-apb-gpio-port"; > > >>>>>>> gpio-controller; > > >>>>>>> #gpio-cells = <2>; > > >>>>>>> ngpios = <32>; > > >>>>>>> reg = <0>; > > >>>>>>> }; > > >>>>>>> }; > > >>>>>>> > > >>>>>>> device_foo { > > >>>>>>> status = "okay" > > >>>>>>> ...; > > >>>>>>> reset-gpio = <&porta, 0, GPIO_ACTIVE_HIGH>; > > >>>>>>> }; > > >>>>>>> > > >>>>>>> If I change the reset-gpio property to use another kind of gpio phandle, > > >>>>>>> e.g gpio expander, then device_foo can be probed successfully. > > >>>>>>> > > >>>>>>> The gpio expander dt node looks like: > > >>>>>>> > > >>>>>>> expander3: gpio@44 { > > >>>>>>> compatible = "fcs,fxl6408"; > > >>>>>>> pinctrl-names = "default"; > > >>>>>>> pinctrl-0 = <&expander3_pmux>; > > >>>>>>> reg = <0x44>; > > >>>>>>> gpio-controller; > > >>>>>>> #gpio-cells = <2>; > > >>>>>>> interrupt-parent = <&portb>; > > >>>>>>> interrupts = <23 IRQ_TYPE_NONE>; > > >>>>>>> interrupt-controller; > > >>>>>>> #interrupt-cells = <2>; > > >>>>>>> }; > > >>>>>>> > > >>>>>>> The common pattern looks like the devlink can't cope with suppliers from > > >>>>>>> child dt node. > > >>>>>> > > >>>>>> fw_devlink doesn't have any problem dealing with child devices being > > >>>>>> suppliers. The problem with your case is that the > > >>>>>> drivers/gpio/gpio-dwapb.c driver directly parses the child nodes and > > >>>>>> never creates struct devices for them. If you have a node with > > >>>>>> compatible string, fw_devlink expects you to create and probe a struct > > >>>>>> device for it. So change your driver to add the child devices as > > >>>>>> devices instead of just parsing the node directly and doing stuff with > > >>>>>> it. > > >>>>>> > > >>>>>> Either that, or stop putting "compatible" string in a node if you > > >>>>>> don't plan to actually treat it as a device -- but that's too late for > > >>>>>> this driver (it needs to be backward compatible). So change the driver > > >>>>>> to add of_platform_populate() and write a driver that probes > > >>>>>> "snps,dw-apb-gpio-port". > > >>>>>> > > >>>>> > > >>>>> Thanks for the information. The "snps,dw-apb-gpio-port" is never used, > > >>>>> so I just sent out a series to remove it. > > >>>> > > >>>> I'd actually prefer that you fix the kernel code to actually use it. > > >>>> So that fw_devlink can be backward compatible (Older DT + new kernel). > > >>>> The change is pretty trivial (I just have time to do it for you). > > >>>> > > >>> > > >>> I agree the change is trivial, but it will add some useless LoCs like below. > > >> > > >> It's not useless if it preserves backward compatibility with DT. > > >> > > >>> I'm not sure whether this is acceptable.So add GPIO and DT maintainers to comment. > > >>> > > >>> Hi Linus, Rob, > > >>> > > >>> Could you please comment? A simple introduction of the problem: > > >>> > > >>> As pointed out by Saravana, "gpio-dwapb.c driver directly parses the child > > >>> nodes and never creates struct devices for them. If you have a node with > > >>> compatible string, fw_devlink expects you to create and probe a struct > > >>> device for it", so once we set fw_devlink=on, then any users of gpio-dwapb > > >>> as below won't be probed. > > >>> > > >>> device_foo { > > >>> status = "okay" > > >>> ...; > > >>> reset-gpio = <&porta, 0, GPIO_ACTIVE_HIGH>; > > >>> }; > > >>> > > >>> The compatible string "snps,dw-apb-gpio-port" is never used, but it's in > > >>> the dt-binding since the dw gpio mainlined. I believe the every dw apb > > >>> users just copy the compatible string in to soc dtsi. So I submit a series > > >>> to remove the unused "snps,dw-apb-gpio-port" > > >>> But this will break Older DT + new kernel with fw_devlink on. Which solution > > >>> is better? > > >>> > > >>> If the following patch is acceptable, I can submit it once 5.10-rc1 is out. > > >>> > > >>> thanks > > >>> > > >>> diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c > > >>> index 1d8d55bd63aa..b8e012e48b59 100644 > > >>> --- a/drivers/gpio/gpio-dwapb.c > > >>> +++ b/drivers/gpio/gpio-dwapb.c > > >>> @@ -19,6 +19,7 @@ > > >>> #include <linux/of_address.h> > > >>> #include <linux/of_device.h> > > >>> #include <linux/of_irq.h> > > >>> +#include <linux/of_platform.h> > > >>> #include <linux/platform_device.h> > > >>> #include <linux/property.h> > > >>> #include <linux/reset.h> > > >>> @@ -694,6 +695,10 @@ static int dwapb_gpio_probe(struct platform_device *pdev) > > >>> } > > >>> platform_set_drvdata(pdev, gpio); > > >>> > > >>> + err = devm_of_platform_populate(dev); > > >>> + if (err) > > >>> + goto out_unregister; > > >>> + > > >>> return 0; > > >>> > > >>> out_unregister: > > >>> @@ -820,6 +825,25 @@ static struct platform_driver dwapb_gpio_driver = { > > >>> > > >>> module_platform_driver(dwapb_gpio_driver); > > >>> > > >>> +static const struct of_device_id dwapb_port_of_match[] = { > > >>> + { .compatible = "snps,dw-apb-gpio-port" }, > > >>> + { /* Sentinel */ } > > >>> +}; > > >>> + > > >>> +static int dwapb_gpio_port_probe(struct platform_device *pdev) > > >>> +{ > > >>> + return 0; > > >> > > >> No, I'm not asking to do a stub/dummy probe. Move the stuff you do > > >> inside device_for_each_child_node{} and dwapb_gpio_add_port() into > > >> this probe function. Those two pieces of code together are effectively > > >> "probing" a separate gpio controller for each of the child nodes. So > > >> just create a real struct device (like we do for every other > > >> "compatible" DT node) and probe each of them properly using the device > > >> driver core. > > > > > > Then I believe the modifications are non-trivial. Maybe Linus and Rob > > > can comment which way is better, fix the dts or modify the gpio-dwapb.c. > > > Personally, I prefer fixing dts, because this doesn't remove or modify > > > any used properties or compatible string, it just removes the unused > > > compatible string. > > > > You appear to be assuming that: > > > > A) There a no consumers of DTBs and DT bindings other than Linux. > > B) No Linux user ever updates their kernel image without also updating > > their DTB. > > > > I can assure you that, in general, neither of those hold true. Hacking > > Just my humble opinion, this is fixing rather than hacking DTs. > > > DTs to work around internal implementation details in Linux is rarely if > > ever a good or even viable idea. > > > > I got your opinion. So it looks like modify the dwapb gpio driver is > avoidable. I will submit patch to do so once 5.10-rc1 is out. > > But the device link also introduces below warning for all dw-apb-gpio users: > > [ 0.016113] OF: /soc/apb@f7e80000/gpio@0800/gpio-port@1: could not find phandle > [ 0.016197] OF: /soc/apb@f7e80000/gpio@0c00/gpio-port@1: could not find phandle > [ 0.016464] OF: /soc/apb@f7e80000/gpio@2400/gpio-port@0: could not find phandle > [ 0.016697] OF: /soc/apb@f7fc0000/gpio@8000/gpio-port@4: could not find phandle > [ 0.017054] OF: /soc/apb@f7e80000/gpio@0800/gpio-port@1: could not find phandle > [ 0.017128] OF: /soc/apb@f7e80000/gpio@0c00/gpio-port@1: could not find phandle To clarify, this warning doesn't break any functionality. It's just fw_devlink not being happy that a property ends in -gpios but doesn't follow the -gpios format. You can do both of these then? 1. Update the dwapb gpiodriver so that the older DT still works. 2. Update the DT to not use snps,nr-gpios so that going forward, we won't be using a deprecated property and causing this warning. Older DT + newer kernel will have this warning, but that's not the end of the world. -Saravana ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-10-17 5:34 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-10-14 11:12 fw_devlink on will break all snps,dw-apb-gpio users Jisheng Zhang 2020-10-14 17:29 ` Saravana Kannan 2020-10-15 4:02 ` Jisheng Zhang 2020-10-15 5:04 ` Saravana Kannan 2020-10-15 8:14 ` Jisheng Zhang 2020-10-15 8:48 ` Saravana Kannan 2020-10-15 9:52 ` Jisheng Zhang 2020-10-15 14:08 ` Robin Murphy 2020-10-16 3:39 ` Jisheng Zhang 2020-10-17 0:44 ` Saravana Kannan
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).