diff mbox series

[2/4] firmware: arm_scmi: bus: Bypass setting fwnode for pinctrl

Message ID 20241225-scmi-fwdevlink-v1-2-e9a3a5341362@nxp.com (mailing list archive)
State New
Headers show
Series scmi: Bypass set fwnode to address devlink issue | expand

Commit Message

Peng Fan (OSS) Dec. 25, 2024, 8:20 a.m. UTC
From: Peng Fan <peng.fan@nxp.com>

pinctrl-scmi.c and pinctrl-imx-scmi.c, both use SCMI_PROTOCOL_PINCTRL.
If both drivers are built in, and the scmi device with name "pinctrl-imx"
is created earlier, and the fwnode device points to the scmi device,
non-i.MX platforms will never have the pinctrl supplier ready.

So bypass setting fwnode for scmi pinctrl devices that non
compatible with socs.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/firmware/arm_scmi/bus.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

Comments

Sudeep Holla Dec. 27, 2024, 3:28 p.m. UTC | #1
On Wed, Dec 25, 2024 at 04:20:45PM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
>
> pinctrl-scmi.c and pinctrl-imx-scmi.c, both use SCMI_PROTOCOL_PINCTRL.
> If both drivers are built in, and the scmi device with name "pinctrl-imx"
> is created earlier, and the fwnode device points to the scmi device,
> non-i.MX platforms will never have the pinctrl supplier ready.
>

I wonder if we can prevent creation of "pinctrl-imx" scmi device on non
i.MX platforms instead of this hack which IMO is little less hackier
(and little more cleaner as we don't create problem and then fix here)
than this change.

--
Regards,
Sudeep
Peng Fan (OSS) Dec. 30, 2024, 2:08 a.m. UTC | #2
On Fri, Dec 27, 2024 at 03:28:07PM +0000, Sudeep Holla wrote:
>On Wed, Dec 25, 2024 at 04:20:45PM +0800, Peng Fan (OSS) wrote:
>> From: Peng Fan <peng.fan@nxp.com>
>>
>> pinctrl-scmi.c and pinctrl-imx-scmi.c, both use SCMI_PROTOCOL_PINCTRL.
>> If both drivers are built in, and the scmi device with name "pinctrl-imx"
>> is created earlier, and the fwnode device points to the scmi device,
>> non-i.MX platforms will never have the pinctrl supplier ready.
>>
>
>I wonder if we can prevent creation of "pinctrl-imx" scmi device on non
>i.MX platforms instead of this hack which IMO is little less hackier
>(and little more cleaner as we don't create problem and then fix here)
>than this change.

I thought two ways that introduce new entries in scmi_device_id,
1. compatible string.
2. allowed machine string and blcoked machine string.

Thanks,
Peng
>
>--
>Regards,
>Sudeep
Cristian Marussi Dec. 31, 2024, 6:13 p.m. UTC | #3
On Wed, Dec 25, 2024 at 04:20:45PM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> pinctrl-scmi.c and pinctrl-imx-scmi.c, both use SCMI_PROTOCOL_PINCTRL.
> If both drivers are built in, and the scmi device with name "pinctrl-imx"
> is created earlier, and the fwnode device points to the scmi device,
> non-i.MX platforms will never have the pinctrl supplier ready.
> 
> So bypass setting fwnode for scmi pinctrl devices that non
> compatible with socs.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/firmware/arm_scmi/bus.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c
> index 12190d4dabb65484543044b4424fbe3b67245466..87665b09c8ff492953c8300f80ed73eab6cce4fd 100644
> --- a/drivers/firmware/arm_scmi/bus.c
> +++ b/drivers/firmware/arm_scmi/bus.c
> @@ -345,6 +345,11 @@ static void __scmi_device_destroy(struct scmi_device *scmi_dev)
>  	device_unregister(&scmi_dev->dev);
>  }
>  
> +static const char * const scmi_pinctrl_imx_lists[] = {
> +	"fsl,imx95",
> +	NULL
> +};
> +
>  static int
>  __scmi_device_set_node(struct scmi_device *scmi_dev, struct device_node *np,
>  		       int protocol, const char *name)
> @@ -353,6 +358,15 @@ __scmi_device_set_node(struct scmi_device *scmi_dev, struct device_node *np,
>  	if ((protocol == SCMI_PROTOCOL_PERF) && !strcmp(name, "cpufreq"))
>  		return 0;
>  
> +	if (protocol == SCMI_PROTOCOL_PINCTRL) {
> +		if (!strcmp(name, "pinctrl") &&
> +		    of_machine_compatible_match(scmi_pinctrl_imx_lists))
> +			return 0;
> +		if (!strcmp(name, "pinctrl-imx") &&
> +		    !of_machine_compatible_match(scmi_pinctrl_imx_lists))
> +			return 0;
> +	}

...and same here, you could set a flag scmi_dev->avoid_devlink and
just avoid calling device_link_add instead of killing the device_node...

Thanks,
Cristian
Cristian Marussi Dec. 31, 2024, 6:16 p.m. UTC | #4
On Fri, Dec 27, 2024 at 03:28:07PM +0000, Sudeep Holla wrote:
> On Wed, Dec 25, 2024 at 04:20:45PM +0800, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > pinctrl-scmi.c and pinctrl-imx-scmi.c, both use SCMI_PROTOCOL_PINCTRL.
> > If both drivers are built in, and the scmi device with name "pinctrl-imx"
> > is created earlier, and the fwnode device points to the scmi device,
> > non-i.MX platforms will never have the pinctrl supplier ready.
> >
> 
> I wonder if we can prevent creation of "pinctrl-imx" scmi device on non
> i.MX platforms instead of this hack which IMO is little less hackier
> (and little more cleaner as we don't create problem and then fix here)
> than this change.

...or indeed this is another possibility

Thanks,
Cristian
Peng Fan (OSS) Jan. 6, 2025, 4:41 a.m. UTC | #5
On Tue, Dec 31, 2024 at 06:16:12PM +0000, Cristian Marussi wrote:
>On Fri, Dec 27, 2024 at 03:28:07PM +0000, Sudeep Holla wrote:
>> On Wed, Dec 25, 2024 at 04:20:45PM +0800, Peng Fan (OSS) wrote:
>> > From: Peng Fan <peng.fan@nxp.com>
>> >
>> > pinctrl-scmi.c and pinctrl-imx-scmi.c, both use SCMI_PROTOCOL_PINCTRL.
>> > If both drivers are built in, and the scmi device with name "pinctrl-imx"
>> > is created earlier, and the fwnode device points to the scmi device,
>> > non-i.MX platforms will never have the pinctrl supplier ready.
>> >
>> 
>> I wonder if we can prevent creation of "pinctrl-imx" scmi device on non
>> i.MX platforms instead of this hack which IMO is little less hackier
>> (and little more cleaner as we don't create problem and then fix here)
>> than this change.
>
>...or indeed this is another possibility

I am doing a patch as below, how to do you think?

With below patch, we could resolve the devlink issue and also support mutitple
vendor drivers built in, with each vendor driver has a machine_allowlist.

diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c
index 1d2aedfcfdb4..c1c45b545480 100644
--- a/drivers/firmware/arm_scmi/bus.c
+++ b/drivers/firmware/arm_scmi/bus.c
@@ -55,6 +55,20 @@ static int scmi_protocol_device_request(const struct scmi_device_id *id_table)
        unsigned int id = 0;
        struct list_head *head, *phead = NULL;
        struct scmi_requested_dev *rdev;
+       const char * const *allowlist = id_table->machine_allowlist;
+       const char * const *blocklist = id_table->machine_blocklist;
+
+       if (blocklist && of_machine_compatible_match(blocklist)) {
+               pr_debug("block SCMI device (%s) for protocol %x\n",
+                        id_table->name, id_table->protocol_id);
+               return 0;
+       }
+
+       if (allowlist && !of_machine_compatible_match(allowlist)) {
+               pr_debug("block SCMI device (%s) for protocol %x\n",
+                        id_table->name, id_table->protocol_id);
+               return 0;
+       }

        pr_debug("Requesting SCMI device (%s) for protocol %x\n",
                 id_table->name, id_table->protocol_id);
diff --git a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
index 688466a0e816..e1b822d3522f 100644
--- a/include/linux/scmi_protocol.h
+++ b/include/linux/scmi_protocol.h
@@ -950,6 +950,9 @@ struct scmi_device {
 struct scmi_device_id {
        u8 protocol_id;
        const char *name;
+       /* Optional */
+       const char * const *machine_blocklist;
+       const char * const *machine_allowlist;
 };

 struct scmi_driver {

Thanks,
Peng
>
>Thanks,
>Cristian
Peng Fan Jan. 14, 2025, 8:31 a.m. UTC | #6
Hi Cristian, Sudeep

> Subject: Re: [PATCH 2/4] firmware: arm_scmi: bus: Bypass setting
> fwnode for pinctrl
> 
[...]

> >> fix here) than this change.
> >
> >...or indeed this is another possibility
> 
> I am doing a patch as below, how to do you think?

Do you have any comments on below ideas?

I am thinking to send out new patchset based on
below ideas in this week.

> 
> With below patch, we could resolve the devlink issue and also support
> mutitple vendor drivers built in, with each vendor driver has a
> machine_allowlist.
> 
> diff --git a/drivers/firmware/arm_scmi/bus.c
> b/drivers/firmware/arm_scmi/bus.c index
> 1d2aedfcfdb4..c1c45b545480 100644
> --- a/drivers/firmware/arm_scmi/bus.c
> +++ b/drivers/firmware/arm_scmi/bus.c
> @@ -55,6 +55,20 @@ static int scmi_protocol_device_request(const
> struct scmi_device_id *id_table)
>         unsigned int id = 0;
>         struct list_head *head, *phead = NULL;
>         struct scmi_requested_dev *rdev;
> +       const char * const *allowlist = id_table->machine_allowlist;
> +       const char * const *blocklist = id_table->machine_blocklist;
> +
> +       if (blocklist && of_machine_compatible_match(blocklist)) {
> +               pr_debug("block SCMI device (%s) for protocol %x\n",
> +                        id_table->name, id_table->protocol_id);
> +               return 0;
> +       }
> +
> +       if (allowlist && !of_machine_compatible_match(allowlist)) {
> +               pr_debug("block SCMI device (%s) for protocol %x\n",
> +                        id_table->name, id_table->protocol_id);
> +               return 0;
> +       }
> 
>         pr_debug("Requesting SCMI device (%s) for protocol %x\n",
>                  id_table->name, id_table->protocol_id); diff --git
> a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h index
> 688466a0e816..e1b822d3522f 100644
> --- a/include/linux/scmi_protocol.h
> +++ b/include/linux/scmi_protocol.h
> @@ -950,6 +950,9 @@ struct scmi_device {  struct scmi_device_id {
>         u8 protocol_id;
>         const char *name;
> +       /* Optional */
> +       const char * const *machine_blocklist;
> +       const char * const *machine_allowlist;
>  };

Thanks,
Peng.

> 
>  struct scmi_driver {
> 
> Thanks,
> Peng
> >
> >Thanks,
> >Cristian
Cristian Marussi Jan. 14, 2025, 10:07 a.m. UTC | #7
On Tue, Jan 14, 2025 at 08:31:03AM +0000, Peng Fan wrote:
> Hi Cristian, Sudeep
> 
> > Subject: Re: [PATCH 2/4] firmware: arm_scmi: bus: Bypass setting
> > fwnode for pinctrl
> > 
> [...]
> 
> > >> fix here) than this change.
> > >
> > >...or indeed this is another possibility
> > 
> > I am doing a patch as below, how to do you think?
> 
> Do you have any comments on below ideas?
> 
> I am thinking to send out new patchset based on
> below ideas in this week.
> 

Hi Peng,

sorry for the delay.

Why both blacklist and allowlist ?

Cristian

> > 
> > With below patch, we could resolve the devlink issue and also support
> > mutitple vendor drivers built in, with each vendor driver has a
> > machine_allowlist.
> > 
> > diff --git a/drivers/firmware/arm_scmi/bus.c
> > b/drivers/firmware/arm_scmi/bus.c index
> > 1d2aedfcfdb4..c1c45b545480 100644
> > --- a/drivers/firmware/arm_scmi/bus.c
> > +++ b/drivers/firmware/arm_scmi/bus.c
> > @@ -55,6 +55,20 @@ static int scmi_protocol_device_request(const
> > struct scmi_device_id *id_table)
> >         unsigned int id = 0;
> >         struct list_head *head, *phead = NULL;
> >         struct scmi_requested_dev *rdev;
> > +       const char * const *allowlist = id_table->machine_allowlist;
> > +       const char * const *blocklist = id_table->machine_blocklist;
> > +
> > +       if (blocklist && of_machine_compatible_match(blocklist)) {
> > +               pr_debug("block SCMI device (%s) for protocol %x\n",
> > +                        id_table->name, id_table->protocol_id);
> > +               return 0;
> > +       }
> > +
> > +       if (allowlist && !of_machine_compatible_match(allowlist)) {
> > +               pr_debug("block SCMI device (%s) for protocol %x\n",
> > +                        id_table->name, id_table->protocol_id);
> > +               return 0;
> > +       }
> > 
> >         pr_debug("Requesting SCMI device (%s) for protocol %x\n",
> >                  id_table->name, id_table->protocol_id); diff --git
> > a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h index
> > 688466a0e816..e1b822d3522f 100644
> > --- a/include/linux/scmi_protocol.h
> > +++ b/include/linux/scmi_protocol.h
> > @@ -950,6 +950,9 @@ struct scmi_device {  struct scmi_device_id {
> >         u8 protocol_id;
> >         const char *name;
> > +       /* Optional */
> > +       const char * const *machine_blocklist;
> > +       const char * const *machine_allowlist;
> >  };
> 
> Thanks,
> Peng.
> 
> > 
> >  struct scmi_driver {
> > 
> > Thanks,
> > Peng
> > >
> > >Thanks,
> > >Cristian
Peng Fan Jan. 15, 2025, 7:22 a.m. UTC | #8
> Subject: Re: [PATCH 2/4] firmware: arm_scmi: bus: Bypass setting
> fwnode for pinctrl
> 
> On Tue, Jan 14, 2025 at 08:31:03AM +0000, Peng Fan wrote:
> > Hi Cristian, Sudeep
> >
> > > Subject: Re: [PATCH 2/4] firmware: arm_scmi: bus: Bypass setting
> > > fwnode for pinctrl
> > >
> > [...]
> >
> > > >> fix here) than this change.
> > > >
> > > >...or indeed this is another possibility
> > >
> > > I am doing a patch as below, how to do you think?
> >
> > Do you have any comments on below ideas?
> >
> > I am thinking to send out new patchset based on below ideas in this
> > week.
> >
> 
> Hi Peng,
> 
> sorry for the delay.
> 
> Why both blacklist and allowlist ?

Because there is blocklist in pinctrl-scmi.c and allowlist in
pinctrl-imx-scmi.c.

we need to make sure only one pinctrl device are
created when both drivers are built in. So to pinctrl-scmi.c,
use blacklist, to pinctrl-imx-scmi.c use allowlist.

To vendor protocols, just need allowlist.

Regards,
Peng.

> 
> Cristian
> 
> > >
> > > With below patch, we could resolve the devlink issue and also
> > > support mutitple vendor drivers built in, with each vendor driver
> > > has a machine_allowlist.
> > >
> > > diff --git a/drivers/firmware/arm_scmi/bus.c
> > > b/drivers/firmware/arm_scmi/bus.c index
> > > 1d2aedfcfdb4..c1c45b545480 100644
> > > --- a/drivers/firmware/arm_scmi/bus.c
> > > +++ b/drivers/firmware/arm_scmi/bus.c
> > > @@ -55,6 +55,20 @@ static int
> scmi_protocol_device_request(const
> > > struct scmi_device_id *id_table)
> > >         unsigned int id = 0;
> > >         struct list_head *head, *phead = NULL;
> > >         struct scmi_requested_dev *rdev;
> > > +       const char * const *allowlist = id_table->machine_allowlist;
> > > +       const char * const *blocklist = id_table->machine_blocklist;
> > > +
> > > +       if (blocklist && of_machine_compatible_match(blocklist)) {
> > > +               pr_debug("block SCMI device (%s) for protocol %x\n",
> > > +                        id_table->name, id_table->protocol_id);
> > > +               return 0;
> > > +       }
> > > +
> > > +       if (allowlist && !of_machine_compatible_match(allowlist)) {
> > > +               pr_debug("block SCMI device (%s) for protocol %x\n",
> > > +                        id_table->name, id_table->protocol_id);
> > > +               return 0;
> > > +       }
> > >
> > >         pr_debug("Requesting SCMI device (%s) for protocol %x\n",
> > >                  id_table->name, id_table->protocol_id); diff --git
> > > a/include/linux/scmi_protocol.h b/include/linux/scmi_protocol.h
> > > index 688466a0e816..e1b822d3522f 100644
> > > --- a/include/linux/scmi_protocol.h
> > > +++ b/include/linux/scmi_protocol.h
> > > @@ -950,6 +950,9 @@ struct scmi_device {  struct scmi_device_id
> {
> > >         u8 protocol_id;
> > >         const char *name;
> > > +       /* Optional */
> > > +       const char * const *machine_blocklist;
> > > +       const char * const *machine_allowlist;
> > >  };
> >
> > Thanks,
> > Peng.
> >
> > >
> > >  struct scmi_driver {
> > >
> > > Thanks,
> > > Peng
> > > >
> > > >Thanks,
> > > >Cristian
diff mbox series

Patch

diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c
index 12190d4dabb65484543044b4424fbe3b67245466..87665b09c8ff492953c8300f80ed73eab6cce4fd 100644
--- a/drivers/firmware/arm_scmi/bus.c
+++ b/drivers/firmware/arm_scmi/bus.c
@@ -345,6 +345,11 @@  static void __scmi_device_destroy(struct scmi_device *scmi_dev)
 	device_unregister(&scmi_dev->dev);
 }
 
+static const char * const scmi_pinctrl_imx_lists[] = {
+	"fsl,imx95",
+	NULL
+};
+
 static int
 __scmi_device_set_node(struct scmi_device *scmi_dev, struct device_node *np,
 		       int protocol, const char *name)
@@ -353,6 +358,15 @@  __scmi_device_set_node(struct scmi_device *scmi_dev, struct device_node *np,
 	if ((protocol == SCMI_PROTOCOL_PERF) && !strcmp(name, "cpufreq"))
 		return 0;
 
+	if (protocol == SCMI_PROTOCOL_PINCTRL) {
+		if (!strcmp(name, "pinctrl") &&
+		    of_machine_compatible_match(scmi_pinctrl_imx_lists))
+			return 0;
+		if (!strcmp(name, "pinctrl-imx") &&
+		    !of_machine_compatible_match(scmi_pinctrl_imx_lists))
+			return 0;
+	}
+
 	device_set_node(&scmi_dev->dev, of_fwnode_handle(np));
 
 	return 0;