* Re: [PATCH] phylib: make mdio-gpio work without OF (v2)
From: David Brownell @ 2008-11-05 0:04 UTC (permalink / raw)
To: Grant Likely
Cc: Paulius Zaleckas, netdev, linux-arm-kernel, linux-embedded,
Laurent Pinchart, Mike Frysinger
In-Reply-To: <fa686aa40811040824x52dab7fel9f76567cb6d49c8@mail.gmail.com>
On Tuesday 04 November 2008, Grant Likely wrote:
> At this point, instead of #ifdeffing the function signature, I would
> much rather see this generalized as something like
> 'mdio_gpio_setup()'. Then move the OF and non-OF specific bits into
> two new functions; mdio_ofgpio_probe() and mdio_gpio_probe(). The two
> new functions should be placed with the appropriate bus binding in the
> #ifdef/#else block at the bottom of the file.
Or if possible something that creates a single #ifdef that
ensures dead code elimination will remove the "other" branch,
but ensures all platforms will build both versions:
#ifdef OF
#define using_of true
#else
#define using_of false
#endif
...
static int __init mdio_gpio_init(void)
{
if (using_of)
return register mdio_ofgpio driver;
else
return register mdio_gpio driver;
}
subsys_initcall(mdio_gpio_init);
...
That's generally the preferred way to handle #ifdeffery.
But I could imagine OF isn't (yet?) set up to handle it.
- Dave
^ permalink raw reply
* Re: [PATCH] phylib: make mdio-gpio work without OF (v2)
From: Mike Frysinger @ 2008-11-05 0:09 UTC (permalink / raw)
To: David Brownell
Cc: Grant Likely, Paulius Zaleckas, netdev, linux-arm-kernel,
linux-embedded, Laurent Pinchart
In-Reply-To: <200811041604.22947.david-b@pacbell.net>
On Tue, Nov 4, 2008 at 19:04, David Brownell wrote:
> On Tuesday 04 November 2008, Grant Likely wrote:
>> At this point, instead of #ifdeffing the function signature, I would
>> much rather see this generalized as something like
>> 'mdio_gpio_setup()'. Then move the OF and non-OF specific bits into
>> two new functions; mdio_ofgpio_probe() and mdio_gpio_probe(). The two
>> new functions should be placed with the appropriate bus binding in the
>> #ifdef/#else block at the bottom of the file.
>
> Or if possible something that creates a single #ifdef that
> ensures dead code elimination will remove the "other" branch,
> but ensures all platforms will build both versions:
>
> #ifdef OF
> #define using_of true
> #else
> #define using_of false
> #endif
>
> ...
>
> static int __init mdio_gpio_init(void)
> {
> if (using_of)
> return register mdio_ofgpio driver;
> else
> return register mdio_gpio driver;
> }
> subsys_initcall(mdio_gpio_init);
>
> ...
>
> That's generally the preferred way to handle #ifdeffery.
> But I could imagine OF isn't (yet?) set up to handle it.
i agree completely with the inclination to do it all in C as you've
suggested and let the compiler do dead code elimination, but that only
works if the functions in question are defined everywhere (in other
words, there's a linux/ api for it). i dont think that's the case for
OF (which is what you were implying?) ...
-mike
^ permalink raw reply
* Re: [PATCH 1/6] [PWM] Generic PWM API implementation
From: Mike Frysinger @ 2008-11-05 0:17 UTC (permalink / raw)
To: David Brownell; +Cc: Bill Gatliff, linux-embedded
In-Reply-To: <200811041555.18234.david-b@pacbell.net>
On Tue, Nov 4, 2008 at 18:55, David Brownell wrote:
> On Tuesday 04 November 2008, Bill Gatliff wrote:
>> So, what's the next step? How do I advocate for getting
>> this API into mainline?
>
> The number of backing implementations seemed a bit low ...
> just one. And that's switching Atmel's dedicated PWM, but
> not the other PWM driver in the tree (for PXA); so it's
> not terrifically accessible.
>
> My rule of thumb is that it's not worth considering a
> framework as being general enough until it's got three
> fairly different backing implementations. Two is enough
> to consider merging, especially with complicated drivers.
every Blackfin processor so far has had dedicated PWM hardware in it,
so a backend driver for that arch would show up ...
> Plus the number of framework users is an issue. The
> PWM led driver is a good demo for two basic features,
> and it's useful: brightness (duty cycle beyond visual
> perception) and blink rate (duty cycle easily seen with
> the eye). But it's still only really one API customer,
> even given that new LED trigger...
there is an lcd driver or two in the tree (probably Blackfin specific)
that uses the PWM hardware as an output to drive the hsync/vsync
signals.
and while LIRC is still out-of-tree for mainline, i wrote a driver
using the PWM hardware as an input ... but there wasnt anything else
Blackfin specific in said driver other than the PWM stuff ...
i'm guessing you'd prefer to see those implemented before moving to
mainline though ?
-mike
^ permalink raw reply
* Re: [PATCH 1/6] [PWM] Generic PWM API implementation
From: Bill Gatliff @ 2008-11-05 2:56 UTC (permalink / raw)
To: David Brownell, linux-embedded
In-Reply-To: <200811041555.18234.david-b@pacbell.net>
David Brownell wrote:
>
> Patches #1 and #2 seem to be in the wrong order ... and
> #2 shouldn't give the header's full pathname. I'd have
> merged the two, myself.
Ok, that's good feedback.
> Patch #6 seems to remove LEDS_ATMEL_PWM from Kconfig but
> leave the driver around ... and restore the now-deleted
> LEDS_CORGI support (leds-gpio suffices). If you're going
> to remove LEDS_ATMEL_PWM you should update the code in
> mach-at91/leds.c which sets up for that driver...
I'll look into this before my next update.
> The number of backing implementations seemed a bit low ...
> just one. And that's switching Atmel's dedicated PWM, but
> not the other PWM driver in the tree (for PXA); so it's
> not terrifically accessible.
>
> And even for Atmel's SOC's it seemed a bit light: the TC
> blocks will do PWM quite happily, and are found on many
> more chips than the dedicated PWM hardware.
Alright. The SAM9263 has both PWMC and TC, so it's pretty easy to add TC's to
the list of backing implementations because that hardware is already plugged in.
:) And I have PXA hardware here somewhere too, so I'll bring that one over as
well. Finally, I suddenly have the need to do bit-bang PWM over GPIO, so if
nobody beats me to it (please, someone beat me to it!) then I'll implement that
one as well.
> Plus the number of framework users is an issue. The
> PWM led driver is a good demo for two basic features,
> and it's useful: brightness (duty cycle beyond visual
> perception) and blink rate (duty cycle easily seen with
> the eye). But it's still only really one API customer,
> even given that new LED trigger...
That's a bit of a catch-22. The whole point of the API is to provide a
framework for users to adopt, where previously there was none. Adopters are
more likely if the framework is mainlined.
Granted, having a git tree on kernel.org is pretty darned close. I'll look into
that per Mike Frysinger.
> So I'd suggest fleshing it out a bit more, so it works
> on some non-Atmel hardware as well as more Atmel SOCs.
> And fix the goofs I noted above. :)
Roger that. And then I expect an Acked-by from you. :)
b.g.
--
Bill Gatliff
bgat@billgatliff.com
^ permalink raw reply
* Re: [PATCH 1/6] [PWM] Generic PWM API implementation
From: Bill Gatliff @ 2008-11-05 2:59 UTC (permalink / raw)
To: Mike Frysinger; +Cc: David Brownell, linux-embedded
In-Reply-To: <8bd0f97a0811041617k6761866bn20e0c8b6e415cd9a@mail.gmail.com>
Mike Frysinger wrote:
> every Blackfin processor so far has had dedicated PWM hardware in it,
> so a backend driver for that arch would show up ...
From who? I don't have that hardware. I'm happy to hear that someone is
working on it, though, and I would be happy to help review.
> i'm guessing you'd prefer to see those implemented before moving to
> mainline though ?
Sounds like if we can get a git tree on kernel.org, then the first stop would be
my pulling the code into the tree, and then asking Linus to do a pull.
To which is response will probably be: "Bill who?" :)
b.g.
--
Bill Gatliff
bgat@billgatliff.com
^ permalink raw reply
* Re: [PATCH] phylib: make mdio-gpio work without OF (v2)
From: David Brownell @ 2008-11-05 4:45 UTC (permalink / raw)
To: Mike Frysinger
Cc: Grant Likely, Paulius Zaleckas, netdev, linux-arm-kernel,
linux-embedded, Laurent Pinchart
In-Reply-To: <8bd0f97a0811041609l3bd474afh7e526be368d4c5cf@mail.gmail.com>
On Tuesday 04 November 2008, Mike Frysinger wrote:
> > That's generally the preferred way to handle #ifdeffery.
> > But I could imagine OF isn't (yet?) set up to handle it.
>
> i agree completely with the inclination to do it all in C as you've
> suggested and let the compiler do dead code elimination, but that only
> works if the functions in question are defined everywhere (in other
> words, there's a linux/ api for it). i dont think that's the case for
> OF (which is what you were implying?) ...
Only works if the functions are "declared" everywhere;
the stuff that's included in header files.
A "definition" would be available at link time (except
for inlined functions, in headers). Actual C functions.
If indeed OF functions don't have declarations which are
available on all platforms, that might be worth fixing
to enable this approach to #ifdef elimination.
- Dave
^ permalink raw reply
* Re: [PATCH 1/6] [PWM] Generic PWM API implementation
From: David Brownell @ 2008-11-05 5:08 UTC (permalink / raw)
To: Mike Frysinger; +Cc: Bill Gatliff, linux-embedded
In-Reply-To: <8bd0f97a0811041617k6761866bn20e0c8b6e415cd9a@mail.gmail.com>
On Tuesday 04 November 2008, Mike Frysinger wrote:
> On Tue, Nov 4, 2008 at 18:55, David Brownell wrote:
> > On Tuesday 04 November 2008, Bill Gatliff wrote:
> >> So, what's the next step? How do I advocate for getting
> >> this API into mainline?
> >
> > The number of backing implementations seemed a bit low ...
> > just one. And that's switching Atmel's dedicated PWM, but
> > not the other PWM driver in the tree (for PXA); so it's
> > not terrifically accessible.
> >
> > My rule of thumb is that it's not worth considering a
> > framework as being general enough until it's got three
> > fairly different backing implementations. Two is enough
> > to consider merging, especially with complicated drivers.
>
> every Blackfin processor so far has had dedicated PWM hardware in it,
> so a backend driver for that arch would show up ...
I'm not questioning the notion that more generic PWM support
would be useful, note! Just pointing out my rule of thumb,
which has proven to be a good one.
A variety of implementations shakes out portability issues,
and highlights (what *WE* already know!) that this stuff is
actually generally useful. And when there's no test harness,
having most of a 3-providers vs 3-users matrix known to work
is a good stand-in.
> > Plus the number of framework users is an issue. ...
>
> there is an lcd driver or two in the tree (probably Blackfin specific)
> that uses the PWM hardware as an output to drive the hsync/vsync
> signals.
... Right, but not through *this* framework. The proof that
the framework is sufficient for that application too would be
the code. And hsync/vsync seems like a usefully "different" app
context to me. Certainly not one that came quickly to my mind!
But as we know, PWM gets used in all kinds of ways. :)
> and while LIRC is still out-of-tree for mainline, i wrote a driver
> using the PWM hardware as an input ... but there wasnt anything else
> Blackfin specific in said driver other than the PWM stuff ...
>
> i'm guessing you'd prefer to see those implemented before moving to
> mainline though ?
Well, more than just Bill's single implementation and client, yes.
Maybe the Blackfin PWM provider, and one more API client ... though
more of each would be fine too.
I'd imagine Bill would be glad to have more active hands on the
code too ... a few issues always seem to turn up with the first
users, and it's better to sort them out with "early adopters"
before the floodgates open.
Plus: more users provides some evidence to folk like Andrew and
Linus that the code is useful enough to merge. Embedded Linux
isn't as visible to them as x86.
- Dave
^ permalink raw reply
* Re: [PATCH] phylib: make mdio-gpio work without OF (v2)
From: Mike Frysinger @ 2008-11-05 5:34 UTC (permalink / raw)
To: David Brownell
Cc: Grant Likely, Paulius Zaleckas, netdev, linux-arm-kernel,
linux-embedded, Laurent Pinchart
In-Reply-To: <200811042045.41052.david-b@pacbell.net>
On Tue, Nov 4, 2008 at 23:45, David Brownell wrote:
> On Tuesday 04 November 2008, Mike Frysinger wrote:
>> > That's generally the preferred way to handle #ifdeffery.
>> > But I could imagine OF isn't (yet?) set up to handle it.
>>
>> i agree completely with the inclination to do it all in C as you've
>> suggested and let the compiler do dead code elimination, but that only
>> works if the functions in question are defined everywhere (in other
>> words, there's a linux/ api for it). i dont think that's the case for
>> OF (which is what you were implying?) ...
>
> Only works if the functions are "declared" everywhere;
> the stuff that's included in header files.
>
> A "definition" would be available at link time (except
> for inlined functions, in headers). Actual C functions.
>
> If indeed OF functions don't have declarations which are
> available on all platforms, that might be worth fixing
> to enable this approach to #ifdef elimination.
OF is open firmware right ? no way there's going to be OF for every
port that supports GPIO, so filling out the stubs in linux/of_gpio.h
will need to be done.
-mike
^ permalink raw reply
* Re: [PATCH] phylib: make mdio-gpio work without OF (v2)
From: David Brownell @ 2008-11-05 8:11 UTC (permalink / raw)
To: Mike Frysinger
Cc: Grant Likely, Paulius Zaleckas, netdev, linux-arm-kernel,
linux-embedded, Laurent Pinchart
In-Reply-To: <8bd0f97a0811042134n3c5543bbwed7a8ebfd0e3f77e@mail.gmail.com>
On Tuesday 04 November 2008, Mike Frysinger wrote:
> > If indeed OF functions don't have declarations which are
> > available on all platforms, that might be worth fixing
> > to enable this approach to #ifdef elimination.
>
> OF is open firmware right ? no way there's going to be OF for every
> port that supports GPIO, so filling out the stubs in linux/of_gpio.h
> will need to be done.
In which case the boolean "we don't have OF" would be "false" and
any code referencing the declared functions would be compiled out
(being dead code/data). So there would be no link-time references
to any OF routines... standard technique.
^ permalink raw reply
* Re: [PATCH] phylib: make mdio-gpio work without OF (v2)
From: Mike Frysinger @ 2008-11-05 8:32 UTC (permalink / raw)
To: David Brownell
Cc: Grant Likely, Paulius Zaleckas, netdev, linux-arm-kernel,
linux-embedded, Laurent Pinchart
In-Reply-To: <200811050011.56804.david-b@pacbell.net>
On Wed, Nov 5, 2008 at 03:11, David Brownell wrote:
> On Tuesday 04 November 2008, Mike Frysinger wrote:
>> > If indeed OF functions don't have declarations which are
>> > available on all platforms, that might be worth fixing
>> > to enable this approach to #ifdef elimination.
>>
>> OF is open firmware right ? no way there's going to be OF for every
>> port that supports GPIO, so filling out the stubs in linux/of_gpio.h
>> will need to be done.
>
> In which case the boolean "we don't have OF" would be "false" and
> any code referencing the declared functions would be compiled out
> (being dead code/data). So there would be no link-time references
> to any OF routines... standard technique.
i understand the compiler side just fine. my point is that the
current linux/of_gpio.h only defines the OF structure and related
prototypes when CONFIG_OF_GPIO is defined. so it needs updating
first.
-mike
^ permalink raw reply
* Re: [PATCH] phylib: make mdio-gpio work without OF (v2)
From: Marc Pignat @ 2008-11-05 8:57 UTC (permalink / raw)
To: Mike Frysinger
Cc: David Brownell, Grant Likely, Paulius Zaleckas, netdev,
linux-arm-kernel, linux-embedded, Laurent Pinchart
In-Reply-To: <8bd0f97a0811050032i206214a6r8d30a58f354995b4@mail.gmail.com>
On Wednesday 05 November 2008, Mike Frysinger wrote:
...
> i understand the compiler side just fine. my point is that the
> current linux/of_gpio.h only defines the OF structure and related
> prototypes when CONFIG_OF_GPIO is defined. so it needs updating
> first.
This is why I proposed to split the driver in two parts (gpio only mdio
driver + OF platform device). Just treating OF GPIO as any other GPIO.
This will remove all #ifdefs, the same code will be used for OF and non-OF
platforms and defining a platform device is not so complicated.
Best regards
Marc
^ permalink raw reply
* Re: [PATCH] phylib: make mdio-gpio work without OF (v2)
From: Paulius Zaleckas @ 2008-11-05 8:57 UTC (permalink / raw)
To: Mike Frysinger
Cc: David Brownell, Grant Likely, netdev, linux-arm-kernel,
linux-embedded, Laurent Pinchart
In-Reply-To: <8bd0f97a0811050032i206214a6r8d30a58f354995b4@mail.gmail.com>
Mike Frysinger wrote:
> On Wed, Nov 5, 2008 at 03:11, David Brownell wrote:
>> On Tuesday 04 November 2008, Mike Frysinger wrote:
>>>> If indeed OF functions don't have declarations which are
>>>> available on all platforms, that might be worth fixing
>>>> to enable this approach to #ifdef elimination.
>>> OF is open firmware right ? no way there's going to be OF for every
>>> port that supports GPIO, so filling out the stubs in linux/of_gpio.h
>>> will need to be done.
>> In which case the boolean "we don't have OF" would be "false" and
>> any code referencing the declared functions would be compiled out
>> (being dead code/data). So there would be no link-time references
>> to any OF routines... standard technique.
>
> i understand the compiler side just fine. my point is that the
> current linux/of_gpio.h only defines the OF structure and related
> prototypes when CONFIG_OF_GPIO is defined. so it needs updating
> first.
This is small problem compared to what you will find in linux/of_device.h
It includes <asm/of_device.h> which contains main of_device structure
and this won't work with any non OF platform. Maybe the best solution is
to write separate driver for OF which will be just wrapper for platform
driver... Just like you suggested.
> -mike
>
^ permalink raw reply
* Re: [PATCH] phylib: make mdio-gpio work without OF (v2)
From: Paulius Zaleckas @ 2008-11-05 9:04 UTC (permalink / raw)
Cc: Mike Frysinger, David Brownell, Grant Likely, netdev,
linux-arm-kernel, linux-embedded, Laurent Pinchart
In-Reply-To: <49116000.7000005@teltonika.lt>
Paulius Zaleckas wrote:
> Mike Frysinger wrote:
>> On Wed, Nov 5, 2008 at 03:11, David Brownell wrote:
>>> On Tuesday 04 November 2008, Mike Frysinger wrote:
>>>>> If indeed OF functions don't have declarations which are
>>>>> available on all platforms, that might be worth fixing
>>>>> to enable this approach to #ifdef elimination.
>>>> OF is open firmware right ? no way there's going to be OF for every
>>>> port that supports GPIO, so filling out the stubs in linux/of_gpio.h
>>>> will need to be done.
>>> In which case the boolean "we don't have OF" would be "false" and
>>> any code referencing the declared functions would be compiled out
>>> (being dead code/data). So there would be no link-time references
>>> to any OF routines... standard technique.
>> i understand the compiler side just fine. my point is that the
>> current linux/of_gpio.h only defines the OF structure and related
>> prototypes when CONFIG_OF_GPIO is defined. so it needs updating
>> first.
>
> This is small problem compared to what you will find in linux/of_device.h
> It includes <asm/of_device.h> which contains main of_device structure
> and this won't work with any non OF platform. Maybe the best solution is
> to write separate driver for OF which will be just wrapper for platform
> driver... Just like you suggested.
Oops... Not you, but Marc Pignat suggested.
>> -mike
>>
^ permalink raw reply
* Re: [PATCH] phylib: make mdio-gpio work without OF (v2)
From: Grant Likely @ 2008-11-05 15:04 UTC (permalink / raw)
To: Paulius Zaleckas
Cc: Mike Frysinger, David Brownell, netdev, linux-arm-kernel,
linux-embedded, Laurent Pinchart
In-Reply-To: <49116000.7000005@teltonika.lt>
On Wed, Nov 5, 2008 at 1:57 AM, Paulius Zaleckas
<paulius.zaleckas@teltonika.lt> wrote:
> Mike Frysinger wrote:
>> On Wed, Nov 5, 2008 at 03:11, David Brownell wrote:
>>> On Tuesday 04 November 2008, Mike Frysinger wrote:
>>>>> If indeed OF functions don't have declarations which are
>>>>> available on all platforms, that might be worth fixing
>>>>> to enable this approach to #ifdef elimination.
>>>> OF is open firmware right ? no way there's going to be OF for every
>>>> port that supports GPIO, so filling out the stubs in linux/of_gpio.h
>>>> will need to be done.
>>> In which case the boolean "we don't have OF" would be "false" and
>>> any code referencing the declared functions would be compiled out
>>> (being dead code/data). So there would be no link-time references
>>> to any OF routines... standard technique.
>>
>> i understand the compiler side just fine. my point is that the
>> current linux/of_gpio.h only defines the OF structure and related
>> prototypes when CONFIG_OF_GPIO is defined. so it needs updating
>> first.
>
> This is small problem compared to what you will find in linux/of_device.h
> It includes <asm/of_device.h> which contains main of_device structure
> and this won't work with any non OF platform. Maybe the best solution is
> to write separate driver for OF which will be just wrapper for platform
> driver... Just like you suggested.
More complex than necessary. Just give the driver 2 blocks of binding
code; one for OF, one for non-OF. No need to split up the driver.
g.
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
^ permalink raw reply
* Re: [PATCH] phylib: make mdio-gpio work without OF (v2)
From: David Brownell @ 2008-11-05 22:59 UTC (permalink / raw)
To: Grant Likely
Cc: Paulius Zaleckas, Mike Frysinger, netdev, linux-arm-kernel,
linux-embedded, Laurent Pinchart
In-Reply-To: <fa686aa40811050704s2775fba8nc957a23cb81bb21d@mail.gmail.com>
On Wednesday 05 November 2008, Grant Likely wrote:
> > Maybe the best solution is
> > to write separate driver for OF which will be just wrapper for platform
> > driver... Just like you suggested.
I'd certainly prefer that: packaging all the OF glue in
a small bit of OF glue code, which doesn't need to affect
the majority of platforms (which don't and won't use OF).
> More complex than necessary. Just give the driver 2 blocks of binding
> code; one for OF, one for non-OF. No need to split up the driver.
The preceding discussion pointed out ways the "2 blocks"
approach doesn't currently work well ... minimally, there
is currently no way to do that without much ugly #ifdeffing.
And clearly, you and I view this a bit differently. I'm
happier to think of these kinds of configuration as one
driver plus *separate* platform glue, while you want to be
modifying every driver to accomodate OF. Or, by extension,
ACPI and any other hardware abstraction layer that comes
along.
- Dave
^ permalink raw reply
* Re: [PATCH] phylib: make mdio-gpio work without OF (v2)
From: David Brownell @ 2008-11-05 23:03 UTC (permalink / raw)
To: Marc Pignat
Cc: Mike Frysinger, Grant Likely, Paulius Zaleckas, netdev,
linux-arm-kernel, linux-embedded, Laurent Pinchart
In-Reply-To: <200811050957.25821.marc.pignat@hevs.ch>
On Wednesday 05 November 2008, Marc Pignat wrote:
> This is why I proposed to split the driver in two parts (gpio only mdio
> driver + OF platform device). Just treating OF GPIO as any other GPIO.
I think that's overall a much cleaner approach too. Keeps
OF's hardware abstraction layer out of drivers.
But for some reason the OF folk don't like that model.
It comes up repeatedly, with lots of frameworks and their
drivers.
^ permalink raw reply
* Alignment on XScale ARM
From: Marcus Tangermann @ 2008-11-07 22:52 UTC (permalink / raw)
To: linux-embedded
Hi,
we use an XScale 422 (ARM) with a big endian Linux system based on the buildroot environment. It seems there is an alignment problem we have. I've tracked down the problem to the following.
1. The data received via a netlink communication from the kernel is stored into a buffer buf. The data is correct.
2. The address of the buffer is converted to a pointer to nlmsghdr:
struct nlmsghdr *h;
h = (struct nlmsghdr*) buf;
printf("IPSECTOOLS: Length %04d\n", h->nlmsg_len);
printf("IPSECTOOLS: Type %02d\n", h->nlmsg_type);
The results of printf are nonsens. The first fields seem to have an offset of 2 bytes, at the end there also seem to be swapped bytes.
3. To test what might happen, I've created an own struct
struct aligntest{
__u32 first;
__u32 int second;
__u32 third;
__u32 fourth;
};
When you now set a point to a buffer, again the values are wrong:
s1 = (struct aligntest*) buf;
printf("1: %08X\n", s1->first);
printf("2: %08X\n", s1->second);
printf("3: %08X\n", s1->third);
printf("4: %08X\n", s1->fourth);
It seems, that there is a problem with the unsigned int values. When you add the attribute "packed" to the definition of struct aligntest ( __attribute__(__packed__) ), all values are displayed correctly.
So, any hints what the problem can be?
Regards
Marcus
____________________________________________________________________
Psssst! Schon vom neuen WEB.DE MultiMessenger gehört?
Der kann`s mit allen: http://www.produkte.web.de/messenger/?did=3123
^ permalink raw reply
* Re: Alignment on XScale ARM
From: Will Newton @ 2008-11-08 14:22 UTC (permalink / raw)
To: Marcus Tangermann; +Cc: linux-embedded
In-Reply-To: <1733787858@web.de>
On Fri, Nov 7, 2008 at 10:52 PM, Marcus Tangermann
<Marcus.Tangermann@web.de> wrote:
> Hi,
>
> we use an XScale 422 (ARM) with a big endian Linux system based on the buildroot environment. It seems there is an alignment problem we have. I've tracked down the problem to the following.
> 1. The data received via a netlink communication from the kernel is stored into a buffer buf. The data is correct.
> 2. The address of the buffer is converted to a pointer to nlmsghdr:
> struct nlmsghdr *h;
> h = (struct nlmsghdr*) buf;
> printf("IPSECTOOLS: Length %04d\n", h->nlmsg_len);
> printf("IPSECTOOLS: Type %02d\n", h->nlmsg_type);
> The results of printf are nonsens. The first fields seem to have an offset of 2 bytes, at the end there also seem to be swapped bytes.
> 3. To test what might happen, I've created an own struct
> struct aligntest{
> __u32 first;
> __u32 int second;
> __u32 third;
> __u32 fourth;
> };
> When you now set a point to a buffer, again the values are wrong:
> s1 = (struct aligntest*) buf;
> printf("1: %08X\n", s1->first);
> printf("2: %08X\n", s1->second);
> printf("3: %08X\n", s1->third);
> printf("4: %08X\n", s1->fourth);
>
> It seems, that there is a problem with the unsigned int values. When you add the attribute "packed" to the definition of struct aligntest ( __attribute__(__packed__) ), all values are displayed correctly.
> So, any hints what the problem can be?
I'm not sure what the problem is? You can't generally misalign a
struct pointer safely.
^ permalink raw reply
* Re: Alignment on XScale ARM
From: Nicolas Pitre @ 2008-11-08 15:37 UTC (permalink / raw)
To: Marcus Tangermann; +Cc: linux-embedded
In-Reply-To: <1733787858@web.de>
On Fri, 7 Nov 2008, Marcus Tangermann wrote:
> Hi,
>
> we use an XScale 422 (ARM) with a big endian Linux system based on the
> buildroot environment. It seems there is an alignment problem we have.
> I've tracked down the problem to the following. 1. The data received
> via a netlink communication from the kernel is stored into a buffer
> buf. The data is correct. 2. The address of the buffer is converted to
> a pointer to nlmsghdr:
> struct nlmsghdr *h;
> h = (struct nlmsghdr*) buf;
> printf("IPSECTOOLS: Length %04d\n", h->nlmsg_len);
> printf("IPSECTOOLS: Type %02d\n", h->nlmsg_type);
> The results of printf are nonsens. The first fields seem to have an
> offset of 2 bytes, at the end there also seem to be swapped bytes. 3.
> To test what might happen, I've created an own struct
> struct aligntest{
> __u32 first;
> __u32 int second;
> __u32 third;
> __u32 fourth;
> };
> When you now set a point to a buffer, again the values are wrong:
> s1 = (struct aligntest*) buf;
> printf("1: %08X\n", s1->first);
> printf("2: %08X\n", s1->second);
> printf("3: %08X\n", s1->third);
> printf("4: %08X\n", s1->fourth);
>
> It seems, that there is a problem with the unsigned int values. When
> you add the attribute "packed" to the definition of struct aligntest (
> __attribute__(__packed__) ), all values are displayed correctly. So,
> any hints what the problem can be?
The ARM processor traditionally cannot access misaligned short and int
values in memory. By using the packed attribute you tell the compiler
that the structure should not have inserted padding to align its
members, and that accesses are to be performed wth byte accesses and the
value reconstructed that way.
Nicolas
^ permalink raw reply
* Re: Alignment on XScale ARM
From: David Brownell @ 2008-11-08 18:02 UTC (permalink / raw)
To: Nicolas Pitre, Marcus Tangermann; +Cc: linux-embedded
In-Reply-To: <alpine.LFD.2.00.0811081029500.13034@xanadu.home>
On Saturday 08 November 2008, Nicolas Pitre wrote:
> The ARM processor traditionally cannot access misaligned short and int
> values in memory. By using the packed attribute you tell the compiler
> that the structure should not have inserted padding to align its
> members, and that accesses are to be performed wth byte accesses and the
> value reconstructed that way.
... but then when you pass "&misaligned_ptr->member" to some other
function, that other function generally won't know it needs to use
byte access. So, be careful. The "packed" struct attribute isn't
a panacea.
General Linux kernel policy is to use <asm/unaligned.h> accessors,
but obviously "packed" has advantages when you've got to pull
structures out of protocol buffers. The userspace headers don't
seem to support such accessors though.
Also, the network stack is known for making assumptions about its
data being properly aligned. See how NET_IP_ALIGN is used inside
the kernel; you might be happier aligning your RX buffers before
reading data into them from the kernel.
- Dave
^ permalink raw reply
* Re: Alignment on XScale ARM
From: Marcus Tangermann @ 2008-11-08 18:37 UTC (permalink / raw)
To: linux-embedded
Hello,
due to your hints I've found a solution. The problem was simple: The buffer was not word aligned as the Xscale requires.
With
char buffer[8192] __attribute__((aligned(4)));
things are working. Sometimes you don't see the wood for the trees.
So thanks to all for the good hints!
Marcus
> -----Ursprüngliche Nachricht-----
> Von: "Marcus Tangermann" <Marcus.Tangermann@web.de>
> Gesendet: 07.11.08 23:52:43
> An: linux-embedded@vger.kernel.org
> Betreff: Alignment on XScale ARM
> Hi,
>
> we use an XScale 422 (ARM) with a big endian Linux system based on the buildroot environment. It seems there is an alignment problem we have. I've tracked down the problem to the following.
> 1. The data received via a netlink communication from the kernel is stored into a buffer buf. The data is correct.
> 2. The address of the buffer is converted to a pointer to nlmsghdr:
> struct nlmsghdr *h;
> h = (struct nlmsghdr*) buf;
> printf("IPSECTOOLS: Length %04d\n", h->nlmsg_len);
> printf("IPSECTOOLS: Type %02d\n", h->nlmsg_type);
> The results of printf are nonsens. The first fields seem to have an offset of 2 bytes, at the end there also seem to be swapped bytes.
> 3. To test what might happen, I've created an own struct
> struct aligntest{
> __u32 first;
> __u32 int second;
> __u32 third;
> __u32 fourth;
> };
> When you now set a point to a buffer, again the values are wrong:
> s1 = (struct aligntest*) buf;
> printf("1: %08X\n", s1->first);
> printf("2: %08X\n", s1->second);
> printf("3: %08X\n", s1->third);
> printf("4: %08X\n", s1->fourth);
>
> It seems, that there is a problem with the unsigned int values. When you add the attribute "packed" to the definition of struct aligntest ( __attribute__(__packed__) ), all values are displayed correctly.
> So, any hints what the problem can be?
>
> Regards
> Marcus
> ____________________________________________________________________
> Psssst! Schon vom neuen WEB.DE MultiMessenger gehört?
> Der kann`s mit allen: http://www.produkte.web.de/messenger/?did=3123
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-embedded" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
__________________________________________________________________
"Run, Fatboy, Run" sowie "Rails & Ties" kostenlos anschauen!
Blockbuster-Gutscheine sichern unter http://www.blockbuster.web.de
^ permalink raw reply
* [PATCH] phylib: make mdio-gpio work without OF (v3)
From: Paulius Zaleckas @ 2008-11-10 15:11 UTC (permalink / raw)
To: netdev
Cc: linux-arm-kernel, linux-embedded, Paulius Zaleckas,
Laurent Pinchart, Grant Likely, Mike Frysinger
make mdio-gpio work with non OpenFirmware gpio implementation.
Aditional changes to mdio-gpio:
- use gpio_request() and gpio_free()
- place irq[] array in struct mdio_gpio_info
- add module description, author and license
- add note about compiling this driver as module
- rename mdc and mdio function (were ugly names)
- change MII to MDIO in bus name
- add __init __exit to module (un)loading functions
- probe fails if no phys added to the bus
- kzalloc bitbang with sizeof(*bitbang)
Changes since v2:
- more #ifdefs reduction
- platform driver will be registered on OF platforms also
- unified platform and OF bus_id to phy%i
Changes since v1:
- removed NO_IRQ
- reduced #idefs
Laurent, please test this driver under OF.
Signed-off-by: Paulius Zaleckas <paulius.zaleckas@teltonika.lt>
Cc: Laurent Pinchart <laurent.pinchart@skynet.be>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: Mike Frysinger <vapier.adi@gmail.com>
---
drivers/net/phy/Kconfig | 5 +
drivers/net/phy/mdio-gpio.c | 232 ++++++++++++++++++++++++++++++-------------
include/linux/mdio-gpio.h | 25 +++++
3 files changed, 191 insertions(+), 71 deletions(-)
create mode 100644 include/linux/mdio-gpio.h
diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 0318077..c4c5a2f 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -86,8 +86,11 @@ config MDIO_BITBANG
config MDIO_GPIO
tristate "Support for GPIO lib-based bitbanged MDIO buses"
- depends on MDIO_BITBANG && OF_GPIO
+ depends on MDIO_BITBANG && GENERIC_GPIO
---help---
Supports GPIO lib-based MDIO busses.
+ To compile this driver as a module, choose M here: the module
+ will be called mdio-gpio.
+
endif # PHYLIB
diff --git a/drivers/net/phy/mdio-gpio.c b/drivers/net/phy/mdio-gpio.c
index 2ff9775..1a889fa 100644
--- a/drivers/net/phy/mdio-gpio.c
+++ b/drivers/net/phy/mdio-gpio.c
@@ -1,9 +1,12 @@
/*
- * OpenFirmware GPIO based MDIO bitbang driver.
+ * GPIO based MDIO bitbang driver.
+ * Supports OpenFirmware.
*
* Copyright (c) 2008 CSE Semaphore Belgium.
* by Laurent Pinchart <laurentp@cse-semaphore.com>
*
+ * Copyright (C) 2008, Paulius Zaleckas <paulius.zaleckas@teltonika.lt>
+ *
* Based on earlier work by
*
* Copyright (c) 2003 Intracom S.A.
@@ -21,9 +24,14 @@
#include <linux/slab.h>
#include <linux/init.h>
#include <linux/interrupt.h>
-#include <linux/mdio-bitbang.h>
+#include <linux/platform_device.h>
+#include <linux/gpio.h>
+#include <linux/mdio-gpio.h>
+
+#ifdef CONFIG_OF_GPIO
#include <linux/of_gpio.h>
#include <linux/of_platform.h>
+#endif
struct mdio_gpio_info {
struct mdiobb_ctrl ctrl;
@@ -41,7 +49,7 @@ static void mdio_dir(struct mdiobb_ctrl *ctrl, int dir)
gpio_direction_input(bitbang->mdio);
}
-static int mdio_read(struct mdiobb_ctrl *ctrl)
+static int mdio_get(struct mdiobb_ctrl *ctrl)
{
struct mdio_gpio_info *bitbang =
container_of(ctrl, struct mdio_gpio_info, ctrl);
@@ -49,7 +57,7 @@ static int mdio_read(struct mdiobb_ctrl *ctrl)
return gpio_get_value(bitbang->mdio);
}
-static void mdio(struct mdiobb_ctrl *ctrl, int what)
+static void mdio_set(struct mdiobb_ctrl *ctrl, int what)
{
struct mdio_gpio_info *bitbang =
container_of(ctrl, struct mdio_gpio_info, ctrl);
@@ -57,7 +65,7 @@ static void mdio(struct mdiobb_ctrl *ctrl, int what)
gpio_set_value(bitbang->mdio, what);
}
-static void mdc(struct mdiobb_ctrl *ctrl, int what)
+static void mdc_set(struct mdiobb_ctrl *ctrl, int what)
{
struct mdio_gpio_info *bitbang =
container_of(ctrl, struct mdio_gpio_info, ctrl);
@@ -67,93 +75,69 @@ static void mdc(struct mdiobb_ctrl *ctrl, int what)
static struct mdiobb_ops mdio_gpio_ops = {
.owner = THIS_MODULE,
- .set_mdc = mdc,
+ .set_mdc = mdc_set,
.set_mdio_dir = mdio_dir,
- .set_mdio_data = mdio,
- .get_mdio_data = mdio_read,
+ .set_mdio_data = mdio_set,
+ .get_mdio_data = mdio_get,
};
-static int __devinit mdio_ofgpio_bitbang_init(struct mii_bus *bus,
- struct device_node *np)
-{
- struct mdio_gpio_info *bitbang = bus->priv;
-
- bitbang->mdc = of_get_gpio(np, 0);
- bitbang->mdio = of_get_gpio(np, 1);
-
- if (bitbang->mdc < 0 || bitbang->mdio < 0)
- return -ENODEV;
-
- snprintf(bus->id, MII_BUS_ID_SIZE, "%x", bitbang->mdc);
- return 0;
-}
-
-static void __devinit add_phy(struct mii_bus *bus, struct device_node *np)
+static int __devinit mdio_gpio_bus_init(struct device *dev,
+ struct mdio_gpio_platform_data *pdata,
+ int bus_id)
{
- const u32 *data;
- int len, id, irq;
-
- data = of_get_property(np, "reg", &len);
- if (!data || len != 4)
- return;
-
- id = *data;
- bus->phy_mask &= ~(1 << id);
-
- irq = of_irq_to_resource(np, 0, NULL);
- if (irq != NO_IRQ)
- bus->irq[id] = irq;
-}
-
-static int __devinit mdio_ofgpio_probe(struct of_device *ofdev,
- const struct of_device_id *match)
-{
- struct device_node *np = NULL;
struct mii_bus *new_bus;
struct mdio_gpio_info *bitbang;
int ret = -ENOMEM;
int i;
- bitbang = kzalloc(sizeof(struct mdio_gpio_info), GFP_KERNEL);
+ bitbang = kzalloc(sizeof(*bitbang), GFP_KERNEL);
if (!bitbang)
goto out;
bitbang->ctrl.ops = &mdio_gpio_ops;
+ bitbang->mdc = pdata->mdc;
+ bitbang->mdio = pdata->mdio;
new_bus = alloc_mdio_bitbang(&bitbang->ctrl);
if (!new_bus)
goto out_free_bitbang;
- new_bus->name = "GPIO Bitbanged MII",
+ new_bus->name = "GPIO Bitbanged MDIO",
- ret = mdio_ofgpio_bitbang_init(new_bus, ofdev->node);
- if (ret)
- goto out_free_bus;
+ ret = -ENODEV;
- new_bus->phy_mask = ~0;
- new_bus->irq = kmalloc(sizeof(int) * PHY_MAX_ADDR, GFP_KERNEL);
- if (!new_bus->irq)
+ new_bus->phy_mask = pdata->phy_mask;
+ new_bus->irq = pdata->irqs;
+ new_bus->parent = dev;
+
+ if (new_bus->phy_mask == ~0)
goto out_free_bus;
for (i = 0; i < PHY_MAX_ADDR; i++)
- new_bus->irq[i] = -1;
+ if (!new_bus->irq[i])
+ new_bus->irq[i] = PHY_POLL;
- while ((np = of_get_next_child(ofdev->node, np)))
- if (!strcmp(np->type, "ethernet-phy"))
- add_phy(new_bus, np);
+ snprintf(new_bus->id, MII_BUS_ID_SIZE, "phy%i", bus_id);
+
+ if (gpio_request(bitbang->mdc, "mdc"))
+ goto out_free_bus;
- new_bus->parent = &ofdev->dev;
- dev_set_drvdata(&ofdev->dev, new_bus);
+ if (gpio_request(bitbang->mdio, "mdio"))
+ goto out_free_mdc;
+
+ dev_set_drvdata(dev, new_bus);
ret = mdiobus_register(new_bus);
if (ret)
- goto out_free_irqs;
+ goto out_free_all;
return 0;
-out_free_irqs:
- dev_set_drvdata(&ofdev->dev, NULL);
- kfree(new_bus->irq);
+out_free_all:
+ dev_set_drvdata(dev, NULL);
+ gpio_free(bitbang->mdio);
+out_free_mdc:
+ gpio_free(bitbang->mdc);
out_free_bus:
free_mdio_bitbang(new_bus);
out_free_bitbang:
@@ -162,16 +146,86 @@ out:
return ret;
}
-static int mdio_ofgpio_remove(struct of_device *ofdev)
+static void __devexit mdio_gpio_bus_destroy(struct device *dev)
{
- struct mii_bus *bus = dev_get_drvdata(&ofdev->dev);
+ struct mii_bus *bus = dev_get_drvdata(dev);
struct mdio_gpio_info *bitbang = bus->priv;
mdiobus_unregister(bus);
- kfree(bus->irq);
free_mdio_bitbang(bus);
- dev_set_drvdata(&ofdev->dev, NULL);
+ dev_set_drvdata(dev, NULL);
+ gpio_free(bitbang->mdc);
+ gpio_free(bitbang->mdio);
kfree(bitbang);
+}
+
+static int __devinit mdio_gpio_probe(struct platform_device *pdev)
+{
+ struct mdio_gpio_platform_data *pdata = pdev->dev.platform_data;
+
+ if (!pdata)
+ return -ENODEV;
+
+ return mdio_gpio_bus_init(&pdev->dev, pdata, pdev->id);
+}
+
+static int __devexit mdio_gpio_remove(struct platform_device *pdev)
+{
+ mdio_gpio_bus_destroy(&pdev->dev);
+
+ return 0;
+}
+
+#ifdef CONFIG_OF_GPIO
+static void __devinit add_phy(struct mdio_gpio_platform_data *pdata,
+ struct device_node *np)
+{
+ const u32 *data;
+ int len, id, irq;
+
+ data = of_get_property(np, "reg", &len);
+ if (!data || len != 4)
+ return;
+
+ id = *data;
+ pdata->phy_mask &= ~(1 << id);
+
+ irq = of_irq_to_resource(np, 0, NULL);
+ if (irq)
+ pdata->irqs[id] = irq;
+}
+
+static int __devinit mdio_ofgpio_probe(struct of_device *ofdev,
+ const struct of_device_id *match)
+{
+ struct device_node *np = NULL;
+ struct mdio_gpio_platform_data *pdata;
+
+ pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
+ if (!pdata)
+ return -ENOMEM;
+
+ pdata->mdc = of_get_gpio(ofdev->node, 0);
+ pdata->mdio = of_get_gpio(ofdev->node, 1);
+
+ if (pdata->mdc < 0 || pdata->mdio < 0)
+ goto out_free;
+
+ while ((np = of_get_next_child(ofdev->node, np)))
+ if (!strcmp(np->type, "ethernet-phy"))
+ add_phy(pdata, np);
+
+ return mdio_gpio_bus_init(&ofdev->dev, pdata, pdata->mdc);
+
+out_free:
+ kfree(pdata);
+ return -ENODEV;
+}
+
+static int __devexit mdio_ofgpio_remove(struct of_device *ofdev)
+{
+ mdio_gpio_bus_destroy(&ofdev->dev);
+ kfree(ofdev->dev.platform_data);
return 0;
}
@@ -187,18 +241,56 @@ static struct of_platform_driver mdio_ofgpio_driver = {
.name = "mdio-gpio",
.match_table = mdio_ofgpio_match,
.probe = mdio_ofgpio_probe,
- .remove = mdio_ofgpio_remove,
+ .remove = __devexit_p(mdio_ofgpio_remove),
};
-static int mdio_ofgpio_init(void)
+static inline int __init mdio_ofgpio_init(void)
{
return of_register_platform_driver(&mdio_ofgpio_driver);
}
-static void mdio_ofgpio_exit(void)
+static inline void __exit mdio_ofgpio_exit(void)
{
of_unregister_platform_driver(&mdio_ofgpio_driver);
}
+#else
+static inline int __init mdio_ofgpio_init(void) { return 0; }
+static inline void __exit mdio_ofgpio_exit(void) { }
+#endif /* CONFIG_OF_GPIO */
+
+static struct platform_driver mdio_gpio_driver = {
+ .probe = mdio_gpio_probe,
+ .remove = __devexit_p(mdio_gpio_remove),
+ .driver = {
+ .name = "mdio-gpio",
+ .owner = THIS_MODULE,
+ },
+};
+
+static int __init mdio_gpio_init(void)
+{
+ int ret;
+
+ ret = mdio_ofgpio_init();
+ if (ret)
+ return ret;
+
+ ret = platform_driver_register(&mdio_gpio_driver);
+ if (ret)
+ mdio_ofgpio_exit();
+
+ return ret;
+}
+module_init(mdio_gpio_init);
+
+static void __exit mdio_gpio_exit(void)
+{
+ platform_driver_unregister(&mdio_gpio_driver);
+ mdio_ofgpio_exit();
+}
+module_exit(mdio_gpio_exit);
-module_init(mdio_ofgpio_init);
-module_exit(mdio_ofgpio_exit);
+MODULE_ALIAS("platform:mdio-gpio");
+MODULE_AUTHOR("Laurent Pinchart, Paulius Zaleckas");
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Generic driver for MDIO bus emulation using GPIO");
diff --git a/include/linux/mdio-gpio.h b/include/linux/mdio-gpio.h
new file mode 100644
index 0000000..e9d3fdf
--- /dev/null
+++ b/include/linux/mdio-gpio.h
@@ -0,0 +1,25 @@
+/*
+ * MDIO-GPIO bus platform data structures
+ *
+ * Copyright (C) 2008, Paulius Zaleckas <paulius.zaleckas@teltonika.lt>
+ *
+ * This file is licensed under the terms of the GNU General Public License
+ * version 2. This program is licensed "as is" without any warranty of any
+ * kind, whether express or implied.
+ */
+
+#ifndef __LINUX_MDIO_GPIO_H
+#define __LINUX_MDIO_GPIO_H
+
+#include <linux/mdio-bitbang.h>
+
+struct mdio_gpio_platform_data {
+ /* GPIO numbers for bus pins */
+ unsigned int mdc;
+ unsigned int mdio;
+
+ unsigned int phy_mask;
+ int irqs[PHY_MAX_ADDR];
+};
+
+#endif /* __LINUX_MDIO_GPIO_H */
^ permalink raw reply related
* Re: [PATCH] phylib: make mdio-gpio work without OF (v3)
From: Laurent Pinchart @ 2008-11-10 17:15 UTC (permalink / raw)
To: Paulius Zaleckas
Cc: netdev, linux-arm-kernel, linux-embedded, Grant Likely,
Mike Frysinger
In-Reply-To: <20081110151145.12986.45668.stgit@Programuotojas.82-135-208-232.ip.zebra.lt>
Hi Paulius,
sorry for the late reply. I've been busy at work with a release and had no
time to test the patch. Comments follow.
On Monday 10 November 2008, Paulius Zaleckas wrote:
> make mdio-gpio work with non OpenFirmware gpio implementation.
>
> Aditional changes to mdio-gpio:
> - use gpio_request() and gpio_free()
> - place irq[] array in struct mdio_gpio_info
> - add module description, author and license
> - add note about compiling this driver as module
> - rename mdc and mdio function (were ugly names)
> - change MII to MDIO in bus name
> - add __init __exit to module (un)loading functions
> - probe fails if no phys added to the bus
> - kzalloc bitbang with sizeof(*bitbang)
>
> Changes since v2:
> - more #ifdefs reduction
> - platform driver will be registered on OF platforms also
> - unified platform and OF bus_id to phy%i
>
> Changes since v1:
> - removed NO_IRQ
> - reduced #idefs
>
> Laurent, please test this driver under OF.
Unfortunately this breaks on OF platforms (at least on mine):
~ # ifconfig eth0 up
[ 258.886051] eb:01 not found
[ 258.887409] eth0: Could not attach to phy
See below for an analysis.
> Signed-off-by: Paulius Zaleckas <paulius.zaleckas@teltonika.lt>
> Cc: Laurent Pinchart <laurent.pinchart@skynet.be>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Cc: Mike Frysinger <vapier.adi@gmail.com>
> ---
>
> drivers/net/phy/Kconfig | 5 +
> drivers/net/phy/mdio-gpio.c | 232
> ++++++++++++++++++++++++++++++------------- include/linux/mdio-gpio.h |
> 25 +++++
> 3 files changed, 191 insertions(+), 71 deletions(-)
> create mode 100644 include/linux/mdio-gpio.h
>
>
> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> index 0318077..c4c5a2f 100644
> --- a/drivers/net/phy/Kconfig
> +++ b/drivers/net/phy/Kconfig
> @@ -86,8 +86,11 @@ config MDIO_BITBANG
>
> config MDIO_GPIO
> tristate "Support for GPIO lib-based bitbanged MDIO buses"
> - depends on MDIO_BITBANG && OF_GPIO
> + depends on MDIO_BITBANG && GENERIC_GPIO
> ---help---
> Supports GPIO lib-based MDIO busses.
>
> + To compile this driver as a module, choose M here: the module
> + will be called mdio-gpio.
> +
> endif # PHYLIB
> diff --git a/drivers/net/phy/mdio-gpio.c b/drivers/net/phy/mdio-gpio.c
> index 2ff9775..1a889fa 100644
> --- a/drivers/net/phy/mdio-gpio.c
> +++ b/drivers/net/phy/mdio-gpio.c
> @@ -1,9 +1,12 @@
> /*
> - * OpenFirmware GPIO based MDIO bitbang driver.
> + * GPIO based MDIO bitbang driver.
> + * Supports OpenFirmware.
> *
> * Copyright (c) 2008 CSE Semaphore Belgium.
> * by Laurent Pinchart <laurentp@cse-semaphore.com>
> *
> + * Copyright (C) 2008, Paulius Zaleckas <paulius.zaleckas@teltonika.lt>
> + *
> * Based on earlier work by
> *
> * Copyright (c) 2003 Intracom S.A.
> @@ -21,9 +24,14 @@
> #include <linux/slab.h>
> #include <linux/init.h>
> #include <linux/interrupt.h>
> -#include <linux/mdio-bitbang.h>
> +#include <linux/platform_device.h>
> +#include <linux/gpio.h>
> +#include <linux/mdio-gpio.h>
> +
> +#ifdef CONFIG_OF_GPIO
> #include <linux/of_gpio.h>
> #include <linux/of_platform.h>
> +#endif
>
> struct mdio_gpio_info {
> struct mdiobb_ctrl ctrl;
> @@ -41,7 +49,7 @@ static void mdio_dir(struct mdiobb_ctrl *ctrl, int dir)
> gpio_direction_input(bitbang->mdio);
> }
>
> -static int mdio_read(struct mdiobb_ctrl *ctrl)
> +static int mdio_get(struct mdiobb_ctrl *ctrl)
> {
> struct mdio_gpio_info *bitbang =
> container_of(ctrl, struct mdio_gpio_info, ctrl);
> @@ -49,7 +57,7 @@ static int mdio_read(struct mdiobb_ctrl *ctrl)
> return gpio_get_value(bitbang->mdio);
> }
>
> -static void mdio(struct mdiobb_ctrl *ctrl, int what)
> +static void mdio_set(struct mdiobb_ctrl *ctrl, int what)
> {
> struct mdio_gpio_info *bitbang =
> container_of(ctrl, struct mdio_gpio_info, ctrl);
> @@ -57,7 +65,7 @@ static void mdio(struct mdiobb_ctrl *ctrl, int what)
> gpio_set_value(bitbang->mdio, what);
> }
>
> -static void mdc(struct mdiobb_ctrl *ctrl, int what)
> +static void mdc_set(struct mdiobb_ctrl *ctrl, int what)
> {
> struct mdio_gpio_info *bitbang =
> container_of(ctrl, struct mdio_gpio_info, ctrl);
> @@ -67,93 +75,69 @@ static void mdc(struct mdiobb_ctrl *ctrl, int what)
>
> static struct mdiobb_ops mdio_gpio_ops = {
> .owner = THIS_MODULE,
> - .set_mdc = mdc,
> + .set_mdc = mdc_set,
> .set_mdio_dir = mdio_dir,
> - .set_mdio_data = mdio,
> - .get_mdio_data = mdio_read,
> + .set_mdio_data = mdio_set,
> + .get_mdio_data = mdio_get,
> };
>
> -static int __devinit mdio_ofgpio_bitbang_init(struct mii_bus *bus,
> - struct device_node *np)
> -{
> - struct mdio_gpio_info *bitbang = bus->priv;
> -
> - bitbang->mdc = of_get_gpio(np, 0);
> - bitbang->mdio = of_get_gpio(np, 1);
> -
> - if (bitbang->mdc < 0 || bitbang->mdio < 0)
> - return -ENODEV;
> -
> - snprintf(bus->id, MII_BUS_ID_SIZE, "%x", bitbang->mdc);
The bus id was "%x"...
> - return 0;
> -}
> -
> -static void __devinit add_phy(struct mii_bus *bus, struct device_node *np)
> +static int __devinit mdio_gpio_bus_init(struct device *dev,
> + struct mdio_gpio_platform_data *pdata,
> + int bus_id)
> {
> - const u32 *data;
> - int len, id, irq;
> -
> - data = of_get_property(np, "reg", &len);
> - if (!data || len != 4)
> - return;
> -
> - id = *data;
> - bus->phy_mask &= ~(1 << id);
> -
> - irq = of_irq_to_resource(np, 0, NULL);
> - if (irq != NO_IRQ)
> - bus->irq[id] = irq;
> -}
> -
> -static int __devinit mdio_ofgpio_probe(struct of_device *ofdev,
> - const struct of_device_id *match)
> -{
> - struct device_node *np = NULL;
> struct mii_bus *new_bus;
> struct mdio_gpio_info *bitbang;
> int ret = -ENOMEM;
> int i;
>
> - bitbang = kzalloc(sizeof(struct mdio_gpio_info), GFP_KERNEL);
> + bitbang = kzalloc(sizeof(*bitbang), GFP_KERNEL);
> if (!bitbang)
> goto out;
>
> bitbang->ctrl.ops = &mdio_gpio_ops;
> + bitbang->mdc = pdata->mdc;
> + bitbang->mdio = pdata->mdio;
>
> new_bus = alloc_mdio_bitbang(&bitbang->ctrl);
> if (!new_bus)
> goto out_free_bitbang;
>
> - new_bus->name = "GPIO Bitbanged MII",
> + new_bus->name = "GPIO Bitbanged MDIO",
>
> - ret = mdio_ofgpio_bitbang_init(new_bus, ofdev->node);
> - if (ret)
> - goto out_free_bus;
> + ret = -ENODEV;
>
> - new_bus->phy_mask = ~0;
> - new_bus->irq = kmalloc(sizeof(int) * PHY_MAX_ADDR, GFP_KERNEL);
> - if (!new_bus->irq)
> + new_bus->phy_mask = pdata->phy_mask;
> + new_bus->irq = pdata->irqs;
> + new_bus->parent = dev;
> +
> + if (new_bus->phy_mask == ~0)
> goto out_free_bus;
>
> for (i = 0; i < PHY_MAX_ADDR; i++)
> - new_bus->irq[i] = -1;
> + if (!new_bus->irq[i])
> + new_bus->irq[i] = PHY_POLL;
>
> - while ((np = of_get_next_child(ofdev->node, np)))
> - if (!strcmp(np->type, "ethernet-phy"))
> - add_phy(new_bus, np);
> + snprintf(new_bus->id, MII_BUS_ID_SIZE, "phy%i", bus_id);
And you renamed it to phy%i. Ethernet drivers relying on the id should be
updated as well.
Updating drivers/net/fs_enet/fs_enet-main.c (in find_phy) fixed the problem
for my platform. Please not that this requires a change to
drivers/net/fs_enet/mii-bitbang.c (in fs_mii_bitbang_init) as well.
> +
> + if (gpio_request(bitbang->mdc, "mdc"))
> + goto out_free_bus;
>
> - new_bus->parent = &ofdev->dev;
> - dev_set_drvdata(&ofdev->dev, new_bus);
> + if (gpio_request(bitbang->mdio, "mdio"))
> + goto out_free_mdc;
> +
> + dev_set_drvdata(dev, new_bus);
>
> ret = mdiobus_register(new_bus);
> if (ret)
> - goto out_free_irqs;
> + goto out_free_all;
>
> return 0;
>
> -out_free_irqs:
> - dev_set_drvdata(&ofdev->dev, NULL);
> - kfree(new_bus->irq);
> +out_free_all:
> + dev_set_drvdata(dev, NULL);
> + gpio_free(bitbang->mdio);
> +out_free_mdc:
> + gpio_free(bitbang->mdc);
> out_free_bus:
> free_mdio_bitbang(new_bus);
> out_free_bitbang:
> @@ -162,16 +146,86 @@ out:
> return ret;
> }
>
> -static int mdio_ofgpio_remove(struct of_device *ofdev)
> +static void __devexit mdio_gpio_bus_destroy(struct device *dev)
> {
> - struct mii_bus *bus = dev_get_drvdata(&ofdev->dev);
> + struct mii_bus *bus = dev_get_drvdata(dev);
> struct mdio_gpio_info *bitbang = bus->priv;
>
> mdiobus_unregister(bus);
> - kfree(bus->irq);
> free_mdio_bitbang(bus);
> - dev_set_drvdata(&ofdev->dev, NULL);
> + dev_set_drvdata(dev, NULL);
> + gpio_free(bitbang->mdc);
> + gpio_free(bitbang->mdio);
> kfree(bitbang);
> +}
> +
> +static int __devinit mdio_gpio_probe(struct platform_device *pdev)
> +{
> + struct mdio_gpio_platform_data *pdata = pdev->dev.platform_data;
> +
> + if (!pdata)
> + return -ENODEV;
> +
> + return mdio_gpio_bus_init(&pdev->dev, pdata, pdev->id);
> +}
> +
> +static int __devexit mdio_gpio_remove(struct platform_device *pdev)
> +{
> + mdio_gpio_bus_destroy(&pdev->dev);
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_OF_GPIO
> +static void __devinit add_phy(struct mdio_gpio_platform_data *pdata,
> + struct device_node *np)
> +{
> + const u32 *data;
> + int len, id, irq;
> +
> + data = of_get_property(np, "reg", &len);
> + if (!data || len != 4)
> + return;
> +
> + id = *data;
> + pdata->phy_mask &= ~(1 << id);
> +
> + irq = of_irq_to_resource(np, 0, NULL);
> + if (irq)
> + pdata->irqs[id] = irq;
> +}
> +
> +static int __devinit mdio_ofgpio_probe(struct of_device *ofdev,
> + const struct of_device_id *match)
> +{
> + struct device_node *np = NULL;
> + struct mdio_gpio_platform_data *pdata;
> +
> + pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
> + if (!pdata)
> + return -ENOMEM;
> +
> + pdata->mdc = of_get_gpio(ofdev->node, 0);
> + pdata->mdio = of_get_gpio(ofdev->node, 1);
> +
> + if (pdata->mdc < 0 || pdata->mdio < 0)
> + goto out_free;
> +
> + while ((np = of_get_next_child(ofdev->node, np)))
> + if (!strcmp(np->type, "ethernet-phy"))
> + add_phy(pdata, np);
> +
> + return mdio_gpio_bus_init(&ofdev->dev, pdata, pdata->mdc);
> +
> +out_free:
> + kfree(pdata);
> + return -ENODEV;
> +}
> +
> +static int __devexit mdio_ofgpio_remove(struct of_device *ofdev)
> +{
> + mdio_gpio_bus_destroy(&ofdev->dev);
> + kfree(ofdev->dev.platform_data);
>
> return 0;
> }
> @@ -187,18 +241,56 @@ static struct of_platform_driver mdio_ofgpio_driver =
> { .name = "mdio-gpio",
> .match_table = mdio_ofgpio_match,
> .probe = mdio_ofgpio_probe,
> - .remove = mdio_ofgpio_remove,
> + .remove = __devexit_p(mdio_ofgpio_remove),
> };
>
> -static int mdio_ofgpio_init(void)
> +static inline int __init mdio_ofgpio_init(void)
> {
> return of_register_platform_driver(&mdio_ofgpio_driver);
> }
>
> -static void mdio_ofgpio_exit(void)
> +static inline void __exit mdio_ofgpio_exit(void)
> {
> of_unregister_platform_driver(&mdio_ofgpio_driver);
> }
> +#else
> +static inline int __init mdio_ofgpio_init(void) { return 0; }
> +static inline void __exit mdio_ofgpio_exit(void) { }
> +#endif /* CONFIG_OF_GPIO */
> +
> +static struct platform_driver mdio_gpio_driver = {
> + .probe = mdio_gpio_probe,
> + .remove = __devexit_p(mdio_gpio_remove),
> + .driver = {
> + .name = "mdio-gpio",
> + .owner = THIS_MODULE,
> + },
> +};
> +
> +static int __init mdio_gpio_init(void)
> +{
> + int ret;
> +
> + ret = mdio_ofgpio_init();
> + if (ret)
> + return ret;
> +
> + ret = platform_driver_register(&mdio_gpio_driver);
> + if (ret)
> + mdio_ofgpio_exit();
> +
> + return ret;
> +}
> +module_init(mdio_gpio_init);
> +
> +static void __exit mdio_gpio_exit(void)
> +{
> + platform_driver_unregister(&mdio_gpio_driver);
> + mdio_ofgpio_exit();
> +}
> +module_exit(mdio_gpio_exit);
>
> -module_init(mdio_ofgpio_init);
> -module_exit(mdio_ofgpio_exit);
> +MODULE_ALIAS("platform:mdio-gpio");
> +MODULE_AUTHOR("Laurent Pinchart, Paulius Zaleckas");
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Generic driver for MDIO bus emulation using GPIO");
> diff --git a/include/linux/mdio-gpio.h b/include/linux/mdio-gpio.h
> new file mode 100644
> index 0000000..e9d3fdf
> --- /dev/null
> +++ b/include/linux/mdio-gpio.h
> @@ -0,0 +1,25 @@
> +/*
> + * MDIO-GPIO bus platform data structures
> + *
> + * Copyright (C) 2008, Paulius Zaleckas <paulius.zaleckas@teltonika.lt>
> + *
> + * This file is licensed under the terms of the GNU General Public License
> + * version 2. This program is licensed "as is" without any warranty of any
> + * kind, whether express or implied.
> + */
> +
> +#ifndef __LINUX_MDIO_GPIO_H
> +#define __LINUX_MDIO_GPIO_H
> +
> +#include <linux/mdio-bitbang.h>
> +
> +struct mdio_gpio_platform_data {
> + /* GPIO numbers for bus pins */
> + unsigned int mdc;
> + unsigned int mdio;
> +
> + unsigned int phy_mask;
> + int irqs[PHY_MAX_ADDR];
> +};
> +
> +#endif /* __LINUX_MDIO_GPIO_H */
Best regards,
Laurent Pinchart
^ permalink raw reply
* Re: [PATCH] phylib: make mdio-gpio work without OF (v3)
From: Paulius Zaleckas @ 2008-11-11 9:03 UTC (permalink / raw)
To: Laurent Pinchart
Cc: netdev, linux-arm-kernel, linux-embedded, Grant Likely,
Mike Frysinger
In-Reply-To: <200811101815.54126.laurentp@cse-semaphore.com>
Laurent Pinchart wrote:
> Hi Paulius,
>
> sorry for the late reply. I've been busy at work with a release and had no
> time to test the patch. Comments follow.
>
> On Monday 10 November 2008, Paulius Zaleckas wrote:
>> make mdio-gpio work with non OpenFirmware gpio implementation.
>>
>> Aditional changes to mdio-gpio:
>> - use gpio_request() and gpio_free()
>> - place irq[] array in struct mdio_gpio_info
>> - add module description, author and license
>> - add note about compiling this driver as module
>> - rename mdc and mdio function (were ugly names)
>> - change MII to MDIO in bus name
>> - add __init __exit to module (un)loading functions
>> - probe fails if no phys added to the bus
>> - kzalloc bitbang with sizeof(*bitbang)
>>
>> Changes since v2:
>> - more #ifdefs reduction
>> - platform driver will be registered on OF platforms also
>> - unified platform and OF bus_id to phy%i
>>
>> Changes since v1:
>> - removed NO_IRQ
>> - reduced #idefs
>>
>> Laurent, please test this driver under OF.
>
> Unfortunately this breaks on OF platforms (at least on mine):
>
> ~ # ifconfig eth0 up
> [ 258.886051] eb:01 not found
> [ 258.887409] eth0: Could not attach to phy
>
> The bus id was "%x"...
>
> And you renamed it to phy%i. Ethernet drivers relying on the id should be
> updated as well.
I done this to be closer to what phylib documentation says...
I did search over drivers and found out that almost all of them uses "%x"...
> Updating drivers/net/fs_enet/fs_enet-main.c (in find_phy) fixed the problem
> for my platform. Please not that this requires a change to
> drivers/net/fs_enet/mii-bitbang.c (in fs_mii_bitbang_init) as well.
A lot more than that...
So actually it is better to just change it back to "%x".
Waiting for comments from other people and I will respin patch with this change.
^ permalink raw reply
* [PATCH] uio: add ioctl callback
From: Neil Armstrong @ 2008-11-12 14:11 UTC (permalink / raw)
To: Embedded Linux mailing list
[-- Attachment #1.1: Type: text/plain, Size: 1638 bytes --]
Add an ioctl callback to the UIO device class.
This can be useful when status and data are needed after an interrupt
occurs.
Changes :
- Add an uio_ioctl method
- Add en ioctl entry in uio_info
Neil Armstrong <narmstrong@neotion.com>
Index: drivers/uio/uio.c
===================================================================
--- drivers/uio/uio.c (revision 80)
+++ drivers/uio/uio.c (working copy)
@@ -378,6 +378,17 @@
return 0;
}
+static int uio_ioctl(struct inode *inode, struct file *filep, unsigned
int cmd, unsigned long arg)
+{
+ struct uio_listener *listener = filep->private_data;
+ struct uio_device *idev = listener->dev;
+
+ if (idev->info->ioctl)
+ return idev->info->ioctl(idev->info, cmd, arg);
+
+ return -ENOSYS;
+}
+
static ssize_t uio_read(struct file *filep, char __user *buf,
size_t count, loff_t *ppos)
{
@@ -575,6 +586,7 @@
.mmap = uio_mmap,
.poll = uio_poll,
.fasync = uio_fasync,
+ .ioctl = uio_ioctl,
};
static int uio_major_init(void)
Index: include/linux/uio_driver.h
===================================================================
--- include/linux/uio_driver.h (revision 80)
+++ include/linux/uio_driver.h (working copy)
@@ -68,6 +68,7 @@
int (*open)(struct uio_info *info, struct inode *inode);
int (*release)(struct uio_info *info, struct inode *inode);
int (*irqcontrol)(struct uio_info *info, s32 irq_on);
+ int (*ioctl)(struct uio_info *info, unsigned int cmd, unsigned long
arg);
};
extern int __must_check
[-- Attachment #1.2: narmstrong.vcf --]
[-- Type: text/x-vcard, Size: 319 bytes --]
begin:vcard
fn:Neil Armstrong
n:Armstrong;Neil
org:Neotion;Neotion Sophia Antipolis
adr:;;;Sophia Antipolis;;;France
email;internet:narmstrong@neotion.com
title:Embedded Linux Software Engineer
tel;cell:0667474169
note:PGP 0x1166F485
x-mozilla-html:FALSE
url:http://www.neotion.com
version:2.1
end:vcard
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 252 bytes --]
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox