[PATCH] MIPS: ralink: define stubs for clk_set_parent to fix compile testing

[PATCH] MIPS: ralink: define stubs for clk_set_parent to fix compile testing

[Krzysztof Kozlowski]

The Ralink MIPS platform does not use Common Clock Framework and does
not define certain clock operations leading to compile test failures:

    /usr/bin/mips-linux-gnu-ld: drivers/usb/phy/phy-tegra-usb.o: in function `tegra_usb_phy_init':
    phy-tegra-usb.c:(.text+0x1dd4): undefined reference to `clk_get_parent'

Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
---
 arch/mips/ralink/clk.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/arch/mips/ralink/clk.c b/arch/mips/ralink/clk.c
index 2f9d5acb38ea..8387177a47ef 100644
--- a/arch/mips/ralink/clk.c
+++ b/arch/mips/ralink/clk.c
@@ -70,6 +70,20 @@ long clk_round_rate(struct clk *clk, unsigned long rate)
 }
 EXPORT_SYMBOL_GPL(clk_round_rate);
 
+int clk_set_parent(struct clk *clk, struct clk *parent)
+{
+	WARN_ON(clk);
+	return -1;
+}
+EXPORT_SYMBOL(clk_set_parent);
+
+struct clk *clk_get_parent(struct clk *clk)
+{
+	WARN_ON(clk);
+	return NULL;
+}
+EXPORT_SYMBOL(clk_get_parent);
+
 void __init plat_time_init(void)
 {
 	struct clk *clk;
-- 
2.25.1

Re: [PATCH] MIPS: ralink: define stubs for clk_set_parent to fix compile testing

[John Crispin]

On 16.03.21 18:57, Krzysztof Kozlowski wrote:
> The Ralink MIPS platform does not use Common Clock Framework and does
> not define certain clock operations leading to compile test failures:
>
>      /usr/bin/mips-linux-gnu-ld: drivers/usb/phy/phy-tegra-usb.o: in function `tegra_usb_phy_init':
>      phy-tegra-usb.c:(.text+0x1dd4): undefined reference to `clk_get_parent'
>
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
Acked-by John Crispin <john@phrozen.org>
> ---
>   arch/mips/ralink/clk.c | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
>
> diff --git a/arch/mips/ralink/clk.c b/arch/mips/ralink/clk.c
> index 2f9d5acb38ea..8387177a47ef 100644
> --- a/arch/mips/ralink/clk.c
> +++ b/arch/mips/ralink/clk.c
> @@ -70,6 +70,20 @@ long clk_round_rate(struct clk *clk, unsigned long rate)
>   }
>   EXPORT_SYMBOL_GPL(clk_round_rate);
>   
> +int clk_set_parent(struct clk *clk, struct clk *parent)
> +{
> +	WARN_ON(clk);
> +	return -1;
> +}
> +EXPORT_SYMBOL(clk_set_parent);
> +
> +struct clk *clk_get_parent(struct clk *clk)
> +{
> +	WARN_ON(clk);
> +	return NULL;
> +}
> +EXPORT_SYMBOL(clk_get_parent);
> +
>   void __init plat_time_init(void)
>   {
>   	struct clk *clk;

Re: [PATCH] MIPS: ralink: define stubs for clk_set_parent to fix compile testing

[Thomas Bogendoerfer]

On Tue, Mar 16, 2021 at 06:57:25PM +0100, Krzysztof Kozlowski wrote:
> The Ralink MIPS platform does not use Common Clock Framework and does
> not define certain clock operations leading to compile test failures:
> 
>     /usr/bin/mips-linux-gnu-ld: drivers/usb/phy/phy-tegra-usb.o: in function `tegra_usb_phy_init':
>     phy-tegra-usb.c:(.text+0x1dd4): undefined reference to `clk_get_parent'

hmm, why not make it use common clock framework ? And shouldn't 
include/linux/clk.h provide what you need, if CONFIG_HAVE_CLK is not set ?

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

Re: [PATCH] MIPS: ralink: define stubs for clk_set_parent to fix compile testing

[Dmitry Osipenko]

17.03.2021 00:58, Thomas Bogendoerfer пишет:
> On Tue, Mar 16, 2021 at 06:57:25PM +0100, Krzysztof Kozlowski wrote:
>> The Ralink MIPS platform does not use Common Clock Framework and does
>> not define certain clock operations leading to compile test failures:
>>
>>     /usr/bin/mips-linux-gnu-ld: drivers/usb/phy/phy-tegra-usb.o: in function `tegra_usb_phy_init':
>>     phy-tegra-usb.c:(.text+0x1dd4): undefined reference to `clk_get_parent'
> 
> hmm, why not make it use common clock framework ? And shouldn't 
> include/linux/clk.h provide what you need, if CONFIG_HAVE_CLK is not set ?

That should increase kernel size by a couple kbytes. If size isn't
important, then somebody should dedicate time and energy on creating the
patches.

Re: [PATCH] MIPS: ralink: define stubs for clk_set_parent to fix compile testing

[Krzysztof Kozlowski]

On 16/03/2021 22:58, Thomas Bogendoerfer wrote:
> On Tue, Mar 16, 2021 at 06:57:25PM +0100, Krzysztof Kozlowski wrote:
>> The Ralink MIPS platform does not use Common Clock Framework and does
>> not define certain clock operations leading to compile test failures:
>>
>>     /usr/bin/mips-linux-gnu-ld: drivers/usb/phy/phy-tegra-usb.o: in function `tegra_usb_phy_init':
>>     phy-tegra-usb.c:(.text+0x1dd4): undefined reference to `clk_get_parent'
> 
> hmm, why not make it use common clock framework ? And shouldn't 
> include/linux/clk.h provide what you need, if CONFIG_HAVE_CLK is not set ?

Converting entire Ralink machine to the CCF is quite a task requiring
testing and basic knowledge about this platform. I am just trying to
plug the build failure reported some months ago [1][2]. The CCF does not
provide stubs if platform provides its own clocks.

[1] https://lore.kernel.org/lkml/202102170017.MgPVy7aZ-lkp@intel.com/
[2]
https://lore.kernel.org/lkml/20210316075551.10259-1-krzysztof.kozlowski@canonical.com/

Best regards,
Krzysztof

Re: [PATCH] MIPS: ralink: define stubs for clk_set_parent to fix compile testing

[Dmitry Osipenko]

16.03.2021 20:57, Krzysztof Kozlowski пишет:
> The Ralink MIPS platform does not use Common Clock Framework and does
> not define certain clock operations leading to compile test failures:
> 
>     /usr/bin/mips-linux-gnu-ld: drivers/usb/phy/phy-tegra-usb.o: in function `tegra_usb_phy_init':
>     phy-tegra-usb.c:(.text+0x1dd4): undefined reference to `clk_get_parent'
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
> ---
>  arch/mips/ralink/clk.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/arch/mips/ralink/clk.c b/arch/mips/ralink/clk.c
> index 2f9d5acb38ea..8387177a47ef 100644
> --- a/arch/mips/ralink/clk.c
> +++ b/arch/mips/ralink/clk.c
> @@ -70,6 +70,20 @@ long clk_round_rate(struct clk *clk, unsigned long rate)
>  }
>  EXPORT_SYMBOL_GPL(clk_round_rate);
>  
> +int clk_set_parent(struct clk *clk, struct clk *parent)
> +{
> +	WARN_ON(clk);
> +	return -1;
> +}
> +EXPORT_SYMBOL(clk_set_parent);
> +
> +struct clk *clk_get_parent(struct clk *clk)
> +{
> +	WARN_ON(clk);
> +	return NULL;
> +}
> +EXPORT_SYMBOL(clk_get_parent);

I'm wondering whether symbols should be GPL or it doesn't matter in this
case. Otherwise this looks good to me.

Also, I guess it should be possible to create a generic clk stubs that
will use weak functions, allowing platforms to override only the wanted
stubs and then we won't need to worry about the missing stubs for each
platform individually. But of course that will be a much bigger change.
Just wanted to share my thoughts.

Re: [PATCH] MIPS: ralink: define stubs for clk_set_parent to fix compile testing

[Krzysztof Kozlowski]

On 17/03/2021 01:10, Dmitry Osipenko wrote:
> 16.03.2021 20:57, Krzysztof Kozlowski пишет:
>> The Ralink MIPS platform does not use Common Clock Framework and does
>> not define certain clock operations leading to compile test failures:
>>
>>     /usr/bin/mips-linux-gnu-ld: drivers/usb/phy/phy-tegra-usb.o: in function `tegra_usb_phy_init':
>>     phy-tegra-usb.c:(.text+0x1dd4): undefined reference to `clk_get_parent'
>>
>> Reported-by: kernel test robot <lkp@intel.com>
>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
>> ---
>>  arch/mips/ralink/clk.c | 14 ++++++++++++++
>>  1 file changed, 14 insertions(+)
>>
>> diff --git a/arch/mips/ralink/clk.c b/arch/mips/ralink/clk.c
>> index 2f9d5acb38ea..8387177a47ef 100644
>> --- a/arch/mips/ralink/clk.c
>> +++ b/arch/mips/ralink/clk.c
>> @@ -70,6 +70,20 @@ long clk_round_rate(struct clk *clk, unsigned long rate)
>>  }
>>  EXPORT_SYMBOL_GPL(clk_round_rate);
>>  
>> +int clk_set_parent(struct clk *clk, struct clk *parent)
>> +{
>> +	WARN_ON(clk);
>> +	return -1;
>> +}
>> +EXPORT_SYMBOL(clk_set_parent);
>> +
>> +struct clk *clk_get_parent(struct clk *clk)
>> +{
>> +	WARN_ON(clk);
>> +	return NULL;
>> +}
>> +EXPORT_SYMBOL(clk_get_parent);
> 
> I'm wondering whether symbols should be GPL or it doesn't matter in this
> case. Otherwise this looks good to me.

The ones in arch/mips/ar7/clock.c were not GPL but other stubs already
defined here are, so indeed I'll make them GPL for consistency.

> 
> Also, I guess it should be possible to create a generic clk stubs that
> will use weak functions, allowing platforms to override only the wanted
> stubs and then we won't need to worry about the missing stubs for each
> platform individually. But of course that will be a much bigger change.
> Just wanted to share my thoughts.

Yes, it would be a good idea but also a bigger task. I am not sure if
these platforms are alive enough to get that attention.

Best regards,
Krzysztof

Re: [PATCH] MIPS: ralink: define stubs for clk_set_parent to fix compile testing

[Sergei Shtylyov]

Hello!

On 16.03.2021 20:57, Krzysztof Kozlowski wrote:

> The Ralink MIPS platform does not use Common Clock Framework and does
> not define certain clock operations leading to compile test failures:
> 
>      /usr/bin/mips-linux-gnu-ld: drivers/usb/phy/phy-tegra-usb.o: in function `tegra_usb_phy_init':
>      phy-tegra-usb.c:(.text+0x1dd4): undefined reference to `clk_get_parent'
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
> ---
>   arch/mips/ralink/clk.c | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
> 
> diff --git a/arch/mips/ralink/clk.c b/arch/mips/ralink/clk.c
> index 2f9d5acb38ea..8387177a47ef 100644
> --- a/arch/mips/ralink/clk.c
> +++ b/arch/mips/ralink/clk.c
> @@ -70,6 +70,20 @@ long clk_round_rate(struct clk *clk, unsigned long rate)
>   }
>   EXPORT_SYMBOL_GPL(clk_round_rate);
>   
> +int clk_set_parent(struct clk *clk, struct clk *parent)
> +{
> +	WARN_ON(clk);
> +	return -1;

    Shouldn't this be a proepr error code (-1 corresponds to -EPRERM)?

> +}
> +EXPORT_SYMBOL(clk_set_parent);
> +
> +struct clk *clk_get_parent(struct clk *clk)
> +{
> +	WARN_ON(clk);
> +	return NULL;
> +}
> +EXPORT_SYMBOL(clk_get_parent);
> +
>   void __init plat_time_init(void)
>   {
>   	struct clk *clk;

MBR, Sergei

Re: [PATCH] MIPS: ralink: define stubs for clk_set_parent to fix compile testing

[Krzysztof Kozlowski]

On 17/03/2021 10:52, Sergei Shtylyov wrote:
> Hello!
> 
> On 16.03.2021 20:57, Krzysztof Kozlowski wrote:
> 
>> The Ralink MIPS platform does not use Common Clock Framework and does
>> not define certain clock operations leading to compile test failures:
>>
>>      /usr/bin/mips-linux-gnu-ld: drivers/usb/phy/phy-tegra-usb.o: in function `tegra_usb_phy_init':
>>      phy-tegra-usb.c:(.text+0x1dd4): undefined reference to `clk_get_parent'
>>
>> Reported-by: kernel test robot <lkp@intel.com>
>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
>> ---
>>   arch/mips/ralink/clk.c | 14 ++++++++++++++
>>   1 file changed, 14 insertions(+)
>>
>> diff --git a/arch/mips/ralink/clk.c b/arch/mips/ralink/clk.c
>> index 2f9d5acb38ea..8387177a47ef 100644
>> --- a/arch/mips/ralink/clk.c
>> +++ b/arch/mips/ralink/clk.c
>> @@ -70,6 +70,20 @@ long clk_round_rate(struct clk *clk, unsigned long rate)
>>   }
>>   EXPORT_SYMBOL_GPL(clk_round_rate);
>>   
>> +int clk_set_parent(struct clk *clk, struct clk *parent)
>> +{
>> +	WARN_ON(clk);
>> +	return -1;
> 
>     Shouldn't this be a proepr error code (-1 corresponds to -EPRERM)?

Could be ENODEV or EINVAL but all other stubs here and in ar7/clock.c
use -1. Do you prefer it then to have it inconsistent with others?

Best regards,
Krzysztof

Re: [PATCH] MIPS: ralink: define stubs for clk_set_parent to fix compile testing

[Sergei Shtylyov]

On 17.03.2021 12:56, Krzysztof Kozlowski wrote:

[...]
>>> The Ralink MIPS platform does not use Common Clock Framework and does
>>> not define certain clock operations leading to compile test failures:
>>>
>>>       /usr/bin/mips-linux-gnu-ld: drivers/usb/phy/phy-tegra-usb.o: in function `tegra_usb_phy_init':
>>>       phy-tegra-usb.c:(.text+0x1dd4): undefined reference to `clk_get_parent'
>>>
>>> Reported-by: kernel test robot <lkp@intel.com>
>>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
>>> ---
>>>    arch/mips/ralink/clk.c | 14 ++++++++++++++
>>>    1 file changed, 14 insertions(+)
>>>
>>> diff --git a/arch/mips/ralink/clk.c b/arch/mips/ralink/clk.c
>>> index 2f9d5acb38ea..8387177a47ef 100644
>>> --- a/arch/mips/ralink/clk.c
>>> +++ b/arch/mips/ralink/clk.c
>>> @@ -70,6 +70,20 @@ long clk_round_rate(struct clk *clk, unsigned long rate)
>>>    }
>>>    EXPORT_SYMBOL_GPL(clk_round_rate);
>>>    
>>> +int clk_set_parent(struct clk *clk, struct clk *parent)
>>> +{
>>> +	WARN_ON(clk);
>>> +	return -1;
>>
>>      Shouldn't this be a proepr error code (-1 corresponds to -EPRERM)?
> 
> Could be ENODEV or EINVAL but all other stubs here and in ar7/clock.c
> use -1. Do you prefer it then to have it inconsistent with others?

    No. :-)

> Best regards,
> Krzysztof

MBR, Sergei

Re: [PATCH] MIPS: ralink: define stubs for clk_set_parent to fix compile testing

[Dmitry Osipenko]

17.03.2021 12:56, Krzysztof Kozlowski пишет:
> On 17/03/2021 10:52, Sergei Shtylyov wrote:
>> Hello!
>>
>> On 16.03.2021 20:57, Krzysztof Kozlowski wrote:
>>
>>> The Ralink MIPS platform does not use Common Clock Framework and does
>>> not define certain clock operations leading to compile test failures:
>>>
>>>      /usr/bin/mips-linux-gnu-ld: drivers/usb/phy/phy-tegra-usb.o: in function `tegra_usb_phy_init':
>>>      phy-tegra-usb.c:(.text+0x1dd4): undefined reference to `clk_get_parent'
>>>
>>> Reported-by: kernel test robot <lkp@intel.com>
>>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
>>> ---
>>>   arch/mips/ralink/clk.c | 14 ++++++++++++++
>>>   1 file changed, 14 insertions(+)
>>>
>>> diff --git a/arch/mips/ralink/clk.c b/arch/mips/ralink/clk.c
>>> index 2f9d5acb38ea..8387177a47ef 100644
>>> --- a/arch/mips/ralink/clk.c
>>> +++ b/arch/mips/ralink/clk.c
>>> @@ -70,6 +70,20 @@ long clk_round_rate(struct clk *clk, unsigned long rate)
>>>   }
>>>   EXPORT_SYMBOL_GPL(clk_round_rate);
>>>   
>>> +int clk_set_parent(struct clk *clk, struct clk *parent)
>>> +{
>>> +	WARN_ON(clk);
>>> +	return -1;
>>
>>     Shouldn't this be a proepr error code (-1 corresponds to -EPRERM)?
> 
> Could be ENODEV or EINVAL but all other stubs here and in ar7/clock.c
> use -1. Do you prefer it then to have it inconsistent with others?

I don't see where -1 is used, ar7/clock.c returns 0. Other drivers
either return 0 or EINVAL.

Since linux/clk.h returns 0 in the stub, I think 0 is the correct variant.

Re: [PATCH] MIPS: ralink: define stubs for clk_set_parent to fix compile testing

[Krzysztof Kozlowski]

On Wed, 17 Mar 2021 at 20:37, Dmitry Osipenko <digetx@gmail.com> wrote:
>
> 17.03.2021 12:56, Krzysztof Kozlowski пишет:
> > On 17/03/2021 10:52, Sergei Shtylyov wrote:
> >> Hello!
> >>
> >> On 16.03.2021 20:57, Krzysztof Kozlowski wrote:
> >>
> >>> The Ralink MIPS platform does not use Common Clock Framework and does
> >>> not define certain clock operations leading to compile test failures:
> >>>
> >>>      /usr/bin/mips-linux-gnu-ld: drivers/usb/phy/phy-tegra-usb.o: in function `tegra_usb_phy_init':
> >>>      phy-tegra-usb.c:(.text+0x1dd4): undefined reference to `clk_get_parent'
> >>>
> >>> Reported-by: kernel test robot <lkp@intel.com>
> >>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
> >>> ---
> >>>   arch/mips/ralink/clk.c | 14 ++++++++++++++
> >>>   1 file changed, 14 insertions(+)
> >>>
> >>> diff --git a/arch/mips/ralink/clk.c b/arch/mips/ralink/clk.c
> >>> index 2f9d5acb38ea..8387177a47ef 100644
> >>> --- a/arch/mips/ralink/clk.c
> >>> +++ b/arch/mips/ralink/clk.c
> >>> @@ -70,6 +70,20 @@ long clk_round_rate(struct clk *clk, unsigned long rate)
> >>>   }
> >>>   EXPORT_SYMBOL_GPL(clk_round_rate);
> >>>
> >>> +int clk_set_parent(struct clk *clk, struct clk *parent)
> >>> +{
> >>> +   WARN_ON(clk);
> >>> +   return -1;
> >>
> >>     Shouldn't this be a proepr error code (-1 corresponds to -EPRERM)?
> >
> > Could be ENODEV or EINVAL but all other stubs here and in ar7/clock.c
> > use -1. Do you prefer it then to have it inconsistent with others?
>
> I don't see where -1 is used, ar7/clock.c returns 0. Other drivers
> either return 0 or EINVAL.
>
> Since linux/clk.h returns 0 in the stub, I think 0 is the correct variant.

The ar7 returns 0 but the other stubs in ralink return -1.

Best regards,
Krzysztof