Re: [PATCH v12 1/6] ARM: sunxi: Add driver for SD/MMC hosts found on Allwinner sunxi SoCs

[Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

Hi,

On 05/12/2014 01:34 PM, Ulf Hansson wrote:
> On 12 May 2014 13:20, Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> Hi,
>>
>> On 05/12/2014 11:15 AM, Ulf Hansson wrote:
>>> On 11 May 2014 09:46, Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>>>> From: David Lanzendörfer <david.lanzendoerfer-Z7Kmv9EsliU@public.gmane.org>
>>>>
>>>> The Allwinner sunxi mmc host uses dma in bus-master mode using a built-in
>>>> designware idmac controller, which is identical to the one found in the mmc-dw
>>>> hosts. However the rest of the host is not identical to mmc-dw, it deals with
>>>> sending stop commands in hardware which makes it significantly different
>>>> from the mmc-dw devices.
>>>>
>>>> HdG: Various cleanups and fixes.
>>>>
>>>> Signed-off-by: David Lanzendörfer <david.lanzendoerfer-Z7Kmv9EsliU@public.gmane.org>
>>>> Signed-off-by: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>>>
>>> [snip]
>>>
>>>> +
>>>> +static int sunxi_mmc_clk_set_rate(struct sunxi_mmc_host *host,
>>>> +                                 struct mmc_ios *ios)
>>>> +{
>>>> +       u32 rate, oclk_dly, rval, sclk_dly, src_clk;
>>>> +       struct clk_hw *hw = __clk_get_hw(host->clk_mmc);
>>>
>>> Hi Hans,
>>>
>>> This seems like the wrong approach. But I guess it's related to the
>>> "clock phase control" API you have been discussing with the clk
>>> maintainer, Mike Turquette!?
>>
>> Yes, this is meant as a temporary solution until we get a proper
>> "clock phase control" API.
>>
>>>
>>> __clk_get_hw is supposed to be used by clk providers, not clk consumers.
>>>
>>>> +       int ret;
>>>> +
>>>> +       rate = clk_round_rate(host->clk_mmc, ios->clock);
>>>> +       dev_dbg(mmc_dev(host->mmc), "setting clk to %d, rounded %d\n",
>>>> +               ios->clock, rate);
>>>> +
>>>> +       /* setting clock rate */
>>>> +       ret = clk_set_rate(host->clk_mmc, rate);
>>>> +       if (ret) {
>>>> +               dev_err(mmc_dev(host->mmc), "error setting clk to %d: %d\n",
>>>> +                       rate, ret);
>>>> +               return ret;
>>>> +       }
>>>> +
>>>> +       ret = sunxi_mmc_oclk_onoff(host, 0);
>>>> +       if (ret)
>>>> +               return ret;
>>>> +
>>>> +       /* clear internal divider */
>>>> +       rval = mmc_readl(host, REG_CLKCR);
>>>> +       rval &= ~0xff;
>>>> +       mmc_writel(host, REG_CLKCR, rval);
>>>> +
>>>> +       /* determine delays */
>>>> +       if (rate <= 400000) {
>>>> +               oclk_dly = 0;
>>>> +               sclk_dly = 7;
>>>> +       } else if (rate <= 25000000) {
>>>> +               oclk_dly = 0;
>>>> +               sclk_dly = 5;
>>>> +       } else if (rate <= 50000000) {
>>>> +               if (ios->timing == MMC_TIMING_UHS_DDR50) {
>>>> +                       oclk_dly = 2;
>>>> +                       sclk_dly = 4;
>>>> +               } else {
>>>> +                       oclk_dly = 3;
>>>> +                       sclk_dly = 5;
>>>> +               }
>>>> +       } else {
>>>> +               /* rate > 50000000 */
>>>> +               oclk_dly = 2;
>>>> +               sclk_dly = 4;
>>>> +       }
>>>> +
>>>> +       src_clk = clk_get_rate(clk_get_parent(host->clk_mmc));
>>>> +       if (src_clk >= 300000000 && src_clk <= 400000000) {
>>>> +               if (oclk_dly)
>>>> +                       oclk_dly--;
>>>> +               if (sclk_dly)
>>>> +                       sclk_dly--;
>>>> +       }
>>>> +
>>>> +       clk_sunxi_mmc_phase_control(hw, sclk_dly, oclk_dly);
>>>> +
>>>> +       return sunxi_mmc_oclk_onoff(host, 1);
>>>> +}
>>>> +
>>>
>>> [snip]
>>>
>>> Besides the above, I think this looks good!
>>
>> Thanks!
>>
>> Since Mike Turquette has already merged clk_sunxi_mmc_phase_control
>> I would like to keep this as is, as I already promised Mike, I can
>> safely promise you too: I promise clean this up as soon as the
>> "clock phase control" API has been worked out.
> 
> Thanks for promise. :-)
> 
> Anyway, I think I would like Mike to confirm this violation is
> accepted as a temporary solution. Additionally I would like it to be
> commented in the code here, that it's a temporary solution and that it
> violates the clk API.

Thinking more about this the __clk_get_hw call really should have
been inside the clk_sunxi_mmc_phase_control function. I'll do a followup
patch for Mike / the clk tree to fix this, and do a v13 of the
sunxi-mmc driver using the updated version.

Regards,

Hans

-- 
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.