On 2020/11/7 下午7: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.
>
> 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.
>
> 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.
My bad, it's not that easy. The dtsi has vpcie1v8 and vpcie0v9 defined.
It should be something else wrong.
Thanks,
Qu
>
> 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
>>
