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

[Qu Wenruo <wqu@suse.com>]

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