Commit 452503ebc (ARM: Orion: Eth: Add clk/clkdev support.) added use of
the clk driver API which results in compile errors on architectures that
don't implement the clk API.
ERROR: "clk_enable" [drivers/net/ethernet/marvell/mv643xx_eth.ko] undefined!
ERROR: "clk_disable" [drivers/net/ethernet/marvell/mv643xx_eth.ko] undefined!
ERROR: "clk_put" [drivers/net/ethernet/marvell/mv643xx_eth.ko] undefined!
ERROR: "clk_get_rate" [drivers/net/ethernet/marvell/mv643xx_eth.ko] undefined!
ERROR: "clk_get" [drivers/net/ethernet/marvell/mv643xx_eth.ko] undefined!
Selecting CLKDEV_LOOKUP doesn't fix this either, as the build then fails with:
In file included from drivers/clk/clkdev.c:21:0:
include/linux/clkdev.h:15:24: fatal error: asm/clkdev.h: No such file or directory
So we just prevent this from building at all on PPC32.
Signed-off-by: Josh Boyer <redacted>
---
drivers/net/ethernet/marvell/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Ben Hutchings <hidden> Date: 2012-06-05 23:30:19
On Tue, 2012-06-05 at 15:28 -0400, Josh Boyer wrote:
Commit 452503ebc (ARM: Orion: Eth: Add clk/clkdev support.) added use of
the clk driver API which results in compile errors on architectures that
don't implement the clk API.
ERROR: "clk_enable" [drivers/net/ethernet/marvell/mv643xx_eth.ko] undefined!
ERROR: "clk_disable" [drivers/net/ethernet/marvell/mv643xx_eth.ko] undefined!
ERROR: "clk_put" [drivers/net/ethernet/marvell/mv643xx_eth.ko] undefined!
ERROR: "clk_get_rate" [drivers/net/ethernet/marvell/mv643xx_eth.ko] undefined!
ERROR: "clk_get" [drivers/net/ethernet/marvell/mv643xx_eth.ko] undefined!
Selecting CLKDEV_LOOKUP doesn't fix this either, as the build then fails with:
In file included from drivers/clk/clkdev.c:21:0:
include/linux/clkdev.h:15:24: fatal error: asm/clkdev.h: No such file or directory
So we just prevent this from building at all on PPC32.
[...]
This dependency was introduced by:
commit 16b817579fb61050f1abcc0e81089974328a9c27
Author: Benjamin Herrenschmidt [off-list ref]
Date: Sat Apr 16 15:24:30 2005 -0700
[PATCH] ppc32: MV643XX ethernet is an option for Pegasos
commit 06ede91017d015a03cf8c1c87b3ff668f9a846e0
Author: Dale Farnsworth [off-list ref]
Date: Wed Sep 20 12:24:34 2006 -0700
[PATCH] mv643xx_eth: restrict to 32-bit PPC_MULTIPLATFORM
If Pegasos is still supposed to be supported then this needs to be fixed
properly.
Ben.
--
Ben Hutchings
I haven't lost my mind; it's backed up on tape somewhere.
On Tue, Jun 05, 2012 at 03:28:21PM -0400, Josh Boyer wrote:
Commit 452503ebc (ARM: Orion: Eth: Add clk/clkdev support.) added use of
the clk driver API which results in compile errors on architectures that
don't implement the clk API.
ERROR: "clk_enable" [drivers/net/ethernet/marvell/mv643xx_eth.ko] undefined!
ERROR: "clk_disable" [drivers/net/ethernet/marvell/mv643xx_eth.ko] undefined!
ERROR: "clk_put" [drivers/net/ethernet/marvell/mv643xx_eth.ko] undefined!
ERROR: "clk_get_rate" [drivers/net/ethernet/marvell/mv643xx_eth.ko] undefined!
ERROR: "clk_get" [drivers/net/ethernet/marvell/mv643xx_eth.ko] undefined!
Selecting CLKDEV_LOOKUP doesn't fix this either, as the build then fails with:
In file included from drivers/clk/clkdev.c:21:0:
include/linux/clkdev.h:15:24: fatal error: asm/clkdev.h: No such file or directory
So we just prevent this from building at all on PPC32.
If the PPC32 dependency is no longer relevant (e.g. if Pegasos platform
support was removed from the kernel), then the commit message should
mention that -- the above reasoning is a poor sole justification for
this change.
On Wed, Jun 06, 2012 at 12:30:05AM +0100, Ben Hutchings wrote:
On Tue, 2012-06-05 at 15:28 -0400, Josh Boyer wrote:
quoted
Commit 452503ebc (ARM: Orion: Eth: Add clk/clkdev support.) added use of
the clk driver API which results in compile errors on architectures that
don't implement the clk API.
ERROR: "clk_enable" [drivers/net/ethernet/marvell/mv643xx_eth.ko] undefined!
ERROR: "clk_disable" [drivers/net/ethernet/marvell/mv643xx_eth.ko] undefined!
ERROR: "clk_put" [drivers/net/ethernet/marvell/mv643xx_eth.ko] undefined!
ERROR: "clk_get_rate" [drivers/net/ethernet/marvell/mv643xx_eth.ko] undefined!
ERROR: "clk_get" [drivers/net/ethernet/marvell/mv643xx_eth.ko] undefined!
Selecting CLKDEV_LOOKUP doesn't fix this either, as the build then fails with:
In file included from drivers/clk/clkdev.c:21:0:
include/linux/clkdev.h:15:24: fatal error: asm/clkdev.h: No such file or directory
So we just prevent this from building at all on PPC32.
[...]
This dependency was introduced by:
commit 16b817579fb61050f1abcc0e81089974328a9c27
Author: Benjamin Herrenschmidt [off-list ref]
Date: Sat Apr 16 15:24:30 2005 -0700
[PATCH] ppc32: MV643XX ethernet is an option for Pegasos
commit 06ede91017d015a03cf8c1c87b3ff668f9a846e0
Author: Dale Farnsworth [off-list ref]
Date: Wed Sep 20 12:24:34 2006 -0700
[PATCH] mv643xx_eth: restrict to 32-bit PPC_MULTIPLATFORM
If Pegasos is still supposed to be supported then this needs to be fixed
properly.
The proper fix, from my minimal looking, was one of:
1) revert the change for ARM that introduced th clk stuff
2) do a similar change as the original commit but with a bunch of
#ifdef-ery
3) implement the clkdev API stuff for 32-bit ppc
Honestly, I'd go for either 1 or 2. The commit that introduced it was
broken to begin with, but that isn't my call.
josh
On Wed, Jun 06, 2012 at 02:49:07AM +0200, Lennert Buytenhek wrote:
On Tue, Jun 05, 2012 at 03:28:21PM -0400, Josh Boyer wrote:
quoted
Commit 452503ebc (ARM: Orion: Eth: Add clk/clkdev support.) added use of
the clk driver API which results in compile errors on architectures that
don't implement the clk API.
ERROR: "clk_enable" [drivers/net/ethernet/marvell/mv643xx_eth.ko] undefined!
ERROR: "clk_disable" [drivers/net/ethernet/marvell/mv643xx_eth.ko] undefined!
ERROR: "clk_put" [drivers/net/ethernet/marvell/mv643xx_eth.ko] undefined!
ERROR: "clk_get_rate" [drivers/net/ethernet/marvell/mv643xx_eth.ko] undefined!
ERROR: "clk_get" [drivers/net/ethernet/marvell/mv643xx_eth.ko] undefined!
Selecting CLKDEV_LOOKUP doesn't fix this either, as the build then fails with:
In file included from drivers/clk/clkdev.c:21:0:
include/linux/clkdev.h:15:24: fatal error: asm/clkdev.h: No such file or directory
So we just prevent this from building at all on PPC32.
If the PPC32 dependency is no longer relevant (e.g. if Pegasos platform
support was removed from the kernel), then the commit message should
mention that -- the above reasoning is a poor sole justification for
this change.
You are correct. As it stands, it's no better than just breaking it
outright with commit 452503ebc. I've described the 3 possible solutions
in my other reply.
josh
On Tue, Jun 05, 2012 at 10:40:14PM -0400, Josh Boyer wrote:
quoted
quoted
Commit 452503ebc (ARM: Orion: Eth: Add clk/clkdev support.) added use of
the clk driver API which results in compile errors on architectures that
don't implement the clk API.
ERROR: "clk_enable" [drivers/net/ethernet/marvell/mv643xx_eth.ko] undefined!
ERROR: "clk_disable" [drivers/net/ethernet/marvell/mv643xx_eth.ko] undefined!
ERROR: "clk_put" [drivers/net/ethernet/marvell/mv643xx_eth.ko] undefined!
ERROR: "clk_get_rate" [drivers/net/ethernet/marvell/mv643xx_eth.ko] undefined!
ERROR: "clk_get" [drivers/net/ethernet/marvell/mv643xx_eth.ko] undefined!
Selecting CLKDEV_LOOKUP doesn't fix this either, as the build then fails with:
In file included from drivers/clk/clkdev.c:21:0:
include/linux/clkdev.h:15:24: fatal error: asm/clkdev.h: No such file or directory
So we just prevent this from building at all on PPC32.
If the PPC32 dependency is no longer relevant (e.g. if Pegasos platform
support was removed from the kernel), then the commit message should
mention that -- the above reasoning is a poor sole justification for
this change.
You are correct. As it stands, it's no better than just breaking it
outright with commit 452503ebc.
ACK. If I'd have seen that commit ("ARM: Orion: Eth: Add clk/clkdev
support.") come by I would have said something about it, but noone
bothered to CC me on it -- and it doesn't seem that it was CCed to
netdev@ either...?
From: Andrew Lunn <andrew@lunn.ch> Date: 2012-06-06 05:29:22
The proper fix, from my minimal looking, was one of:
1) revert the change for ARM that introduced th clk stuff
2) do a similar change as the original commit but with a bunch of
#ifdef-ery
3) implement the clkdev API stuff for 32-bit ppc
Honestly, I'd go for either 1 or 2. The commit that introduced it was
broken to begin with, but that isn't my call.
I broke it. Sorry.
At the time, there was a push to remove all the #ifdefs. The following
patchset was doing this:
https://lkml.org/lkml/2012/4/21/94
it would provide dummy implementations for those systems without clk
support. However, it seems that patch set never made it in, and i did
not declare my dependency on it.
I'm happy to add #ifdef. However, i would first like to understand
what was 'broken to begin with'.
Thanks
Andrew
On Wed, Jun 06, 2012 at 07:29:10AM +0200, Andrew Lunn wrote:
quoted
The proper fix, from my minimal looking, was one of:
1) revert the change for ARM that introduced th clk stuff
2) do a similar change as the original commit but with a bunch of
#ifdef-ery
3) implement the clkdev API stuff for 32-bit ppc
Honestly, I'd go for either 1 or 2. The commit that introduced it was
broken to begin with, but that isn't my call.
I broke it. Sorry.
At the time, there was a push to remove all the #ifdefs. The following
patchset was doing this:
https://lkml.org/lkml/2012/4/21/94
it would provide dummy implementations for those systems without clk
support. However, it seems that patch set never made it in, and i did
not declare my dependency on it.
I'm happy to add #ifdef. However, i would first like to understand
what was 'broken to begin with'.
Simply that a commit was introduced that did not build on all the
existing platforms the driver supports. The world is not ARM, or x86,
or PPC32, etc. I haven't looked to see if it would still function
correctly in the presence of a dummy clk implementation, but if not that
would also be bad.
josh
From: Mark Brown <hidden> Date: 2012-06-07 23:51:21
On Tue, Jun 05, 2012 at 10:38:42PM -0400, Josh Boyer wrote:
1) revert the change for ARM that introduced th clk stuff
2) do a similar change as the original commit but with a bunch of
#ifdef-ery
3) implement the clkdev API stuff for 32-bit ppc
Honestly, I'd go for either 1 or 2. The commit that introduced it was
broken to begin with, but that isn't my call.
There's a change going in which stubs out the clock API when not used
which should resolve the immediate issue, though really the best thing
here is just to enable use of the generic clock API if the platform
doesn't have one of its own - it's not just platforms that need clocks
so we really want to get that rolled out as widely as possible.
This sort of issue is just the tip of the iceberg in terms of what it's
useful to do with the API.
On Fri, Jun 08, 2012 at 12:51:15AM +0100, Mark Brown wrote:
On Tue, Jun 05, 2012 at 10:38:42PM -0400, Josh Boyer wrote:
quoted
1) revert the change for ARM that introduced th clk stuff
2) do a similar change as the original commit but with a bunch of
#ifdef-ery
3) implement the clkdev API stuff for 32-bit ppc
quoted
Honestly, I'd go for either 1 or 2. The commit that introduced it was
broken to begin with, but that isn't my call.
There's a change going in which stubs out the clock API when not used
which should resolve the immediate issue, though really the best thing
here is just to enable use of the generic clock API if the platform
doesn't have one of its own - it's not just platforms that need clocks
so we really want to get that rolled out as widely as possible.
Sounds great. I have no objections with any of those plans.
This sort of issue is just the tip of the iceberg in terms of what it's
useful to do with the API.
Yes, sounds like it. All I ask is that people test their patches along
the way so things don't get broken. I mean, it's great we have an
iceberg but I don't want tons of drivers on other architectures running
into the thing and sinking because people aren't being careful. Except
maybe the one already appropriately (nick)named.
josh
From: Mark Brown <hidden> Date: 2012-06-08 00:34:48
On Thu, Jun 07, 2012 at 07:55:51PM -0400, Josh Boyer wrote:
On Fri, Jun 08, 2012 at 12:51:15AM +0100, Mark Brown wrote:
quoted
This sort of issue is just the tip of the iceberg in terms of what it's
useful to do with the API.
Yes, sounds like it. All I ask is that people test their patches along
the way so things don't get broken. I mean, it's great we have an
iceberg but I don't want tons of drivers on other architectures running
into the thing and sinking because people aren't being careful. Except
maybe the one already appropriately (nick)named.
It's really hard to blame the submitters here - this really isn't the
sort of API that you'd expect to only be available conditionally so this
isn't something that one would expect to have to worry about. It's a
product of the age of the clock API and the glacial progress on the
generic clock API.
On Fri, Jun 08, 2012 at 01:34:45AM +0100, Mark Brown wrote:
On Thu, Jun 07, 2012 at 07:55:51PM -0400, Josh Boyer wrote:
quoted
On Fri, Jun 08, 2012 at 12:51:15AM +0100, Mark Brown wrote:
quoted
quoted
This sort of issue is just the tip of the iceberg in terms of what it's
useful to do with the API.
quoted
Yes, sounds like it. All I ask is that people test their patches along
the way so things don't get broken. I mean, it's great we have an
iceberg but I don't want tons of drivers on other architectures running
into the thing and sinking because people aren't being careful. Except
maybe the one already appropriately (nick)named.
It's really hard to blame the submitters here - this really isn't the
sort of API that you'd expect to only be available conditionally so this
isn't something that one would expect to have to worry about. It's a
product of the age of the clock API and the glacial progress on the
generic clock API.
I'm not placing blame. I'm declaring people should be cautious going
forward. 5 arches have the clock API. 21 don't. Whatever reasons
there are for that, I don't care. It should be a big warning sign.
It might even be beneficial to put some Kconfig dependencies on both
CONFIG_COMMON_CLK (which is somewhat misleadingly named) and
CONFIG_CLKDEV_LOOKUP so those are only selectable on those 5 arches.
Something like:
Regardless, hopefully things like this will get hit in linux-next in the
future. I believe the only reason that it wasn't this time is that
none of the PPC defconfigs build in linux-next bother to build the
driver at all.
josh
From: Mark Brown <hidden> Date: 2012-06-09 04:01:22
On Thu, Jun 07, 2012 at 09:04:03PM -0400, Josh Boyer wrote:
I'm not placing blame. I'm declaring people should be cautious going
forward. 5 arches have the clock API. 21 don't. Whatever reasons
there are for that, I don't care. It should be a big warning sign.
My point here is that it's a warning sign for the API, not really for
the drivers that use it.
It might even be beneficial to put some Kconfig dependencies on both
CONFIG_COMMON_CLK (which is somewhat misleadingly named) and
CONFIG_CLKDEV_LOOKUP so those are only selectable on those 5 arches.
Something like:
This is a really bad approach. It's sending totally the wrong message
about where we want to be (we want to have the clock API available
everywhere) and more importantly it still means that drivers need to go
on carrying around ifdefery or unhelpful dependencies which is just lots
of pointless work. A very large proportion of the drivers that use
clocks are just making sure clocks are enabled when the device is active
to integrate with system wide power optimisation and don't actually care
if there are clocks there at all, we should be making their life as easy
as possible.
A much better approach is get the stubs mentioned earlier merged
(they're already on their way) faster. That way there are no compile
time dependencies and the problem goes away unless the driver is doing
something more active with clocks like managing the clock rate.
In the case of CLKDEV_LOOKUP the symbol should only be selected by an
architecture anyway, it's a layer on top of the architecture clock code.
Regardless, hopefully things like this will get hit in linux-next in the
future. I believe the only reason that it wasn't this time is that
none of the PPC defconfigs build in linux-next bother to build the
driver at all.
They do generally, people do randconfig and allXconfig builds all the
time.