[PATCH 1/2] PCI: rockchip: Make some regulators non-optional
Qu Wenruo
wqu at suse.com
Sat Nov 7 08:30:59 EST 2020
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 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...
>
> 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 at lists.infradead.org
>>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>>>
>>>
>>
>
>
> _______________________________________________
> 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-arm-kernel
mailing list