| Message ID | 1462475700-1654-2-git-send-email-f.fainelli@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thursday 05 May 2016 12:14:59 Florian Fainelli wrote: > From: Jim Quinlan <jim2101024@gmail.com> > > This patchs adds the Device Tree bindings for the Broadcom STB PCIe root > complex hardware. > > Signed-off-by: Jim Quinlan <jim2101024@gmail.com> > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > --- > Changes in v2: > > - rewrite the binding document almost from scratch to include many more > references to existing documents > - describe missing properties > - give better examples > > .../devicetree/bindings/pci/brcm,brcmstb-pcie.txt | 98 ++++++++++++++++++++++ > 1 file changed, 98 insertions(+) > create mode 100644 Documentation/devicetree/bindings/pci/brcm,brcmstb-pcie.txt > > diff --git a/Documentation/devicetree/bindings/pci/brcm,brcmstb-pcie.txt b/Documentation/devicetree/bindings/pci/brcm,brcmstb-pcie.txt > new file mode 100644 > index 000000000000..3682b0f0bc26 > --- /dev/null > +++ b/Documentation/devicetree/bindings/pci/brcm,brcmstb-pcie.txt > @@ -0,0 +1,98 @@ > +Broadcom STB PCIe Host Controller Device Tree Bindings > + > +This document describes the binding of the PCIe Root Complex hardware found in > +Broadcom Set Top Box System-on-Chips such as BCM7425 (MIPS), BCM7435 (MIPS) and > +BCM7445 (ARMv7). > + > +Required properties: > +- compatible: must be one of: "brcm,bcm7425-pcie" > + "brcm,bcm7435-pcie" > + "brcm,bcm7445-pcie" > + > +- reg: specifies the physical base address of the controller registers and > + its length > + > +- interrupt-parent: must be a reference (phandle) to the parent interrupt > + controller in the system (7038-l1-intc on MIPS, GIC on ARM/ARM64) > + > +- interrrupts: first interrupt must be the Level 1 interrupt number corresponding > + to the main PCIe RC interrupt, second interrupt must be the MSI interrupt > + See the interrupt-parent documentation for the number of cells and their meaning: > + MIPS: Documentation/devicetree/bindings/interrupt-controller/brcm,bcm7038-l1-intc.txt > + ARM/ARM64: Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt > + > +- interrupt-names: must be "pcie", and if present "msi" When I suggested splitting out the MSI support, I was thinking (but not writing) that you'd use an msi-parent property to refer to the node that holds the msi-controller as well. > +- ranges: ranges for the PCI outbound windows, no I/O or prefetchable windows > + must be specified here, only non-prefetchable. 32-bits windows or 64-bits > + windows are allowed based on the host processor's capabilities (ARM w/ LPAE, > + ARM64). So this supports 64-bit non-prefetchable windows? Usually 64-bit windows are prefetchable. > +- brcm,log2-scb-sizes: log2 size of the SCB window that is mapped to PCIe space > + there must be exactly one value per memory controller present in the system > + (ranges from 1 to 3) I'm still not too happy with this property. I see no reason for the log2 format (rather than length in bytes, or offset/length tuples, or dma-ranges, or phandles pointing to the memory controllers). I think we need to discuss this some more. > +- brcm,gen: integer that indicates desired forced generation of link: 1 => 2.5 > + Gbps, 2 => 5.0 Gbps, 3 => 8.0 Gbps. Will override the auto-negotation if > + specified. to repeat my earlier comment from v1: Shouldn't the link generation be probed automatically? > +- <*>-supply: see Documentation/devicetree/bindings/regulator/regulator.txt > + > +- <*>-supply-names: see Documentation/devicetree/bindings/regulator/regulator.txt I don't see supply-names documented there. Arnd
On 05/05/16 14:15, Arnd Bergmann wrote: > On Thursday 05 May 2016 12:14:59 Florian Fainelli wrote: >> From: Jim Quinlan <jim2101024@gmail.com> >> >> This patchs adds the Device Tree bindings for the Broadcom STB PCIe root >> complex hardware. >> >> Signed-off-by: Jim Quinlan <jim2101024@gmail.com> >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> >> --- >> Changes in v2: >> >> - rewrite the binding document almost from scratch to include many more >> references to existing documents >> - describe missing properties >> - give better examples >> >> .../devicetree/bindings/pci/brcm,brcmstb-pcie.txt | 98 ++++++++++++++++++++++ >> 1 file changed, 98 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/pci/brcm,brcmstb-pcie.txt >> >> diff --git a/Documentation/devicetree/bindings/pci/brcm,brcmstb-pcie.txt b/Documentation/devicetree/bindings/pci/brcm,brcmstb-pcie.txt >> new file mode 100644 >> index 000000000000..3682b0f0bc26 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/pci/brcm,brcmstb-pcie.txt >> @@ -0,0 +1,98 @@ >> +Broadcom STB PCIe Host Controller Device Tree Bindings >> + >> +This document describes the binding of the PCIe Root Complex hardware found in >> +Broadcom Set Top Box System-on-Chips such as BCM7425 (MIPS), BCM7435 (MIPS) and >> +BCM7445 (ARMv7). >> + >> +Required properties: >> +- compatible: must be one of: "brcm,bcm7425-pcie" >> + "brcm,bcm7435-pcie" >> + "brcm,bcm7445-pcie" >> + >> +- reg: specifies the physical base address of the controller registers and >> + its length >> + >> +- interrupt-parent: must be a reference (phandle) to the parent interrupt >> + controller in the system (7038-l1-intc on MIPS, GIC on ARM/ARM64) >> + >> +- interrrupts: first interrupt must be the Level 1 interrupt number corresponding >> + to the main PCIe RC interrupt, second interrupt must be the MSI interrupt >> + See the interrupt-parent documentation for the number of cells and their meaning: >> + MIPS: Documentation/devicetree/bindings/interrupt-controller/brcm,bcm7038-l1-intc.txt >> + ARM/ARM64: Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt >> + >> +- interrupt-names: must be "pcie", and if present "msi" > > When I suggested splitting out the MSI support, I was thinking (but not writing) > that you'd use an msi-parent property to refer to the node that holds the > msi-controller as well. Humm fair enough. > >> +- ranges: ranges for the PCI outbound windows, no I/O or prefetchable windows >> + must be specified here, only non-prefetchable. 32-bits windows or 64-bits >> + windows are allowed based on the host processor's capabilities (ARM w/ LPAE, >> + ARM64). > > So this supports 64-bit non-prefetchable windows? Usually 64-bit windows > are prefetchable. > >> +- brcm,log2-scb-sizes: log2 size of the SCB window that is mapped to PCIe space >> + there must be exactly one value per memory controller present in the system >> + (ranges from 1 to 3) > > I'm still not too happy with this property. I see no reason for the log2 > format (rather than length in bytes, or offset/length tuples, or dma-ranges, > or phandles pointing to the memory controllers). I think we need to discuss > this some more. Sure, works for me. > >> +- brcm,gen: integer that indicates desired forced generation of link: 1 => 2.5 >> + Gbps, 2 => 5.0 Gbps, 3 => 8.0 Gbps. Will override the auto-negotation if >> + specified. > > to repeat my earlier comment from v1: > > Shouldn't the link generation be probed automatically? It is, but if the property is present, we can foce the link negotiation, We have had cases where this was desired and helpful. > >> +- <*>-supply: see Documentation/devicetree/bindings/regulator/regulator.txt >> + >> +- <*>-supply-names: see Documentation/devicetree/bindings/regulator/regulator.txt > > I don't see supply-names documented there. Meh, I read that wrong, will fix that. Thanks!
On Thursday 05 May 2016 14:46:22 Florian Fainelli wrote: > > > > >> +- brcm,gen: integer that indicates desired forced generation of link: 1 => 2.5 > >> + Gbps, 2 => 5.0 Gbps, 3 => 8.0 Gbps. Will override the auto-negotation if > >> + specified. > > > > to repeat my earlier comment from v1: > > > > Shouldn't the link generation be probed automatically? > > It is, but if the property is present, we can foce the link negotiation, > We have had cases where this was desired and helpful. > Right, I missed that this is listed as 'optional', so that seems perfectly fine. Thanks, Arnd
On Thu, May 05, 2016 at 12:14:59PM -0700, Florian Fainelli wrote: > From: Jim Quinlan <jim2101024@gmail.com> > > This patchs adds the Device Tree bindings for the Broadcom STB PCIe root > complex hardware. > > Signed-off-by: Jim Quinlan <jim2101024@gmail.com> > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> > --- > Changes in v2: > > - rewrite the binding document almost from scratch to include many more > references to existing documents > - describe missing properties > - give better examples > > .../devicetree/bindings/pci/brcm,brcmstb-pcie.txt | 98 ++++++++++++++++++++++ > 1 file changed, 98 insertions(+) > create mode 100644 Documentation/devicetree/bindings/pci/brcm,brcmstb-pcie.txt > > diff --git a/Documentation/devicetree/bindings/pci/brcm,brcmstb-pcie.txt b/Documentation/devicetree/bindings/pci/brcm,brcmstb-pcie.txt > new file mode 100644 > index 000000000000..3682b0f0bc26 > --- /dev/null > +++ b/Documentation/devicetree/bindings/pci/brcm,brcmstb-pcie.txt > @@ -0,0 +1,98 @@ > +Broadcom STB PCIe Host Controller Device Tree Bindings > + > +This document describes the binding of the PCIe Root Complex hardware found in > +Broadcom Set Top Box System-on-Chips such as BCM7425 (MIPS), BCM7435 (MIPS) and > +BCM7445 (ARMv7). > + > +Required properties: > +- compatible: must be one of: "brcm,bcm7425-pcie" > + "brcm,bcm7435-pcie" > + "brcm,bcm7445-pcie" > + > +- reg: specifies the physical base address of the controller registers and > + its length > + > +- interrupt-parent: must be a reference (phandle) to the parent interrupt > + controller in the system (7038-l1-intc on MIPS, GIC on ARM/ARM64) > + > +- interrrupts: first interrupt must be the Level 1 interrupt number corresponding > + to the main PCIe RC interrupt, second interrupt must be the MSI interrupt > + See the interrupt-parent documentation for the number of cells and their meaning: > + MIPS: Documentation/devicetree/bindings/interrupt-controller/brcm,bcm7038-l1-intc.txt > + ARM/ARM64: Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt > + > +- interrupt-names: must be "pcie", and if present "msi" > + > +- interrupt-map: see pci.txt > + > +- interrupt-map-mask: see pci.txt > + > +- #address-cells: must be set to <3>, see pci.txt > + > +- #size-cells: must be set to <2>, see pci.txt > + > +- ranges: ranges for the PCI outbound windows, no I/O or prefetchable windows > + must be specified here, only non-prefetchable. 32-bits windows or 64-bits > + windows are allowed based on the host processor's capabilities (ARM w/ LPAE, > + ARM64). > + > +- #interrupt-cells: set to <1>, see pci.txt > + > +- brcm,log2-scb-sizes: log2 size of the SCB window that is mapped to PCIe space > + there must be exactly one value per memory controller present in the system > + (ranges from 1 to 3) > + > +Optional properties: > + > +- msi-controller: indicates that this is a MSI controller node (when supported) > + > +- clocks: phandle to the functional clock that feeds into the PCIe RC block > + > +- clock-names: the name(s) of the clocks specified in 'clocks'. Note > + that if the 'clocks' property is given, 'clock-names' is mandatory, > + and the name of the clock is expected to be "pcie". > + > +- brcm,ssc: boolean that indicates usage of spread-spectrum clocking Shouldn't this be a property of the phy? Is there no separate phy to control? > + > +- brcm,gen: integer that indicates desired forced generation of link: 1 => 2.5 > + Gbps, 2 => 5.0 Gbps, 3 => 8.0 Gbps. Will override the auto-negotation if > + specified. > + > +- <*>-supply: see Documentation/devicetree/bindings/regulator/regulator.txt > + > +- <*>-supply-names: see Documentation/devicetree/bindings/regulator/regulator.txt > + > +Example Node: > + > +This example assumes that the top-level #address-cells = <2> and #size-cells = > +<2>, e.g: ARM LPAE configuration. > + > + pcie0: pcie-controller@f0460000 { > + reg = <0x0 0xf0460000 0x0 0x9310>; > + interrupts = <0x0 0x33 0x4>, <0x0 0x34, 0x4>; ^ typo > + interrupt-names = "pcie", "msi"; > + compatible = "brcm,bcm7445-pcie"; > + #address-cells = <3>; > + #size-cells = <2>; > + > + /* Two non-prefetchable 32-bits memory space, each of 128MB > + * with the following mapping: > + * PCIe address => CPU physical address space > + * 0x00_0000_0000 => 0x00_C000_0000 > + * 0x00_0800_0000 => 0x00_C800_0000 > + */ > + ranges = <0x02000000 0x00000000 0x00000000 0x00000000 0xc0000000 0x00000000 0x08000000>, > + <0x02000000 0x00000000 0x08000000 0x00000000 0xc8000000 0x00000000 0x08000000>; > + #interrupt-cells = <1>; > + interrupt-map-mask = <0xf800 0 0 7>; > + interrupt-map = <0 0 0 1 &intc 47 3 > + 0 0 0 2 &intc 48 3 > + 0 0 0 3 &intc 49 3 > + 0 0 0 4 &intc 50 3>; > + clocks = <&pcie0>; > + clock-names = "pcie"; > + brcm,ssc; > + brcm,log2-scb-sizes = <0x1e 0x1e 0x1e>; > + vreg-wifi-pwr-supply-names = "vreg-wifi-pwr"; > + vreg-wifi-pwr-supply = <&vreg-wifi-pwr>; The supply for the PCIE RC is called "vreg-wifi-pwr"? Seems a bit strange. Rob
On 05/09/2016 12:26 PM, Rob Herring wrote: > On Thu, May 05, 2016 at 12:14:59PM -0700, Florian Fainelli wrote: >> From: Jim Quinlan <jim2101024@gmail.com> >> >> This patchs adds the Device Tree bindings for the Broadcom STB PCIe root >> complex hardware. >> >> Signed-off-by: Jim Quinlan <jim2101024@gmail.com> >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> >> --- >> Changes in v2: >> >> - rewrite the binding document almost from scratch to include many more >> references to existing documents >> - describe missing properties >> - give better examples >> >> .../devicetree/bindings/pci/brcm,brcmstb-pcie.txt | 98 ++++++++++++++++++++++ >> 1 file changed, 98 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/pci/brcm,brcmstb-pcie.txt >> >> diff --git a/Documentation/devicetree/bindings/pci/brcm,brcmstb-pcie.txt b/Documentation/devicetree/bindings/pci/brcm,brcmstb-pcie.txt >> new file mode 100644 >> index 000000000000..3682b0f0bc26 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/pci/brcm,brcmstb-pcie.txt >> @@ -0,0 +1,98 @@ >> +Broadcom STB PCIe Host Controller Device Tree Bindings >> + >> +This document describes the binding of the PCIe Root Complex hardware found in >> +Broadcom Set Top Box System-on-Chips such as BCM7425 (MIPS), BCM7435 (MIPS) and >> +BCM7445 (ARMv7). >> + >> +Required properties: >> +- compatible: must be one of: "brcm,bcm7425-pcie" >> + "brcm,bcm7435-pcie" >> + "brcm,bcm7445-pcie" >> + >> +- reg: specifies the physical base address of the controller registers and >> + its length >> + >> +- interrupt-parent: must be a reference (phandle) to the parent interrupt >> + controller in the system (7038-l1-intc on MIPS, GIC on ARM/ARM64) >> + >> +- interrrupts: first interrupt must be the Level 1 interrupt number corresponding >> + to the main PCIe RC interrupt, second interrupt must be the MSI interrupt >> + See the interrupt-parent documentation for the number of cells and their meaning: >> + MIPS: Documentation/devicetree/bindings/interrupt-controller/brcm,bcm7038-l1-intc.txt >> + ARM/ARM64: Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt >> + >> +- interrupt-names: must be "pcie", and if present "msi" >> + >> +- interrupt-map: see pci.txt >> + >> +- interrupt-map-mask: see pci.txt >> + >> +- #address-cells: must be set to <3>, see pci.txt >> + >> +- #size-cells: must be set to <2>, see pci.txt >> + >> +- ranges: ranges for the PCI outbound windows, no I/O or prefetchable windows >> + must be specified here, only non-prefetchable. 32-bits windows or 64-bits >> + windows are allowed based on the host processor's capabilities (ARM w/ LPAE, >> + ARM64). >> + >> +- #interrupt-cells: set to <1>, see pci.txt >> + >> +- brcm,log2-scb-sizes: log2 size of the SCB window that is mapped to PCIe space >> + there must be exactly one value per memory controller present in the system >> + (ranges from 1 to 3) >> + >> +Optional properties: >> + >> +- msi-controller: indicates that this is a MSI controller node (when supported) >> + >> +- clocks: phandle to the functional clock that feeds into the PCIe RC block >> + >> +- clock-names: the name(s) of the clocks specified in 'clocks'. Note >> + that if the 'clocks' property is given, 'clock-names' is mandatory, >> + and the name of the clock is expected to be "pcie". >> + >> +- brcm,ssc: boolean that indicates usage of spread-spectrum clocking > > Shouldn't this be a property of the phy? Is there no separate phy to > control? No, the PHY is baked into the PCIe root complex, which is why this is a property of the root complex node since the two are conflated. > >> + >> +- brcm,gen: integer that indicates desired forced generation of link: 1 => 2.5 >> + Gbps, 2 => 5.0 Gbps, 3 => 8.0 Gbps. Will override the auto-negotation if >> + specified. >> + >> +- <*>-supply: see Documentation/devicetree/bindings/regulator/regulator.txt >> + >> +- <*>-supply-names: see Documentation/devicetree/bindings/regulator/regulator.txt >> + >> +Example Node: >> + >> +This example assumes that the top-level #address-cells = <2> and #size-cells = >> +<2>, e.g: ARM LPAE configuration. >> + >> + pcie0: pcie-controller@f0460000 { >> + reg = <0x0 0xf0460000 0x0 0x9310>; >> + interrupts = <0x0 0x33 0x4>, <0x0 0x34, 0x4>; > ^ > typo Whoops, thanks. > >> + interrupt-names = "pcie", "msi"; >> + compatible = "brcm,bcm7445-pcie"; >> + #address-cells = <3>; >> + #size-cells = <2>; >> + >> + /* Two non-prefetchable 32-bits memory space, each of 128MB >> + * with the following mapping: >> + * PCIe address => CPU physical address space >> + * 0x00_0000_0000 => 0x00_C000_0000 >> + * 0x00_0800_0000 => 0x00_C800_0000 >> + */ >> + ranges = <0x02000000 0x00000000 0x00000000 0x00000000 0xc0000000 0x00000000 0x08000000>, >> + <0x02000000 0x00000000 0x08000000 0x00000000 0xc8000000 0x00000000 0x08000000>; >> + #interrupt-cells = <1>; >> + interrupt-map-mask = <0xf800 0 0 7>; >> + interrupt-map = <0 0 0 1 &intc 47 3 >> + 0 0 0 2 &intc 48 3 >> + 0 0 0 3 &intc 49 3 >> + 0 0 0 4 &intc 50 3>; >> + clocks = <&pcie0>; >> + clock-names = "pcie"; >> + brcm,ssc; >> + brcm,log2-scb-sizes = <0x1e 0x1e 0x1e>; >> + vreg-wifi-pwr-supply-names = "vreg-wifi-pwr"; >> + vreg-wifi-pwr-supply = <&vreg-wifi-pwr>; > > The supply for the PCIE RC is called "vreg-wifi-pwr"? Seems a bit > strange. It can be called anything, this just happens to be the example, but I can certainly drop that, since a) this is optional and b) we need to rework that in the driver anyway.
On 05/09/2016 12:26 PM, Rob Herring wrote: > On Thu, May 05, 2016 at 12:14:59PM -0700, Florian Fainelli wrote: >> From: Jim Quinlan <jim2101024@gmail.com> >> >> This patchs adds the Device Tree bindings for the Broadcom STB PCIe root >> complex hardware. >> >> Signed-off-by: Jim Quinlan <jim2101024@gmail.com> >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> >> --- >> Changes in v2: >> >> - rewrite the binding document almost from scratch to include many more >> references to existing documents >> - describe missing properties >> - give better examples >> >> .../devicetree/bindings/pci/brcm,brcmstb-pcie.txt | 98 ++++++++++++++++++++++ >> 1 file changed, 98 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/pci/brcm,brcmstb-pcie.txt >> >> diff --git a/Documentation/devicetree/bindings/pci/brcm,brcmstb-pcie.txt b/Documentation/devicetree/bindings/pci/brcm,brcmstb-pcie.txt >> new file mode 100644 >> index 000000000000..3682b0f0bc26 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/pci/brcm,brcmstb-pcie.txt >> @@ -0,0 +1,98 @@ >> +Broadcom STB PCIe Host Controller Device Tree Bindings >> + >> +This document describes the binding of the PCIe Root Complex hardware found in >> +Broadcom Set Top Box System-on-Chips such as BCM7425 (MIPS), BCM7435 (MIPS) and >> +BCM7445 (ARMv7). >> + >> +Required properties: >> +- compatible: must be one of: "brcm,bcm7425-pcie" >> + "brcm,bcm7435-pcie" >> + "brcm,bcm7445-pcie" >> + >> +- reg: specifies the physical base address of the controller registers and >> + its length >> + >> +- interrupt-parent: must be a reference (phandle) to the parent interrupt >> + controller in the system (7038-l1-intc on MIPS, GIC on ARM/ARM64) >> + >> +- interrrupts: first interrupt must be the Level 1 interrupt number corresponding >> + to the main PCIe RC interrupt, second interrupt must be the MSI interrupt >> + See the interrupt-parent documentation for the number of cells and their meaning: >> + MIPS: Documentation/devicetree/bindings/interrupt-controller/brcm,bcm7038-l1-intc.txt >> + ARM/ARM64: Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt >> + >> +- interrupt-names: must be "pcie", and if present "msi" >> + >> +- interrupt-map: see pci.txt >> + >> +- interrupt-map-mask: see pci.txt >> + >> +- #address-cells: must be set to <3>, see pci.txt >> + >> +- #size-cells: must be set to <2>, see pci.txt >> + >> +- ranges: ranges for the PCI outbound windows, no I/O or prefetchable windows >> + must be specified here, only non-prefetchable. 32-bits windows or 64-bits >> + windows are allowed based on the host processor's capabilities (ARM w/ LPAE, >> + ARM64). >> + >> +- #interrupt-cells: set to <1>, see pci.txt >> + >> +- brcm,log2-scb-sizes: log2 size of the SCB window that is mapped to PCIe space >> + there must be exactly one value per memory controller present in the system >> + (ranges from 1 to 3) >> + >> +Optional properties: >> + >> +- msi-controller: indicates that this is a MSI controller node (when supported) >> + >> +- clocks: phandle to the functional clock that feeds into the PCIe RC block >> + >> +- clock-names: the name(s) of the clocks specified in 'clocks'. Note >> + that if the 'clocks' property is given, 'clock-names' is mandatory, >> + and the name of the clock is expected to be "pcie". >> + >> +- brcm,ssc: boolean that indicates usage of spread-spectrum clocking > > Shouldn't this be a property of the phy? Is there no separate phy to > control? No, the PHY is baked into the PCIe root complex, which is why this is a property of the root complex node since the two are conflated. > >> + >> +- brcm,gen: integer that indicates desired forced generation of link: 1 => 2.5 >> + Gbps, 2 => 5.0 Gbps, 3 => 8.0 Gbps. Will override the auto-negotation if >> + specified. >> + >> +- <*>-supply: see Documentation/devicetree/bindings/regulator/regulator.txt >> + >> +- <*>-supply-names: see Documentation/devicetree/bindings/regulator/regulator.txt >> + >> +Example Node: >> + >> +This example assumes that the top-level #address-cells = <2> and #size-cells = >> +<2>, e.g: ARM LPAE configuration. >> + >> + pcie0: pcie-controller@f0460000 { >> + reg = <0x0 0xf0460000 0x0 0x9310>; >> + interrupts = <0x0 0x33 0x4>, <0x0 0x34, 0x4>; > ^ > typo Whoops, thanks. > >> + interrupt-names = "pcie", "msi"; >> + compatible = "brcm,bcm7445-pcie"; >> + #address-cells = <3>; >> + #size-cells = <2>; >> + >> + /* Two non-prefetchable 32-bits memory space, each of 128MB >> + * with the following mapping: >> + * PCIe address => CPU physical address space >> + * 0x00_0000_0000 => 0x00_C000_0000 >> + * 0x00_0800_0000 => 0x00_C800_0000 >> + */ >> + ranges = <0x02000000 0x00000000 0x00000000 0x00000000 0xc0000000 0x00000000 0x08000000>, >> + <0x02000000 0x00000000 0x08000000 0x00000000 0xc8000000 0x00000000 0x08000000>; >> + #interrupt-cells = <1>; >> + interrupt-map-mask = <0xf800 0 0 7>; >> + interrupt-map = <0 0 0 1 &intc 47 3 >> + 0 0 0 2 &intc 48 3 >> + 0 0 0 3 &intc 49 3 >> + 0 0 0 4 &intc 50 3>; >> + clocks = <&pcie0>; >> + clock-names = "pcie"; >> + brcm,ssc; >> + brcm,log2-scb-sizes = <0x1e 0x1e 0x1e>; >> + vreg-wifi-pwr-supply-names = "vreg-wifi-pwr"; >> + vreg-wifi-pwr-supply = <&vreg-wifi-pwr>; > > The supply for the PCIE RC is called "vreg-wifi-pwr"? Seems a bit > strange. It can be called anything, this just happens to be the example, but I can certainly drop that, since a) this is optional and b) we need to rework that in the driver anyway.
On 05/05/2016 02:15 PM, Arnd Bergmann wrote: >> +Required properties: >> +- compatible: must be one of: "brcm,bcm7425-pcie" >> + "brcm,bcm7435-pcie" >> + "brcm,bcm7445-pcie" >> + >> +- reg: specifies the physical base address of the controller registers and >> + its length >> + >> +- interrupt-parent: must be a reference (phandle) to the parent interrupt >> + controller in the system (7038-l1-intc on MIPS, GIC on ARM/ARM64) >> + >> +- interrrupts: first interrupt must be the Level 1 interrupt number corresponding >> + to the main PCIe RC interrupt, second interrupt must be the MSI interrupt >> + See the interrupt-parent documentation for the number of cells and their meaning: >> + MIPS: Documentation/devicetree/bindings/interrupt-controller/brcm,bcm7038-l1-intc.txt >> + ARM/ARM64: Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt >> + >> +- interrupt-names: must be "pcie", and if present "msi" > > When I suggested splitting out the MSI support, I was thinking (but not writing) > that you'd use an msi-parent property to refer to the node that holds the > msi-controller as well. Oh got it now, and it can actually refer to iself. > >> +- ranges: ranges for the PCI outbound windows, no I/O or prefetchable windows >> + must be specified here, only non-prefetchable. 32-bits windows or 64-bits >> + windows are allowed based on the host processor's capabilities (ARM w/ LPAE, >> + ARM64). > > So this supports 64-bit non-prefetchable windows? Usually 64-bit windows > are prefetchable. I have yet to verify this, but on ARM64-based STB this should indeed be the case. > >> +- brcm,log2-scb-sizes: log2 size of the SCB window that is mapped to PCIe space >> + there must be exactly one value per memory controller present in the system >> + (ranges from 1 to 3) > > I'm still not too happy with this property. I see no reason for the log2 > format (rather than length in bytes, or offset/length tuples, or dma-ranges, > or phandles pointing to the memory controllers). I think we need to discuss > this some more. The two critical pieces of information that the PCIe RC needs are: - number of memory controllers present in the system to avoid configuring a window to a non-existing or non-populated memory controller - size of the memory populated on the memory controller You would think that we should somehow be able to derive this information from the "memory" node, but there are a few caveats: - on MIPS platforms, we have discontiguous physical memory ranges (256MB @ 0x0, then 768MB or 1792MB @ 0x 0x20000000 and 1GB @ 0x90000000), yet the PCIe RC does not know about the hole(s) - on ARM platforms, we have an identiy mapping for the PAs below 4GB, but when we cross it, we have additional non-linear memory regions Both of these memory nodes would imply we know how to identify which physical address belongs to which memory controller, while this is actually possible and known, it creates a dependency on another piece of driver to provide that information. We do have memory controller nodes populated in the Device Tree, however their binding (out of tree for the moment) does only specify their programmable register interface, and not how how much physical memory and where they provide to the system. It does seem like we should be able to utilize the "dma-ranges" property to determine both the memory controller number and their respective populated memory sizes. In the case where we have an identiy mapping though, this seems a little redundant, but less of a stretch than our custom properties and nodes. Does that sound reasonable to you?
On Tuesday 10 May 2016 10:00:12 Florian Fainelli wrote: > > The two critical pieces of information that the PCIe RC needs are: > > - number of memory controllers present in the system to avoid > configuring a window to a non-existing or non-populated memory controller > > - size of the memory populated on the memory controller > > You would think that we should somehow be able to derive this > information from the "memory" node, but there are a few caveats: > > - on MIPS platforms, we have discontiguous physical memory ranges (256MB > @ 0x0, then 768MB or 1792MB @ 0x 0x20000000 and 1GB @ 0x90000000), yet > the PCIe RC does not know about the hole(s) That sounds like you need a dma-ranges property anyway to describe how the DMA addresses are mapped. > - on ARM platforms, we have an identiy mapping for the PAs below 4GB, > but when we cross it, we have additional non-linear memory regions > > Both of these memory nodes would imply we know how to identify which > physical address belongs to which memory controller, while this is > actually possible and known, it creates a dependency on another piece of > driver to provide that information. > > We do have memory controller nodes populated in the Device Tree, however > their binding (out of tree for the moment) does only specify their > programmable register interface, and not how how much physical memory > and where they provide to the system. > > It does seem like we should be able to utilize the "dma-ranges" property > to determine both the memory controller number and their respective > populated memory sizes. > > In the case where we have an identiy mapping though, this seems a little > redundant, but less of a stretch than our custom properties and nodes. > > Does that sound reasonable to you? I'd have to look at an example first. Ideally, each line in the dma-ranges property would refer to one memory controller, and that would be very easy to mandate, but it sounds like in the MIPS case you end up with an extra entry for the first 256MB that is on the same memory controller as the second 768MB. Arnd
> > .../devicetree/bindings/pci/brcm,brcmstb-pcie.txt | 98 > ++++++++++++++++++++++ > 1 file changed, 98 insertions(+) > create mode 100644 > Documentation/devicetree/bindings/pci/brcm,brcmstb-pcie.txt > > diff --git a/Documentation/devicetree/bindings/pci/brcm,brcmstb-pcie.txt > b/Documentation/devicetree/bindings/pci/brcm,brcmstb-pcie.txt > new file mode 100644 > index 000000000000..3682b0f0bc26 > --- /dev/null > +++ b/Documentation/devicetree/bindings/pci/brcm,brcmstb-pcie.txt > @@ -0,0 +1,98 @@ > +Broadcom STB PCIe Host Controller Device Tree Bindings > + > +This document describes the binding of the PCIe Root Complex hardware > +found in Broadcom Set Top Box System-on-Chips such as BCM7425 (MIPS), > +BCM7435 (MIPS) and > +BCM7445 (ARMv7). > + > +Required properties: > +- compatible: must be one of: "brcm,bcm7425-pcie" > + "brcm,bcm7435-pcie" > + "brcm,bcm7445-pcie" > + > +- reg: specifies the physical base address of the controller registers > +and > + its length > + > +- interrupt-parent: must be a reference (phandle) to the parent > +interrupt > + controller in the system (7038-l1-intc on MIPS, GIC on ARM/ARM64) > + > +- interrrupts: first interrupt must be the Level 1 interrupt number > +corresponding > + to the main PCIe RC interrupt, second interrupt must be the MSI > +interrupt > + See the interrupt-parent documentation for the number of cells and their > meaning: > + MIPS: > +Documentation/devicetree/bindings/interrupt-controller/brcm,bcm7038- > l1- > +intc.txt > + ARM/ARM64: > +Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt > + > +- interrupt-names: must be "pcie", and if present "msi" > + > +- interrupt-map: see pci.txt > + > +- interrupt-map-mask: see pci.txt > + > +- #address-cells: must be set to <3>, see pci.txt > + > +- #size-cells: must be set to <2>, see pci.txt > + > +- ranges: ranges for the PCI outbound windows, no I/O or prefetchable > +windows > + must be specified here, only non-prefetchable. 32-bits windows or > +64-bits > + windows are allowed based on the host processor's capabilities (ARM > +w/ LPAE, > + ARM64). > + > +- #interrupt-cells: set to <1>, see pci.txt > + > +- brcm,log2-scb-sizes: log2 size of the SCB window that is mapped to > +PCIe space > + there must be exactly one value per memory controller present in the > +system > + (ranges from 1 to 3) Can you please give more insight on what does this property do ? Bharat > + > +Optional properties: > + > +- msi-controller: indicates that this is a MSI controller node (when > +supported) > + > +- clocks: phandle to the functional clock that feeds into the PCIe RC > +block > + > +- clock-names: the name(s) of the clocks specified in 'clocks'. Note > + that if the 'clocks' property is given, 'clock-names' is mandatory, > + and the name of the clock is expected to be "pcie". > + > +- brcm,ssc: boolean that indicates usage of spread-spectrum clocking > + > +- brcm,gen: integer that indicates desired forced generation of link: 1 > +=> 2.5 > + Gbps, 2 => 5.0 Gbps, 3 => 8.0 Gbps. Will override the auto-negotation > +if > + specified. > + > +- <*>-supply: see > +Documentation/devicetree/bindings/regulator/regulator.txt > + > +- <*>-supply-names: see > +Documentation/devicetree/bindings/regulator/regulator.txt > + > +Example Node: > + > +This example assumes that the top-level #address-cells = <2> and > +#size-cells = <2>, e.g: ARM LPAE configuration. > + > + pcie0: pcie-controller@f0460000 { > + reg = <0x0 0xf0460000 0x0 0x9310>; > + interrupts = <0x0 0x33 0x4>, <0x0 0x34, 0x4>; > + interrupt-names = "pcie", "msi"; > + compatible = "brcm,bcm7445-pcie"; > + #address-cells = <3>; > + #size-cells = <2>; > + > + /* Two non-prefetchable 32-bits memory space, each of > 128MB > + * with the following mapping: > + * PCIe address => CPU physical address space > + * 0x00_0000_0000 => 0x00_C000_0000 > + * 0x00_0800_0000 => 0x00_C800_0000 > + */ > + ranges = <0x02000000 0x00000000 0x00000000 0x00000000 > 0xc0000000 0x00000000 0x08000000>, > + <0x02000000 0x00000000 0x08000000 0x00000000 > 0xc8000000 0x00000000 0x08000000>; > + #interrupt-cells = <1>; > + interrupt-map-mask = <0xf800 0 0 7>; > + interrupt-map = <0 0 0 1 &intc 47 3 > + 0 0 0 2 &intc 48 3 > + 0 0 0 3 &intc 49 3 > + 0 0 0 4 &intc 50 3>; > + clocks = <&pcie0>; > + clock-names = "pcie"; > + brcm,ssc; > + brcm,log2-scb-sizes = <0x1e 0x1e 0x1e>; > + vreg-wifi-pwr-supply-names = "vreg-wifi-pwr"; > + vreg-wifi-pwr-supply = <&vreg-wifi-pwr>; > + }; > -- > 2.1.0 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-pci" in the > body of a message to majordomo@vger.kernel.org More majordomo info at > http://vger.kernel.org/majordomo-info.html
On 05/11/2016 07:45 AM, Bharat Kumar Gogada wrote: >> +- brcm,log2-scb-sizes: log2 size of the SCB window that is mapped to >> +PCIe space >> + there must be exactly one value per memory controller present in the >> +system >> + (ranges from 1 to 3) > > Can you please give more insight on what does this property do ? There is another part of the thread that should tell you a bit more what this is about, here is where it starts: http://www.spinics.net/lists/linux-pci/msg50973.html And this is where we discuss what we should be doing about it: http://www.spinics.net/lists/linux-pci/msg51070.html
diff --git a/Documentation/devicetree/bindings/pci/brcm,brcmstb-pcie.txt b/Documentation/devicetree/bindings/pci/brcm,brcmstb-pcie.txt new file mode 100644 index 000000000000..3682b0f0bc26 --- /dev/null +++ b/Documentation/devicetree/bindings/pci/brcm,brcmstb-pcie.txt @@ -0,0 +1,98 @@ +Broadcom STB PCIe Host Controller Device Tree Bindings + +This document describes the binding of the PCIe Root Complex hardware found in +Broadcom Set Top Box System-on-Chips such as BCM7425 (MIPS), BCM7435 (MIPS) and +BCM7445 (ARMv7). + +Required properties: +- compatible: must be one of: "brcm,bcm7425-pcie" + "brcm,bcm7435-pcie" + "brcm,bcm7445-pcie" + +- reg: specifies the physical base address of the controller registers and + its length + +- interrupt-parent: must be a reference (phandle) to the parent interrupt + controller in the system (7038-l1-intc on MIPS, GIC on ARM/ARM64) + +- interrrupts: first interrupt must be the Level 1 interrupt number corresponding + to the main PCIe RC interrupt, second interrupt must be the MSI interrupt + See the interrupt-parent documentation for the number of cells and their meaning: + MIPS: Documentation/devicetree/bindings/interrupt-controller/brcm,bcm7038-l1-intc.txt + ARM/ARM64: Documentation/devicetree/bindings/interrupt-controller/arm,gic.txt + +- interrupt-names: must be "pcie", and if present "msi" + +- interrupt-map: see pci.txt + +- interrupt-map-mask: see pci.txt + +- #address-cells: must be set to <3>, see pci.txt + +- #size-cells: must be set to <2>, see pci.txt + +- ranges: ranges for the PCI outbound windows, no I/O or prefetchable windows + must be specified here, only non-prefetchable. 32-bits windows or 64-bits + windows are allowed based on the host processor's capabilities (ARM w/ LPAE, + ARM64). + +- #interrupt-cells: set to <1>, see pci.txt + +- brcm,log2-scb-sizes: log2 size of the SCB window that is mapped to PCIe space + there must be exactly one value per memory controller present in the system + (ranges from 1 to 3) + +Optional properties: + +- msi-controller: indicates that this is a MSI controller node (when supported) + +- clocks: phandle to the functional clock that feeds into the PCIe RC block + +- clock-names: the name(s) of the clocks specified in 'clocks'. Note + that if the 'clocks' property is given, 'clock-names' is mandatory, + and the name of the clock is expected to be "pcie". + +- brcm,ssc: boolean that indicates usage of spread-spectrum clocking + +- brcm,gen: integer that indicates desired forced generation of link: 1 => 2.5 + Gbps, 2 => 5.0 Gbps, 3 => 8.0 Gbps. Will override the auto-negotation if + specified. + +- <*>-supply: see Documentation/devicetree/bindings/regulator/regulator.txt + +- <*>-supply-names: see Documentation/devicetree/bindings/regulator/regulator.txt + +Example Node: + +This example assumes that the top-level #address-cells = <2> and #size-cells = +<2>, e.g: ARM LPAE configuration. + + pcie0: pcie-controller@f0460000 { + reg = <0x0 0xf0460000 0x0 0x9310>; + interrupts = <0x0 0x33 0x4>, <0x0 0x34, 0x4>; + interrupt-names = "pcie", "msi"; + compatible = "brcm,bcm7445-pcie"; + #address-cells = <3>; + #size-cells = <2>; + + /* Two non-prefetchable 32-bits memory space, each of 128MB + * with the following mapping: + * PCIe address => CPU physical address space + * 0x00_0000_0000 => 0x00_C000_0000 + * 0x00_0800_0000 => 0x00_C800_0000 + */ + ranges = <0x02000000 0x00000000 0x00000000 0x00000000 0xc0000000 0x00000000 0x08000000>, + <0x02000000 0x00000000 0x08000000 0x00000000 0xc8000000 0x00000000 0x08000000>; + #interrupt-cells = <1>; + interrupt-map-mask = <0xf800 0 0 7>; + interrupt-map = <0 0 0 1 &intc 47 3 + 0 0 0 2 &intc 48 3 + 0 0 0 3 &intc 49 3 + 0 0 0 4 &intc 50 3>; + clocks = <&pcie0>; + clock-names = "pcie"; + brcm,ssc; + brcm,log2-scb-sizes = <0x1e 0x1e 0x1e>; + vreg-wifi-pwr-supply-names = "vreg-wifi-pwr"; + vreg-wifi-pwr-supply = <&vreg-wifi-pwr>; + };