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