RE: [PATCH 02/12] Blackfin arch: Add label to call new GPIO API

RE: [PATCH 02/12] Blackfin arch: Add label to call new GPIO API

[Hennerich, Michael]

>-----Original Message-----
>From: David Brownell [mailto:david-b@pacbell.net]
>
>On Friday 17 August 2007, Mike Frysinger wrote:
>> On 8/17/07, David Brownell <david-b@pacbell.net> wrote:
>> > ...
>> > Just for the record, this is an unusual way to use these calls.
>> >
>> > Other platforms completely decouple these issues from the
>> > IRQ infrastructure ... doing the pinmux and gpio claiming
>> > separately from the request_irq()/free_irq() paths, mostly
>> > as part of board setup.  Doing all of that "early":
>> >
>> >  - keeps those error returns from causing hard-to-track-down
>> >    runtime bugs;
>> >
>> >  - works always, even on platforms where a given IRQ may
>> >    appear on any of several pins/balls;
>> >
>> >  - makes it easier to cross-check against board schematics,
>> >    by keeping most board-specific setup in one source file;
>> >
>> >  - shrinks the kernel's runtime footprint;
>> >
>> >  - allows the label to be more descriptive ... describeing
>> >    exactly *which* IRQ, so that using the labels for better
>> >    diagnostics actually gives better diagnostics.
>> >
>> > Again, not "wrong"; but probably sub-optimal.  You might
>> > want to move towards earlier binding now, while Linux is
>> > still young on Blackfin and you don't have legacy code to
>> > worry about.
>>
>> in the Blackfin port, if you want to use a pin as an IRQ rather than
>> GPIO, you use the normal request_irq/free_irq API ... those functions
>> will call back into the proper GPIO/PORTMUX code so that the pin is
>> setup properly.  this is done so that code isnt duplicated across
>> files and so that we can easily detect if someone does something
>> incorrect like try to take the same pin and use it as
>> irq/gpio/whatever at the same time ...
>>
>> are you saying that other ports dont unify the backend code paths at
all
>?
>
>Some platforms try to "unify" the pin setup in the boot loader.
>Most of them cope with bogus bootloaders by doing it in the board
>setup code.
>
>I don't know of any who try to do it "late" as you summarized.
>
>See above why "late" unification is not necessarily as good as
>"early" unification.
>
>And then there's the OMAP1 example, where for example you might
>know that you want MPUIO-0 but that's insufficient to tell whether
>you must mux ball F18 or R13 ... so it's impossible to do the kind
>of "late" unification done here, and pinmux *MUST* be separate from
>IRQ setup.

Dave,

We are not talking about PIN routing. One physical PIN/BALL can only
have one dedicated function the same time. It's more about telling about
possible conflicts, on a development board level.

What Mike wants to point out is that a external IRQ is first a GPIO and
needs to be configured like an INPUT GPIO and then a specific bit needs
to be set unmask it as IRQ.

So why not use the GPIO infrastructure to setup this pin as GPIO?

-Michael  

  


>
>- Dave

Re: [PATCH 02/12] Blackfin arch: Add label to call new GPIO API

[David Brownell]

On Friday 17 August 2007, Hennerich, Michael wrote:
> What Mike wants to point out is that a external IRQ is first a GPIO and
> needs to be configured like an INPUT GPIO and then a specific bit needs
> to be set unmask it as IRQ.
> 
> So why not use the GPIO infrastructure to setup this pin as GPIO?

My comments about the advantages of using that infrastructure
for *early* binding captured the key points ... it's "failfast".

For IRQs you're probably on decently firm ground, since it's
extremely rare that people not handle request_irq() errors.

Remember, I just pointed out that the "late fail" strategy
is unusual.  That doesn't mean it's wrong ... just it'll be
a bit of surprise, some cognitive dissonance to developers
picking up a Blackfin project, potentially more error prone.

- Dave

RE: [PATCH 02/12] Blackfin arch: Add label to call new GPIO API

[Hennerich, Michael]

>-----Original Message-----
>From: David Brownell [mailto:david-b@pacbell.net]
>
>On Friday 17 August 2007, Hennerich, Michael wrote:
>> What Mike wants to point out is that a external IRQ is first a GPIO
and
>> needs to be configured like an INPUT GPIO and then a specific bit
needs
>> to be set unmask it as IRQ.
>>
>> So why not use the GPIO infrastructure to setup this pin as GPIO?
>
>My comments about the advantages of using that infrastructure
>for *early* binding captured the key points ... it's "failfast".
>
>For IRQs you're probably on decently firm ground, since it's
>extremely rare that people not handle request_irq() errors.
>
>Remember, I just pointed out that the "late fail" strategy
>is unusual.  That doesn't mean it's wrong ... just it'll be
>a bit of surprise, some cognitive dissonance to developers
>picking up a Blackfin project, potentially more error prone.
>

Dave,

Thanks - we really appreciate your feedback.
Please believe me - since a great while we have similar internal
discussion how we should handle these things.

Things need to be DAU proof.

We rather prefer having some verbal runtime messages, than having a
system that doesn't do what expected and being silent.
(The bootloader doesn't know what kernel modules are being loaded
requiring specific HW setup) 

We also don't fear the memory overhead (compared to the support
overhead), the runtime overhead is almost neglectable since these
functions are only called once, best case twice (module remove).

I see your points - I would prefer having a fix function board suiting
all our customers' needs - or something like an x86 system where
everything is fixed or dedicated and abstracted by IO/Memory and IRQ.

-Michael 

  


>- Dave

[PATCH 00/12] Blackfin arch GPIO updating

[Bryan Wu]

As David mentioned, I send out these series patch to LKML for review.
These patches are related Blackfin arch GPIO updating, not big change at all.
I think it is OK for git-pull in -RC2 or later.

Thanks
- Bryan Wu

[PATCH 02/12] Blackfin arch: Add label to call new GPIO API

[Bryan Wu]

From: Michael Hennerich <michael.hennerich@analog.com>

Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
Signed-off-by: Bryan Wu <bryan.wu@analog.com>
---
 arch/blackfin/mach-common/ints-priority-dc.c |    4 ++--
 arch/blackfin/mach-common/ints-priority-sc.c |    8 ++++----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/blackfin/mach-common/ints-priority-dc.c b/arch/blackfin/mach-common/ints-priority-dc.c
index 660f881..d5d9e57 100644
--- a/arch/blackfin/mach-common/ints-priority-dc.c
+++ b/arch/blackfin/mach-common/ints-priority-dc.c
@@ -221,7 +221,7 @@ static unsigned int bf561_gpio_irq_startup(unsigned int irq)
 
 	if (!(gpio_enabled[gpio_bank(gpionr)] & gpio_bit(gpionr))) {
 
-		ret = gpio_request(gpionr, NULL);
+		ret = gpio_request(gpionr, "IRQ");
 		if (ret)
 			return ret;
 
@@ -261,7 +261,7 @@ static int bf561_gpio_irq_type(unsigned int irq, unsigned int type)
 
 		if (!(gpio_enabled[gpio_bank(gpionr)] & gpio_bit(gpionr))) {
 
-			ret = gpio_request(gpionr, NULL);
+			ret = gpio_request(gpionr, "IRQ");
 			if (ret)
 				return ret;
 
diff --git a/arch/blackfin/mach-common/ints-priority-sc.c b/arch/blackfin/mach-common/ints-priority-sc.c
index 4708023..505b948 100644
--- a/arch/blackfin/mach-common/ints-priority-sc.c
+++ b/arch/blackfin/mach-common/ints-priority-sc.c
@@ -343,7 +343,7 @@ static unsigned int bfin_gpio_irq_startup(unsigned int irq)
 	u16 gpionr = irq - IRQ_PF0;
 
 	if (!(gpio_enabled[gpio_bank(gpionr)] & gpio_bit(gpionr))) {
-		ret = gpio_request(gpionr, NULL);
+		ret = gpio_request(gpionr, "IRQ");
 		if (ret)
 			return ret;
 	}
@@ -377,7 +377,7 @@ static int bfin_gpio_irq_type(unsigned int irq, unsigned int type)
 	if (type & (IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING |
 		    IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW)) {
 		if (!(gpio_enabled[gpio_bank(gpionr)] & gpio_bit(gpionr))) {
-			ret = gpio_request(gpionr, NULL);
+			ret = gpio_request(gpionr, "IRQ");
 			if (ret)
 				return ret;
 		}
@@ -587,7 +587,7 @@ static unsigned int bfin_gpio_irq_startup(unsigned int irq)
 	}
 
 	if (!(gpio_enabled[gpio_bank(gpionr)] & gpio_bit(gpionr))) {
-		ret = gpio_request(gpionr, NULL);
+		ret = gpio_request(gpionr, "IRQ");
 		if (ret)
 			return ret;
 	}
@@ -627,7 +627,7 @@ static int bfin_gpio_irq_type(unsigned int irq, unsigned int type)
 	if (type & (IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING |
 		    IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW)) {
 		if (!(gpio_enabled[gpio_bank(gpionr)] & gpio_bit(gpionr))) {
-			ret = gpio_request(gpionr, NULL);
+			ret = gpio_request(gpionr, "IRQ");
 			if (ret)
 				return ret;
 		}
-- 
1.5.2

Re: [PATCH 02/12] Blackfin arch: Add label to call new GPIO API

[David Brownell]

Again, the patch descriptions need work.  This changes the
IRQ code (to add those labels).  $SUBJECT doesn't mention IRQs,
neither does the description ...


On Tuesday 07 August 2007, Bryan Wu wrote:
> --- a/arch/blackfin/mach-common/ints-priority-dc.c
> +++ b/arch/blackfin/mach-common/ints-priority-dc.c
> @@ -221,7 +221,7 @@ static unsigned int bf561_gpio_irq_startup(unsigned int irq)
>  
>         if (!(gpio_enabled[gpio_bank(gpionr)] & gpio_bit(gpionr))) {
>  
> -               ret = gpio_request(gpionr, NULL);
> +               ret = gpio_request(gpionr, "IRQ");
>                 if (ret)
>                         return ret;
>  
> @@ -261,7 +261,7 @@ static int bf561_gpio_irq_type(unsigned int irq, unsigned int type)
>  
>                 if (!(gpio_enabled[gpio_bank(gpionr)] & gpio_bit(gpionr))) {
>  
> -                       ret = gpio_request(gpionr, NULL);
> +                       ret = gpio_request(gpionr, "IRQ");
>                         if (ret)
>                                 return ret;
>  

Just for the record, this is an unusual way to use these calls.

Other platforms completely decouple these issues from the
IRQ infrastructure ... doing the pinmux and gpio claiming
separately from the request_irq()/free_irq() paths, mostly
as part of board setup.  Doing all of that "early":

 - keeps those error returns from causing hard-to-track-down
   runtime bugs;

 - works always, even on platforms where a given IRQ may
   appear on any of several pins/balls;

 - makes it easier to cross-check against board schematics,
   by keeping most board-specific setup in one source file;

 - shrinks the kernel's runtime footprint;

 - allows the label to be more descriptive ... describeing
   exactly *which* IRQ, so that using the labels for better
   diagnostics actually gives better diagnostics.

Again, not "wrong"; but probably sub-optimal.  You might
want to move towards earlier binding now, while Linux is
still young on Blackfin and you don't have legacy code to
worry about.

- Dave

Re: [PATCH 02/12] Blackfin arch: Add label to call new GPIO API

[Mike Frysinger]

On 8/17/07, David Brownell <david-b@pacbell.net> wrote:
> Again, the patch descriptions need work.  This changes the
> IRQ code (to add those labels).  $SUBJECT doesn't mention IRQs,
> neither does the description ...
>
>
> On Tuesday 07 August 2007, Bryan Wu wrote:
> > --- a/arch/blackfin/mach-common/ints-priority-dc.c
> > +++ b/arch/blackfin/mach-common/ints-priority-dc.c
> > @@ -221,7 +221,7 @@ static unsigned int bf561_gpio_irq_startup(unsigned int irq)
> >
> > if (!(gpio_enabled[gpio_bank(gpionr)] & gpio_bit(gpionr))) {
> >
> > -ret = gpio_request(gpionr, NULL);
> > +ret = gpio_request(gpionr, "IRQ");
> > if (ret)
> > return ret;
> >
> > @@ -261,7 +261,7 @@ static int bf561_gpio_irq_type(unsigned int irq, unsigned int type)
> >
> > if (!(gpio_enabled[gpio_bank(gpionr)] & gpio_bit(gpionr))) {
> >
> > -ret = gpio_request(gpionr, NULL);
> > +ret = gpio_request(gpionr, "IRQ");
> > if (ret)
> > return ret;
> >
>
> Just for the record, this is an unusual way to use these calls.
>
> Other platforms completely decouple these issues from the
> IRQ infrastructure ... doing the pinmux and gpio claiming
> separately from the request_irq()/free_irq() paths, mostly
> as part of board setup.  Doing all of that "early":
>
>  - keeps those error returns from causing hard-to-track-down
>    runtime bugs;
>
>  - works always, even on platforms where a given IRQ may
>    appear on any of several pins/balls;
>
>  - makes it easier to cross-check against board schematics,
>    by keeping most board-specific setup in one source file;
>
>  - shrinks the kernel's runtime footprint;
>
>  - allows the label to be more descriptive ... describeing
>    exactly *which* IRQ, so that using the labels for better
>    diagnostics actually gives better diagnostics.
>
> Again, not "wrong"; but probably sub-optimal.  You might
> want to move towards earlier binding now, while Linux is
> still young on Blackfin and you don't have legacy code to
> worry about.

in the Blackfin port, if you want to use a pin as an IRQ rather than
GPIO, you use the normal request_irq/free_irq API ... those functions
will call back into the proper GPIO/PORTMUX code so that the pin is
setup properly.  this is done so that code isnt duplicated across
files and so that we can easily detect if someone does something
incorrect like try to take the same pin and use it as
irq/gpio/whatever at the same time ...

are you saying that other ports dont unify the backend code paths at all ?
-mike

Re: [PATCH 02/12] Blackfin arch: Add label to call new GPIO API

[David Brownell]

On Friday 17 August 2007, Mike Frysinger wrote:
> On 8/17/07, David Brownell <david-b@pacbell.net> wrote:
> > ...
> > Just for the record, this is an unusual way to use these calls.
> >
> > Other platforms completely decouple these issues from the
> > IRQ infrastructure ... doing the pinmux and gpio claiming
> > separately from the request_irq()/free_irq() paths, mostly
> > as part of board setup.  Doing all of that "early":
> >
> >  - keeps those error returns from causing hard-to-track-down
> >    runtime bugs;
> >
> >  - works always, even on platforms where a given IRQ may
> >    appear on any of several pins/balls;
> >
> >  - makes it easier to cross-check against board schematics,
> >    by keeping most board-specific setup in one source file;
> >
> >  - shrinks the kernel's runtime footprint;
> >
> >  - allows the label to be more descriptive ... describeing
> >    exactly *which* IRQ, so that using the labels for better
> >    diagnostics actually gives better diagnostics.
> >
> > Again, not "wrong"; but probably sub-optimal.  You might
> > want to move towards earlier binding now, while Linux is
> > still young on Blackfin and you don't have legacy code to
> > worry about.
> 
> in the Blackfin port, if you want to use a pin as an IRQ rather than
> GPIO, you use the normal request_irq/free_irq API ... those functions
> will call back into the proper GPIO/PORTMUX code so that the pin is
> setup properly.  this is done so that code isnt duplicated across
> files and so that we can easily detect if someone does something
> incorrect like try to take the same pin and use it as
> irq/gpio/whatever at the same time ...
> 
> are you saying that other ports dont unify the backend code paths at all ?

Some platforms try to "unify" the pin setup in the boot loader.
Most of them cope with bogus bootloaders by doing it in the board
setup code.

I don't know of any who try to do it "late" as you summarized.

See above why "late" unification is not necessarily as good as
"early" unification.

And then there's the OMAP1 example, where for example you might
know that you want MPUIO-0 but that's insufficient to tell whether
you must mux ball F18 or R13 ... so it's impossible to do the kind
of "late" unification done here, and pinmux *MUST* be separate from
IRQ setup.

- Dave

Re: [PATCH 02/12] Blackfin arch: Add label to call new GPIO API

[Mike Frysinger]

On 8/17/07, David Brownell <david-b@pacbell.net> wrote:
> On Friday 17 August 2007, Mike Frysinger wrote:
> > On 8/17/07, David Brownell <david-b@pacbell.net> wrote:
> > > ...
> > > Just for the record, this is an unusual way to use these calls.
> > >
> > > Other platforms completely decouple these issues from the
> > > IRQ infrastructure ... doing the pinmux and gpio claiming
> > > separately from the request_irq()/free_irq() paths, mostly
> > > as part of board setup.  Doing all of that "early":
> > >
> > >  - keeps those error returns from causing hard-to-track-down
> > >    runtime bugs;
> > >
> > >  - works always, even on platforms where a given IRQ may
> > >    appear on any of several pins/balls;
> > >
> > >  - makes it easier to cross-check against board schematics,
> > >    by keeping most board-specific setup in one source file;
> > >
> > >  - shrinks the kernel's runtime footprint;
> > >
> > >  - allows the label to be more descriptive ... describeing
> > >    exactly *which* IRQ, so that using the labels for better
> > >    diagnostics actually gives better diagnostics.
> > >
> > > Again, not "wrong"; but probably sub-optimal.  You might
> > > want to move towards earlier binding now, while Linux is
> > > still young on Blackfin and you don't have legacy code to
> > > worry about.
> >
> > in the Blackfin port, if you want to use a pin as an IRQ rather than
> > GPIO, you use the normal request_irq/free_irq API ... those functions
> > will call back into the proper GPIO/PORTMUX code so that the pin is
> > setup properly.  this is done so that code isnt duplicated across
> > files and so that we can easily detect if someone does something
> > incorrect like try to take the same pin and use it as
> > irq/gpio/whatever at the same time ...
> >
> > are you saying that other ports dont unify the backend code paths at all ?
>
> Some platforms try to "unify" the pin setup in the boot loader.
> Most of them cope with bogus bootloaders by doing it in the board
> setup code.
>
> I don't know of any who try to do it "late" as you summarized.
>
> See above why "late" unification is not necessarily as good as
> "early" unification.
>
> And then there's the OMAP1 example, where for example you might
> know that you want MPUIO-0 but that's insufficient to tell whether
> you must mux ball F18 or R13 ... so it's impossible to do the kind
> of "late" unification done here, and pinmux *MUST* be separate from
> IRQ setup.

sorry, i'm not familiar at all with anything OMAP, but the Blackfin
architecture is such that all of its pins can be flipped on the fly
between modes ... i have seen once or twice some drivers which
actually needed this ability at runtime as they were muxing things
(maybe our fault there ... give someone the ability and they'll use
it).

as Michael pointed out, in the Blackfin world we tend to keep things
very dynamic as we have dev systems which allow for dropping in of
optional cards at will, so doing this in the bootloader is way too
inflexible.
-mike

Re: [PATCH 02/12] Blackfin arch: Add label to call new GPIO API

[Mike Frysinger]

On 8/17/07, Mike Frysinger <vapier.adi@gmail.com> wrote:
> as Michael pointed out, in the Blackfin world we tend to keep things
> very dynamic as we have dev systems which allow for dropping in of
> optional cards at will, so doing this in the bootloader is way too
> inflexible.

oh, and another [smallish] data point.  the Blackfin processor has a
small bootrom on it that could be likened to a very micro bios.  so
it's possible to actually boot the linux kernel straight without a
boot loader.  send the kernel over the UART to a Blackfin and watch it
go go go :)
-mike

Re: [PATCH 02/12] Blackfin arch: Add label to call new GPIO API

[David Brownell]

On Friday 17 August 2007, Mike Frysinger wrote:

> as Michael pointed out, in the Blackfin world we tend to keep things
> very dynamic as we have dev systems which allow for dropping in of
> optional cards at will, so doing this in the bootloader is way too
> inflexible.

That's the tradeoff:  optimize for development boards, or
instead for more fixed-function product boards.


> oh, and another [smallish] data point.  the Blackfin processor has a
> small bootrom on it that could be likened to a very micro bios.  so
> it's possible to actually boot the linux kernel straight without a
> boot loader.  send the kernel over the UART to a Blackfin and watch it
> go go go :)

That's not uncommon, although I'm more used to seeing the
on-chip ROMs relying on a second stage loader for stuff like
getting memory and other clocks going at optimal speeds,
and loading "big" images (that won't fit on-chip SRAM).

- Dave

Re: [PATCH 02/12] Blackfin arch: Add label to call new GPIO API

[Robin Getz]

On Fri 17 Aug 2007 14:24, David Brownell pondered:
> Just for the record, this is an unusual way to use these calls.

That is part of the natural evolution of the kernel isn't it - per James's 
keynote at OLS - you release something, and see how people [ab]use it until 
it either grows, evolves, or it dies.

> Other platforms completely decouple these issues from the
> IRQ infrastructure ... doing the pinmux and gpio claiming
> separately from the request_irq()/free_irq() paths, mostly
> as part of board setup.  Doing all of that "early":

is early:
 - early in the kernel?
 - early before the kernel? (in the bootloader).

>  - keeps those error returns from causing hard-to-track-down
>    runtime bugs;

The current Blackfin implementation causes a run time message:
"the pin xxxx driver requested, was already claimed by yyy driver".

I don't think that is too bad?

>  - works always, even on platforms where a given IRQ may
>    appear on any of several pins/balls;

But requires custom bootloaders or board setup for every hardware platform? 
Most of our users would not like that, since they do as you say - use the 
same kernel - with different drivers on multiple platforms.

>  - makes it easier to cross-check against board schematics,
>    by keeping most board-specific setup in one source file;

Yes - but we are not talking about muxing a common peripheral (like a single 
UART) out many different pins (A or B or C). The UART pins are fixed. If you 
want the UART, you need to use pin A. If you want to use the I2C that also 
sits on pin A, you will get the message:
"pin A, requested by I2C, was already claimed by UART driver".

>  - shrinks the kernel's runtime footprint;

I agree - making things more flexible/easier to use - is normally more 
complex/larger/slower. (I know - easier to use is a matter of opinion). Since 
this is normally done once, in _init functions, I'm not sure that makes much 
of a difference here.
 
>  - allows the label to be more descriptive ... describeing
>    exactly *which* IRQ, so that using the labels for better
>    diagnostics actually gives better diagnostics.

I'm not sure what you mean?

> Again, not "wrong"; but probably sub-optimal.  You might
> want to move towards earlier binding now, while Linux is
> still young on Blackfin and you don't have legacy code to
> worry about.

Our overall goal is to keep as much code - including bootloader - platform 
agnostic, and not require people to write any of code/configuration data to 
boot up something, and get things working in a semi-standard manner.

This still has it's limits - which is why we publish all our hardware designs. 
If you implement things the similar way (because for the most part it is 
fixed by the processor designer) - the bootloader/kernel/driver will just 
work.

I would rather force a little extra complexity on me (as a kernel developer) 
than have to answer thousands of questions from end users, who are trying to 
move the kernel onto their hardware.

-Robin

Re: [PATCH 02/12] Blackfin arch: Add label to call new GPIO API

[David Brownell]

On Friday 17 August 2007, Robin Getz wrote:
> On Fri 17 Aug 2007 14:24, David Brownell pondered:
> > Just for the record, this is an unusual way to use these calls.
> 
> That is part of the natural evolution of the kernel isn't it - per James's 
> keynote at OLS - you release something, and see how people [ab]use it until 
> it either grows, evolves, or it dies.

Yep ... and it's worth knowing when you're doing
something different.  Different isn't always worse,
isn't always better.


> > Other platforms completely decouple these issues from the
> > IRQ infrastructure ... doing the pinmux and gpio claiming
> > separately from the request_irq()/free_irq() paths, mostly
> > as part of board setup.  Doing all of that "early":
> 
> is early:
>  - early in the kernel?
>  - early before the kernel? (in the bootloader).

Both of those are "earlier", yes.  Different product developers
may argue for either placement.

 
> >  - keeps those error returns from causing hard-to-track-down
> >    runtime bugs;
> 
> The current Blackfin implementation causes a run time message:
> "the pin xxxx driver requested, was already claimed by yyy driver".
> 
> I don't think that is too bad?

Given some product with a Blackfin chip, would you expect a
customer -- who may not even see the Linux bits!! -- to be
able to solve such problems?  If it's not possible for such
problems to crop up in the field, product support (and field
troubleshooting) gets easier...


> >  - works always, even on platforms where a given IRQ may
> >    appear on any of several pins/balls;
> 
> But requires custom bootloaders or board setup for every hardware platform?

One or both, yes.  That's typical in embedded setups.
They're not necessarily all that different, but that
code does need to handle the hardware differences.


> Most of our users would not like that, since they do as you say - use the 
> same kernel - with different drivers on multiple platforms.

I thought I referred to different revisions of one platform... :)

 
> >  - makes it easier to cross-check against board schematics,
> >    by keeping most board-specific setup in one source file;
> 
> Yes - but we are not talking about muxing a common peripheral (like a single 
> UART) out many different pins (A or B or C). The UART pins are fixed. If you 
> want the UART, you need to use pin A. If you want to use the I2C that also 
> sits on pin A, you will get the message:
> "pin A, requested by I2C, was already claimed by UART driver".

Not all platforms work that way though.  There can often be several
options for where a given signal gets routed.

And then there are the errors where someone accidentally copies
something like "GPIO 29" to two places ... invariants like "only
one GPIO requestor at a time" are needed to turn up such stuff.


> >  - shrinks the kernel's runtime footprint;
> 
> I agree - making things more flexible/easier to use - is normally more 
> complex/larger/slower. (I know - easier to use is a matter of opinion). Since 
> this is normally done once, in _init functions, I'm not sure that makes much 
> of a difference here.
>  
> >  - allows the label to be more descriptive ... describeing
> >    exactly *which* IRQ, so that using the labels for better
> >    diagnostics actually gives better diagnostics.
> 
> I'm not sure what you mean?

The $SUBJECT patch uses the string "IRQ" in all cases.
But "smc_irq" and "codec_irq" would be more informative
as entries in a list of even just a handful of GPIOs.
And with a few dozen, I'd find "IRQ" not at all helpful.


> > Again, not "wrong"; but probably sub-optimal.  You might
> > want to move towards earlier binding now, while Linux is
> > still young on Blackfin and you don't have legacy code to
> > worry about.
> 
> Our overall goal is to keep as much code - including bootloader - platform 
> agnostic, and not require people to write any of code/configuration data to 
> boot up something, and get things working in a semi-standard manner.

The issue is just where those limits lie.  IMO it's not at
all unreasonable to require board-specific code.  External
chips will need board-specfic glue data in most cases (how
they're addressed, what IRQs they use, and so on); and you
may have drivers available that correspond to devices that
are not wired up on that particular hardware.


> This still has it's limits - which is why we publish all our hardware designs. 
> If you implement things the similar way (because for the most part it is 
> fixed by the processor designer) - the bootloader/kernel/driver will just 
> work.

Sure ... you'd need to say "this board uses <these devices>"
and if integrated in the SOC that's often enough.  External
devices need more configuration.  Even for integrated ones,
that knowledge doesn't belong in the driver ... "which of the
many UARTS to use as console" isn't standard, and neither
is "what hardware handshaking pins are in use".

 
> I would rather force a little extra complexity on me (as a kernel developer) 
> than have to answer thousands of questions from end users, who are trying to 
> move the kernel onto their hardware.

Just remember that "Aunt Tilly" doesn't configure kernels.
And that even deeply technical people who do configure
them may not have the details fresh in mind a few months
later.  ;)

- Dave

Re: [PATCH 02/12] Blackfin arch: Add label to call new GPIO API

[Robin Getz]

On Fri 17 Aug 2007 18:34, David Brownell pondered:
> On Friday 17 August 2007, Robin Getz wrote:
> > On Fri 17 Aug 2007 14:24, David Brownell pondered:
> > > Just for the record, this is an unusual way to use these calls.
> > 
> > That is part of the natural evolution of the kernel isn't it - per
> > James's keynote at OLS - you release something, and see how people 
> > [ab]use it until it either grows, evolves, or it dies.
> 
> Yep ... and it's worth knowing when you're doing
> something different.  Different isn't always worse,
> isn't always better.

No disagreements here.

> > > Other platforms completely decouple these issues from the
> > > IRQ infrastructure ... doing the pinmux and gpio claiming
> > > separately from the request_irq()/free_irq() paths, mostly
> > > as part of board setup.  Doing all of that "early":
> > 
> > is early:
> >  - early in the kernel?
> >  - early before the kernel? (in the bootloader).
> 
> Both of those are "earlier", yes.  Different product developers
> may argue for either placement.

Just like we say things are better/easier for us later.

> > >  - keeps those error returns from causing hard-to-track-down
> > >    runtime bugs;
> > 
> > The current Blackfin implementation causes a run time message:
> > "the pin xxxx driver requested, was already claimed by yyy driver".
> > 
> > I don't think that is too bad?
> 
> Given some product with a Blackfin chip, would you expect a
> customer -- who may not even see the Linux bits!! -- to be
> able to solve such problems?  If it's not possible for such
> problems to crop up in the field, product support (and field
> troubleshooting) gets easier...

Typically customers who are not familiar with the linux bits are not doing 
modprobe either...

I don't see how early/late makes the problem easier/worse to debug. No matter 
when you do it - the driver refuses to install (or at least should).

> > >  - works always, even on platforms where a given IRQ may
> > >    appear on any of several pins/balls;
> > 
> > But requires custom bootloaders or board setup for every hardware
> > platform?
> 
> One or both, yes.  That's typical in embedded setups.
> They're not necessarily all that different, but that
> code does need to handle the hardware differences.

Right - for us - the code handing the hardware differences is easier in the 
drivers, rather than the bootloaders.

For other systems - where you can have a UART on any pin - I completely 
understand your point.

> > Most of our users would not like that, since they do as you say - use
> > the same kernel - with different drivers on multiple platforms.
> 
> I thought I referred to different revisions of one platform... :)

You did - I was just saying that some of our customers don't do it the way you 
were thinking.

> > >  - makes it easier to cross-check against board schematics,
> > >    by keeping most board-specific setup in one source file;
> > 
> > Yes - but we are not talking about muxing a common peripheral (like a
> > single UART) out many different pins (A or B or C). The UART pins are
> > fixed. If you want the UART, you need to use pin A. If you want to use
> > the I2C that also sits on pin A, you will get the message:
> > "pin A, requested by I2C, was already claimed by UART driver".
> 
> Not all platforms work that way though.  There can often be several
> options for where a given signal gets routed.

But this is how it _always_ works on Blackfin. For other systems - like ARM, 
where n+1 silicon manufactures are all implementing things differently - I 
can understand your comments.


> > >  - allows the label to be more descriptive ... describeing
> > >    exactly *which* IRQ, so that using the labels for better
> > >    diagnostics actually gives better diagnostics.
> > 
> > I'm not sure what you mean?
> 
> The $SUBJECT patch uses the string "IRQ" in all cases.
> But "smc_irq" and "codec_irq" would be more informative
> as entries in a list of even just a handful of GPIOs.
> And with a few dozen, I'd find "IRQ" not at all helpful.

I agree - things can always be more descriptive.

> > > Again, not "wrong"; but probably sub-optimal.  You might
> > > want to move towards earlier binding now, while Linux is
> > > still young on Blackfin and you don't have legacy code to
> > > worry about.
> > 
> > Our overall goal is to keep as much code - including bootloader -
> > platform  agnostic, and not require people to write any of
> > code/configuration data to boot up something, and get things
> > working in a semi-standard manner. 
> 
> The issue is just where those limits lie.  IMO it's not at
> all unreasonable to require board-specific code.  External
> chips will need board-specfic glue data in most cases (how
> they're addressed, what IRQs they use, and so on); and you
> may have drivers available that correspond to devices that
> are not wired up on that particular hardware.
> 
> 
> > This still has it's limits - which is why we publish all our hardware
> > designs. If you implement things the similar way (because for the
> > most part it is fixed by the processor designer) - the
> > bootloader/kernel/driver will just work.
> 
> Sure ... you'd need to say "this board uses <these devices>"
> and if integrated in the SOC that's often enough. 

with the kernel .config - that is what happens. If you have 2 serial drivers 
connected - you enable 2 serial drivers in Kconfig.

> External 
> devices need more configuration.  Even for integrated ones,
> that knowledge doesn't belong in the driver ... "which of the
> many UARTS to use as console" isn't standard, and neither
> is "what hardware handshaking pins are in use".

When hardware handshaking pins are fixed - it sure is. When they are not (when 
the hardware doesn't support hardware handshaking, and you need to do it in 
software) - we still allow you do to it via Kconfig.

linux-2.6.x/drivers/serial/Kconfig:

config UART0_CTS_PIN
        int "UART0 CTS pin"
        depends on BFIN_UART0_CTSRTS
        default 23
        help
          The default pin is GPIO_GP7.
          Refer to ./include/asm-blackfin/gpio.h to see the GPIO map.

config UART0_RTS_PIN
        int "UART0 RTS pin"
        depends on BFIN_UART0_CTSRTS
        default 22
        help
          The default pin is GPIO_GP6.
          Refer to ./include/asm-blackfin/gpio.h to see the GPIO map.

Board configs are in one place - under source control - the kernel .config
  
> > I would rather force a little extra complexity on me (as a kernel
> > developer) than have to answer thousands of questions from end 
> > users, who are trying to move the kernel onto their hardware.
> 
> Just remember that "Aunt Tilly" doesn't configure kernels.
> And that even deeply technical people who do configure
> them may not have the details fresh in mind a few months
> later.  ;)

I wish some of our customers where as good as my aunt Tilly when it comes to 
kernel config, or could remember as well as she can.

I guess we thought it was easier for people to select a few things in config, 
rather than have to write C code/include files for board specific 
implementations options - It is like you said - everything is all in one 
place...

-Robin

Re: [PATCH 02/12] Blackfin arch: Add label to call new GPIO API

[David Brownell]

On Saturday 18 August 2007, Robin Getz wrote:

> I don't see how early/late makes the problem easier/worse to debug. No matter 
> when you do it - the driver refuses to install (or at least should).

If you arrange to *reliably* detect the pinmux/setup problems by
the time the system starts ""init" (early), that means one large
class of hard-to-sort problems never needs runtime troubleshooting.

Think of it this way:  folk have observed that pin setup issues can
be painful to sort out.  So they adopt a strategy ("failfast"/"early")
which helps surface them early and basically removes them as issues
in later debugging.  I think you're hoping that by adding extra
resource tracking code, you can make that later debugging easier
even though, by "late" binding, you've introduced extra error paths.


> Right - for us - the code handing the hardware differences is easier in the 
> drivers, rather than the bootloaders.

Remember that I didn't argue in favor of putting that code into
boot loaders ... I just pointed out that some product lines work
that way, so Linux needs to cope with that strategy.  (One of the
many examples involves OpenFirmware device tables.)

But regardless:  I can't buy any argument that it's better to put
lots of board-specific code into drivers.  That adds up quickly,
making maintaining the drivers painful.  "Real" updates (bugfixes,
new features, API updates, cleanup, and so on) regularly end up
in conflict with patches to support a few more boards, and board
support patches must then always involve those driver maintainers.
So merging new boards involves many more people than necessary...


> For other systems - where you can have a UART on any pin - I completely 
> understand your point.

UART on any pin?  Few kernels dynamically reprogram FPGAs!  :)


> > Sure ... you'd need to say "this board uses <these devices>"
> > and if integrated in the SOC that's often enough. 
> 
> with the kernel .config - that is what happens. If you have 2 serial drivers 
> connected - you enable 2 serial drivers in Kconfig.

Your language is incorrect here.  What your Kconfig does is
not configure two different *drivers* ... but some number of
different serial *devices* handled by the same driver.

One obvious downside of that is that making it needlessly hard
to support several boards with one kernel.  As a rule, those
boards can have different serial devices, and the devices can
be configured differently.  Yet you said you wanted to make it
easy to support many boards with one kernel...


> > External 
> > devices need more configuration.  Even for integrated ones,
> > that knowledge doesn't belong in the driver ... "which of the
> > many UARTs to use as console" isn't standard, and neither
> > is "what hardware handshaking pins are in use".
> 
> When hardware handshaking pins are fixed - it sure is.

Not unless the UART for some odd reason *requires* those pins to work.

There's almost always support for pure software handshaking (XON/XOFF),
with one board option being "don't handshake".  Board A might use two
pins for UART2 RTS/CTS; board B might use UART as well, but use those
two pins for another I2C bus.  The differences belong in board-specific
configuration, not in drivers.


> When they are not (when  
> the hardware doesn't support hardware handshaking, and you need to do it in 
> software) - we still allow you do to it via Kconfig.
> 
> linux-2.6.x/drivers/serial/Kconfig:

That can work, at least for *single-board* kernel builds.  Of course,
that gets into territory some people will say is Kconfig abuse ...
and the need for many ugly #ifdefs is very obvious.  :)

In fact one could argue that those bits of Kconfig syntax are really
just support for one Blackfin board (ezkit), and so they don't belong
in that Kconfig file or with those names...

Plus, that approach only works with fairly simple types of device glue.
It's routine to find chip hookups that can't fit smoothly into some
pre-planned Kconfig, since they require board-specific function hooks.
(Sometimes even with UARTs, but clearly not in this case.)


> Board configs are in one place - under source control - the kernel .config

And in arch/blackfin/mach-*/boards/*.c ... all that stuff you set up
in Kconfig could as easily have been coded in those files, without any
need for #ifdefs or confusing Kconfig.  Still under source control,
plus it's a lot harder to break.  :)


> I guess we thought it was easier for people to select a few things in config, 
> rather than have to write C code/include files for board specific 
> implementations options - It is like you said - everything is all in one 
> place...

Doing that in Kconfig is atypical ... it may well be a bit easier to
pick up at the beginning of a developer's learning curve, but I think
it doesn't scale very well as multi-board product lines evolve.

- Dave

Re: [PATCH 02/12] Blackfin arch: Add label to call new GPIO API

[Robin Getz]

On Sun 19 Aug 2007 17:54, David Brownell pondered:
> On Saturday 18 August 2007, Robin Getz wrote:
> 
> > I don't see how early/late makes the problem easier/worse to debug. No
> > matter when you do it - the driver refuses to install (or at least
> > should). 
> 
> If you arrange to *reliably* detect the pinmux/setup problems by
> the time the system starts ""init" (early), that means one large
> class of hard-to-sort problems never needs runtime troubleshooting.

Sure it does - it just needs to do it in the bootloader, not the kernel. You 
haven't eliminated the problem - or made it any easier to debug - it is just 
moved somewhere else. Again - this is not bad. We decided/are attempting to 
do it in the kernel. Normally what we find is there are more kernel people on 
a project than bootloader (whether this makes this easier - or not - is still 
TDB :)

> Think of it this way:  folk have observed that pin setup issues can
> be painful to sort out. 

Absolutely - the problem that everyone is trying to solve - is how to do plug 
and play, with no enumeration?

Doing it early in the bootloader is akin to historical PC solution where early 
ISA PNP (shudder) filled out a table in memory, and passed it to the OS.

> So they adopt a strategy ("failfast"/"early") 
> which helps surface them early and basically removes them as issues
> in later debugging.

When the kernel engineer runs into a problem because a driver won't load, is 
it better that he can fix it himself (hopefully), or that they have to call 
the person maintaining the bootloader? There are pro/cons of either. I can 
see the value in both. 

We choose to do it in the driver, not the bootloader.

> > Right - for us - the code handing the hardware differences is easier
> > in the drivers, rather than the bootloaders.
> 
> Remember that I didn't argue in favor of putting that code into
> boot loaders ... I just pointed out that some product lines work
> that way, so Linux needs to cope with that strategy.  (One of the
> many examples involves OpenFirmware device tables.)
> 
> But regardless:  I can't buy any argument that it's better to put
> lots of board-specific code into drivers. 

I don't think we are putting board specific code in drivers (or if there is - 
it should get removed).

I did a quick look, and the only place this has happened is in some of our 
drivers that have not made it to main line yet - where we accidently put some 
mtd_partitions in the drivers, rather than the boards file. I know we are 
working on fixing this.

> That adds up quickly, 
> making maintaining the drivers painful.  "Real" updates (bugfixes,
> new features, API updates, cleanup, and so on) regularly end up
> in conflict with patches to support a few more boards, and board
> support patches must then always involve those driver maintainers.
> So merging new boards involves many more people than necessary...

I agree - which is why don't do this either. Board specific info does not go 
into drivers. I think this is something that Michael, Bryan and others 
working on the Blackfin arch & drivers will agree to 100%.

What we do is try to make the driver agnostic to the hardware as much as 
possible, where hardware specifics (chip selects, IRQ, GPIO, etc) are managed 
in Kconfig.

I can see the value of doing the initialisation in the bootloader - since this 
would allow you have a common driver - and hardware customisation is done in 
the bootloader.

>
> > 
> > linux-2.6.x/drivers/serial/Kconfig:
> 
> That can work, at least for *single-board* kernel builds.  Of course,
> that gets into territory some people will say is Kconfig abuse ...
> and the need for many ugly #ifdefs is very obvious.  :)

We have been trying to minimise that - and I think we have been doing a pretty 
good job. There doesn't seem to be any platform specific ifdefs in our 
drivers that are not abstracted out.

> Doing that in Kconfig is atypical ... it may well be a bit easier to
> pick up at the beginning of a developer's learning curve, but I think
> it doesn't scale very well as multi-board product lines evolve.

We have lots of end users (obviously not as many as ARM or PPC yet), and they 
have not been complaining, in fact some say that this is easier to deploy.

But - I think we both agree - that what we are doing is just an alternative 
implemention of hardware abstraction - that is different than the way that 
some others are doing it. Not better/worse (from what I can tell) - it just 
has different tradeoffs.

-Robin

Re: [PATCH 02/12] Blackfin arch: Add label to call new GPIO API

[David Brownell]

On Sunday 19 August 2007, Robin Getz wrote:
> On Sun 19 Aug 2007 17:54, David Brownell pondered:
> > On Saturday 18 August 2007, Robin Getz wrote:
> > 
> > > I don't see how early/late makes the problem easier/worse to debug. No
> > > matter when you do it - the driver refuses to install (or at least
> > > should). 
> > 
> > If you arrange to *reliably* detect the pinmux/setup problems by
> > the time the system starts ""init" (early), that means one large
> > class of hard-to-sort problems never needs runtime troubleshooting.
> 
> Sure it does - it just needs to do it in the bootloader, not the kernel.

Kernel arch/.../board-X.c code is the place to "reliably" handle
that.  Bootloader is out-of-scope here; it's a separate codebase.


> You  
> haven't eliminated the problem - or made it any easier to debug - it is just 
> moved somewhere else.

Moved it out of runtime, to a small window before "init" sections
get discarded and "init" is invoked.  So if it ever *could* show
up, it'll show up then ... every time the system boots, a message
will appear.  Easy to notice while the bug is fresh, just from a
developer's routine scan of bootup messages.

The alternative lets the problem appear later, almost randomly,
based on whether the conflicting drivers happen to be loaded
at the same time.  Which *IS* harder to debug.


I still remember the time a board had to be respun because the
hardware guys goofed on use of one signal.  Two different drivers
both needed it ... but until later in system integration, they
were never tested together, so that conflict never showed up.
(It was a non-obvious thing, for on-chip signal routing not the
on-board type verified by module tests in hardware QA and design
review.)

Doing that setup "early" would have prevented that problem, and
avoided one annoying schedule slip and hardware respin.


> Again - this is not bad. We decided/are attempting to  
> do it in the kernel. Normally what we find is there are more kernel people on 
> a project than bootloader (whether this makes this easier - or not - is still 
> TDB :)

I don't know why you're reading what I write as any kind of
preference for doing that in the bootloader.  It's nothing
more than an *acknowledgement* that some vendors work that
way, so Linux is better off with an approach which won't be
broken when they do.  (That is, always having finished pin
mux setup before drivers get probed.)

When Linux does that setup in arch/..../board-X.c files, mostly
before drivers come into play, it's normal for driver code to
ignore it ... which means the bootloader might also have done
it, for those kinds of product.

Usually I'd expect one person on the bootloader, plus ideally
understudies.  Not a full time job; that person might mostly
do kernel development, hardware test/bringup, or something
else depending on how the product team was structured.


> > Remember that I didn't argue in favor of putting that code into
> > boot loaders ... I just pointed out that some product lines work
> > that way, so Linux needs to cope with that strategy.  (One of the
> > many examples involves OpenFirmware device tables.)
> > 
> > But regardless:  I can't buy any argument that it's better to put
> > lots of board-specific code into drivers. 
> 
> I don't think we are putting board specific code in drivers (or if there is - 
> it should get removed).
> 
> I did a quick look, and the only place this has happened is in some of our 
> drivers that have not made it to main line yet

Good!!  If it's not in drivers, I suspect you'll agree that it
will probably all land in that arch/..../board-X.c setup code.


> I agree - which is why don't do this either. Board specific info does not go 
> into drivers. I think this is something that Michael, Bryan and others 
> working on the Blackfin arch & drivers will agree to 100%.
> 
> What we do is try to make the driver agnostic to the hardware as much as 
> possible,

Good ...

> where hardware specifics (chip selects, IRQ, GPIO, etc) are managed  
> in Kconfig.

... IMO less good, but that's all your worry.  I suspect that
by the time you handle a few dozen boards, you'll find Kconfig
is not well suited to this.  (Because of the way it prevents one
kernel from handling multiple boards, unless they really aren't
very different.)


> > Doing that in Kconfig is atypical ... it may well be a bit easier to
> > pick up at the beginning of a developer's learning curve, but I think
> > it doesn't scale very well as multi-board product lines evolve.
> 
> We have lots of end users (obviously not as many as ARM or PPC yet), and they 
> have not been complaining, in fact some say that this is easier to deploy.

These are people already familiar with ARM or PPC embedded Linux?
I guess one question is "easier than what".

Kconfig doesn't try to help with the "give me a minimal .config
for <THIS> hardware" problem; say, by reading "lspci -n" output
or some other system description.  It expects people to know their
way around.  Maybe a bit of a "20 questions" style interface
comes across as providing useful hand-holding; some of that can
be done in Kconfig, but it's still a kind of menu maze.

I'd hope people don't still get degrees for writing automagic
system configuration tools.  Even though it's still not easy.  :)


> But - I think we both agree - that what we are doing is just an alternative 
> implemention of hardware abstraction - that is different than the way that 
> some others are doing it. Not better/worse (from what I can tell) - it just 
> has different tradeoffs.

We could have that discussion in a few years.  I suspect you'll
find less going into Kconfig and more into C code, if for no
other reason than to make sure things scale sanely.  Or if lots
of stuff is in Kconfig ... it'll be in the arch part of the tree
associated with boards, not the driver parts.

- Dave