[PATCH 2/9] PCI: host: brcmstb: add DT docs for Brcmstb PCIe device
Jim Quinlan
jim2101024 at gmail.com
Thu Oct 19 16:04:56 PDT 2017
On Thu, Oct 19, 2017 at 5:49 PM, Rob Herring <robh at kernel.org> wrote:
> On Tue, Oct 17, 2017 at 5:42 PM, Jim Quinlan <jim2101024 at gmail.com> wrote:
>> On Tue, Oct 17, 2017 at 4:24 PM, Rob Herring <robh at 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 at 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
More information about the linux-arm-kernel
mailing list