| Message ID | 1396025579-14344-3-git-send-email-l.stach@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Friday, March 28, 2014 at 05:52:53 PM, Lucas Stach wrote: > The glue around the core designware IP is significantly > different between the Exynos and i.MX implementation, > which is reflected in the DT bindings. > > This changes the i.MX6 binding to reuse as much as > possible from the common designware binding and > removes old cruft. > > I removed the optional GPIOs with the following reasoning: > - disable-gpio: endpoint specific GPIO, not currently > wired up in any code. Should be handled by the PCI device > driver, not the host controller driver. > - wake-up-gpio: same as above. > - power-on-gpio: No user in any upstream DT. This should > be handled by a regulator which shouldn't be controlled > by the host driver, but rather by the PCI device driver. This power-on-gpio should indeed be handled by the regulator, but the regulator cannot be handled by the PCIe device driver. This power-on-gpio must be operated on per-slot basis if I understand it correctly, so it cannot be controlled by the host controller driver either. The reason why this cannot be controlled by the device driver is that if the device is powered down, it won't be detected on the PCIe bus, thus it cannot enable the regulator which will power up the slot the device is sitting in. [...] btw. am I blind or do I just not see devicetree-discuss on CC ? Best regards, Marek Vasut
Am Sonntag, den 30.03.2014, 19:36 +0200 schrieb Marek Vasut: > On Friday, March 28, 2014 at 05:52:53 PM, Lucas Stach wrote: > > The glue around the core designware IP is significantly > > different between the Exynos and i.MX implementation, > > which is reflected in the DT bindings. > > > > This changes the i.MX6 binding to reuse as much as > > possible from the common designware binding and > > removes old cruft. > > > > I removed the optional GPIOs with the following reasoning: > > - disable-gpio: endpoint specific GPIO, not currently > > wired up in any code. Should be handled by the PCI device > > driver, not the host controller driver. > > - wake-up-gpio: same as above. > > - power-on-gpio: No user in any upstream DT. This should > > be handled by a regulator which shouldn't be controlled > > by the host driver, but rather by the PCI device driver. > > This power-on-gpio should indeed be handled by the regulator, but the regulator > cannot be handled by the PCIe device driver. This power-on-gpio must be operated > on per-slot basis if I understand it correctly, so it cannot be controlled by > the host controller driver either. > > The reason why this cannot be controlled by the device driver is that if the > device is powered down, it won't be detected on the PCIe bus, thus it cannot > enable the regulator which will power up the slot the device is sitting in. > So we are on the same page with regard to a GPIO being the wrong abstraction for this, I think. For the regulator part I would argue that PCI is a bus that is built around the ability to inspect the bus and detect devices on the bus at probe time, so any regulator that's powering a PCI device should be boot-on. Only after the device driver is loaded it should be able to fetch the regulator to power down/up the device as it wishes. In the x86 world this is AFAIK done using ACPI methods. I think the host controller driver has no business in controlling the device power, more so as there possibly could be a lot of devices on the bus even with a single host lane. > > btw. am I blind or do I just not see devicetree-discuss on CC ? > Hm, there is devicetree@vger.kernel.org on CC, which MAINTAINER says is the right list for this stuff. Regards, Lucas
On Monday, March 31, 2014 at 11:28:29 AM, Lucas Stach wrote: > Am Sonntag, den 30.03.2014, 19:36 +0200 schrieb Marek Vasut: > > On Friday, March 28, 2014 at 05:52:53 PM, Lucas Stach wrote: > > > The glue around the core designware IP is significantly > > > different between the Exynos and i.MX implementation, > > > which is reflected in the DT bindings. > > > > > > This changes the i.MX6 binding to reuse as much as > > > possible from the common designware binding and > > > removes old cruft. > > > > > > I removed the optional GPIOs with the following reasoning: > > > - disable-gpio: endpoint specific GPIO, not currently > > > > > > wired up in any code. Should be handled by the PCI device > > > driver, not the host controller driver. > > > > > > - wake-up-gpio: same as above. > > > - power-on-gpio: No user in any upstream DT. This should > > > > > > be handled by a regulator which shouldn't be controlled > > > by the host driver, but rather by the PCI device driver. > > > > This power-on-gpio should indeed be handled by the regulator, but the > > regulator cannot be handled by the PCIe device driver. This > > power-on-gpio must be operated on per-slot basis if I understand it > > correctly, so it cannot be controlled by the host controller driver > > either. > > > > The reason why this cannot be controlled by the device driver is that if > > the device is powered down, it won't be detected on the PCIe bus, thus > > it cannot enable the regulator which will power up the slot the device > > is sitting in. > > So we are on the same page with regard to a GPIO being the wrong > abstraction for this, I think. Yes. > For the regulator part I would argue that PCI is a bus that is built > around the ability to inspect the bus and detect devices on the bus at > probe time, so any regulator that's powering a PCI device should be > boot-on. This thing about regulator being boot-on should really be documented. Moreover, I think it's a waste of power to keep the devices ON on boot even if the PCIe bus was not started (yet). The bus might not be started at all and the regulators would still be ON, which would be quite a waste. > Only after the device driver is loaded it should be able to fetch the > regulator to power down/up the device as it wishes. In the x86 world > this is AFAIK done using ACPI methods. > > I think the host controller driver has no business in controlling the > device power, more so as there possibly could be a lot of devices on the > bus even with a single host lane. The power should be controlled per-slot, but I don't know how to model that. Note that there might be a PCIe device with a switch popped into a single slot, which makes things much more interesting. In such case, you need to power up the slot and neither of the downstream devices should control the power regulator of that slot. > > btw. am I blind or do I just not see devicetree-discuss on CC ? > > Hm, there is devicetree@vger.kernel.org on CC, which MAINTAINER says is > the right list for this stuff. OK, I was blind, sorry. Best regards, Marek Vasut
Am Montag, den 31.03.2014, 11:36 +0200 schrieb Marek Vasut: > On Monday, March 31, 2014 at 11:28:29 AM, Lucas Stach wrote: > > Am Sonntag, den 30.03.2014, 19:36 +0200 schrieb Marek Vasut: > > > On Friday, March 28, 2014 at 05:52:53 PM, Lucas Stach wrote: > > > > The glue around the core designware IP is significantly > > > > different between the Exynos and i.MX implementation, > > > > which is reflected in the DT bindings. > > > > > > > > This changes the i.MX6 binding to reuse as much as > > > > possible from the common designware binding and > > > > removes old cruft. > > > > > > > > I removed the optional GPIOs with the following reasoning: > > > > - disable-gpio: endpoint specific GPIO, not currently > > > > > > > > wired up in any code. Should be handled by the PCI device > > > > driver, not the host controller driver. > > > > > > > > - wake-up-gpio: same as above. > > > > - power-on-gpio: No user in any upstream DT. This should > > > > > > > > be handled by a regulator which shouldn't be controlled > > > > by the host driver, but rather by the PCI device driver. > > > > > > This power-on-gpio should indeed be handled by the regulator, but the > > > regulator cannot be handled by the PCIe device driver. This > > > power-on-gpio must be operated on per-slot basis if I understand it > > > correctly, so it cannot be controlled by the host controller driver > > > either. > > > > > > The reason why this cannot be controlled by the device driver is that if > > > the device is powered down, it won't be detected on the PCIe bus, thus > > > it cannot enable the regulator which will power up the slot the device > > > is sitting in. > > > > So we are on the same page with regard to a GPIO being the wrong > > abstraction for this, I think. > > Yes. > > > For the regulator part I would argue that PCI is a bus that is built > > around the ability to inspect the bus and detect devices on the bus at > > probe time, so any regulator that's powering a PCI device should be > > boot-on. > > This thing about regulator being boot-on should really be documented. > > Moreover, I think it's a waste of power to keep the devices ON on boot even if > the PCIe bus was not started (yet). The bus might not be started at all and the > regulators would still be ON, which would be quite a waste. > It's the exact same behavior that you get on x86: all devices are powered after boot, once you loaded a device driver it may choose to turn the device off. I don't think it makes sense to deviate here for the sake of being embedded and special. > > Only after the device driver is loaded it should be able to fetch the > > regulator to power down/up the device as it wishes. In the x86 world > > this is AFAIK done using ACPI methods. > > > > I think the host controller driver has no business in controlling the > > device power, more so as there possibly could be a lot of devices on the > > bus even with a single host lane. > > The power should be controlled per-slot, but I don't know how to model that. > Note that there might be a PCIe device with a switch popped into a single slot, > which makes things much more interesting. In such case, you need to power up the > slot and neither of the downstream devices should control the power regulator of > that slot. > We could just add the regulator to the PCI hierarchy in the DT and handle it similar to IRQs where we just walk up the DT starting from the device until we find the matching IRQ/regulator. Still for this to work with a regulator shared between several devices, all the device drivers have to be aware of the regulator. Otherwise a single device may choose to power down and erroneously cut power to a sibling device, where the driver hasn't requested the regulator. In all those scenarios the host controller driver still would not have any business dealing with the regulator. Same situation as with the legacy IRQs, that aren't handled by the host driver, but just routed to the right instance through the DT. Regards, Lucas
On Monday, March 31, 2014 at 12:38:01 PM, Lucas Stach wrote: > Am Montag, den 31.03.2014, 11:36 +0200 schrieb Marek Vasut: > > On Monday, March 31, 2014 at 11:28:29 AM, Lucas Stach wrote: > > > Am Sonntag, den 30.03.2014, 19:36 +0200 schrieb Marek Vasut: > > > > On Friday, March 28, 2014 at 05:52:53 PM, Lucas Stach wrote: > > > > > The glue around the core designware IP is significantly > > > > > different between the Exynos and i.MX implementation, > > > > > which is reflected in the DT bindings. > > > > > > > > > > This changes the i.MX6 binding to reuse as much as > > > > > possible from the common designware binding and > > > > > removes old cruft. > > > > > > > > > > I removed the optional GPIOs with the following reasoning: > > > > > - disable-gpio: endpoint specific GPIO, not currently > > > > > > > > > > wired up in any code. Should be handled by the PCI device > > > > > driver, not the host controller driver. > > > > > > > > > > - wake-up-gpio: same as above. > > > > > - power-on-gpio: No user in any upstream DT. This should > > > > > > > > > > be handled by a regulator which shouldn't be controlled > > > > > by the host driver, but rather by the PCI device driver. > > > > > > > > This power-on-gpio should indeed be handled by the regulator, but the > > > > regulator cannot be handled by the PCIe device driver. This > > > > power-on-gpio must be operated on per-slot basis if I understand it > > > > correctly, so it cannot be controlled by the host controller driver > > > > either. > > > > > > > > The reason why this cannot be controlled by the device driver is that > > > > if the device is powered down, it won't be detected on the PCIe bus, > > > > thus it cannot enable the regulator which will power up the slot the > > > > device is sitting in. > > > > > > So we are on the same page with regard to a GPIO being the wrong > > > abstraction for this, I think. > > > > Yes. > > > > > For the regulator part I would argue that PCI is a bus that is built > > > around the ability to inspect the bus and detect devices on the bus at > > > probe time, so any regulator that's powering a PCI device should be > > > boot-on. > > > > This thing about regulator being boot-on should really be documented. > > > > Moreover, I think it's a waste of power to keep the devices ON on boot > > even if the PCIe bus was not started (yet). The bus might not be started > > at all and the regulators would still be ON, which would be quite a > > waste. > > It's the exact same behavior that you get on x86: all devices are > powered after boot, once you loaded a device driver it may choose to > turn the device off. I don't think it makes sense to deviate here for > the sake of being embedded and special. > > > > Only after the device driver is loaded it should be able to fetch the > > > regulator to power down/up the device as it wishes. In the x86 world > > > this is AFAIK done using ACPI methods. > > > > > > I think the host controller driver has no business in controlling the > > > device power, more so as there possibly could be a lot of devices on > > > the bus even with a single host lane. > > > > The power should be controlled per-slot, but I don't know how to model > > that. Note that there might be a PCIe device with a switch popped into a > > single slot, which makes things much more interesting. In such case, you > > need to power up the slot and neither of the downstream devices should > > control the power regulator of that slot. > > We could just add the regulator to the PCI hierarchy in the DT and > handle it similar to IRQs where we just walk up the DT starting from the > device until we find the matching IRQ/regulator. > > Still for this to work with a regulator shared between several devices, > all the device drivers have to be aware of the regulator. Otherwise a > single device may choose to power down and erroneously cut power to a > sibling device, where the driver hasn't requested the regulator. > > In all those scenarios the host controller driver still would not have > any business dealing with the regulator. Same situation as with the > legacy IRQs, that aren't handled by the host driver, but just routed to > the right instance through the DT. OK, it all makes sense. What I find a bit unfortunate is that we loose the Plug- n-Play nature of the PCIe here, but I guess that cannot be helped.
diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt b/Documentation/devicetree/bindings/pci/designware-pcie.txt index d6fae13ff062..228b37684305 100644 --- a/Documentation/devicetree/bindings/pci/designware-pcie.txt +++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt @@ -1,15 +1,7 @@ * Synopsys Designware PCIe interface Required properties: -- compatible: should contain "snps,dw-pcie" to identify the - core, plus an identifier for the specific instance, such - as "samsung,exynos5440-pcie" or "fsl,imx6q-pcie". -- reg: base addresses and lengths of the pcie controller, - the phy controller, additional register for the phy controller. -- interrupts: interrupt values for level interrupt, - pulse interrupt, special interrupt. -- clocks: from common clock binding: handle to pci clock. -- clock-names: from common clock binding: should be "pcie" and "pcie_bus". +- compatible: should contain "snps,dw-pcie" to identify the core. - #address-cells: set to <3> - #size-cells: set to <2> - device_type: set to "pci" @@ -19,65 +11,11 @@ Required properties: to define the mapping of the PCIe interface to interrupt numbers. - num-lanes: number of lanes to use +- clocks: Must contain an entry for each entry in clock-names. + See ../clocks/clock-bindings.txt for details. +- clock-names: Must include the following entries: + - "pcie" + - "pcie_bus" Optional properties: - reset-gpio: gpio pin number of power good signal - -Optional properties for fsl,imx6q-pcie -- power-on-gpio: gpio pin number of power-enable signal -- wake-up-gpio: gpio pin number of incoming wakeup signal -- disable-gpio: gpio pin number of outgoing rfkill/endpoint disable signal - -Example: - -SoC specific DT Entry: - - pcie@290000 { - compatible = "samsung,exynos5440-pcie", "snps,dw-pcie"; - reg = <0x290000 0x1000 - 0x270000 0x1000 - 0x271000 0x40>; - interrupts = <0 20 0>, <0 21 0>, <0 22 0>; - clocks = <&clock 28>, <&clock 27>; - clock-names = "pcie", "pcie_bus"; - #address-cells = <3>; - #size-cells = <2>; - device_type = "pci"; - ranges = <0x00000800 0 0x40000000 0x40000000 0 0x00001000 /* configuration space */ - 0x81000000 0 0 0x40001000 0 0x00010000 /* downstream I/O */ - 0x82000000 0 0x40011000 0x40011000 0 0x1ffef000>; /* non-prefetchable memory */ - #interrupt-cells = <1>; - interrupt-map-mask = <0 0 0 0>; - interrupt-map = <0x0 0 &gic 53>; - num-lanes = <4>; - }; - - pcie@2a0000 { - compatible = "samsung,exynos5440-pcie", "snps,dw-pcie"; - reg = <0x2a0000 0x1000 - 0x272000 0x1000 - 0x271040 0x40>; - interrupts = <0 23 0>, <0 24 0>, <0 25 0>; - clocks = <&clock 29>, <&clock 27>; - clock-names = "pcie", "pcie_bus"; - #address-cells = <3>; - #size-cells = <2>; - device_type = "pci"; - ranges = <0x00000800 0 0x60000000 0x60000000 0 0x00001000 /* configuration space */ - 0x81000000 0 0 0x60001000 0 0x00010000 /* downstream I/O */ - 0x82000000 0 0x60011000 0x60011000 0 0x1ffef000>; /* non-prefetchable memory */ - #interrupt-cells = <1>; - interrupt-map-mask = <0 0 0 0>; - interrupt-map = <0x0 0 &gic 56>; - num-lanes = <4>; - }; - -Board specific DT Entry: - - pcie@290000 { - reset-gpio = <&pin_ctrl 5 0>; - }; - - pcie@2a0000 { - reset-gpio = <&pin_ctrl 22 0>; - }; diff --git a/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt new file mode 100644 index 000000000000..cb61578bd346 --- /dev/null +++ b/Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt @@ -0,0 +1,38 @@ +* Freescale i.MX6 PCIe interface + +This PCIe host controller is based on the Synopsis Designware PCIe IP +and thus inherits all the common properties defined in designware-pcie.txt. + +Required properties: +- compatible: "fsl,imx6q-pcie" +- reg: base addresse and length of the pcie controller +- interrupts: A list of interrupt outputs of the controller. Must contain an + entry for each entry in the interrupt-names property. +- interrupt-names: Must include the following entries: + - "msi": The interrupt that is asserted when an MSI is received +- clock-names: Must include the following additional entries: + - "pcie_phy" + +Example: + + pcie@0x01000000 { + compatible = "fsl,imx6q-pcie", "snps,dw-pcie"; + reg = <0x01ffc000 0x4000>; + #address-cells = <3>; + #size-cells = <2>; + device_type = "pci"; + ranges = <0x00000800 0 0x01f00000 0x01f00000 0 0x00080000 + 0x81000000 0 0 0x01f80000 0 0x00010000 + 0x82000000 0 0x01000000 0x01000000 0 0x00f00000>; + num-lanes = <1>; + interrupts = <GIC_SPI 120 IRQ_TYPE_LEVEL_HIGH>; + interrupt-names = "msi"; + #interrupt-cells = <1>; + interrupt-map-mask = <0 0 0 0x7>; + interrupt-map = <0 0 0 1 &intc GIC_SPI 123 IRQ_TYPE_LEVEL_HIGH>, + <0 0 0 2 &intc GIC_SPI 122 IRQ_TYPE_LEVEL_HIGH>, + <0 0 0 3 &intc GIC_SPI 121 IRQ_TYPE_LEVEL_HIGH>, + <0 0 0 4 &intc GIC_SPI 120 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&clks 144>, <&clks 206>, <&clks 189>; + clock-names = "pcie", "pcie_bus", "pcie_phy"; + }; diff --git a/Documentation/devicetree/bindings/pci/samsung,exynos5440-pcie.txt b/Documentation/devicetree/bindings/pci/samsung,exynos5440-pcie.txt new file mode 100644 index 000000000000..4f9d23d2ed67 --- /dev/null +++ b/Documentation/devicetree/bindings/pci/samsung,exynos5440-pcie.txt @@ -0,0 +1,65 @@ +* Samsung Exynos 5440 PCIe interface + +This PCIe host controller is based on the Synopsis Designware PCIe IP +and thus inherits all the common properties defined in designware-pcie.txt. + +Required properties: +- compatible: "samsung,exynos5440-pcie" +- reg: base addresses and lengths of the pcie controller, + the phy controller, additional register for the phy controller. +- interrupts: A list of interrupt outputs for level interrupt, + pulse interrupt, special interrupt. + +Example: + +SoC specific DT Entry: + + pcie@290000 { + compatible = "samsung,exynos5440-pcie", "snps,dw-pcie"; + reg = <0x290000 0x1000 + 0x270000 0x1000 + 0x271000 0x40>; + interrupts = <0 20 0>, <0 21 0>, <0 22 0>; + clocks = <&clock 28>, <&clock 27>; + clock-names = "pcie", "pcie_bus"; + #address-cells = <3>; + #size-cells = <2>; + device_type = "pci"; + ranges = <0x00000800 0 0x40000000 0x40000000 0 0x00001000 /* configuration space */ + 0x81000000 0 0 0x40001000 0 0x00010000 /* downstream I/O */ + 0x82000000 0 0x40011000 0x40011000 0 0x1ffef000>; /* non-prefetchable memory */ + #interrupt-cells = <1>; + interrupt-map-mask = <0 0 0 0>; + interrupt-map = <0 0 0 0 &gic GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>; + num-lanes = <4>; + }; + + pcie@2a0000 { + compatible = "samsung,exynos5440-pcie", "snps,dw-pcie"; + reg = <0x2a0000 0x1000 + 0x272000 0x1000 + 0x271040 0x40>; + interrupts = <0 23 0>, <0 24 0>, <0 25 0>; + clocks = <&clock 29>, <&clock 27>; + clock-names = "pcie", "pcie_bus"; + #address-cells = <3>; + #size-cells = <2>; + device_type = "pci"; + ranges = <0x00000800 0 0x60000000 0x60000000 0 0x00001000 /* configuration space */ + 0x81000000 0 0 0x60001000 0 0x00010000 /* downstream I/O */ + 0x82000000 0 0x60011000 0x60011000 0 0x1ffef000>; /* non-prefetchable memory */ + #interrupt-cells = <1>; + interrupt-map-mask = <0 0 0 0>; + interrupt-map = <0 0 0 0 &gic GIC_SPI 24 IRQ_TYPE_LEVEL_HIGH>; + num-lanes = <4>; + }; + +Board specific DT Entry: + + pcie@290000 { + reset-gpio = <&pin_ctrl 5 0>; + }; + + pcie@2a0000 { + reset-gpio = <&pin_ctrl 22 0>; + };
The glue around the core designware IP is significantly different between the Exynos and i.MX implementation, which is reflected in the DT bindings. This changes the i.MX6 binding to reuse as much as possible from the common designware binding and removes old cruft. I removed the optional GPIOs with the following reasoning: - disable-gpio: endpoint specific GPIO, not currently wired up in any code. Should be handled by the PCI device driver, not the host controller driver. - wake-up-gpio: same as above. - power-on-gpio: No user in any upstream DT. This should be handled by a regulator which shouldn't be controlled by the host driver, but rather by the PCI device driver. Signed-off-by: Lucas Stach <l.stach@pengutronix.de> --- .../devicetree/bindings/pci/designware-pcie.txt | 74 ++-------------------- .../devicetree/bindings/pci/fsl,imx6q-pcie.txt | 38 +++++++++++ .../bindings/pci/samsung,exynos5440-pcie.txt | 65 +++++++++++++++++++ 3 files changed, 109 insertions(+), 68 deletions(-) create mode 100644 Documentation/devicetree/bindings/pci/fsl,imx6q-pcie.txt create mode 100644 Documentation/devicetree/bindings/pci/samsung,exynos5440-pcie.txt