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
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
