[PATCH 1/2] PCI: rockchip: Make some regulators non-optional
Robin Murphy
robin.murphy at arm.com
Sat Nov 7 07:47:59 EST 2020
On 2020-11-07 11:36, Qu Wenruo wrote:
>
>
> On 2019/11/21 上午1:05, Lorenzo Pieralisi wrote:
>> On Sat, Nov 16, 2019 at 12:54:19PM +0000, Robin Murphy wrote:
>>> The 0V9 and 1V8 supplies power the PCIe block in the SoC itself, and
>>> are thus fundamental to PCIe being usable at all. As such, it makes
>>> sense to treat them as non-optional and rely on dummy regulators if
>>> not explicitly described.
>>>
>>> Signed-off-by: Robin Murphy <robin.murphy at arm.com>
>>> ---
>>> drivers/pci/controller/pcie-rockchip-host.c | 69 ++++++++-------------
>>> 1 file changed, 25 insertions(+), 44 deletions(-)
>>
>> Applied to pci/rockchip, thanks.
>
> Sorry, this commit is cause regression for RK3399 boards unable to
> detect the controller anymore.
>
> The 1v8 (and 0v9) is causing -517 and reject the controller initialization.
That's -EPROBE_DEFER, which must mean that a regulator *is* described,
but you're missing the relevant driver - that's an issue with your
config/initrd. Being optional should only change the behaviour if the
supply is totally absent (i.e. you get -ENODEV instead of a dummy
regulator), so I don't see that it would make any difference in this
situation anyway :/
> I'm not a PCI guy, but a quick google search shows these two voltages
> are not related to PCIE core functionality, especially considering the
> controller used in RK3399 are mostly to provide NVME support.
Unlike the 12V and 3V3 supplies to the slot, these supplies are to the
PCIE_AVDD_0V9 and PCIE_AVDD_1V8 pins on the SoC itself, which the
datasheet describe as "Supply voltage for PCIE". Having power is kind of
important for the I/O circuits on all the signal pins to work.
Now it's almost certainly true that these supplies technically belong to
the phy rather than the controller, but it's a bit late to change the
bindings for the sake of semantics.
> This bug makes all RK3399 users who put root fs into NVME driver unable
> to boot the device.
>
> I really hope some one could test the patch before affecting the end
> users or at least try to understand how most users would use the PCIE
> interface for.
I *am* that end user in this case - I use an M.2 NVME on my board, which
prompted me to take a look at the regulator handling here in the first
place, to see if it might be possible to shut up the annoying message
about a 12V supply that is entirely irrelevant to a board without a
full-size PCIe slot. I use a mainline-based distro, so I've been running
this change for nearly a year since it landed in v5.5, and I'm sure many
others have too. I've not heard of any other complaints in that time...
Robin.
>
> Thanks,
> Qu
>
>>
>> Lorenzo
>>
>>> diff --git a/drivers/pci/controller/pcie-rockchip-host.c b/drivers/pci/controller/pcie-rockchip-host.c
>>> index ef8e677ce9d1..68525f8ac4d9 100644
>>> --- a/drivers/pci/controller/pcie-rockchip-host.c
>>> +++ b/drivers/pci/controller/pcie-rockchip-host.c
>>> @@ -620,19 +620,13 @@ static int rockchip_pcie_parse_host_dt(struct rockchip_pcie *rockchip)
>>> dev_info(dev, "no vpcie3v3 regulator found\n");
>>> }
>>>
>>> - rockchip->vpcie1v8 = devm_regulator_get_optional(dev, "vpcie1v8");
>>> - if (IS_ERR(rockchip->vpcie1v8)) {
>>> - if (PTR_ERR(rockchip->vpcie1v8) != -ENODEV)
>>> - return PTR_ERR(rockchip->vpcie1v8);
>>> - dev_info(dev, "no vpcie1v8 regulator found\n");
>>> - }
>>> + rockchip->vpcie1v8 = devm_regulator_get(dev, "vpcie1v8");
>>> + if (IS_ERR(rockchip->vpcie1v8))
>>> + return PTR_ERR(rockchip->vpcie1v8);
>>>
>>> - rockchip->vpcie0v9 = devm_regulator_get_optional(dev, "vpcie0v9");
>>> - if (IS_ERR(rockchip->vpcie0v9)) {
>>> - if (PTR_ERR(rockchip->vpcie0v9) != -ENODEV)
>>> - return PTR_ERR(rockchip->vpcie0v9);
>>> - dev_info(dev, "no vpcie0v9 regulator found\n");
>>> - }
>>> + rockchip->vpcie0v9 = devm_regulator_get(dev, "vpcie0v9");
>>> + if (IS_ERR(rockchip->vpcie0v9))
>>> + return PTR_ERR(rockchip->vpcie0v9);
>>>
>>> return 0;
>>> }
>>> @@ -658,27 +652,22 @@ static int rockchip_pcie_set_vpcie(struct rockchip_pcie *rockchip)
>>> }
>>> }
>>>
>>> - if (!IS_ERR(rockchip->vpcie1v8)) {
>>> - err = regulator_enable(rockchip->vpcie1v8);
>>> - if (err) {
>>> - dev_err(dev, "fail to enable vpcie1v8 regulator\n");
>>> - goto err_disable_3v3;
>>> - }
>>> + err = regulator_enable(rockchip->vpcie1v8);
>>> + if (err) {
>>> + dev_err(dev, "fail to enable vpcie1v8 regulator\n");
>>> + goto err_disable_3v3;
>>> }
>>>
>>> - if (!IS_ERR(rockchip->vpcie0v9)) {
>>> - err = regulator_enable(rockchip->vpcie0v9);
>>> - if (err) {
>>> - dev_err(dev, "fail to enable vpcie0v9 regulator\n");
>>> - goto err_disable_1v8;
>>> - }
>>> + err = regulator_enable(rockchip->vpcie0v9);
>>> + if (err) {
>>> + dev_err(dev, "fail to enable vpcie0v9 regulator\n");
>>> + goto err_disable_1v8;
>>> }
>>>
>>> return 0;
>>>
>>> err_disable_1v8:
>>> - if (!IS_ERR(rockchip->vpcie1v8))
>>> - regulator_disable(rockchip->vpcie1v8);
>>> + regulator_disable(rockchip->vpcie1v8);
>>> err_disable_3v3:
>>> if (!IS_ERR(rockchip->vpcie3v3))
>>> regulator_disable(rockchip->vpcie3v3);
>>> @@ -897,8 +886,7 @@ static int __maybe_unused rockchip_pcie_suspend_noirq(struct device *dev)
>>>
>>> rockchip_pcie_disable_clocks(rockchip);
>>>
>>> - if (!IS_ERR(rockchip->vpcie0v9))
>>> - regulator_disable(rockchip->vpcie0v9);
>>> + regulator_disable(rockchip->vpcie0v9);
>>>
>>> return ret;
>>> }
>>> @@ -908,12 +896,10 @@ static int __maybe_unused rockchip_pcie_resume_noirq(struct device *dev)
>>> struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
>>> int err;
>>>
>>> - if (!IS_ERR(rockchip->vpcie0v9)) {
>>> - err = regulator_enable(rockchip->vpcie0v9);
>>> - if (err) {
>>> - dev_err(dev, "fail to enable vpcie0v9 regulator\n");
>>> - return err;
>>> - }
>>> + err = regulator_enable(rockchip->vpcie0v9);
>>> + if (err) {
>>> + dev_err(dev, "fail to enable vpcie0v9 regulator\n");
>>> + return err;
>>> }
>>>
>>> err = rockchip_pcie_enable_clocks(rockchip);
>>> @@ -939,8 +925,7 @@ static int __maybe_unused rockchip_pcie_resume_noirq(struct device *dev)
>>> err_pcie_resume:
>>> rockchip_pcie_disable_clocks(rockchip);
>>> err_disable_0v9:
>>> - if (!IS_ERR(rockchip->vpcie0v9))
>>> - regulator_disable(rockchip->vpcie0v9);
>>> + regulator_disable(rockchip->vpcie0v9);
>>> return err;
>>> }
>>>
>>> @@ -1081,10 +1066,8 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
>>> regulator_disable(rockchip->vpcie12v);
>>> 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);
>>> + regulator_disable(rockchip->vpcie1v8);
>>> + regulator_disable(rockchip->vpcie0v9);
>>> err_set_vpcie:
>>> rockchip_pcie_disable_clocks(rockchip);
>>> return err;
>>> @@ -1108,10 +1091,8 @@ static int rockchip_pcie_remove(struct platform_device *pdev)
>>> regulator_disable(rockchip->vpcie12v);
>>> 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);
>>> + regulator_disable(rockchip->vpcie1v8);
>>> + regulator_disable(rockchip->vpcie0v9);
>>>
>>> return 0;
>>> }
>>> --
>>> 2.17.1
>>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>
>
More information about the Linux-rockchip
mailing list