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

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

[Tim Harvey]

If reset-gpio is defined by device-tree use that instead of a
board-specific function to toggle PCI reset.

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. 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 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.

Cc: Ian Ray <ian.ray@ge.com> (maintainer:GE BX50V3 BOARD)
Cc: Sebastian Reichel <sebastian.reichel@collabora.com> (maintainer:GE BX50V3 BOARD)
Cc: Fabio Estevam <festevam@gmail.com> (maintainer:MX6SABRESD BOARD)
Cc: Marek Vasut <marex@denx.de> (maintainer:NOVENA BOARD)
Cc: Soeren Moch <smoch@web.de> (maintainer:TBS2910 BOARD)
Cc: Silvio Fricke <open-source@softing.de> (maintainer:VINING_2000 BOARD)
Signed-off-by: Tim Harvey <tharvey@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);
 }
 
-- 
2.17.1

Re: [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@ge.com> (maintainer:GE BX50V3 BOARD)
> Cc: Sebastian Reichel <sebastian.reichel@collabora.com> (maintainer:GE BX50V3 BOARD)
> Cc: Fabio Estevam <festevam@gmail.com> (maintainer:MX6SABRESD BOARD)
> Cc: Marek Vasut <marex@denx.de> (maintainer:NOVENA BOARD)
> Cc: Soeren Moch <smoch@web.de> (maintainer:TBS2910 BOARD)
> Cc: Silvio Fricke <open-source@softing.de> (maintainer:VINING_2000 BOARD)
> Signed-off-by: Tim Harvey <tharvey@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);
>  }
>

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

[Tim Harvey]

On Sat, Jul 3, 2021 at 4:35 AM Soeren Moch <smoch@web.de> wrote:
>
> 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.
>

Soeren,

I agree I can do this in a cleaner way. I will change
imx6_pcie_toggle_reset() to pass in the gpio_desc and use it
CONFIG_PCIE_IMX_PERST_GPIO is not defined, then I will remove my
board's override of imx6_pcie_toggle_reset.

Best regards,

Tim





> Regards,
> Soeren
> >
> > Cc: Ian Ray <ian.ray@ge.com> (maintainer:GE BX50V3 BOARD)
> > Cc: Sebastian Reichel <sebastian.reichel@collabora.com> (maintainer:GE BX50V3 BOARD)
> > Cc: Fabio Estevam <festevam@gmail.com> (maintainer:MX6SABRESD BOARD)
> > Cc: Marek Vasut <marex@denx.de> (maintainer:NOVENA BOARD)
> > Cc: Soeren Moch <smoch@web.de> (maintainer:TBS2910 BOARD)
> > Cc: Silvio Fricke <open-source@softing.de> (maintainer:VINING_2000 BOARD)
> > Signed-off-by: Tim Harvey <tharvey@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);
> >  }
> >
>