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

[Qu Wenruo]

On 2020/11/9 下午8:00, Robin Murphy wrote:
> On 2020-11-07 13:30, Qu Wenruo wrote:
>>
>>
>> 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?
> 
> Frankly PCIe is the least of your worries there - if the system PMIC is
> failing to probe then pretty much everything will be degraded: cpufreq
> won't work, some SD card modes won't work, on some boards entire
> peripherals like ethernet might not work if they're wired up to use the
> opposite I/O domain voltage setting from the SoC's power-on default.
> 
> Focusing on PCIe getting probe-deferred seems a bit like complaining
> that the carpets on the Titanic are wet ;)

That's true. Thankfully bisect lead to the cause. It's even deeper in
the core regulator code.

Commit aea6cb99703e ("regulator: resolve supply after creating
regulator") makes the RK808 unable to register.

And fix for that commit is already merged in upstream.

Sorry for the disruption.

Thanks,
Qu

> 
> Robin.
>