Re: [PATCH 2/3] phy: rockchip-emmc: Be tolerant to card clock of 0 in power on

[Ulf Hansson]

On 29 June 2016 at 17:18, Doug Anderson <dianders@xxxxxxxxxxxx> wrote:
> Kishon,
>
> On Wed, Jun 29, 2016 at 6:49 AM, Kishon Vijay Abraham I <kishon@xxxxxx> wrote:
>> Hi,
>>
>> On Monday 27 June 2016 11:09 PM, Douglas Anderson wrote:
>>> It's possible that there are some reasons to turn the PHY on while the
>>> clock is 0.  In this case we just won't wait for the DLL to lock.
>>>
>>> This is a bit of a stopgap until we figure out exactly when we're
>>> supposed to wait for the DLL to lock and when we're supposed to power
>>> cycle the PHY.
>>>
>>> Note: this patch should help with suspend/resume where the system will
>>> try to turn the PHY back on when the clock is 0.
>>>
>>> Signed-off-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
>>> ---
>>>  drivers/phy/phy-rockchip-emmc.c | 59 ++++++++++++++++++++++++++---------------
>>>  1 file changed, 37 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/drivers/phy/phy-rockchip-emmc.c b/drivers/phy/phy-rockchip-emmc.c
>>> index 9dce958233a0..a2aa6aca7dec 100644
>>> --- a/drivers/phy/phy-rockchip-emmc.c
>>> +++ b/drivers/phy/phy-rockchip-emmc.c
>>> @@ -88,15 +88,36 @@ static int rockchip_emmc_phy_power(struct phy *phy, bool on_off)
>>>       unsigned int caldone;
>>>       unsigned int dllrdy;
>>>       unsigned int freqsel = PHYCTRL_FREQSEL_200M;
>>> +     unsigned long rate;
>>>       unsigned long timeout;
>>>
>>> -     if (rk_phy->emmcclk != NULL) {
>>> -             unsigned long rate = clk_get_rate(rk_phy->emmcclk);
>>> +     /*
>>> +      * Keep phyctrl_pdb and phyctrl_endll low to allow
>>> +      * initialization of CALIO state M/C DFFs
>>> +      */
>>> +     regmap_write(rk_phy->reg_base,
>>> +                  rk_phy->reg_offset + GRF_EMMCPHY_CON6,
>>> +                  HIWORD_UPDATE(PHYCTRL_PDB_PWR_OFF,
>>> +                                PHYCTRL_PDB_MASK,
>>> +                                PHYCTRL_PDB_SHIFT));
>>> +     regmap_write(rk_phy->reg_base,
>>> +                  rk_phy->reg_offset + GRF_EMMCPHY_CON6,
>>> +                  HIWORD_UPDATE(PHYCTRL_ENDLL_DISABLE,
>>> +                                PHYCTRL_ENDLL_MASK,
>>> +                                PHYCTRL_ENDLL_SHIFT));
>>> +
>>> +     /* Already finish power_off above */
>>> +     if (on_off == PHYCTRL_PDB_PWR_OFF)
>>> +             return 0;
>>> +
>>> +     rate = clk_get_rate(rk_phy->emmcclk);
>>> +
>>> +     if (rate != 0) {
>>>               unsigned long ideal_rate;
>>>               unsigned long diff;
>>>
>>>               switch (rate) {
>>> -             case 0 ... 74999999:
>>> +             case 1 ... 74999999:
>>>                       ideal_rate = 50000000;
>>>                       freqsel = PHYCTRL_FREQSEL_50M;
>>>                       break;
>>> @@ -127,25 +148,6 @@ static int rockchip_emmc_phy_power(struct phy *phy, bool on_off)
>>>       }
>>>
>>>       /*
>>> -      * Keep phyctrl_pdb and phyctrl_endll low to allow
>>> -      * initialization of CALIO state M/C DFFs
>>> -      */
>>> -     regmap_write(rk_phy->reg_base,
>>> -                  rk_phy->reg_offset + GRF_EMMCPHY_CON6,
>>> -                  HIWORD_UPDATE(PHYCTRL_PDB_PWR_OFF,
>>> -                                PHYCTRL_PDB_MASK,
>>> -                                PHYCTRL_PDB_SHIFT));
>>> -     regmap_write(rk_phy->reg_base,
>>> -                  rk_phy->reg_offset + GRF_EMMCPHY_CON6,
>>> -                  HIWORD_UPDATE(PHYCTRL_ENDLL_DISABLE,
>>> -                                PHYCTRL_ENDLL_MASK,
>>> -                                PHYCTRL_ENDLL_SHIFT));
>>> -
>>> -     /* Already finish power_off above */
>>> -     if (on_off == PHYCTRL_PDB_PWR_OFF)
>>> -             return 0;
>>> -
>>> -     /*
>>>        * According to the user manual, calpad calibration
>>>        * cycle takes more than 2us without the minimal recommended
>>>        * value, so we may need a little margin here
>>> @@ -183,6 +185,19 @@ static int rockchip_emmc_phy_power(struct phy *phy, bool on_off)
>>>                    HIWORD_UPDATE(PHYCTRL_ENDLL_ENABLE,
>>>                                  PHYCTRL_ENDLL_MASK,
>>>                                  PHYCTRL_ENDLL_SHIFT));
>>> +
>>> +     /*
>>> +      * We turned on the DLL even though the rate was 0 because we the
>>> +      * clock might be turned on later.  ...but we can't wait for the DLL
>>> +      * to lock when the rate is 0 because it will never lock with no
>>> +      * input clock.
>>> +      *
>>> +      * Technically we should be checking the lock later when the clock
>>> +      * is turned on, but for now we won't.
>>> +      */
>>> +     if (rate == 0)
>>> +             return 0;
>>
>> Why not return initially from rockchip_emmc_phy_power if the clock rate is '0'.
>> Are there other functions to lock the DLL apart from phy_power?
>
> Yeah, it's a big ugly right now.  This ugliness is really needed
> because of <https://patchwork.kernel.org/patch/9201035/> because:
>
> 1. We power on the PHY at probe time and the card clock is in an
> unknown state at that time.  It will be reported as 0 right now, but
> it may or may not actually be 0.
>
> 2. We don't have an easy way to call back into the PHY when we
> actually set the clock to a low rate (like 400kHz) for ID mode.
> Before this series I tried to power the PHY off and on for every clock
> change, but apparently that was causing problems.
>
>
> As talked about in <https://patchwork.kernel.org/patch/9201035/>, I
> think the right answer is to figure out how to get the common clock
> framework notifications to happen for the card clock and then remove
> the wholesale PHY power off / power on for every clock change.  The
> PHY itself can register for the clock change notifications and figure
> out how much or how little to do on every clock change.
>
> Unfortunately, as also discussed in the other patch, it's not trivial
> to do this because I think it requires surgery on the main SDHCI
> driver to change the way it deals with the card clock.  I'm not sure I
> have time for this delicate surgery right now and I'm hoping that
> perhaps Shawn will be able to help figure something out (maybe?) or I
> can try coming back to it later.
>
>
> In any case, I think a wholesale revert of my previous 150 MHz series
> probably puts us in a worse state than we started with, so I was just
> proposing reverting the one patch.  Once we do that, this PHY patch
> helps keep us in a sane state (keeps suspend/resume working).
>
>
> -Doug

Doug, Kishon,

Did you agree on how to move forward with this change?

Kind regards
Uffe