[PATCH v2 0/4] scmi: Bypass set fwnode and introduce allow/block list to address devlink issue

[PATCH v2 0/4] scmi: Bypass set fwnode and introduce allow/block list to address devlink issue

[Peng Fan (OSS)]

Current scmi drivers not work well with devlink. This patchset is a
retry to address the issue in [1] which was a few months ago.

Current scmi devices are not created from device tree, they are created
from a scmi_device_id entry of each driver with the protocol matches
with the fwnode reg value, this means there could be multiple devices created
for one fwnode, but the fwnode only has one device pointer.

This patchset is to do more checking before setting the device fwnode.
And Introduce machine_allowlist and machine_blocklist.

The reason to introduce machine_blocklist is for case that
if pinctrl-scmi.c probes before pinctrl-imx-scmi.c probes on i.MX platform.
Need to block pinctrl-scmi.c on i.MX platform.

This may looks like hack, but seems no better way to make scmi works
well with devlink.

[1]: https://lore.kernel.org/arm-scmi/CAGETcx8m48cy-EzP6_uoGN7KWsQw=CfZWQ-hNUzz_7LZ0voG8A@mail.gmail.com/

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
Changes in v2:
- Introduce machine_allowlist and machine_blocklist
- Keep of_node for cpufreq device per Cristian
- Patch 2 is an optimization patch when fixing the devlink issue
- Link to v1: https://lore.kernel.org/r/20241225-scmi-fwdevlink-v1-0-e9a3a5341362@nxp.com

---
Peng Fan (4):
      firmware: arm_scmi: Bypass setting fwnode for scmi cpufreq
      firmware: arm_scmi: Add machine_allowlist and machine_blocklist
      pinctrl: freescale: scmi: Switch to use machine_allowlist
      pinctrl: scmi: Switch to use machine_blocklist

 drivers/firmware/arm_scmi/bus.c              | 31 +++++++++++++++++++++++++++-
 drivers/pinctrl/freescale/pinctrl-imx-scmi.c | 15 ++++++--------
 drivers/pinctrl/pinctrl-scmi.c               | 15 ++++++--------
 include/linux/scmi_protocol.h                |  3 +++
 4 files changed, 45 insertions(+), 19 deletions(-)
---
base-commit: 9dff7bbdd359c73f1b44ab592bbb17e1c174fe43
change-id: 20241225-scmi-fwdevlink-afb5131f19ea

Best regards,
-- 
Peng Fan <peng.fan@nxp.com>

[PATCH v2 1/4] firmware: arm_scmi: Bypass setting fwnode for scmi cpufreq

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

Two drivers scmi_cpufreq.c and scmi_perf_domain.c both use
SCMI_PROTCOL_PERF protocol, but with different name, so two scmi devices
will be created. But the fwnode->dev could only point to one device.

If scmi cpufreq device created earlier, the fwnode->dev will point to
the scmi cpufreq device. Then the fw_devlink will link performance
domain user device(consumer) to the scmi cpufreq device(supplier).
But actually the performance domain user device, such as GPU, should use
the scmi perf device as supplier. Also if 'cpufreq.off=1' in bootargs,
the GPU driver will defer probe always, because of the scmi cpufreq
device not ready.

Because for cpufreq, no need use fw_devlink. So bypass setting fwnode
for scmi cpufreq device.

Fixes: 96da4a99ce50 ("firmware: arm_scmi: Set fwnode for the scmi_device")
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/firmware/arm_scmi/bus.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c
index 2c853c84b58f530898057e4ab274ba76070de05e..7850eb7710f499888d32aebf5d99df63db8bfa26 100644
--- a/drivers/firmware/arm_scmi/bus.c
+++ b/drivers/firmware/arm_scmi/bus.c
@@ -344,6 +344,21 @@ static void __scmi_device_destroy(struct scmi_device *scmi_dev)
 	device_unregister(&scmi_dev->dev);
 }
 
+static int
+__scmi_device_set_node(struct scmi_device *scmi_dev, struct device_node *np,
+		       int protocol, const char *name)
+{
+	/* cpufreq device does not need to be supplier from devlink perspective */
+	if ((protocol == SCMI_PROTOCOL_PERF) && !strcmp(name, "cpufreq")) {
+		scmi_dev->dev.of_node = np;
+		return 0;
+	}
+
+	device_set_node(&scmi_dev->dev, of_fwnode_handle(np));
+
+	return 0;
+}
+
 static struct scmi_device *
 __scmi_device_create(struct device_node *np, struct device *parent,
 		     int protocol, const char *name)
@@ -396,7 +411,7 @@ __scmi_device_create(struct device_node *np, struct device *parent,
 	scmi_dev->id = id;
 	scmi_dev->protocol_id = protocol;
 	scmi_dev->dev.parent = parent;
-	device_set_node(&scmi_dev->dev, of_fwnode_handle(np));
+	__scmi_device_set_node(scmi_dev, np, protocol, name);
 	scmi_dev->dev.bus = &scmi_bus_type;
 	scmi_dev->dev.release = scmi_device_release;
 	dev_set_name(&scmi_dev->dev, "scmi_dev.%d", id);

-- 
2.37.1

[PATCH v2 2/4] firmware: arm_scmi: Add machine_allowlist and machine_blocklist

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

There are two cases:
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.

Vendor A use 0x80 for feature X, Vendor B use 0x80 for feature Y.
With both drivers built in, two scmi devices will be created, and both
drivers will be probed. On A's patform, feature Y probe may fail, vice
verus.

Introduce machine_allowlist and machine_blocklist to allow or block
the creation of scmi devices to address above issues.

machine_blocklist is non-vendor protocols, but vendor has its own
implementation. Saying need to block pinctrl-scmi.c on i.MX95.
machine_allowlist is for vendor protocols. Saying vendor A drivers only
allow vendor A machine, vendor B machines only allow vendor B machine.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/firmware/arm_scmi/bus.c | 14 ++++++++++++++
 include/linux/scmi_protocol.h   |  3 +++
 2 files changed, 17 insertions(+)

diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c
index 7850eb7710f499888d32aebf5d99df63db8bfa26..76a5d946de7a8e16f5d940abc4f542aac5bb2b92 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 688466a0e816247d24704f7ba109667a14226b67..e1b822d3522ff25168f895a4b1ed4c4e9a35bfff 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 {

-- 
2.37.1

[PATCH v2 3/4] pinctrl: freescale: scmi: Switch to use machine_allowlist

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

With machine_allowlist, only allowed machines have pinctrl imx scmi
devices created. The fw_devlink will link consumer and supplier
correctly.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/pinctrl/freescale/pinctrl-imx-scmi.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/pinctrl/freescale/pinctrl-imx-scmi.c b/drivers/pinctrl/freescale/pinctrl-imx-scmi.c
index 8f15c4c4dc4412dddb40505699fc3f459fdc0adc..058b4f0477039d57ddae06f385ad809cbb4784d6 100644
--- a/drivers/pinctrl/freescale/pinctrl-imx-scmi.c
+++ b/drivers/pinctrl/freescale/pinctrl-imx-scmi.c
@@ -287,11 +287,6 @@ scmi_pinctrl_imx_get_pins(struct scmi_pinctrl_imx *pmx, struct pinctrl_desc *des
 	return 0;
 }
 
-static const char * const scmi_pinctrl_imx_allowlist[] = {
-	"fsl,imx95",
-	NULL
-};
-
 static int scmi_pinctrl_imx_probe(struct scmi_device *sdev)
 {
 	struct device *dev = &sdev->dev;
@@ -304,9 +299,6 @@ static int scmi_pinctrl_imx_probe(struct scmi_device *sdev)
 	if (!handle)
 		return -EINVAL;
 
-	if (!of_machine_compatible_match(scmi_pinctrl_imx_allowlist))
-		return -ENODEV;
-
 	pinctrl_ops = handle->devm_protocol_get(sdev, SCMI_PROTOCOL_PINCTRL, &ph);
 	if (IS_ERR(pinctrl_ops))
 		return PTR_ERR(pinctrl_ops);
@@ -339,8 +331,13 @@ static int scmi_pinctrl_imx_probe(struct scmi_device *sdev)
 	return pinctrl_enable(pmx->pctldev);
 }
 
+static const char * const scmi_pinctrl_imx_allowlist[] = {
+	"fsl,imx95",
+	NULL
+};
+
 static const struct scmi_device_id scmi_id_table[] = {
-	{ SCMI_PROTOCOL_PINCTRL, "pinctrl-imx" },
+	{ SCMI_PROTOCOL_PINCTRL, "pinctrl-imx", NULL, scmi_pinctrl_imx_allowlist },
 	{ }
 };
 MODULE_DEVICE_TABLE(scmi, scmi_id_table);

-- 
2.37.1

[PATCH v2 4/4] pinctrl: scmi: Switch to use machine_blocklist

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

With machine_blocklist, the blocked machines will not have pinctrl scmi
devices created. The fw_devlink will link consumer and supplier
correctly.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/pinctrl/pinctrl-scmi.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-scmi.c b/drivers/pinctrl/pinctrl-scmi.c
index df4bbcd7d1d59ac2c8ddc320dc10d702ad1ed5b2..f041478758b50e85d99214f4fe42208d0c8c808f 100644
--- a/drivers/pinctrl/pinctrl-scmi.c
+++ b/drivers/pinctrl/pinctrl-scmi.c
@@ -505,11 +505,6 @@ static int pinctrl_scmi_get_pins(struct scmi_pinctrl *pmx,
 	return 0;
 }
 
-static const char * const scmi_pinctrl_blocklist[] = {
-	"fsl,imx95",
-	NULL
-};
-
 static int scmi_pinctrl_probe(struct scmi_device *sdev)
 {
 	int ret;
@@ -521,9 +516,6 @@ static int scmi_pinctrl_probe(struct scmi_device *sdev)
 	if (!sdev->handle)
 		return -EINVAL;
 
-	if (of_machine_compatible_match(scmi_pinctrl_blocklist))
-		return -ENODEV;
-
 	handle = sdev->handle;
 
 	pinctrl_ops = handle->devm_protocol_get(sdev, SCMI_PROTOCOL_PINCTRL, &ph);
@@ -561,8 +553,13 @@ static int scmi_pinctrl_probe(struct scmi_device *sdev)
 	return pinctrl_enable(pmx->pctldev);
 }
 
+static const char * const scmi_pinctrl_blocklist[] = {
+	"fsl,imx95",
+	NULL
+};
+
 static const struct scmi_device_id scmi_id_table[] = {
-	{ SCMI_PROTOCOL_PINCTRL, "pinctrl" },
+	{ SCMI_PROTOCOL_PINCTRL, "pinctrl", scmi_pinctrl_blocklist, NULL },
 	{ }
 };
 MODULE_DEVICE_TABLE(scmi, scmi_id_table);

-- 
2.37.1

RE: [PATCH v2 0/4] scmi: Bypass set fwnode and introduce allow/block list to address devlink issue

[Peng Fan]

> Subject: [PATCH v2 0/4] scmi: Bypass set fwnode and introduce
> allow/block list to address devlink issue

Any comments on this patchset?

Thanks,
Peng.

> 
> Current scmi drivers not work well with devlink. This patchset is a retry
> to address the issue in [1] which was a few months ago.
> 
> Current scmi devices are not created from device tree, they are created
> from a scmi_device_id entry of each driver with the protocol matches
> with the fwnode reg value, this means there could be multiple devices
> created for one fwnode, but the fwnode only has one device pointer.
> 
> This patchset is to do more checking before setting the device fwnode.
> And Introduce machine_allowlist and machine_blocklist.
> 
> The reason to introduce machine_blocklist is for case that if pinctrl-
> scmi.c probes before pinctrl-imx-scmi.c probes on i.MX platform.
> Need to block pinctrl-scmi.c on i.MX platform.
> 
> This may looks like hack, but seems no better way to make scmi works
> well with devlink.
> 
> [1]: https://lore.kernel.org/arm-scmi/CAGETcx8m48cy-
> EzP6_uoGN7KWsQw=CfZWQ-hNUzz_7LZ0voG8A@mail.gmail.com/
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
> Changes in v2:
> - Introduce machine_allowlist and machine_blocklist
> - Keep of_node for cpufreq device per Cristian
> - Patch 2 is an optimization patch when fixing the devlink issue
> - Link to v1: https://lore.kernel.org/r/20241225-scmi-fwdevlink-v1-0-
> e9a3a5341362@nxp.com
> 
> ---
> Peng Fan (4):
>       firmware: arm_scmi: Bypass setting fwnode for scmi cpufreq
>       firmware: arm_scmi: Add machine_allowlist and machine_blocklist
>       pinctrl: freescale: scmi: Switch to use machine_allowlist
>       pinctrl: scmi: Switch to use machine_blocklist
> 
>  drivers/firmware/arm_scmi/bus.c              | 31
> +++++++++++++++++++++++++++-
>  drivers/pinctrl/freescale/pinctrl-imx-scmi.c | 15 ++++++--------
>  drivers/pinctrl/pinctrl-scmi.c               | 15 ++++++--------
>  include/linux/scmi_protocol.h                |  3 +++
>  4 files changed, 45 insertions(+), 19 deletions(-)
> ---
> base-commit: 9dff7bbdd359c73f1b44ab592bbb17e1c174fe43
> change-id: 20241225-scmi-fwdevlink-afb5131f19ea
> 
> Best regards,
> --
> Peng Fan <peng.fan@nxp.com>

Re: [PATCH v2 1/4] firmware: arm_scmi: Bypass setting fwnode for scmi cpufreq

[Dan Carpenter]

On Mon, Jan 20, 2025 at 03:13:29PM +0800, Peng Fan (OSS) wrote:
> diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c
> index 2c853c84b58f530898057e4ab274ba76070de05e..7850eb7710f499888d32aebf5d99df63db8bfa26 100644
> --- a/drivers/firmware/arm_scmi/bus.c
> +++ b/drivers/firmware/arm_scmi/bus.c
> @@ -344,6 +344,21 @@ static void __scmi_device_destroy(struct scmi_device *scmi_dev)
>  	device_unregister(&scmi_dev->dev);
>  }
>  
> +static int
> +__scmi_device_set_node(struct scmi_device *scmi_dev, struct device_node *np,
> +		       int protocol, const char *name)
> +{
> +	/* cpufreq device does not need to be supplier from devlink perspective */
> +	if ((protocol == SCMI_PROTOCOL_PERF) && !strcmp(name, "cpufreq")) {

I don't love this...  It seems like an hack.  Could we put a flag
somewhere instead?  Perhaps in scmi_device?  (I'm just saying that
because that's what we're passing to this function).

regards,
dan carpenter

Re: [PATCH v2 2/4] firmware: arm_scmi: Add machine_allowlist and machine_blocklist

[Dan Carpenter]

On Mon, Jan 20, 2025 at 03:13:30PM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> There are two cases:
> 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.
> 
> Vendor A use 0x80 for feature X, Vendor B use 0x80 for feature Y.
> With both drivers built in, two scmi devices will be created, and both
> drivers will be probed. On A's patform, feature Y probe may fail, vice
> verus.
> 
> Introduce machine_allowlist and machine_blocklist to allow or block
> the creation of scmi devices to address above issues.
> 
> machine_blocklist is non-vendor protocols, but vendor has its own
> implementation. Saying need to block pinctrl-scmi.c on i.MX95.
> machine_allowlist is for vendor protocols. Saying vendor A drivers only
> allow vendor A machine, vendor B machines only allow vendor B machine.
> 

I think patches 2-4 should be combined into one patch.  This commit
message is a bit confusing.  I don't really understand how the
"fwnode device points to the scmi device".  I understand vaguely
what that means but in terms of code, I couldn't point to it.

> Vendor A use 0x80 for feature X, Vendor B use 0x80 for feature Y.
> With both drivers built in, two scmi devices will be created, and both
> drivers will be probed. On A's patform, feature Y probe may fail, vice
> verus.

You're describing the code before.  Is it a problem that only one driver
is probed successfully?  I thought that would be fine.  What's the
problem?

It should have a Fixes tag.
Fixes: b755521fd6eb ("pinctrl: imx: support SCMI pinctrl protocol for i.MX95")

Here is my suggestion for a commit message:

  We have two drivers, pinctrl-scmi.c which is generic and
  pinctrl-imx-scmi.c which is for IMX hardware.  They do the same
  thing.  Both provide support for the SCMI_PROTOCOL_PINCTRL protocol.

  If you have a kernel with both modules built in then they way it
  was designed to work is that the probe() functions would only
  allow the appropriate driver to probe.  Unfortunately, what happens
  is that <vague>there is an issue earlier in the process so the
  fwnode device points to the wrong driver.</vague>  This means that
  even though the correct driver is probed it still wants to use
  whichever driver was loaded first so if the driver you want came
  second then it won't work.

  To fix this, move the checking for which driver to use into the
  scmi_protocol_device_request() function.

  Now both drivers will be probed but only one will be used?

regards,
dan carpenter

Re: [PATCH v2 0/4] scmi: Bypass set fwnode and introduce allow/block list to address devlink issue

[Linus Walleij]

On Tue, Feb 4, 2025 at 4:31 AM Peng Fan <peng.fan@nxp.com> wrote:

> > Subject: [PATCH v2 0/4] scmi: Bypass set fwnode and introduce
> > allow/block list to address devlink issue
>
> Any comments on this patchset?

The pinctrl changes look OK to me so for those 2 patches:
Acked-by: Linus Walleij <linus.walleij@linaro.org>
in case you want to merge it through some other tree.

Yours,
Linus Walleij

Re: [PATCH v2 1/4] firmware: arm_scmi: Bypass setting fwnode for scmi cpufreq

[Peng Fan]

On Wed, Feb 05, 2025 at 03:45:00PM +0300, Dan Carpenter wrote:
>On Mon, Jan 20, 2025 at 03:13:29PM +0800, Peng Fan (OSS) wrote:
>> diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c
>> index 2c853c84b58f530898057e4ab274ba76070de05e..7850eb7710f499888d32aebf5d99df63db8bfa26 100644
>> --- a/drivers/firmware/arm_scmi/bus.c
>> +++ b/drivers/firmware/arm_scmi/bus.c
>> @@ -344,6 +344,21 @@ static void __scmi_device_destroy(struct scmi_device *scmi_dev)
>>  	device_unregister(&scmi_dev->dev);
>>  }
>>  
>> +static int
>> +__scmi_device_set_node(struct scmi_device *scmi_dev, struct device_node *np,
>> +		       int protocol, const char *name)
>> +{
>> +	/* cpufreq device does not need to be supplier from devlink perspective */
>> +	if ((protocol == SCMI_PROTOCOL_PERF) && !strcmp(name, "cpufreq")) {
>
>I don't love this...  It seems like an hack.  Could we put a flag
>somewhere instead?  Perhaps in scmi_device?  (I'm just saying that
>because that's what we're passing to this function).

This means when creating scmi_device, a flag needs to be set which requires
to extend scmi_device_id to include a flag entry or else.

As below in scmi-cpufreq.c
{ SCMI_PROTOCOL_PERF, "cpufreq", SCMI_FWNODE_NO }

I am not sure Sudeep or Cristian are happy with the idea or not.

But back to the patch here, we are in the path creating the scmi_device and
cpufreq scmi device seems the only one that cause issue. So it should be
fine using this patch?

Thanks,
Peng


>
>regards,
>dan carpenter
>

Re: [PATCH v2 2/4] firmware: arm_scmi: Add machine_allowlist and machine_blocklist

[Peng Fan]

Hi Dan,

On Thu, Feb 06, 2025 at 11:02:04AM +0300, Dan Carpenter wrote:
>On Mon, Jan 20, 2025 at 03:13:30PM +0800, Peng Fan (OSS) wrote:
>> From: Peng Fan <peng.fan@nxp.com>
>> 
>> There are two cases:
>> 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.
>> 
>> Vendor A use 0x80 for feature X, Vendor B use 0x80 for feature Y.
>> With both drivers built in, two scmi devices will be created, and both
>> drivers will be probed. On A's patform, feature Y probe may fail, vice
>> verus.
>> 
>> Introduce machine_allowlist and machine_blocklist to allow or block
>> the creation of scmi devices to address above issues.
>> 
>> machine_blocklist is non-vendor protocols, but vendor has its own
>> implementation. Saying need to block pinctrl-scmi.c on i.MX95.
>> machine_allowlist is for vendor protocols. Saying vendor A drivers only
>> allow vendor A machine, vendor B machines only allow vendor B machine.
>> 
>
>I think patches 2-4 should be combined into one patch.  This commit

They are in different subsystems, so I separate them.

>message is a bit confusing.  I don't really understand how the
>"fwnode device points to the scmi device".  I understand vaguely
>what that means but in terms of code, I couldn't point to it.

Sorry for not being clear.

The devlink framework will take i.MX as pinctrl provider, because the
fwnode is occupied by i.MX pinctrl scmi device which is created earlier
than generic pinctrl scmi device.

>
>> Vendor A use 0x80 for feature X, Vendor B use 0x80 for feature Y.
>> With both drivers built in, two scmi devices will be created, and both
>> drivers will be probed. On A's patform, feature Y probe may fail, vice
>> verus.
>
>You're describing the code before.  Is it a problem that only one driver
>is probed successfully?  I thought that would be fine.  What's the
>problem?

VendorA 0x80
VendorB 0x80

If both drivers runs into probe, VenderB 0x80 driver may crash VendorA firmware
if the firmware not designed well.

Not big issue. I just think we should block the probe.

For pure device tree compatible, if compatible not match, the driver will not
runs into probe. I think scmi driver is also good to follow.

>
>It should have a Fixes tag.
>Fixes: b755521fd6eb ("pinctrl: imx: support SCMI pinctrl protocol for i.MX95")

The issue only exists when devlink is forced.
I would like to wait Suddeep and Cristian's comments on merge 2-4 into one
and add Fixes tag.

>
>Here is my suggestion for a commit message:
>
>  We have two drivers, pinctrl-scmi.c which is generic and
>  pinctrl-imx-scmi.c which is for IMX hardware.  They do the same
>  thing.  Both provide support for the SCMI_PROTOCOL_PINCTRL protocol.
>
>  If you have a kernel with both modules built in then they way it
>  was designed to work is that the probe() functions would only
>  allow the appropriate driver to probe.  Unfortunately, what happens
>  is that <vague>there is an issue earlier in the process so the
>  fwnode device points to the wrong driver.</vague>  This means that
>  even though the correct driver is probed it still wants to use
>  whichever driver was loaded first so if the driver you want came
>  second then it won't work.
>
>  To fix this, move the checking for which driver to use into the
>  scmi_protocol_device_request() function.

Thanks for your patient on reviewing the patchset.

>
>  Now both drivers will be probed but only one will be used?

No. with block/allow list, only one driver will be probed.

Thanks,
Peng.

>
>regards,
>dan carpenter
>

Re: [PATCH v2 1/4] firmware: arm_scmi: Bypass setting fwnode for scmi cpufreq

[Dan Carpenter]

On Thu, Feb 06, 2025 at 06:52:20PM +0800, Peng Fan wrote:
> On Wed, Feb 05, 2025 at 03:45:00PM +0300, Dan Carpenter wrote:
> >On Mon, Jan 20, 2025 at 03:13:29PM +0800, Peng Fan (OSS) wrote:
> >> diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c
> >> index 2c853c84b58f530898057e4ab274ba76070de05e..7850eb7710f499888d32aebf5d99df63db8bfa26 100644
> >> --- a/drivers/firmware/arm_scmi/bus.c
> >> +++ b/drivers/firmware/arm_scmi/bus.c
> >> @@ -344,6 +344,21 @@ static void __scmi_device_destroy(struct scmi_device *scmi_dev)
> >>  	device_unregister(&scmi_dev->dev);
> >>  }
> >>  
> >> +static int
> >> +__scmi_device_set_node(struct scmi_device *scmi_dev, struct device_node *np,
> >> +		       int protocol, const char *name)
> >> +{
> >> +	/* cpufreq device does not need to be supplier from devlink perspective */
> >> +	if ((protocol == SCMI_PROTOCOL_PERF) && !strcmp(name, "cpufreq")) {
> >
> >I don't love this...  It seems like an hack.  Could we put a flag
> >somewhere instead?  Perhaps in scmi_device?  (I'm just saying that
> >because that's what we're passing to this function).
> 
> This means when creating scmi_device, a flag needs to be set which requires
> to extend scmi_device_id to include a flag entry or else.
> 
> As below in scmi-cpufreq.c
> { SCMI_PROTOCOL_PERF, "cpufreq", SCMI_FWNODE_NO }
> 

Yeah, I like that.

-	if ((protocol == SCMI_PROTOCOL_PERF) && !strcmp(name, "cpufreq")) {
+	if (scmi_dev->flags & SCMI_FWNODE_NO) {

Or we could do something like "if (scmi_dev->no_fwnode) {"

regards,
dan carpenter

Re: [PATCH v2 2/4] firmware: arm_scmi: Add machine_allowlist and machine_blocklist

[Dan Carpenter]

On Thu, Feb 06, 2025 at 07:05:08PM +0800, Peng Fan wrote:
> Hi Dan,
> 
> On Thu, Feb 06, 2025 at 11:02:04AM +0300, Dan Carpenter wrote:
> >On Mon, Jan 20, 2025 at 03:13:30PM +0800, Peng Fan (OSS) wrote:
> >> From: Peng Fan <peng.fan@nxp.com>
> >> 
> >> There are two cases:
> >> 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.
> >> 
> >> Vendor A use 0x80 for feature X, Vendor B use 0x80 for feature Y.
> >> With both drivers built in, two scmi devices will be created, and both
> >> drivers will be probed. On A's patform, feature Y probe may fail, vice
> >> verus.
> >> 
> >> Introduce machine_allowlist and machine_blocklist to allow or block
> >> the creation of scmi devices to address above issues.
> >> 
> >> machine_blocklist is non-vendor protocols, but vendor has its own
> >> implementation. Saying need to block pinctrl-scmi.c on i.MX95.
> >> machine_allowlist is for vendor protocols. Saying vendor A drivers only
> >> allow vendor A machine, vendor B machines only allow vendor B machine.
> >> 
> >
> >I think patches 2-4 should be combined into one patch.  This commit
> 
> They are in different subsystems, so I separate them.
> 

I mean if the i.MX driver prevents the generic driver from working then
we need a Fixes tag.  It really makes it simpler to understand and backport
if they're sent as one patch.  Normally we would collect Acks from the
maintainers who're involved and but still do it as one patch.

> >message is a bit confusing.  I don't really understand how the
> >"fwnode device points to the scmi device".  I understand vaguely
> >what that means but in terms of code, I couldn't point to it.
> 
> Sorry for not being clear.
> 
> The devlink framework will take i.MX as pinctrl provider, because the
> fwnode is occupied by i.MX pinctrl scmi device which is created earlier
> than generic pinctrl scmi device.
> 

Ah.  Got it.  Thanks.

> >
> >> Vendor A use 0x80 for feature X, Vendor B use 0x80 for feature Y.
> >> With both drivers built in, two scmi devices will be created, and both
> >> drivers will be probed. On A's patform, feature Y probe may fail, vice
> >> verus.
> >
> >You're describing the code before.  Is it a problem that only one driver
> >is probed successfully?  I thought that would be fine.  What's the
> >problem?
> 
> VendorA 0x80
> VendorB 0x80
> 
> If both drivers runs into probe, VenderB 0x80 driver may crash VendorA firmware
> if the firmware not designed well.
> 
> Not big issue. I just think we should block the probe.
> 

This is a theoretical issue for now.  I would just leave it out of the
commit message because it's kind of confusing and it might not even happen
in real life.

regards,
dan carpenter

Re: [PATCH v2 1/4] firmware: arm_scmi: Bypass setting fwnode for scmi cpufreq

[Cristian Marussi]

On Thu, Feb 06, 2025 at 02:31:19PM +0300, Dan Carpenter wrote:
> On Thu, Feb 06, 2025 at 06:52:20PM +0800, Peng Fan wrote:
> > On Wed, Feb 05, 2025 at 03:45:00PM +0300, Dan Carpenter wrote:
> > >On Mon, Jan 20, 2025 at 03:13:29PM +0800, Peng Fan (OSS) wrote:
> > >> diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c
> > >> index 2c853c84b58f530898057e4ab274ba76070de05e..7850eb7710f499888d32aebf5d99df63db8bfa26 100644
> > >> --- a/drivers/firmware/arm_scmi/bus.c
> > >> +++ b/drivers/firmware/arm_scmi/bus.c
> > >> @@ -344,6 +344,21 @@ static void __scmi_device_destroy(struct scmi_device *scmi_dev)
> > >>  	device_unregister(&scmi_dev->dev);
> > >>  }
> > >>  
> > >> +static int
> > >> +__scmi_device_set_node(struct scmi_device *scmi_dev, struct device_node *np,
> > >> +		       int protocol, const char *name)
> > >> +{
> > >> +	/* cpufreq device does not need to be supplier from devlink perspective */
> > >> +	if ((protocol == SCMI_PROTOCOL_PERF) && !strcmp(name, "cpufreq")) {
> > >
> > >I don't love this...  It seems like an hack.  Could we put a flag
> > >somewhere instead?  Perhaps in scmi_device?  (I'm just saying that
> > >because that's what we're passing to this function).
> > 
> > This means when creating scmi_device, a flag needs to be set which requires
> > to extend scmi_device_id to include a flag entry or else.
> > 
> > As below in scmi-cpufreq.c
> > { SCMI_PROTOCOL_PERF, "cpufreq", SCMI_FWNODE_NO }
> > 
> 
> Yeah, I like that.
> 
> -	if ((protocol == SCMI_PROTOCOL_PERF) && !strcmp(name, "cpufreq")) {
> +	if (scmi_dev->flags & SCMI_FWNODE_NO) {
> 
> Or we could do something like "if (scmi_dev->no_fwnode) {"

I proposed a flag a few review ago about this, it shoule come somehow
from the device_table above like Peng was proposing, so that a driver
can just declare that does NOT need fw_devlink.

Thanks,
Cristian

Re: [PATCH v2 2/4] firmware: arm_scmi: Add machine_allowlist and machine_blocklist

[Dan Carpenter]

On Thu, Feb 06, 2025 at 02:40:11PM +0300, Dan Carpenter wrote:
> On Thu, Feb 06, 2025 at 07:05:08PM +0800, Peng Fan wrote:
> > Hi Dan,
> > 
> > On Thu, Feb 06, 2025 at 11:02:04AM +0300, Dan Carpenter wrote:
> > >On Mon, Jan 20, 2025 at 03:13:30PM +0800, Peng Fan (OSS) wrote:
> > >> From: Peng Fan <peng.fan@nxp.com>
> > >> 
> > >> There are two cases:
> > >> 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.
> > >> 
> > >> Vendor A use 0x80 for feature X, Vendor B use 0x80 for feature Y.
> > >> With both drivers built in, two scmi devices will be created, and both
> > >> drivers will be probed. On A's patform, feature Y probe may fail, vice
> > >> verus.
> > >> 
> > >> Introduce machine_allowlist and machine_blocklist to allow or block
> > >> the creation of scmi devices to address above issues.
> > >> 
> > >> machine_blocklist is non-vendor protocols, but vendor has its own
> > >> implementation. Saying need to block pinctrl-scmi.c on i.MX95.
> > >> machine_allowlist is for vendor protocols. Saying vendor A drivers only
> > >> allow vendor A machine, vendor B machines only allow vendor B machine.
> > >> 
> > >
> > >I think patches 2-4 should be combined into one patch.  This commit
> > 
> > They are in different subsystems, so I separate them.
> > 
> 
> I mean if the i.MX driver prevents the generic driver from working then
> we need a Fixes tag.  It really makes it simpler to understand and backport
> if they're sent as one patch.  Normally we would collect Acks from the
> maintainers who're involved and but still do it as one patch.
> 

Wait.  Just to be clear.  Does PATCH 1/4 fix that bug so that when both
are built-in then the generic driver works?  This is in some ways an
alternative way to fix the same bug as well as being a cleanup?

regards,
dan carpenter

Re: [PATCH v2 1/4] firmware: arm_scmi: Bypass setting fwnode for scmi cpufreq

[Dan Carpenter]

On Thu, Feb 06, 2025 at 11:42:12AM +0000, Cristian Marussi wrote:
> On Thu, Feb 06, 2025 at 02:31:19PM +0300, Dan Carpenter wrote:
> > On Thu, Feb 06, 2025 at 06:52:20PM +0800, Peng Fan wrote:
> > > On Wed, Feb 05, 2025 at 03:45:00PM +0300, Dan Carpenter wrote:
> > > >On Mon, Jan 20, 2025 at 03:13:29PM +0800, Peng Fan (OSS) wrote:
> > > >> diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c
> > > >> index 2c853c84b58f530898057e4ab274ba76070de05e..7850eb7710f499888d32aebf5d99df63db8bfa26 100644
> > > >> --- a/drivers/firmware/arm_scmi/bus.c
> > > >> +++ b/drivers/firmware/arm_scmi/bus.c
> > > >> @@ -344,6 +344,21 @@ static void __scmi_device_destroy(struct scmi_device *scmi_dev)
> > > >>  	device_unregister(&scmi_dev->dev);
> > > >>  }
> > > >>  
> > > >> +static int
> > > >> +__scmi_device_set_node(struct scmi_device *scmi_dev, struct device_node *np,
> > > >> +		       int protocol, const char *name)
> > > >> +{
> > > >> +	/* cpufreq device does not need to be supplier from devlink perspective */
> > > >> +	if ((protocol == SCMI_PROTOCOL_PERF) && !strcmp(name, "cpufreq")) {
> > > >
> > > >I don't love this...  It seems like an hack.  Could we put a flag
> > > >somewhere instead?  Perhaps in scmi_device?  (I'm just saying that
> > > >because that's what we're passing to this function).
> > > 
> > > This means when creating scmi_device, a flag needs to be set which requires
> > > to extend scmi_device_id to include a flag entry or else.
> > > 
> > > As below in scmi-cpufreq.c
> > > { SCMI_PROTOCOL_PERF, "cpufreq", SCMI_FWNODE_NO }
> > > 
> > 
> > Yeah, I like that.
> > 
> > -	if ((protocol == SCMI_PROTOCOL_PERF) && !strcmp(name, "cpufreq")) {
> > +	if (scmi_dev->flags & SCMI_FWNODE_NO) {
> > 
> > Or we could do something like "if (scmi_dev->no_fwnode) {"
> 
> I proposed a flag a few review ago about this, it shoule come somehow
> from the device_table above like Peng was proposing, so that a driver
> can just declare that does NOT need fw_devlink.

Great.  I think we're on the same page then.

regards,
dan carpenter

Re: [PATCH v2 2/4] firmware: arm_scmi: Add machine_allowlist and machine_blocklist

[Cristian Marussi]

On Mon, Jan 20, 2025 at 03:13:30PM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 

Hi,

> There are two cases:
> 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.
> 

The pinctrl-imx case is an unfortunate exception because you have a
custom Vendor SCMI driver using a STANDARD protocol: this was never
meant to be an allowed normal practice: the idea of SCMI Vendor extensions
is to allow Vendors to write their own Vendor protocols (>0x80) and their
own SCMI drivers on top of their custom vendor protocols.

This list-based generalization seems to me dangerous because allows the
spreading of such bad practice of loading custom Vendor drivers on top of
standard protocols using the same protocol to do the same thing in a
slightly different manner, with all the rfelated issues of fragmentation

...also I feel it could lead to an umaintainable mess of tables of
compatibles....what happens if I write a 3rd pinctrl-cristian driver on
top of it...both the new allowlist and the general pinctrl blocklist
will need to be updated.

The issue as we know is the interaction with devlink given that all of
these same-protocol devices are created with the same device_node, since
there is only one of them per-protocol in the DT....

...not sure what Sudeep thinks..just my opinion...

> Vendor A use 0x80 for feature X, Vendor B use 0x80 for feature Y.
> With both drivers built in, two scmi devices will be created, and both
> drivers will be probed. On A's patform, feature Y probe may fail, vice
> verus.
>

That's definitely an issue much worse than fw_devlink above....we indeed take
care to pick the right vendor-protocol at runtime BUT no check is peformed
against the SCMI drivers so you could end up picking up a completely unrelated
protocol operations...damn...

I think this is more esily solvable though....by adding a Vendor tag in
the device_table (like the protocols do) you could skip device creation
based on a mismatching Vendor, instead of using compatibles that are
doomed to grow indefinitely as a list....

So at this point you would have an optional Vendor and an optional flags
(as mentioned in the other thread) in the device_table...

I wonder if we can use the same logic for the above pinctrl case,
without using compatibles ?
I have not really thougth this through properly....

In general, most of these issues are somehow Vendor-dependent, so I was
also wondering if it was not the case to frame all of this in some sort
of general vendor-quirks framework that could be used also when there
are evident and NOT fixable issues on deployed FW SCMI server, so that
we will have to flex a bit the kernel tolerance to cope with existing
deployed HW that cannot be fixed fw-wise....

...BUT I thought about this even less thoroughly :P...so it could be just a
bad idea of mine...

Thanks,
Cristian

Re: [PATCH v2 2/4] firmware: arm_scmi: Add machine_allowlist and machine_blocklist

[Peng Fan]

On Thu, Feb 06, 2025 at 12:06:03PM +0000, Cristian Marussi wrote:
>On Mon, Jan 20, 2025 at 03:13:30PM +0800, Peng Fan (OSS) wrote:
>> From: Peng Fan <peng.fan@nxp.com>
>> 
>
>Hi,
>
>> There are two cases:
>> 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.
>> 
>
>The pinctrl-imx case is an unfortunate exception because you have a
>custom Vendor SCMI driver using a STANDARD protocol: this was never
>meant to be an allowed normal practice: the idea of SCMI Vendor extensions
>is to allow Vendors to write their own Vendor protocols (>0x80) and their
>own SCMI drivers on top of their custom vendor protocols.
>
>This list-based generalization seems to me dangerous because allows the
>spreading of such bad practice of loading custom Vendor drivers on top of
>standard protocols using the same protocol to do the same thing in a
>slightly different manner, with all the rfelated issues of fragmentation
>
>...also I feel it could lead to an umaintainable mess of tables of
>compatibles....what happens if I write a 3rd pinctrl-cristian driver on
>top of it...both the new allowlist and the general pinctrl blocklist
>will need to be updated.
>
>The issue as we know is the interaction with devlink given that all of
>these same-protocol devices are created with the same device_node, since
>there is only one of them per-protocol in the DT....
>
>...not sure what Sudeep thinks..just my opinion...
>
>> Vendor A use 0x80 for feature X, Vendor B use 0x80 for feature Y.
>> With both drivers built in, two scmi devices will be created, and both
>> drivers will be probed. On A's patform, feature Y probe may fail, vice
>> verus.
>>
>
>That's definitely an issue much worse than fw_devlink above....we indeed take
>care to pick the right vendor-protocol at runtime BUT no check is peformed
>against the SCMI drivers so you could end up picking up a completely unrelated
>protocol operations...damn...
>
>I think this is more esily solvable though....by adding a Vendor tag in
>the device_table (like the protocols do) you could skip device creation
>based on a mismatching Vendor, instead of using compatibles that are
>doomed to grow indefinitely as a list....
>
>So at this point you would have an optional Vendor and an optional flags
>(as mentioned in the other thread) in the device_table...

This is indeed better.

>
>I wonder if we can use the same logic for the above pinctrl case,
>without using compatibles ?
>I have not really thougth this through properly....

Since i.MX pinctrl driver probe earlier than generic pinctrl scmi driver(
compilation order or whatelse may change the order in future),
so adding a vendor flag to i.MX pinctrl could work for now.

But if order changes..

Anyway I will give a look, then back here.

Thanks,
Peng.

>
>In general, most of these issues are somehow Vendor-dependent, so I was
>also wondering if it was not the case to frame all of this in some sort
>of general vendor-quirks framework that could be used also when there
>are evident and NOT fixable issues on deployed FW SCMI server, so that
>we will have to flex a bit the kernel tolerance to cope with existing
>deployed HW that cannot be fixed fw-wise....
>
>...BUT I thought about this even less thoroughly :P...so it could be just a
>bad idea of mine...
>
>Thanks,
>Cristian

Re: [PATCH v2 2/4] firmware: arm_scmi: Add machine_allowlist and machine_blocklist

[Peng Fan]

On Thu, Feb 06, 2025 at 02:46:27PM +0300, Dan Carpenter wrote:
>On Thu, Feb 06, 2025 at 02:40:11PM +0300, Dan Carpenter wrote:
>> On Thu, Feb 06, 2025 at 07:05:08PM +0800, Peng Fan wrote:
>> > Hi Dan,
>> > 
>> > On Thu, Feb 06, 2025 at 11:02:04AM +0300, Dan Carpenter wrote:
>> > >On Mon, Jan 20, 2025 at 03:13:30PM +0800, Peng Fan (OSS) wrote:
>> > >> From: Peng Fan <peng.fan@nxp.com>
>> > >> 
>> > >> There are two cases:
>> > >> 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.
>> > >> 
>> > >> Vendor A use 0x80 for feature X, Vendor B use 0x80 for feature Y.
>> > >> With both drivers built in, two scmi devices will be created, and both
>> > >> drivers will be probed. On A's patform, feature Y probe may fail, vice
>> > >> verus.
>> > >> 
>> > >> Introduce machine_allowlist and machine_blocklist to allow or block
>> > >> the creation of scmi devices to address above issues.
>> > >> 
>> > >> machine_blocklist is non-vendor protocols, but vendor has its own
>> > >> implementation. Saying need to block pinctrl-scmi.c on i.MX95.
>> > >> machine_allowlist is for vendor protocols. Saying vendor A drivers only
>> > >> allow vendor A machine, vendor B machines only allow vendor B machine.
>> > >> 
>> > >
>> > >I think patches 2-4 should be combined into one patch.  This commit
>> > 
>> > They are in different subsystems, so I separate them.
>> > 
>> 
>> I mean if the i.MX driver prevents the generic driver from working then
>> we need a Fixes tag.  It really makes it simpler to understand and backport
>> if they're sent as one patch.  Normally we would collect Acks from the
>> maintainers who're involved and but still do it as one patch.
>> 
>
>Wait.  Just to be clear.  Does PATCH 1/4 fix that bug so that when both
>are built-in then the generic driver works?  This is in some ways an
>alternative way to fix the same bug as well as being a cleanup?

patch 1/4 is not related to the pinctrl stuff. It could be a standalone
patch, I put it in this patchset, just because all are related to fwdevlink.

Thanks,
Peng

>
>regards,
>dan carpenter
>

RE: [PATCH v2 2/4] firmware: arm_scmi: Add machine_allowlist and machine_blocklist

[Peng Fan]

Hi Cristian, Sudeep, 

> Subject: Re: [PATCH v2 2/4] firmware: arm_scmi: Add
> machine_allowlist and machine_blocklist
> 
> On Mon, Jan 20, 2025 at 03:13:30PM +0800, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> 
> Hi,
> 
> > There are two cases:
> > 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.
> >
> 
> The pinctrl-imx case is an unfortunate exception because you have a
> custom Vendor SCMI driver using a STANDARD protocol: this was never
> meant to be an allowed normal practice: the idea of SCMI Vendor
> extensions is to allow Vendors to write their own Vendor protocols
> (>0x80) and their own SCMI drivers on top of their custom vendor
> protocols.
> 
> This list-based generalization seems to me dangerous because allows
> the spreading of such bad practice of loading custom Vendor drivers on
> top of standard protocols using the same protocol to do the same thing
> in a slightly different manner, with all the rfelated issues of
> fragmentation
> 
> ...also I feel it could lead to an umaintainable mess of tables of
> compatibles....what happens if I write a 3rd pinctrl-cristian driver on
> top of it...both the new allowlist and the general pinctrl blocklist will
> need to be updated.
> 
> The issue as we know is the interaction with devlink given that all of
> these same-protocol devices are created with the same device_node,
> since there is only one of them per-protocol in the DT....
> 
> ...not sure what Sudeep thinks..just my opinion...
> 
> > Vendor A use 0x80 for feature X, Vendor B use 0x80 for feature Y.
> > With both drivers built in, two scmi devices will be created, and both
> > drivers will be probed. On A's patform, feature Y probe may fail, vice
> > verus.
> >
> 
> That's definitely an issue much worse than fw_devlink above....we
> indeed take care to pick the right vendor-protocol at runtime BUT no
> check is peformed against the SCMI drivers so you could end up picking
> up a completely unrelated protocol operations...damn...
> 
> I think this is more esily solvable though....by adding a Vendor tag in
> the device_table (like the protocols do) you could skip device creation
> based on a mismatching Vendor, instead of using compatibles that are
> doomed to grow indefinitely as a list....
> 
> So at this point you would have an optional Vendor and an optional
> flags (as mentioned in the other thread) in the device_table...
> 
> I wonder if we can use the same logic for the above pinctrl case,
> without using compatibles ?
> I have not really thougth this through properly....
> 
> In general, most of these issues are somehow Vendor-dependent, so I
> was also wondering if it was not the case to frame all of this in some
> sort of general vendor-quirks framework that could be used also when
> there are evident and NOT fixable issues on deployed FW SCMI server,
> so that we will have to flex a bit the kernel tolerance to cope with
> existing deployed HW that cannot be fixed fw-wise....

I just have a prototype and tested on i.MX95.  

How do you think?

Extend scmi_device_id with flags, allowed_ids, blocked_ids.
The ids are SCMI vendor/subvendor, so need to use compatible
anymore.

/* The scmi device does not have fwnode handle */                                                   
#define SCMI_DEVICE_NO_FWNODE   BIT(0)                                                              
                                                                                                    
struct scmi_device_vendor_id                                                                        
{                                                                                                   
        const char *vendor_id;                                                                      
        const char *sub_vendor_id;                                                                  
};                                                                                                  
                                                                                                    
struct scmi_device_id {                                                                             
        u8 protocol_id;                                                                             
        const char *name;                                                                           
        u32 flags;                                                                                  
        /* Optional */                                                                              
        struct scmi_device_vendor_id *blocked_ids;                                                  
        struct scmi_device_vendor_id *allowed_ids;                                                  
};

In scmi_create_device, check block and allowed.

struct scmi_device *scmi_device_create(struct device_node *np,
-                                      struct device *parent, int protocol,
+                                      struct device *parent,
+                                      struct scmi_revision_info *revision,
+                                      int protocol,
                                       const char *name, u32 flags)
 {
        struct list_head *phead;
@@ -476,8 +494,16 @@ struct scmi_device *scmi_device_create(struct device_node *np,
 
        /* Walk the list of requested devices for protocol and create them */
        list_for_each_entry(rdev, phead, node) {
+               struct scmi_device_vendor_id *allowed_ids = rdev->id_table->allowed_ids;
+               struct scmi_device_vendor_id *blocked_ids = rdev->id_table->blocked_ids;
                struct scmi_device *sdev;
 
+               if (blocked_ids && __scmi_device_vendor_id_match(revision, blocked_ids))
+                       continue;
+
+               if (allowed_ids && !__scmi_device_vendor_id_match(revision, allowed_ids))
+                       continue;
+
                sdev = __scmi_device_create(np, parent,
                                            rdev->id_table->protocol_id,
                                            rdev->id_table->name,

In for cpufreq, use below to set device node.
+static int
+__scmi_device_set_node(struct scmi_device *scmi_dev, struct device_node *np,
+                      int protocol, const char *name, u32 flags)
+{
+       if (flags & SCMI_DEVICE_NO_FWNODE) {
+               scmi_dev->dev.of_node = np;
+               return 0;
+       }
+
+       device_set_node(&scmi_dev->dev, of_fwnode_handle(np));
+
+       return 0;
+}

Are these looks good? I could post the patchset later to see whether Sudeep
has any more comments on the current patchset or the new proposal.

BTW: I also pushed patch to
https://github.com/MrVan/linux.git  branch: b4/scmi-fwdevlink-v2
and 
Following Dan's suggestion to merge patch 2-4.

Thanks,
Peng.

> 
> ...BUT I thought about this even less thoroughly :P...so it could be just a
> bad idea of mine...
> 
> Thanks,
> Cristian

Re: [PATCH v2 2/4] firmware: arm_scmi: Add machine_allowlist and machine_blocklist

[Sudeep Holla]

On Mon, Feb 10, 2025 at 01:19:14PM +0000, Peng Fan wrote:
> 
> I just have a prototype and tested on i.MX95.

You didn't answer me @[1]. How can we disable it for perf/cpufreq if there
are users already. I will look at the code once I am convince we can do that.
For now, I am not. I am worried we may break some platform.

-- 
Regards,
Sudeep

[1] https://lore.kernel.org/all/20241227151306.jh2oabc64xd54dms@bogus

RE: [PATCH v2 2/4] firmware: arm_scmi: Add machine_allowlist and machine_blocklist

[Peng Fan]

> Subject: Re: [PATCH v2 2/4] firmware: arm_scmi: Add
> machine_allowlist and machine_blocklist
> 
> On Tue, Feb 11, 2025 at 03:46:36PM +0000, Sudeep Holla wrote:
> >On Mon, Feb 10, 2025 at 01:19:14PM +0000, Peng Fan wrote:
> >>
> >> I just have a prototype and tested on i.MX95.
> >
> >You didn't answer me @[1]. How can we disable it for perf/cpufreq if
> >there are users already. I will look at the code once I am convince we
> can do that.
> >For now, I am not. I am worried we may break some platform.
> 
> The only user in upstream kernel with using the dummy clock is juno-
> scmi.dtsi.
> 
> SCMI_PROTOCOL_PERF is used by two drivers cpufreq-scmi.c,
> scmi_perf_domain.c.
> 
> In cpufreq-scmi.c a dummy clock proviver is created, the gpu node in
> juno-scmi.dtsi takes "<&scmi_dvfs 2>" into clocks property. I think this
> is wrong.
> 
> Why not use scmi_clk node? cpufreq created clk provider should only
> be limited for cpu device which will not be impacted by fwdevlink.
> 
> If wanna to tune gpu performance, the power-domains property should
> be used, not clocks property.
> 
> It is the juno-scmi.dtsi should be fixed.
> 
> If juno-scmi.dtsi will keep as it is, please suggest possible solution on
> fixing the issue.

Ignore the upper which maybe wrong.

The dummy clock provider will always be ready before opp. So no issue.
But anyway if juno-scmi.dtsi using power-domains for perf, it should
be better.

I just replied in V1.

Regards,
Peng.

> 
> Regards,
> Peng
> 
> >
> >--
> >Regards,
> >Sudeep
> >
> >[1]
> https://lore.kernel.org/all/20241227151306.jh2oabc64xd54dms@bog
> us

Re: [PATCH v2 2/4] firmware: arm_scmi: Add machine_allowlist and machine_blocklist

[Peng Fan]

On Tue, Feb 11, 2025 at 03:46:36PM +0000, Sudeep Holla wrote:
>On Mon, Feb 10, 2025 at 01:19:14PM +0000, Peng Fan wrote:
>> 
>> I just have a prototype and tested on i.MX95.
>
>You didn't answer me @[1]. How can we disable it for perf/cpufreq if there
>are users already. I will look at the code once I am convince we can do that.
>For now, I am not. I am worried we may break some platform.

The only user in upstream kernel with using the dummy clock is juno-scmi.dtsi.

SCMI_PROTOCOL_PERF is used by two drivers cpufreq-scmi.c, scmi_perf_domain.c.

In cpufreq-scmi.c a dummy clock proviver is created, the gpu node in juno-scmi.dtsi
takes "<&scmi_dvfs 2>" into clocks property. I think this is wrong.

Why not use scmi_clk node? cpufreq created clk provider should only be limited
for cpu device which will not be impacted by fwdevlink.

If wanna to tune gpu performance, the power-domains property should be used,
not clocks property.

It is the juno-scmi.dtsi should be fixed.

If juno-scmi.dtsi will keep as it is, please suggest possible solution
on fixing the issue.

Regards,
Peng

>
>-- 
>Regards,
>Sudeep
>
>[1] https://lore.kernel.org/all/20241227151306.jh2oabc64xd54dms@bogus

Re: [PATCH v2 3/4] pinctrl: freescale: scmi: Switch to use machine_allowlist

[Saravana Kannan]

On Sun, Jan 19, 2025 at 11:14 PM Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote:
>
> From: Peng Fan <peng.fan@nxp.com>
>
> With machine_allowlist, only allowed machines have pinctrl imx scmi
> devices created. The fw_devlink will link consumer and supplier
> correctly.
>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/pinctrl/freescale/pinctrl-imx-scmi.c | 15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/pinctrl/freescale/pinctrl-imx-scmi.c b/drivers/pinctrl/freescale/pinctrl-imx-scmi.c
> index 8f15c4c4dc4412dddb40505699fc3f459fdc0adc..058b4f0477039d57ddae06f385ad809cbb4784d6 100644
> --- a/drivers/pinctrl/freescale/pinctrl-imx-scmi.c
> +++ b/drivers/pinctrl/freescale/pinctrl-imx-scmi.c
> @@ -287,11 +287,6 @@ scmi_pinctrl_imx_get_pins(struct scmi_pinctrl_imx *pmx, struct pinctrl_desc *des
>         return 0;
>  }
>
> -static const char * const scmi_pinctrl_imx_allowlist[] = {
> -       "fsl,imx95",
> -       NULL
> -};
> -
>  static int scmi_pinctrl_imx_probe(struct scmi_device *sdev)
>  {
>         struct device *dev = &sdev->dev;
> @@ -304,9 +299,6 @@ static int scmi_pinctrl_imx_probe(struct scmi_device *sdev)
>         if (!handle)
>                 return -EINVAL;
>
> -       if (!of_machine_compatible_match(scmi_pinctrl_imx_allowlist))
> -               return -ENODEV;
> -
>         pinctrl_ops = handle->devm_protocol_get(sdev, SCMI_PROTOCOL_PINCTRL, &ph);
>         if (IS_ERR(pinctrl_ops))
>                 return PTR_ERR(pinctrl_ops);
> @@ -339,8 +331,13 @@ static int scmi_pinctrl_imx_probe(struct scmi_device *sdev)
>         return pinctrl_enable(pmx->pctldev);
>  }
>
> +static const char * const scmi_pinctrl_imx_allowlist[] = {
> +       "fsl,imx95",
> +       NULL
> +};
> +
>  static const struct scmi_device_id scmi_id_table[] = {
> -       { SCMI_PROTOCOL_PINCTRL, "pinctrl-imx" },
> +       { SCMI_PROTOCOL_PINCTRL, "pinctrl-imx", NULL, scmi_pinctrl_imx_allowlist },
>         { }
>  };
>  MODULE_DEVICE_TABLE(scmi, scmi_id_table);
>

Definite NACK to this. Please don't depend on indirect
conditions/flags. There's no guarantee that this check will hold true
in the future.

-Saravana

Re: [PATCH v2 4/4] pinctrl: scmi: Switch to use machine_blocklist

[Saravana Kannan]

On Sun, Jan 19, 2025 at 11:14 PM Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote:
>
> From: Peng Fan <peng.fan@nxp.com>
>
> With machine_blocklist, the blocked machines will not have pinctrl scmi
> devices created. The fw_devlink will link consumer and supplier
> correctly.
>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/pinctrl/pinctrl-scmi.c | 15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/pinctrl/pinctrl-scmi.c b/drivers/pinctrl/pinctrl-scmi.c
> index df4bbcd7d1d59ac2c8ddc320dc10d702ad1ed5b2..f041478758b50e85d99214f4fe42208d0c8c808f 100644
> --- a/drivers/pinctrl/pinctrl-scmi.c
> +++ b/drivers/pinctrl/pinctrl-scmi.c
> @@ -505,11 +505,6 @@ static int pinctrl_scmi_get_pins(struct scmi_pinctrl *pmx,
>         return 0;
>  }
>
> -static const char * const scmi_pinctrl_blocklist[] = {
> -       "fsl,imx95",
> -       NULL
> -};
> -
>  static int scmi_pinctrl_probe(struct scmi_device *sdev)
>  {
>         int ret;
> @@ -521,9 +516,6 @@ static int scmi_pinctrl_probe(struct scmi_device *sdev)
>         if (!sdev->handle)
>                 return -EINVAL;
>
> -       if (of_machine_compatible_match(scmi_pinctrl_blocklist))
> -               return -ENODEV;
> -
>         handle = sdev->handle;
>
>         pinctrl_ops = handle->devm_protocol_get(sdev, SCMI_PROTOCOL_PINCTRL, &ph);
> @@ -561,8 +553,13 @@ static int scmi_pinctrl_probe(struct scmi_device *sdev)
>         return pinctrl_enable(pmx->pctldev);
>  }
>
> +static const char * const scmi_pinctrl_blocklist[] = {
> +       "fsl,imx95",
> +       NULL
> +};
> +
>  static const struct scmi_device_id scmi_id_table[] = {
> -       { SCMI_PROTOCOL_PINCTRL, "pinctrl" },
> +       { SCMI_PROTOCOL_PINCTRL, "pinctrl", scmi_pinctrl_blocklist, NULL },
>         { }
>  };
>  MODULE_DEVICE_TABLE(scmi, scmi_id_table);
>

Definite NACK to this. Please don't depend on indirect
conditions/flags. There's no guarantee that this check will hold true
in the future.

-Saravana

Re: [PATCH v2 1/4] firmware: arm_scmi: Bypass setting fwnode for scmi cpufreq

[Saravana Kannan]

On Thu, Feb 6, 2025 at 3:42 AM Cristian Marussi
<cristian.marussi@arm.com> wrote:
>
> On Thu, Feb 06, 2025 at 02:31:19PM +0300, Dan Carpenter wrote:
> > On Thu, Feb 06, 2025 at 06:52:20PM +0800, Peng Fan wrote:
> > > On Wed, Feb 05, 2025 at 03:45:00PM +0300, Dan Carpenter wrote:
> > > >On Mon, Jan 20, 2025 at 03:13:29PM +0800, Peng Fan (OSS) wrote:
> > > >> diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c
> > > >> index 2c853c84b58f530898057e4ab274ba76070de05e..7850eb7710f499888d32aebf5d99df63db8bfa26 100644
> > > >> --- a/drivers/firmware/arm_scmi/bus.c
> > > >> +++ b/drivers/firmware/arm_scmi/bus.c
> > > >> @@ -344,6 +344,21 @@ static void __scmi_device_destroy(struct scmi_device *scmi_dev)
> > > >>          device_unregister(&scmi_dev->dev);
> > > >>  }
> > > >>
> > > >> +static int
> > > >> +__scmi_device_set_node(struct scmi_device *scmi_dev, struct device_node *np,
> > > >> +                       int protocol, const char *name)
> > > >> +{
> > > >> +        /* cpufreq device does not need to be supplier from devlink perspective */
> > > >> +        if ((protocol == SCMI_PROTOCOL_PERF) && !strcmp(name, "cpufreq")) {
> > > >
> > > >I don't love this...  It seems like an hack.  Could we put a flag
> > > >somewhere instead?  Perhaps in scmi_device?  (I'm just saying that
> > > >because that's what we're passing to this function).
> > >
> > > This means when creating scmi_device, a flag needs to be set which requires
> > > to extend scmi_device_id to include a flag entry or else.
> > >
> > > As below in scmi-cpufreq.c
> > > { SCMI_PROTOCOL_PERF, "cpufreq", SCMI_FWNODE_NO }
> > >
> >
> > Yeah, I like that.
> >
> > -     if ((protocol == SCMI_PROTOCOL_PERF) && !strcmp(name, "cpufreq")) {
> > +     if (scmi_dev->flags & SCMI_FWNODE_NO) {
> >
> > Or we could do something like "if (scmi_dev->no_fwnode) {"
>
> I proposed a flag a few review ago about this, it shoule come somehow
> from the device_table above like Peng was proposing, so that a driver
> can just declare that does NOT need fw_devlink.

Sorry, looks I replied to v1 series. Can you take a look at that
response please?
https://lore.kernel.org/lkml/CAGETcx87Stfkru9gJrc1sf=PtFGLY7=jrfFaCzK5Z4hq+2TCzg@mail.gmail.com/

If that suggestion I gave there would work, then that's the cleanest
approach. This patch series is just kicking the can down the road (or
down an inch).

-Saravana

Re: [PATCH v2 1/4] firmware: arm_scmi: Bypass setting fwnode for scmi cpufreq

[Cristian Marussi]

On Thu, Feb 13, 2025 at 12:17:06AM -0800, Saravana Kannan wrote:
> On Thu, Feb 6, 2025 at 3:42 AM Cristian Marussi
> <cristian.marussi@arm.com> wrote:
> >
> > On Thu, Feb 06, 2025 at 02:31:19PM +0300, Dan Carpenter wrote:
> > > On Thu, Feb 06, 2025 at 06:52:20PM +0800, Peng Fan wrote:
> > > > On Wed, Feb 05, 2025 at 03:45:00PM +0300, Dan Carpenter wrote:
> > > > >On Mon, Jan 20, 2025 at 03:13:29PM +0800, Peng Fan (OSS) wrote:
> > > > >> diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c
> > > > >> index 2c853c84b58f530898057e4ab274ba76070de05e..7850eb7710f499888d32aebf5d99df63db8bfa26 100644
> > > > >> --- a/drivers/firmware/arm_scmi/bus.c
> > > > >> +++ b/drivers/firmware/arm_scmi/bus.c
> > > > >> @@ -344,6 +344,21 @@ static void __scmi_device_destroy(struct scmi_device *scmi_dev)
> > > > >>          device_unregister(&scmi_dev->dev);
> > > > >>  }
> > > > >>
> > > > >> +static int
> > > > >> +__scmi_device_set_node(struct scmi_device *scmi_dev, struct device_node *np,
> > > > >> +                       int protocol, const char *name)
> > > > >> +{
> > > > >> +        /* cpufreq device does not need to be supplier from devlink perspective */
> > > > >> +        if ((protocol == SCMI_PROTOCOL_PERF) && !strcmp(name, "cpufreq")) {
> > > > >
> > > > >I don't love this...  It seems like an hack.  Could we put a flag
> > > > >somewhere instead?  Perhaps in scmi_device?  (I'm just saying that
> > > > >because that's what we're passing to this function).
> > > >
> > > > This means when creating scmi_device, a flag needs to be set which requires
> > > > to extend scmi_device_id to include a flag entry or else.
> > > >
> > > > As below in scmi-cpufreq.c
> > > > { SCMI_PROTOCOL_PERF, "cpufreq", SCMI_FWNODE_NO }
> > > >
> > >
> > > Yeah, I like that.
> > >
> > > -     if ((protocol == SCMI_PROTOCOL_PERF) && !strcmp(name, "cpufreq")) {
> > > +     if (scmi_dev->flags & SCMI_FWNODE_NO) {
> > >
> > > Or we could do something like "if (scmi_dev->no_fwnode) {"
> >
> > I proposed a flag a few review ago about this, it shoule come somehow
> > from the device_table above like Peng was proposing, so that a driver
> > can just declare that does NOT need fw_devlink.
> 
> Sorry, looks I replied to v1 series. Can you take a look at that
> response please?
> https://lore.kernel.org/lkml/CAGETcx87Stfkru9gJrc1sf=PtFGLY7=jrfFaCzK5Z4hq+2TCzg@mail.gmail.com/
> 
> If that suggestion I gave there would work, then that's the cleanest
> approach. This patch series is just kicking the can down the road (or
> down an inch).

Thanks for the reply, I will answer on that other thread.
Cristian