[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 at 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 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);
> >  }
> >
>