[PATCH] pci: imx: use reset-gpios if defined by device-tree

[Soeren Moch]

Hi Tim,

On 01.07.21 19:34, Tim Harvey wrote:
> If reset-gpio is defined by device-tree use that instead of a
> board-specific function to toggle PCI reset.
For me it looks like this: There is a common function that handles the
reset. The only exception being your boards, which override this
function to use a reset gpio dependent on the board type, fine. If you
want to change the reset handling for your boards to a more common
approach, why not extending the common function instead of adding an
alternative code path?
> The presense of 'reset-gpio' in the device-tree will override calling
> the weak imx6_pcie_toggle_reset function therefore I've Cc'd all the
> board maintainers who rely on that function here as I would like to
> remove that function completely.
I would prefer to keep this function for now and extend it:
if CONFIG_PCIE_IMX_PERST_GPIO is defined, just use it. If not, try to
use the gpio from the DT, if this also fails, complain as it is done now
(or maybe fail completely).
Or copy the CONFIG_PCIE_IMX_PERST_GPIO (if available) into the private
struct in pci_init_board()/ imx_pcie_dm_probe() and simplify
imx6_pcie_toggle_reset() to just use that value. Not sure what looks
more elegant in the end.
> The only user of a board-specific weak
> imx6_pcie_toggle_reset is gwventana which I am the maintainer for and
> once this patch is accepted I will be able to remove that use case and
> would then like to remove the use of CONFIG_PCIE_IMX_PERST_GPIO
> completely.
I would see this clean-up in broader context. If we just want to rely on
DT reset-gpio, we should remove all the non-DM code from the driver,
together with the CONFIG_PCIE_IMX_PERST_GPIO option. I did not check, if
any non-DM users of this driver still exist.
>
> I would have preferred implementing this without changing the codepath
> for the users of CONFIG_PCIE_IMX_PERST_GPIO but that would require
> passing in the imx_pcie_priv struct to imx6_pcie_toggle_reset which
> creates a chicken-and-egg scenario as that's currently a weak function
> for boards to override.
I don't see any problem in changing the signature of
imx6_pcie_toggle_reset(). You are the only external user of this
function now, and after the changes no external user will remain.

Just my personal opinion.

Regards,
Soeren
>
> Cc: Ian Ray <ian.ray at ge.com> (maintainer:GE BX50V3 BOARD)
> Cc: Sebastian Reichel <sebastian.reichel at collabora.com> (maintainer:GE BX50V3 BOARD)
> Cc: Fabio Estevam <festevam at gmail.com> (maintainer:MX6SABRESD BOARD)
> Cc: Marek Vasut <marex at denx.de> (maintainer:NOVENA BOARD)
> Cc: Soeren Moch <smoch at web.de> (maintainer:TBS2910 BOARD)
> Cc: Silvio Fricke <open-source at softing.de> (maintainer:VINING_2000 BOARD)
> Signed-off-by: Tim Harvey <tharvey at gateworks.com>
> ---
>  drivers/pci/pcie_imx.c | 24 +++++++++++++++++++++---
>  1 file changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pci/pcie_imx.c b/drivers/pci/pcie_imx.c
> index 73875e00db..c28951655d 100644
> --- a/drivers/pci/pcie_imx.c
> +++ b/drivers/pci/pcie_imx.c
> @@ -100,6 +100,8 @@
>  struct imx_pcie_priv {
>  	void __iomem		*dbi_base;
>  	void __iomem		*cfg_base;
> +	struct gpio_desc	reset_gpio;
> +	bool			gpio_active_high;
>  };
>
>  /*
> @@ -584,7 +586,7 @@ __weak int imx6_pcie_toggle_reset(void)
>  	return 0;
>  }
>
> -static int imx6_pcie_deassert_core_reset(void)
> +static int imx6_pcie_deassert_core_reset(struct imx_pcie_priv *priv)
>  {
>  	struct iomuxc *iomuxc_regs = (struct iomuxc *)IOMUXC_BASE_ADDR;
>
> @@ -612,7 +614,14 @@ static int imx6_pcie_deassert_core_reset(void)
>  	setbits_le32(&iomuxc_regs->gpr[1], IOMUXC_GPR1_REF_SSP_EN);
>  #endif
>
> -	imx6_pcie_toggle_reset();
> +	if (dm_gpio_is_valid(&priv->reset_gpio)) {
> +		/* Assert PERST# for 50ms then de-assert */
> +		dm_gpio_set_value(&priv->reset_gpio, priv->gpio_active_high ? 0 : 1);
> +		mdelay(50);
> +		dm_gpio_set_value(&priv->reset_gpio, priv->gpio_active_high ? 1 : 0);
> +	} else {
> +		imx6_pcie_toggle_reset();
> +	}
>
>  	return 0;
>  }
> @@ -625,7 +634,7 @@ static int imx_pcie_link_up(struct imx_pcie_priv *priv)
>
>  	imx6_pcie_assert_core_reset(priv, false);
>  	imx6_pcie_init_phy();
> -	imx6_pcie_deassert_core_reset();
> +	imx6_pcie_deassert_core_reset(priv);
>
>  	imx_pcie_regions_setup(priv);
>
> @@ -787,6 +796,15 @@ static int imx_pcie_dm_probe(struct udevice *dev)
>  {
>  	struct imx_pcie_priv *priv = dev_get_priv(dev);
>
> +	/* if PERST# valid from dt then assert it */
> +	gpio_request_by_name(dev, "reset-gpio", 0, &priv->reset_gpio,
> +			     GPIOD_IS_OUT);
> +	priv->gpio_active_high = dev_read_bool(dev, "reset-gpio-active-high");
> +	if (dm_gpio_is_valid(&priv->reset_gpio)) {
> +		dm_gpio_set_value(&priv->reset_gpio,
> +				  priv->gpio_active_high ? 0 : 1);
> +	}
> +
>  	return imx_pcie_link_up(priv);
>  }
>