IXP4xx: unneeded #include platform-specific include files?

IXP4xx: unneeded #include platform-specific include files?

[Krzysztof Halasa]

I wonder about one more thing: why are the platform-specific include
files included by the non-platform code?

mach-ixp4xx/include/mach/hardware.h:

/* Platform specific details */
#include "ixdp425.h"
#include "avila.h"
#include "coyote.h"
#include "prpmc1100.h"
#include "nslu2.h"
#include "nas100d.h"
#include "dsmg600.h"
#include "fsg.h"

I'm going to move them to the platform code, either incorporate them
into platform .c files or #include "specific-platform.h" from there.

Objections?
-- 
Krzysztof Halasa

IXP4xx: unneeded #include platform-specific include files?

[Mikael Pettersson]

Krzysztof Halasa writes:
 > I wonder about one more thing: why are the platform-specific include
 > files included by the non-platform code?
 > 
 > mach-ixp4xx/include/mach/hardware.h:
 > 
 > /* Platform specific details */
 > #include "ixdp425.h"
 > #include "avila.h"
 > #include "coyote.h"
 > #include "prpmc1100.h"
 > #include "nslu2.h"
 > #include "nas100d.h"
 > #include "dsmg600.h"
 > #include "fsg.h"
 > 
 > I'm going to move them to the platform code, either incorporate them
 > into platform .c files or #include "specific-platform.h" from there.
 > 
 > Objections?

None. Having worked on the Synology DS101 support for mach-ixp4xx
I'm 99,99% certain that platform details can and should be private.

/Mikael

IXP4xx: unneeded #include platform-specific include files?

[Mike Westerhof]

Krzysztof Halasa wrote:
> I wonder about one more thing: why are the platform-specific include
> files included by the non-platform code?
> 
> mach-ixp4xx/include/mach/hardware.h:
> 
> /* Platform specific details */
> #include "ixdp425.h"
> #include "avila.h"
> #include "coyote.h"
> #include "prpmc1100.h"
> #include "nslu2.h"
> #include "nas100d.h"
> #include "dsmg600.h"
> #include "fsg.h"
> 
> I'm going to move them to the platform code, either incorporate them
> into platform .c files or #include "specific-platform.h" from there.
> 
> Objections?

None at all (for nslu2 and dsmg600 platforms).
-Mike (mwester)

IXP4xx: unneeded #include platform-specific include files?

[Imre Kaloz]

On Sun, 15 Nov 2009 19:27:33 +0100, Mikael Pettersson <mikpe@it.uu.se> wrote:

> Krzysztof Halasa writes:
>  > I'm going to move them to the platform code, either incorporate them
>  > into platform .c files or #include "specific-platform.h" from there.
>  >
>  > Objections?
>
> None. Having worked on the Synology DS101 support for mach-ixp4xx
> I'm 99,99% certain that platform details can and should be private.
>

Or people can simply stop the #define brain damage and use the GPIO defines.

(hint: IRQ_IXP4XX_GPIO* for PCI stuff, and the gpio number in i2c_gpio_platform_data)


Imre

IXP4xx: unneeded #include platform-specific include files?

[Krzysztof Halasa]

"Imre Kaloz" <kaloz@openwrt.org> writes:

>> None. Having worked on the Synology DS101 support for mach-ixp4xx
>> I'm 99,99% certain that platform details can and should be private.
>>
>
> Or people can simply stop the #define brain damage and use the GPIO defines.
>
> (hint: IRQ_IXP4XX_GPIO* for PCI stuff, and the gpio number in
> i2c_gpio_platform_data)

I think it's not "or", it's orthogonal to the platforms' private
#defines issue.

Personally I use
#define xgpio_irq(n)            (IRQ_IXP4XX_GPIO ## n)
#define gpio_irq(n)             xgpio_irq(n)
I think we can switch to something similar on other platforms.
-- 
Krzysztof Halasa

IXP4xx: unneeded #include platform-specific include files?

[Imre Kaloz]

On 2009.11.15. 22:14:56 Krzysztof Halasa <khc@pm.waw.pl> wrote:

> "Imre Kaloz" <kaloz@openwrt.org> writes:
>
>>> None. Having worked on the Synology DS101 support for mach-ixp4xx
>>> I'm 99,99% certain that platform details can and should be private.
>>>
>>
>> Or people can simply stop the #define brain damage and use the GPIO defines.
>>
>> (hint: IRQ_IXP4XX_GPIO* for PCI stuff, and the gpio number in
>> i2c_gpio_platform_data)
>
> I think it's not "or", it's orthogonal to the platforms' private
> #defines issue.
>
> Personally I use
> #define xgpio_irq(n)            (IRQ_IXP4XX_GPIO ## n)
> #define gpio_irq(n)             xgpio_irq(n)
> I think we can switch to something similar on other platforms.

Ok, I should be completely missing the point here, but could someone enlighten
me why would the following make sense? (nslu2 example)

#define NSLU2_PCI_INTA_PIN	11
#define NSLU2_PCI_INTB_PIN	10
#define NSLU2_PCI_INTC_PIN	9
#define NSLU2_PCI_INTD_PIN	8

#define        IRQ_NSLU2_PCI_INTA      IRQ_IXP4XX_GPIO11
#define        IRQ_NSLU2_PCI_INTB      IRQ_IXP4XX_GPIO10
#define        IRQ_NSLU2_PCI_INTC      IRQ_IXP4XX_GPIO9

Then do all the "fun" in the setup code, instead of simply using
The already defined (and hardware wise more logical) IRQ_IXP4XX_GPIOx values like
I do in arch/arch/mach-ixp4xx/wg302v2-pci.c for example?


Imre

IXP4xx: unneeded #include platform-specific include files?

[Krzysztof Halasa]

"Imre Kaloz" <kaloz@openwrt.org> writes:

> Ok, I should be completely missing the point here, but could someone enlighten
> me why would the following make sense? (nslu2 example)
>
> #define NSLU2_PCI_INTA_PIN	11
> #define NSLU2_PCI_INTB_PIN	10
> #define NSLU2_PCI_INTC_PIN	9
> #define NSLU2_PCI_INTD_PIN	8
>
> #define        IRQ_NSLU2_PCI_INTA      IRQ_IXP4XX_GPIO11
> #define        IRQ_NSLU2_PCI_INTB      IRQ_IXP4XX_GPIO10
> #define        IRQ_NSLU2_PCI_INTC      IRQ_IXP4XX_GPIO9

Well, this isn't something I really like because it duplicates the
assignments.

> Then do all the "fun" in the setup code, instead of simply using
> The already defined (and hardware wise more logical) IRQ_IXP4XX_GPIOx values like
> I do in arch/arch/mach-ixp4xx/wg302v2-pci.c for example?

void __init wg302v2_pci_preinit(void)
{
        set_irq_type(IRQ_IXP4XX_GPIO8, IRQ_TYPE_LEVEL_LOW);
        set_irq_type(IRQ_IXP4XX_GPIO9, IRQ_TYPE_LEVEL_LOW);

        ixp4xx_pci_preinit();
}

static int __init wg302v2_map_irq(struct pci_dev *dev, u8 slot, u8 pin)
{
        if (slot == 1)
                return IRQ_IXP4XX_GPIO8;
        else if (slot == 2)
                return IRQ_IXP4XX_GPIO9;
        else return -1;
}

Not very different from what other platforms have, except that you have
to remember the assignment. When the code gets more complicated you
really don't want to remember that INTA is GPIO8, INTB is GPIO9, INTC,
INTD, IDE_IRQ, USB_IRQ etc. And this gets even more complicated when the
platform code supports more than one version of hw.
-- 
Krzysztof Halasa

IXP4xx: unneeded #include platform-specific include files?

[Imre Kaloz]

On 2009.11.17. 22:53:38 Krzysztof Halasa <khc@pm.waw.pl> wrote:

> "Imre Kaloz" <kaloz@openwrt.org> writes:
>
>> Ok, I should be completely missing the point here, but could someone enlighten
>> me why would the following make sense? (nslu2 example)
>>
>> #define NSLU2_PCI_INTA_PIN	11
>> #define NSLU2_PCI_INTB_PIN	10
>> #define NSLU2_PCI_INTC_PIN	9
>> #define NSLU2_PCI_INTD_PIN	8
>>
>> #define        IRQ_NSLU2_PCI_INTA      IRQ_IXP4XX_GPIO11
>> #define        IRQ_NSLU2_PCI_INTB      IRQ_IXP4XX_GPIO10
>> #define        IRQ_NSLU2_PCI_INTC      IRQ_IXP4XX_GPIO9
>
> Well, this isn't something I really like because it duplicates the
> assignments.
>
>> Then do all the "fun" in the setup code, instead of simply using
>> The already defined (and hardware wise more logical) IRQ_IXP4XX_GPIOx values like
>> I do in arch/arch/mach-ixp4xx/wg302v2-pci.c for example?
>
> void __init wg302v2_pci_preinit(void)
> {
>         set_irq_type(IRQ_IXP4XX_GPIO8, IRQ_TYPE_LEVEL_LOW);
>         set_irq_type(IRQ_IXP4XX_GPIO9, IRQ_TYPE_LEVEL_LOW);
>
>         ixp4xx_pci_preinit();
> }
>
> static int __init wg302v2_map_irq(struct pci_dev *dev, u8 slot, u8 pin)
> {
>         if (slot == 1)
>                 return IRQ_IXP4XX_GPIO8;
>         else if (slot == 2)
>                 return IRQ_IXP4XX_GPIO9;
>         else return -1;
> }
>
> Not very different from what other platforms have, except that you have
> to remember the assignment. When the code gets more complicated you
> really don't want to remember that INTA is GPIO8, INTB is GPIO9, INTC,
> INTD, IDE_IRQ, USB_IRQ etc. And this gets even more complicated when the
> platform code supports more than one version of hw.

Why wouldn't you want to remember that "INTA" was "GPIO8"? When you want to
touch those lines you do have to remember or look up what GPIO was that one.
So this hardly makes it easier to work with or to remember to -- it certainly
duplicates the assignments, as you've also noted.

The INTx naming convention is also stupid, as in most cases these are not different
INT lines of the PCI slot but either separate slots or subdevices -- and note,
most of the boards only have INTa connected mechanically.

And I didn't even get to the topic of *_PCI_MAX_DEV, *_PCI_IRQ_LINES and the mania
of pci_irq_table usage..


Imre

IXP4xx: unneeded #include platform-specific include files?

[Krzysztof Halasa]

"Imre Kaloz" <kaloz@openwrt.org> writes:

> Why wouldn't you want to remember that "INTA" was "GPIO8"? When you want to
> touch those lines you do have to remember or look up what GPIO was that one.
> So this hardly makes it easier to work with or to remember to -- it certainly
> duplicates the assignments, as you've also noted.

Actually I'm not sure if I understand you correctly.

If I use GPIO8 (presumably in different places) I have to remember what
it is. This may be use with a couple of IRQ lines but gets harder and
harder with a more complicated machine. Macros like SLOT0_INTA etc.
don't have this problem. That's BTW why macros and symbolic names were
invented.

> The INTx naming convention is also stupid, as in most cases these are
> not different
> INT lines of the PCI slot but either separate slots or subdevices -- and note,
> most of the boards only have INTa connected mechanically.
>
> And I didn't even get to the topic of *_PCI_MAX_DEV, *_PCI_IRQ_LINES
> and the mania
> of pci_irq_table usage..

But you've seen the patches I just posted, haven't you?
-- 
Krzysztof Halasa

IXP4xx: unneeded #include platform-specific include files?

[Imre Kaloz]

On 2009.11.17. 23:22:21 Krzysztof Halasa <khc@pm.waw.pl> wrote:

> "Imre Kaloz" <kaloz@openwrt.org> writes:
>
>> Why wouldn't you want to remember that "INTA" was "GPIO8"? When you want to
>> touch those lines you do have to remember or look up what GPIO was that one.
>> So this hardly makes it easier to work with or to remember to -- it certainly
>> duplicates the assignments, as you've also noted.
>
> Actually I'm not sure if I understand you correctly.
>
> If I use GPIO8 (presumably in different places) I have to remember what
> it is. This may be use with a couple of IRQ lines but gets harder and
> harder with a more complicated machine. Macros like SLOT0_INTA etc.
> don't have this problem. That's BTW why macros and symbolic names were
> invented.

I doubt they were invented for "#define a b" "#define b c" "#define c d" games.

>> The INTx naming convention is also stupid, as in most cases these are
>> not different
>> INT lines of the PCI slot but either separate slots or subdevices -- and note,
>> most of the boards only have INTa connected mechanically.
>>
>> And I didn't even get to the topic of *_PCI_MAX_DEV, *_PCI_IRQ_LINES
>> and the mania
>> of pci_irq_table usage..
>
> But you've seen the patches I just posted, haven't you?

Yeah, you get rid of the external .h files for dropping all that bloat into the setup
files. Honestly, do however you please, but this hardly makes sense IMHO when you have
the chance to get rid of all this mess.


Imre

IXP4xx: unneeded #include platform-specific include files?

[Krzysztof Halasa]

"Imre Kaloz" <kaloz@openwrt.org> writes:

> Yeah, you get rid of the external .h files for dropping all that bloat
> into the setup
> files. Honestly, do however you please, but this hardly makes sense
> IMHO when you have
> the chance to get rid of all this mess.

What I'm thinking about is (cut & paste):

--- a/arch/arm/mach-ixp4xx/avila-pci.c
+++ b/arch/arm/mach-ixp4xx/avila-pci.c
@@ -27,49 +27,40 @@
 #include <mach/hardware.h>
 #include <asm/mach-types.h>
 
-#define AVILA_PCI_MAX_DEV      4
-#define LOFT_PCI_MAX_DEV       6
-#define AVILA_PCI_IRQ_LINES    4
+#define AVILA_MAX_DEV  4
+#define LOFT_MAX_DEV   6
+#define IRQ_LINES      4
 
 /* PCI controller GPIO to IRQ pin mappings */
-#define AVILA_PCI_INTA_PIN     11
-#define AVILA_PCI_INTB_PIN     10
-#define AVILA_PCI_INTC_PIN     9
-#define AVILA_PCI_INTD_PIN     8
-
-#define IRQ_AVILA_PCI_INTA     IRQ_IXP4XX_GPIO11
-#define IRQ_AVILA_PCI_INTB     IRQ_IXP4XX_GPIO10
-#define IRQ_AVILA_PCI_INTC     IRQ_IXP4XX_GPIO9
-#define IRQ_AVILA_PCI_INTD     IRQ_IXP4XX_GPIO8
+#define INTA           11
+#define INTB           10
+#define INTC           9
+#define INTD           8

You can see the duplicate definitions removed.
BTW: I don't like it much, but using ordinary GPIOx certainly
isn't better.

(Yep, I see at least one bug which the patches should fix, will do).

 void __init avila_pci_preinit(void)
 {
-       set_irq_type(IRQ_AVILA_PCI_INTA, IRQ_TYPE_LEVEL_LOW);
-       set_irq_type(IRQ_AVILA_PCI_INTB, IRQ_TYPE_LEVEL_LOW);
-       set_irq_type(IRQ_AVILA_PCI_INTC, IRQ_TYPE_LEVEL_LOW);
-       set_irq_type(IRQ_AVILA_PCI_INTD, IRQ_TYPE_LEVEL_LOW);
-
+       set_irq_type(IXP4XX_GPIO_IRQ(INTA), IRQ_TYPE_LEVEL_LOW);
+       set_irq_type(IXP4XX_GPIO_IRQ(INTB), IRQ_TYPE_LEVEL_LOW);
+       set_irq_type(IXP4XX_GPIO_IRQ(INTC), IRQ_TYPE_LEVEL_LOW);
+       set_irq_type(IXP4XX_GPIO_IRQ(INTD), IRQ_TYPE_LEVEL_LOW);
        ixp4xx_pci_preinit();
 }
-- 
Krzysztof Halasa

IXP4xx: unneeded #include platform-specific include files?

[Imre Kaloz]

On 2009.11.17. 23:47:09 Krzysztof Halasa <khc@pm.waw.pl> wrote:

> "Imre Kaloz" <kaloz@openwrt.org> writes:
>
>> Yeah, you get rid of the external .h files for dropping all that bloat
>> into the setup
>> files. Honestly, do however you please, but this hardly makes sense
>> IMHO when you have
>> the chance to get rid of all this mess.
>
> What I'm thinking about is (cut & paste):
>
> --- a/arch/arm/mach-ixp4xx/avila-pci.c
> +++ b/arch/arm/mach-ixp4xx/avila-pci.c
> @@ -27,49 +27,40 @@
>  #include <mach/hardware.h>
>  #include <asm/mach-types.h>
>-#define AVILA_PCI_MAX_DEV      4
> -#define LOFT_PCI_MAX_DEV       6
> -#define AVILA_PCI_IRQ_LINES    4
> +#define AVILA_MAX_DEV  4
> +#define LOFT_MAX_DEV   6
> +#define IRQ_LINES      4
> /* PCI controller GPIO to IRQ pin mappings */
> -#define AVILA_PCI_INTA_PIN     11
> -#define AVILA_PCI_INTB_PIN     10
> -#define AVILA_PCI_INTC_PIN     9
> -#define AVILA_PCI_INTD_PIN     8
> -
> -#define IRQ_AVILA_PCI_INTA     IRQ_IXP4XX_GPIO11
> -#define IRQ_AVILA_PCI_INTB     IRQ_IXP4XX_GPIO10
> -#define IRQ_AVILA_PCI_INTC     IRQ_IXP4XX_GPIO9
> -#define IRQ_AVILA_PCI_INTD     IRQ_IXP4XX_GPIO8
> +#define INTA           11
> +#define INTB           10
> +#define INTC           9
> +#define INTD           8
>
> You can see the duplicate definitions removed.
> BTW: I don't like it much, but using ordinary GPIOx certainly
> isn't better.
>
> (Yep, I see at least one bug which the patches should fix, will do).
>
>  void __init avila_pci_preinit(void)
>  {
> -       set_irq_type(IRQ_AVILA_PCI_INTA, IRQ_TYPE_LEVEL_LOW);
> -       set_irq_type(IRQ_AVILA_PCI_INTB, IRQ_TYPE_LEVEL_LOW);
> -       set_irq_type(IRQ_AVILA_PCI_INTC, IRQ_TYPE_LEVEL_LOW);
> -       set_irq_type(IRQ_AVILA_PCI_INTD, IRQ_TYPE_LEVEL_LOW);
> -
> +       set_irq_type(IXP4XX_GPIO_IRQ(INTA), IRQ_TYPE_LEVEL_LOW);
> +       set_irq_type(IXP4XX_GPIO_IRQ(INTB), IRQ_TYPE_LEVEL_LOW);
> +       set_irq_type(IXP4XX_GPIO_IRQ(INTC), IRQ_TYPE_LEVEL_LOW);
> +       set_irq_type(IXP4XX_GPIO_IRQ(INTD), IRQ_TYPE_LEVEL_LOW);
>         ixp4xx_pci_preinit();
>  }


This is far more tolerable, but SLOT1-3 would be better. Also, the
_MAX thingie should be dropped, for example the LOFT is nothing more
then an avila with an USB2 controller on the same IRQ as slot 3, where
the USB1.1 subdevices will show up as well. Those subdevices won't show
up on a board without the USB controller, but all this will blow up when
someone plugs a miniPCI USB controller into one of the slots, not to
mention if he/she plugs in more ;)

Now the best question: do those PCI subdevices need an IRQ? If there is only
one IRQ connected to a PCI slot (probably), does it make sense to assign
IRQs/GPIOs without any effect?


Imre

IXP4xx: unneeded #include platform-specific include files?

[Krzysztof Halasa]

"Imre Kaloz" <kaloz@openwrt.org> writes:

> This is far more tolerable, but SLOT1-3 would be better. Also, the
> _MAX thingie should be dropped, for example the LOFT is nothing more
> then an avila with an USB2 controller on the same IRQ as slot 3, where
> the USB1.1 subdevices will show up as well.

I'd prefer that patches like that, changing the actual behaviour of the
code, are done and especially are tested by people with access to the
actual hardware. I don't know if it's ok to assign an IRQ when there is
nothing to assign. If both have the same PCB, well, perhaps it's the way
to go, but I've never seen those boards.

> Those subdevices won't show
> up on a board without the USB controller, but all this will blow up when
> someone plugs a miniPCI USB controller into one of the slots, not to
> mention if he/she plugs in more ;)

Well... Why would it blow up?
A mini-PCI card should use the same IRQ routing, except that it can't
use more than two INT lines. If the INTA/B/C/D lines are rotated
correctly on board and set up in the PCI config space, it should work
fine.

I'm not sure about a mini-PCI card pretending to use more than two INTx
lines. Probably the "pin <= IRQ_LINES" should be changed to
"pin <= 2" for mini-PCI slots, or maybe % 2 should be used. Can't see
anything about that in the standard and personally I don't have such
card.

I didn't know Avila and Loft are based on mini-PCI, and which devices
exactly are mini-PCI slots.

> Now the best question: do those PCI subdevices need an IRQ?

Most probably.

> If there is only
> one IRQ connected to a PCI slot (probably), does it make sense to assign
> IRQs/GPIOs without any effect?

I don't know how are the lines connected to the slots. The mini-PCI slot
has physically 2 INTx lines. The code suggests that the lines are
rotated, that would mean a slot gets two INTx lines. Yes, that means we
shouldn't assign INTC and INTD (in slot's numbering) to such devices,
but I'm not going to make such changes blindly.

If both lines are connected together, both (or all four) "pins" should
get the same INTx.
-- 
Krzysztof Halasa