| Message ID | 1507761269-7017-3-git-send-email-jim2101024@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/9] SOC: brcmstb: add memory API | expand |
Hi Jim, On Wed, Oct 11, 2017 at 06:34:22PM -0400, Jim Quinlan wrote: > The DT bindings description of the Brcmstb PCIe device is described. This > node can be used by almost all Broadcom settop box chips, using > ARM, ARM64, or MIPS CPU architectures. > > Signed-off-by: Jim Quinlan <jim2101024@gmail.com> > --- > .../devicetree/bindings/pci/brcmstb-pci.txt | 106 +++++++++++++++++++++ > 1 file changed, 106 insertions(+) > create mode 100644 Documentation/devicetree/bindings/pci/brcmstb-pci.txt > > diff --git a/Documentation/devicetree/bindings/pci/brcmstb-pci.txt b/Documentation/devicetree/bindings/pci/brcmstb-pci.txt > new file mode 100644 > index 0000000..2f699da > --- /dev/null > +++ b/Documentation/devicetree/bindings/pci/brcmstb-pci.txt > @@ -0,0 +1,106 @@ > +Brcmstb PCIe Host Controller Device Tree Bindings > + > +Introduction: > + The brcmstb host controller closely follows the example set in > + > + [1] http://devicetree.org/Device_Tree_Usage#PCI_Host_Bridge > + > + The rest of this document explains some added customizations and > + offers an example Brcmstb PCIe host controller DT node. > + > +Required Properties: > + reg -- the register start address and length for the PCIe block. > + Additional start,length pairs may be specified for clock addresses. > + interrupts -- two interrupts are specified; the first interrupt is for > + the PCI host controller and the second is for MSI if the built-in > + MSI controller is to be used. > + interrupt-names -- names of the interrupts (above): "pcie" and "msi". > + compatible -- must be one of: "brcm,bcm7425-pcie", "brcm,bcm7435-pcie", > + or "brcm,bcm7278-pcie". > + #address-cells -- the number of address cells for PCI-space. > + #size-cells -- the number of size cells for PCI-space. > + ranges -- See [1]; a specification of the outbound windows for the host > + controller. Each outbound window is described by a n-tuple: > + (3 cells) -- PCIe space start address; one cell for attributes > + and two cells for the 64-bit PCIe address. > + (x cells) -- CPU/System start address, number of cells is determined > + by the parent node's #address-cells. > + (y cells) -- Size of region, number of cells determined by the > + parent node's #size-cells. > + Due to hardware limitations, there may be a maximum of four > + non-contiguous ranges specified. > + #interrupt-cells -- number of cells used to describe the interrupt. > + interrupt-map-mask -- see [1]; four cells, the first three are zero > + for our uses and the fourth cell is the mask (val = 0x7) for > + the legacy interrupt number [1..4]. > + interrupt-map -- See [1]; there are four interrupts (INTA, INTB, > + INTC, and INTD) to be mapped; each interrupt requires 5 cells > + plus the size of the interrupt specifier. > + linux,pci-domain -- the domain of the host controller. > + > +Optional Properties: > + clocks -- list of clock phandles. If specified, this should list one > + clock. > + clock-names -- the "local" names 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 "sw_pcie". > + dma-ranges -- Similar in structure to ranges, each dma region is > + specified with a n-tuple. Dma-regions describe the inbound > + accesses from EP to RC; it translates the pci address that the > + EP "sees" to the CPU address in memory. This property is needed > + because the design of the Brcmstb memory subsystem often precludes > + idenity-mapping between CPU address space and PCIe address space. > + Each range is described by a n-tuple: > + (3 cells) -- PCIe space start address; one cell for attributes > + and two cells for the 64-bit PCIe address. > + (x cells) -- CPU/System start address, number of cells is determined > + by the parent node's #address-cells. > + (y cells) -- Size of region, number of cells determined by the > + parent node's #size-cells. > + msi-parent -- if MSI is to be used, this must be a phandle to the > + msi-parent. If this prop is set to the phandle of the PCIe > + node, or if the msi-parent prop is missing, the PCIE controller > + will attempt to use its built in MSI controller. > + msi-controller -- this property should only be specified if the > + PCIe controller is using its internal MSI controller. > + brcm,ssc -- (boolean) indicates usage of spread-spectrum clocking. > + brcm,gen -- (integer) indicates desired generation of link: > + 1 => 2.5 Gbps, 2 => 5.0 Gbps, 3 => 8.0 Gbps. Does this differ from the 'max-link-speed' now documented in Documentation/devicetree/bindings/pci/pci.txt? If not, might as well use it, and of_pci_get_max_link_speed(). > + supply-names -- the names of voltage regulators that the root > + complex should turn off/on/on on suspend/resume/boot. This > + is a string list. > + supplies -- A collection of phandles to a regulator nodes, see > + Documentation/devicetree/bindings/regulator/ for specific > + bindings. The number and order of phandles must match > + exactly the number of strings in the "supply-names" property. > + > +Example Node: > + > +pcie0: pcie@f0460000 { ^^ You've got a tab after the colon. Makes this look funky in my vim/mutt :) Brian > + reg = <0x0 0xf0460000 0x0 0x9310>; > + interrupts = <0x0 0x0 0x4>; > + compatible = "brcm,pci-plat-dev"; > + #address-cells = <3>; > + #size-cells = <2>; > + ranges = <0x02000000 0x00000000 0x00000000 0x00000000 0xc0000000 0x00000000 0x08000000 > + 0x02000000 0x00000000 0x08000000 0x00000000 0xc8000000 0x00000000 0x08000000>; > + #interrupt-cells = <1>; > + interrupt-map-mask = <0 0 0 7>; > + interrupt-map = <0 0 0 1 &intc 0 47 3 > + 0 0 0 2 &intc 0 48 3 > + 0 0 0 3 &intc 0 49 3 > + 0 0 0 4 &intc 0 50 3>; > + interrupt-names = "pcie_0_inta", > + "pcie_0_intb", > + "pcie_0_intc", > + "pcie_0_intd"; > + clocks = <&sw_pcie0>; > + clock-names = "sw_pcie"; > + msi-parent = <&pcie0>; /* use PCIe's internal MSI controller */ > + msi-controller; /* use PCIe's internal MSI controller */ > + brcm,ssc; > + brcm,gen = <1>; > + supply-names = "vreg-wifi-pwr"; > + supplies = <&vreg-wifi-pwr>; > + linux,pci-domain = <0>; > + }; > -- > 1.9.0.138.g2de3478 >
On Wed, Oct 11, 2017 at 8:55 PM, Brian Norris <computersforpeace@gmail.com> wrote: > Hi Jim, > > On Wed, Oct 11, 2017 at 06:34:22PM -0400, Jim Quinlan wrote: pcie->gen = 0; >> The DT bindings description of the Brcmstb PCIe device is described. This >> node can be used by almost all Broadcom settop box chips, using >> ARM, ARM64, or MIPS CPU architectures. >> >> Signed-off-by: Jim Quinlan <jim2101024@gmail.com> >> --- >> .../devicetree/bindings/pci/brcmstb-pci.txt | 106 +++++++++++++++++++++ >> 1 file changed, 106 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/pci/brcmstb-pci.txt >> >> diff --git a/Documentation/devicetree/bindings/pci/brcmstb-pci.txt b/Documentation/devicetree/bindings/pci/brcmstb-pci.txt >> new file mode 100644 >> index 0000000..2f699da >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/pci/brcmstb-pci.txt >> @@ -0,0 +1,106 @@ >> +Brcmstb PCIe Host Controller Device Tree Bindings >> + >> +Introduction: >> + The brcmstb host controller closely follows the example set in >> + >> + [1] http://devicetree.org/Device_Tree_Usage#PCI_Host_Bridge >> + >> + The rest of this document explains some added customizations and >> + offers an example Brcmstb PCIe host controller DT node. >> + >> +Required Properties: >> + reg -- the register start address and length for the PCIe block. >> + Additional start,length pairs may be specified for clock addresses. >> + interrupts -- two interrupts are specified; the first interrupt is for >> + the PCI host controller and the second is for MSI if the built-in >> + MSI controller is to be used. >> + interrupt-names -- names of the interrupts (above): "pcie" and "msi". >> + compatible -- must be one of: "brcm,bcm7425-pcie", "brcm,bcm7435-pcie", >> + or "brcm,bcm7278-pcie". >> + #address-cells -- the number of address cells for PCI-space. >> + #size-cells -- the number of size cells for PCI-space. >> + ranges -- See [1]; a specification of the outbound windows for the host >> + controller. Each outbound window is described by a n-tuple: >> + (3 cells) -- PCIe space start address; one cell for attributes >> + and two cells for the 64-bit PCIe address. >> + (x cells) -- CPU/System start address, number of cells is determined >> + by the parent node's #address-cells. >> + (y cells) -- Size of region, number of cells determined by the >> + parent node's #size-cells. >> + Due to hardware limitations, there may be a maximum of four >> + non-contiguous ranges specified. >> + #interrupt-cells -- number of cells used to describe the interrupt. >> + interrupt-map-mask -- see [1]; four cells, the first three are zero >> + for our uses and the fourth cell is the mask (val = 0x7) for >> + the legacy interrupt number [1..4]. >> + interrupt-map -- See [1]; there are four interrupts (INTA, INTB, >> + INTC, and INTD) to be mapped; each interrupt requires 5 cells >> + plus the size of the interrupt specifier. >> + linux,pci-domain -- the domain of the host controller. >> + >> +Optional Properties: >> + clocks -- list of clock phandles. If specified, this should list one >> + clock. >> + clock-names -- the "local" names 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 "sw_pcie". >> + dma-ranges -- Similar in structure to ranges, each dma region is >> + specified with a n-tuple. Dma-regions describe the inbound >> + accesses from EP to RC; it translates the pci address that the >> + EP "sees" to the CPU address in memory. This property is needed >> + because the design of the Brcmstb memory subsystem often precludes >> + idenity-mapping between CPU address space and PCIe address space. >> + Each range is described by a n-tuple: >> + (3 cells) -- PCIe space start address; one cell for attributes >> + and two cells for the 64-bit PCIe address. >> + (x cells) -- CPU/System start address, number of cells is determined >> + by the parent node's #address-cells. >> + (y cells) -- Size of region, number of cells determined by the >> + parent node's #size-cells. >> + msi-parent -- if MSI is to be used, this must be a phandle to the >> + msi-parent. If this prop is set to the phandle of the PCIe >> + node, or if the msi-parent prop is missing, the PCIE controller >> + will attempt to use its built in MSI controller. >> + msi-controller -- this property should only be specified if the >> + PCIe controller is using its internal MSI controller. >> + brcm,ssc -- (boolean) indicates usage of spread-spectrum clocking. >> + brcm,gen -- (integer) indicates desired generation of link: >> + 1 => 2.5 Gbps, 2 => 5.0 Gbps, 3 => 8.0 Gbps. > > Does this differ from the 'max-link-speed' now documented in > Documentation/devicetree/bindings/pci/pci.txt? If not, might as well use > it, and of_pci_get_max_link_speed(). > Hi Brian, yes that's what it is, will fix. >> + supply-names -- the names of voltage regulators that the root >> + complex should turn off/on/on on suspend/resume/boot. This >> + is a string list. >> + supplies -- A collection of phandles to a regulator nodes, see >> + Documentation/devicetree/bindings/regulator/ for specific >> + bindings. The number and order of phandles must match >> + exactly the number of strings in the "supply-names" property. >> + >> +Example Node: >> + >> +pcie0: pcie@f0460000 { > > ^^ You've got a tab after the colon. Makes this look funky in my > vim/mutt :) > > Brian > Will fix. >> + reg = <0x0 0xf0460000 0x0 0x9310>; >> + interrupts = <0x0 0x0 0x4>; >> + compatible = "brcm,pci-plat-dev"; >> + #address-cells = <3>; >> + #size-cells = <2>; >> + ranges = <0x02000000 0x00000000 0x00000000 0x00000000 0xc0000000 0x00000000 0x08000000 >> + 0x02000000 0x00000000 0x08000000 0x00000000 0xc8000000 0x00000000 0x08000000>; >> + #interrupt-cells = <1>; >> + interrupt-map-mask = <0 0 0 7>; >> + interrupt-map = <0 0 0 1 &intc 0 47 3 >> + 0 0 0 2 &intc 0 48 3 >> + 0 0 0 3 &intc 0 49 3 >> + 0 0 0 4 &intc 0 50 3>; >> + interrupt-names = "pcie_0_inta", >> + "pcie_0_intb", >> + "pcie_0_intc", >> + "pcie_0_intd"; >> + clocks = <&sw_pcie0>; >> + clock-names = "sw_pcie"; >> + msi-parent = <&pcie0>; /* use PCIe's internal MSI controller */ >> + msi-controller; /* use PCIe's internal MSI controller */ >> + brcm,ssc; >> + brcm,gen = <1>; >> + supply-names = "vreg-wifi-pwr"; >> + supplies = <&vreg-wifi-pwr>; >> + linux,pci-domain = <0>; >> + }; >> -- >> 1.9.0.138.g2de3478 >>
On Wed, Oct 11, 2017 at 06:34:22PM -0400, Jim Quinlan wrote: > The DT bindings description of the Brcmstb PCIe device is described. This > node can be used by almost all Broadcom settop box chips, using > ARM, ARM64, or MIPS CPU architectures. > > Signed-off-by: Jim Quinlan <jim2101024@gmail.com> > --- > .../devicetree/bindings/pci/brcmstb-pci.txt | 106 +++++++++++++++++++++ > 1 file changed, 106 insertions(+) > create mode 100644 Documentation/devicetree/bindings/pci/brcmstb-pci.txt > > diff --git a/Documentation/devicetree/bindings/pci/brcmstb-pci.txt b/Documentation/devicetree/bindings/pci/brcmstb-pci.txt > new file mode 100644 > index 0000000..2f699da > --- /dev/null > +++ b/Documentation/devicetree/bindings/pci/brcmstb-pci.txt > @@ -0,0 +1,106 @@ > +Brcmstb PCIe Host Controller Device Tree Bindings > + > +Introduction: > + The brcmstb host controller closely follows the example set in > + > + [1] http://devicetree.org/Device_Tree_Usage#PCI_Host_Bridge > + > + The rest of this document explains some added customizations and > + offers an example Brcmstb PCIe host controller DT node. > + > +Required Properties: > + reg -- the register start address and length for the PCIe block. > + Additional start,length pairs may be specified for clock addresses. Kind of vague and why do you need clock addresses and the clock binding? Also, typically the config space is defined here? Is that missing or you don't support memory mapped config space? > + interrupts -- two interrupts are specified; the first interrupt is for > + the PCI host controller and the second is for MSI if the built-in > + MSI controller is to be used. > + interrupt-names -- names of the interrupts (above): "pcie" and "msi". > + compatible -- must be one of: "brcm,bcm7425-pcie", "brcm,bcm7435-pcie", > + or "brcm,bcm7278-pcie". One compatible per line. > + #address-cells -- the number of address cells for PCI-space. > + #size-cells -- the number of size cells for PCI-space. > + ranges -- See [1]; a specification of the outbound windows for the host > + controller. Each outbound window is described by a n-tuple: > + (3 cells) -- PCIe space start address; one cell for attributes > + and two cells for the 64-bit PCIe address. > + (x cells) -- CPU/System start address, number of cells is determined > + by the parent node's #address-cells. > + (y cells) -- Size of region, number of cells determined by the > + parent node's #size-cells. > + Due to hardware limitations, there may be a maximum of four > + non-contiguous ranges specified. > + #interrupt-cells -- number of cells used to describe the interrupt. How many cells? > + interrupt-map-mask -- see [1]; four cells, the first three are zero > + for our uses and the fourth cell is the mask (val = 0x7) for > + the legacy interrupt number [1..4]. > + interrupt-map -- See [1]; there are four interrupts (INTA, INTB, > + INTC, and INTD) to be mapped; each interrupt requires 5 cells > + plus the size of the interrupt specifier. > + linux,pci-domain -- the domain of the host controller. > + > +Optional Properties: > + clocks -- list of clock phandles. If specified, this should list one > + clock. > + clock-names -- the "local" names 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 "sw_pcie". > + dma-ranges -- Similar in structure to ranges, each dma region is > + specified with a n-tuple. Dma-regions describe the inbound > + accesses from EP to RC; it translates the pci address that the > + EP "sees" to the CPU address in memory. This property is needed > + because the design of the Brcmstb memory subsystem often precludes > + idenity-mapping between CPU address space and PCIe address space. > + Each range is described by a n-tuple: > + (3 cells) -- PCIe space start address; one cell for attributes > + and two cells for the 64-bit PCIe address. > + (x cells) -- CPU/System start address, number of cells is determined > + by the parent node's #address-cells. > + (y cells) -- Size of region, number of cells determined by the > + parent node's #size-cells. There's no need to describe standard properties. Just put whatever is specific to your platform. That applies throughout this doc. > + msi-parent -- if MSI is to be used, this must be a phandle to the > + msi-parent. If this prop is set to the phandle of the PCIe > + node, or if the msi-parent prop is missing, the PCIE controller > + will attempt to use its built in MSI controller. > + msi-controller -- this property should only be specified if the > + PCIe controller is using its internal MSI controller. > + brcm,ssc -- (boolean) indicates usage of spread-spectrum clocking. > + brcm,gen -- (integer) indicates desired generation of link: > + 1 => 2.5 Gbps, 2 => 5.0 Gbps, 3 => 8.0 Gbps. We have a standard property for this IIRC. > + supply-names -- the names of voltage regulators that the root > + complex should turn off/on/on on suspend/resume/boot. This > + is a string list. > + supplies -- A collection of phandles to a regulator nodes, see > + Documentation/devicetree/bindings/regulator/ for specific > + bindings. The number and order of phandles must match > + exactly the number of strings in the "supply-names" property. This is not the regulator binding. Use the standard binding. > + > +Example Node: > + > +pcie0: pcie@f0460000 { > + reg = <0x0 0xf0460000 0x0 0x9310>; > + interrupts = <0x0 0x0 0x4>; > + compatible = "brcm,pci-plat-dev"; > + #address-cells = <3>; > + #size-cells = <2>; > + ranges = <0x02000000 0x00000000 0x00000000 0x00000000 0xc0000000 0x00000000 0x08000000 > + 0x02000000 0x00000000 0x08000000 0x00000000 0xc8000000 0x00000000 0x08000000>; > + #interrupt-cells = <1>; > + interrupt-map-mask = <0 0 0 7>; > + interrupt-map = <0 0 0 1 &intc 0 47 3 > + 0 0 0 2 &intc 0 48 3 > + 0 0 0 3 &intc 0 49 3 > + 0 0 0 4 &intc 0 50 3>; > + interrupt-names = "pcie_0_inta", > + "pcie_0_intb", > + "pcie_0_intc", > + "pcie_0_intd"; > + clocks = <&sw_pcie0>; > + clock-names = "sw_pcie"; > + msi-parent = <&pcie0>; /* use PCIe's internal MSI controller */ > + msi-controller; /* use PCIe's internal MSI controller */ > + brcm,ssc; > + brcm,gen = <1>; > + supply-names = "vreg-wifi-pwr"; > + supplies = <&vreg-wifi-pwr>; > + linux,pci-domain = <0>; > + }; > -- > 1.9.0.138.g2de3478 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Tue, Oct 17, 2017 at 4:24 PM, Rob Herring <robh@kernel.org> wrote: > On Wed, Oct 11, 2017 at 06:34:22PM -0400, Jim Quinlan wrote: >> The DT bindings description of the Brcmstb PCIe device is described. This >> node can be used by almost all Broadcom settop box chips, using >> ARM, ARM64, or MIPS CPU architectures. >> >> Signed-off-by: Jim Quinlan <jim2101024@gmail.com> >> --- >> .../devicetree/bindings/pci/brcmstb-pci.txt | 106 +++++++++++++++++++++ >> 1 file changed, 106 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/pci/brcmstb-pci.txt >> >> diff --git a/Documentation/devicetree/bindings/pci/brcmstb-pci.txt b/Documentation/devicetree/bindings/pci/brcmstb-pci.txt >> new file mode 100644 >> index 0000000..2f699da >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/pci/brcmstb-pci.txt >> @@ -0,0 +1,106 @@ >> +Brcmstb PCIe Host Controller Device Tree Bindings >> +We don't; this line is erroneous. >> +Introduction: >> + The brcmstb host controller closely follows the example set in >> + >> + [1] http://devicetree.org/Device_Tree_Usage#PCI_Host_Bridge >> + >> + The rest of this document explains some added customizations and >> + offers an example Brcmstb PCIe host controller DT node. >> + >> +Required Properties: >> + reg -- the register start address and length for the PCIe block. >> + Additional start,length pairs may be specified for clock addresses. > > Kind of vague and why do you need clock addresses and the clock binding? > We don't; this line is erroneous and will be removed. > Also, typically the config space is defined here? Is that missing or you > don't support memory mapped config space? > We do not support memory mapped config space. >> + interrupts -- two interrupts are specified; the first interrupt is for >> + the PCI host controller and the second is for MSI if the built-in >> + MSI controller is to be used. >> + interrupt-names -- names of the interrupts (above): "pcie" and "msi". >> + compatible -- must be one of: "brcm,bcm7425-pcie", "brcm,bcm7435-pcie", >> + or "brcm,bcm7278-pcie". > > One compatible per line. > Will fix. >> + #address-cells -- the number of address cells for PCI-space. >> + #size-cells -- the number of size cells for PCI-space. >> + ranges -- See [1]; a specification of the outbound windows for the host >> + controller. Each outbound window is described by a n-tuple: >> + (3 cells) -- PCIe space start address; one cell for attributes >> + and two cells for the 64-bit PCIe address. >> + (x cells) -- CPU/System start address, number of cells is determined >> + by the parent node's #address-cells. >> + (y cells) -- Size of region, number of cells determined by the >> + parent node's #size-cells. >> + Due to hardware limitations, there may be a maximum of four >> + non-contiguous ranges specified.We don't; this line is erroneous. >> + #interrupt-cells -- number of cells used to describe the interrupt. > > How many cells? > This line will be removed. >> + interrupt-map-mask -- see [1]; four cells, the first three are zero >> + for our uses and the fourth cell is the mask (val = 0x7) for >> + the legacy interrupt number [1..4]. >> + interrupt-map -- See [1]; there are four interrupts (INTA, INTB, >> + INTC, and INTD) to be mapped; each interrupt requires 5 cells >> + plus the size of the interrupt specifier. >> + linux,pci-domain -- the domain of the host controller. >> + >> +Optional Properties: >> + clocks -- list of clock phandles. If specified, this should list one >> + clock. >> + clock-names -- the "local" names 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 "sw_pcie". >> + dma-ranges -- Similar in structure to ranges, each dma region is >> + specified with a n-tuple. Dma-regions describe the inbound >> + accesses from EP to RC; it translates the pci address that the >> + EP "sees" to the CPU address in memory. This property is needed >> + because the design of the Brcmstb memory subsystem often precludes >> + idenity-mapping between CPU address space and PCIe address space. >> + Each range is described by a n-tuple: >> + (3 cells) -- PCIe space start address; one cell for attributes >> + and two cells for the 64-bit PCIe address. >> + (x cells) -- CPU/System start address, number of cells is determined >> + by the parent node's #address-cells. >> + (y cells) -- Size of region, number of cells determined by the >> + parent node's #size-cells. > > There's no need to describe standard properties. Just put whatever is > specific to your platform. That applies throughout this doc. > Will fix. >> + msi-parent -- if MSI is to be used, this must be a phandle to the >> + msi-parent. If this prop is set to the phandle of the PCIe >> + node, or if the msi-parent prop is missing, the PCIE controller >> + will attempt to use its built in MSI controller. >> + msi-controller -- this property should only be specified if the >> + PCIe controller is using its internal MSI controller. >> + brcm,ssc -- (boolean) indicates usage of spread-spectrum clocking. >> + brcm,gen -- (integer) indicates desired generation of link: >> + 1 => 2.5 Gbps, 2 => 5.0 Gbps, 3 => 8.0 Gbps. > > We have a standard property for this IIRC. > Yes, BrianN pointed that out and it will be fixed. >> + supply-names -- the names of voltage regulators that the root >> + complex should turn off/on/on on suspend/resume/boot. This >> + is a string list. >> + supplies -- A collection of phandles to a regulator nodes, see >> + Documentation/devicetree/bindings/regulator/ for specificWe don't; this line is erroneous. >> + bindings. The number and order of phandles must match >> + exactly the number of strings in the "supply-names" property. > > This is not the regulator binding. Use the standard binding. > The reason we do it this way is because the PCIe controller does not know or care what the names of the supplies are, or how many there will be. The list of regulators can be different for each board we support, as these regulators turn on/off the downstream EP device. All the PCIe controller does is turn on/off this list of regulators when booting,resuming/suspending. An alternative would have the node specifying the standard properties xyz-supply = <&xyz_reg>; abc-supply = <&abc_reg>; pdq-supply = <&pdq_reg>; ... and then have this driver search all of the properties in the PCIe node for names matching /-supply$/, and then create a list of phandles from that. Is that what you would like? Thanks, Jim >> + >> +Example Node: >> + >> +pcie0: pcie@f0460000 { >> + reg = <0x0 0xf0460000 0x0 0x9310>; >> + interrupts = <0x0 0x0 0x4>; >> + compatible = "brcm,pci-plat-dev"; >> + #address-cells = <3>; >> + #size-cells = <2>; >> + ranges = <0x02000000 0x00000000 0x00000000 0x00000000 0xc0000000 0x00000000 0x08000000 >> + 0x02000000 0x00000000 0x08000000 0x00000000 0xc8000000 0x00000000 0x08000000>; >> + #interrupt-cells = <1>; >> + interrupt-map-mask = <0 0 0 7>; >> + interrupt-map = <0 0 0 1 &intc 0 47 3 >> + 0 0 0 2 &intc 0 48 3 >> + 0 0 0 3 &intc 0 49 3 >> + 0 0 0 4 &intc 0 50 3>; >> + interrupt-names = "pcie_0_inta", >> + "pcie_0_intb", >> + "pcie_0_intc", >> + "pcie_0_intd"; >> + clocks = <&sw_pcie0>; >> + clock-names = "sw_pcie"; >> + msi-parent = <&pcie0>; /* use PCIe's internal MSI controller */ >> + msi-controller; /* use PCIe's internal MSI controller */ >> + brcm,ssc; >> + brcm,gen = <1>; >> + supply-names = "vreg-wifi-pwr"; >> + supplies = <&vreg-wifi-pwr>; >> + linux,pci-domain = <0>; >> + }; >> -- >> 1.9.0.138.g2de3478 >> >> >> _______________________________________________ >> linux-arm-kernel mailing list >> linux-arm-kernel@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Tue, Oct 17, 2017 at 5:42 PM, Jim Quinlan <jim2101024@gmail.com> wrote: > On Tue, Oct 17, 2017 at 4:24 PM, Rob Herring <robh@kernel.org> wrote: >> On Wed, Oct 11, 2017 at 06:34:22PM -0400, Jim Quinlan wrote: >>> The DT bindings description of the Brcmstb PCIe device is described. This >>> node can be used by almost all Broadcom settop box chips, using >>> ARM, ARM64, or MIPS CPU architectures. >>> >>> Signed-off-by: Jim Quinlan <jim2101024@gmail.com> >>> --- >>> .../devicetree/bindings/pci/brcmstb-pci.txt | 106 +++++++++++++++++++++ >>> 1 file changed, 106 insertions(+) >>> create mode 100644 Documentation/devicetree/bindings/pci/brcmstb-pci.txt >>> >>> diff --git a/Documentation/devicetree/bindings/pci/brcmstb-pci.txt b/Documentation/devicetree/bindings/pci/brcmstb-pci.txt >>> new file mode 100644 >>> index 0000000..2f699da >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/pci/brcmstb-pci.txt >>> @@ -0,0 +1,106 @@ >>> +Brcmstb PCIe Host Controller Device Tree Bindings >>> +We don't; this line is erroneous. >>> +Introduction: >>> + The brcmstb host controller closely follows the example set in >>> + >>> + [1] http://devicetree.org/Device_Tree_Usage#PCI_Host_Bridge >>> + >>> + The rest of this document explains some added customizations and >>> + offers an example Brcmstb PCIe host controller DT node. >>> + >>> +Required Properties: >>> + reg -- the register start address and length for the PCIe block. >>> + Additional start,length pairs may be specified for clock addresses. >> >> Kind of vague and why do you need clock addresses and the clock binding? >> > We don't; this line is erroneous and will be removed. > >> Also, typically the config space is defined here? Is that missing or you >> don't support memory mapped config space? >> > We do not support memory mapped config space. > >>> + interrupts -- two interrupts are specified; the first interrupt is for >>> + the PCI host controller and the second is for MSI if the built-in >>> + MSI controller is to be used. >>> + interrupt-names -- names of the interrupts (above): "pcie" and "msi". >>> + compatible -- must be one of: "brcm,bcm7425-pcie", "brcm,bcm7435-pcie", >>> + or "brcm,bcm7278-pcie". >> >> One compatible per line. >> > Will fix. > >>> + #address-cells -- the number of address cells for PCI-space. >>> + #size-cells -- the number of size cells for PCI-space. >>> + ranges -- See [1]; a specification of the outbound windows for the host >>> + controller. Each outbound window is described by a n-tuple: >>> + (3 cells) -- PCIe space start address; one cell for attributes >>> + and two cells for the 64-bit PCIe address. >>> + (x cells) -- CPU/System start address, number of cells is determined >>> + by the parent node's #address-cells. >>> + (y cells) -- Size of region, number of cells determined by the >>> + parent node's #size-cells. >>> + Due to hardware limitations, there may be a maximum of four >>> + non-contiguous ranges specified.We don't; this line is erroneous. >>> + #interrupt-cells -- number of cells used to describe the interrupt. >> >> How many cells? >> > This line will be removed. Humm, why? You need it to have interrupt-map. You just need to say what the value is, not what the property is. >>> + interrupt-map-mask -- see [1]; four cells, the first three are zero >>> + for our uses and the fourth cell is the mask (val = 0x7) for >>> + the legacy interrupt number [1..4]. >>> + interrupt-map -- See [1]; there are four interrupts (INTA, INTB, >>> + INTC, and INTD) to be mapped; each interrupt requires 5 cells >>> + plus the size of the interrupt specifier. >>> + linux,pci-domain -- the domain of the host controller. >>> + >>> +Optional Properties: >>> + clocks -- list of clock phandles. If specified, this should list one >>> + clock. >>> + clock-names -- the "local" names 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 "sw_pcie". >>> + dma-ranges -- Similar in structure to ranges, each dma region is >>> + specified with a n-tuple. Dma-regions describe the inbound >>> + accesses from EP to RC; it translates the pci address that the >>> + EP "sees" to the CPU address in memory. This property is needed >>> + because the design of the Brcmstb memory subsystem often precludes >>> + idenity-mapping between CPU address space and PCIe address space. >>> + Each range is described by a n-tuple: >>> + (3 cells) -- PCIe space start address; one cell for attributes >>> + and two cells for the 64-bit PCIe address. >>> + (x cells) -- CPU/System start address, number of cells is determined >>> + by the parent node's #address-cells. >>> + (y cells) -- Size of region, number of cells determined by the >>> + parent node's #size-cells. >> >> There's no need to describe standard properties. Just put whatever is >> specific to your platform. That applies throughout this doc. >> > Will fix. > >>> + msi-parent -- if MSI is to be used, this must be a phandle to the >>> + msi-parent. If this prop is set to the phandle of the PCIe >>> + node, or if the msi-parent prop is missing, the PCIE controller >>> + will attempt to use its built in MSI controller. >>> + msi-controller -- this property should only be specified if the >>> + PCIe controller is using its internal MSI controller. >>> + brcm,ssc -- (boolean) indicates usage of spread-spectrum clocking. >>> + brcm,gen -- (integer) indicates desired generation of link: >>> + 1 => 2.5 Gbps, 2 => 5.0 Gbps, 3 => 8.0 Gbps. >> >> We have a standard property for this IIRC. >> > Yes, BrianN pointed that out and it will be fixed. > >>> + supply-names -- the names of voltage regulators that the root >>> + complex should turn off/on/on on suspend/resume/boot. This >>> + is a string list. >>> + supplies -- A collection of phandles to a regulator nodes, see >>> + Documentation/devicetree/bindings/regulator/ for specificWe don't; this line is erroneous. >>> + bindings. The number and order of phandles must match >>> + exactly the number of strings in the "supply-names" property. >> >> This is not the regulator binding. Use the standard binding. >> > The reason we do it this way is because the PCIe controller does not > know or care what the names of the supplies are, or how many there > will be. The list of regulators can be different for each board we > support, as these regulators turn on/off the downstream EP device. > All the PCIe controller does is turn on/off this list of regulators > when booting,resuming/suspending. > > An alternative would have the node specifying the standard properties > > xyz-supply = <&xyz_reg>; > abc-supply = <&abc_reg>; > pdq-supply = <&pdq_reg>; > ... > > and then have this driver search all of the properties in the PCIe > node for names matching /-supply$/, and then create a list of phandles > from that. Is that what you would like? Really, you should have child nodes of the PCIe devices and have the supplies in there. The driver could do what you describe, but you've still got to define the names here. Rob
On 10/19/2017 02:49 PM, Rob Herring wrote: > On Tue, Oct 17, 2017 at 5:42 PM, Jim Quinlan <jim2101024@gmail.com> wrote: >> On Tue, Oct 17, 2017 at 4:24 PM, Rob Herring <robh@kernel.org> wrote: >>> On Wed, Oct 11, 2017 at 06:34:22PM -0400, Jim Quinlan wrote: >>>> The DT bindings description of the Brcmstb PCIe device is described. This >>>> node can be used by almost all Broadcom settop box chips, using >>>> ARM, ARM64, or MIPS CPU architectures. >>>> >>>> Signed-off-by: Jim Quinlan <jim2101024@gmail.com> >>>> --- >>>> .../devicetree/bindings/pci/brcmstb-pci.txt | 106 +++++++++++++++++++++ >>>> 1 file changed, 106 insertions(+) >>>> create mode 100644 Documentation/devicetree/bindings/pci/brcmstb-pci.txt >>>> >>>> diff --git a/Documentation/devicetree/bindings/pci/brcmstb-pci.txt b/Documentation/devicetree/bindings/pci/brcmstb-pci.txt >>>> new file mode 100644 >>>> index 0000000..2f699da >>>> --- /dev/null >>>> +++ b/Documentation/devicetree/bindings/pci/brcmstb-pci.txt >>>> @@ -0,0 +1,106 @@ >>>> +Brcmstb PCIe Host Controller Device Tree Bindings >>>> +We don't; this line is erroneous. >>>> +Introduction: >>>> + The brcmstb host controller closely follows the example set in >>>> + >>>> + [1] http://devicetree.org/Device_Tree_Usage#PCI_Host_Bridge >>>> + >>>> + The rest of this document explains some added customizations and >>>> + offers an example Brcmstb PCIe host controller DT node. >>>> + >>>> +Required Properties: >>>> + reg -- the register start address and length for the PCIe block. >>>> + Additional start,length pairs may be specified for clock addresses. >>> >>> Kind of vague and why do you need clock addresses and the clock binding? >>> >> We don't; this line is erroneous and will be removed. >> >>> Also, typically the config space is defined here? Is that missing or you >>> don't support memory mapped config space? >>> >> We do not support memory mapped config space. >> >>>> + interrupts -- two interrupts are specified; the first interrupt is for >>>> + the PCI host controller and the second is for MSI if the built-in >>>> + MSI controller is to be used. >>>> + interrupt-names -- names of the interrupts (above): "pcie" and "msi". >>>> + compatible -- must be one of: "brcm,bcm7425-pcie", "brcm,bcm7435-pcie", >>>> + or "brcm,bcm7278-pcie". >>> >>> One compatible per line. >>> >> Will fix. >> >>>> + #address-cells -- the number of address cells for PCI-space. >>>> + #size-cells -- the number of size cells for PCI-space. >>>> + ranges -- See [1]; a specification of the outbound windows for the host >>>> + controller. Each outbound window is described by a n-tuple: >>>> + (3 cells) -- PCIe space start address; one cell for attributes >>>> + and two cells for the 64-bit PCIe address. >>>> + (x cells) -- CPU/System start address, number of cells is determined >>>> + by the parent node's #address-cells. >>>> + (y cells) -- Size of region, number of cells determined by the >>>> + parent node's #size-cells. >>>> + Due to hardware limitations, there may be a maximum of four >>>> + non-contiguous ranges specified.We don't; this line is erroneous. >>>> + #interrupt-cells -- number of cells used to describe the interrupt. >>> >>> How many cells? >>> >> This line will be removed. > > Humm, why? You need it to have interrupt-map. You just need to say > what the value is, not what the property is. > >>>> + interrupt-map-mask -- see [1]; four cells, the first three are zero >>>> + for our uses and the fourth cell is the mask (val = 0x7) for >>>> + the legacy interrupt number [1..4]. >>>> + interrupt-map -- See [1]; there are four interrupts (INTA, INTB, >>>> + INTC, and INTD) to be mapped; each interrupt requires 5 cells >>>> + plus the size of the interrupt specifier. >>>> + linux,pci-domain -- the domain of the host controller. >>>> + >>>> +Optional Properties: >>>> + clocks -- list of clock phandles. If specified, this should list one >>>> + clock. >>>> + clock-names -- the "local" names 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 "sw_pcie". >>>> + dma-ranges -- Similar in structure to ranges, each dma region is >>>> + specified with a n-tuple. Dma-regions describe the inbound >>>> + accesses from EP to RC; it translates the pci address that the >>>> + EP "sees" to the CPU address in memory. This property is needed >>>> + because the design of the Brcmstb memory subsystem often precludes >>>> + idenity-mapping between CPU address space and PCIe address space. >>>> + Each range is described by a n-tuple: >>>> + (3 cells) -- PCIe space start address; one cell for attributes >>>> + and two cells for the 64-bit PCIe address. >>>> + (x cells) -- CPU/System start address, number of cells is determined >>>> + by the parent node's #address-cells. >>>> + (y cells) -- Size of region, number of cells determined by the >>>> + parent node's #size-cells. >>> >>> There's no need to describe standard properties. Just put whatever is >>> specific to your platform. That applies throughout this doc. >>> >> Will fix. >> >>>> + msi-parent -- if MSI is to be used, this must be a phandle to the >>>> + msi-parent. If this prop is set to the phandle of the PCIe >>>> + node, or if the msi-parent prop is missing, the PCIE controller >>>> + will attempt to use its built in MSI controller. >>>> + msi-controller -- this property should only be specified if the >>>> + PCIe controller is using its internal MSI controller. >>>> + brcm,ssc -- (boolean) indicates usage of spread-spectrum clocking. >>>> + brcm,gen -- (integer) indicates desired generation of link: >>>> + 1 => 2.5 Gbps, 2 => 5.0 Gbps, 3 => 8.0 Gbps. >>> >>> We have a standard property for this IIRC. >>> >> Yes, BrianN pointed that out and it will be fixed. >> >>>> + supply-names -- the names of voltage regulators that the root >>>> + complex should turn off/on/on on suspend/resume/boot. This >>>> + is a string list. >>>> + supplies -- A collection of phandles to a regulator nodes, see >>>> + Documentation/devicetree/bindings/regulator/ for specificWe don't; this line is erroneous. >>>> + bindings. The number and order of phandles must match >>>> + exactly the number of strings in the "supply-names" property. >>> >>> This is not the regulator binding. Use the standard binding. >>> >> The reason we do it this way is because the PCIe controller does not >> know or care what the names of the supplies are, or how many there >> will be. The list of regulators can be different for each board we >> support, as these regulators turn on/off the downstream EP device. >> All the PCIe controller does is turn on/off this list of regulators >> when booting,resuming/suspending. >> >> An alternative would have the node specifying the standard properties >> >> xyz-supply = <&xyz_reg>; >> abc-supply = <&abc_reg>; >> pdq-supply = <&pdq_reg>; >> ... >> >> and then have this driver search all of the properties in the PCIe >> node for names matching /-supply$/, and then create a list of phandles >> from that. Is that what you would like? > > Really, you should have child nodes of the PCIe devices and have the > supplies in there. While that would be technically more correct this poses a number of issues: - there is precedence in that area, and you already Acked binding documents proposing the same thing: * Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt (commit c26ebe98a103479dae9284fe0a86a95af4a5cd46) * Documentation/devicetree/bindings/pci/rockchip-pcie.txt (commit 828bdcfbdb98eeb97facb05fe6c96ba0aed65c4a and before) (which may indicate those should also be corrected, at the possible expense of creating incompatibilities) - there is a chicken and egg problem, you can't detect the EP without turning its regulator on, and if you can't create a pci_device, you certainly won't be able to associate it with an device_node counterpart - PCIe being dynamic by nature, can you have "wildcard" PCIE EP DT node that would be guaranteed to match any physically attached PCIE EP which is discovered by the PCI bus layer (and then back to issue #2) If we can solve #2 and #3, it would be reasonable to move the regulator to a PCIE EP node. Ideally, we may want the generic PCI layer to be responsible for managing regulators within pci_enable_device() and pci_disable_device() for instance? > > The driver could do what you describe, but you've still got to define > the names here. > > Rob >
On Thu, Oct 19, 2017 at 5:49 PM, Rob Herring <robh@kernel.org> wrote: > On Tue, Oct 17, 2017 at 5:42 PM, Jim Quinlan <jim2101024@gmail.com> wrote: >> On Tue, Oct 17, 2017 at 4:24 PM, Rob Herring <robh@kernel.org> wrote: >>> On Wed, Oct 11, 2017 at 06:34:22PM -0400, Jim Quinlan wrote: >>>> The DT bindings description of the Brcmstb PCIe device is described. This >>>> node can be used by almost all Broadcom settop box chips, using >>>> ARM, ARM64, or MIPS CPU architectures. >>>> >>>> Signed-off-by: Jim Quinlan <jim2101024@gmail.com> >>>> --- >>>> .../devicetree/bindings/pci/brcmstb-pci.txt | 106 +++++++++++++++++++++ >>>> 1 file changed, 106 insertions(+) >>>> create mode 100644 Documentation/devicetree/bindings/pci/brcmstb-pci.txt >>>> >>>> diff --git a/Documentation/devicetree/bindings/pci/brcmstb-pci.txt b/Documentation/devicetree/bindings/pci/brcmstb-pci.txt >>>> new file mode 100644 >>>> index 0000000..2f699da >>>> --- /dev/null >>>> +++ b/Documentation/devicetree/bindings/pci/brcmstb-pci.txt >>>> @@ -0,0 +1,106 @@ >>>> +Brcmstb PCIe Host Controller Device Tree Bindings >>>> +We don't; this line is erroneous. >>>> +Introduction: >>>> + The brcmstb host controller closely follows the example set in >>>> + >>>> + [1] http://devicetree.org/Device_Tree_Usage#PCI_Host_Bridge >>>> + >>>> + The rest of this document explains some added customizations and >>>> + offers an example Brcmstb PCIe host controller DT node. >>>> + >>>> +Required Properties: >>>> + reg -- the register start address and length for the PCIe block. >>>> + Additional start,length pairs may be specified for clock addresses. >>> >>> Kind of vague and why do you need clock addresses and the clock binding? >>> >> We don't; this line is erroneous and will be removed. >> >>> Also, typically the config space is defined here? Is that missing or you >>> don't support memory mapped config space? >>> >> We do not support memory mapped config space. >> >>>> + interrupts -- two interrupts are specified; the first interrupt is for >>>> + the PCI host controller and the second is for MSI if the built-in >>>> + MSI controller is to be used. >>>> + interrupt-names -- names of the interrupts (above): "pcie" and "msi". >>>> + compatible -- must be one of: "brcm,bcm7425-pcie", "brcm,bcm7435-pcie", >>>> + or "brcm,bcm7278-pcie". >>> >>> One compatible per line. >>> >> Will fix. >> >>>> + #address-cells -- the number of address cells for PCI-space. >>>> + #size-cells -- the number of size cells for PCI-space. >>>> + ranges -- See [1]; a specification of the outbound windows for the host >>>> + controller. Each outbound window is described by a n-tuple: >>>> + (3 cells) -- PCIe space start address; one cell for attributes >>>> + and two cells for the 64-bit PCIe address. >>>> + (x cells) -- CPU/System start address, number of cells is determined >>>> + by the parent node's #address-cells. >>>> + (y cells) -- Size of region, number of cells determined by the >>>> + parent node's #size-cells. >>>> + Due to hardware limitations, there may be a maximum of four >>>> + non-contiguous ranges specified.We don't; this line is erroneous. >>>> + #interrupt-cells -- number of cells used to describe the interrupt. >>> >>> How many cells? >>> >> This line will be removed. > > Humm, why? You need it to have interrupt-map. You just need to say > what the value is, not what the property is. Okay, I got this from https://elinux.org/Device_Tree_Usage#Advanced_Interrupt_Mapping which says that "First you'll notice that PCI interrupt numbers use only one cell, unlike the system interrupt controller which uses 2 cells; one for the irq number, and one for flags. PCI only needs one cell for interrupts because PCI interrupts are specified to always be level-low sensitive." Is that not the convention for legacy PCI interrupts? > >>>> + interrupt-map-mask -- see [1]; four cells, the first three are zero >>>> + for our uses and the fourth cell is the mask (val = 0x7) for >>>> + the legacy interrupt number [1..4]. >>>> + interrupt-map -- See [1]; there are four interrupts (INTA, INTB, >>>> + INTC, and INTD) to be mapped; each interrupt requires 5 cells >>>> + plus the size of the interrupt specifier. >>>> + linux,pci-domain -- the domain of the host controller. >>>> + >>>> +Optional Properties: >>>> + clocks -- list of clock phandles. If specified, this should list one >>>> + clock. >>>> + clock-names -- the "local" names 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 "sw_pcie". >>>> + dma-ranges -- Similar in structure to ranges, each dma region is >>>> + specified with a n-tuple. Dma-regions describe the inbound >>>> + accesses from EP to RC; it translates the pci address that the >>>> + EP "sees" to the CPU address in memory. This property is needed >>>> + because the design of the Brcmstb memory subsystem often precludes >>>> + idenity-mapping between CPU address space and PCIe address space. >>>> + Each range is described by a n-tuple: >>>> + (3 cells) -- PCIe space start address; one cell for attributes >>>> + and two cells for the 64-bit PCIe address. >>>> + (x cells) -- CPU/System start address, number of cells is determined >>>> + by the parent node's #address-cells. >>>> + (y cells) -- Size of region, number of cells determined by the >>>> + parent node's #size-cells. >>> >>> There's no need to describe standard properties. Just put whatever is >>> specific to your platform. That applies throughout this doc. >>> >> Will fix. >> >>>> + msi-parent -- if MSI is to be used, this must be a phandle to the >>>> + msi-parent. If this prop is set to the phandle of the PCIe >>>> + node, or if the msi-parent prop is missing, the PCIE controller >>>> + will attempt to use its built in MSI controller. >>>> + msi-controller -- this property should only be specified if the >>>> + PCIe controller is using its internal MSI controller. >>>> + brcm,ssc -- (boolean) indicates usage of spread-spectrum clocking. >>>> + brcm,gen -- (integer) indicates desired generation of link: >>>> + 1 => 2.5 Gbps, 2 => 5.0 Gbps, 3 => 8.0 Gbps. >>> >>> We have a standard property for this IIRC. >>> >> Yes, BrianN pointed that out and it will be fixed. >> >>>> + supply-names -- the names of voltage regulators that the root >>>> + complex should turn off/on/on on suspend/resume/boot. This >>>> + is a string list. >>>> + supplies -- A collection of phandles to a regulator nodes, see >>>> + Documentation/devicetree/bindings/regulator/ for specificWe don't; this line is erroneous. >>>> + bindings. The number and order of phandles must match >>>> + exactly the number of strings in the "supply-names" property. >>> >>> This is not the regulator binding. Use the standard binding. >>> >> The reason we do it this way is because the PCIe controller does not >> know or care what the names of the supplies are, or how many there >> will be. The list of regulators can be different for each board we >> support, as these regulators turn on/off the downstream EP device. >> All the PCIe controller does is turn on/off this list of regulators >> when booting,resuming/suspending. >> >> An alternative would have the node specifying the standard properties >> >> xyz-supply = <&xyz_reg>; >> abc-supply = <&abc_reg>; >> pdq-supply = <&pdq_reg>; >> ... >> >> and then have this driver search all of the properties in the PCIe >> node for names matching /-supply$/, and then create a list of phandles >> from that. Is that what you would like? > > Really, you should have child nodes of the PCIe devices and have the > supplies in there. > > The driver could do what you describe, but you've still got to define > the names here. > > Rob
Hi, On Thu, Oct 19, 2017 at 02:58:55PM -0700, Florian Fainelli wrote: > On 10/19/2017 02:49 PM, Rob Herring wrote: > > On Tue, Oct 17, 2017 at 5:42 PM, Jim Quinlan <jim2101024@gmail.com> wrote: > >> On Tue, Oct 17, 2017 at 4:24 PM, Rob Herring <robh@kernel.org> wrote: > >>> This is not the regulator binding. Use the standard binding. > >>> > >> The reason we do it this way is because the PCIe controller does not > >> know or care what the names of the supplies are, or how many there > >> will be. The list of regulators can be different for each board we > >> support, as these regulators turn on/off the downstream EP device. > >> All the PCIe controller does is turn on/off this list of regulators > >> when booting,resuming/suspending. > >> > >> An alternative would have the node specifying the standard properties > >> > >> xyz-supply = <&xyz_reg>; > >> abc-supply = <&abc_reg>; > >> pdq-supply = <&pdq_reg>; > >> ... > >> > >> and then have this driver search all of the properties in the PCIe > >> node for names matching /-supply$/, and then create a list of phandles > >> from that. Is that what you would like? > > > > Really, you should have child nodes of the PCIe devices and have the > > supplies in there. > > While that would be technically more correct this poses a number of issues: > > - there is precedence in that area, and you already Acked binding > documents proposing the same thing: > * Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt (commit > c26ebe98a103479dae9284fe0a86a95af4a5cd46) > * Documentation/devicetree/bindings/pci/rockchip-pcie.txt (commit > 828bdcfbdb98eeb97facb05fe6c96ba0aed65c4a and before) I can actually speak to the Rockchip binding a bit :) It actually has a mixture of true controller regulators (e.g., the 1.8V and 0.9V regulators power analog portions of the SoC's PCIe logic) and controls for the PCIe endpoints (e.g., 12V). Additionally, some of these have been misused to represent a little of both, since the regulator framework is actually quite flexible ;) That may or may not help, but they are at least partially correct. The Freescale one does look like it's plainly *not* about the root controller. Also, those rails *are* all fairly well defined by the various PCIe electromechanical specifications, so isn't it reasonable to expect that a controller can optionally control power for 1 of each of the standard power types? Or do we really want to represent the flexibility that there can be up to N of each type for N endpoints? As a side note: it's also been pretty tough to get the power sequencing requirements represented correctly for some PCIe endpoints. I've tried to make use of this previously, but the series took so long (>1 year and counting!) my team lost interest: [PATCH v16 2/7] power: add power sequence library https://www.spinics.net/lists/linux-usb/msg158452.html It doesn't really solve all of this problem, but it could be worth looking at. > (which may indicate those should also be corrected, at the possible > expense of creating incompatibilities) If a better way is developed, we can always augment the bindings. The current bindings aren't that hard to maintain as "deprecated backups." > - there is a chicken and egg problem, you can't detect the EP without > turning its regulator on, and if you can't create a pci_device, you > certainly won't be able to associate it with an device_node counterpart That's definitely a major problem driving some of the current bindings. We're just not set up to walk the child devices if we can't probe them. > - PCIe being dynamic by nature, can you have "wildcard" PCIE EP DT node > that would be guaranteed to match any physically attached PCIE EP which > is discovered by the PCI bus layer (and then back to issue #2) Technically, you *can* walk the DT (i.e., 'device_node's) without having pci_dev's for each yet. Something like of_pci_find_child_device() would do it. But that still seems kind of wonky, and it's currently neither precise enough nor flexible enough for this, I don't think. Brian > If we can solve #2 and #3, it would be reasonable to move the regulator > to a PCIE EP node. Ideally, we may want the generic PCI layer to be > responsible for managing regulators within pci_enable_device() and > pci_disable_device() for instance? > > > > > The driver could do what you describe, but you've still got to define > > the names here. > > > > Rob > > > > > -- > Florian
On Fri, Oct 20, 2017 at 12:27 PM, Brian Norris <computersforpeace@gmail.com> wrote: > Hi, > > On Thu, Oct 19, 2017 at 02:58:55PM -0700, Florian Fainelli wrote: >> On 10/19/2017 02:49 PM, Rob Herring wrote: >> > On Tue, Oct 17, 2017 at 5:42 PM, Jim Quinlan <jim2101024@gmail.com> wrote: >> >> On Tue, Oct 17, 2017 at 4:24 PM, Rob Herring <robh@kernel.org> wrote: >> >>> This is not the regulator binding. Use the standard binding. >> >>> >> >> The reason we do it this way is because the PCIe controller does not >> >> know or care what the names of the supplies are, or how many there >> >> will be. The list of regulators can be different for each board we >> >> support, as these regulators turn on/off the downstream EP device. >> >> All the PCIe controller does is turn on/off this list of regulators >> >> when booting,resuming/suspending. >> >> >> >> An alternative would have the node specifying the standard properties >> >> >> >> xyz-supply = <&xyz_reg>; >> >> abc-supply = <&abc_reg>; >> >> pdq-supply = <&pdq_reg>; >> >> ... >> >> >> >> and then have this driver search all of the properties in the PCIe >> >> node for names matching /-supply$/, and then create a list of phandles >> >> from that. Is that what you would like? >> > >> > Really, you should have child nodes of the PCIe devices and have the >> > supplies in there. >> >> While that would be technically more correct this poses a number of issues: >> >> - there is precedence in that area, and you already Acked binding >> documents proposing the same thing: >> * Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt (commit >> c26ebe98a103479dae9284fe0a86a95af4a5cd46) >> * Documentation/devicetree/bindings/pci/rockchip-pcie.txt (commit >> 828bdcfbdb98eeb97facb05fe6c96ba0aed65c4a and before) > > I can actually speak to the Rockchip binding a bit :) > > It actually has a mixture of true controller regulators (e.g., the 1.8V > and 0.9V regulators power analog portions of the SoC's PCIe logic) and > controls for the PCIe endpoints (e.g., 12V). Additionally, some of these > have been misused to represent a little of both, since the regulator > framework is actually quite flexible ;) > > That may or may not help, but they are at least partially correct. > > The Freescale one does look like it's plainly *not* about the root > controller. Maybe not. I don't find that to be obvious reading the binding. > Also, those rails *are* all fairly well defined by the various PCIe > electromechanical specifications, so isn't it reasonable to expect that > a controller can optionally control power for 1 of each of the standard > power types? That seems okay. The MMC binding is done that way. I never said you can't just put things in the host node. That's just not an ideal match to the h/w and there's a limit to what makes sense. > Or do we really want to represent the flexibility that > there can be up to N of each type for N endpoints? No, then you need to describe the topology. > As a side note: it's also been pretty tough to get the power sequencing > requirements represented correctly for some PCIe endpoints. I've tried > to make use of this previously, but the series took so long (>1 year and > counting!) my team lost interest: > > [PATCH v16 2/7] power: add power sequence library > https://www.spinics.net/lists/linux-usb/msg158452.html > > It doesn't really solve all of this problem, but it could be worth > looking at. > >> (which may indicate those should also be corrected, at the possible >> expense of creating incompatibilities) > > If a better way is developed, we can always augment the bindings. The > current bindings aren't that hard to maintain as "deprecated backups." > >> - there is a chicken and egg problem, you can't detect the EP without >> turning its regulator on, and if you can't create a pci_device, you >> certainly won't be able to associate it with an device_node counterpart > > That's definitely a major problem driving some of the current bindings. > We're just not set up to walk the child devices if we can't probe them. It's common to all the probeable buses that still need DT additions. >> - PCIe being dynamic by nature, can you have "wildcard" PCIE EP DT node >> that would be guaranteed to match any physically attached PCIE EP which >> is discovered by the PCI bus layer (and then back to issue #2) > > Technically, you *can* walk the DT (i.e., 'device_node's) without having > pci_dev's for each yet. Something like of_pci_find_child_device() would > do it. But that still seems kind of wonky, and it's currently neither > precise enough nor flexible enough for this, I don't think. That's essentially what the generic power seq stuff tries to do. I said early on we need some sort of pre-probe hook for drivers. Trying to handle things generically only gets you so far. There will always be that special case that needs specific handling. Rob
diff --git a/Documentation/devicetree/bindings/pci/brcmstb-pci.txt b/Documentation/devicetree/bindings/pci/brcmstb-pci.txt new file mode 100644 index 0000000..2f699da --- /dev/null +++ b/Documentation/devicetree/bindings/pci/brcmstb-pci.txt @@ -0,0 +1,106 @@ +Brcmstb PCIe Host Controller Device Tree Bindings + +Introduction: + The brcmstb host controller closely follows the example set in + + [1] http://devicetree.org/Device_Tree_Usage#PCI_Host_Bridge + + The rest of this document explains some added customizations and + offers an example Brcmstb PCIe host controller DT node. + +Required Properties: + reg -- the register start address and length for the PCIe block. + Additional start,length pairs may be specified for clock addresses. + interrupts -- two interrupts are specified; the first interrupt is for + the PCI host controller and the second is for MSI if the built-in + MSI controller is to be used. + interrupt-names -- names of the interrupts (above): "pcie" and "msi". + compatible -- must be one of: "brcm,bcm7425-pcie", "brcm,bcm7435-pcie", + or "brcm,bcm7278-pcie". + #address-cells -- the number of address cells for PCI-space. + #size-cells -- the number of size cells for PCI-space. + ranges -- See [1]; a specification of the outbound windows for the host + controller. Each outbound window is described by a n-tuple: + (3 cells) -- PCIe space start address; one cell for attributes + and two cells for the 64-bit PCIe address. + (x cells) -- CPU/System start address, number of cells is determined + by the parent node's #address-cells. + (y cells) -- Size of region, number of cells determined by the + parent node's #size-cells. + Due to hardware limitations, there may be a maximum of four + non-contiguous ranges specified. + #interrupt-cells -- number of cells used to describe the interrupt. + interrupt-map-mask -- see [1]; four cells, the first three are zero + for our uses and the fourth cell is the mask (val = 0x7) for + the legacy interrupt number [1..4]. + interrupt-map -- See [1]; there are four interrupts (INTA, INTB, + INTC, and INTD) to be mapped; each interrupt requires 5 cells + plus the size of the interrupt specifier. + linux,pci-domain -- the domain of the host controller. + +Optional Properties: + clocks -- list of clock phandles. If specified, this should list one + clock. + clock-names -- the "local" names 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 "sw_pcie". + dma-ranges -- Similar in structure to ranges, each dma region is + specified with a n-tuple. Dma-regions describe the inbound + accesses from EP to RC; it translates the pci address that the + EP "sees" to the CPU address in memory. This property is needed + because the design of the Brcmstb memory subsystem often precludes + idenity-mapping between CPU address space and PCIe address space. + Each range is described by a n-tuple: + (3 cells) -- PCIe space start address; one cell for attributes + and two cells for the 64-bit PCIe address. + (x cells) -- CPU/System start address, number of cells is determined + by the parent node's #address-cells. + (y cells) -- Size of region, number of cells determined by the + parent node's #size-cells. + msi-parent -- if MSI is to be used, this must be a phandle to the + msi-parent. If this prop is set to the phandle of the PCIe + node, or if the msi-parent prop is missing, the PCIE controller + will attempt to use its built in MSI controller. + msi-controller -- this property should only be specified if the + PCIe controller is using its internal MSI controller. + brcm,ssc -- (boolean) indicates usage of spread-spectrum clocking. + brcm,gen -- (integer) indicates desired generation of link: + 1 => 2.5 Gbps, 2 => 5.0 Gbps, 3 => 8.0 Gbps. + supply-names -- the names of voltage regulators that the root + complex should turn off/on/on on suspend/resume/boot. This + is a string list. + supplies -- A collection of phandles to a regulator nodes, see + Documentation/devicetree/bindings/regulator/ for specific + bindings. The number and order of phandles must match + exactly the number of strings in the "supply-names" property. + +Example Node: + +pcie0: pcie@f0460000 { + reg = <0x0 0xf0460000 0x0 0x9310>; + interrupts = <0x0 0x0 0x4>; + compatible = "brcm,pci-plat-dev"; + #address-cells = <3>; + #size-cells = <2>; + ranges = <0x02000000 0x00000000 0x00000000 0x00000000 0xc0000000 0x00000000 0x08000000 + 0x02000000 0x00000000 0x08000000 0x00000000 0xc8000000 0x00000000 0x08000000>; + #interrupt-cells = <1>; + interrupt-map-mask = <0 0 0 7>; + interrupt-map = <0 0 0 1 &intc 0 47 3 + 0 0 0 2 &intc 0 48 3 + 0 0 0 3 &intc 0 49 3 + 0 0 0 4 &intc 0 50 3>; + interrupt-names = "pcie_0_inta", + "pcie_0_intb", + "pcie_0_intc", + "pcie_0_intd"; + clocks = <&sw_pcie0>; + clock-names = "sw_pcie"; + msi-parent = <&pcie0>; /* use PCIe's internal MSI controller */ + msi-controller; /* use PCIe's internal MSI controller */ + brcm,ssc; + brcm,gen = <1>; + supply-names = "vreg-wifi-pwr"; + supplies = <&vreg-wifi-pwr>; + linux,pci-domain = <0>; + };
The DT bindings description of the Brcmstb PCIe device is described. This node can be used by almost all Broadcom settop box chips, using ARM, ARM64, or MIPS CPU architectures. Signed-off-by: Jim Quinlan <jim2101024@gmail.com> --- .../devicetree/bindings/pci/brcmstb-pci.txt | 106 +++++++++++++++++++++ 1 file changed, 106 insertions(+) create mode 100644 Documentation/devicetree/bindings/pci/brcmstb-pci.txt