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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 2020/11/7 下午8:54, Qu Wenruo wrote:
> 
> 
> 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@xxxxxxx>
>>>>> ---
>>>>>   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?

Not a regulator guys by all means, but the dtsi shows the 1v8 and 0v9
are all provided by rk808, while the dmesg shows:

[    0.195604] reg-fixed-voltage vcc3v3-pcie-regulator: Looking up
vin-supply from device tree
[    0.196096] reg-fixed-voltage vcc3v3-pcie-regulator: vcc3v3_pcie
supplying 3300000uV
[    0.197724] reg-fixed-voltage vcc5v0-host-regulator: Looking up
vin-supply from device tree
[    0.198211] reg-fixed-voltage vcc5v0-host-regulator: vcc5v0_host
supplying 0uV
[    0.198581] reg-fixed-voltage vcc5v0-typec-regulator: Looking up
vin-supply from device tree
[    0.199082] reg-fixed-voltage vcc5v0-typec-regulator: vcc5v0_typec
supplying 0uV
[    0.199395] reg-fixed-voltage vcc3v3-phy-regulator: vcc_lan supplying
3300000uV
[    1.074253] rockchip-pcie f8000000.pcie: no vpcie12v regulator found
[    1.086470] pwm-regulator vdd-log: Looking up pwm-supply from device tree
[    1.086484] pwm-regulator vdd-log: Looking up pwm-supply property in
node /vdd-log failed
[    1.086501] vdd_log: supplied by regulator-dummy
[    1.402500] rk808-regulator rk808-regulator: there is no dvs0 gpio
[    1.403104] rk808-regulator rk808-regulator: there is no dvs1 gpio
[    1.419856] rk808 0-001b: failed to register 12 regulator
[    1.422801] rk808-regulator: probe of rk808-regulator failed with
error -22

So this means something wrong with the rk808, resulting no voltage
provided from rk808 and screwing up the pcie controller?

Thanks,
Qu
> 
> 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@xxxxxxxxxxxxxxxxxxx
>>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>>>
>>>
>>
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 





[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux