Re: [PATCH v2 6/8] pci: Add DW PCIe controller support for iMX8MP SoC

On Mon, 26 Feb 2024 at 14:24, Marek Vasut marex@denx.de wrote:

On 2/26/24 9:04 AM, Sumit Garg wrote:

...

pcie_imx doesn't seem to share any useful code for iMX8 SoC and it is tied to quite old port of pcie_designware driver from Linux which suffices only iMX6 specific needs.

It is probably hand-rolled, but it does suck, that's true.

...

But currently we have the common DWC specific bits which alligns pretty well with DW PCIe controller on iMX8MP SoC. So lets reuse those common bits instead as a new driver for iMX8 SoCs. It should be fairly easy to add support for other iMX8 variants to this driver.

iMX8MP SoC also comes up with standalone PCIe PHY support, so hence we can reuse the generic PHY infrastructure to power on PCIe PHY.

[...]

...

+static int pcie_dw_imx_of_to_plat(struct udevice *dev) +{

• struct pcie_dw_imx *priv = dev_get_priv(dev);
  

• ofnode gpr;
  

• int ret;
  

• /* Get the controller base address */
  

• priv->dw.dbi_base = (void *)dev_read_addr_name(dev, "dbi");
  

• if ((fdt_addr_t)priv->dw.dbi_base == FDT_ADDR_T_NONE) {
  

•         dev_err(dev, "failed to get dbi_base address\n");
  

•         return -EINVAL;
  

• }
  

• /* Get the config space base address and size */
  

• priv->dw.cfg_base = (void *)dev_read_addr_size_name(dev, "config",
  

•                                                     &priv->dw.cfg_size);
  

• if ((fdt_addr_t)priv->dw.cfg_base == FDT_ADDR_T_NONE) {
  

•         dev_err(dev, "failed to get cfg_base address\n");
  

•         return -EINVAL;
  

• }
  

• ret = clk_get_bulk(dev, &priv->clks);
  

• if (ret) {
  

•         dev_err(dev, "failed to get PCIe clks\n");
  

•         return ret;
  

• }
  

• ret = reset_get_by_name(dev, "apps", &priv->apps_reset);
  

• if (ret) {
  

•         dev_err(dev,
  

•                 "Failed to get PCIe apps reset control\n");
  

•         return ret;
  

• }
  

• ret = gpio_request_by_name(dev, "reset-gpio", 0, &priv->reset_gpio,
  

•                            (GPIOD_IS_OUT | GPIOD_IS_OUT_ACTIVE));
  

• if (ret) {
  

•         dev_err(dev, "unable to get reset-gpio\n");
  

•         return ret;
  

• }
  

• ret = generic_phy_get_by_name(dev, "pcie-phy", &priv->phy);
  

• if (ret) {
  

•         dev_err(dev, "failed to get pcie phy\n");
  

•         return ret;
  

• }
  

• gpr = ofnode_by_compatible(ofnode_null(), "fsl,imx8mp-iomuxc-gpr");
  

• if (ofnode_equal(gpr, ofnode_null())) {
  

•         dev_err(dev, "unable to find GPR node\n");
  

•         return -ENODEV;
  

• }
  

You likely need a fail path here too, to undo allocation and co.

Ack.

-Sumit