[PATCH] phy: rockchip-emmc: emmc_phy_init() always return 0

[Chris Ruehl]

On 5/12/2020 1:53 am, Doug Anderson wrote:
> Hi,
> 
> On Thu, Dec 3, 2020 at 7:01 PM Chris Ruehl <chris.ruehl at gtsys.com.hk> wrote:
>>
>> Hi,
>>
>> On 2/12/2020 4:36 pm, Chris Ruehl wrote:
>>>
>>> On 2/12/2020 12:05 am, Doug Anderson wrote:
>>>> Hi,
>>>>
>>>> On Mon, Nov 30, 2020 at 7:10 PM Chris Ruehl <chris.ruehl at gtsys.com.hk> wrote:
>>>>>
>>>>> rockchip_emmc_phy_init() return variable is not set with the error value
>>>>> if clk_get() failed. The debug message print 0 on error and the function
>>>>> always return 0.
>>>>> Fix it using PTR_ERR().
>>>>>
>>>>> Fixes: 52c0624a10cce phy: rockchip-emmc: Set phyctrl_frqsel based on card clock
>>>>>
>>>>> Signed-off-by: Chris Ruehl <chris.ruehl at gtsys.com.hk>
>>>>> ---
>>>>>    drivers/phy/rockchip/phy-rockchip-emmc.c | 1 +
>>>>>    1 file changed, 1 insertion(+)
>>>>>
>>>>> diff --git a/drivers/phy/rockchip/phy-rockchip-emmc.c
>>>>> b/drivers/phy/rockchip/phy-rockchip-emmc.c
>>>>> index 48e2d75b1004..75faee5c0d27 100644
>>>>> --- a/drivers/phy/rockchip/phy-rockchip-emmc.c
>>>>> +++ b/drivers/phy/rockchip/phy-rockchip-emmc.c
>>>>> @@ -253,6 +253,7 @@ static int rockchip_emmc_phy_init(struct phy *phy)
>>>>>            */
>>>>>           rk_phy->emmcclk = clk_get(&phy->dev, "emmcclk");
>>>>>           if (IS_ERR(rk_phy->emmcclk)) {
>>>>> +               ret = PTR_ERR(rk_phy->emmcclk);
>>>>
>>>> I'm pretty sure your patch isn't correct and it would break use cases.
>>>> Is it fixing some bug that you're aware of, or you found it via code
>>>> inspection?
>>>>
>>>> Specifically:
>>>>
>>>> * The big comment block in this function says that the clock is
>>>> optional and that we're ignoring errors.
>>>>
>>>> * The printout in this function is "dbg" level, which is an extra
>>>> indication that we aren't concerned with these errors.
>>>>
>>>> Arguably the code could be made better.  If you want to improve it,
>>>> you could check for just the error we expect if the clock isn't
>>>> specified (probably -ENODEV, but you should check) and treat all other
>>>> failures as real errors.
>>>>
>>>>
>>>> -Doug
>>>>
>>>
>>> Hi Doug,
>>>    I reviewed the code while hunting behind an other bug, with hs400
>>> and yes I saw the comment that they don't care about the problem
>>> if the clk_get() return an error, and set the rk_phy->emmcclk = NULL
>>> regardless, not using the ret variable but define it isn't useful.
>>>
>>> If return a error code break something on the other hand, better it
>>> hit it rather then suppress it in IMHO.
>>>
>>> Let me follow the caller of the function and see how they treat the
>>> err != 0.
>>>
>>> If something is in danger, I will be effected with my rk3399 rollout :)
>>>
>>> Chris
>>>
>>
>> I check my case, the dts properties emmcclk is defined for the rk3399.
>> (checked it I do not have an error, clk_get() works)
>> If clk_get() failed and we propagate error<0 to the phy-core.c which then
>> not increase the phy->init_count, but throw error message that something goes
>> wrong.
>>
>> Someone should explain to me, why we should cover up an error.
> 
> If you want to change this I'd guess you'd want to change the bindings
> in a backward-incompatible way.  The bindings
> (Documentation/devicetree/bindings/phy/rockchip-emmc-phy.txt) say:
> 
>   - clock-names: Should contain "emmcclk".  Although this is listed as optional
>                  (because most boards can get basic functionality without having
>                  access to it), it is strongly suggested.
> 
> Since it's officially listed as optional then the driver needs to
> function without it.  It was a long time ago, but my guess about why I
> needed to list it as optional was for the whole "device tree purity"
> reasons.  In other words the goal is always that old device trees
> should continue to work, so if you're adding new properties they need
> to be optional.
> 
> It seems highly unlikely anyone actually has a device tree that
> doesn't specify this clock, though.  If you want to cleanly go through
> and mark this as required everywhere I won't personally object.
> 
> -Doug
> 

Doug,

Yes, seems unlikely to me too, the definition for emmcclk is in rk3399.dtsi and
therefore in all the rk3399 boards I find in the dts/rockchip/.

To be backwards compatible, and keep it optional, I can check if clock-names is 
defined and if not, set emmcclk=null and return 0.
But if emmcclk is defined and an error returned from clk_get(), escalate it.

like below.

diff --git a/drivers/phy/rockchip/phy-rockchip-emmc.c 
b/drivers/phy/rockchip/phy-rockchip-emmc.c
index 75faee5c0d27..47126f9af31d 100644
--- a/drivers/phy/rockchip/phy-rockchip-emmc.c
+++ b/drivers/phy/rockchip/phy-rockchip-emmc.c
@@ -85,6 +85,7 @@ struct rockchip_emmc_phy {
         struct clk      *emmcclk;
         unsigned int drive_impedance;
         unsigned int enable_strobe_pulldown;
+       bool hasclockname;
  };

  static int rockchip_emmc_phy_power(struct phy *phy, bool on_off)
@@ -251,10 +252,14 @@ static int rockchip_emmc_phy_init(struct phy *phy)
          * above expected use case, EPROBE_DEFER isn't sensible to expect, so
          * it's just like any other error.
          */
-       rk_phy->emmcclk = clk_get(&phy->dev, "emmcclk");
-       if (IS_ERR(rk_phy->emmcclk)) {
-               ret = PTR_ERR(rk_phy->emmcclk);
-               dev_dbg(&phy->dev, "Error getting emmcclk: %d\n", ret);
+       if (rk_phy->hasclockname) {
+               rk_phy->emmcclk = clk_get(&phy->dev, "emmcclk");
+               if (IS_ERR(rk_phy->emmcclk)) {
+                       ret = PTR_ERR(rk_phy->emmcclk);
+                       dev_err(&phy->dev, "Error getting emmcclk: %d\n", ret);
+                       rk_phy->emmcclk = NULL;
+               }
+       } else {
                 rk_phy->emmcclk = NULL;
         }

@@ -373,6 +378,7 @@ static int rockchip_emmc_phy_probe(struct platform_device *pdev)
         rk_phy->reg_base = grf;
         rk_phy->drive_impedance = PHYCTRL_DR_50OHM;
         rk_phy->enable_strobe_pulldown = PHYCTRL_REN_STRB_DISABLE;
+       rk_phy->hasclockname = of_property_read_bool(dev->of_node, "clock-names");


Chris