[PATCH 0/2] firmware: arm_scmi: create scmi devices for protocols that not have of_node

[PATCH 0/2] firmware: arm_scmi: create scmi devices for protocols that not have of_node

[Peng Fan (OSS)]

Per
https://lore.kernel.org/all/20230125141113.kkbowopusikuogx6@bogus/
"
In short we shouldn't have to add a node if there are no consumers. It
was one of the topic of discussion initially when SCMI binding was added
and they exist only for the consumers otherwise we don't need it as
everything is discoverable from the interface.
"
https://lore.kernel.org/all/Y9JLUIioxFPn4BS0@e120937-lin/
If a node has its own channel, the of_node is still needed.

i.MX95 SCMI firmware not have dedicated channel for 0x12, and no need
of_node. This patchset is to support protocol 0x12 without the procotol
node in device tree.

Without of_node, still need to create the scmi devices. As of now,
it is based on an array 'protocols[]' in 'scmi_probe'. 

And no of_node, means no per protocol channel, so reuse the base
protocol channel. Need patch chan_available to support.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
Peng Fan (2):
      firmware: arm_scmi: channel unavailable if no of_node
      firmware: arm_scmi: create scmi_devices that not have of_node

 drivers/firmware/arm_scmi/driver.c  | 33 ++++++++++++++++++++++++++++++++-
 drivers/firmware/arm_scmi/mailbox.c |  2 ++
 drivers/firmware/arm_scmi/optee.c   |  3 +++
 drivers/firmware/arm_scmi/smc.c     |  7 ++++++-
 drivers/firmware/arm_scmi/virtio.c  |  3 +++
 5 files changed, 46 insertions(+), 2 deletions(-)
---
base-commit: d8003eb2eb0200352b5d63af77ec0912a52e79ad
change-id: 20240626-scmi-driver-96dc61b036a2

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

[PATCH 1/2] firmware: arm_scmi: channel unavailable if no of_node

[Peng Fan (OSS)]

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

If there is no of_node for the protocol, there is no per protocol
channel, so return false. Then it will reuse the base protocol
channel per `scmi_chan_setup`.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/firmware/arm_scmi/mailbox.c | 2 ++
 drivers/firmware/arm_scmi/optee.c   | 3 +++
 drivers/firmware/arm_scmi/smc.c     | 7 ++++++-
 drivers/firmware/arm_scmi/virtio.c  | 3 +++
 4 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/arm_scmi/mailbox.c b/drivers/firmware/arm_scmi/mailbox.c
index 0219a12e3209..4f3abc933315 100644
--- a/drivers/firmware/arm_scmi/mailbox.c
+++ b/drivers/firmware/arm_scmi/mailbox.c
@@ -71,6 +71,8 @@ static bool mailbox_chan_available(struct device_node *of_node, int idx)
 {
 	int num_mb;
 
+	if (!of_node)
+		return false;
 	/*
 	 * Just check if bidirrectional channels are involved, and check the
 	 * index accordingly; proper full validation will be made later
diff --git a/drivers/firmware/arm_scmi/optee.c b/drivers/firmware/arm_scmi/optee.c
index 4e7944b91e38..c0a198baa706 100644
--- a/drivers/firmware/arm_scmi/optee.c
+++ b/drivers/firmware/arm_scmi/optee.c
@@ -334,6 +334,9 @@ static bool scmi_optee_chan_available(struct device_node *of_node, int idx)
 {
 	u32 channel_id;
 
+	if (!of_node)
+		return false;
+
 	return !of_property_read_u32_index(of_node, "linaro,optee-channel-id",
 					   idx, &channel_id);
 }
diff --git a/drivers/firmware/arm_scmi/smc.c b/drivers/firmware/arm_scmi/smc.c
index 39936e1dd30e..5af1f781fa10 100644
--- a/drivers/firmware/arm_scmi/smc.c
+++ b/drivers/firmware/arm_scmi/smc.c
@@ -81,7 +81,12 @@ static irqreturn_t smc_msg_done_isr(int irq, void *data)
 
 static bool smc_chan_available(struct device_node *of_node, int idx)
 {
-	struct device_node *np = of_parse_phandle(of_node, "shmem", 0);
+	struct device_node *np;
+
+	if (!of_node)
+		return false;
+
+	np = of_parse_phandle(of_node, "shmem", 0)
 	if (!np)
 		return false;
 
diff --git a/drivers/firmware/arm_scmi/virtio.c b/drivers/firmware/arm_scmi/virtio.c
index 4892058445ce..4d8d6ad3ab5b 100644
--- a/drivers/firmware/arm_scmi/virtio.c
+++ b/drivers/firmware/arm_scmi/virtio.c
@@ -389,6 +389,9 @@ static bool virtio_chan_available(struct device_node *of_node, int idx)
 {
 	struct scmi_vio_channel *channels, *vioch = NULL;
 
+	if (!of_node)
+		return false;
+
 	if (WARN_ON_ONCE(!scmi_vdev))
 		return false;
 

-- 
2.37.1

[PATCH 2/2] firmware: arm_scmi: create scmi_devices that not have of_node

[Peng Fan (OSS)]

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

The scmi protocol device tree node is expected to have consumers or
per node properties expect `reg`. For System power management protocol,
if no per node channel information, no need to add it in device tree,
and it will also trigger dtbs_check error "scmi: 'protocol@12' does not
match any of the regexes: 'pinctrl-[0-9]+'".

To enable system power protocol, need to explictily create the scmi
device and bind with protocol driver.

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

diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
index 6b6957f4743f..eac4dab979c2 100644
--- a/drivers/firmware/arm_scmi/driver.c
+++ b/drivers/firmware/arm_scmi/driver.c
@@ -2952,7 +2952,7 @@ static int scmi_debugfs_raw_mode_setup(struct scmi_info *info)
 
 static int scmi_probe(struct platform_device *pdev)
 {
-	int ret;
+	int i, ret;
 	char *err_str = "probe failure\n";
 	struct scmi_handle *handle;
 	const struct scmi_desc *desc;
@@ -2960,6 +2960,7 @@ static int scmi_probe(struct platform_device *pdev)
 	bool coex = IS_ENABLED(CONFIG_ARM_SCMI_RAW_MODE_SUPPORT_COEX);
 	struct device *dev = &pdev->dev;
 	struct device_node *child, *np = dev->of_node;
+	uint32_t protocols[] = { SCMI_PROTOCOL_SYSTEM, SCMI_PROTOCOL_POWERCAP };
 
 	desc = of_device_get_match_data(dev);
 	if (!desc)
@@ -3114,6 +3115,36 @@ static int scmi_probe(struct platform_device *pdev)
 		scmi_create_protocol_devices(child, info, prot_id, NULL);
 	}
 
+	/* Create devices that not have a device node */
+	for (i = 0; i < ARRAY_SIZE(protocols); i++) {
+		void *p;
+		u32 prot_id = protocols[i];
+
+		p = idr_find(&info->active_protocols, prot_id);
+		if (p)
+			continue;
+
+		if (!scmi_is_protocol_implemented(handle, prot_id)) {
+			dev_info(dev, "SCMI protocol %d not implemented\n",
+				 protocols[i]);
+			continue;
+		}
+
+		ret = scmi_txrx_setup(info, NULL, prot_id);
+		if (ret) {
+			dev_err(dev, "SCMI protocol %d txrx setup fail(%d)\n",
+				prot_id, ret);
+			continue;
+		}
+
+		ret = idr_alloc(&info->active_protocols, NULL,
+				prot_id, prot_id + 1, GFP_KERNEL);
+		if (ret != prot_id)
+			continue;
+
+		scmi_create_protocol_devices(NULL, info, prot_id, NULL);
+	}
+
 	return 0;
 
 notification_exit:

-- 
2.37.1

RE: [PATCH 0/2] firmware: arm_scmi: create scmi devices for protocols that not have of_node

[Peng Fan]

Please ignore this patchset, there is an build error that I push the button
earlier.

Regards,
Peng.
> Subject: [PATCH 0/2] firmware: arm_scmi: create scmi devices for
> protocols that not have of_node
> 
> Per
> https://lore.kernel.org/all/20230125141113.kkbowopusikuogx6@bog
> us/
> "
> In short we shouldn't have to add a node if there are no consumers. It
> was one of the topic of discussion initially when SCMI binding was
> added and they exist only for the consumers otherwise we don't need
> it as everything is discoverable from the interface.
> "
> https://lore.kernel.org/all/Y9JLUIioxFPn4BS0@e120937-lin/
> If a node has its own channel, the of_node is still needed.
> 
> i.MX95 SCMI firmware not have dedicated channel for 0x12, and no
> need of_node. This patchset is to support protocol 0x12 without the
> procotol node in device tree.
> 
> Without of_node, still need to create the scmi devices. As of now, it is
> based on an array 'protocols[]' in 'scmi_probe'.
> 
> And no of_node, means no per protocol channel, so reuse the base
> protocol channel. Need patch chan_available to support.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
> Peng Fan (2):
>       firmware: arm_scmi: channel unavailable if no of_node
>       firmware: arm_scmi: create scmi_devices that not have of_node
> 
>  drivers/firmware/arm_scmi/driver.c  | 33
> ++++++++++++++++++++++++++++++++-
> drivers/firmware/arm_scmi/mailbox.c |  2 ++
>  drivers/firmware/arm_scmi/optee.c   |  3 +++
>  drivers/firmware/arm_scmi/smc.c     |  7 ++++++-
>  drivers/firmware/arm_scmi/virtio.c  |  3 +++
>  5 files changed, 46 insertions(+), 2 deletions(-)
> ---
> base-commit: d8003eb2eb0200352b5d63af77ec0912a52e79ad
> change-id: 20240626-scmi-driver-96dc61b036a2
> 
> Best regards,
> --
> Peng Fan <peng.fan@nxp.com>

Re: [PATCH 0/2] firmware: arm_scmi: create scmi devices for protocols that not have of_node

[Cristian Marussi]

On Wed, Jun 26, 2024 at 02:58:38PM +0800, Peng Fan (OSS) wrote:
> Per

Hi,

> https://lore.kernel.org/all/20230125141113.kkbowopusikuogx6@bogus/
> "
> In short we shouldn't have to add a node if there are no consumers. It
> was one of the topic of discussion initially when SCMI binding was added
> and they exist only for the consumers otherwise we don't need it as
> everything is discoverable from the interface.
> "
> https://lore.kernel.org/all/Y9JLUIioxFPn4BS0@e120937-lin/
> If a node has its own channel, the of_node is still needed.
> 
> i.MX95 SCMI firmware not have dedicated channel for 0x12, and no need
> of_node. This patchset is to support protocol 0x12 without the procotol
> node in device tree.
> 

With this patch you change a bit of the core logic to allow for
protocols not explicitly described in the DT to be instantiated, and you
use a static builtin array to list such protocols...so any future change
or any downstream vendor protocols that want to use this, we will have to
patch and extend such protocols[] array.

Moreover, if anyone DO want to use a per-protocol channel in the future
on some of these protocols, it will work fine with your solution on the code
side, BUT you will still have anyway a DT binding check error when you
try to add that 0x12 node to contain a channel description, right ?
... because in that case you will have re-added a (supposedly) empty
protocol node in order to containn the channels definitions and that wont
be yaml-compliant, am I right ?

IOW this solves your issue in the immediate, while adding complexity to
the core code and changing the core behaviour around protocols, but it
wont stand any future addition or different usage.

For these reasons, I still think that the cleanest solution is to just let
protocol nodes to exist even if not referenced anywhere from the DT (your
original patch to add protocol0x12 I think) simply because we allow
per-protocol channel definitions and so any empty unreferenced protocol
node could be needed in the future for this reason.

In this way we'll also keep treating protocols in an uniform way.

Just my opinion, though, I'll settle with what is finally decided
anyway.

Thank
Cristian

RE: [PATCH 0/2] firmware: arm_scmi: create scmi devices for protocols that not have of_node

[Peng Fan]

Hi Cristian,

> Subject: Re: [PATCH 0/2] firmware: arm_scmi: create scmi devices for
> protocols that not have of_node
> 
> On Wed, Jun 26, 2024 at 02:58:38PM +0800, Peng Fan (OSS) wrote:
> > Per
> 
> Hi,
> 
> >

...
> >
> > i.MX95 SCMI firmware not have dedicated channel for 0x12, and no
> need
> > of_node. This patchset is to support protocol 0x12 without the
> > procotol node in device tree.
> >
> 
> With this patch you change a bit of the core logic to allow for protocols
> not explicitly described in the DT to be instantiated, and you use a
> static builtin array to list such protocols...so any future change or any
> downstream vendor protocols that want to use this, we will have to
> patch and extend such protocols[] array.
> 
> Moreover, if anyone DO want to use a per-protocol channel in the
> future on some of these protocols, it will work fine with your solution
> on the code side, BUT you will still have anyway a DT binding check
> error when you try to add that 0x12 node to contain a channel
> description, right ?

Right.

> ... because in that case you will have re-added a (supposedly) empty
> protocol node in order to containn the channels definitions and that
> wont be yaml-compliant, am I right ?
> 
> IOW this solves your issue in the immediate, while adding complexity
> to the core code and changing the core behaviour around protocols,
> but it wont stand any future addition or different usage.
> 
> For these reasons, I still think that the cleanest solution is to just let
> protocol nodes to exist even if not referenced anywhere from the DT
> (your original patch to add protocol0x12 I think) simply because we
> allow per-protocol channel definitions and so any empty unreferenced
> protocol node could be needed in the future for this reason.

You mean this one [1], right?

I could rebase and send out it again.

> 
> In this way we'll also keep treating protocols in an uniform way.
> 
> Just my opinion, though, I'll settle with what is finally decided anyway.

From reading the previous discussion as listed in cover letter,
I thought there was an agreement that for non consumers, no per
protocol channel node, we should not add it in device tree.
But indeed binding is needed in case the channel has its own channel.

This patchset could be dropped if Sudeep and you both agree with [1]
 
[1]
https://lore.kernel.org/all/20240226130243.3820915-1-peng.fan@oss.nxp.com/

Thanks,
Peng.
> 
> Thank
> Cristian

Re: [PATCH 0/2] firmware: arm_scmi: create scmi devices for protocols that not have of_node

[Cristian Marussi]

On Wed, Jun 26, 2024 at 11:50:26AM +0000, Peng Fan wrote:
> Hi Cristian,
> 
> > Subject: Re: [PATCH 0/2] firmware: arm_scmi: create scmi devices for
> > protocols that not have of_node
> > 
> > On Wed, Jun 26, 2024 at 02:58:38PM +0800, Peng Fan (OSS) wrote:
> > > Per
> > 
> > Hi,
> > 
> > >
> 
> ...
> > >
> > > i.MX95 SCMI firmware not have dedicated channel for 0x12, and no
> > need
> > > of_node. This patchset is to support protocol 0x12 without the
> > > procotol node in device tree.
> > >
> > 
> > With this patch you change a bit of the core logic to allow for protocols
> > not explicitly described in the DT to be instantiated, and you use a
> > static builtin array to list such protocols...so any future change or any
> > downstream vendor protocols that want to use this, we will have to
> > patch and extend such protocols[] array.
> > 
> > Moreover, if anyone DO want to use a per-protocol channel in the
> > future on some of these protocols, it will work fine with your solution
> > on the code side, BUT you will still have anyway a DT binding check
> > error when you try to add that 0x12 node to contain a channel
> > description, right ?
> 
> Right.
> 
> > ... because in that case you will have re-added a (supposedly) empty
> > protocol node in order to containn the channels definitions and that
> > wont be yaml-compliant, am I right ?
> > 
> > IOW this solves your issue in the immediate, while adding complexity
> > to the core code and changing the core behaviour around protocols,
> > but it wont stand any future addition or different usage.
> > 
> > For these reasons, I still think that the cleanest solution is to just let
> > protocol nodes to exist even if not referenced anywhere from the DT
> > (your original patch to add protocol0x12 I think) simply because we
> > allow per-protocol channel definitions and so any empty unreferenced
> > protocol node could be needed in the future for this reason.
> 
> You mean this one [1], right?
> 
> I could rebase and send out it again.
> 
> > 
> > In this way we'll also keep treating protocols in an uniform way.
> > 
> > Just my opinion, though, I'll settle with what is finally decided anyway.
> 
> From reading the previous discussion as listed in cover letter,
> I thought there was an agreement that for non consumers, no per
> protocol channel node, we should not add it in device tree.
> But indeed binding is needed in case the channel has its own channel.
> 
> This patchset could be dropped if Sudeep and you both agree with [1]
>  

Yes indeed, not sure what at the end Sudeep thinks about this after
reading that thread again....that's why I specified that was just my opinion :P

Moreover, regarding this series, I wonder if, in general, allowing
protocol devices without an underlyng DT node could not be asking for
trouble in the future...in the sense that these devices are used by SCMI
drivers and can be used by them as they wish, including using them to
register with other subsytems, subsystems that can have assumnptions on
the fact the device has a valid underlying of_node... I maybe overthinking...

Anyway let's see if Sudeep prefers to go down this way I will post some
more comments on specifically how this series works..

Thanks,
Cristian

RE: [PATCH 0/2] firmware: arm_scmi: create scmi devices for protocols that not have of_node

[Peng Fan]

> Subject: Re: [PATCH 0/2] firmware: arm_scmi: create scmi devices for
> protocols that not have of_node
> 
> On Wed, Jun 26, 2024 at 02:58:38PM +0800, Peng Fan (OSS) wrote:
> > Per
> 
> Hi,
> 

...
> > rved=0 If a node has its own channel, the of_node is still needed.
> >
> > i.MX95 SCMI firmware not have dedicated channel for 0x12, and no
> need
> > of_node. This patchset is to support protocol 0x12 without the
> > procotol node in device tree.
> >
> 
> With this patch you change a bit of the core logic to allow for protocols
> not explicitly described in the DT to be instantiated, and you use a
> static builtin array to list such protocols...so any future change or any
> downstream vendor protocols that want to use this, we will have to
> patch and extend such protocols[] array.

Just recheck this again, we might address with iterate
rdev->id_table->protocol_id, just as scmi_device_create.

Regards,
Peng.

> 
> Moreover, if anyone DO want to use a per-protocol channel in the
> future on some of these protocols, it will work fine with your solution
> on the code side, BUT you will still have anyway a DT binding check
> error when you try to add that 0x12 node to contain a channel
> description, right ?
> ... because in that case you will have re-added a (supposedly) empty
> protocol node in order to containn the channels definitions and that
> wont be yaml-compliant, am I right ?
> 
> IOW this solves your issue in the immediate, while adding complexity
> to the core code and changing the core behaviour around protocols,
> but it wont stand any future addition or different usage.
> 
> For these reasons, I still think that the cleanest solution is to just let
> protocol nodes to exist even if not referenced anywhere from the DT
> (your original patch to add protocol0x12 I think) simply because we
> allow per-protocol channel definitions and so any empty unreferenced
> protocol node could be needed in the future for this reason.
> 
> In this way we'll also keep treating protocols in an uniform way.
> 
> Just my opinion, though, I'll settle with what is finally decided anyway.
> 
> Thank
> Cristian