[PATCH 1/3] PCI: rockchip: fix sign issues for current limits

[PATCH 1/3] PCI: rockchip: fix sign issues for current limits

[Brian Norris]

The regulator framework can return negative error codes via
regulator_get_current_limit() for regulators that don't provide current
information. The subsequent check for postive values isn't very useful,
if the variable is unsigned.

Let's just match the signedness of the return value.

Prevents error messages like this, seen on Samsung Chromebook Plus:

[    1.069372] rockchip-pcie f8000000.pcie: invalid power supply

Fixes: 4816c4c7b82b ("PCI: rockchip: Provide captured slot power limit and scale")
Signed-off-by: Brian Norris <briannorris@chromium.org>
---
v4.11 candidate?

 drivers/pci/host/pcie-rockchip.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
index 26ddd3535272..d785f64ec03b 100644
--- a/drivers/pci/host/pcie-rockchip.c
+++ b/drivers/pci/host/pcie-rockchip.c
@@ -425,7 +425,8 @@ static struct pci_ops rockchip_pcie_ops = {
 
 static void rockchip_pcie_set_power_limit(struct rockchip_pcie *rockchip)
 {
-	u32 status, curr, scale, power;
+	int curr;
+	u32 status, scale, power;
 
 	if (IS_ERR(rockchip->vpcie3v3))
 		return;
-- 
2.12.0.246.ga2ecc84866-goog

[PATCH 2/3] PCI: rockchip: make 'return 0' more obvious in probe()

[Brian Norris]

There's no way to get here with 'err != 0'. Just return 0 to be more
obvious and prevent future changes from accidentally erroring out here
without going through the right error paths.

Signed-off-by: Brian Norris <briannorris@chromium.org>
---
 drivers/pci/host/pcie-rockchip.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
index d785f64ec03b..5d7b27b1e941 100644
--- a/drivers/pci/host/pcie-rockchip.c
+++ b/drivers/pci/host/pcie-rockchip.c
@@ -1398,7 +1398,7 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
 		pcie_bus_configure_settings(child);
 
 	pci_bus_add_devices(bus);
-	return err;
+	return 0;
 
 err_free_res:
 	pci_free_resource_list(&res);
-- 
2.12.0.246.ga2ecc84866-goog

[RFC PATCH 3/3] WIP: PCI: rockchip: add remove() support

[Brian Norris]

*** THIS IS WIP; DO NOT MERGE ***

I haven't quite figured out the right way to invert pci_remap_iospace().
I guess no one supports this yet? So currently, if you try to
remove/re-probe we'll hit a BUG() in ioremap code when we call this a
second time.

I post the unfinished work here as a bug report, so that if I can't get
this cleaned up quickly, we should instead prevent unbinding the device
once it's probed.

***

Currently, if we try to unbind the platform device, the remove will
succeed, but the removal won't undo most of the registration, leaving
partially-configured PCI devices in the system.

This allows, for example, a simple 'lspci' to crash the system, as it
will try to touch the freed (via devm_*) driver structures.

Signed-off-by: Brian Norris <briannorris@chromium.org>
---
 drivers/pci/host/pcie-rockchip.c | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
index 5d7b27b1e941..e111b3bf63ca 100644
--- a/drivers/pci/host/pcie-rockchip.c
+++ b/drivers/pci/host/pcie-rockchip.c
@@ -225,6 +225,7 @@ struct rockchip_pcie {
 	struct	irq_domain *irq_domain;
 	u32     io_size;
 	int     offset;
+	struct pci_bus *root_bus;
 	phys_addr_t io_bus_addr;
 	void    __iomem *msg_region;
 	u32     mem_size;
@@ -1391,6 +1392,7 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
 		err = -ENOMEM;
 		goto err_free_res;
 	}
+	rockchip->root_bus = bus;
 
 	pci_bus_size_bridges(bus);
 	pci_bus_assign_resources(bus);
@@ -1421,6 +1423,32 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
 	return err;
 }
 
+static int rockchip_pcie_remove(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
+
+	pci_stop_root_bus(rockchip->root_bus);
+	pci_remove_root_bus(rockchip->root_bus);
+
+	phy_power_off(rockchip->phy);
+	phy_exit(rockchip->phy);
+
+	clk_disable_unprepare(rockchip->clk_pcie_pm);
+	clk_disable_unprepare(rockchip->hclk_pcie);
+	clk_disable_unprepare(rockchip->aclk_perf_pcie);
+	clk_disable_unprepare(rockchip->aclk_pcie);
+
+	if (!IS_ERR(rockchip->vpcie3v3))
+		regulator_disable(rockchip->vpcie3v3);
+	if (!IS_ERR(rockchip->vpcie1v8))
+		regulator_disable(rockchip->vpcie1v8);
+	if (!IS_ERR(rockchip->vpcie0v9))
+		regulator_disable(rockchip->vpcie0v9);
+
+	return 0;
+}
+
 static const struct dev_pm_ops rockchip_pcie_pm_ops = {
 	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(rockchip_pcie_suspend_noirq,
 				      rockchip_pcie_resume_noirq)
@@ -1438,6 +1466,6 @@ static struct platform_driver rockchip_pcie_driver = {
 		.pm = &rockchip_pcie_pm_ops,
 	},
 	.probe = rockchip_pcie_probe,
-
+	.remove = rockchip_pcie_remove,
 };
 builtin_platform_driver(rockchip_pcie_driver);
-- 
2.12.0.246.ga2ecc84866-goog

Re: [RFC PATCH 3/3] WIP: PCI: rockchip: add remove() support

[Brian Norris]

On Wed, Mar 08, 2017 at 03:37:48PM -0800, Brian Norris wrote:
> I haven't quite figured out the right way to invert pci_remap_iospace().
> I guess no one supports this yet?

Jeffy Chen pointed out to me that there's a pci_unmap_iospace() as of
4.8. Looks like that should probably do the job. I'll rework this and
send it out sometime. The first 2 patches are still relevant though, and
the first one is a bugfix.

Brian

Re: [PATCH 1/3] PCI: rockchip: fix sign issues for current limits

[Shawn Lin]

On 2017/3/9 7:37, Brian Norris wrote:
> The regulator framework can return negative error codes via
> regulator_get_current_limit() for regulators that don't provide current
> information. The subsequent check for postive values isn't very useful,
> if the variable is unsigned.
>
> Let's just match the signedness of the return value.
>
> Prevents error messages like this, seen on Samsung Chromebook Plus:
>
> [    1.069372] rockchip-pcie f8000000.pcie: invalid power supply
>

For this patch,

Acked-by: Shawn Lin <shawn.lin@rock-chips.com>

And I think patch 2 is not so urgent so we could just wait for your
non-WIP patch 3?

> Fixes: 4816c4c7b82b ("PCI: rockchip: Provide captured slot power limit and scale")
> Signed-off-by: Brian Norris <briannorris@chromium.org>
> ---
> v4.11 candidate?
>
>  drivers/pci/host/pcie-rockchip.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
> index 26ddd3535272..d785f64ec03b 100644
> --- a/drivers/pci/host/pcie-rockchip.c
> +++ b/drivers/pci/host/pcie-rockchip.c
> @@ -425,7 +425,8 @@ static struct pci_ops rockchip_pcie_ops = {
>
>  static void rockchip_pcie_set_power_limit(struct rockchip_pcie *rockchip)
>  {
> -	u32 status, curr, scale, power;
> +	int curr;
> +	u32 status, scale, power;
>
>  	if (IS_ERR(rockchip->vpcie3v3))
>  		return;
>


-- 
Best Regards
Shawn Lin

Re: [PATCH 1/3] PCI: rockchip: fix sign issues for current limits

[Brian Norris]

On Thu, Mar 09, 2017 at 04:59:15PM +0800, Shawn Lin wrote:
> On 2017/3/9 7:37, Brian Norris wrote:
> >The regulator framework can return negative error codes via
> >regulator_get_current_limit() for regulators that don't provide current
> >information. The subsequent check for postive values isn't very useful,
> >if the variable is unsigned.
> >
> >Let's just match the signedness of the return value.
> >
> >Prevents error messages like this, seen on Samsung Chromebook Plus:
> >
> >[    1.069372] rockchip-pcie f8000000.pcie: invalid power supply
> >
> 
> For this patch,
> 
> Acked-by: Shawn Lin <shawn.lin@rock-chips.com>

Thanks.

> And I think patch 2 is not so urgent so we could just wait for your
> non-WIP patch 3?

Sure. I'll be resending the series with a proper patch 3 (and 4 and 5
actually) soon anyway. No changes to the first 2.

Brian