On 25/05/2017 10:48, Marc Zyngier wrote: > On 20/04/17 15:31, Marc Gonzalez wrote: > >> This driver is required to work around several hardware bugs in the >> PCIe controller. >> >> NB: Revision 1 does not support legacy interrupts, or IO space. >> >> Signed-off-by: Marc Gonzalez <marc_gonzalez@xxxxxxxxxxxxxxxx> >> --- >> Documentation/devicetree/bindings/pci/tango-pcie.txt | 32 ++++++++ >> drivers/pci/host/Kconfig | 8 ++ >> drivers/pci/host/Makefile | 1 + >> drivers/pci/host/pcie-tango.c | 161 +++++++++++++++++++++++++++++++++++++++ >> include/linux/pci_ids.h | 2 + >> 5 files changed, 204 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/pci/tango-pcie.txt b/Documentation/devicetree/bindings/pci/tango-pcie.txt >> new file mode 100644 >> index 000000000000..3353b4e77309 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/pci/tango-pcie.txt >> @@ -0,0 +1,32 @@ >> +Sigma Designs Tango PCIe controller >> + >> +Required properties: >> + >> +- compatible: "sigma,smp8759-pcie" >> +- reg: address/size of PCI configuration space, address/size of register area >> +- device_type: "pci" >> +- #size-cells: <2> >> +- #address-cells: <3> >> +- #interrupt-cells: <1> > > What is the point of having an #interrupt-cells when this is *not* an > interrupt controller (as it doesn't support legacy interrupts)? My mistake. Thanks for kindly pointing out that the #interrupt-cells property is not needed when a controller doesn't support legacy interrupts. If a controller does support legacy interrupts, then I see other bindings define #interrupt-cells and interrupt-map. Is interrupt-controller also required? Is that redundant with msi-controller? (Rev2 will support legacy interrupts.) References for my own information: http://elixir.free-electrons.com/linux/latest/source/Documentation/devicetree/bindings/pci/host-generic-pci.txt http://elixir.free-electrons.com/linux/latest/source/Documentation/devicetree/bindings/pci/altera-pcie.txt http://elinux.org/Device_Tree_Usage#Advanced_Interrupt_Mapping > As mentioned earlier, this needs to be a separate patch to be reviewed > by the Keepers of the Faith (aka the DT maintainers). robh already acked v3 two months ago, but can split it up, and CC the DT folks for v5. >> +static int smp8759_init(struct tango_pcie *pcie, void __iomem *base) >> +{ >> + pcie->mux = base + 0x48; >> + pcie->msi_status = base + 0x80; >> + pcie->msi_enable = base + 0xa0; >> + pcie->msi_doorbell = 0xa0000000 + 0x2e07c; >> + >> + return tango_check_pcie_link(base + 0x74); > > Please have some defines for these magic values. Typical driver do #define MUX_OFFSET 0x48 and then access the register's value through readl_relaxed(pcie->base + MUX_OFFSET); I can't do that because the registers were shuffled around between revision 1 and revision 2. Thus, instead of an explicitly-named macro (MUX_OFFSET), I used an explicitly- named field (pcie->mux) and access the register's value through readl_relaxed(pcie->mux); This is equivalent to providing the offset definitions in the init functions, instead of at the top of the file. Regards.
