[PATCH 2/2] arm64: a37xx: pci: Assert PERST# signal when unloading driver

[Stefan Roese]

On 19.08.20 15:57, Pali Rohár wrote:
> This change ensures that PCIe card is put into reset state when U-Boot
> stops using it.
> 
> DM_FLAG_OS_PREPARE ensures that U-Boot executes driver's remove callback
> prior booting Linux kernel.
> 
> Linux kernel pci-aardvark driver needs to reset PCIe card via PERST# signal
> prior initializing it. If it does not issue reset then some PCIe cards
> (specially Compex WiFi cards) are not detected at all.
> 
> Putting PCIe card into reset state prior booting Linux kernel would ensure
> that card would be properly reset at time when Linux kernel starts
> initializing pci-aardvark driver.
> 
> Signed-off-by: Pali Rohár <pali at kernel.org>
> ---
>   drivers/pci/pci-aardvark.c | 27 +++++++++++++++++++++------
>   1 file changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pci/pci-aardvark.c b/drivers/pci/pci-aardvark.c
> index 5b3f23c184..8996be5309 100644
> --- a/drivers/pci/pci-aardvark.c
> +++ b/drivers/pci/pci-aardvark.c
> @@ -148,6 +148,9 @@ struct pcie_advk {
>   	void           *base;
>   	int            first_busno;
>   	struct udevice *dev;
> +#if CONFIG_IS_ENABLED(DM_GPIO)
> +	struct gpio_desc reset_gpio;
> +#endif
>   };

Adding more #ifdef's is not recommended. Can't you "depend" this driver
on DM_GPIO in Kconfig instead? Are there any used that don't support
DM_GPIO right now?

>   static inline void advk_writel(struct pcie_advk *pcie, uint val, uint reg)
> @@ -614,9 +617,7 @@ static int pcie_advk_probe(struct udevice *dev)
>   	struct pcie_advk *pcie = dev_get_priv(dev);
>   
>   #if CONFIG_IS_ENABLED(DM_GPIO)

If you change the driver to rely on DM_GPIO, then please also remove all
other #ifdef's with DM_GPIO.

Thanks,
Stefan

> -	struct gpio_desc reset_gpio;
> -
> -	gpio_request_by_name(dev, "reset-gpios", 0, &reset_gpio,
> +	gpio_request_by_name(dev, "reset-gpios", 0, &pcie->reset_gpio,
>   			     GPIOD_IS_OUT);
>   	/*
>   	 * Issue reset to add-in card through the dedicated GPIO.
> @@ -631,11 +632,11 @@ static int pcie_advk_probe(struct udevice *dev)
>   	 *     possible before PCIe PHY initialization. Moreover, the PCIe
>   	 *     clock should be gated as well.
>   	 */
> -	if (dm_gpio_is_valid(&reset_gpio)) {
> +	if (dm_gpio_is_valid(&pcie->reset_gpio)) {
>   		dev_dbg(pcie->dev, "Toggle PCIE Reset GPIO ...\n");
> -		dm_gpio_set_value(&reset_gpio, 1);
> +		dm_gpio_set_value(&pcie->reset_gpio, 1);
>   		mdelay(200);
> -		dm_gpio_set_value(&reset_gpio, 0);
> +		dm_gpio_set_value(&pcie->reset_gpio, 0);
>   	}
>   #else
>   	dev_dbg(pcie->dev, "PCIE Reset on GPIO support is missing\n");
> @@ -647,6 +648,18 @@ static int pcie_advk_probe(struct udevice *dev)
>   	return pcie_advk_setup_hw(pcie);
>   }
>   
> +static int pcie_advk_remove(struct udevice *dev)
> +{
> +#if CONFIG_IS_ENABLED(DM_GPIO)
> +	struct pcie_advk *pcie = dev_get_priv(dev);
> +
> +	if (dm_gpio_is_valid(&pcie->reset_gpio))
> +		dm_gpio_set_value(&pcie->reset_gpio, 1);
> +#endif /* DM_GPIO */
> +
> +	return 0;
> +}
> +
>   /**
>    * pcie_advk_ofdata_to_platdata() - Translate from DT to device state
>    *
> @@ -687,5 +700,7 @@ U_BOOT_DRIVER(pcie_advk) = {
>   	.ops			= &pcie_advk_ops,
>   	.ofdata_to_platdata	= pcie_advk_ofdata_to_platdata,
>   	.probe			= pcie_advk_probe,
> +	.remove			= pcie_advk_remove,
> +	.flags			= DM_FLAG_OS_PREPARE,
>   	.priv_auto_alloc_size	= sizeof(struct pcie_advk),
>   };
> 


Viele Grüße,
Stefan

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr at denx.de