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

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

On 2015/10/19 15:50, Ulf Hansson wrote:
> [...]
>
>>> I understand the phy is optional, but you still need to handle the
>>> EPROBE_DEFER case.
>>>
>>> Perhaps you should also use devm_phy_optional_get() instead!?
>>
>>
>> I already changed it in version-2 [1]. :)
>> phy is mandatory for sdhci-arasan,5.1.
>>
>> [1]: https://patchwork.kernel.org/patch/7173251/
>
> Oh, apologize for reviewing the old version!
>
>>
>>>
>>>> +               ret = phy_power_on(sdhci_arasan->phy);
>>>
>>>
>>> This looks a bit weird. Shouldn't you do phy_init() prior phy_power_on()?
>>>
>>> Similar comment applies to phy_exit() and phy_power_off().
>>
>>
>> Both are okay. It depends how the phy-driver implement the four interfaces.
>>  From my case, power_on for arasan's phy driver firstly enable phy's clk and
>> open power-domain, then programme phy internal registers to activate phy.
>> Without enabling phy's clk and power-domain, we cannot do phy_init since phy
>> can't be accessed by CPU.
>>
>> But here, I think you are right. It does look a bit weird.
>> I think the better way is to remove phy_power_on here, and let phy-driver do
>> power_on in phy_init internally. Similarly in the remove call.
>
> That makes sense to me! I think it would also follow other phy clients
> use of the phy API.
>
> What makes me wonder though is from a power management point of view.
> *When* do you need to have phy initialized and powered?
>

Whenever controller need to communicate with card, must init/power_on 
phy firstly.

> 1) For a removable card can leave the phy uninitialised or powered
> off, but still detect card insertion/removal via GPIO? Is that a valid
> scenario for you?
>

Theoretically, it is.  Although my soc don't use phy for removable card, 
I also consider how to handle this case. Should we add a hook
for sdhci_get_cd, and initialize phy if it's non-removeable device or 
removeable card in slot ? Doesn't seem like a good idea.

Also seems not always work if we just use SDHCI_PRESENT_STATE to detect 
card insertion/removal without phy in active state.

> 2) Considering the runtime PM case for the sdhci device. Typically you
> can gate clocks etc at runtime suspend to save power, but what about
> the phy? Can you power off it in runtime suspend?

yes, we can power off it in runtime suspend. So we can append some 
patches later to introduce runtime pm for sdhci-of-arasan?

>
> [...]
>
> Kind regards
> Uffe
>
>
>


-- 
Best Regards
Shawn Lin