Re: [PATCH] [POWERPC] 4xx: Add endpoint support to 4xx PCIe driver

[Stefan Roese <sr@denx.de>]

On Thursday 10 April 2008, Benjamin Herrenschmidt wrote:
> On Wed, 2008-04-02 at 17:12 +0200, Stefan Roese wrote:
>
>    ../..
>
> Overall looks good, just a few things:

Ben, thanks for the review.

> In general, while I have nothing against the idea of reading the HW
> setup left by uboot, I wonder if it wouldn't be best to have this file
> capable of fully configuring it in either mode based on DT properties
> instead.

Sure, this would be optimal. But frankly, I currently have no need for this 
non U-Boot mode, and therefore I didn't implement it. This can be added later 
when really needed don't you think?

> The PCI node in endpoint mode would be called something 
> different, such as "pci-endpoint" and would contain some kind of
> "endpoint-mode" property, maybe ? That way people using other
> bootloaders or even booting of straight flash kernels can still use
> this.

Good. But again, I would really prefer to first include this U-Boot style 
endpoint support and later add this independent device tree endpoint 
configuration when really needed.

> > +
> > +               out_le32(mbase + PCI_BASE_ADDRESS_0,
> > RES_TO_U32_LOW(res->start)); +               out_le32(mbase +
> > PCI_BASE_ADDRESS_1, RES_TO_U32_HIGH(res->start)); +       }
> >
> >         /* Enable inbound mapping */
> >         out_le32(mbase + PECFG_PIMEN, 0x1);
> >
> > -       out_le32(mbase + PCI_BASE_ADDRESS_0, RES_TO_U32_LOW(res->start));
> > -       out_le32(mbase + PCI_BASE_ADDRESS_1,
> > RES_TO_U32_HIGH(res->start)); -
>
> does it work properly to setup the BARs before enabling the inbound
> mapping ?

Yes. At least I have seen no problems so far.

> >          * OMRs are already reset, also disable PIMs
> > @@ -1531,14 +1569,26 @@ static void __init
> > ppc4xx_pciex_port_setup_hose(struct ppc4xx_pciex_port *port) * and device
> > IDs into it. Those are the same bogus one that the * initial code in
> > arch/ppc add. We might want to change that. */
> > -       out_le16(mbase + 0x200, 0xaaa0 + port->index);
> > -       out_le16(mbase + 0x202, 0xbed0 + port->index);
> > +       if (!port->endpoint) {
> > +               out_le16(mbase + 0x200, 0xaaa0 + port->index);
> > +               out_le16(mbase + 0x202, 0xbed0 + port->index);
>
> We should probably setup the config space IDs based on some device-tree
> properties no ?

Good idea. How about "vendor-id" and "device-id"? And we probably need some 
for endpoint mode too now. "vendor-id-endpoint" and "device-id-endpoint"?

And how should we handle the backward compatibility? Should I set the old 
values as default when those properties are not available?

Thanks.

Best regards,
Stefan