[PATCH 0/3] PCI: apple: PWREN GPIO support & related fixes

[PATCH 0/3] PCI: apple: PWREN GPIO support & related fixes

[Hector Martin]

Hi all,

This short series adds support for the PWREN GPIO to the Apple PCIe
controller driver, and along the way fixes some GPIO handling issues
which cropped up.

Since the PWREN GPIO is provided by SMC which is a fairly high level
driver, PCI can probe before it is ready. Worse, only some devices
need PWREN, which can make the probe fail halfway through with some
ports initialized and not others, which the driver cannot recover
gracefully from. The second patch fixes this situation, so probe
deferral works properly in these cases. The third patch adds PWREN
support per se, and the first one is just related stuff I noticed
while writing this.

Hector Martin (3):
  PCI: apple: GPIO handling nitfixes
  PCI: apple: Probe all GPIOs for availability first
  PCI: apple: Add support for optional PWREN GPIO

 drivers/pci/controller/pcie-apple.c | 67 ++++++++++++++++++++++++++---
 1 file changed, 60 insertions(+), 7 deletions(-)

-- 
2.35.1

[PATCH 1/3] PCI: apple: GPIO handling nitfixes

[Hector Martin]

- Use devm managed GPIO getter
- GPIO ops can sleep in this context

Fixes: 1e33888fbe44 ("PCI: apple: Add initial hardware bring-up")
Cc: stable@vger.kernel.org
Signed-off-by: Hector Martin <marcan@marcan.st>
---
 drivers/pci/controller/pcie-apple.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c
index a2c3c207a04b..e0c06c0ee731 100644
--- a/drivers/pci/controller/pcie-apple.c
+++ b/drivers/pci/controller/pcie-apple.c
@@ -516,8 +516,8 @@ static int apple_pcie_setup_port(struct apple_pcie *pcie,
 	u32 stat, idx;
 	int ret, i;
 
-	reset = gpiod_get_from_of_node(np, "reset-gpios", 0,
-				       GPIOD_OUT_LOW, "PERST#");
+	reset = devm_gpiod_get_from_of_node(pcie->dev, np, "reset-gpios", 0,
+					    GPIOD_OUT_LOW, "PERST#");
 	if (IS_ERR(reset))
 		return PTR_ERR(reset);
 
@@ -541,7 +541,7 @@ static int apple_pcie_setup_port(struct apple_pcie *pcie,
 	rmw_set(PORT_APPCLK_EN, port->base + PORT_APPCLK);
 
 	/* Assert PERST# before setting up the clock */
-	gpiod_set_value(reset, 1);
+	gpiod_set_value_cansleep(reset, 1);
 
 	ret = apple_pcie_setup_refclk(pcie, port);
 	if (ret < 0)
@@ -552,7 +552,7 @@ static int apple_pcie_setup_port(struct apple_pcie *pcie,
 
 	/* Deassert PERST# */
 	rmw_set(PORT_PERST_OFF, port->base + PORT_PERST);
-	gpiod_set_value(reset, 0);
+	gpiod_set_value_cansleep(reset, 0);
 
 	/* Wait for 100ms after PERST# deassertion (PCIe r5.0, 6.6.1) */
 	msleep(100);
-- 
2.35.1

Re: [PATCH 1/3] PCI: apple: GPIO handling nitfixes

[Marc Zyngier]

On Mon, 02 May 2022 10:38:30 +0100,
Hector Martin <marcan@marcan.st> wrote:
> 
> - Use devm managed GPIO getter
> - GPIO ops can sleep in this context
> 
> Fixes: 1e33888fbe44 ("PCI: apple: Add initial hardware bring-up")
> Cc: stable@vger.kernel.org

Why the Cc: stable? I'd guess that at a push, the devm_*() usage help
with potential memory leaks when the driver fails to probe, but it
would be good to call that out in the commit message.

> Signed-off-by: Hector Martin <marcan@marcan.st>
> ---
>  drivers/pci/controller/pcie-apple.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c
> index a2c3c207a04b..e0c06c0ee731 100644
> --- a/drivers/pci/controller/pcie-apple.c
> +++ b/drivers/pci/controller/pcie-apple.c
> @@ -516,8 +516,8 @@ static int apple_pcie_setup_port(struct apple_pcie *pcie,
>  	u32 stat, idx;
>  	int ret, i;
>  
> -	reset = gpiod_get_from_of_node(np, "reset-gpios", 0,
> -				       GPIOD_OUT_LOW, "PERST#");
> +	reset = devm_gpiod_get_from_of_node(pcie->dev, np, "reset-gpios", 0,
> +					    GPIOD_OUT_LOW, "PERST#");
>  	if (IS_ERR(reset))
>  		return PTR_ERR(reset);
>  
> @@ -541,7 +541,7 @@ static int apple_pcie_setup_port(struct apple_pcie *pcie,
>  	rmw_set(PORT_APPCLK_EN, port->base + PORT_APPCLK);
>  
>  	/* Assert PERST# before setting up the clock */
> -	gpiod_set_value(reset, 1);
> +	gpiod_set_value_cansleep(reset, 1);
>  
>  	ret = apple_pcie_setup_refclk(pcie, port);
>  	if (ret < 0)
> @@ -552,7 +552,7 @@ static int apple_pcie_setup_port(struct apple_pcie *pcie,
>  
>  	/* Deassert PERST# */
>  	rmw_set(PORT_PERST_OFF, port->base + PORT_PERST);
> -	gpiod_set_value(reset, 0);
> +	gpiod_set_value_cansleep(reset, 0);
>  
>  	/* Wait for 100ms after PERST# deassertion (PCIe r5.0, 6.6.1) */
>  	msleep(100);

Otherwise:

Acked-by: Marc Zyngier <maz@kernel.org>

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH 1/3] PCI: apple: GPIO handling nitfixes

[Hector Martin]

On 02/05/2022 19.20, Marc Zyngier wrote:
> On Mon, 02 May 2022 10:38:30 +0100,
> Hector Martin <marcan@marcan.st> wrote:
>>
>> - Use devm managed GPIO getter
>> - GPIO ops can sleep in this context
>>
>> Fixes: 1e33888fbe44 ("PCI: apple: Add initial hardware bring-up")
>> Cc: stable@vger.kernel.org
> 
> Why the Cc: stable? I'd guess that at a push, the devm_*() usage help
> with potential memory leaks when the driver fails to probe, but it
> would be good to call that out in the commit message.

Ack, I mentioned what this fixes for v2 and reworded the commit message.

-- 
Hector Martin (marcan@marcan.st)
Public Key: https://mrcn.st/pub

Re: [PATCH 1/3] PCI: apple: GPIO handling nitfixes

[Greg KH]

On Mon, May 02, 2022 at 06:38:30PM +0900, Hector Martin wrote:
> - Use devm managed GPIO getter
> - GPIO ops can sleep in this context

Please read the section entitled "The canonical patch format" in the
kernel file, Documentation/SubmittingPatches for what is needed in order
to properly describe the change.  This text does not make any sense.

Same for your subject, it needs to be reworked.

thanks,

greg k-h

[PATCH 2/3] PCI: apple: Probe all GPIOs for availability first

[Hector Martin]

If we're probing the PCI controller and some GPIOs are not available and
cause a probe defer, we can end up leaving some ports initialized and
not others and making a mess.

Check for PERST# GPIOs for all ports first, and just return
-EPROBE_DEFER if any are not ready yet, without bringing anything up.

Fixes: 1e33888fbe44 ("PCI: apple: Add initial hardware bring-up")
Cc: stable@vger.kernel.org
Signed-off-by: Hector Martin <marcan@marcan.st>
---
 drivers/pci/controller/pcie-apple.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c
index e0c06c0ee731..e3aa2d461739 100644
--- a/drivers/pci/controller/pcie-apple.c
+++ b/drivers/pci/controller/pcie-apple.c
@@ -507,6 +507,20 @@ static u32 apple_pcie_rid2sid_write(struct apple_pcie_port *port,
 	return readl_relaxed(port->base + PORT_RID2SID(idx));
 }
 
+static int apple_pcie_probe_port(struct device_node *np)
+{
+	struct gpio_desc *gd;
+
+	gd = gpiod_get_from_of_node(np, "reset-gpios", 0,
+				    GPIOD_OUT_LOW, "PERST#");
+	if (IS_ERR(gd)) {
+		return PTR_ERR(gd);
+	}
+
+	gpiod_put(gd);
+	return 0;
+}
+
 static int apple_pcie_setup_port(struct apple_pcie *pcie,
 				 struct device_node *np)
 {
@@ -797,8 +811,18 @@ static int apple_pcie_init(struct pci_config_window *cfg)
 
 static int apple_pcie_probe(struct platform_device *pdev)
 {
+	struct device *dev = &pdev->dev;
+	struct device_node *of_port;
 	int ret;
 
+	/* Check for probe dependencies for all ports first */
+	for_each_child_of_node(dev->of_node, of_port) {
+		ret = apple_pcie_probe_port(of_port);
+		of_node_put(of_port);
+		if (ret)
+			return dev_err_probe(dev, ret, "Port %pOF probe fail\n", of_port);
+	}
+
 	ret = bus_register_notifier(&pci_bus_type, &apple_pcie_nb);
 	if (ret)
 		return ret;
-- 
2.35.1

Re: [PATCH 2/3] PCI: apple: Probe all GPIOs for availability first

[Marc Zyngier]

On Mon, 02 May 2022 10:38:31 +0100,
Hector Martin <marcan@marcan.st> wrote:
> 
> If we're probing the PCI controller and some GPIOs are not available and
> cause a probe defer, we can end up leaving some ports initialized and
> not others and making a mess.
> 
> Check for PERST# GPIOs for all ports first, and just return
> -EPROBE_DEFER if any are not ready yet, without bringing anything up.
> 
> Fixes: 1e33888fbe44 ("PCI: apple: Add initial hardware bring-up")
> Cc: stable@vger.kernel.org
> Signed-off-by: Hector Martin <marcan@marcan.st>
> ---
>  drivers/pci/controller/pcie-apple.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c
> index e0c06c0ee731..e3aa2d461739 100644
> --- a/drivers/pci/controller/pcie-apple.c
> +++ b/drivers/pci/controller/pcie-apple.c
> @@ -507,6 +507,20 @@ static u32 apple_pcie_rid2sid_write(struct apple_pcie_port *port,
>  	return readl_relaxed(port->base + PORT_RID2SID(idx));
>  }
>  
> +static int apple_pcie_probe_port(struct device_node *np)
> +{
> +	struct gpio_desc *gd;
> +
> +	gd = gpiod_get_from_of_node(np, "reset-gpios", 0,
> +				    GPIOD_OUT_LOW, "PERST#");
> +	if (IS_ERR(gd)) {
> +		return PTR_ERR(gd);
> +	}
> +
> +	gpiod_put(gd);
> +	return 0;
> +}
> +
>  static int apple_pcie_setup_port(struct apple_pcie *pcie,
>  				 struct device_node *np)
>  {
> @@ -797,8 +811,18 @@ static int apple_pcie_init(struct pci_config_window *cfg)
>  
>  static int apple_pcie_probe(struct platform_device *pdev)
>  {
> +	struct device *dev = &pdev->dev;
> +	struct device_node *of_port;
>  	int ret;
>  
> +	/* Check for probe dependencies for all ports first */
> +	for_each_child_of_node(dev->of_node, of_port) {
> +		ret = apple_pcie_probe_port(of_port);
> +		of_node_put(of_port);
> +		if (ret)
> +			return dev_err_probe(dev, ret, "Port %pOF probe fail\n", of_port);
> +	}
> +
>  	ret = bus_register_notifier(&pci_bus_type, &apple_pcie_nb);
>  	if (ret)
>  		return ret;

Acked-by: Marc Zyngier <maz@kernel.org>

	M.

-- 
Without deviation from the norm, progress is not possible.

[PATCH 3/3] PCI: apple: Add support for optional PWREN GPIO

[Hector Martin]

WiFi and SD card devices on M1 Macs have a separate power enable GPIO.
Add support for this to the PCIe controller. This is modeled after how
pcie-fu740 does it.

Signed-off-by: Hector Martin <marcan@marcan.st>
---
 drivers/pci/controller/pcie-apple.c | 35 ++++++++++++++++++++++++++---
 1 file changed, 32 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c
index e3aa2d461739..5b73c03ebe94 100644
--- a/drivers/pci/controller/pcie-apple.c
+++ b/drivers/pci/controller/pcie-apple.c
@@ -518,6 +518,16 @@ static int apple_pcie_probe_port(struct device_node *np)
 	}
 
 	gpiod_put(gd);
+
+	gd = gpiod_get_from_of_node(np, "pwren-gpios", 0,
+				    GPIOD_OUT_LOW, "PWREN");
+	if (IS_ERR(gd)) {
+		if (PTR_ERR(gd) != -ENOENT)
+			return PTR_ERR(gd);
+	} else {
+		gpiod_put(gd);
+	}
+
 	return 0;
 }
 
@@ -526,7 +536,7 @@ static int apple_pcie_setup_port(struct apple_pcie *pcie,
 {
 	struct platform_device *platform = to_platform_device(pcie->dev);
 	struct apple_pcie_port *port;
-	struct gpio_desc *reset;
+	struct gpio_desc *reset, *pwren = NULL;
 	u32 stat, idx;
 	int ret, i;
 
@@ -535,6 +545,15 @@ static int apple_pcie_setup_port(struct apple_pcie *pcie,
 	if (IS_ERR(reset))
 		return PTR_ERR(reset);
 
+	pwren = devm_gpiod_get_from_of_node(pcie->dev, np, "pwren-gpios", 0,
+					    GPIOD_OUT_LOW, "PWREN");
+	if (IS_ERR(pwren)) {
+		if (PTR_ERR(pwren) == -ENOENT)
+			pwren = NULL;
+		else
+			return PTR_ERR(pwren);
+	}
+
 	port = devm_kzalloc(pcie->dev, sizeof(*port), GFP_KERNEL);
 	if (!port)
 		return -ENOMEM;
@@ -557,12 +576,22 @@ static int apple_pcie_setup_port(struct apple_pcie *pcie,
 	/* Assert PERST# before setting up the clock */
 	gpiod_set_value_cansleep(reset, 1);
 
+	/* Power on the device if required */
+	if (pwren)
+		gpiod_set_value_cansleep(pwren, 1);
+
 	ret = apple_pcie_setup_refclk(pcie, port);
 	if (ret < 0)
 		return ret;
 
-	/* The minimal Tperst-clk value is 100us (PCIe CEM r5.0, 2.9.2) */
-	usleep_range(100, 200);
+	/*
+	 * The minimal Tperst-clk value is 100us (PCIe CEM r5.0, 2.9.2)
+	 * If powering up, the minimal Tpvperl is 100ms
+	 */
+	if (pwren)
+		msleep(100);
+	else
+		usleep_range(100, 200);
 
 	/* Deassert PERST# */
 	rmw_set(PORT_PERST_OFF, port->base + PORT_PERST);
-- 
2.35.1

Re: [PATCH 3/3] PCI: apple: Add support for optional PWREN GPIO

[Marc Zyngier]

On Mon, 02 May 2022 10:38:32 +0100,
Hector Martin <marcan@marcan.st> wrote:
> 
> WiFi and SD card devices on M1 Macs have a separate power enable GPIO.
> Add support for this to the PCIe controller. This is modeled after how
> pcie-fu740 does it.

Please update the DT binding to reflect this as an optional property.

> 
> Signed-off-by: Hector Martin <marcan@marcan.st>
> ---
>  drivers/pci/controller/pcie-apple.c | 35 ++++++++++++++++++++++++++---
>  1 file changed, 32 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-apple.c b/drivers/pci/controller/pcie-apple.c
> index e3aa2d461739..5b73c03ebe94 100644
> --- a/drivers/pci/controller/pcie-apple.c
> +++ b/drivers/pci/controller/pcie-apple.c
> @@ -518,6 +518,16 @@ static int apple_pcie_probe_port(struct device_node *np)
>  	}
>  
>  	gpiod_put(gd);
> +
> +	gd = gpiod_get_from_of_node(np, "pwren-gpios", 0,
> +				    GPIOD_OUT_LOW, "PWREN");
> +	if (IS_ERR(gd)) {
> +		if (PTR_ERR(gd) != -ENOENT)
> +			return PTR_ERR(gd);
> +	} else {
> +		gpiod_put(gd);
> +	}
> +
>  	return 0;
>  }
>  
> @@ -526,7 +536,7 @@ static int apple_pcie_setup_port(struct apple_pcie *pcie,
>  {
>  	struct platform_device *platform = to_platform_device(pcie->dev);
>  	struct apple_pcie_port *port;
> -	struct gpio_desc *reset;
> +	struct gpio_desc *reset, *pwren = NULL;
>  	u32 stat, idx;
>  	int ret, i;
>  
> @@ -535,6 +545,15 @@ static int apple_pcie_setup_port(struct apple_pcie *pcie,
>  	if (IS_ERR(reset))
>  		return PTR_ERR(reset);
>  
> +	pwren = devm_gpiod_get_from_of_node(pcie->dev, np, "pwren-gpios", 0,
> +					    GPIOD_OUT_LOW, "PWREN");
> +	if (IS_ERR(pwren)) {
> +		if (PTR_ERR(pwren) == -ENOENT)
> +			pwren = NULL;
> +		else
> +			return PTR_ERR(pwren);
> +	}
> +
>  	port = devm_kzalloc(pcie->dev, sizeof(*port), GFP_KERNEL);
>  	if (!port)
>  		return -ENOMEM;
> @@ -557,12 +576,22 @@ static int apple_pcie_setup_port(struct apple_pcie *pcie,
>  	/* Assert PERST# before setting up the clock */
>  	gpiod_set_value_cansleep(reset, 1);
>  
> +	/* Power on the device if required */
> +	if (pwren)
> +		gpiod_set_value_cansleep(pwren, 1);

nit: AFAICT, the gpiod_* helpers already check for a NULL descriptor,
and silently return without an error.

> +
>  	ret = apple_pcie_setup_refclk(pcie, port);
>  	if (ret < 0)
>  		return ret;
>  
> -	/* The minimal Tperst-clk value is 100us (PCIe CEM r5.0, 2.9.2) */
> -	usleep_range(100, 200);
> +	/*
> +	 * The minimal Tperst-clk value is 100us (PCIe CEM r5.0, 2.9.2)
> +	 * If powering up, the minimal Tpvperl is 100ms
> +	 */
> +	if (pwren)
> +		msleep(100);
> +	else
> +		usleep_range(100, 200);
>  
>  	/* Deassert PERST# */
>  	rmw_set(PORT_PERST_OFF, port->base + PORT_PERST);

With the documentation aspect addressed:

Acked-by: Marc Zyngier <maz@kernel.org>

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH 3/3] PCI: apple: Add support for optional PWREN GPIO

[Hector Martin]

On 02/05/2022 19.31, Marc Zyngier wrote:
> On Mon, 02 May 2022 10:38:32 +0100,
> Hector Martin <marcan@marcan.st> wrote:
>>
>> WiFi and SD card devices on M1 Macs have a separate power enable GPIO.
>> Add support for this to the PCIe controller. This is modeled after how
>> pcie-fu740 does it.
> 
> Please update the DT binding to reflect this as an optional property.

That's a bit more involved than you might think, considering we aren't
checking *any* properties in the root port child nodes right now :-)

How's this?

patternProperties:
  "^pci@":
    $ref: /schemas/pci/pci-bus.yaml#
    type: object
    description: A single PCI root port

    properties:
      reg:
        maxItems: 1

      pwren-gpios:
        description: Optional GPIO to power on the device
        maxItems: 1

    required:
      - reset-gpios
      - interrupt-controller
      - "#interrupt-cells"
      - interrupt-map-mask
      - interrupt-map

-- 
Hector Martin (marcan@marcan.st)
Public Key: https://mrcn.st/pub

Re: [PATCH 3/3] PCI: apple: Add support for optional PWREN GPIO

[Rob Herring]

On Mon, May 2, 2022 at 4:39 AM Hector Martin <marcan@marcan.st> wrote:
>
> WiFi and SD card devices on M1 Macs have a separate power enable GPIO.
> Add support for this to the PCIe controller. This is modeled after how
> pcie-fu740 does it.

It did, but it's not ideal really. The problem is the GPIO is really
associated with the device (WiFi/SD) rather than the PCI host and
therefore should be part of a WiFi or SD node. You probably don't have
one (yet), but I would suspect that SD will need one for all the
standard MMC/SD DT properties. The secondary issue is we'll end up
adding more power sequencing properties to control ordering and timing
for different devices. The exception here is standard PCI slot
properties like perst#, clkreq, and standard voltage rails can go in
the host bridge (and for new bindings, those should really be in the
root port node). For a complicated example, see Hikey960 or 970.

Of course with power control related properties there's a chicken or
egg issue that the PCI device is not discoverable until the device is
powered on. This issue comes up over and over with various hacky
solutions in the bindings. The PCI subsystem needs to solve this. My
suggestion is that if the firmware says there is a device on the bus
and it wasn't probed, then we should force probing (or add a pre-probe
hook for drivers). That is what MDIO bus does for example.

Rob

Re: [PATCH 3/3] PCI: apple: Add support for optional PWREN GPIO

[Hector Martin]

On 03/05/2022 00.14, Rob Herring wrote:
> On Mon, May 2, 2022 at 4:39 AM Hector Martin <marcan@marcan.st> wrote:
>>
>> WiFi and SD card devices on M1 Macs have a separate power enable GPIO.
>> Add support for this to the PCIe controller. This is modeled after how
>> pcie-fu740 does it.
> 
> It did, but it's not ideal really. The problem is the GPIO is really
> associated with the device (WiFi/SD) rather than the PCI host and
> therefore should be part of a WiFi or SD node. You probably don't have
> one (yet), but I would suspect that SD will need one for all the
> standard MMC/SD DT properties. The secondary issue is we'll end up
> adding more power sequencing properties to control ordering and timing
> for different devices. The exception here is standard PCI slot
> properties like perst#, clkreq, and standard voltage rails can go in
> the host bridge (and for new bindings, those should really be in the
> root port node). For a complicated example, see Hikey960 or 970.
> 
> Of course with power control related properties there's a chicken or
> egg issue that the PCI device is not discoverable until the device is
> powered on. This issue comes up over and over with various hacky
> solutions in the bindings. The PCI subsystem needs to solve this. My
> suggestion is that if the firmware says there is a device on the bus
> and it wasn't probed, then we should force probing (or add a pre-probe
> hook for drivers). That is what MDIO bus does for example.
> 

I agree with the premise. Right now macOS does not actually power down
these devices as far as I know (except maybe sleep mode? not sure what
goes on then yet), but I think the hardware actually has an SD card
detect GPIO hookup that would allow us to entirely power down the SD
controller when no card is inserted. That would obviously be ideal.

FWIW, we do have the device nodes downstream [1]. I did in fact have to
add the SD one for the CD/WP inversion flags (and had to add driver
support for that too).

That said, as for how to make this happen in the PCI subsystem
properly... I think I'll defer to the maintainers' opinion there before
trying to hack something up ;)

Meanwhile, I guess I better get PCIe hotplug working, since doing it in
the driver isn't going to work without that first...

[1]
https://github.com/AsahiLinux/linux/blob/bits/000-devicetree/arch/arm64/boot/dts/apple/t600x-j314-j316.dtsi#L222

-- 
Hector Martin (marcan@marcan.st)
Public Key: https://mrcn.st/pub

Re: [PATCH 3/3] PCI: apple: Add support for optional PWREN GPIO

[Hector Martin]

On 03/05/2022 00.32, Hector Martin wrote:
> On 03/05/2022 00.14, Rob Herring wrote:
>> On Mon, May 2, 2022 at 4:39 AM Hector Martin <marcan@marcan.st> wrote:
>>>
>>> WiFi and SD card devices on M1 Macs have a separate power enable GPIO.
>>> Add support for this to the PCIe controller. This is modeled after how
>>> pcie-fu740 does it.
>>
>> It did, but it's not ideal really. The problem is the GPIO is really
>> associated with the device (WiFi/SD) rather than the PCI host and
>> therefore should be part of a WiFi or SD node. You probably don't have
>> one (yet), but I would suspect that SD will need one for all the
>> standard MMC/SD DT properties. The secondary issue is we'll end up
>> adding more power sequencing properties to control ordering and timing
>> for different devices. The exception here is standard PCI slot
>> properties like perst#, clkreq, and standard voltage rails can go in
>> the host bridge (and for new bindings, those should really be in the
>> root port node). For a complicated example, see Hikey960 or 970.
>>
>> Of course with power control related properties there's a chicken or
>> egg issue that the PCI device is not discoverable until the device is
>> powered on. This issue comes up over and over with various hacky
>> solutions in the bindings. The PCI subsystem needs to solve this. My
>> suggestion is that if the firmware says there is a device on the bus
>> and it wasn't probed, then we should force probing (or add a pre-probe
>> hook for drivers). That is what MDIO bus does for example.
>>
> 
> I agree with the premise. Right now macOS does not actually power down
> these devices as far as I know (except maybe sleep mode? not sure what
> goes on then yet), but I think the hardware actually has an SD card
> detect GPIO hookup that would allow us to entirely power down the SD
> controller when no card is inserted. That would obviously be ideal.
> 
> FWIW, we do have the device nodes downstream [1]. I did in fact have to
> add the SD one for the CD/WP inversion flags (and had to add driver
> support for that too).
> 
> That said, as for how to make this happen in the PCI subsystem
> properly... I think I'll defer to the maintainers' opinion there before
> trying to hack something up ;)
> 
> Meanwhile, I guess I better get PCIe hotplug working, since doing it in
> the driver isn't going to work without that first...
> 
> [1]
> https://github.com/AsahiLinux/linux/blob/bits/000-devicetree/arch/arm64/boot/dts/apple/t600x-j314-j316.dtsi#L222

Thinking about this some more, I think it still makes sense to have the
power enable GPIO in the PCI root port node. A generic power enable GPIO
still makes sense there (think "slot power"). The PCI core could handle
it properly by default, including turning it on prior to initial probing
and shutting it down when the device should go into whatever the PCI
core's idea of D3cold is. AIUI this already happens on some platforms
via firmware, right? Since D3cold is supposed to be a state where the
device receives no power after all.

Obviously this can't handle funky power sequencing requirements, but we
don't have any of those here and we don't know if we ever will (at least
Apple seems to be a fan of throwing little CPLDs on their boards for
fine grained power sequencing, driven by a single IO). If we do, then
that would be the time to have GPIOs in the device node.

In addition, sometimes a single power enable is shared between multiple
functions of one device. This is the case with WiFi/BT, which is a combo
chip with two functions. Coordinating GPIO usage between both drivers
would be problematic if they both try to own it.

The individual device drivers still need to have some kind of API to be
able to put devices into a low-power state. For example, the WiFi driver
could outright power down the device when it is wholly unused and the
interface is down (same for BT, and the PCI core should only put the
slot GPIO into powerdown if both functions say they should be off).
Similarly, the SD driver needs to support an external SD detect GPIO,
and have a mode where it tells the PCI core to shut down the device when
no SD is inserted, and power it back up on insertion. This all allows
the devices to behave a users might expect, with the device nodes
existing and the PCI devices "visible" even when they are powered down
behind the scenes, until they are needed. AIUI this is already how e.g.
hybrid graphics power management works, where power is outright yanked
from the secondary card when it is not needed even though it is still
visible from the userspace point of view (and it is automatically
powered and reinitialized on use).

I'm not super familiar with PCI device power states (making brcmfmac
sleep work properly on these platforms is on my TODO list...) so I'd
love to get some feedback from the PCI folks on what they think about
this whole issue.

-- 
Hector Martin (marcan@marcan.st)
Public Key: https://mrcn.st/pub

Re: [PATCH 3/3] PCI: apple: Add support for optional PWREN GPIO

[Rob Herring]

On Tue, May 03, 2022 at 12:20:48PM +0900, Hector Martin wrote:
> On 03/05/2022 00.32, Hector Martin wrote:
> > On 03/05/2022 00.14, Rob Herring wrote:
> >> On Mon, May 2, 2022 at 4:39 AM Hector Martin <marcan@marcan.st> wrote:
> >>>
> >>> WiFi and SD card devices on M1 Macs have a separate power enable GPIO.
> >>> Add support for this to the PCIe controller. This is modeled after how
> >>> pcie-fu740 does it.
> >>
> >> It did, but it's not ideal really. The problem is the GPIO is really
> >> associated with the device (WiFi/SD) rather than the PCI host and
> >> therefore should be part of a WiFi or SD node. You probably don't have
> >> one (yet), but I would suspect that SD will need one for all the
> >> standard MMC/SD DT properties. The secondary issue is we'll end up
> >> adding more power sequencing properties to control ordering and timing
> >> for different devices. The exception here is standard PCI slot
> >> properties like perst#, clkreq, and standard voltage rails can go in
> >> the host bridge (and for new bindings, those should really be in the
> >> root port node). For a complicated example, see Hikey960 or 970.
> >>
> >> Of course with power control related properties there's a chicken or
> >> egg issue that the PCI device is not discoverable until the device is
> >> powered on. This issue comes up over and over with various hacky
> >> solutions in the bindings. The PCI subsystem needs to solve this. My
> >> suggestion is that if the firmware says there is a device on the bus
> >> and it wasn't probed, then we should force probing (or add a pre-probe
> >> hook for drivers). That is what MDIO bus does for example.
> >>
> > 
> > I agree with the premise. Right now macOS does not actually power down
> > these devices as far as I know (except maybe sleep mode? not sure what
> > goes on then yet), but I think the hardware actually has an SD card
> > detect GPIO hookup that would allow us to entirely power down the SD
> > controller when no card is inserted. That would obviously be ideal.
> > 
> > FWIW, we do have the device nodes downstream [1]. I did in fact have to
> > add the SD one for the CD/WP inversion flags (and had to add driver
> > support for that too).
> > 
> > That said, as for how to make this happen in the PCI subsystem
> > properly... I think I'll defer to the maintainers' opinion there before
> > trying to hack something up ;)
> > 
> > Meanwhile, I guess I better get PCIe hotplug working, since doing it in
> > the driver isn't going to work without that first...
> > 
> > [1]
> > https://github.com/AsahiLinux/linux/blob/bits/000-devicetree/arch/arm64/boot/dts/apple/t600x-j314-j316.dtsi#L222
> 
> Thinking about this some more, I think it still makes sense to have the
> power enable GPIO in the PCI root port node. A generic power enable GPIO
> still makes sense there (think "slot power"). The PCI core could handle
> it properly by default, including turning it on prior to initial probing
> and shutting it down when the device should go into whatever the PCI
> core's idea of D3cold is. AIUI this already happens on some platforms
> via firmware, right? Since D3cold is supposed to be a state where the
> device receives no power after all.

We have put slot stuff in the bridge node. That's because PCI child 
nodes already have a definition and we didn't define slot nodes up 
front. We often have started without a slot/connector and then 
retrofitted nodes in.

I can think of lots of ways to implement 'slot power' with a single GPIO 
being just one way. If it's not defined by PCIe specs or defacto 
standards then I don't think we want to try to do something generic.

The other way to model power gpios is as a GPIO regulator. We probably 
already have PCI examples doing that.

> Obviously this can't handle funky power sequencing requirements, but we
> don't have any of those here and we don't know if we ever will (at least
> Apple seems to be a fan of throwing little CPLDs on their boards for
> fine grained power sequencing, driven by a single IO). If we do, then
> that would be the time to have GPIOs in the device node.
> 
> In addition, sometimes a single power enable is shared between multiple
> functions of one device. This is the case with WiFi/BT, which is a combo
> chip with two functions. Coordinating GPIO usage between both drivers
> would be problematic if they both try to own it.

A GPIO regulator solves this problem as it is reference counted.

> The individual device drivers still need to have some kind of API to be
> able to put devices into a low-power state. For example, the WiFi driver
> could outright power down the device when it is wholly unused and the
> interface is down (same for BT, and the PCI core should only put the
> slot GPIO into powerdown if both functions say they should be off).
> Similarly, the SD driver needs to support an external SD detect GPIO,
> and have a mode where it tells the PCI core to shut down the device when
> no SD is inserted, and power it back up on insertion. This all allows
> the devices to behave a users might expect, with the device nodes
> existing and the PCI devices "visible" even when they are powered down
> behind the scenes, until they are needed. AIUI this is already how e.g.
> hybrid graphics power management works, where power is outright yanked
> from the secondary card when it is not needed even though it is still
> visible from the userspace point of view (and it is automatically
> powered and reinitialized on use).

Can it detect a card insertion when powered down?

I think a GPIO regulator solves all this as long as card detect still 
works.

> I'm not super familiar with PCI device power states (making brcmfmac
> sleep work properly on these platforms is on my TODO list...) so I'd
> love to get some feedback from the PCI folks on what they think about
> this whole issue.

I'm not either. I know there's some backlog of work to rework PCI power 
management to be more inline with how the rest of kernel drivers work.
 
Isn't D3cold an ACPI thing, not PCI?

Rob

Re: [PATCH 3/3] PCI: apple: Add support for optional PWREN GPIO

[Hector Martin]

On 04/05/2022 09.33, Rob Herring wrote:
> On Tue, May 03, 2022 at 12:20:48PM +0900, Hector Martin wrote:
>> Thinking about this some more, I think it still makes sense to have the
>> power enable GPIO in the PCI root port node. A generic power enable GPIO
>> still makes sense there (think "slot power"). The PCI core could handle
>> it properly by default, including turning it on prior to initial probing
>> and shutting it down when the device should go into whatever the PCI
>> core's idea of D3cold is. AIUI this already happens on some platforms
>> via firmware, right? Since D3cold is supposed to be a state where the
>> device receives no power after all.
> 
> We have put slot stuff in the bridge node. That's because PCI child 
> nodes already have a definition and we didn't define slot nodes up 
> front. We often have started without a slot/connector and then 
> retrofitted nodes in.
> 
> I can think of lots of ways to implement 'slot power' with a single GPIO 
> being just one way. If it's not defined by PCIe specs or defacto 
> standards then I don't think we want to try to do something generic.

The first question is whether it should go in the slot/port node for
apcie or the device node, the second question is whether it should be
generic or just specific to apcie :)

But the d3cold concept *is* defined by PCIe specs. See PCIe 4.0
5.3.1.4.2. "D3cold State".

> The other way to model power gpios is as a GPIO regulator. We probably 
> already have PCI examples doing that.

Could certainly do it as a regulator, yes, though that still leaves the
question of what node to put it in. If it goes in the device node we
still have the chicken and egg problem with device probing.

>> Obviously this can't handle funky power sequencing requirements, but we
>> don't have any of those here and we don't know if we ever will (at least
>> Apple seems to be a fan of throwing little CPLDs on their boards for
>> fine grained power sequencing, driven by a single IO). If we do, then
>> that would be the time to have GPIOs in the device node.
>>
>> In addition, sometimes a single power enable is shared between multiple
>> functions of one device. This is the case with WiFi/BT, which is a combo
>> chip with two functions. Coordinating GPIO usage between both drivers
>> would be problematic if they both try to own it.
> 
> A GPIO regulator solves this problem as it is reference counted.

Right, that it does.

>> The individual device drivers still need to have some kind of API to be
>> able to put devices into a low-power state. For example, the WiFi driver
>> could outright power down the device when it is wholly unused and the
>> interface is down (same for BT, and the PCI core should only put the
>> slot GPIO into powerdown if both functions say they should be off).
>> Similarly, the SD driver needs to support an external SD detect GPIO,
>> and have a mode where it tells the PCI core to shut down the device when
>> no SD is inserted, and power it back up on insertion. This all allows
>> the devices to behave a users might expect, with the device nodes
>> existing and the PCI devices "visible" even when they are powered down
>> behind the scenes, until they are needed. AIUI this is already how e.g.
>> hybrid graphics power management works, where power is outright yanked
>> from the secondary card when it is not needed even though it is still
>> visible from the userspace point of view (and it is automatically
>> powered and reinitialized on use).
> 
> Can it detect a card insertion when powered down?

The card detect is also hooked up to a PMU GPIO accessible via SMC which
has interrupt capability (which is not in that driver yet but I will add
that :-)). We can use that in conjunction with or in replacement of the
internal card detect.

> 
>> I'm not super familiar with PCI device power states (making brcmfmac
>> sleep work properly on these platforms is on my TODO list...) so I'd
>> love to get some feedback from the PCI folks on what they think about
>> this whole issue.
> 
> I'm not either. I know there's some backlog of work to rework PCI power 
> management to be more inline with how the rest of kernel drivers work.
>  
> Isn't D3cold an ACPI thing, not PCI?

It's a PCI thing. See Documentation/power/pci.txt for a discussion of
the various PCI power states. This is already part of the API, e.g.
pci_d3cold_enable and friends. Given how d3cold works today on existing
ACPI systems, I think it makes a lot more sense to add that to the PCI
port nodes (whether as a GPIO or a regulator, I don't really care) and
hook it up to that plumbing, rather than try to make the downstream
device driver reinvent that wheel. The PCI core needs to know about
d3cold transitions to save/restore certain config space registers. And
as far as I know this is all hooked up to runtime-pm, so if a driver
enables d3cold and runtime-pm the device can be powered down by the PCI
core when not in use.

So it'd go something like this:

- apcie driver, on slot activation, sees a pwren gpio/reg and powers it
on prior to bringing up the link (and marks that port as d3cold supported)
- sdhci-pci driver, on probe, sees an external card detect GPIO declared
and considers that license to call pci_d3cold_enable and enable
runtime-pm (since it won't need the internal card detect IRQ/GPIO)
- No SD card is inserted, so SD driver goes into runtime suspend and
saves whatever controller state it needs
- PCI core saves whatever config space stuff it needs to save, SD
controller is powered down via GPIO
- SD card is inserted, SMC GPIO IRQ notifies SD driver
- SD driver goes out of runtime suspend
- PCI core powers on controller, re-establishes link, restores config space
- SD driver restores host controller registers and discovers the new card

No new APIs, this is all existing kernel stuff. PCI manages slot power
same as it does on ACPI systems (that support it), driver interacts with
it via runtime-pm and the d3cold control stuff.

I'm tempted to prototype this today and see how it goes...

-- 
Hector Martin (marcan@marcan.st)
Public Key: https://mrcn.st/pub

Re: [PATCH 3/3] PCI: apple: Add support for optional PWREN GPIO

[Marc Zyngier]

On Wed, 04 May 2022 04:36:02 +0100,
Hector Martin <marcan@marcan.st> wrote:

[...]

> 
> So it'd go something like this:
> 
> - apcie driver, on slot activation, sees a pwren gpio/reg and powers it
> on prior to bringing up the link (and marks that port as d3cold supported)
> - sdhci-pci driver, on probe, sees an external card detect GPIO declared
> and considers that license to call pci_d3cold_enable and enable
> runtime-pm (since it won't need the internal card detect IRQ/GPIO)
> - No SD card is inserted, so SD driver goes into runtime suspend and
> saves whatever controller state it needs
> - PCI core saves whatever config space stuff it needs to save, SD
> controller is powered down via GPIO

I haven't quite found yet how this goes, but I can't say I tried in
anger. I guess that the root port device has to provide some standard
PM callbacks that would further tickle the regulator?

> - SD card is inserted, SMC GPIO IRQ notifies SD driver
> - SD driver goes out of runtime suspend
> - PCI core powers on controller, re-establishes link, restores config space
> - SD driver restores host controller registers and discovers the new card
> 
> No new APIs, this is all existing kernel stuff. PCI manages slot power
> same as it does on ACPI systems (that support it), driver interacts with
> it via runtime-pm and the d3cold control stuff.
> 
> I'm tempted to prototype this today and see how it goes...

Well, let us know how that goes.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH 3/3] PCI: apple: Add support for optional PWREN GPIO

[Mark Kettenis]

Hi Rob (& Hector),

On 03/05/2022 00.14, Rob Herring wrote:
> On Mon, May 2, 2022 at 4:39 AM Hector Martin <marcan@marcan.st> wrote:
>>
>> WiFi and SD card devices on M1 Macs have a separate power enable GPIO.
>> Add support for this to the PCIe controller. This is modeled after how
>> pcie-fu740 does it.
> 
> It did, but it's not ideal really. The problem is the GPIO is really
> associated with the device (WiFi/SD) rather than the PCI host and
> therefore should be part of a WiFi or SD node. You probably don't have
> one (yet), but I would suspect that SD will need one for all the
> standard MMC/SD DT properties.

Not really.  The SD card controller is a "standard" GL9755 PCIe SDHC
controller that is already supported by Linux.

It does indeed get a DT node though because the card detect and write
protect signals are inverted and the driver needs to initialize some
PCIe config space registers to compensate for that.

> The secondary issue is we'll end up adding more power sequencing
> properties to control ordering and timing for different devices.

That isn't obvious.  Even though there isn't an actual PCIe slot these
still are PCIe compliant devices and therefore governed by the PCIe
standard power up sequencing.

> The exception here is standard PCI slot properties like perst#,
> clkreq, and standard voltage rails can go in the host bridge (and
> for new bindings, those should really be in the root port node). For
> a complicated example, see Hikey960 or 970.

I don't think there is a fundamental difference between having a GPIO
that controls the standard voltage rails of a PCIe slot (like on the
HiFive Unmatched board) and a GPIO that controls the power to a chip
soldered onto the motherboard (like the ASM2824 soldered onto the
HiFive Unmatched board and WiFi/SD on these Apple M1 systems).  I
don't think it makes sense to describe this in different ways just
because in one case there is a physical connector present.

Note that the proposed patch does add the "pwren-gpio" property on the
root port node as you suggest.

> Of course with power control related properties there's a chicken or
> egg issue that the PCI device is not discoverable until the device is
> powered on. This issue comes up over and over with various hacky
> solutions in the bindings. The PCI subsystem needs to solve this. My
> suggestion is that if the firmware says there is a device on the bus
> and it wasn't probed, then we should force probing (or add a pre-probe
> hook for drivers). That is what MDIO bus does for example.

But in the case of an actual PCIe slot firmware can't really describe
the PCIe device itself in the DT since it might not be there.

And your suggestion would be quite painful in other contexts where the
device tree will be used (U-Boot, *BSD), which all assume that a PCI
bus can be enumerated.