Re: fw_devlink on will break all snps,dw-apb-gpio users
On Thu, Oct 15, 2020 at 8:39 PM Jisheng Zhang
wrote:
>
> On Thu, 15 Oct 2020 15:08:33 +0100
> Robin Murphy wrote:
>
> >
> >
> > On 2020-10-15 10:52, Jisheng Zhang wrote:
> > > On Thu, 15 Oct 2020 01:48:13 -0700
> > > Saravana Kannan wrote:
> > >
> > >> On Thu, Oct 15, 2020 at 1:15 AM Jisheng Zhang
> > >> wrote:
> > >>>
> > >>> On Wed, 14 Oct 2020 22:04:24 -0700 Saravana Kannan wrote:
> > >>>
> >
> >
> > On Wed, Oct 14, 2020 at 9:02 PM Jisheng Zhang
> > wrote:
> > >
> > > On Wed, 14 Oct 2020 10:29:36 -0700
> > > Saravana Kannan wrote:
> > >
> > >>
> > >>
> > >> On Wed, Oct 14, 2020 at 4:12 AM Jisheng Zhang
> > >> 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 = <, 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 = <_pmux>;
> > >>> reg = <0x44>;
> > >>> gpio-controller;
> > >>> #gpio-cells = <2>;
> > >>> interrupt-parent = <>;
> > >>> 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 = <, 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
> >
Re: fw_devlink on will break all snps,dw-apb-gpio users
On Thu, 15 Oct 2020 15:08:33 +0100
Robin Murphy wrote:
>
>
> On 2020-10-15 10:52, Jisheng Zhang wrote:
> > On Thu, 15 Oct 2020 01:48:13 -0700
> > Saravana Kannan wrote:
> >
> >> On Thu, Oct 15, 2020 at 1:15 AM Jisheng Zhang
> >> wrote:
> >>>
> >>> On Wed, 14 Oct 2020 22:04:24 -0700 Saravana Kannan wrote:
> >>>
>
>
> On Wed, Oct 14, 2020 at 9:02 PM Jisheng Zhang
> wrote:
> >
> > On Wed, 14 Oct 2020 10:29:36 -0700
> > Saravana Kannan wrote:
> >
> >>
> >>
> >> On Wed, Oct 14, 2020 at 4:12 AM Jisheng Zhang
> >> 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 = <, 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 = <_pmux>;
> >>> reg = <0x44>;
> >>> gpio-controller;
> >>> #gpio-cells = <2>;
> >>> interrupt-parent = <>;
> >>> 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 = <, 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.
> >>>
> >>>
Re: fw_devlink on will break all snps,dw-apb-gpio users
On 2020-10-15 10:52, Jisheng Zhang wrote:
On Thu, 15 Oct 2020 01:48:13 -0700
Saravana Kannan wrote:
On Thu, Oct 15, 2020 at 1:15 AM Jisheng Zhang
wrote:
On Wed, 14 Oct 2020 22:04:24 -0700 Saravana Kannan wrote:
On Wed, Oct 14, 2020 at 9:02 PM Jisheng Zhang
wrote:
On Wed, 14 Oct 2020 10:29:36 -0700
Saravana Kannan wrote:
On Wed, Oct 14, 2020 at 4:12 AM Jisheng Zhang
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 = <, 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 = <_pmux>;
reg = <0x44>;
gpio-controller;
#gpio-cells = <2>;
interrupt-parent = <>;
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 = <, 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
#include
#include
+#include
#include
#include
#include
@@ -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
Re: fw_devlink on will break all snps,dw-apb-gpio users
On Thu, 15 Oct 2020 01:48:13 -0700
Saravana Kannan wrote:
> On Thu, Oct 15, 2020 at 1:15 AM Jisheng Zhang
> wrote:
> >
> > On Wed, 14 Oct 2020 22:04:24 -0700 Saravana Kannan wrote:
> >
> > >
> > >
> > > On Wed, Oct 14, 2020 at 9:02 PM Jisheng Zhang
> > > wrote:
> > > >
> > > > On Wed, 14 Oct 2020 10:29:36 -0700
> > > > Saravana Kannan wrote:
> > > >
> > > > >
> > > > >
> > > > > On Wed, Oct 14, 2020 at 4:12 AM Jisheng Zhang
> > > > > 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 = <, 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 = <_pmux>;
> > > > > > reg = <0x44>;
> > > > > > gpio-controller;
> > > > > > #gpio-cells = <2>;
> > > > > > interrupt-parent = <>;
> > > > > > 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 = <, 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
Re: fw_devlink on will break all snps,dw-apb-gpio users
On Thu, Oct 15, 2020 at 1:15 AM Jisheng Zhang
wrote:
>
> On Wed, 14 Oct 2020 22:04:24 -0700 Saravana Kannan wrote:
>
> >
> >
> > On Wed, Oct 14, 2020 at 9:02 PM Jisheng Zhang
> > wrote:
> > >
> > > On Wed, 14 Oct 2020 10:29:36 -0700
> > > Saravana Kannan wrote:
> > >
> > > >
> > > >
> > > > On Wed, Oct 14, 2020 at 4:12 AM Jisheng Zhang
> > > > 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 = <, 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 = <_pmux>;
> > > > > reg = <0x44>;
> > > > > gpio-controller;
> > > > > #gpio-cells = <2>;
> > > > > interrupt-parent = <>;
> > > > > 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 = <, 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
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -694,6 +695,10 @@ static int dwapb_gpio_probe(struct platform_device *pdev)
> }
> platform_set_drvdata(pdev, gpio);
Re: fw_devlink on will break all snps,dw-apb-gpio users
On Wed, 14 Oct 2020 22:04:24 -0700 Saravana Kannan wrote:
>
>
> On Wed, Oct 14, 2020 at 9:02 PM Jisheng Zhang
> wrote:
> >
> > On Wed, 14 Oct 2020 10:29:36 -0700
> > Saravana Kannan wrote:
> >
> > >
> > >
> > > On Wed, Oct 14, 2020 at 4:12 AM Jisheng Zhang
> > > 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 = <, 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 = <_pmux>;
> > > > reg = <0x44>;
> > > > gpio-controller;
> > > > #gpio-cells = <2>;
> > > > interrupt-parent = <>;
> > > > 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 = <, 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
#include
#include
+#include
#include
#include
#include
@@ -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" },
+ { /*
Re: fw_devlink on will break all snps,dw-apb-gpio users
On Wed, Oct 14, 2020 at 9:02 PM Jisheng Zhang
wrote:
>
> On Wed, 14 Oct 2020 10:29:36 -0700
> Saravana Kannan 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
> > 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 = <, 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 = <_pmux>;
> > > reg = <0x44>;
> > > gpio-controller;
> > > #gpio-cells = <2>;
> > > interrupt-parent = <>;
> > > 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
Re: fw_devlink on will break all snps,dw-apb-gpio users
On Wed, 14 Oct 2020 10:29:36 -0700
Saravana Kannan 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
> 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 = <, 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 = <_pmux>;
> > reg = <0x44>;
> > gpio-controller;
> > #gpio-cells = <2>;
> > interrupt-parent = <>;
> > 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
Re: fw_devlink on will break all snps,dw-apb-gpio users
On Wed, Oct 14, 2020 at 4:12 AM Jisheng Zhang
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 = <, 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 = <_pmux>;
> reg = <0x44>;
> gpio-controller;
> #gpio-cells = <2>;
> interrupt-parent = <>;
> 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
