On Sat, Jul 3, 2021 at 4:35 AM Soeren Moch <[email protected]> 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 <[email protected]> (maintainer:GE BX50V3 BOARD) > > Cc: Sebastian Reichel <[email protected]> (maintainer:GE > > BX50V3 BOARD) > > Cc: Fabio Estevam <[email protected]> (maintainer:MX6SABRESD BOARD) > > Cc: Marek Vasut <[email protected]> (maintainer:NOVENA BOARD) > > Cc: Soeren Moch <[email protected]> (maintainer:TBS2910 BOARD) > > Cc: Silvio Fricke <[email protected]> (maintainer:VINING_2000 BOARD) > > Signed-off-by: Tim Harvey <[email protected]> > > --- > > 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); > > } > > >
