Re: [PATCH 2/2] cpufreq: brcmstb-avs-cpufreq: prefer SCMI cpufreq if supported

2018-04-22 Thread Viresh Kumar 
On 20-04-18, 09:50, Florian Fainelli wrote:
> On 04/20/2018 02:35 AM, Viresh Kumar wrote:
> > On 20-04-18, 10:15, Sudeep Holla wrote:
> >> It still doesn't give the flexibility to switch between the two
> >> implementations boot time based on some firmware config(e.g. DT status
> >> property).
> > 
> > I agree, but it didn't look like they need flexibility :)
> > 
> > Lets see how the intend to use it. If they are *always* going to use SCPI if
> > that is available, then it should be solved at Kconfig level only. Else they
> > shouldn't put such code in the driver to quit early.
> 
> We have both drivers (brcmstb-avs-cpufreq and scmi-cpufreq) enabled in
> our kernel configuration, however, depending on the firmware version, we
> may have a number of combinations:
> 
> - arm,scmi DT node is present and enabled (status = okay) as well as
> brcmstb-avs-cpufreq being present and enabled
> - arm,scmi DT node is present but disabled (status = disabled) and
> brcmstb-avs-cpufreq is being present and enabled

In this case the Kconfig thing I have been talking about doesn't apply anymore.

-- 
viresh

Re: [PATCH 2/2] cpufreq: brcmstb-avs-cpufreq: prefer SCMI cpufreq if supported

2018-04-22 Thread Viresh Kumar 
On 20-04-18, 09:50, Florian Fainelli wrote:
> On 04/20/2018 02:35 AM, Viresh Kumar wrote:
> > On 20-04-18, 10:15, Sudeep Holla wrote:
> >> It still doesn't give the flexibility to switch between the two
> >> implementations boot time based on some firmware config(e.g. DT status
> >> property).
> > 
> > I agree, but it didn't look like they need flexibility :)
> > 
> > Lets see how the intend to use it. If they are *always* going to use SCPI if
> > that is available, then it should be solved at Kconfig level only. Else they
> > shouldn't put such code in the driver to quit early.
> 
> We have both drivers (brcmstb-avs-cpufreq and scmi-cpufreq) enabled in
> our kernel configuration, however, depending on the firmware version, we
> may have a number of combinations:
> 
> - arm,scmi DT node is present and enabled (status = okay) as well as
> brcmstb-avs-cpufreq being present and enabled
> - arm,scmi DT node is present but disabled (status = disabled) and
> brcmstb-avs-cpufreq is being present and enabled

In this case the Kconfig thing I have been talking about doesn't apply anymore.

-- 
viresh

Re: [PATCH 2/2] cpufreq: brcmstb-avs-cpufreq: prefer SCMI cpufreq if supported

2018-04-20 Thread Florian Fainelli 
On 04/20/2018 02:35 AM, Viresh Kumar wrote:
> On 20-04-18, 10:15, Sudeep Holla wrote:
>> It still doesn't give the flexibility to switch between the two
>> implementations boot time based on some firmware config(e.g. DT status
>> property).
> 
> I agree, but it didn't look like they need flexibility :)
> 
> Lets see how the intend to use it. If they are *always* going to use SCPI if
> that is available, then it should be solved at Kconfig level only. Else they
> shouldn't put such code in the driver to quit early.

We have both drivers (brcmstb-avs-cpufreq and scmi-cpufreq) enabled in
our kernel configuration, however, depending on the firmware version, we
may have a number of combinations:

- arm,scmi DT node is present and enabled (status = okay) as well as
brcmstb-avs-cpufreq being present and enabled
- arm,scmi DT node is present but disabled (status = disabled) and
brcmstb-avs-cpufreq is being present and enabled

If you think this is a self inflicted, downstream and backwards/forwards
compatible relevant only change, I suppose we are fine with that too.
-- 
Florian

Re: [PATCH 2/2] cpufreq: brcmstb-avs-cpufreq: prefer SCMI cpufreq if supported

2018-04-20 Thread Florian Fainelli 
On 04/20/2018 02:35 AM, Viresh Kumar wrote:
> On 20-04-18, 10:15, Sudeep Holla wrote:
>> It still doesn't give the flexibility to switch between the two
>> implementations boot time based on some firmware config(e.g. DT status
>> property).
> 
> I agree, but it didn't look like they need flexibility :)
> 
> Lets see how the intend to use it. If they are *always* going to use SCPI if
> that is available, then it should be solved at Kconfig level only. Else they
> shouldn't put such code in the driver to quit early.

We have both drivers (brcmstb-avs-cpufreq and scmi-cpufreq) enabled in
our kernel configuration, however, depending on the firmware version, we
may have a number of combinations:

- arm,scmi DT node is present and enabled (status = okay) as well as
brcmstb-avs-cpufreq being present and enabled
- arm,scmi DT node is present but disabled (status = disabled) and
brcmstb-avs-cpufreq is being present and enabled

If you think this is a self inflicted, downstream and backwards/forwards
compatible relevant only change, I suppose we are fine with that too.
-- 
Florian

Re: [PATCH 2/2] cpufreq: brcmstb-avs-cpufreq: prefer SCMI cpufreq if supported

2018-04-20 Thread Viresh Kumar 
On 20-04-18, 10:15, Sudeep Holla wrote:
> It still doesn't give the flexibility to switch between the two
> implementations boot time based on some firmware config(e.g. DT status
> property).

I agree, but it didn't look like they need flexibility :)

Lets see how the intend to use it. If they are *always* going to use SCPI if
that is available, then it should be solved at Kconfig level only. Else they
shouldn't put such code in the driver to quit early.

-- 
viresh

Re: [PATCH 2/2] cpufreq: brcmstb-avs-cpufreq: prefer SCMI cpufreq if supported

2018-04-20 Thread Viresh Kumar 
On 20-04-18, 10:15, Sudeep Holla wrote:
> It still doesn't give the flexibility to switch between the two
> implementations boot time based on some firmware config(e.g. DT status
> property).

I agree, but it didn't look like they need flexibility :)

Lets see how the intend to use it. If they are *always* going to use SCPI if
that is available, then it should be solved at Kconfig level only. Else they
shouldn't put such code in the driver to quit early.

-- 
viresh

Re: [PATCH 2/2] cpufreq: brcmstb-avs-cpufreq: prefer SCMI cpufreq if supported

2018-04-20 Thread Sudeep Holla 


On 20/04/18 05:42, Viresh Kumar wrote:
> On 19-04-18, 11:37, Sudeep Holla wrote:
>>
>>
>> On 19/04/18 05:16, Viresh Kumar wrote:
>>> On 18-04-18, 08:56, Markus Mayer wrote:
 From: Jim Quinlan 

 If the SCMI cpufreq driver is supported, we bail, so that the new
 approach can be used.

 Signed-off-by: Jim Quinlan 
 Signed-off-by: Markus Mayer 
 ---
  drivers/cpufreq/brcmstb-avs-cpufreq.c | 16 
  1 file changed, 16 insertions(+)

 diff --git a/drivers/cpufreq/brcmstb-avs-cpufreq.c 
 b/drivers/cpufreq/brcmstb-avs-cpufreq.c
 index b07559b9ed99..b4861a730162 100644
 --- a/drivers/cpufreq/brcmstb-avs-cpufreq.c
 +++ b/drivers/cpufreq/brcmstb-avs-cpufreq.c
 @@ -164,6 +164,8 @@
  #define BRCM_AVS_CPU_INTR "brcm,avs-cpu-l2-intr"
  #define BRCM_AVS_HOST_INTR"sw_intr"
  
 +#define ARM_SCMI_COMPAT   "arm,scmi"
 +
  struct pmap {
unsigned int mode;
unsigned int p1;
 @@ -511,6 +513,20 @@ static int brcm_avs_prepare_init(struct 
 platform_device *pdev)
struct device *dev;
int host_irq, ret;
  
 +  /*
 +   * If the SCMI cpufreq driver is supported, we bail, so that the more
 +   * modern approach can be used.
 +   */
 +  if (IS_ENABLED(CONFIG_ARM_SCMI_PROTOCOL)) {
 +  struct device_node *np;
 +
 +  np = of_find_compatible_node(NULL, NULL, ARM_SCMI_COMPAT);
 +  if (np) {
 +  of_node_put(np);
 +  return -ENXIO;
 +  }
 +  }
 +
>>>
>>> What about adding !CONFIG_ARM_SCMI_PROTOCOL in Kconfig dependency and don't
>>> compile the driver at all ?
>>>
>>
>> Unfortunately, that may not be good idea with single image needing both
>> configs to be enabled.
> 
> Sure, but looking at the above code, it looked like they don't need the other
> config if SCMI is enabled.
> 

Yes, I understand that. But if they just want to run a distro kernel or
a defconfig with all the options enabled, then it's not possible. But if
they always build kernel with some custom config options, then fine.

It still doesn't give the flexibility to switch between the two
implementations boot time based on some firmware config(e.g. DT status
property).

-- 
Regards,
Sudeep

Re: [PATCH 2/2] cpufreq: brcmstb-avs-cpufreq: prefer SCMI cpufreq if supported

2018-04-20 Thread Sudeep Holla 


On 20/04/18 05:42, Viresh Kumar wrote:
> On 19-04-18, 11:37, Sudeep Holla wrote:
>>
>>
>> On 19/04/18 05:16, Viresh Kumar wrote:
>>> On 18-04-18, 08:56, Markus Mayer wrote:
 From: Jim Quinlan 

 If the SCMI cpufreq driver is supported, we bail, so that the new
 approach can be used.

 Signed-off-by: Jim Quinlan 
 Signed-off-by: Markus Mayer 
 ---
  drivers/cpufreq/brcmstb-avs-cpufreq.c | 16 
  1 file changed, 16 insertions(+)

 diff --git a/drivers/cpufreq/brcmstb-avs-cpufreq.c 
 b/drivers/cpufreq/brcmstb-avs-cpufreq.c
 index b07559b9ed99..b4861a730162 100644
 --- a/drivers/cpufreq/brcmstb-avs-cpufreq.c
 +++ b/drivers/cpufreq/brcmstb-avs-cpufreq.c
 @@ -164,6 +164,8 @@
  #define BRCM_AVS_CPU_INTR "brcm,avs-cpu-l2-intr"
  #define BRCM_AVS_HOST_INTR"sw_intr"
  
 +#define ARM_SCMI_COMPAT   "arm,scmi"
 +
  struct pmap {
unsigned int mode;
unsigned int p1;
 @@ -511,6 +513,20 @@ static int brcm_avs_prepare_init(struct 
 platform_device *pdev)
struct device *dev;
int host_irq, ret;
  
 +  /*
 +   * If the SCMI cpufreq driver is supported, we bail, so that the more
 +   * modern approach can be used.
 +   */
 +  if (IS_ENABLED(CONFIG_ARM_SCMI_PROTOCOL)) {
 +  struct device_node *np;
 +
 +  np = of_find_compatible_node(NULL, NULL, ARM_SCMI_COMPAT);
 +  if (np) {
 +  of_node_put(np);
 +  return -ENXIO;
 +  }
 +  }
 +
>>>
>>> What about adding !CONFIG_ARM_SCMI_PROTOCOL in Kconfig dependency and don't
>>> compile the driver at all ?
>>>
>>
>> Unfortunately, that may not be good idea with single image needing both
>> configs to be enabled.
> 
> Sure, but looking at the above code, it looked like they don't need the other
> config if SCMI is enabled.
> 

Yes, I understand that. But if they just want to run a distro kernel or
a defconfig with all the options enabled, then it's not possible. But if
they always build kernel with some custom config options, then fine.

It still doesn't give the flexibility to switch between the two
implementations boot time based on some firmware config(e.g. DT status
property).

-- 
Regards,
Sudeep

Re: [PATCH 2/2] cpufreq: brcmstb-avs-cpufreq: prefer SCMI cpufreq if supported

2018-04-19 Thread Viresh Kumar 
On 19-04-18, 11:37, Sudeep Holla wrote:
> 
> 
> On 19/04/18 05:16, Viresh Kumar wrote:
> > On 18-04-18, 08:56, Markus Mayer wrote:
> >> From: Jim Quinlan 
> >>
> >> If the SCMI cpufreq driver is supported, we bail, so that the new
> >> approach can be used.
> >>
> >> Signed-off-by: Jim Quinlan 
> >> Signed-off-by: Markus Mayer 
> >> ---
> >>  drivers/cpufreq/brcmstb-avs-cpufreq.c | 16 
> >>  1 file changed, 16 insertions(+)
> >>
> >> diff --git a/drivers/cpufreq/brcmstb-avs-cpufreq.c 
> >> b/drivers/cpufreq/brcmstb-avs-cpufreq.c
> >> index b07559b9ed99..b4861a730162 100644
> >> --- a/drivers/cpufreq/brcmstb-avs-cpufreq.c
> >> +++ b/drivers/cpufreq/brcmstb-avs-cpufreq.c
> >> @@ -164,6 +164,8 @@
> >>  #define BRCM_AVS_CPU_INTR "brcm,avs-cpu-l2-intr"
> >>  #define BRCM_AVS_HOST_INTR"sw_intr"
> >>  
> >> +#define ARM_SCMI_COMPAT   "arm,scmi"
> >> +
> >>  struct pmap {
> >>unsigned int mode;
> >>unsigned int p1;
> >> @@ -511,6 +513,20 @@ static int brcm_avs_prepare_init(struct 
> >> platform_device *pdev)
> >>struct device *dev;
> >>int host_irq, ret;
> >>  
> >> +  /*
> >> +   * If the SCMI cpufreq driver is supported, we bail, so that the more
> >> +   * modern approach can be used.
> >> +   */
> >> +  if (IS_ENABLED(CONFIG_ARM_SCMI_PROTOCOL)) {
> >> +  struct device_node *np;
> >> +
> >> +  np = of_find_compatible_node(NULL, NULL, ARM_SCMI_COMPAT);
> >> +  if (np) {
> >> +  of_node_put(np);
> >> +  return -ENXIO;
> >> +  }
> >> +  }
> >> +
> > 
> > What about adding !CONFIG_ARM_SCMI_PROTOCOL in Kconfig dependency and don't
> > compile the driver at all ?
> > 
> 
> Unfortunately, that may not be good idea with single image needing both
> configs to be enabled.

Sure, but looking at the above code, it looked like they don't need the other
config if SCMI is enabled.

-- 
viresh

Re: [PATCH 2/2] cpufreq: brcmstb-avs-cpufreq: prefer SCMI cpufreq if supported

2018-04-19 Thread Viresh Kumar 
On 19-04-18, 11:37, Sudeep Holla wrote:
> 
> 
> On 19/04/18 05:16, Viresh Kumar wrote:
> > On 18-04-18, 08:56, Markus Mayer wrote:
> >> From: Jim Quinlan 
> >>
> >> If the SCMI cpufreq driver is supported, we bail, so that the new
> >> approach can be used.
> >>
> >> Signed-off-by: Jim Quinlan 
> >> Signed-off-by: Markus Mayer 
> >> ---
> >>  drivers/cpufreq/brcmstb-avs-cpufreq.c | 16 
> >>  1 file changed, 16 insertions(+)
> >>
> >> diff --git a/drivers/cpufreq/brcmstb-avs-cpufreq.c 
> >> b/drivers/cpufreq/brcmstb-avs-cpufreq.c
> >> index b07559b9ed99..b4861a730162 100644
> >> --- a/drivers/cpufreq/brcmstb-avs-cpufreq.c
> >> +++ b/drivers/cpufreq/brcmstb-avs-cpufreq.c
> >> @@ -164,6 +164,8 @@
> >>  #define BRCM_AVS_CPU_INTR "brcm,avs-cpu-l2-intr"
> >>  #define BRCM_AVS_HOST_INTR"sw_intr"
> >>  
> >> +#define ARM_SCMI_COMPAT   "arm,scmi"
> >> +
> >>  struct pmap {
> >>unsigned int mode;
> >>unsigned int p1;
> >> @@ -511,6 +513,20 @@ static int brcm_avs_prepare_init(struct 
> >> platform_device *pdev)
> >>struct device *dev;
> >>int host_irq, ret;
> >>  
> >> +  /*
> >> +   * If the SCMI cpufreq driver is supported, we bail, so that the more
> >> +   * modern approach can be used.
> >> +   */
> >> +  if (IS_ENABLED(CONFIG_ARM_SCMI_PROTOCOL)) {
> >> +  struct device_node *np;
> >> +
> >> +  np = of_find_compatible_node(NULL, NULL, ARM_SCMI_COMPAT);
> >> +  if (np) {
> >> +  of_node_put(np);
> >> +  return -ENXIO;
> >> +  }
> >> +  }
> >> +
> > 
> > What about adding !CONFIG_ARM_SCMI_PROTOCOL in Kconfig dependency and don't
> > compile the driver at all ?
> > 
> 
> Unfortunately, that may not be good idea with single image needing both
> configs to be enabled.

Sure, but looking at the above code, it looked like they don't need the other
config if SCMI is enabled.

-- 
viresh

Re: [PATCH 2/2] cpufreq: brcmstb-avs-cpufreq: prefer SCMI cpufreq if supported

2018-04-19 Thread Markus Mayer 
On 18 April 2018 at 09:37, Florian Fainelli  wrote:
> On 04/18/2018 08:56 AM, Markus Mayer wrote:
>> From: Jim Quinlan 
>>
>> If the SCMI cpufreq driver is supported, we bail, so that the new
>> approach can be used.
>>
>> Signed-off-by: Jim Quinlan 
>> Signed-off-by: Markus Mayer 
>> ---
>>  drivers/cpufreq/brcmstb-avs-cpufreq.c | 16 
>>  1 file changed, 16 insertions(+)
>>
>> diff --git a/drivers/cpufreq/brcmstb-avs-cpufreq.c 
>> b/drivers/cpufreq/brcmstb-avs-cpufreq.c
>> index b07559b9ed99..b4861a730162 100644
>> --- a/drivers/cpufreq/brcmstb-avs-cpufreq.c
>> +++ b/drivers/cpufreq/brcmstb-avs-cpufreq.c
>> @@ -164,6 +164,8 @@
>>  #define BRCM_AVS_CPU_INTR"brcm,avs-cpu-l2-intr"
>>  #define BRCM_AVS_HOST_INTR   "sw_intr"
>>
>> +#define ARM_SCMI_COMPAT  "arm,scmi"
>> +
>>  struct pmap {
>>   unsigned int mode;
>>   unsigned int p1;
>> @@ -511,6 +513,20 @@ static int brcm_avs_prepare_init(struct platform_device 
>> *pdev)
>>   struct device *dev;
>>   int host_irq, ret;
>>
>> + /*
>> +  * If the SCMI cpufreq driver is supported, we bail, so that the more
>> +  * modern approach can be used.
>> +  */
>> + if (IS_ENABLED(CONFIG_ARM_SCMI_PROTOCOL)) {
>> + struct device_node *np;
>> +
>> + np = of_find_compatible_node(NULL, NULL, ARM_SCMI_COMPAT);
>> + if (np) {
>> + of_node_put(np);
>> + return -ENXIO;
>> + }
>
> We would probably want to make sure that the node is also enabled (that
> is, does not have a status = "disabled" property) otherwise the check
> can be defeated. Something like:
>
> if (np && of_device_is_available(np))

Would we want something like this instead?

 if (np) {
  bool bail_early =
(of_device_is_available(np) > 0);

  of_node_put(np);
  if (bail_early)
  return -ENXIO;
 }

To ensure of_node_put() is called?

> should be good for that.
>
> Thanks!
> --
> Florian

Re: [PATCH 2/2] cpufreq: brcmstb-avs-cpufreq: prefer SCMI cpufreq if supported

2018-04-19 Thread Markus Mayer 
On 18 April 2018 at 09:37, Florian Fainelli  wrote:
> On 04/18/2018 08:56 AM, Markus Mayer wrote:
>> From: Jim Quinlan 
>>
>> If the SCMI cpufreq driver is supported, we bail, so that the new
>> approach can be used.
>>
>> Signed-off-by: Jim Quinlan 
>> Signed-off-by: Markus Mayer 
>> ---
>>  drivers/cpufreq/brcmstb-avs-cpufreq.c | 16 
>>  1 file changed, 16 insertions(+)
>>
>> diff --git a/drivers/cpufreq/brcmstb-avs-cpufreq.c 
>> b/drivers/cpufreq/brcmstb-avs-cpufreq.c
>> index b07559b9ed99..b4861a730162 100644
>> --- a/drivers/cpufreq/brcmstb-avs-cpufreq.c
>> +++ b/drivers/cpufreq/brcmstb-avs-cpufreq.c
>> @@ -164,6 +164,8 @@
>>  #define BRCM_AVS_CPU_INTR"brcm,avs-cpu-l2-intr"
>>  #define BRCM_AVS_HOST_INTR   "sw_intr"
>>
>> +#define ARM_SCMI_COMPAT  "arm,scmi"
>> +
>>  struct pmap {
>>   unsigned int mode;
>>   unsigned int p1;
>> @@ -511,6 +513,20 @@ static int brcm_avs_prepare_init(struct platform_device 
>> *pdev)
>>   struct device *dev;
>>   int host_irq, ret;
>>
>> + /*
>> +  * If the SCMI cpufreq driver is supported, we bail, so that the more
>> +  * modern approach can be used.
>> +  */
>> + if (IS_ENABLED(CONFIG_ARM_SCMI_PROTOCOL)) {
>> + struct device_node *np;
>> +
>> + np = of_find_compatible_node(NULL, NULL, ARM_SCMI_COMPAT);
>> + if (np) {
>> + of_node_put(np);
>> + return -ENXIO;
>> + }
>
> We would probably want to make sure that the node is also enabled (that
> is, does not have a status = "disabled" property) otherwise the check
> can be defeated. Something like:
>
> if (np && of_device_is_available(np))

Would we want something like this instead?

 if (np) {
  bool bail_early =
(of_device_is_available(np) > 0);

  of_node_put(np);
  if (bail_early)
  return -ENXIO;
 }

To ensure of_node_put() is called?

> should be good for that.
>
> Thanks!
> --
> Florian

Re: [PATCH 2/2] cpufreq: brcmstb-avs-cpufreq: prefer SCMI cpufreq if supported

2018-04-19 Thread Florian Fainelli 


On 04/19/2018 03:35 AM, Sudeep Holla wrote:
> 
> 
> On 18/04/18 16:56, Markus Mayer wrote:
>> From: Jim Quinlan 
>>
>> If the SCMI cpufreq driver is supported, we bail, so that the new
>> approach can be used.
>>
>> Signed-off-by: Jim Quinlan 
>> Signed-off-by: Markus Mayer 
>> ---
>>  drivers/cpufreq/brcmstb-avs-cpufreq.c | 16 
>>  1 file changed, 16 insertions(+)
>>
>> diff --git a/drivers/cpufreq/brcmstb-avs-cpufreq.c 
>> b/drivers/cpufreq/brcmstb-avs-cpufreq.c
>> index b07559b9ed99..b4861a730162 100644
>> --- a/drivers/cpufreq/brcmstb-avs-cpufreq.c
>> +++ b/drivers/cpufreq/brcmstb-avs-cpufreq.c
>> @@ -164,6 +164,8 @@
>>  #define BRCM_AVS_CPU_INTR   "brcm,avs-cpu-l2-intr"
>>  #define BRCM_AVS_HOST_INTR  "sw_intr"
>>  
>> +#define ARM_SCMI_COMPAT "arm,scmi"
>> +
>>  struct pmap {
>>  unsigned int mode;
>>  unsigned int p1;
>> @@ -511,6 +513,20 @@ static int brcm_avs_prepare_init(struct platform_device 
>> *pdev)
>>  struct device *dev;
>>  int host_irq, ret;
>>  
> 
> Will this platform have both SCMI and BRCM_AVS_CPU_DATA nodes enabled ?
> If so, is it not better to just keep only the preferred node enabled
> instead ?

The kernel image has both drivers enabled, the Device Tree blob we pass
contains both nodes, and should flip the status properties based on what
is available. We had some internal discussion about that specific
change, and we ended up having the patch being submitted to seek
external advice, I guess we have an answer now this is not desired.
-- 
Florian

Re: [PATCH 2/2] cpufreq: brcmstb-avs-cpufreq: prefer SCMI cpufreq if supported

2018-04-19 Thread Florian Fainelli 


On 04/19/2018 03:35 AM, Sudeep Holla wrote:
> 
> 
> On 18/04/18 16:56, Markus Mayer wrote:
>> From: Jim Quinlan 
>>
>> If the SCMI cpufreq driver is supported, we bail, so that the new
>> approach can be used.
>>
>> Signed-off-by: Jim Quinlan 
>> Signed-off-by: Markus Mayer 
>> ---
>>  drivers/cpufreq/brcmstb-avs-cpufreq.c | 16 
>>  1 file changed, 16 insertions(+)
>>
>> diff --git a/drivers/cpufreq/brcmstb-avs-cpufreq.c 
>> b/drivers/cpufreq/brcmstb-avs-cpufreq.c
>> index b07559b9ed99..b4861a730162 100644
>> --- a/drivers/cpufreq/brcmstb-avs-cpufreq.c
>> +++ b/drivers/cpufreq/brcmstb-avs-cpufreq.c
>> @@ -164,6 +164,8 @@
>>  #define BRCM_AVS_CPU_INTR   "brcm,avs-cpu-l2-intr"
>>  #define BRCM_AVS_HOST_INTR  "sw_intr"
>>  
>> +#define ARM_SCMI_COMPAT "arm,scmi"
>> +
>>  struct pmap {
>>  unsigned int mode;
>>  unsigned int p1;
>> @@ -511,6 +513,20 @@ static int brcm_avs_prepare_init(struct platform_device 
>> *pdev)
>>  struct device *dev;
>>  int host_irq, ret;
>>  
> 
> Will this platform have both SCMI and BRCM_AVS_CPU_DATA nodes enabled ?
> If so, is it not better to just keep only the preferred node enabled
> instead ?

The kernel image has both drivers enabled, the Device Tree blob we pass
contains both nodes, and should flip the status properties based on what
is available. We had some internal discussion about that specific
change, and we ended up having the patch being submitted to seek
external advice, I guess we have an answer now this is not desired.
-- 
Florian

Re: [PATCH 2/2] cpufreq: brcmstb-avs-cpufreq: prefer SCMI cpufreq if supported

2018-04-19 Thread Sudeep Holla 


On 19/04/18 05:16, Viresh Kumar wrote:
> On 18-04-18, 08:56, Markus Mayer wrote:
>> From: Jim Quinlan 
>>
>> If the SCMI cpufreq driver is supported, we bail, so that the new
>> approach can be used.
>>
>> Signed-off-by: Jim Quinlan 
>> Signed-off-by: Markus Mayer 
>> ---
>>  drivers/cpufreq/brcmstb-avs-cpufreq.c | 16 
>>  1 file changed, 16 insertions(+)
>>
>> diff --git a/drivers/cpufreq/brcmstb-avs-cpufreq.c 
>> b/drivers/cpufreq/brcmstb-avs-cpufreq.c
>> index b07559b9ed99..b4861a730162 100644
>> --- a/drivers/cpufreq/brcmstb-avs-cpufreq.c
>> +++ b/drivers/cpufreq/brcmstb-avs-cpufreq.c
>> @@ -164,6 +164,8 @@
>>  #define BRCM_AVS_CPU_INTR   "brcm,avs-cpu-l2-intr"
>>  #define BRCM_AVS_HOST_INTR  "sw_intr"
>>  
>> +#define ARM_SCMI_COMPAT "arm,scmi"
>> +
>>  struct pmap {
>>  unsigned int mode;
>>  unsigned int p1;
>> @@ -511,6 +513,20 @@ static int brcm_avs_prepare_init(struct platform_device 
>> *pdev)
>>  struct device *dev;
>>  int host_irq, ret;
>>  
>> +/*
>> + * If the SCMI cpufreq driver is supported, we bail, so that the more
>> + * modern approach can be used.
>> + */
>> +if (IS_ENABLED(CONFIG_ARM_SCMI_PROTOCOL)) {
>> +struct device_node *np;
>> +
>> +np = of_find_compatible_node(NULL, NULL, ARM_SCMI_COMPAT);
>> +if (np) {
>> +of_node_put(np);
>> +return -ENXIO;
>> +}
>> +}
>> +
> 
> What about adding !CONFIG_ARM_SCMI_PROTOCOL in Kconfig dependency and don't
> compile the driver at all ?
> 

Unfortunately, that may not be good idea with single image needing both
configs to be enabled.

-- 
Regards,
Sudeep

Re: [PATCH 2/2] cpufreq: brcmstb-avs-cpufreq: prefer SCMI cpufreq if supported

2018-04-19 Thread Sudeep Holla 


On 19/04/18 05:16, Viresh Kumar wrote:
> On 18-04-18, 08:56, Markus Mayer wrote:
>> From: Jim Quinlan 
>>
>> If the SCMI cpufreq driver is supported, we bail, so that the new
>> approach can be used.
>>
>> Signed-off-by: Jim Quinlan 
>> Signed-off-by: Markus Mayer 
>> ---
>>  drivers/cpufreq/brcmstb-avs-cpufreq.c | 16 
>>  1 file changed, 16 insertions(+)
>>
>> diff --git a/drivers/cpufreq/brcmstb-avs-cpufreq.c 
>> b/drivers/cpufreq/brcmstb-avs-cpufreq.c
>> index b07559b9ed99..b4861a730162 100644
>> --- a/drivers/cpufreq/brcmstb-avs-cpufreq.c
>> +++ b/drivers/cpufreq/brcmstb-avs-cpufreq.c
>> @@ -164,6 +164,8 @@
>>  #define BRCM_AVS_CPU_INTR   "brcm,avs-cpu-l2-intr"
>>  #define BRCM_AVS_HOST_INTR  "sw_intr"
>>  
>> +#define ARM_SCMI_COMPAT "arm,scmi"
>> +
>>  struct pmap {
>>  unsigned int mode;
>>  unsigned int p1;
>> @@ -511,6 +513,20 @@ static int brcm_avs_prepare_init(struct platform_device 
>> *pdev)
>>  struct device *dev;
>>  int host_irq, ret;
>>  
>> +/*
>> + * If the SCMI cpufreq driver is supported, we bail, so that the more
>> + * modern approach can be used.
>> + */
>> +if (IS_ENABLED(CONFIG_ARM_SCMI_PROTOCOL)) {
>> +struct device_node *np;
>> +
>> +np = of_find_compatible_node(NULL, NULL, ARM_SCMI_COMPAT);
>> +if (np) {
>> +of_node_put(np);
>> +return -ENXIO;
>> +}
>> +}
>> +
> 
> What about adding !CONFIG_ARM_SCMI_PROTOCOL in Kconfig dependency and don't
> compile the driver at all ?
> 

Unfortunately, that may not be good idea with single image needing both
configs to be enabled.

-- 
Regards,
Sudeep

Re: [PATCH 2/2] cpufreq: brcmstb-avs-cpufreq: prefer SCMI cpufreq if supported

2018-04-19 Thread Sudeep Holla 


On 18/04/18 16:56, Markus Mayer wrote:
> From: Jim Quinlan 
> 
> If the SCMI cpufreq driver is supported, we bail, so that the new
> approach can be used.
> 
> Signed-off-by: Jim Quinlan 
> Signed-off-by: Markus Mayer 
> ---
>  drivers/cpufreq/brcmstb-avs-cpufreq.c | 16 
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/cpufreq/brcmstb-avs-cpufreq.c 
> b/drivers/cpufreq/brcmstb-avs-cpufreq.c
> index b07559b9ed99..b4861a730162 100644
> --- a/drivers/cpufreq/brcmstb-avs-cpufreq.c
> +++ b/drivers/cpufreq/brcmstb-avs-cpufreq.c
> @@ -164,6 +164,8 @@
>  #define BRCM_AVS_CPU_INTR"brcm,avs-cpu-l2-intr"
>  #define BRCM_AVS_HOST_INTR   "sw_intr"
>  
> +#define ARM_SCMI_COMPAT  "arm,scmi"
> +
>  struct pmap {
>   unsigned int mode;
>   unsigned int p1;
> @@ -511,6 +513,20 @@ static int brcm_avs_prepare_init(struct platform_device 
> *pdev)
>   struct device *dev;
>   int host_irq, ret;
>  

Will this platform have both SCMI and BRCM_AVS_CPU_DATA nodes enabled ?
If so, is it not better to just keep only the preferred node enabled
instead ?

> + /*
> +  * If the SCMI cpufreq driver is supported, we bail, so that the more
> +  * modern approach can be used.
> +  */
> + if (IS_ENABLED(CONFIG_ARM_SCMI_PROTOCOL)) {
> + struct device_node *np;
> +
> + np = of_find_compatible_node(NULL, NULL, ARM_SCMI_COMPAT);
> + if (np) {
> + of_node_put(np);
> + return -ENXIO;
> + }
> + }
> +

Clearly not a good approach.

-- 
Regards,
Sudeep

Re: [PATCH 2/2] cpufreq: brcmstb-avs-cpufreq: prefer SCMI cpufreq if supported

2018-04-19 Thread Sudeep Holla 


On 18/04/18 16:56, Markus Mayer wrote:
> From: Jim Quinlan 
> 
> If the SCMI cpufreq driver is supported, we bail, so that the new
> approach can be used.
> 
> Signed-off-by: Jim Quinlan 
> Signed-off-by: Markus Mayer 
> ---
>  drivers/cpufreq/brcmstb-avs-cpufreq.c | 16 
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/cpufreq/brcmstb-avs-cpufreq.c 
> b/drivers/cpufreq/brcmstb-avs-cpufreq.c
> index b07559b9ed99..b4861a730162 100644
> --- a/drivers/cpufreq/brcmstb-avs-cpufreq.c
> +++ b/drivers/cpufreq/brcmstb-avs-cpufreq.c
> @@ -164,6 +164,8 @@
>  #define BRCM_AVS_CPU_INTR"brcm,avs-cpu-l2-intr"
>  #define BRCM_AVS_HOST_INTR   "sw_intr"
>  
> +#define ARM_SCMI_COMPAT  "arm,scmi"
> +
>  struct pmap {
>   unsigned int mode;
>   unsigned int p1;
> @@ -511,6 +513,20 @@ static int brcm_avs_prepare_init(struct platform_device 
> *pdev)
>   struct device *dev;
>   int host_irq, ret;
>  

Will this platform have both SCMI and BRCM_AVS_CPU_DATA nodes enabled ?
If so, is it not better to just keep only the preferred node enabled
instead ?

> + /*
> +  * If the SCMI cpufreq driver is supported, we bail, so that the more
> +  * modern approach can be used.
> +  */
> + if (IS_ENABLED(CONFIG_ARM_SCMI_PROTOCOL)) {
> + struct device_node *np;
> +
> + np = of_find_compatible_node(NULL, NULL, ARM_SCMI_COMPAT);
> + if (np) {
> + of_node_put(np);
> + return -ENXIO;
> + }
> + }
> +

Clearly not a good approach.

-- 
Regards,
Sudeep

Re: [PATCH 2/2] cpufreq: brcmstb-avs-cpufreq: prefer SCMI cpufreq if supported

2018-04-18 Thread Viresh Kumar 
On 18-04-18, 08:56, Markus Mayer wrote:
> From: Jim Quinlan 
> 
> If the SCMI cpufreq driver is supported, we bail, so that the new
> approach can be used.
> 
> Signed-off-by: Jim Quinlan 
> Signed-off-by: Markus Mayer 
> ---
>  drivers/cpufreq/brcmstb-avs-cpufreq.c | 16 
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/cpufreq/brcmstb-avs-cpufreq.c 
> b/drivers/cpufreq/brcmstb-avs-cpufreq.c
> index b07559b9ed99..b4861a730162 100644
> --- a/drivers/cpufreq/brcmstb-avs-cpufreq.c
> +++ b/drivers/cpufreq/brcmstb-avs-cpufreq.c
> @@ -164,6 +164,8 @@
>  #define BRCM_AVS_CPU_INTR"brcm,avs-cpu-l2-intr"
>  #define BRCM_AVS_HOST_INTR   "sw_intr"
>  
> +#define ARM_SCMI_COMPAT  "arm,scmi"
> +
>  struct pmap {
>   unsigned int mode;
>   unsigned int p1;
> @@ -511,6 +513,20 @@ static int brcm_avs_prepare_init(struct platform_device 
> *pdev)
>   struct device *dev;
>   int host_irq, ret;
>  
> + /*
> +  * If the SCMI cpufreq driver is supported, we bail, so that the more
> +  * modern approach can be used.
> +  */
> + if (IS_ENABLED(CONFIG_ARM_SCMI_PROTOCOL)) {
> + struct device_node *np;
> +
> + np = of_find_compatible_node(NULL, NULL, ARM_SCMI_COMPAT);
> + if (np) {
> + of_node_put(np);
> + return -ENXIO;
> + }
> + }
> +

What about adding !CONFIG_ARM_SCMI_PROTOCOL in Kconfig dependency and don't
compile the driver at all ?

-- 
viresh

Re: [PATCH 2/2] cpufreq: brcmstb-avs-cpufreq: prefer SCMI cpufreq if supported

2018-04-18 Thread Viresh Kumar 
On 18-04-18, 08:56, Markus Mayer wrote:
> From: Jim Quinlan 
> 
> If the SCMI cpufreq driver is supported, we bail, so that the new
> approach can be used.
> 
> Signed-off-by: Jim Quinlan 
> Signed-off-by: Markus Mayer 
> ---
>  drivers/cpufreq/brcmstb-avs-cpufreq.c | 16 
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/cpufreq/brcmstb-avs-cpufreq.c 
> b/drivers/cpufreq/brcmstb-avs-cpufreq.c
> index b07559b9ed99..b4861a730162 100644
> --- a/drivers/cpufreq/brcmstb-avs-cpufreq.c
> +++ b/drivers/cpufreq/brcmstb-avs-cpufreq.c
> @@ -164,6 +164,8 @@
>  #define BRCM_AVS_CPU_INTR"brcm,avs-cpu-l2-intr"
>  #define BRCM_AVS_HOST_INTR   "sw_intr"
>  
> +#define ARM_SCMI_COMPAT  "arm,scmi"
> +
>  struct pmap {
>   unsigned int mode;
>   unsigned int p1;
> @@ -511,6 +513,20 @@ static int brcm_avs_prepare_init(struct platform_device 
> *pdev)
>   struct device *dev;
>   int host_irq, ret;
>  
> + /*
> +  * If the SCMI cpufreq driver is supported, we bail, so that the more
> +  * modern approach can be used.
> +  */
> + if (IS_ENABLED(CONFIG_ARM_SCMI_PROTOCOL)) {
> + struct device_node *np;
> +
> + np = of_find_compatible_node(NULL, NULL, ARM_SCMI_COMPAT);
> + if (np) {
> + of_node_put(np);
> + return -ENXIO;
> + }
> + }
> +

What about adding !CONFIG_ARM_SCMI_PROTOCOL in Kconfig dependency and don't
compile the driver at all ?

-- 
viresh

Re: [PATCH 2/2] cpufreq: brcmstb-avs-cpufreq: prefer SCMI cpufreq if supported

2018-04-18 Thread Florian Fainelli 
On 04/18/2018 08:56 AM, Markus Mayer wrote:
> From: Jim Quinlan 
> 
> If the SCMI cpufreq driver is supported, we bail, so that the new
> approach can be used.
> 
> Signed-off-by: Jim Quinlan 
> Signed-off-by: Markus Mayer 
> ---
>  drivers/cpufreq/brcmstb-avs-cpufreq.c | 16 
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/cpufreq/brcmstb-avs-cpufreq.c 
> b/drivers/cpufreq/brcmstb-avs-cpufreq.c
> index b07559b9ed99..b4861a730162 100644
> --- a/drivers/cpufreq/brcmstb-avs-cpufreq.c
> +++ b/drivers/cpufreq/brcmstb-avs-cpufreq.c
> @@ -164,6 +164,8 @@
>  #define BRCM_AVS_CPU_INTR"brcm,avs-cpu-l2-intr"
>  #define BRCM_AVS_HOST_INTR   "sw_intr"
>  
> +#define ARM_SCMI_COMPAT  "arm,scmi"
> +
>  struct pmap {
>   unsigned int mode;
>   unsigned int p1;
> @@ -511,6 +513,20 @@ static int brcm_avs_prepare_init(struct platform_device 
> *pdev)
>   struct device *dev;
>   int host_irq, ret;
>  
> + /*
> +  * If the SCMI cpufreq driver is supported, we bail, so that the more
> +  * modern approach can be used.
> +  */
> + if (IS_ENABLED(CONFIG_ARM_SCMI_PROTOCOL)) {
> + struct device_node *np;
> +
> + np = of_find_compatible_node(NULL, NULL, ARM_SCMI_COMPAT);
> + if (np) {
> + of_node_put(np);
> + return -ENXIO;
> + }

We would probably want to make sure that the node is also enabled (that
is, does not have a status = "disabled" property) otherwise the check
can be defeated. Something like:

if (np && of_device_is_available(np))

should be good for that.

Thanks!
-- 
Florian

Re: [PATCH 2/2] cpufreq: brcmstb-avs-cpufreq: prefer SCMI cpufreq if supported

2018-04-18 Thread Florian Fainelli 
On 04/18/2018 08:56 AM, Markus Mayer wrote:
> From: Jim Quinlan 
> 
> If the SCMI cpufreq driver is supported, we bail, so that the new
> approach can be used.
> 
> Signed-off-by: Jim Quinlan 
> Signed-off-by: Markus Mayer 
> ---
>  drivers/cpufreq/brcmstb-avs-cpufreq.c | 16 
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/cpufreq/brcmstb-avs-cpufreq.c 
> b/drivers/cpufreq/brcmstb-avs-cpufreq.c
> index b07559b9ed99..b4861a730162 100644
> --- a/drivers/cpufreq/brcmstb-avs-cpufreq.c
> +++ b/drivers/cpufreq/brcmstb-avs-cpufreq.c
> @@ -164,6 +164,8 @@
>  #define BRCM_AVS_CPU_INTR"brcm,avs-cpu-l2-intr"
>  #define BRCM_AVS_HOST_INTR   "sw_intr"
>  
> +#define ARM_SCMI_COMPAT  "arm,scmi"
> +
>  struct pmap {
>   unsigned int mode;
>   unsigned int p1;
> @@ -511,6 +513,20 @@ static int brcm_avs_prepare_init(struct platform_device 
> *pdev)
>   struct device *dev;
>   int host_irq, ret;
>  
> + /*
> +  * If the SCMI cpufreq driver is supported, we bail, so that the more
> +  * modern approach can be used.
> +  */
> + if (IS_ENABLED(CONFIG_ARM_SCMI_PROTOCOL)) {
> + struct device_node *np;
> +
> + np = of_find_compatible_node(NULL, NULL, ARM_SCMI_COMPAT);
> + if (np) {
> + of_node_put(np);
> + return -ENXIO;
> + }

We would probably want to make sure that the node is also enabled (that
is, does not have a status = "disabled" property) otherwise the check
can be defeated. Something like:

if (np && of_device_is_available(np))

should be good for that.

Thanks!
-- 
Florian

[PATCH 2/2] cpufreq: brcmstb-avs-cpufreq: prefer SCMI cpufreq if supported

2018-04-18 Thread Markus Mayer 
From: Jim Quinlan 

If the SCMI cpufreq driver is supported, we bail, so that the new
approach can be used.

Signed-off-by: Jim Quinlan 
Signed-off-by: Markus Mayer 
---
 drivers/cpufreq/brcmstb-avs-cpufreq.c | 16 
 1 file changed, 16 insertions(+)

diff --git a/drivers/cpufreq/brcmstb-avs-cpufreq.c 
b/drivers/cpufreq/brcmstb-avs-cpufreq.c
index b07559b9ed99..b4861a730162 100644
--- a/drivers/cpufreq/brcmstb-avs-cpufreq.c
+++ b/drivers/cpufreq/brcmstb-avs-cpufreq.c
@@ -164,6 +164,8 @@
 #define BRCM_AVS_CPU_INTR  "brcm,avs-cpu-l2-intr"
 #define BRCM_AVS_HOST_INTR "sw_intr"
 
+#define ARM_SCMI_COMPAT"arm,scmi"
+
 struct pmap {
unsigned int mode;
unsigned int p1;
@@ -511,6 +513,20 @@ static int brcm_avs_prepare_init(struct platform_device 
*pdev)
struct device *dev;
int host_irq, ret;
 
+   /*
+* If the SCMI cpufreq driver is supported, we bail, so that the more
+* modern approach can be used.
+*/
+   if (IS_ENABLED(CONFIG_ARM_SCMI_PROTOCOL)) {
+   struct device_node *np;
+
+   np = of_find_compatible_node(NULL, NULL, ARM_SCMI_COMPAT);
+   if (np) {
+   of_node_put(np);
+   return -ENXIO;
+   }
+   }
+
dev = >dev;
priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
if (!priv)
-- 
2.7.4

[PATCH 2/2] cpufreq: brcmstb-avs-cpufreq: prefer SCMI cpufreq if supported

2018-04-18 Thread Markus Mayer 
From: Jim Quinlan 

If the SCMI cpufreq driver is supported, we bail, so that the new
approach can be used.

Signed-off-by: Jim Quinlan 
Signed-off-by: Markus Mayer 
---
 drivers/cpufreq/brcmstb-avs-cpufreq.c | 16 
 1 file changed, 16 insertions(+)

diff --git a/drivers/cpufreq/brcmstb-avs-cpufreq.c 
b/drivers/cpufreq/brcmstb-avs-cpufreq.c
index b07559b9ed99..b4861a730162 100644
--- a/drivers/cpufreq/brcmstb-avs-cpufreq.c
+++ b/drivers/cpufreq/brcmstb-avs-cpufreq.c
@@ -164,6 +164,8 @@
 #define BRCM_AVS_CPU_INTR  "brcm,avs-cpu-l2-intr"
 #define BRCM_AVS_HOST_INTR "sw_intr"
 
+#define ARM_SCMI_COMPAT"arm,scmi"
+
 struct pmap {
unsigned int mode;
unsigned int p1;
@@ -511,6 +513,20 @@ static int brcm_avs_prepare_init(struct platform_device 
*pdev)
struct device *dev;
int host_irq, ret;
 
+   /*
+* If the SCMI cpufreq driver is supported, we bail, so that the more
+* modern approach can be used.
+*/
+   if (IS_ENABLED(CONFIG_ARM_SCMI_PROTOCOL)) {
+   struct device_node *np;
+
+   np = of_find_compatible_node(NULL, NULL, ARM_SCMI_COMPAT);
+   if (np) {
+   of_node_put(np);
+   return -ENXIO;
+   }
+   }
+
dev = >dev;
priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
if (!priv)
-- 
2.7.4