Date:	Wed, 19 Aug 2015 09:22:36 +0800
From:	Ley Foon Tan <lftan@...era.com>
To:	Dinh Nguyen <dinh.linux@...il.com>
Cc:	Bjorn Helgaas <bhelgaas@...gle.com>,
	Russell King <linux@....linux.org.uk>,
	Marc Zyngier <marc.zyngier@....com>,
	Mark Rutland <mark.rutland@....com>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	Arnd Bergmann <arnd@...db.de>,
	"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
	linux-pci@...r.kernel.org,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Linux List <linux-kernel@...r.kernel.org>,
	Rob Herring <robh+dt@...nel.org>,
	Pawel Moll <pawel.moll@....com>,
	Kumar Gala <galak@...eaurora.org>,
	Dinh Nguyen <dinguyen@...nsource.altera.com>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v4 2/5] pci:host: Add Altera PCIe host controller driver

On Wed, Aug 19, 2015 at 3:11 AM, Dinh Nguyen <dinh.linux@...il.com> wrote:
>
> On Mon, Aug 17, 2015 at 4:09 AM, Ley Foon Tan <lftan@...era.com> wrote:
> > This patch adds the Altera PCIe host controller driver.
> >
> > Signed-off-by: Ley Foon Tan <lftan@...era.com>
> > ---
> >  drivers/pci/host/Kconfig       |   7 +
> >  drivers/pci/host/Makefile      |   1 +
> >  drivers/pci/host/pcie-altera.c | 543 +++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 551 insertions(+)
> >  create mode 100644 drivers/pci/host/pcie-altera.c
> >
> > diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
> > index 675c2d1..4b4754a 100644
> > --- a/drivers/pci/host/Kconfig
> > +++ b/drivers/pci/host/Kconfig
> > @@ -145,4 +145,11 @@ config PCIE_IPROC_BCMA
> >           Say Y here if you want to use the Broadcom iProc PCIe controller
> >           through the BCMA bus interface
> >
>
> <snip>
>
> > +
> > +/* Address translation table entry size */
> > +#define ATT_ENTRY_SIZE         8
> > +
> > +#define DWORD_MASK             3
> > +
> > +struct altera_pcie {
> > +       struct platform_device  *pdev;
> > +       struct resource         *txs;
>
> You have "Txs" documented in the bindings document, you have a pointer
> here, but you've never used it
> anywhre in the code? What is it for?
Good catch. Forgot to remove this txs field here, we no longer require
to keep this in struct.

>
> > +       void __iomem            *cra_base;
> > +       int                     irq;
> > +       u8                      root_bus_nr;
> > +       struct irq_domain               *irq_domain;
> > +       struct resource         bus_range;
> > +       struct list_head                resources;
> > +};
> > +
>
> <snip>
>
> > +
> > +static int tlp_cfg_dword_read(struct altera_pcie *pcie, u8 bus, u32 devfn,
> > +                             int where, u32 *value)
> > +{
> > +       int ret;
> > +       u32 headers[TLP_HDR_SIZE];
> > +
> > +       if (bus == pcie->root_bus_nr)
> > +               headers[0] = TLP_CFG_DW0(TLP_FMTTYPE_CFGRD0);
> > +       else
> > +               headers[0] = TLP_CFG_DW0(TLP_FMTTYPE_CFGRD1);
> > +
> > +       headers[1] = TLP_CFG_DW1(TLP_REQ_ID(pcie->root_bus_nr, devfn),
> > +                                       TLP_READ_TAG);
> > +       headers[2] = TLP_CFG_DW2(bus, devfn, where);
> > +
> > +       tlp_write_packet(pcie, headers, 0);
> > +
> > +       ret = tlp_read_packet(pcie, value);
> > +       if (ret)
> > +               *value = ~0UL;  /* return 0xFFFFFFFF if error */
> > +
> > +       return ret;
> > +}
> > +
> > +static int tlp_cfg_dword_write(struct altera_pcie *pcie, u8 bus, u32 devfn,
> > +                              int where, u32 value)
> > +{
> > +       u32 headers[TLP_HDR_SIZE];
> > +
> > +       if (bus == pcie->root_bus_nr)
> > +               headers[0] = TLP_CFG_DW0(TLP_FMTTYPE_CFGWR0);
> > +       else
> > +               headers[0] = TLP_CFG_DW0(TLP_FMTTYPE_CFGWR1);
> > +
> > +       headers[1] = TLP_CFG_DW1(TLP_REQ_ID(pcie->root_bus_nr, devfn),
> > +                                       TLP_WRITE_TAG);
> > +       headers[2] = TLP_CFG_DW2(bus, devfn, where);
> > +
> > +       tlp_write_packet(pcie, headers, value);
> > +
> > +       tlp_read_packet(pcie, NULL);
>
> You need to check for the error here.
Okay.

>
> > +
> > +       /* Keep an eye out for changes to the root bus number */
> > +       if ((bus == pcie->root_bus_nr) && (where == PCI_PRIMARY_BUS))
> > +               pcie->root_bus_nr = (u8)(value);
> > +
> > +       return PCIBIOS_SUCCESSFUL;
> > +}
> > +
>
> <snip>
>
> > +
> > +static int altera_pcie_parse_dt(struct altera_pcie *pcie)
> > +{
> > +       struct resource *cra;
> > +       struct platform_device *pdev = pcie->pdev;
> > +
> > +       cra = platform_get_resource_byname(pdev, IORESOURCE_MEM, "Cra");
> > +       if (!cra) {
> > +               cra = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cra");
> > +               if (!cra) {
> > +                       dev_err(&pdev->dev,
> > +                               "no cra memory resource defined\n");
> > +                       return -ENODEV;
> > +               }
> > +       }
> > +
>
> What about "Txs"?
We doesn't need to get resource for "Txs" here. We use standard pci
binding with "ranges" dts parameter to map the pci memory region.
Our dts generator still will generate this "Txs" dts parameter,
because it is one of Avalon slave port of PCIe IP.


Regards
Ley Foon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists