* [PATCH 2/3] PCI: rockchip: make 'return 0' more obvious in probe()
2017-03-08 23:37 [PATCH 1/3] PCI: rockchip: fix sign issues for current limits Brian Norris
@ 2017-03-08 23:37 ` Brian Norris
2017-03-08 23:37 ` [RFC PATCH 3/3] WIP: PCI: rockchip: add remove() support Brian Norris
2017-03-09 8:59 ` [PATCH 1/3] PCI: rockchip: fix sign issues for current limits Shawn Lin
2 siblings, 0 replies; 6+ messages in thread
From: Brian Norris @ 2017-03-08 23:37 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: linux-kernel, Brian Norris, Shawn Lin, Jeffy Chen, Wenrui Li,
linux-pci, linux-rockchip, 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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [RFC PATCH 3/3] WIP: PCI: rockchip: add remove() support
2017-03-08 23:37 [PATCH 1/3] PCI: rockchip: fix sign issues for current limits Brian Norris
2017-03-08 23:37 ` [PATCH 2/3] PCI: rockchip: make 'return 0' more obvious in probe() Brian Norris
@ 2017-03-08 23:37 ` Brian Norris
2017-03-09 3:15 ` Brian Norris
2017-03-09 8:59 ` [PATCH 1/3] PCI: rockchip: fix sign issues for current limits Shawn Lin
2 siblings, 1 reply; 6+ messages in thread
From: Brian Norris @ 2017-03-08 23:37 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: linux-kernel, Brian Norris, Shawn Lin, Jeffy Chen, Wenrui Li,
linux-pci, linux-rockchip, 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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [RFC PATCH 3/3] WIP: PCI: rockchip: add remove() support
2017-03-08 23:37 ` [RFC PATCH 3/3] WIP: PCI: rockchip: add remove() support Brian Norris
@ 2017-03-09 3:15 ` Brian Norris
0 siblings, 0 replies; 6+ messages in thread
From: Brian Norris @ 2017-03-09 3:15 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: linux-kernel, Brian Norris, Shawn Lin, Jeffy Chen, Wenrui Li,
linux-pci, linux-rockchip
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/3] PCI: rockchip: fix sign issues for current limits
2017-03-08 23:37 [PATCH 1/3] PCI: rockchip: fix sign issues for current limits Brian Norris
2017-03-08 23:37 ` [PATCH 2/3] PCI: rockchip: make 'return 0' more obvious in probe() Brian Norris
2017-03-08 23:37 ` [RFC PATCH 3/3] WIP: PCI: rockchip: add remove() support Brian Norris
@ 2017-03-09 8:59 ` Shawn Lin
2017-03-10 2:27 ` Brian Norris
2 siblings, 1 reply; 6+ messages in thread
From: Shawn Lin @ 2017-03-09 8:59 UTC (permalink / raw)
To: Brian Norris, Bjorn Helgaas
Cc: shawn.lin, linux-kernel, Brian Norris, Jeffy Chen, Wenrui Li,
linux-pci, linux-rockchip
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 1/3] PCI: rockchip: fix sign issues for current limits
2017-03-09 8:59 ` [PATCH 1/3] PCI: rockchip: fix sign issues for current limits Shawn Lin
@ 2017-03-10 2:27 ` Brian Norris
0 siblings, 0 replies; 6+ messages in thread
From: Brian Norris @ 2017-03-10 2:27 UTC (permalink / raw)
To: Shawn Lin
Cc: Bjorn Helgaas, linux-kernel, Brian Norris, Jeffy Chen, Wenrui Li,
linux-pci, linux-rockchip
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
^ permalink raw reply [flat|nested] 6+ messages in thread