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.1On 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
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
>
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
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
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
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
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
© 2016 - 2026 Red Hat, Inc.