Re: [PATCH v2 1/4] pci: iProc: define Broadcom iProc PCIe binding

[Arend van Spriel <arend@broadcom.com>]

On 12/12/14 18:14, Arnd Bergmann wrote:
> On Friday 12 December 2014 08:53:44 Ray Jui wrote:
>>
>> On 12/12/2014 4:14 AM, Arnd Bergmann wrote:
>>> On Thursday 11 December 2014 18:36:54 Ray Jui wrote:
>>>> index 0000000..040bc0f
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
>>>> @@ -0,0 +1,74 @@
>>>> +* Broadcom iProc PCIe controller
>>>> +
>>>> +Required properties:
>>>> +- compatible: Must be "brcm,iproc-pcie"
>>>> +- reg: base address and length of the PCIe controller and the MDIO interface
>>>> +  that controls the PCIe PHY
>>>> +- #interrupt-cells: set to<1>
>>>> +- interrupts: interrupt IDs
>>>
>>> How many, and what are they?
>>>
>> Different iProc SoCs might have different number of interrupts. I'll
>> elaborate more on the next patch.
>
> Ok.
>
>>>> +- interrupt-map-mask and interrupt-map, standard PCI properties to define the
>>>> +  mapping of the PCIe interface to interrupt numbers
>>>> +- bus-range: PCI bus numbers covered
>>>> +- #address-cells: set to<3>
>>>> +- #size-cells: set to<2>
>>>> +- device_type: set to "pci"
>>>> +- ranges: ranges for the PCI memory and I/O regions
>>>> +- phy-addr: MDC/MDIO adddress of the PCIe PHY
>>>
>>> It looks like the phy controller is separate from the PCI controller,
>>> and you even list the same register range for both PHYs. Better make
>>> that a separate driver and put the phy address into the "phys" reference.
>>>
>> Okay. In this case, I need to create a separate PHY driver under the
>> drivers/phy directory and have the PCIe host driver reference it through
>> the standard PHY API.
>
> Yes, that is what I meant. In particular, that has the advantage of letting
> you reuse the two drivers separately if some new SoC comes up that uses
> one but not the other. A lot of PHY implementations can support multiple
> protocols (e.g. pcie and usb3), but I don't know if yours does.
>
>>>> +- have-msi-inten-reg: Required for legacy iProc PCIe controllers that need the
>>>> +  MSI interrupt enable register to be set explicitly
>>>> +
>>>> +The Broadcom iProc PCie driver adapts the multi-domain structure, i.e., each
>>>> +interface has its own domain and therefore has its own device node
>>>> +Example:
>>>> +
>>>> +SoC specific DT Entry:
>>>> +
>>>> +       pcie0: pcie@18012000 {
>>>> +               compatible = "brcm,iproc-pcie";
>>>> +               reg =<0x18012000 0x1000>,
>>>> +<0x18002000 0x1000>;
>>>
>>> I guess the addresses should be relative to the BCMA bus, and this node
>>> get moved under that. Please see Hauke's patch series, we've discussed
>>> this in great length already.
>>>
>>
>> As Arend van Spriel pointed out in the previous discussion:
>>
>> BCMA core is the bus driver for discoverable ARM AXI interconnect. Apart
>> from that it also provides drivers for some cores. For the chips to be
>> discoverable it needs additional IP logic.
>>
>> Not all iProc family of SoCs have the additional IP logic and for those
>> which don't, they cannot use the BCMA bus.
>
> Ok, but the one from your example almost certainly does because the
> addresses are exactly the same ones as on bcm53xx.
>
> The same problem likely occurs on other peripherals, not just PCI,
> so we will have to come up with a way to have a common driver for
> bcma_bus and platform_bus for USB, SPI, brcmsmac, and likely others
> too.

Makes sense. I think that is what Hauke meant by "adding
additional support for registering to bcma". So the discovery info is a 
piece of read-only memory in the chip. Its address is stored in the 
chipcommon core register space. BCMA parses that memory blob resulting 
in a list of cores which register address info. We could add DT support 
in BCMA matching the compatible string and register a core for it.

However, apart from the discovery info a "discoverable ARM AXI" chip has 
a register space per core that provides common procedures like 
enable/disable, reset, core status, which are implemented in BCMA. I am 
not seeing that register space in the DT examples so I guess this IP 
block is not there for iProc chips.

Regards,
Arend

>>>> +               #interrupt-cells =<1>;
>>>> +               interrupts =<GIC_SPI 96 IRQ_TYPE_NONE>,
>>>> +<GIC_SPI 97 IRQ_TYPE_NONE>,
>>>> +<GIC_SPI 98 IRQ_TYPE_NONE>,
>>>> +<GIC_SPI 99 IRQ_TYPE_NONE>,
>>>> +<GIC_SPI 100 IRQ_TYPE_NONE>,
>>>> +<GIC_SPI 101 IRQ_TYPE_NONE>;
>>>
>>>
>>>
>>>> +               interrupt-map-mask =<0 0 0 0>;
>>>> +               interrupt-map =<0 0 0 0&gic GIC_SPI 100 IRQ_TYPE_NONE>;
>>>
>>> This interrupt is also listed in the "interrupts" above, which is
>>> probably a mistake, unless the IRQ line is shared between all PCI
>>> devices and the PCI host itself.
>>>
>> interrupts are for MSI interrupt support and interrupt-map is for legacy
>> INTx support. To my best knowledge, MSI and INTx cannot be used at the
>> same time. "nvidia,tegra20-pcie.txt" and "rcar-pci.txt" have similar
>> configurations.
>
> Linux drivers will absolutely use MSI and legacy interrupts together, because
> some drivers don't support MSI and others enable it unconditionally.
>
> In both your examples (tegra and rcar), the interrupts that share the same
> number are auxiliary and are correctly used with IRQF_SHARED, so that works.
> If a device MSI just maps to a host IRQ however, you wouldn't be able to
> use IRQF_SHARED.
>
> 	Arnd
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/