On Monday, December 23, 2013 5:02 PM, Tanmay Inamdar wrote:
>
> This patch adds the AppliedMicro X-gene SOC PCIe controller driver.
> APM X-Gene PCIe controller supports maximum upto 8 lanes and
> GEN3 speed. X-Gene has maximum 5 PCIe ports supported.
(+cc Jason Gunthorpe, Arnd Bergmann)
Hi Tanmay Inamdar,
I added some minor comments. :-)
>
> Signed-off-by: Tanmay Inamdar <[email protected]>
> ---
> drivers/pci/host/Kconfig | 5 +
> drivers/pci/host/Makefile | 1 +
> drivers/pci/host/pcie-xgene.c | 1017 +++++++++++++++++++++++++++++++++++++++++
Would you change the file name to 'pci-xgene.c'?
Now, all PCI host drivers are using the prefix 'pci-', not 'pcie-'.
[.....]
> +#include <linux/module.h>
> +#include <linux/delay.h>
> +#include <linux/pci.h>
> +#include <linux/slab.h>
> +#include <linux/memblock.h>
> +#include <linux/io.h>
> +#include <linux/platform_device.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_pci.h>
> +#include <linux/jiffies.h>
> +#include <linux/clk-private.h>
Would you re-order these headers alphabetically?
It enhances the readability.
[.....]
> +static int xgene_pcie_parse_map_ranges(struct xgene_pcie_port *port)
> +{
> + struct device_node *np = port->node;
> + struct of_pci_range range;
> + struct of_pci_range_parser parser;
> + struct device *dev = port->dev;
> + u32 cfg_map_done = 0;
> + int ret;
> +
> + if (of_pci_range_parser_init(&parser, np)) {
> + dev_err(dev, "missing ranges property\n");
> + return -EINVAL;
> + }
> +
> + /* Get the I/O, memory, config ranges from DT */
> + for_each_of_pci_range(&parser, &range) {
> + struct resource *res = NULL;
> + u64 restype = range.flags & IORESOURCE_TYPE_BITS;
> + u64 end = range.cpu_addr + range.size - 1;
> + dev_dbg(port->dev, "0x%08x 0x%016llx..0x%016llx -> 0x%016llx\n",
> + range.flags, range.cpu_addr, end, range.pci_addr);
> +
> + switch (restype) {
> + case IORESOURCE_IO:
> + res = &port->res[XGENE_IO];
> + of_pci_range_to_resource(&range, np, res);
> + xgene_pcie_setup_ob_reg(port->csr_base, OMR1BARL,
> + XGENE_IO, res);
> + break;
> + case IORESOURCE_MEM:
> + res = &port->res[XGENE_MEM];
> + of_pci_range_to_resource(&range, np, res);
> + xgene_pcie_setup_ob_reg(port->csr_base, OMR2BARL,
> + XGENE_MEM, res);
> + break;
> + case 0:
> + if (!cfg_map_done) {
> + /* config region */
> + if (port->type == PTYPE_ROOT_PORT) {
> + ret = xgene_pcie_map_cfg(port, &range);
> + if (ret)
> + return ret;
> + }
> + cfg_map_done = 1;
> + } else {
> + /* msi region */
> + res = &port->res[XGENE_MSI];
> + of_pci_range_to_resource(&range, np, res);
As Jason Gunthorpe said, the DT 'range' property should not
handle MSI. Please refer to other PCI host drivers such as
pci-mvebu.c, pci-tegra.c and pcie-designware.c.
Currently, 'struct msi_chip', ' struct irq_domain' are used
for implementing MSI feature.
Best regards,
Jingoo Han
On Sun, Jan 5, 2014 at 5:47 PM, Jingoo Han <[email protected]> wrote:
> On Monday, December 23, 2013 5:02 PM, Tanmay Inamdar wrote:
>>
>> This patch adds the AppliedMicro X-gene SOC PCIe controller driver.
>> APM X-Gene PCIe controller supports maximum upto 8 lanes and
>> GEN3 speed. X-Gene has maximum 5 PCIe ports supported.
>
> (+cc Jason Gunthorpe, Arnd Bergmann)
>
> Hi Tanmay Inamdar,
>
> I added some minor comments. :-)
>
>>
>> Signed-off-by: Tanmay Inamdar <[email protected]>
>> ---
>> drivers/pci/host/Kconfig | 5 +
>> drivers/pci/host/Makefile | 1 +
>> drivers/pci/host/pcie-xgene.c | 1017 +++++++++++++++++++++++++++++++++++++++++
>
> Would you change the file name to 'pci-xgene.c'?
> Now, all PCI host drivers are using the prefix 'pci-', not 'pcie-'.
I guess designware is an exception. There is
"drivers/pci/host/pcie-designware.c"
>
> [.....]
>
>> +#include <linux/module.h>
>> +#include <linux/delay.h>
>> +#include <linux/pci.h>
>> +#include <linux/slab.h>
>> +#include <linux/memblock.h>
>> +#include <linux/io.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/of.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/of_pci.h>
>> +#include <linux/jiffies.h>
>> +#include <linux/clk-private.h>
>
> Would you re-order these headers alphabetically?
> It enhances the readability.
ok.
>
> [.....]
>
>> +static int xgene_pcie_parse_map_ranges(struct xgene_pcie_port *port)
>> +{
>> + struct device_node *np = port->node;
>> + struct of_pci_range range;
>> + struct of_pci_range_parser parser;
>> + struct device *dev = port->dev;
>> + u32 cfg_map_done = 0;
>> + int ret;
>> +
>> + if (of_pci_range_parser_init(&parser, np)) {
>> + dev_err(dev, "missing ranges property\n");
>> + return -EINVAL;
>> + }
>> +
>> + /* Get the I/O, memory, config ranges from DT */
>> + for_each_of_pci_range(&parser, &range) {
>> + struct resource *res = NULL;
>> + u64 restype = range.flags & IORESOURCE_TYPE_BITS;
>> + u64 end = range.cpu_addr + range.size - 1;
>> + dev_dbg(port->dev, "0x%08x 0x%016llx..0x%016llx -> 0x%016llx\n",
>> + range.flags, range.cpu_addr, end, range.pci_addr);
>> +
>> + switch (restype) {
>> + case IORESOURCE_IO:
>> + res = &port->res[XGENE_IO];
>> + of_pci_range_to_resource(&range, np, res);
>> + xgene_pcie_setup_ob_reg(port->csr_base, OMR1BARL,
>> + XGENE_IO, res);
>> + break;
>> + case IORESOURCE_MEM:
>> + res = &port->res[XGENE_MEM];
>> + of_pci_range_to_resource(&range, np, res);
>> + xgene_pcie_setup_ob_reg(port->csr_base, OMR2BARL,
>> + XGENE_MEM, res);
>> + break;
>> + case 0:
>> + if (!cfg_map_done) {
>> + /* config region */
>> + if (port->type == PTYPE_ROOT_PORT) {
>> + ret = xgene_pcie_map_cfg(port, &range);
>> + if (ret)
>> + return ret;
>> + }
>> + cfg_map_done = 1;
>> + } else {
>> + /* msi region */
>> + res = &port->res[XGENE_MSI];
>> + of_pci_range_to_resource(&range, np, res);
>
> As Jason Gunthorpe said, the DT 'range' property should not
> handle MSI. Please refer to other PCI host drivers such as
> pci-mvebu.c, pci-tegra.c and pcie-designware.c.
Right. I will remove MSI from ranges.
>
> Currently, 'struct msi_chip', ' struct irq_domain' are used
> for implementing MSI feature.
>
> Best regards,
> Jingoo Han
>
On Tuesday, January 07, 2014 11:45 AM, Tanmay Inamdar wrote:
> On Sun, Jan 5, 2014 at 5:47 PM, Jingoo Han <[email protected]> wrote:
> > On Monday, December 23, 2013 5:02 PM, Tanmay Inamdar wrote:
> >>
> >> This patch adds the AppliedMicro X-gene SOC PCIe controller driver.
> >> APM X-Gene PCIe controller supports maximum upto 8 lanes and
> >> GEN3 speed. X-Gene has maximum 5 PCIe ports supported.
> >
> > (+cc Jason Gunthorpe, Arnd Bergmann)
> >
> > Hi Tanmay Inamdar,
> >
> > I added some minor comments. :-)
> >
> >>
> >> Signed-off-by: Tanmay Inamdar <[email protected]>
> >> ---
> >> drivers/pci/host/Kconfig | 5 +
> >> drivers/pci/host/Makefile | 1 +
> >> drivers/pci/host/pcie-xgene.c | 1017 +++++++++++++++++++++++++++++++++++++++++
> >
> > Would you change the file name to 'pci-xgene.c'?
> > Now, all PCI host drivers are using the prefix 'pci-', not 'pcie-'.
>
> I guess designware is an exception. There is
> "drivers/pci/host/pcie-designware.c"
(+cc Thierry Reding, Pratyush Anand, Mohit KUMAR)
Now, the current naming rule is "PCI-" prefix as below.
- Samsung Exynos: "pci"-exynos.c
- Freescale i.MX6: "pci"-imx6.c
- Marvell: pci-mvebu.c
- Nvidia Tegra: pci-tegra.c
- Renesas R-Car: pci-rcar-gen2.c
According to the Thierry Reding's comment,
"I think we should keep these sorted alphabetically. Also Tegra and
Marvell are PCIe controllers but they still use the pci- prefix instead
of pcie-. Perhaps it'd be good to keep consistency here? I initially
chose pci- because from a software point of view it doesn't matter all
that much whether it's PCI or PCIe and because the drivers are part of
the PCI subsystem. However if Exynos now uses the pcie- prefix it makes
it look like Tegra and Marvell are plain old PCI."
(https://groups.google.com/forum/#!msg/linux.kernel/qtimJoNSc3w/_1aayHaG54YJ)
However, "pcie-designware.c" is common layer driver for other
SoC PCI host drivers that use Synopsys Designware PCI IP.
Thus, currently it is shared by other SoC PCI host drivers
such as pci-exynos.c, and pci-imx6.c. Also, ST PCI driver will
use pcie-designware.c as common layer.
Originally, "pci"-designware.c was used. However, Pratyush Anand
suggested "pci"-designware.c can be renamed to "pcie"-designware.c,
because Synopsys PCI IP and Synopsys PCI Express IP are different.
So, currently "pcie"-designware.c is used.
So, if there is no special reason, "pci-" prefix can be used.
Thank you.
Best regards,
Jingoo Han