diff mbox

[5/9] dt-bindings: pci: rcar pcie device tree bindings

Message ID 1394704208-7490-6-git-send-email-phil.edworthy@renesas.com (mailing list archive)
State Superseded
Headers show

Commit Message

Phil Edworthy March 13, 2014, 9:50 a.m. UTC 
This patch adds the bindings for the rcar PCIE driver. The driver
resides under drivers/pci/host/pcie-rcar.c

Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
---
 Documentation/devicetree/bindings/pci/rcar-pci.txt | 40 ++++++++++++++++++++++
 1 file changed, 40 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pci/rcar-pci.txt

Comments

Arnd Bergmann March 13, 2014, 10 a.m. UTC | #1 
On Thursday 13 March 2014 09:50:04 Phil Edworthy wrote:
> This patch adds the bindings for the rcar PCIE driver. The driver
> resides under drivers/pci/host/pcie-rcar.c
> 
> Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> ---
>  Documentation/devicetree/bindings/pci/rcar-pci.txt | 40 ++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pci/rcar-pci.txt
> 
> diff --git a/Documentation/devicetree/bindings/pci/rcar-pci.txt b/Documentation/devicetree/bindings/pci/rcar-pci.txt
> new file mode 100644
> index 0000000..0e219b0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pci/rcar-pci.txt
> @@ -0,0 +1,40 @@
> +* Renesas RCar PCIe interface
> +
> +Required properties:
> +- compatible: should contain one of the following
> +	"renesas,r8a7779-pcie", "renesas,r8a7790-pcie", "renesas,r8a7791-pcie"
> +- reg: base addresses and lengths of the pcie controller.
> +- #address-cells: set to <3>
> +- #size-cells: set to <2>
> +- device_type: set to "pci"
> +- ranges: ranges for the PCI memory and I/O regions

I see your description and example includes I/O regions, but the driver
claims in a comment that these don't work. Do you also have a patch to
make them work?

In case that patch is part of the 9-patch series, do you mind adding the
arm kernel mailing list (and maybe me personally) to Cc the next time?

> +- interrupts: interrupt values for MSI interrupt
> +- #interrupt-cells: set to <1>
> +- interrupt-map-mask and interrupt-map: standard PCI properties
> +	to define the mapping of the PCIe interface to interrupt
> +	numbers.
> +- clocks: from common clock binding: handle to pci clock.
> +- clock-names: from common clock binding: should be "pcie"
> +
> +Example:
> +
> +SoC specific DT Entry:
> +
> +	pcie: pcie@0x01000000 {
> +		compatible = "renesas,r8a7790-pcie";
> +		reg = <0 0xfe000000 0 0x80000>;
> +		#address-cells = <3>;
> +		#size-cells = <2>;
> +		device_type = "pci";
> +		ranges = <0x01000000 0 0x00000000 0 0xfe100000 0 0x00100000
> +			  0x02000000 0 0xfe200000 0 0xfe200000 0 0x00200000
> +			  0x02000000 0 0x30000000 0 0x30000000 0 0x08000000
> +			  0x42000000 0 0x38000000 0 0x38000000 0 0x08000000>;

I'm curious about why there are two non-prefetchable regions. What is
the significance of this?

> +		interrupts = <0 116 4>;
> +		#interrupt-cells = <1>;
> +		interrupt-map-mask = <0 0 0 0>;
> +		interrupt-map = <0 0 0 0 &gic 0 116 4>;
> +		clocks = <&mstp3_clks R8A7790_CLK_PCIE>;
> +		clock-names = "pcie";
> +		status = "disabled";
> +	};

Are you sure that there is only one legacy interrupt?  Most host
controllers wire up IntA through IntD to different output pins.

Otherwise looks good.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Phil Edworthy March 13, 2014, 10:28 a.m. UTC | #2 
Hi Arnd,

Thanks for your review.

On 13/03/2014 10:00, Arnd wrote:
> On Thursday 13 March 2014 09:50:04 Phil Edworthy wrote:
> > This patch adds the bindings for the rcar PCIE driver. The driver
> > resides under drivers/pci/host/pcie-rcar.c
> > 
> > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> > ---
> >  Documentation/devicetree/bindings/pci/rcar-pci.txt | 40 
++++++++++++++++++++++
> >  1 file changed, 40 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/pci/rcar-pci.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/pci/rcar-pci.txt b/
> Documentation/devicetree/bindings/pci/rcar-pci.txt
> > new file mode 100644
> > index 0000000..0e219b0
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pci/rcar-pci.txt
> > @@ -0,0 +1,40 @@
> > +* Renesas RCar PCIe interface
> > +
> > +Required properties:
> > +- compatible: should contain one of the following
> > +   "renesas,r8a7779-pcie", "renesas,r8a7790-pcie", 
"renesas,r8a7791-pcie"
> > +- reg: base addresses and lengths of the pcie controller.
> > +- #address-cells: set to <3>
> > +- #size-cells: set to <2>
> > +- device_type: set to "pci"
> > +- ranges: ranges for the PCI memory and I/O regions
> 
> I see your description and example includes I/O regions, but the driver
> claims in a comment that these don't work. Do you also have a patch to
> make them work?
Are you referring to the comment "The controller does not support/use port 
I/O" in the driver for the internal PCI controller (pci-rcar-gen2.c)?
This is a separate block of hardware, it's a PCIe controller with external 
pins, for which we do support I/O regions.

> In case that patch is part of the 9-patch series, do you mind adding the
> arm kernel mailing list (and maybe me personally) to Cc the next time?
Sure, the rest of the patch set adds this new PCIe driver and adds it to 
the R-Car devices & boards.

> > +- interrupts: interrupt values for MSI interrupt
> > +- #interrupt-cells: set to <1>
> > +- interrupt-map-mask and interrupt-map: standard PCI properties
> > +   to define the mapping of the PCIe interface to interrupt
> > +   numbers.
> > +- clocks: from common clock binding: handle to pci clock.
> > +- clock-names: from common clock binding: should be "pcie"
> > +
> > +Example:
> > +
> > +SoC specific DT Entry:
> > +
> > +   pcie: pcie@0x01000000 {
> > +      compatible = "renesas,r8a7790-pcie";
> > +      reg = <0 0xfe000000 0 0x80000>;
> > +      #address-cells = <3>;
> > +      #size-cells = <2>;
> > +      device_type = "pci";
> > +      ranges = <0x01000000 0 0x00000000 0 0xfe100000 0 0x00100000
> > +           0x02000000 0 0xfe200000 0 0xfe200000 0 0x00200000
> > +           0x02000000 0 0x30000000 0 0x30000000 0 0x08000000
> > +           0x42000000 0 0x38000000 0 0x38000000 0 0x08000000>;
> 
> I'm curious about why there are two non-prefetchable regions. What is
> the significance of this?
The devices that have this PCIe controller both have a number of fixed, 
non-contiguous memory regions that can be used as PCIe windows. The ranges 
here map onto those fixed windows, i.e. if the CPU addresses of the ranges 
change it won't work. The first couple of windows are small, we use the 
first one for I/O, we decided that we may as well use the next small 
window for memory - otherwise we need to figure out a way to tell the 
driver to skip a window.

> > +      interrupts = <0 116 4>;
> > +      #interrupt-cells = <1>;
> > +      interrupt-map-mask = <0 0 0 0>;
> > +      interrupt-map = <0 0 0 0 &gic 0 116 4>;
> > +      clocks = <&mstp3_clks R8A7790_CLK_PCIE>;
> > +      clock-names = "pcie";
> > +      status = "disabled";
> > +   };
> 
> Are you sure that there is only one legacy interrupt?  Most host
> controllers wire up IntA through IntD to different output pins.
Yes, and we've tested that as well.

> Otherwise looks good.
> 
>    Arnd

Thanks
Phil
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann March 13, 2014, 11:30 a.m. UTC | #3 
On Thursday 13 March 2014 10:28:03 Phil.Edworthy@renesas.com wrote:
> On 13/03/2014 10:00, Arnd wrote:
> > On Thursday 13 March 2014 09:50:04 Phil Edworthy wrote:
> > 
> > I see your description and example includes I/O regions, but the driver
> > claims in a comment that these don't work. Do you also have a patch to
> > make them work?
> Are you referring to the comment "The controller does not support/use port 
> I/O" in the driver for the internal PCI controller (pci-rcar-gen2.c)?
> This is a separate block of hardware, it's a PCIe controller with external 
> pins, for which we do support I/O regions.

Ok, thanks for the clarification.

> > > +- interrupts: interrupt values for MSI interrupt
> > > +- #interrupt-cells: set to <1>
> > > +- interrupt-map-mask and interrupt-map: standard PCI properties
> > > +   to define the mapping of the PCIe interface to interrupt
> > > +   numbers.
> > > +- clocks: from common clock binding: handle to pci clock.
> > > +- clock-names: from common clock binding: should be "pcie"
> > > +
> > > +Example:
> > > +
> > > +SoC specific DT Entry:
> > > +
> > > +   pcie: pcie@0x01000000 {
> > > +      compatible = "renesas,r8a7790-pcie";
> > > +      reg = <0 0xfe000000 0 0x80000>;
> > > +      #address-cells = <3>;
> > > +      #size-cells = <2>;
> > > +      device_type = "pci";
> > > +      ranges = <0x01000000 0 0x00000000 0 0xfe100000 0 0x00100000
> > > +           0x02000000 0 0xfe200000 0 0xfe200000 0 0x00200000
> > > +           0x02000000 0 0x30000000 0 0x30000000 0 0x08000000
> > > +           0x42000000 0 0x38000000 0 0x38000000 0 0x08000000>;
> > 
> > I'm curious about why there are two non-prefetchable regions. What is
> > the significance of this?
>
> The devices that have this PCIe controller both have a number of fixed, 
> non-contiguous memory regions that can be used as PCIe windows. The ranges 
> here map onto those fixed windows, i.e. if the CPU addresses of the ranges 
> change it won't work. The first couple of windows are small, we use the 
> first one for I/O, we decided that we may as well use the next small 
> window for memory - otherwise we need to figure out a way to tell the 
> driver to skip a window.

Ok, makes sense. Good to know that this actually works. An idea that
may be helpful in some corner cases: if you can set up the small window as

           0x02000000 0 0 0 0xfe200000 0 0x00200000

or even

           0x02000000 0 0 0 0xfe200000 0 0x00100000
           0x02000000 0 0x00f00000 0 0xfe200000 0 0x00100000

then you will be able to map devices with legacy nonrelocatable ISA
memory BARs such as VGA cards that have PCI resources in the first MB
of bus space, and in the end of the first 16MB.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/pci/rcar-pci.txt b/Documentation/devicetree/bindings/pci/rcar-pci.txt
new file mode 100644
index 0000000..0e219b0
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/rcar-pci.txt
@@ -0,0 +1,40 @@ 
+* Renesas RCar PCIe interface
+
+Required properties:
+- compatible: should contain one of the following
+	"renesas,r8a7779-pcie", "renesas,r8a7790-pcie", "renesas,r8a7791-pcie"
+- reg: base addresses and lengths of the pcie controller.
+- #address-cells: set to <3>
+- #size-cells: set to <2>
+- device_type: set to "pci"
+- ranges: ranges for the PCI memory and I/O regions
+- interrupts: interrupt values for MSI interrupt
+- #interrupt-cells: set to <1>
+- interrupt-map-mask and interrupt-map: standard PCI properties
+	to define the mapping of the PCIe interface to interrupt
+	numbers.
+- clocks: from common clock binding: handle to pci clock.
+- clock-names: from common clock binding: should be "pcie"
+
+Example:
+
+SoC specific DT Entry:
+
+	pcie: pcie@0x01000000 {
+		compatible = "renesas,r8a7790-pcie";
+		reg = <0 0xfe000000 0 0x80000>;
+		#address-cells = <3>;
+		#size-cells = <2>;
+		device_type = "pci";
+		ranges = <0x01000000 0 0x00000000 0 0xfe100000 0 0x00100000
+			  0x02000000 0 0xfe200000 0 0xfe200000 0 0x00200000
+			  0x02000000 0 0x30000000 0 0x30000000 0 0x08000000
+			  0x42000000 0 0x38000000 0 0x38000000 0 0x08000000>;
+		interrupts = <0 116 4>;
+		#interrupt-cells = <1>;
+		interrupt-map-mask = <0 0 0 0>;
+		interrupt-map = <0 0 0 0 &gic 0 116 4>;
+		clocks = <&mstp3_clks R8A7790_CLK_PCIE>;
+		clock-names = "pcie";
+		status = "disabled";
+	};