Re: [PATCH v2 1/2] mmc: sdhci-of-arasan: add phy support for sdhci-of-arasan

[Shawn Lin <shawn.lin@rock-chips.com>]

On 2015/9/14 23:07, Sören Brinkmann wrote:
> Hi Shawn,
>
> overall, it looks good to me. I have some questions though.
>
> On Mon, 2015-09-14 at 02:29PM +0800, Shawn Lin wrote:

[...]

>> +err_phy_exit:
>> +	phy_init(phy);
>
> Just to confirm, are these actions in the error path correct? E.g.
> if the power_off() call fails, is it safe to call power_on()? Isn't
> the phy still powered on? (this would apply to other error paths too)
>

Cool question!

While writing this, I had read generic phy stuffs deliberately to find a 
solution for a case: how to deal with ping-pong fails? In another word, 
if power_off call fails, then we should call power_on, but unfortunately 
it fails again then we call power_off... so endless nested err 
handling... No answer yet.

So, I assumed two cases happened when power_off call fails:
(1) *real power_off* is done, but some other stuffs in the calling path 
fail, so phy is really power_off in theory. We need to power_on it 
again, but if it fail, we don't know PHY is on or off since we don't 
know power_on fails for what? *real power on* ? some other stuffs?

(2) *real power_off* isn't completed, so indeed it's *still* in power_on 
state. The reason we never need to check the return value of power_on 
cross the err handling is that whether power_on call successfully or 
not, it's always make phy in power_on state.

Now, let's think about case(1).
After reading dozens of sample codes(such as USB, UFS, MBUS) that adopt 
generic phy framework for PHY management, real thing should be like 
that: they NEVER deal with case(1).

It's a trick of sub-phy drivers themself. power_on/off calling path 
return err for two case:
<1>  phy_runtime callback fails. It's after *real power on/off*, so
definitely *real power on/off* is conpleted. That is the case(2) I 
mentioned above.
<2>  sub-phy drivers return err for  phy->ops->power_off(phy); Look
into all the sub-phy drivers twice, we find that they always return 
success for phy->ops->power_off hook. Why? Because all of them
write registers to enable/disable something, always consider things 
going well. Actually if we write value into a register, we have to think 
that it's functional.

Anyway, back to this patch.
Indeed we also write value into arasan phy's register to do 
phy_power_on/off/init/exit to make things work. Right, we return success 
state for all of these them just as all the other sub-phy drivers do.

Feel free to let me know if I make mistakes or misunderstanding above.

>> +	return ret;
>> +}

[...]

>> +		}
>> +	}
>
> I assume you looked at options for having the error paths in a
> consolidated location? I guess this may be the nicest solution since all
> of this is in this conditional block?
>

yep, otherwise we should add some *if* statements to check 
sdhci_arasan->phy cross the err handles. And I intent to strictly limit
the phy stuffs under the scope of arasan,sdhci-5.1 currently.

> Feel free to add my
> Acked-by: Sören Brinkmann <soren.brinkmann@xilinx.com>
>

Thanks, Sören. :)

> 	Sören
>
>
>


-- 
Best Regards
Shawn Lin