Subject: [PATCH v2 3/3] PCI: Layerscape: Add Layerscape PCIe driver

From: [email-masked] (Lian Minghuan-B31939)
Date: Tue, 16 Sep 2014 17:38:24 +0000

On 2014?09?16? 04:19, Scott Wood wrote:
> On Fri, 2014-09-12 at 11:10 +0000, Lian Minghuan-B31939 wrote:
>> Hi Scott,
>>
>> Please see my comments inline.
>>
>> On 2014?09?11? 21:24, Scott Wood wrote:
>>> On Thu, 2014-09-11 at 21:09 +0000, Minghuan Lian wrote:
>>>> Add support for Freescale Layerscape PCIe controller. This driver
>>>> re-uses the designware core code.
>>>>
>>>> Signed-off-by: Minghuan Lian <[email-masked]>
>>>> ---
>>>> Change log:
>>>> v2:
>>>> 1. Add pcie-scfg to link scfg device node and remove "fsl, ls-pcie" compatible
>>>> 2. Use regmap API to access scfg.
>>>> 3. remove ls1021 PCI device ID.
>>>> 4. remove unused irq pme_irq and ls_pcie_list.
>>>> 5. Do not set scfg bit reverse mode
>>>>
>>>>    .../devicetree/bindings/pci/layerscape-pci.txt     |  45 ++++
>>>>    drivers/pci/host/Kconfig                           |   8 +
>>>>    drivers/pci/host/Makefile                          |   1 +
>>>>    drivers/pci/host/pci-layerscape.c                  | 278 +++++++++++++++++++++
>>>>    4 files changed, 332 insertions(+)
>>>>    create mode 100644 Documentation/devicetree/bindings/pci/layerscape-pci.txt
>>>>    create mode 100644 drivers/pci/host/pci-layerscape.c
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/pci/layerscape-pci.txt b/Documentation/devicetree/bindings/pci/layerscape-pci.txt
>>>> new file mode 100644
>>>> index 0000000..1a5dbd8
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/pci/layerscape-pci.txt
>>>> @@ -0,0 +1,45 @@
>>>> +Freescale Layerscape PCIe controller
>>>> +
>>>> +This PCIe host controller is based on the Synopsis Designware PCIe IP
>>>> +and thus inherits all the common properties defined in designware-pcie.txt.
>>>> +
>>>> +Required properties:
>>>> +- compatible: should contain the platform identifier such as "fsl,ls1021a-pcie"
>>>> +- reg: base addresses and lengths of the PCIe controller
>>>> +- interrupts: A list of interrupt outputs of the controller. Must contain an
>>>> +  entry for each entry in the interrupt-names property.
>>>> +- interrupt-names: Must include the following entries:
>>>> +  "intr": The interrupt that is asserted for controller interrupts
>>>> +  "msi": The interrupt that is asserted when an MSI is received
>>>> +  "pme": The interrupt that is asserted when PME state changes
>>>> +- pcie-scfg: Must include two entries.
>>> fsl,pcie-scfg
>> [Minghuan] Ok. I will rename the property.
>>>> +  The first entry must be a link to the SCFG device node
>>>> +  The second entry must be '0' or '1' based on physical PCIe controller index.
>>>> +  used to get SCFG PEXN registers
>>>> +
>>>> +Example:
>>>> +
>>>> +	pcie at 3400000 {
>>>> +		compatible = "fsl,ls1021a-pcie", "snps,dw-pcie";
>>> Can we rely on the config space vendor/device ID instead of hardcoding
>>> the SoC here?
>> [Minghuan] No, for a SoC may have several device ID.
>> For example:
>> LS1021A RM says :
>> 0x0E0A LS1021A with security
>> 0x0E0B LS1021A without security
>> But I read device ID 0x0e00  from some LS1021A boards.
>> 0x0e06 from some LS1021A boards. this may be caused by different CPLD.
> :-(
>
>>>> +		reg = <0x00 0x03400000 0x0 0x00010000   /* controller registers */
>>>> +		       0x40 0x00000000 0x0 0x00002000>; /* configuration space */
>>>> +		reg-names = "regs", "config";
>>>> +		interrupts = <GIC_SPI 177 IRQ_TYPE_LEVEL_HIGH>, /* controller interrupt */
>>>> +			     <GIC_SPI 179 IRQ_TYPE_LEVEL_HIGH>, /* MSI interrupt */
>>>> +			     <GIC_SPI 181 IRQ_TYPE_LEVEL_HIGH>; /* PME interrupt */
>>>> +		interrupt-names = "intr", "msi", "pme";
>>>> +		pcie-scfg = <&scfg 0>;
>>>> +		#address-cells = <3>;
>>>> +		#size-cells = <2>;
>>>> +		device_type = "pci";
>>>> +		num-lanes = <4>;
>>>> +		bus-range = <0x00 0xff>;
>>>> +		ranges = <0x81000000 0x0 0x00000000 0x40 0x10000000 0x0 0x00010000   /* downstream I/O */
>>>> +			  0x82000000 0x0 0x00000000 0x41 0x00000000 0x1 0x00000000>; /* non-prefetchable memory */
>>> Are these ranges hardcoded in the SoC, or are they the result of iATU
>>> settings?  If the latter, who configures it and why no prefetchable
>>> region?
>> [Minghuan] 400000_0000 - 480000_0000 is hardcode assigned to PEX1.
>> I separates from this 32 region  1M for IO, 4G for non-prefetchable memory.
>> 4G is the max size iATU supported.
>> IO and memory region will be set to iATU by pci-designware.c
>> Because both powerpc and imx do not set prefechable memory,
>> so  I do not assign prefetchable memory either.
> If there's spare room in the addres space for a prefetchable region, why
> not make one, regardless of what PPC and IMX do?
>
> FWIW, I believe that ARMv8 can make better use of a prefetchable region
> due to the "gathering" storage attribute, so even if you don't use one
> on LS1021A consider using one on ARMv8-based LS chips.
[Minghuan] Ok, I will add 4G prefetchable memory region.
>>>> +		#interrupt-cells = <1>;
>>>> +		interrupt-map-mask = <0 0 0 7>;
>>>> +		interrupt-map = <0000 0 0 1 &gic GIC_SPI 91  IRQ_TYPE_LEVEL_HIGH>,
>>>> +				<0000 0 0 2 &gic GIC_SPI 188 IRQ_TYPE_LEVEL_HIGH>,
>>>> +				<0000 0 0 3 &gic GIC_SPI 190 IRQ_TYPE_LEVEL_HIGH>,
>>>> +				<0000 0 0 4 &gic GIC_SPI 192 IRQ_TYPE_LEVEL_HIGH>;
>>>> +	};
>>>> diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
>>>> index 34134d6..c826949 100644
>>>> --- a/drivers/pci/host/Kconfig
>>>> +++ b/drivers/pci/host/Kconfig
>>>> @@ -82,4 +82,12 @@ config PCIE_XILINX
>>>>    	  Say 'Y' here if you want kernel to support the Xilinx AXI PCIe
>>>>    	  Host Bridge driver.
>>>>    
>>>> +config PCI_LAYERSCAPE
>>>> +	bool "Freescale Layerscape PCIe controller"
>>>> +	depends on SOC_LS1021A
>>>> +	select PCIE_DW
>>>> +	select MFD_SYSCON
>>>> +	help
>>>> +	  Say Y here if you want PCIe controller support on Layerscape SoCs.
>>> Why does it depend on this particular SoC?  Is the same PCIe controller
>>> going to be used on other Layerscape chips?
>> [Minghuan] LS1021A platform patch only contains SOC name no ARCH name.
>> Both LS1042 and LS2085 RM do not contain PCI chapter. I am not sure they
>> will use the same IP.
>> I can remove 'depends on' and wait for the Layerscape ARCH name then add it.
>> The other Layerscape will use ARM v8. I am not even sure LS1021 will be
>> belong
>> to LS1.
> Why do you need any dependency here other than what is required to make
> the driver build?
>
[Minghuan] Ok, I will remove "deponds on"
>>>> +static irqreturn_t ls_pcie_msi_irq_handler(int irq, void *data)
>>>> +{
>>>> +	struct pcie_port *pp = data;
>>>> +	struct ls_pcie *pcie = to_ls_pcie(pp);
>>>> +	unsigned int msi_irq;
>>>> +
>>>> +	/* clear the interrupt */
>>>> +	regmap_write(pcie->scfg, SCFG_SPIMSICLRCR,
>>>> +		     MSI_LS1021A_DATA(pcie->index));
>>>> +
>>>> +	msi_irq = irq_find_mapping(pp->irq_domain, 0);
>>>> +	if (msi_irq)
>>>> +		generic_handle_irq(msi_irq);
>>>> +	else
>>>> +		/*
>>>> +		 * that's weird who triggered this?
>>>> +		 * just clear it
>>>> +		 */
>>>> +		dev_info(pcie->dev, "unexpected MSI\n");
>>>> +
>>>> +	return IRQ_HANDLED;
>>>> +}
>>>> +
>>> Why are you returning IRQ_HANDLED in the "unexpected MSI" case?
>> [Minghuan] The interrupt located in top layer chip is dedicated to 32
>> MSI interrupts.
>> This handler has cleaned the interrupt, so I think it can return IRQ_HANDLED
>> although  the special MSI interrupt belong second layer chip has not
>> been registered.
> If it's wrong enough to print "unexpected MSI" (BTW, please use either
> dev_dbg or a ratelimited dev_err for that, and include the IRQ number),
> then it's wrong enough to return IRQ_NONE so that the upper layers know
> the interrupt was not handled.
[Minghuan] Ok, I will fix it.
> -Scott