| Message ID | 1457452094-5409-2-git-send-email-thierry.reding@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
On 03/08/2016 08:48 AM, Thierry Reding wrote: > From: Thierry Reding <treding@nvidia.com> > > The current XUSB pad controller bindings are insufficient to describe > PHY devices attached to USB controllers. New bindings have been created > to overcome these restrictions. As a side-effect each root port now is > assigned a set of PHY devices, one for each lane associated with the > root port. This has the benefit of allowing fine-grained control of the > power management for each lane. > diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c > +static int tegra_pcie_port_phy_power_on(struct tegra_pcie_port *port) > +{ > + struct device *dev = port->pcie->dev; > + unsigned int i; > + int err; > + > + for (i = 0; i < port->lanes; i++) { > + err = phy_power_on(port->phys[i]); This assume the number of array entries is precisely the number of lanes. That seems to contradict the binding update which said the number might not match. Perhaps there's an expectation that phy_power_on() is a no-op for some "invalid" values like NULL or an error-pointer value? But... > +static struct phy *devm_of_phy_optional_get_index(struct device *dev, > + struct device_node *np, > + const char *consumer, > + unsigned int index) > +{ > + struct phy *phy; > + char *name; > + > + name = kasprintf(GFP_KERNEL, "%s-%u", consumer, index); > + if (!name) > + return ERR_PTR(-ENOMEM); > + > + phy = devm_of_phy_get(dev, np, name); > + kfree(name); > + > + if (IS_ERR(phy) && PTR_ERR(phy) == -ENODEV) > + phy = NULL; > + > + return phy; > +} The error-handling there looks wrong. The function generally returns either a valid PHY or an error pointer. However, in the case of -ENODEV, NULL is returned. Subsystems are supposed to encode their handles as, and functions are supposed to return, either NULL or an error pointer for error cases, not both/either. Is the PHY API broken in this regard? If so, then this code is fine, but if not it might need a fix. Aside from that, this patch looks fine. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Thierry, On Tue, Mar 08, 2016 at 04:48:14PM +0100, Thierry Reding wrote: > From: Thierry Reding <treding@nvidia.com> > > The current XUSB pad controller bindings are insufficient to describe > PHY devices attached to USB controllers. New bindings have been created > to overcome these restrictions. As a side-effect each root port now is > assigned a set of PHY devices, one for each lane associated with the > root port. This has the benefit of allowing fine-grained control of the > power management for each lane. > > Signed-off-by: Thierry Reding <treding@nvidia.com> There are two open questions here (mine and Stephen's). I'll drop this and look for a v4. > --- > Changes in v3: > - cache result of check for new PHY bindings usage (Stephen Warren) > > Changes in v2: > - rework commit message to more accurately describe this change > > drivers/pci/host/pci-tegra.c | 151 ++++++++++++++++++++++++++++++++++++++----- > 1 file changed, 135 insertions(+), 16 deletions(-) > > diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c > index 75c55265ca73..3b59e3162dfa 100644 > --- a/drivers/pci/host/pci-tegra.c > +++ b/drivers/pci/host/pci-tegra.c > @@ -290,6 +290,7 @@ struct tegra_pcie { > struct reset_control *afi_rst; > struct reset_control *pcie_xrst; > > + bool legacy_phy; > struct phy *phy; > > struct tegra_msi msi; > @@ -307,11 +308,14 @@ struct tegra_pcie { > > struct tegra_pcie_port { > struct tegra_pcie *pcie; > + struct device_node *np; > struct list_head list; > struct resource regs; > void __iomem *base; > unsigned int index; > unsigned int lanes; > + > + struct phy **phys; > }; > > struct tegra_pcie_bus { > @@ -844,6 +848,24 @@ static int tegra_pcie_phy_enable(struct tegra_pcie *pcie) > return 0; > } > > +static int tegra_pcie_port_phy_power_on(struct tegra_pcie_port *port) > +{ > + struct device *dev = port->pcie->dev; > + unsigned int i; > + int err; > + > + for (i = 0; i < port->lanes; i++) { > + err = phy_power_on(port->phys[i]); > + if (err < 0) { > + dev_err(dev, "failed to power on PHY#%u: %d\n", i, > + err); > + return err; > + } > + } > + > + return 0; > +} > + > static int tegra_pcie_enable_controller(struct tegra_pcie *pcie) > { > const struct tegra_pcie_soc_data *soc = pcie->soc_data; > @@ -883,14 +905,24 @@ static int tegra_pcie_enable_controller(struct tegra_pcie *pcie) > afi_writel(pcie, value, AFI_FUSE); > } > > - if (!pcie->phy) > - err = tegra_pcie_phy_enable(pcie); > - else > - err = phy_power_on(pcie->phy); > + if (!pcie->legacy_phy) { > + list_for_each_entry(port, &pcie->ports, list) { > + err = tegra_pcie_port_phy_power_on(port); > + if (err < 0) { > + dev_err(pcie->dev, > + "failed to power on PCIe port: %d\n", > + err); > + return err; > + } > + } > + } else { > + if (!pcie->phy) > + err = tegra_pcie_phy_enable(pcie); > + else > + err = phy_power_on(pcie->phy); > > - if (err < 0) { > - dev_err(pcie->dev, "failed to power on PHY: %d\n", err); > - return err; > + if (err < 0) > + dev_err(pcie->dev, "failed to power on PHY: %d\n", err); > } > > /* take the PCIe interface module out of reset */ > @@ -1033,6 +1065,99 @@ static int tegra_pcie_resets_get(struct tegra_pcie *pcie) > return 0; > } > > +static int tegra_pcie_phys_get_legacy(struct tegra_pcie *pcie) > +{ > + int err; > + > + pcie->phy = devm_phy_optional_get(pcie->dev, "pcie"); > + if (IS_ERR(pcie->phy)) { > + err = PTR_ERR(pcie->phy); > + dev_err(pcie->dev, "failed to get PHY: %d\n", err); > + return err; > + } > + > + err = phy_init(pcie->phy); > + if (err < 0) { > + dev_err(pcie->dev, "failed to initialize PHY: %d\n", err); > + return err; > + } > + > + pcie->legacy_phy = true; > + > + return 0; > +} > + > +static struct phy *devm_of_phy_optional_get_index(struct device *dev, > + struct device_node *np, > + const char *consumer, > + unsigned int index) > +{ > + struct phy *phy; > + char *name; > + > + name = kasprintf(GFP_KERNEL, "%s-%u", consumer, index); > + if (!name) > + return ERR_PTR(-ENOMEM); > + > + phy = devm_of_phy_get(dev, np, name); > + kfree(name); > + > + if (IS_ERR(phy) && PTR_ERR(phy) == -ENODEV) > + phy = NULL; > + > + return phy; > +} > + > +static int tegra_pcie_port_get_phys(struct tegra_pcie_port *port) > +{ > + struct device *dev = port->pcie->dev; > + struct phy *phy; > + unsigned int i; > + int err; > + > + port->phys = devm_kcalloc(dev, sizeof(phy), port->lanes, GFP_KERNEL); > + if (!port->phys) > + return -ENOMEM; > + > + for (i = 0; i < port->lanes; i++) { > + phy = devm_of_phy_optional_get_index(dev, port->np, "pcie", i); > + if (IS_ERR(phy)) { > + dev_err(dev, "failed to get PHY#%u: %ld\n", i, > + PTR_ERR(phy)); > + return PTR_ERR(phy); > + } > + > + err = phy_init(phy); > + if (err < 0) { > + dev_err(dev, "failed to initialize PHY#%u: %d\n", i, > + err); > + return err; > + } > + > + port->phys[i] = phy; > + } > + > + return 0; > +} > + > +static int tegra_pcie_phys_get(struct tegra_pcie *pcie) > +{ > + struct tegra_pcie_port *port; > + int err; > + > + if (of_get_property(pcie->dev->of_node, "phys", NULL) != NULL) > + return tegra_pcie_phys_get_legacy(pcie); > + > + list_for_each_entry(port, &pcie->ports, list) { > + err = tegra_pcie_port_get_phys(port); > + if (err < 0) { > + return err; > + } > + } > + > + return 0; > +} > + > static int tegra_pcie_get_resources(struct tegra_pcie *pcie) > { > struct platform_device *pdev = to_platform_device(pcie->dev); > @@ -1051,16 +1176,9 @@ static int tegra_pcie_get_resources(struct tegra_pcie *pcie) > return err; > } > > - pcie->phy = devm_phy_optional_get(pcie->dev, "pcie"); > - if (IS_ERR(pcie->phy)) { > - err = PTR_ERR(pcie->phy); > - dev_err(&pdev->dev, "failed to get PHY: %d\n", err); > - return err; > - } > - > - err = phy_init(pcie->phy); > + err = tegra_pcie_phys_get(pcie); > if (err < 0) { > - dev_err(&pdev->dev, "failed to initialize PHY: %d\n", err); > + dev_err(&pdev->dev, "failed to get PHYs: %d\n", err); > return err; > } > > @@ -1725,6 +1843,7 @@ static int tegra_pcie_parse_dt(struct tegra_pcie *pcie) > rp->index = index; > rp->lanes = value; > rp->pcie = pcie; > + rp->np = port; > > rp->base = devm_ioremap_resource(pcie->dev, &rp->regs); > if (IS_ERR(rp->base)) > -- > 2.7.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-pci" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 16, 2016 at 11:01:19AM -0600, Stephen Warren wrote: > On 03/08/2016 08:48 AM, Thierry Reding wrote: > > From: Thierry Reding <treding@nvidia.com> > > > > The current XUSB pad controller bindings are insufficient to describe > > PHY devices attached to USB controllers. New bindings have been created > > to overcome these restrictions. As a side-effect each root port now is > > assigned a set of PHY devices, one for each lane associated with the > > root port. This has the benefit of allowing fine-grained control of the > > power management for each lane. > > > diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c > > > +static int tegra_pcie_port_phy_power_on(struct tegra_pcie_port *port) > > +{ > > + struct device *dev = port->pcie->dev; > > + unsigned int i; > > + int err; > > + > > + for (i = 0; i < port->lanes; i++) { > > + err = phy_power_on(port->phys[i]); > > This assume the number of array entries is precisely the number of lanes. > That seems to contradict the binding update which said the number might not > match. Perhaps there's an expectation that phy_power_on() is a no-op for > some "invalid" values like NULL or an error-pointer value? But... > > > +static struct phy *devm_of_phy_optional_get_index(struct device *dev, > > + struct device_node *np, > > + const char *consumer, > > + unsigned int index) > > +{ > > + struct phy *phy; > > + char *name; > > + > > + name = kasprintf(GFP_KERNEL, "%s-%u", consumer, index); > > + if (!name) > > + return ERR_PTR(-ENOMEM); > > + > > + phy = devm_of_phy_get(dev, np, name); > > + kfree(name); > > + > > + if (IS_ERR(phy) && PTR_ERR(phy) == -ENODEV) > > + phy = NULL; > > + > > + return phy; > > +} > > The error-handling there looks wrong. The function generally returns either > a valid PHY or an error pointer. However, in the case of -ENODEV, NULL is > returned. Subsystems are supposed to encode their handles as, and functions > are supposed to return, either NULL or an error pointer for error cases, not > both/either. Is the PHY API broken in this regard? If so, then this code is > fine, but if not it might need a fix. This function mimics phy_optional_get() which similarily returns NULL for -ENODEV. The remainder of the PHY API treats NULL pointers as "dummy" PHYs and returns early. I think that's a sensible approach to handling optional resources. It might have been more obvious had I implemented this function within phy-core.c, but I didn't think it universally useful because it uses a rather uncommon lookup pattern. I did keep a generic name in case it's ever deemed useful outside of this driver, at which point it could simply be moved into phy-core.c without requiring this driver to change. Thierry
On 04/13/2016 10:01 AM, Thierry Reding wrote: > On Wed, Mar 16, 2016 at 11:01:19AM -0600, Stephen Warren wrote: >> On 03/08/2016 08:48 AM, Thierry Reding wrote: >>> From: Thierry Reding <treding@nvidia.com> >>> >>> The current XUSB pad controller bindings are insufficient to describe >>> PHY devices attached to USB controllers. New bindings have been created >>> to overcome these restrictions. As a side-effect each root port now is >>> assigned a set of PHY devices, one for each lane associated with the >>> root port. This has the benefit of allowing fine-grained control of the >>> power management for each lane. >> >>> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c >> >>> +static int tegra_pcie_port_phy_power_on(struct tegra_pcie_port *port) >>> +{ >>> + struct device *dev = port->pcie->dev; >>> + unsigned int i; >>> + int err; >>> + >>> + for (i = 0; i < port->lanes; i++) { >>> + err = phy_power_on(port->phys[i]); >> >> This assume the number of array entries is precisely the number of lanes. >> That seems to contradict the binding update which said the number might not >> match. Perhaps there's an expectation that phy_power_on() is a no-op for >> some "invalid" values like NULL or an error-pointer value? But... >> >>> +static struct phy *devm_of_phy_optional_get_index(struct device *dev, >>> + struct device_node *np, >>> + const char *consumer, >>> + unsigned int index) >>> +{ >>> + struct phy *phy; >>> + char *name; >>> + >>> + name = kasprintf(GFP_KERNEL, "%s-%u", consumer, index); >>> + if (!name) >>> + return ERR_PTR(-ENOMEM); >>> + >>> + phy = devm_of_phy_get(dev, np, name); >>> + kfree(name); >>> + >>> + if (IS_ERR(phy) && PTR_ERR(phy) == -ENODEV) >>> + phy = NULL; >>> + >>> + return phy; >>> +} >> >> The error-handling there looks wrong. The function generally returns either >> a valid PHY or an error pointer. However, in the case of -ENODEV, NULL is >> returned. Subsystems are supposed to encode their handles as, and functions >> are supposed to return, either NULL or an error pointer for error cases, not >> both/either. Is the PHY API broken in this regard? If so, then this code is >> fine, but if not it might need a fix. > > This function mimics phy_optional_get() which similarily returns NULL > for -ENODEV. The remainder of the PHY API treats NULL pointers as > "dummy" PHYs and returns early. I think that's a sensible approach to > handling optional resources. > > It might have been more obvious had I implemented this function within > phy-core.c, but I didn't think it universally useful because it uses a > rather uncommon lookup pattern. I did keep a generic name in case it's > ever deemed useful outside of this driver, at which point it could > simply be moved into phy-core.c without requiring this driver to > change. Ah OK, so if a caller of this function is expected to only use IS_ERR(), and hence treat NULL as a perfectly valid PHY value, and all the PHY APIs deal with NULL correctly, then this is probably OK. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Apr 13, 2016 at 11:01:58AM -0600, Stephen Warren wrote: > On 04/13/2016 10:01 AM, Thierry Reding wrote: > > On Wed, Mar 16, 2016 at 11:01:19AM -0600, Stephen Warren wrote: > > > On 03/08/2016 08:48 AM, Thierry Reding wrote: > > > > From: Thierry Reding <treding@nvidia.com> > > > > > > > > The current XUSB pad controller bindings are insufficient to describe > > > > PHY devices attached to USB controllers. New bindings have been created > > > > to overcome these restrictions. As a side-effect each root port now is > > > > assigned a set of PHY devices, one for each lane associated with the > > > > root port. This has the benefit of allowing fine-grained control of the > > > > power management for each lane. > > > > > > > diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c > > > > > > > +static int tegra_pcie_port_phy_power_on(struct tegra_pcie_port *port) > > > > +{ > > > > + struct device *dev = port->pcie->dev; > > > > + unsigned int i; > > > > + int err; > > > > + > > > > + for (i = 0; i < port->lanes; i++) { > > > > + err = phy_power_on(port->phys[i]); > > > > > > This assume the number of array entries is precisely the number of lanes. > > > That seems to contradict the binding update which said the number might not > > > match. Perhaps there's an expectation that phy_power_on() is a no-op for > > > some "invalid" values like NULL or an error-pointer value? But... > > > > > > > +static struct phy *devm_of_phy_optional_get_index(struct device *dev, > > > > + struct device_node *np, > > > > + const char *consumer, > > > > + unsigned int index) > > > > +{ > > > > + struct phy *phy; > > > > + char *name; > > > > + > > > > + name = kasprintf(GFP_KERNEL, "%s-%u", consumer, index); > > > > + if (!name) > > > > + return ERR_PTR(-ENOMEM); > > > > + > > > > + phy = devm_of_phy_get(dev, np, name); > > > > + kfree(name); > > > > + > > > > + if (IS_ERR(phy) && PTR_ERR(phy) == -ENODEV) > > > > + phy = NULL; > > > > + > > > > + return phy; > > > > +} > > > > > > The error-handling there looks wrong. The function generally returns either > > > a valid PHY or an error pointer. However, in the case of -ENODEV, NULL is > > > returned. Subsystems are supposed to encode their handles as, and functions > > > are supposed to return, either NULL or an error pointer for error cases, not > > > both/either. Is the PHY API broken in this regard? If so, then this code is > > > fine, but if not it might need a fix. > > > > This function mimics phy_optional_get() which similarily returns NULL > > for -ENODEV. The remainder of the PHY API treats NULL pointers as > > "dummy" PHYs and returns early. I think that's a sensible approach to > > handling optional resources. > > > > It might have been more obvious had I implemented this function within > > phy-core.c, but I didn't think it universally useful because it uses a > > rather uncommon lookup pattern. I did keep a generic name in case it's > > ever deemed useful outside of this driver, at which point it could > > simply be moved into phy-core.c without requiring this driver to > > change. > > Ah OK, so if a caller of this function is expected to only use IS_ERR(), and > hence treat NULL as a perfectly valid PHY value, and all the PHY APIs deal > with NULL correctly, then this is probably OK. I think it's not completely consistently used, but at least the public API that we do use has the necessary guards, so we should be fine. Thierry
diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c index 75c55265ca73..3b59e3162dfa 100644 --- a/drivers/pci/host/pci-tegra.c +++ b/drivers/pci/host/pci-tegra.c @@ -290,6 +290,7 @@ struct tegra_pcie { struct reset_control *afi_rst; struct reset_control *pcie_xrst; + bool legacy_phy; struct phy *phy; struct tegra_msi msi; @@ -307,11 +308,14 @@ struct tegra_pcie { struct tegra_pcie_port { struct tegra_pcie *pcie; + struct device_node *np; struct list_head list; struct resource regs; void __iomem *base; unsigned int index; unsigned int lanes; + + struct phy **phys; }; struct tegra_pcie_bus { @@ -844,6 +848,24 @@ static int tegra_pcie_phy_enable(struct tegra_pcie *pcie) return 0; } +static int tegra_pcie_port_phy_power_on(struct tegra_pcie_port *port) +{ + struct device *dev = port->pcie->dev; + unsigned int i; + int err; + + for (i = 0; i < port->lanes; i++) { + err = phy_power_on(port->phys[i]); + if (err < 0) { + dev_err(dev, "failed to power on PHY#%u: %d\n", i, + err); + return err; + } + } + + return 0; +} + static int tegra_pcie_enable_controller(struct tegra_pcie *pcie) { const struct tegra_pcie_soc_data *soc = pcie->soc_data; @@ -883,14 +905,24 @@ static int tegra_pcie_enable_controller(struct tegra_pcie *pcie) afi_writel(pcie, value, AFI_FUSE); } - if (!pcie->phy) - err = tegra_pcie_phy_enable(pcie); - else - err = phy_power_on(pcie->phy); + if (!pcie->legacy_phy) { + list_for_each_entry(port, &pcie->ports, list) { + err = tegra_pcie_port_phy_power_on(port); + if (err < 0) { + dev_err(pcie->dev, + "failed to power on PCIe port: %d\n", + err); + return err; + } + } + } else { + if (!pcie->phy) + err = tegra_pcie_phy_enable(pcie); + else + err = phy_power_on(pcie->phy); - if (err < 0) { - dev_err(pcie->dev, "failed to power on PHY: %d\n", err); - return err; + if (err < 0) + dev_err(pcie->dev, "failed to power on PHY: %d\n", err); } /* take the PCIe interface module out of reset */ @@ -1033,6 +1065,99 @@ static int tegra_pcie_resets_get(struct tegra_pcie *pcie) return 0; } +static int tegra_pcie_phys_get_legacy(struct tegra_pcie *pcie) +{ + int err; + + pcie->phy = devm_phy_optional_get(pcie->dev, "pcie"); + if (IS_ERR(pcie->phy)) { + err = PTR_ERR(pcie->phy); + dev_err(pcie->dev, "failed to get PHY: %d\n", err); + return err; + } + + err = phy_init(pcie->phy); + if (err < 0) { + dev_err(pcie->dev, "failed to initialize PHY: %d\n", err); + return err; + } + + pcie->legacy_phy = true; + + return 0; +} + +static struct phy *devm_of_phy_optional_get_index(struct device *dev, + struct device_node *np, + const char *consumer, + unsigned int index) +{ + struct phy *phy; + char *name; + + name = kasprintf(GFP_KERNEL, "%s-%u", consumer, index); + if (!name) + return ERR_PTR(-ENOMEM); + + phy = devm_of_phy_get(dev, np, name); + kfree(name); + + if (IS_ERR(phy) && PTR_ERR(phy) == -ENODEV) + phy = NULL; + + return phy; +} + +static int tegra_pcie_port_get_phys(struct tegra_pcie_port *port) +{ + struct device *dev = port->pcie->dev; + struct phy *phy; + unsigned int i; + int err; + + port->phys = devm_kcalloc(dev, sizeof(phy), port->lanes, GFP_KERNEL); + if (!port->phys) + return -ENOMEM; + + for (i = 0; i < port->lanes; i++) { + phy = devm_of_phy_optional_get_index(dev, port->np, "pcie", i); + if (IS_ERR(phy)) { + dev_err(dev, "failed to get PHY#%u: %ld\n", i, + PTR_ERR(phy)); + return PTR_ERR(phy); + } + + err = phy_init(phy); + if (err < 0) { + dev_err(dev, "failed to initialize PHY#%u: %d\n", i, + err); + return err; + } + + port->phys[i] = phy; + } + + return 0; +} + +static int tegra_pcie_phys_get(struct tegra_pcie *pcie) +{ + struct tegra_pcie_port *port; + int err; + + if (of_get_property(pcie->dev->of_node, "phys", NULL) != NULL) + return tegra_pcie_phys_get_legacy(pcie); + + list_for_each_entry(port, &pcie->ports, list) { + err = tegra_pcie_port_get_phys(port); + if (err < 0) { + return err; + } + } + + return 0; +} + static int tegra_pcie_get_resources(struct tegra_pcie *pcie) { struct platform_device *pdev = to_platform_device(pcie->dev); @@ -1051,16 +1176,9 @@ static int tegra_pcie_get_resources(struct tegra_pcie *pcie) return err; } - pcie->phy = devm_phy_optional_get(pcie->dev, "pcie"); - if (IS_ERR(pcie->phy)) { - err = PTR_ERR(pcie->phy); - dev_err(&pdev->dev, "failed to get PHY: %d\n", err); - return err; - } - - err = phy_init(pcie->phy); + err = tegra_pcie_phys_get(pcie); if (err < 0) { - dev_err(&pdev->dev, "failed to initialize PHY: %d\n", err); + dev_err(&pdev->dev, "failed to get PHYs: %d\n", err); return err; } @@ -1725,6 +1843,7 @@ static int tegra_pcie_parse_dt(struct tegra_pcie *pcie) rp->index = index; rp->lanes = value; rp->pcie = pcie; + rp->np = port; rp->base = devm_ioremap_resource(pcie->dev, &rp->regs); if (IS_ERR(rp->base))