Re: [PATCH 4/5] clk: sunxi-ng: Add driver for A83T CCU

[Maxime Ripard]

Hi Stephen,

On Fri, Mar 03, 2017 at 03:56:39PM -0800, Stephen Boyd wrote:
> On 03/03, Maxime Ripard wrote:
> > On Wed, Mar 01, 2017 at 11:17:05AM -0800, Stephen Boyd wrote:
> > 
> > > Can someone explain what the issue is? Could something like
> > > clk_get_phase() + clk_get_rate() tell us if we're in one mode
> > > vs. the other?
> > 
> > So we have two modes of operation for that clock, old vs new (I know,
> > I didn't pick the names).
> > 
> > The old mode is what we support right now. It has a combination of a
> > linear multiplier and divider, plus some phase controls.
> > 
> > The new mode however disables the phase controls and adds post-divider
> > of 2 on the rate.
> > 
> > We cannot really rely on the rate itself, since there's a huge overlap
> > between the rates we can obtain in the old and new modes. Same thing
> > for the phase, having a 0 deg phase is achieved both in the old and
> > new modes.
> > 
> > To make things worse, the new mode is only available on one out of
> > three MMC controllers (and associated clocks), and that MMC controller
> > needs to set a bit as well to switch to the new mode if needed. So we
> > definitely needs some synchronisation there, and also to be able to
> > retrieve if the mode switching is available, and if we're already
> > using that mode.
> > 
> > Mike agreed that the easiest way forward was to use a custom function.
> 
> Ok. Is there any need to change the mode dynamically at runtime?
> Or could it be decided once at clk driver probe time/boot time
> and detected via set_phase() failing when we're in the new mode?

One thing I forgot to mention is that we also still have to support
the old DTs that use our old clock drivers, that will probably never
get to see this new mode. So the first thing we need is being able to
tell whether that mode is supported and if it's already enabled.

And if it's supported, and not enabled, enable it, both in the clock
and MMC drivers.

> At least, it sounds like set_phase() should bail out there
> because it doesn't exist, although it could be argued that
> setting the phase to something it already is set to is valid and
> shouldn't return an error.

Yep, once the new mode is set (disregarding how we do it), we should
prevent any clk_set_phase != 0. We'll also need to adjust the reported
clock rate.

> I'm not saying I'm opposed to the custom function, just thinking
> of alternatives if MMC maintainers don't agree with the custom
> function.

Ok, thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Attachment: signature.asc
Description: PGP signature