Question regarding scmi_perf_domain.c

Question regarding scmi_perf_domain.c

[Peng Fan]

Hi Ulf,

I just see you wrote scmi_perf_domain.c, just wonder this driver is only
for devices, not support arm cores, right?

For ARM cores, we still need scmi_cpufreq.c for performance settings,
right?

Thanks,
Peng.

Re: Question regarding scmi_perf_domain.c

[Ulf Hansson]

On Tue, 10 Oct 2023 at 12:30, Peng Fan <peng.fan@nxp.com> wrote:
>
> Hi Ulf,
>
> I just see you wrote scmi_perf_domain.c, just wonder this driver is only
> for devices, not support arm cores, right?
>
> For ARM cores, we still need scmi_cpufreq.c for performance settings,
> right?

Yes, correct!

Although we may want to switch the scmi_cpufreq to use the SCMI
performance domain too. I plan to have a closer look at this, to see
if it really makes sense or not.

Kind regards
Uffe

Re: Question regarding scmi_perf_domain.c

[Sudeep Holla]

On Tue, Oct 10, 2023 at 10:30:17AM +0000, Peng Fan wrote:
> Hi Ulf,
> 
> I just see you wrote scmi_perf_domain.c, just wonder this driver is only
> for devices, not support arm cores, right?
> 
> For ARM cores, we still need scmi_cpufreq.c for performance settings,
> right?

Sorry if I wasn't clear. The reason I mentioned it in private is that
we now support the power domain bindings in the scmi-cpufreq.c as you
were little bit nervous to use the clock bindings(though they work just
fine, I understand the possible confusion with the clock protocol).

There is also separate SCMI performance genpd driver that works for
non-CPU devices. As Ulf mentioned, scmi-cpufreq is not dependent on
those genpd performance driver ATM and we still need to check if it
has to be yet. Hope that clarifies.

-- 
Regards,
Sudeep

Re: Question regarding scmi_perf_domain.c

[Ulf Hansson]

On Tue, 10 Oct 2023 at 12:55, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Tue, Oct 10, 2023 at 10:30:17AM +0000, Peng Fan wrote:
> > Hi Ulf,
> >
> > I just see you wrote scmi_perf_domain.c, just wonder this driver is only
> > for devices, not support arm cores, right?
> >
> > For ARM cores, we still need scmi_cpufreq.c for performance settings,
> > right?
>
> Sorry if I wasn't clear. The reason I mentioned it in private is that
> we now support the power domain bindings in the scmi-cpufreq.c as you
> were little bit nervous to use the clock bindings(though they work just
> fine, I understand the possible confusion with the clock protocol).

Right, good point!

I think we discussed earlier whether we should deprecate the use of
the clock bindings. Maybe that's a good idea, to indicate that we
prefer the power-domain bindings when going forward?

[...]

Kind regards
Uffe

RE: Question regarding scmi_perf_domain.c

[Peng Fan]

Hi Sudeep, Ulf
> Subject: Re: Question regarding scmi_perf_domain.c
> 
> On Tue, 10 Oct 2023 at 12:55, Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > On Tue, Oct 10, 2023 at 10:30:17AM +0000, Peng Fan wrote:
> > > Hi Ulf,
> > >
> > > I just see you wrote scmi_perf_domain.c, just wonder this driver is
> > > only for devices, not support arm cores, right?
> > >
> > > For ARM cores, we still need scmi_cpufreq.c for performance
> > > settings, right?
> >
> > Sorry if I wasn't clear. The reason I mentioned it in private is that
> > we now support the power domain bindings in the scmi-cpufreq.c as you
> > were little bit nervous to use the clock bindings(though they work
> > just fine, I understand the possible confusion with the clock protocol).
> 
> Right, good point!
> 
> I think we discussed earlier whether we should deprecate the use of the clock
> bindings. Maybe that's a good idea, to indicate that we prefer the power-
> domain bindings when going forward?

But why use power-domains? Power domains may not same as perf domains
per my understanding and SCMI spec not has such restriction.

Currently our SCMI server power domain ids and perf domain ids are
different.  If linux has the restriction that perf domain id should be same
as power domain id, we need redesign our scmi firmware on this part.

Thanks,
Peng.

> 
> [...]
> 
> Kind regards
> Uffe

Re: Question regarding scmi_perf_domain.c

[Sudeep Holla]

On Tue, Oct 10, 2023 at 12:01:57PM +0000, Peng Fan wrote:
> Hi Sudeep, Ulf
> > Subject: Re: Question regarding scmi_perf_domain.c
> >
> > On Tue, 10 Oct 2023 at 12:55, Sudeep Holla <sudeep.holla@arm.com> wrote:
> > >
> > > On Tue, Oct 10, 2023 at 10:30:17AM +0000, Peng Fan wrote:
> > > > Hi Ulf,
> > > >
> > > > I just see you wrote scmi_perf_domain.c, just wonder this driver is
> > > > only for devices, not support arm cores, right?
> > > >
> > > > For ARM cores, we still need scmi_cpufreq.c for performance
> > > > settings, right?
> > >
> > > Sorry if I wasn't clear. The reason I mentioned it in private is that
> > > we now support the power domain bindings in the scmi-cpufreq.c as you
> > > were little bit nervous to use the clock bindings(though they work
> > > just fine, I understand the possible confusion with the clock protocol).
> >
> > Right, good point!
> >
> > I think we discussed earlier whether we should deprecate the use of the clock
> > bindings. Maybe that's a good idea, to indicate that we prefer the power-
> > domain bindings when going forward?
>
> But why use power-domains? Power domains may not same as perf domains
> per my understanding and SCMI spec not has such restriction.
>

Good question as it can be as confusing as using clocks bindings. I
understand, but Linux genpd domains were extended to support performance
domains and Ulf has worked on to support the same for SCMI.

One key point you have to note and understand is that on SCMI based
platforms, you will end up with one set of SCMI genpd domains that provide
only power domain capability using the SCMI power protocol and another
set of genpd domains providing just the performance capability using the
SCMI perf protocol.

The set of power and perf domains may not overlap at all based on how it
is presented by the firmware. To be clear(to answer to your main confusion
and avoid any further), each will have the set of its own domain IDs
(0 - (N - 1)) and (0 - (M - 1)) where M and N represents the number of
perf and power domains supported by the firmware.

> Currently our SCMI server power domain ids and perf domain ids are
> different.  If linux has the restriction that perf domain id should be same
> as power domain id, we need redesign our scmi firmware on this part.
>

No, there is no such restriction. It is just the exact/similar confusion
you had with clock IDs being used with performance protocol. It is just
your misunderstanding, not the reality.

--
Regards,
Sudeep

RE: Question regarding scmi_perf_domain.c

[Peng Fan]

> Subject: Re: Question regarding scmi_perf_domain.c
> 
> On Tue, Oct 10, 2023 at 12:01:57PM +0000, Peng Fan wrote:
> > Hi Sudeep, Ulf
> > > Subject: Re: Question regarding scmi_perf_domain.c
> > >
> > > On Tue, 10 Oct 2023 at 12:55, Sudeep Holla <sudeep.holla@arm.com>
> wrote:
> > > >
> > > > On Tue, Oct 10, 2023 at 10:30:17AM +0000, Peng Fan wrote:
> > > > > Hi Ulf,
> > > > >
> > > > > I just see you wrote scmi_perf_domain.c, just wonder this driver
> > > > > is only for devices, not support arm cores, right?
> > > > >
> > > > > For ARM cores, we still need scmi_cpufreq.c for performance
> > > > > settings, right?
> > > >
> > > > Sorry if I wasn't clear. The reason I mentioned it in private is
> > > > that we now support the power domain bindings in the
> > > > scmi-cpufreq.c as you were little bit nervous to use the clock
> > > > bindings(though they work just fine, I understand the possible confusion
> with the clock protocol).
> > >
> > > Right, good point!
> > >
> > > I think we discussed earlier whether we should deprecate the use of
> > > the clock bindings. Maybe that's a good idea, to indicate that we
> > > prefer the power- domain bindings when going forward?
> >
> > But why use power-domains? Power domains may not same as perf
> domains
> > per my understanding and SCMI spec not has such restriction.
> >
> 
> Good question as it can be as confusing as using clocks bindings. I understand,
> but Linux genpd domains were extended to support performance domains
> and Ulf has worked on to support the same for SCMI.
> 
> One key point you have to note and understand is that on SCMI based
> platforms, you will end up with one set of SCMI genpd domains that provide
> only power domain capability using the SCMI power protocol and another set
> of genpd domains providing just the performance capability using the SCMI
> perf protocol.
> 
> The set of power and perf domains may not overlap at all based on how it is
> presented by the firmware. To be clear(to answer to your main confusion and
> avoid any further), each will have the set of its own domain IDs
> (0 - (N - 1)) and (0 - (M - 1)) where M and N represents the number of perf and
> power domains supported by the firmware.
> 
> > Currently our SCMI server power domain ids and perf domain ids are
> > different.  If linux has the restriction that perf domain id should be
> > same as power domain id, we need redesign our scmi firmware on this part.
> >
> 
> No, there is no such restriction. It is just the exact/similar confusion you had
> with clock IDs being used with performance protocol. It is just your
> misunderstanding, not the reality.

Thanks for the detailed explanation, so power-domains property could be
used both for power domain or performance domain. But if one
device has both power domain and performance domain. Only power-domain
property is not enough. I may understand wrong, let me look into the code.

Thanks,
Peng.
> 
> --
> Regards,
> Sudeep

Re: Question regarding scmi_perf_domain.c

[Sudeep Holla]

On Tue, Oct 10, 2023 at 01:15:26PM +0000, Peng Fan wrote:
> 
> Thanks for the detailed explanation, so power-domains property could be
> used both for power domain or performance domain. But if one
> device has both power domain and performance domain. Only power-domain
> property is not enough. I may understand wrong, let me look into the code.
> 

I haven't tried this but something I could come up quick wit Juno DTS
as reference:

We can change something like this:

      scmi_dvfs: protocol@13 {
              reg = <0x13>;
-             #clock-cells = <1>;
+             #power-domain-cells = <1>;
              mbox-names = "tx", "rx";
              mboxes = <&mailbox 1 0 &mailbox 1 1>;
              shmem = <&cpu_scp_hpri0 &cpu_scp_hpri1>;
      };

And then in the consumer node(taking GPU as it has both perf and power
domains). The CPUs are simpler as don't have explicit power domains, some
Qcom platforms do use that. Anyways I would change GPU node like this.
Hope this clarifies things for you.

 &gpu {
-       clocks = <&scmi_dvfs 2>;
-       power-domains = <&scmi_devpd 9>;
+       power-domains = <&scmi_dvfs 2 &scmi_devpd 9>;
+       power-domain-names = "perf", "power";
        status = "disabled";
 };

-- 
Regards,
Sudeep

RE: Question regarding scmi_perf_domain.c

[Peng Fan]

> Subject: Re: Question regarding scmi_perf_domain.c
> 
> On Tue, Oct 10, 2023 at 01:15:26PM +0000, Peng Fan wrote:
> >
> > Thanks for the detailed explanation, so power-domains property could
> > be used both for power domain or performance domain. But if one device
> > has both power domain and performance domain. Only power-domain
> > property is not enough. I may understand wrong, let me look into the code.
> >
> 
> I haven't tried this but something I could come up quick wit Juno DTS as
> reference:
> 
> We can change something like this:
> 
>       scmi_dvfs: protocol@13 {
>               reg = <0x13>;
> -             #clock-cells = <1>;
> +             #power-domain-cells = <1>;
>               mbox-names = "tx", "rx";
>               mboxes = <&mailbox 1 0 &mailbox 1 1>;
>               shmem = <&cpu_scp_hpri0 &cpu_scp_hpri1>;
>       };
> 
> And then in the consumer node(taking GPU as it has both perf and power
> domains). The CPUs are simpler as don't have explicit power domains, some
> Qcom platforms do use that. Anyways I would change GPU node like this.
> Hope this clarifies things for you.
> 
>  &gpu {
> -       clocks = <&scmi_dvfs 2>;
> -       power-domains = <&scmi_devpd 9>;
> +       power-domains = <&scmi_dvfs 2 &scmi_devpd 9>;
> +       power-domain-names = "perf", "power";

With one single power domain, the platform common code
will automatically power on the domain before probe, with
help from genpd_dev_pm_attach.

But with multiple entries, device driver should handle power
domains by themselves.

Maybe Ulf could comment whether the genpd could update
to support perf/power case just as one power domain
entry before.

Thanks
Peng.
>         status = "disabled";
>  };
> 
> --
> Regards,
> Sudeep

Re: Question regarding scmi_perf_domain.c

[Sudeep Holla]

On Tue, Oct 10, 2023 at 01:43:32PM +0000, Peng Fan wrote:
> > Subject: Re: Question regarding scmi_perf_domain.c
> >
> > On Tue, Oct 10, 2023 at 01:15:26PM +0000, Peng Fan wrote:
> > >
> > > Thanks for the detailed explanation, so power-domains property could
> > > be used both for power domain or performance domain. But if one device
> > > has both power domain and performance domain. Only power-domain
> > > property is not enough. I may understand wrong, let me look into the code.
> > >
> >
> > I haven't tried this but something I could come up quick wit Juno DTS as
> > reference:
> >
> > We can change something like this:
> >
> >       scmi_dvfs: protocol@13 {
> >               reg = <0x13>;
> > -             #clock-cells = <1>;
> > +             #power-domain-cells = <1>;
> >               mbox-names = "tx", "rx";
> >               mboxes = <&mailbox 1 0 &mailbox 1 1>;
> >               shmem = <&cpu_scp_hpri0 &cpu_scp_hpri1>;
> >       };
> >
> > And then in the consumer node(taking GPU as it has both perf and power
> > domains). The CPUs are simpler as don't have explicit power domains, some
> > Qcom platforms do use that. Anyways I would change GPU node like this.
> > Hope this clarifies things for you.
> >
> >  &gpu {
> > -       clocks = <&scmi_dvfs 2>;
> > -       power-domains = <&scmi_devpd 9>;
> > +       power-domains = <&scmi_dvfs 2 &scmi_devpd 9>;
> > +       power-domain-names = "perf", "power";
>
> With one single power domain, the platform common code
> will automatically power on the domain before probe, with
> help from genpd_dev_pm_attach.
>
> But with multiple entries, device driver should handle power
> domains by themselves.
>
> Maybe Ulf could comment whether the genpd could update
> to support perf/power case just as one power domain
> entry before.
>

Hmm, I would rather check if the genpd can still handle automatic
power on of the domain before probe with one power and one perf domain.
IWO, one power domains and other domains in the mix. The reason why we
can't have single domain to support both power and perf using SCMI is
we don't know if the domains are 1:1 as presented by the SCMI platform
firmware.

AFAIU it was the main issue/confusion you raised initially. I am
surprised as how we had all these discussions and now you are circling
back and requesting to combine the support in single domain which
contradicts your initial confusion. I am seriously lost as what you are
looking for now ?

--
Regards,
Sudeep

Re: Question regarding scmi_perf_domain.c

[Ulf Hansson]

+ Nikunj

On Tue, 10 Oct 2023 at 16:51, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Tue, Oct 10, 2023 at 01:43:32PM +0000, Peng Fan wrote:
> > > Subject: Re: Question regarding scmi_perf_domain.c
> > >
> > > On Tue, Oct 10, 2023 at 01:15:26PM +0000, Peng Fan wrote:
> > > >
> > > > Thanks for the detailed explanation, so power-domains property could
> > > > be used both for power domain or performance domain. But if one device
> > > > has both power domain and performance domain. Only power-domain
> > > > property is not enough. I may understand wrong, let me look into the code.
> > > >
> > >
> > > I haven't tried this but something I could come up quick wit Juno DTS as
> > > reference:
> > >
> > > We can change something like this:
> > >
> > >       scmi_dvfs: protocol@13 {
> > >               reg = <0x13>;
> > > -             #clock-cells = <1>;
> > > +             #power-domain-cells = <1>;
> > >               mbox-names = "tx", "rx";
> > >               mboxes = <&mailbox 1 0 &mailbox 1 1>;
> > >               shmem = <&cpu_scp_hpri0 &cpu_scp_hpri1>;
> > >       };
> > >
> > > And then in the consumer node(taking GPU as it has both perf and power
> > > domains). The CPUs are simpler as don't have explicit power domains, some
> > > Qcom platforms do use that. Anyways I would change GPU node like this.
> > > Hope this clarifies things for you.
> > >
> > >  &gpu {
> > > -       clocks = <&scmi_dvfs 2>;
> > > -       power-domains = <&scmi_devpd 9>;
> > > +       power-domains = <&scmi_dvfs 2 &scmi_devpd 9>;
> > > +       power-domain-names = "perf", "power";
> >
> > With one single power domain, the platform common code
> > will automatically power on the domain before probe, with
> > help from genpd_dev_pm_attach.
> >
> > But with multiple entries, device driver should handle power
> > domains by themselves.
> >
> > Maybe Ulf could comment whether the genpd could update
> > to support perf/power case just as one power domain
> > entry before.
> >
>
> Hmm, I would rather check if the genpd can still handle automatic
> power on of the domain before probe with one power and one perf domain.
> IWO, one power domains and other domains in the mix. The reason why we
> can't have single domain to support both power and perf using SCMI is
> we don't know if the domains are 1:1 as presented by the SCMI platform
> firmware.

Well explained above, thanks!

So in the case where a platform will support both, it means that a
driver for a consumer device, actually needs to attach to two
different PM domains (one for perf and one for power).

It's easy to imagine that we are moving towards getting a lot of
boilerplate code to manage these things in drivers. Therefore I think
we need to consider adding some helper functions to manage attaching
of multiple PM domains. This has been discussed earlier too, but I
will try to pick it up and move it forward as soon as I can.

Another option could be to discuss how we can extend the SCMI spec
and/or the DT bindings, to optionally allow us to use a 1:1 mapping of
an SCMI perf/power domain.

Kind regards
Uffe

Re: Question regarding scmi_perf_domain.c

[Sudeep Holla]

On Tue, Oct 10, 2023 at 05:23:41PM +0200, Ulf Hansson wrote:

[...]

>
> Another option could be to discuss how we can extend the SCMI spec
> and/or the DT bindings, to optionally allow us to use a 1:1 mapping of
> an SCMI perf/power domain.
>

Well I leave that to the spec author and relevant discussion there as
what are the merits in doing so.

In absence of such a support in the spec, we need to support what we have
even after we get that in the spec to support all the platforms without
it. So, IMO it is better to look at that angle as well.

--
Regards,
Sudeep

Re: Question regarding scmi_perf_domain.c

[Ulf Hansson]

+ Abel

On Tue, 10 Oct 2023 at 18:23, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Tue, Oct 10, 2023 at 05:23:41PM +0200, Ulf Hansson wrote:
>
> [...]
>
> >
> > Another option could be to discuss how we can extend the SCMI spec
> > and/or the DT bindings, to optionally allow us to use a 1:1 mapping of
> > an SCMI perf/power domain.
> >
>
> Well I leave that to the spec author and relevant discussion there as
> what are the merits in doing so.
>
> In absence of such a support in the spec, we need to support what we have
> even after we get that in the spec to support all the platforms without
> it. So, IMO it is better to look at that angle as well.

Ack!

Still, I don't think it would be a bad idea to have a chat about it.
Probably best suited at some f2f call. In particular as it could
potentially also enable additional optimizations [1].

Kind regards
Uffe

[1]
commit ae8ac19655e0 ("PM: domains: Reverse the order of performance
and enabling ops")

RE: Question regarding scmi_perf_domain.c

[Peng Fan]

> Subject: Re: Question regarding scmi_perf_domain.c
> 
> On Tue, Oct 10, 2023 at 01:43:32PM +0000, Peng Fan wrote:
> > > Subject: Re: Question regarding scmi_perf_domain.c
> > >
> > > On Tue, Oct 10, 2023 at 01:15:26PM +0000, Peng Fan wrote:
> > > >
> > > > Thanks for the detailed explanation, so power-domains property
> > > > could be used both for power domain or performance domain. But if
> > > > one device has both power domain and performance domain. Only
> > > > power-domain property is not enough. I may understand wrong, let me
> look into the code.
> > > >
> > >
> > > I haven't tried this but something I could come up quick wit Juno
> > > DTS as
> > > reference:
> > >
> > > We can change something like this:
> > >
> > >       scmi_dvfs: protocol@13 {
> > >               reg = <0x13>;
> > > -             #clock-cells = <1>;
> > > +             #power-domain-cells = <1>;
> > >               mbox-names = "tx", "rx";
> > >               mboxes = <&mailbox 1 0 &mailbox 1 1>;
> > >               shmem = <&cpu_scp_hpri0 &cpu_scp_hpri1>;
> > >       };
> > >
> > > And then in the consumer node(taking GPU as it has both perf and
> > > power domains). The CPUs are simpler as don't have explicit power
> > > domains, some Qcom platforms do use that. Anyways I would change
> GPU node like this.
> > > Hope this clarifies things for you.
> > >
> > >  &gpu {
> > > -       clocks = <&scmi_dvfs 2>;
> > > -       power-domains = <&scmi_devpd 9>;
> > > +       power-domains = <&scmi_dvfs 2 &scmi_devpd 9>;
> > > +       power-domain-names = "perf", "power";
> >
> > With one single power domain, the platform common code will
> > automatically power on the domain before probe, with help from
> > genpd_dev_pm_attach.
> >
> > But with multiple entries, device driver should handle power domains
> > by themselves.
> >
> > Maybe Ulf could comment whether the genpd could update to support
> > perf/power case just as one power domain entry before.
> >
> 
> Hmm, I would rather check if the genpd can still handle automatic
> power on of the domain before probe with one power and one perf domain.
> IWO, one power domains and other domains in the mix. The reason why we
> can't have single domain to support both power and perf using SCMI is
> we don't know if the domains are 1:1 as presented by the SCMI platform
> firmware.
> 
> AFAIU it was the main issue/confusion you raised initially. I am
> surprised as how we had all these discussions and now you are circling
> back and requesting to combine the support in single domain which
> contradicts your initial confusion. I am seriously lost as what you are
> looking for now ?

No, I am not requesting to combine in single domain. I still wanna perf
domain and power domain has their own IDs. But I was not aware
perf domain is using power-domains property, so one device has
power domains and perf domains both, the automatic power domain
on is broken. I was thinking we introduce a new property saying
perf-domains property.

Regards,
Peng.

> 
> --
> Regards,
> Sudeep

Re: Question regarding scmi_perf_domain.c

[Sudeep Holla]

On Wed, Oct 11, 2023 at 12:30:35AM +0000, Peng Fan wrote:
> > Subject: Re: Question regarding scmi_perf_domain.c
> > 
> > On Tue, Oct 10, 2023 at 01:43:32PM +0000, Peng Fan wrote:
> > > > Subject: Re: Question regarding scmi_perf_domain.c
> > > >
> > > > On Tue, Oct 10, 2023 at 01:15:26PM +0000, Peng Fan wrote:
> > > > >
> > > > > Thanks for the detailed explanation, so power-domains property
> > > > > could be used both for power domain or performance domain. But if
> > > > > one device has both power domain and performance domain. Only
> > > > > power-domain property is not enough. I may understand wrong, let me
> > look into the code.
> > > > >
> > > >
> > > > I haven't tried this but something I could come up quick wit Juno
> > > > DTS as
> > > > reference:
> > > >
> > > > We can change something like this:
> > > >
> > > >       scmi_dvfs: protocol@13 {
> > > >               reg = <0x13>;
> > > > -             #clock-cells = <1>;
> > > > +             #power-domain-cells = <1>;
> > > >               mbox-names = "tx", "rx";
> > > >               mboxes = <&mailbox 1 0 &mailbox 1 1>;
> > > >               shmem = <&cpu_scp_hpri0 &cpu_scp_hpri1>;
> > > >       };
> > > >
> > > > And then in the consumer node(taking GPU as it has both perf and
> > > > power domains). The CPUs are simpler as don't have explicit power
> > > > domains, some Qcom platforms do use that. Anyways I would change
> > GPU node like this.
> > > > Hope this clarifies things for you.
> > > >
> > > >  &gpu {
> > > > -       clocks = <&scmi_dvfs 2>;
> > > > -       power-domains = <&scmi_devpd 9>;
> > > > +       power-domains = <&scmi_dvfs 2 &scmi_devpd 9>;
> > > > +       power-domain-names = "perf", "power";
> > >
> > > With one single power domain, the platform common code will
> > > automatically power on the domain before probe, with help from
> > > genpd_dev_pm_attach.
> > >
> > > But with multiple entries, device driver should handle power domains
> > > by themselves.
> > >
> > > Maybe Ulf could comment whether the genpd could update to support
> > > perf/power case just as one power domain entry before.
> > >
> > 
> > Hmm, I would rather check if the genpd can still handle automatic
> > power on of the domain before probe with one power and one perf domain.
> > IWO, one power domains and other domains in the mix. The reason why we
> > can't have single domain to support both power and perf using SCMI is
> > we don't know if the domains are 1:1 as presented by the SCMI platform
> > firmware.
> > 
> > AFAIU it was the main issue/confusion you raised initially. I am
> > surprised as how we had all these discussions and now you are circling
> > back and requesting to combine the support in single domain which
> > contradicts your initial confusion. I am seriously lost as what you are
> > looking for now ?
> 
> No, I am not requesting to combine in single domain. I still wanna perf
> domain and power domain has their own IDs. But I was not aware
> perf domain is using power-domains property, so one device has
> power domains and perf domains both, the automatic power domain
> on is broken. I was thinking we introduce a new property saying
> perf-domains property.
> 

IIUC, this should not need any extra information from the DT than what we
already have. It is just implementation could improve to deal with your ask.

-- 
Regards,
Sudeep

Re: Question regarding scmi_perf_domain.c

[Ulf Hansson]

On Wed, 11 Oct 2023 at 02:30, Peng Fan <peng.fan@nxp.com> wrote:
>
> > Subject: Re: Question regarding scmi_perf_domain.c
> >
> > On Tue, Oct 10, 2023 at 01:43:32PM +0000, Peng Fan wrote:
> > > > Subject: Re: Question regarding scmi_perf_domain.c
> > > >
> > > > On Tue, Oct 10, 2023 at 01:15:26PM +0000, Peng Fan wrote:
> > > > >
> > > > > Thanks for the detailed explanation, so power-domains property
> > > > > could be used both for power domain or performance domain. But if
> > > > > one device has both power domain and performance domain. Only
> > > > > power-domain property is not enough. I may understand wrong, let me
> > look into the code.
> > > > >
> > > >
> > > > I haven't tried this but something I could come up quick wit Juno
> > > > DTS as
> > > > reference:
> > > >
> > > > We can change something like this:
> > > >
> > > >       scmi_dvfs: protocol@13 {
> > > >               reg = <0x13>;
> > > > -             #clock-cells = <1>;
> > > > +             #power-domain-cells = <1>;
> > > >               mbox-names = "tx", "rx";
> > > >               mboxes = <&mailbox 1 0 &mailbox 1 1>;
> > > >               shmem = <&cpu_scp_hpri0 &cpu_scp_hpri1>;
> > > >       };
> > > >
> > > > And then in the consumer node(taking GPU as it has both perf and
> > > > power domains). The CPUs are simpler as don't have explicit power
> > > > domains, some Qcom platforms do use that. Anyways I would change
> > GPU node like this.
> > > > Hope this clarifies things for you.
> > > >
> > > >  &gpu {
> > > > -       clocks = <&scmi_dvfs 2>;
> > > > -       power-domains = <&scmi_devpd 9>;
> > > > +       power-domains = <&scmi_dvfs 2 &scmi_devpd 9>;
> > > > +       power-domain-names = "perf", "power";
> > >
> > > With one single power domain, the platform common code will
> > > automatically power on the domain before probe, with help from
> > > genpd_dev_pm_attach.
> > >
> > > But with multiple entries, device driver should handle power domains
> > > by themselves.
> > >
> > > Maybe Ulf could comment whether the genpd could update to support
> > > perf/power case just as one power domain entry before.
> > >
> >
> > Hmm, I would rather check if the genpd can still handle automatic
> > power on of the domain before probe with one power and one perf domain.
> > IWO, one power domains and other domains in the mix. The reason why we
> > can't have single domain to support both power and perf using SCMI is
> > we don't know if the domains are 1:1 as presented by the SCMI platform
> > firmware.
> >
> > AFAIU it was the main issue/confusion you raised initially. I am
> > surprised as how we had all these discussions and now you are circling
> > back and requesting to combine the support in single domain which
> > contradicts your initial confusion. I am seriously lost as what you are
> > looking for now ?
>
> No, I am not requesting to combine in single domain. I still wanna perf
> domain and power domain has their own IDs. But I was not aware
> perf domain is using power-domains property, so one device has
> power domains and perf domains both, the automatic power domain
> on is broken. I was thinking we introduce a new property saying
> perf-domains property.

Not sure exactly what you are referring to when saying that "automatic
power domain on is broken". Genpd power-on the PM domain for a device
that gets attached to it, if the device has only a single PM domain.
This is the legacy behaviour.

When we added support for multiple PM domains per device, we decided
to *not* power-on the PM domain, if the device that gets attached has
multiple PM domains. This behaviour was chosen deliberately, to allow
consumer drivers to decide themselves instead. Is there a problem with
this you think?

Kind regards
Uffe

RE: Question regarding scmi_perf_domain.c

[Peng Fan]

> Subject: Re: Question regarding scmi_perf_domain.c
> 
> On Wed, 11 Oct 2023 at 02:30, Peng Fan <peng.fan@nxp.com> wrote:
> >
> > > Subject: Re: Question regarding scmi_perf_domain.c
> > >
> > > On Tue, Oct 10, 2023 at 01:43:32PM +0000, Peng Fan wrote:
> > > > > Subject: Re: Question regarding scmi_perf_domain.c
> > > > >
> > > > > On Tue, Oct 10, 2023 at 01:15:26PM +0000, Peng Fan wrote:
> > > > > >
> > > > > > Thanks for the detailed explanation, so power-domains property
> > > > > > could be used both for power domain or performance domain. But
> > > > > > if one device has both power domain and performance domain.
> > > > > > Only power-domain property is not enough. I may understand
> > > > > > wrong, let me
> > > look into the code.
> > > > > >
> > > > >
> > > > > I haven't tried this but something I could come up quick wit
> > > > > Juno DTS as
> > > > > reference:
> > > > >
> > > > > We can change something like this:
> > > > >
> > > > >       scmi_dvfs: protocol@13 {
> > > > >               reg = <0x13>;
> > > > > -             #clock-cells = <1>;
> > > > > +             #power-domain-cells = <1>;
> > > > >               mbox-names = "tx", "rx";
> > > > >               mboxes = <&mailbox 1 0 &mailbox 1 1>;
> > > > >               shmem = <&cpu_scp_hpri0 &cpu_scp_hpri1>;
> > > > >       };
> > > > >
> > > > > And then in the consumer node(taking GPU as it has both perf and
> > > > > power domains). The CPUs are simpler as don't have explicit
> > > > > power domains, some Qcom platforms do use that. Anyways I would
> > > > > change
> > > GPU node like this.
> > > > > Hope this clarifies things for you.
> > > > >
> > > > >  &gpu {
> > > > > -       clocks = <&scmi_dvfs 2>;
> > > > > -       power-domains = <&scmi_devpd 9>;
> > > > > +       power-domains = <&scmi_dvfs 2 &scmi_devpd 9>;
> > > > > +       power-domain-names = "perf", "power";
> > > >
> > > > With one single power domain, the platform common code will
> > > > automatically power on the domain before probe, with help from
> > > > genpd_dev_pm_attach.
> > > >
> > > > But with multiple entries, device driver should handle power
> > > > domains by themselves.
> > > >
> > > > Maybe Ulf could comment whether the genpd could update to support
> > > > perf/power case just as one power domain entry before.
> > > >
> > >
> > > Hmm, I would rather check if the genpd can still handle automatic
> > > power on of the domain before probe with one power and one perf
> domain.
> > > IWO, one power domains and other domains in the mix. The reason why
> > > we can't have single domain to support both power and perf using
> > > SCMI is we don't know if the domains are 1:1 as presented by the
> > > SCMI platform firmware.
> > >
> > > AFAIU it was the main issue/confusion you raised initially. I am
> > > surprised as how we had all these discussions and now you are
> > > circling back and requesting to combine the support in single domain
> > > which contradicts your initial confusion. I am seriously lost as
> > > what you are looking for now ?
> >
> > No, I am not requesting to combine in single domain. I still wanna
> > perf domain and power domain has their own IDs. But I was not aware
> > perf domain is using power-domains property, so one device has power
> > domains and perf domains both, the automatic power domain on is
> > broken. I was thinking we introduce a new property saying perf-domains
> > property.
> 
> Not sure exactly what you are referring to when saying that "automatic
> power domain on is broken". Genpd power-on the PM domain for a device
> that gets attached to it, if the device has only a single PM domain.
> This is the legacy behaviour.
> 
> When we added support for multiple PM domains per device, we decided to
> *not* power-on the PM domain, if the device that gets attached has multiple
> PM domains. This behaviour was chosen deliberately, to allow consumer
> drivers to decide themselves instead. Is there a problem with this you think?

No problem. Now perf domain taken as power-domain, then the node
has more than one power-domains, so we need update device driver for
this.  I was thinking device drivers only need to care about performance
domain,  and no need device driver to handle power domain just as "legacy
behaviour".

Thanks,
Peng.

> 
> Kind regards
> Uffe

Re: Question regarding scmi_perf_domain.c

[Sudeep Holla]

On Wed, Oct 11, 2023 at 11:26:54AM +0200, Ulf Hansson wrote:

[..]

> Not sure exactly what you are referring to when saying that "automatic
> power domain on is broken". Genpd power-on the PM domain for a device
> that gets attached to it, if the device has only a single PM domain.
> This is the legacy behaviour.
> 
> When we added support for multiple PM domains per device, we decided
> to *not* power-on the PM domain, if the device that gets attached has
> multiple PM domains. This behaviour was chosen deliberately, to allow
> consumer drivers to decide themselves instead. Is there a problem with
> this you think?
> 

Just my understanding. Since the second PM domain added now is for perf
and is not strictly power domain, Peng's concern is switching to this
binding will make the platform loose this automatic genpd power-on
feature.

-- 
Regards,
Sudeep

Re: Question regarding scmi_perf_domain.c

[Ulf Hansson]

On Wed, 11 Oct 2023 at 16:17, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Wed, Oct 11, 2023 at 11:26:54AM +0200, Ulf Hansson wrote:
>
> [..]
>
> > Not sure exactly what you are referring to when saying that "automatic
> > power domain on is broken". Genpd power-on the PM domain for a device
> > that gets attached to it, if the device has only a single PM domain.
> > This is the legacy behaviour.
> >
> > When we added support for multiple PM domains per device, we decided
> > to *not* power-on the PM domain, if the device that gets attached has
> > multiple PM domains. This behaviour was chosen deliberately, to allow
> > consumer drivers to decide themselves instead. Is there a problem with
> > this you think?
> >
>
> Just my understanding. Since the second PM domain added now is for perf
> and is not strictly power domain, Peng's concern is switching to this
> binding will make the platform loose this automatic genpd power-on
> feature.

Yes, correct, as they way things are today.

It all boils down to that attaching a device to multiple PM domains
can't really be done in a generic way, as it becomes device/platform
specific. Since this needs to be managed by the drivers/buses anyway,
they might as well get control of what PM domain they need to power-on
to probe their devices.

Kind regards
Uffe

Re: Question regarding scmi_perf_domain.c

[Ulf Hansson]

On Thu, 12 Oct 2023 at 13:53, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Wed, 11 Oct 2023 at 16:17, Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > On Wed, Oct 11, 2023 at 11:26:54AM +0200, Ulf Hansson wrote:
> >
> > [..]
> >
> > > Not sure exactly what you are referring to when saying that "automatic
> > > power domain on is broken". Genpd power-on the PM domain for a device
> > > that gets attached to it, if the device has only a single PM domain.
> > > This is the legacy behaviour.
> > >
> > > When we added support for multiple PM domains per device, we decided
> > > to *not* power-on the PM domain, if the device that gets attached has
> > > multiple PM domains. This behaviour was chosen deliberately, to allow
> > > consumer drivers to decide themselves instead. Is there a problem with
> > > this you think?
> > >
> >
> > Just my understanding. Since the second PM domain added now is for perf
> > and is not strictly power domain, Peng's concern is switching to this
> > binding will make the platform loose this automatic genpd power-on
> > feature.
>
> Yes, correct, as they way things are today.
>
> It all boils down to that attaching a device to multiple PM domains
> can't really be done in a generic way, as it becomes device/platform
> specific. Since this needs to be managed by the drivers/buses anyway,
> they might as well get control of what PM domain they need to power-on
> to probe their devices.

Due to the above, it might be a good idea to power-on the SCMI
*power-domains* during boot and leave them on to allow drivers to
continue to probe their devices?

Maybe a module parameter or Kconfig debug option could be used to control this?

In this way an updated DTS with that adds a performance domain to a
consumer device node (which already has a power-domain), should allow
the consumer driver to continue to probe successfully.

Peng, would this resolve your concern?

Kind regards
Uffe

Re: Question regarding scmi_perf_domain.c

[Sudeep Holla]

On Mon, Oct 16, 2023 at 05:08:18PM +0200, Ulf Hansson wrote:
> On Thu, 12 Oct 2023 at 13:53, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > On Wed, 11 Oct 2023 at 16:17, Sudeep Holla <sudeep.holla@arm.com> wrote:
> > >
> > > On Wed, Oct 11, 2023 at 11:26:54AM +0200, Ulf Hansson wrote:
> > >
> > > [..]
> > >
> > > > Not sure exactly what you are referring to when saying that "automatic
> > > > power domain on is broken". Genpd power-on the PM domain for a device
> > > > that gets attached to it, if the device has only a single PM domain.
> > > > This is the legacy behaviour.
> > > >
> > > > When we added support for multiple PM domains per device, we decided
> > > > to *not* power-on the PM domain, if the device that gets attached has
> > > > multiple PM domains. This behaviour was chosen deliberately, to allow
> > > > consumer drivers to decide themselves instead. Is there a problem with
> > > > this you think?
> > > >
> > >
> > > Just my understanding. Since the second PM domain added now is for perf
> > > and is not strictly power domain, Peng's concern is switching to this
> > > binding will make the platform loose this automatic genpd power-on
> > > feature.
> >
> > Yes, correct, as they way things are today.
> >
> > It all boils down to that attaching a device to multiple PM domains
> > can't really be done in a generic way, as it becomes device/platform
> > specific. Since this needs to be managed by the drivers/buses anyway,
> > they might as well get control of what PM domain they need to power-on
> > to probe their devices.
>
> Due to the above, it might be a good idea to power-on the SCMI
> *power-domains* during boot and leave them on to allow drivers to
> continue to probe their devices?
>

Such workarounds in my opinion are always inviting troubles as few platforms
make not like it that way.

> Maybe a module parameter or Kconfig debug option could be used to control this?
>

May be that works, but again I see this as working around. If the expectation
is the driver must manage the PM domain eventually, does it make sense to
start doing that now. You termed the single domain power on automatically
in the genpd as the "legacy support". Do you mean there is a plan to remove
it or make drivers not rely on it ?

My main worry is now we are spreading this work around every where. It was
in genpd but now you want in SCMI power domain driver. It just makes things
harder to remove if the eventual plan is to make drivers take care of the
power domains themselves.

> In this way an updated DTS with that adds a performance domain to a
> consumer device node (which already has a power-domain), should allow
> the consumer driver to continue to probe successfully.
>

This will work, but I find it hard as addition of some extra information
in the DTS is ending up losing some feature which was otherwise available.
If platforms relied on it, they may just stick with clock bindings silently
as it is easier that way. Even expecting a module param might be a big ask
if someone working on the platform isn't aware of all the details.
That is my main concern.


--
Regards,
Sudeep

Re: Question regarding scmi_perf_domain.c

[Ulf Hansson]

On Tue, 17 Oct 2023 at 11:04, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> On Mon, Oct 16, 2023 at 05:08:18PM +0200, Ulf Hansson wrote:
> > On Thu, 12 Oct 2023 at 13:53, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > >
> > > On Wed, 11 Oct 2023 at 16:17, Sudeep Holla <sudeep.holla@arm.com> wrote:
> > > >
> > > > On Wed, Oct 11, 2023 at 11:26:54AM +0200, Ulf Hansson wrote:
> > > >
> > > > [..]
> > > >
> > > > > Not sure exactly what you are referring to when saying that "automatic
> > > > > power domain on is broken". Genpd power-on the PM domain for a device
> > > > > that gets attached to it, if the device has only a single PM domain.
> > > > > This is the legacy behaviour.
> > > > >
> > > > > When we added support for multiple PM domains per device, we decided
> > > > > to *not* power-on the PM domain, if the device that gets attached has
> > > > > multiple PM domains. This behaviour was chosen deliberately, to allow
> > > > > consumer drivers to decide themselves instead. Is there a problem with
> > > > > this you think?
> > > > >
> > > >
> > > > Just my understanding. Since the second PM domain added now is for perf
> > > > and is not strictly power domain, Peng's concern is switching to this
> > > > binding will make the platform loose this automatic genpd power-on
> > > > feature.
> > >
> > > Yes, correct, as they way things are today.
> > >
> > > It all boils down to that attaching a device to multiple PM domains
> > > can't really be done in a generic way, as it becomes device/platform
> > > specific. Since this needs to be managed by the drivers/buses anyway,
> > > they might as well get control of what PM domain they need to power-on
> > > to probe their devices.
> >
> > Due to the above, it might be a good idea to power-on the SCMI
> > *power-domains* during boot and leave them on to allow drivers to
> > continue to probe their devices?
> >
>
> Such workarounds in my opinion are always inviting troubles as few platforms
> make not like it that way.
>
> > Maybe a module parameter or Kconfig debug option could be used to control this?
> >
>
> May be that works, but again I see this as working around. If the expectation
> is the driver must manage the PM domain eventually, does it make sense to
> start doing that now. You termed the single domain power on automatically
> in the genpd as the "legacy support". Do you mean there is a plan to remove
> it or make drivers not rely on it ?

No, we are not planning to change the legacy behaviour.

>
> My main worry is now we are spreading this work around every where. It was
> in genpd but now you want in SCMI power domain driver. It just makes things
> harder to remove if the eventual plan is to make drivers take care of the
> power domains themselves.

The drivers need to take care of this, no matter what. My proposal
isn't going to change that, please see more below.

>
> > In this way an updated DTS with that adds a performance domain to a
> > consumer device node (which already has a power-domain), should allow
> > the consumer driver to continue to probe successfully.
> >
>
> This will work, but I find it hard as addition of some extra information
> in the DTS is ending up losing some feature which was otherwise available.
> If platforms relied on it, they may just stick with clock bindings silently
> as it is easier that way. Even expecting a module param might be a big ask
> if someone working on the platform isn't aware of all the details.
> That is my main concern.

I am not quite sure I get your point. The clock bindings can't be used
for generic performance scaling, but it's limited to CPUs.

Are you worried that the "debug option" (whatever we may decide to
use) would get set and then evolve into becoming the default behavior
for the SCMI power-domain? If so, I certainly agree that it can be a
concern and an argument for not doing something like this!

In principle my suggestion was to avoid us from *upfront* having to
patch lots of drivers, before updating the DTSes. With the debug
option, the idea was that drivers could be extended, step by step, to
deal with multiple PM domains and OPPs - and when all things are
implemented, the debug option would be unset for the platform.

Although, maybe this isn't such a problem after all. I guess we need
to defer to Peng to understand if this is really a concern or not.

Kind regards
Uffe

Re: Question regarding scmi_perf_domain.c

[Sudeep Holla]

On Tue, Oct 17, 2023 at 12:46:16PM +0200, Ulf Hansson wrote:
> On Tue, 17 Oct 2023 at 11:04, Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > On Mon, Oct 16, 2023 at 05:08:18PM +0200, Ulf Hansson wrote:
> > > On Thu, 12 Oct 2023 at 13:53, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > >
> > > > On Wed, 11 Oct 2023 at 16:17, Sudeep Holla <sudeep.holla@arm.com> wrote:
> > > > >
> > > > > On Wed, Oct 11, 2023 at 11:26:54AM +0200, Ulf Hansson wrote:
> > > > >
> > > > > [..]
> > > > >
> > > > > > Not sure exactly what you are referring to when saying that "automatic
> > > > > > power domain on is broken". Genpd power-on the PM domain for a device
> > > > > > that gets attached to it, if the device has only a single PM domain.
> > > > > > This is the legacy behaviour.
> > > > > >
> > > > > > When we added support for multiple PM domains per device, we decided
> > > > > > to *not* power-on the PM domain, if the device that gets attached has
> > > > > > multiple PM domains. This behaviour was chosen deliberately, to allow
> > > > > > consumer drivers to decide themselves instead. Is there a problem with
> > > > > > this you think?
> > > > > >
> > > > >
> > > > > Just my understanding. Since the second PM domain added now is for perf
> > > > > and is not strictly power domain, Peng's concern is switching to this
> > > > > binding will make the platform loose this automatic genpd power-on
> > > > > feature.
> > > >
> > > > Yes, correct, as they way things are today.
> > > >
> > > > It all boils down to that attaching a device to multiple PM domains
> > > > can't really be done in a generic way, as it becomes device/platform
> > > > specific. Since this needs to be managed by the drivers/buses anyway,
> > > > they might as well get control of what PM domain they need to power-on
> > > > to probe their devices.
> > >
> > > Due to the above, it might be a good idea to power-on the SCMI
> > > *power-domains* during boot and leave them on to allow drivers to
> > > continue to probe their devices?
> > >
> >
> > Such workarounds in my opinion are always inviting troubles as few platforms
> > make not like it that way.
> >
> > > Maybe a module parameter or Kconfig debug option could be used to control this?
> > >
> >
> > May be that works, but again I see this as working around. If the expectation
> > is the driver must manage the PM domain eventually, does it make sense to
> > start doing that now. You termed the single domain power on automatically
> > in the genpd as the "legacy support". Do you mean there is a plan to remove
> > it or make drivers not rely on it ?
> 
> No, we are not planning to change the legacy behaviour.
>

Okay.

> >
> > My main worry is now we are spreading this work around every where. It was
> > in genpd but now you want in SCMI power domain driver. It just makes things
> > harder to remove if the eventual plan is to make drivers take care of the
> > power domains themselves.
> 
> The drivers need to take care of this, no matter what. My proposal
> isn't going to change that, please see more below.
>

Then why do we need to add any sort of such support which I call "workaround"
in the SCMI genpd driver.

> >
> > > In this way an updated DTS with that adds a performance domain to a
> > > consumer device node (which already has a power-domain), should allow
> > > the consumer driver to continue to probe successfully.
> > >
> >
> > This will work, but I find it hard as addition of some extra information
> > in the DTS is ending up losing some feature which was otherwise available.
> > If platforms relied on it, they may just stick with clock bindings silently
> > as it is easier that way. Even expecting a module param might be a big ask
> > if someone working on the platform isn't aware of all the details.
> > That is my main concern.
> 
> I am not quite sure I get your point. The clock bindings can't be used
> for generic performance scaling, but it's limited to CPUs.
>

Correct, sorry I was referring to just CPUs and I realise this may not
be any issue with CPUs, so please ignore this comment of mine.

> Are you worried that the "debug option" (whatever we may decide to
> use) would get set and then evolve into becoming the default behavior
> for the SCMI power-domain? If so, I certainly agree that it can be a
> concern and an argument for not doing something like this!
>

Yes exactly. I see what genpd added and you term as legacy behaviour,
lots of driver have already relied on it though they must deal within
the driver. I don't want to people to just start relying on this new
behaviour we add and then becomes difficult to change or remove.

> In principle my suggestion was to avoid us from *upfront* having to
> patch lots of drivers, before updating the DTSes. With the debug
> option, the idea was that drivers could be extended, step by step, to
> deal with multiple PM domains and OPPs - and when all things are
> implemented, the debug option would be unset for the platform.
>

I don't see any platform upstream with SCMI genpd ATM, so I would rather
fix what is needed in the drivers before enabling in them on these platforms.

> Although, maybe this isn't such a problem after all. I guess we need
> to defer to Peng to understand if this is really a concern or not.
> 

That would be ideal, but generally we are never close to the ideal world
😁.

-- 
Regards,
Sudeep

RE: Question regarding scmi_perf_domain.c

[Peng Fan]

> Subject: Re: Question regarding scmi_perf_domain.c
> 
> On Thu, 12 Oct 2023 at 13:53, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > On Wed, 11 Oct 2023 at 16:17, Sudeep Holla <sudeep.holla@arm.com>
> wrote:
> > >
> > > On Wed, Oct 11, 2023 at 11:26:54AM +0200, Ulf Hansson wrote:
> > >
> > > [..]
> > >
> > > > Not sure exactly what you are referring to when saying that
> > > > "automatic power domain on is broken". Genpd power-on the PM
> > > > domain for a device that gets attached to it, if the device has only a
> single PM domain.
> > > > This is the legacy behaviour.
> > > >
> > > > When we added support for multiple PM domains per device, we
> > > > decided to *not* power-on the PM domain, if the device that gets
> > > > attached has multiple PM domains. This behaviour was chosen
> > > > deliberately, to allow consumer drivers to decide themselves
> > > > instead. Is there a problem with this you think?
> > > >
> > >
> > > Just my understanding. Since the second PM domain added now is for
> > > perf and is not strictly power domain, Peng's concern is switching
> > > to this binding will make the platform loose this automatic genpd
> > > power-on feature.
> >
> > Yes, correct, as they way things are today.
> >
> > It all boils down to that attaching a device to multiple PM domains
> > can't really be done in a generic way, as it becomes device/platform
> > specific. Since this needs to be managed by the drivers/buses anyway,
> > they might as well get control of what PM domain they need to power-on
> > to probe their devices.
> 
> Due to the above, it might be a good idea to power-on the SCMI
> *power-domains* during boot and leave them on to allow drivers to continue
> to probe their devices?

For debug, this is ok. But release the code for production, keep them enabled
during boot is not good.
> 
> Maybe a module parameter or Kconfig debug option could be used to control
> this?

Greg might not be happy for introducing module parameter, I guess.

> 
> In this way an updated DTS with that adds a performance domain to a
> consumer device node (which already has a power-domain), should allow the
> consumer driver to continue to probe successfully.
> 
> Peng, would this resolve your concern?

Actually I am not sure. multiple PD is not a technical issue, it is just adding
more changes to various device drivers, we have VPU/GPU/DISPLAY/NPU/
HSIO/CAMERA and etc.. so all the drivers need update, which is
not welcomed by driver developers :)
I am still trying to enable multiple PD for saying MMC,
and see how it works after adding performance domain and how device
dvfs works in such case.

Thanks,
Peng.

> 
> Kind regards
> Uffe

Re: Question regarding scmi_perf_domain.c

[Sudeep Holla]

On Tue, Oct 17, 2023 at 01:18:38PM +0000, Peng Fan wrote:
> > Subject: Re: Question regarding scmi_perf_domain.c
> > 
> > On Thu, 12 Oct 2023 at 13:53, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > >
> > > On Wed, 11 Oct 2023 at 16:17, Sudeep Holla <sudeep.holla@arm.com>
> > wrote:
> > > >
> > > > On Wed, Oct 11, 2023 at 11:26:54AM +0200, Ulf Hansson wrote:
> > > >
> > > > [..]
> > > >
> > > > > Not sure exactly what you are referring to when saying that
> > > > > "automatic power domain on is broken". Genpd power-on the PM
> > > > > domain for a device that gets attached to it, if the device has only a
> > single PM domain.
> > > > > This is the legacy behaviour.
> > > > >
> > > > > When we added support for multiple PM domains per device, we
> > > > > decided to *not* power-on the PM domain, if the device that gets
> > > > > attached has multiple PM domains. This behaviour was chosen
> > > > > deliberately, to allow consumer drivers to decide themselves
> > > > > instead. Is there a problem with this you think?
> > > > >
> > > >
> > > > Just my understanding. Since the second PM domain added now is for
> > > > perf and is not strictly power domain, Peng's concern is switching
> > > > to this binding will make the platform loose this automatic genpd
> > > > power-on feature.
> > >
> > > Yes, correct, as they way things are today.
> > >
> > > It all boils down to that attaching a device to multiple PM domains
> > > can't really be done in a generic way, as it becomes device/platform
> > > specific. Since this needs to be managed by the drivers/buses anyway,
> > > they might as well get control of what PM domain they need to power-on
> > > to probe their devices.
> > 
> > Due to the above, it might be a good idea to power-on the SCMI
> > *power-domains* during boot and leave them on to allow drivers to continue
> > to probe their devices?
> 
> For debug, this is ok. But release the code for production, keep them enabled
> during boot is not good.

What is the point in adding it ? You can always hack and test if you need.

> > 
> > Maybe a module parameter or Kconfig debug option could be used to control
> > this?
> 
> Greg might not be happy for introducing module parameter, I guess.
>

True. But I don't see point in adding a Kconfig as it needs to be enabled
with single (distro) Image.

> > 
> > In this way an updated DTS with that adds a performance domain to a
> > consumer device node (which already has a power-domain), should allow the
> > consumer driver to continue to probe successfully.
> > 
> > Peng, would this resolve your concern?
> 
> Actually I am not sure. multiple PD is not a technical issue, it is just adding
> more changes to various device drivers, we have VPU/GPU/DISPLAY/NPU/
> HSIO/CAMERA and etc.. so all the drivers need update, which is
> not welcomed by driver developers :)

Why ? Have you posted the patches ? Any discussions you can point to ?

> I am still trying to enable multiple PD for saying MMC,
> and see how it works after adding performance domain and how device
> dvfs works in such case.
>

Interesting. So MMC domain is presented as perf domain rather than the clock
+ regulator ? Nice to see that abstraction being used.

-- 
Regards,
Sudeep

RE: Question regarding scmi_perf_domain.c

[Peng Fan]

> Subject: Re: Question regarding scmi_perf_domain.c
> 
> On Tue, Oct 17, 2023 at 01:18:38PM +0000, Peng Fan wrote:
> > > Subject: Re: Question regarding scmi_perf_domain.c
> > >
> > > On Thu, 12 Oct 2023 at 13:53, Ulf Hansson <ulf.hansson@linaro.org>
> wrote:
> > > >
> > > > On Wed, 11 Oct 2023 at 16:17, Sudeep Holla <sudeep.holla@arm.com>
> > > wrote:
> > > > >
> > > > > On Wed, Oct 11, 2023 at 11:26:54AM +0200, Ulf Hansson wrote:
> > > > >
> > > > > [..]
> > > > >
> > > > > > Not sure exactly what you are referring to when saying that
> > > > > > "automatic power domain on is broken". Genpd power-on the PM
> > > > > > domain for a device that gets attached to it, if the device
> > > > > > has only a
> > > single PM domain.
> > > > > > This is the legacy behaviour.
> > > > > >
> > > > > > When we added support for multiple PM domains per device, we
> > > > > > decided to *not* power-on the PM domain, if the device that
> > > > > > gets attached has multiple PM domains. This behaviour was
> > > > > > chosen deliberately, to allow consumer drivers to decide
> > > > > > themselves instead. Is there a problem with this you think?
> > > > > >
> > > > >
> > > > > Just my understanding. Since the second PM domain added now is
> > > > > for perf and is not strictly power domain, Peng's concern is
> > > > > switching to this binding will make the platform loose this
> > > > > automatic genpd power-on feature.
> > > >
> > > > Yes, correct, as they way things are today.
> > > >
> > > > It all boils down to that attaching a device to multiple PM
> > > > domains can't really be done in a generic way, as it becomes
> > > > device/platform specific. Since this needs to be managed by the
> > > > drivers/buses anyway, they might as well get control of what PM
> > > > domain they need to power-on to probe their devices.
> > >
> > > Due to the above, it might be a good idea to power-on the SCMI
> > > *power-domains* during boot and leave them on to allow drivers to
> > > continue to probe their devices?
> >
> > For debug, this is ok. But release the code for production, keep them
> > enabled during boot is not good.
> 
> What is the point in adding it ? You can always hack and test if you need.

I might misunderstand, I thought this was a formal solution.
> 
> > >
> > > Maybe a module parameter or Kconfig debug option could be used to
> > > control this?
> >
> > Greg might not be happy for introducing module parameter, I guess.
> >
> 
> True. But I don't see point in adding a Kconfig as it needs to be enabled with
> single (distro) Image.
> 
> > >
> > > In this way an updated DTS with that adds a performance domain to a
> > > consumer device node (which already has a power-domain), should
> > > allow the consumer driver to continue to probe successfully.
> > >
> > > Peng, would this resolve your concern?
> >
> > Actually I am not sure. multiple PD is not a technical issue, it is
> > just adding more changes to various device drivers, we have
> > VPU/GPU/DISPLAY/NPU/ HSIO/CAMERA and etc.. so all the drivers need
> > update, which is not welcomed by driver developers :)
> 
> Why ? Have you posted the patches ? Any discussions you can point to ?

I mean NXP internal.

> 
> > I am still trying to enable multiple PD for saying MMC, and see how it
> > works after adding performance domain and how device dvfs works in
> > such case.
> >
> 
> Interesting. So MMC domain is presented as perf domain rather than the
> clock
> + regulator ? Nice to see that abstraction being used.

MMC IP is inside a block(named mix) which contains several other IPs.
Current scmi sever perf design only exports MIX level perf,  no regulator
for now.

Regards,
Peng.

> 
> --
> Regards,
> Sudeep

Re: Question regarding scmi_perf_domain.c

[Sudeep Holla]

On Tue, Oct 17, 2023 at 02:35:49PM +0000, Peng Fan wrote:
> > Subject: Re: Question regarding scmi_perf_domain.c
> >

[...]

> > What is the point in adding it ? You can always hack and test if you need.
> 
> I might misunderstand, I thought this was a formal solution.

Not really, it was a proposal which we are discussing the options here.

> > 
> > > >
> > > > Maybe a module parameter or Kconfig debug option could be used to
> > > > control this?
> > >
> > > Greg might not be happy for introducing module parameter, I guess.
> > >
> > 
> > True. But I don't see point in adding a Kconfig as it needs to be enabled with
> > single (distro) Image.


And figured out both module param and Kconfig are not good options.

> > 
> > > >
> > > > In this way an updated DTS with that adds a performance domain to a
> > > > consumer device node (which already has a power-domain), should
> > > > allow the consumer driver to continue to probe successfully.
> > > >
> > > > Peng, would this resolve your concern?
> > >
> > > Actually I am not sure. multiple PD is not a technical issue, it is
> > > just adding more changes to various device drivers, we have
> > > VPU/GPU/DISPLAY/NPU/ HSIO/CAMERA and etc.. so all the drivers need
> > > update, which is not welcomed by driver developers :)
> > 
> > Why ? Have you posted the patches ? Any discussions you can point to ?
> 
> I mean NXP internal.
>

OK at this point, I really don't care then. I was under the assumption
that we were talking everything upstream here. Downstream always has the
burden of constantly adapting to the upstream changes. This is not a
user ABI. I was pushing Ulf assuming something is needed upstream.

Sorry Ulf, I wasn't aware of this downstream driver business here, my bad.

So, if the genpd has changed the behaviour for multiple agents and has
decided not to support this case, then the drivers need to adapt. I don't
see a way out by working around in the SCMI driver as it may break some
other platforms and we haven't got a way to do it conditionally other
than module param which you have raised right concerns.

iAll I can say is downstream drivers need to adapt. Sorry I know it is
not what you want to hear but it is part and parcel for the downstream
code.

> > 
> > > I am still trying to enable multiple PD for saying MMC, and see how it
> > > works after adding performance domain and how device dvfs works in
> > > such case.
> > >
> > 
> > Interesting. So MMC domain is presented as perf domain rather than the
> > clock
> > + regulator ? Nice to see that abstraction being used.
> 
> MMC IP is inside a block(named mix) which contains several other IPs.
> Current scmi sever perf design only exports MIX level perf,  no regulator
> for now.
>

Thanks for the details.

-- 
Regards,
Sudeep

Re: Question regarding scmi_perf_domain.c

[Sudeep Holla]

On Tue, Oct 10, 2023 at 01:02:01PM +0200, Ulf Hansson wrote:
> On Tue, 10 Oct 2023 at 12:55, Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > On Tue, Oct 10, 2023 at 10:30:17AM +0000, Peng Fan wrote:
> > > Hi Ulf,
> > >
> > > I just see you wrote scmi_perf_domain.c, just wonder this driver is only
> > > for devices, not support arm cores, right?
> > >
> > > For ARM cores, we still need scmi_cpufreq.c for performance settings,
> > > right?
> >
> > Sorry if I wasn't clear. The reason I mentioned it in private is that
> > we now support the power domain bindings in the scmi-cpufreq.c as you
> > were little bit nervous to use the clock bindings(though they work just
> > fine, I understand the possible confusion with the clock protocol).
>
> Right, good point!
>
> I think we discussed earlier whether we should deprecate the use of
> the clock bindings. Maybe that's a good idea, to indicate that we
> prefer the power-domain bindings when going forward?

Yes we could do that. I prefer to have some example in the actual DTS
files before we can think of deprecating it. I need to get around, test
and push the change to switch from clock to power domain bindings on
Juno for example.

--
Regards,
Sudeep

RE: Question regarding scmi_perf_domain.c

[Peng Fan]

Hi Sudeep

> Subject: Re: Question regarding scmi_perf_domain.c
> 
> On Tue, Oct 10, 2023 at 01:02:01PM +0200, Ulf Hansson wrote:
> > On Tue, 10 Oct 2023 at 12:55, Sudeep Holla <sudeep.holla@arm.com>
> wrote:
> > >
> > > On Tue, Oct 10, 2023 at 10:30:17AM +0000, Peng Fan wrote:
> > > > Hi Ulf,
> > > >
> > > > I just see you wrote scmi_perf_domain.c, just wonder this driver
> > > > is only for devices, not support arm cores, right?
> > > >
> > > > For ARM cores, we still need scmi_cpufreq.c for performance
> > > > settings, right?
> > >
> > > Sorry if I wasn't clear. The reason I mentioned it in private is
> > > that we now support the power domain bindings in the scmi-cpufreq.c
> > > as you were little bit nervous to use the clock bindings(though they
> > > work just fine, I understand the possible confusion with the clock
> protocol).
> >
> > Right, good point!
> >
> > I think we discussed earlier whether we should deprecate the use of
> > the clock bindings. Maybe that's a good idea, to indicate that we
> > prefer the power-domain bindings when going forward?
> 
> Yes we could do that. I prefer to have some example in the actual DTS files
> before we can think of deprecating it. I need to get around, test and push the
> change to switch from clock to power domain bindings on Juno for example.

Before that, I think we need think about whether it is possible to use
a property saying perf-domain, using power domains implies a restriction
that power domain is same as perf domain, but the spec not say that.

Thanks,
Peng.

> 
> --
> Regards,
> Sudeep

Re: Question regarding scmi_perf_domain.c

[Sudeep Holla]

On Tue, Oct 10, 2023 at 12:53:58PM +0000, Peng Fan wrote:
> Hi Sudeep
>
> > Subject: Re: Question regarding scmi_perf_domain.c
> >
> > On Tue, Oct 10, 2023 at 01:02:01PM +0200, Ulf Hansson wrote:
> > > On Tue, 10 Oct 2023 at 12:55, Sudeep Holla <sudeep.holla@arm.com>
> > wrote:
> > > >
> > > > On Tue, Oct 10, 2023 at 10:30:17AM +0000, Peng Fan wrote:
> > > > > Hi Ulf,
> > > > >
> > > > > I just see you wrote scmi_perf_domain.c, just wonder this driver
> > > > > is only for devices, not support arm cores, right?
> > > > >
> > > > > For ARM cores, we still need scmi_cpufreq.c for performance
> > > > > settings, right?
> > > >
> > > > Sorry if I wasn't clear. The reason I mentioned it in private is
> > > > that we now support the power domain bindings in the scmi-cpufreq.c
> > > > as you were little bit nervous to use the clock bindings(though they
> > > > work just fine, I understand the possible confusion with the clock
> > protocol).
> > >
> > > Right, good point!
> > >
> > > I think we discussed earlier whether we should deprecate the use of
> > > the clock bindings. Maybe that's a good idea, to indicate that we
> > > prefer the power-domain bindings when going forward?
> >
> > Yes we could do that. I prefer to have some example in the actual DTS files
> > before we can think of deprecating it. I need to get around, test and push the
> > change to switch from clock to power domain bindings on Juno for example.
>
> Before that, I think we need think about whether it is possible to use
> a property saying perf-domain, using power domains implies a restriction
> that power domain is same as perf domain, but the spec not say that.
>

Just responded on the other thread, lets continue the discussion there to
keep the related discussions together there.

--
Regards,
Sudeep