| Message ID | 20210316175725.79981-1-krzysztof.kozlowski@canonical.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series | MIPS: ralink: define stubs for clk_set_parent to fix compile testing | expand |
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;
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.
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.
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.
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
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
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
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
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
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.
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
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;
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(+)