Re: [PATCH 1/2] PCI: rockchip: Make some regulators non-optional

[Qu Wenruo <wqu@suse.com>]

On 2020/11/7 下午8:47, Robin Murphy wrote:
> 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@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...

Sorry for the wrong wording, thank you for your contribution first.

It really makes RK3399 the primary testing bed for 64K page size, and
save me a lot of time.

I'm wondering how the -EPROBE_DEFER happens.
I have only tested two kernel versions, v5.9-rc4 and v5.10-rc2.
The config works for rc4, unless something big changed in rc2, it
shouldn't change much, right?

The 1v8 and 0v9 is just alwayson regulator, IMHO it doesn't really need
special driver.
Or does it?

Thanks,
Qu

> 
> 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@lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>>
>>
>