RE: [PATCH v5 1/3] mmc: sh_mobile_sdhi: add support for 2 clocks

[Chris Brandt <Chris.Brandt@renesas.com>]

Hi Geert,

On Monday, January 23, 2017, Geert Uytterhoeven:
> > @@ -190,6 +191,12 @@ static int sh_mobile_sdhi_clk_enable(struct
> tmio_mmc_host *host)
> >         if (ret < 0)
> >                 return ret;
> >
> > +       ret = clk_prepare_enable(priv->clk_cd);
> > +       if (ret < 0) {
> > +               clk_disable_unprepare(priv->clk);
> > +               return ret;
> > +       }
> > +
> 
> As enabling the "core" clock but not the "cd" clock is not a valid setting,
> shouldn't the cd clock be enabled first?

I'm pretty sure the chip designers just didn't want you to 'use' the
peripheral with that clock configuration, and we are not using it in
between those 2 lines so I personally think it's fine.

If I need to change it to get it approved, let me know and I'll change
the code around.

(In reality, I checked and the SDHI does work with the cd clock off,
but I don't know if the restriction in the spec was because they never
fully tested the use of it that way because it wasn't a design goal for
them.)



> >         /*
> >          * The clock driver may not know what maximum frequency
> >          * actually works, so it should be set with the max-frequency
> > @@ -255,6 +262,8 @@ static void sh_mobile_sdhi_clk_disable(struct
> tmio_mmc_host *host)
> >         struct sh_mobile_sdhi *priv = host_to_priv(host);
> >
> >         clk_disable_unprepare(priv->clk);
> > +       if (priv->clk_cd)
> 
> No need to check for a NULL pointer first.

OK, I see that IS_ERR_OR_NULL(clk) is done for clk_disable_unprepare so
I'll take the check out.
Thank you.


> > +               clk_disable_unprepare(priv->clk_cd);
> 
> Disabling is already done in the correct order ;-)

I guess you could get into some theoretical technical discussion around
if it is OK to enable/disable the clocks in any order (as long as you
don't use the SDHI in between), then you would want to enable the cd
circuit last, and disable it first so you would ensure that no cd
interrupts are being registered when the core clock is off?
In that case, the current enable order should remain but the disable
order should change.

Opinions???


Thank you,
Chris