On Thu, Oct 19, 2017 at 5:49 PM, Rob Herring <robh@xxxxxxxxxx> wrote: > On Tue, Oct 17, 2017 at 5:42 PM, Jim Quinlan <jim2101024@xxxxxxxxx> wrote: >> On Tue, Oct 17, 2017 at 4:24 PM, Rob Herring <robh@xxxxxxxxxx> 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@xxxxxxxxx> >>>> --- >>>> .../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
