Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1606245 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 4/5] clk: sunxi-ng: Add driver for A83T CCU Chen-Yu Tsai <wens@csie.org> - 2017-03-22 08:00 +0100
    Re: [PATCH 4/5] clk: sunxi-ng: Add driver for A83T CCU Maxime Ripard <maxime.ripard@free-electrons.com> - 2017-03-27 11:00 +0200
      Re: [PATCH 4/5] clk: sunxi-ng: Add driver for A83T CCU Chen-Yu Tsai <wens@csie.org> - 2017-03-27 11:00 +0200
        Re: [PATCH 4/5] clk: sunxi-ng: Add driver for A83T CCU Maxime Ripard <maxime.ripard@free-electrons.com> - 2017-04-03 10:00 +0200

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

On Wed, Feb 15, 2017 at 5:49 PM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> On Tue, Feb 14, 2017 at 06:26:39PM +0800, Chen-Yu Tsai wrote:
>> On Tue, Feb 14, 2017 at 5:58 PM, Maxime Ripard
>> <maxime.ripard@free-electrons.com> wrote:
>> > On Tue, Feb 14, 2017 at 11:35:25AM +0800, Chen-Yu Tsai wrote:
>> >> +/* Some PLLs are input * N / div1 / P. Model them as NKMP with no K */
>> >
>> > Is that even working?
>>
>> Looking at the nkmp clock code, only .recalc_rate will work properly though.
>> Maybe I could fix up the code so it handles zero width factors.
>>
>> > I'm not quite sure we want to do that. We might model it as a NP clock
>> > with a variable prediv?
>>
>> There's no NP clock type yet. And a problem with a variable prediv is that
>> it doesn't participate in factor calculation. It's effectively fixed.
>>
>> I did this for the A80 as well though. Fixing up the NKMP clock might be
>> easier.
>
> Then maybe we just need a NMP clock type then. What I'm really afraid
> of is that we'll just end up in a clk-factors situation that was
> simply impossible to maintain without breaking anything, hence why we
> had different clock types then.

Upon further review, I think it will work. I did notice a discrepancy
between .set_rate and .round_rate though. Will send fixes later.

About the old clk-factors situation, I'm not exactly sure what part
you're referring to. To me it seems the "factors" bits are mostly the
same. Differences are mostly with parent-specific pre-dividers,
clock post-dividers, and non-standard factors. The first is nicely
handled by the new mux wrapper, the second is currently only used
with NK types, and the last is currently only supported by single
factor divider or multiplier clocks with tables.

Non-standard factors are probably the trickiest one, but given we will
support full factor tables for some of the tricky CPU PLLs, this is
probably solved, even if not implemented yet.

I'll start with the NP style clocks, which only use P when the output
is under a certain frequency.

Regards
ChenYu

>> >
>> >> +/* Use a separate clock for the pre-divider on the AHB1 PLL-PERIPH input */
>> >> +static SUNXI_CCU_M(pll_periph_ahb1_clk, "pll-periph-ahb1", "pll-periph",
>> >> +                0x054, 6, 2, 0);
>> >> +
>> >> +static const char * const ahb1_parents[] = { "osc16M-d512", "osc24M",
>> >> +                                          "pll-periph-ahb1",
>> >> +                                          "pll-periph-ahb1" };
>> >> +static struct ccu_div ahb1_clk = {
>> >> +     .div            = _SUNXI_CCU_DIV_FLAGS(4, 2, CLK_DIVIDER_POWER_OF_TWO),
>> >> +     .mux            = _SUNXI_CCU_MUX(12, 2),
>> >> +     .common         = {
>> >> +             .reg            = 0x054,
>> >> +             .hw.init        = CLK_HW_INIT_PARENTS("ahb1",
>> >> +                                                   ahb1_parents,
>> >> +                                                   &ccu_div_ops,
>> >> +                                                   0),
>> >> +     },
>> >> +};
>> >
>> > What's different from a pre divider only for a given index here?
>>
>> The variable pre-divider is shared for both pll-periph mux inputs.
>> This is one way to handle it. The other would be to extend ccu_mux
>> to handle multiple variable pre-dividers. I don't really want to do
>> that if this is the only instance that needs it though.
>
> Every addition we made was only needed by one instance at first :)
>
> We are working that way for fixed pre-dividers already, I don't see
> why we can't have it for variable ones too.
>
>> >> +/*
>> >> + * MMC2 supports what's called the "new timing mode". The CCU and the MMC
>> >> + * controller must be in sync about which mode is used. The new mode moves
>> >> + * the clock delay controls (and possibly the delay lines) into the MMC
>> >> + * block. Also, the output of the clock is divided by 2. The output and
>> >> + * sample phase clocks are unused under this mode.
>> >> + *
>> >> + * This new mode seems to be preferred. Hence we force this clock to the
>> >> + * new mode. And we don't add the phase clocks.
>> >> + */
>> >
>> > I'm sorry, but I said this several times, this isn't working. We
>> > should model it properly, and not hack this around in the clock
>> > driver.
>> >
>> > As you say in your comment, the MMC driver needs to be aware about
>> > which mode is used, in order to also set a bit in one of its registers
>> > accordingly, and modify its sampling behaviour.
>> >
>> > The new timing is preferred, but our previous clock implementations
>> > didn't hardcode it, so we can't even rely on that behaviour to always
>> > write it in our driver.
>>
>> Correct. With the A83T there has never been a merged clock driver though.
>> I realize this is a one off thing.
>>
>> > This is not something specific to the A83T, but is found in all the
>> > SoCs since the A23, so we need to come up with a good solution to
>> > address that.
>> >
>> > I'm not sure what a good solution would be though. One would be to
>> > just have a private function of our own to switch in the new mode (if
>> > relevant, because only the MMC2 controllers have it), but that would
>> > lead to troubles with !sunxi-ng. Not something we can't deal with, but
>> > some extra precautions should be taken (make sure to protect the call
>> > through an ifdef / IS_DEFINED, check that the sunxi-ng driver has been
>> > probed, etc.)
>>
>> If the custom function route is acceptable, I'll come up with something.
>
> I think it would be a great start yes. I'll try to discuss it with
> Mike and Stephen at ELC and see what they think about that.
>
> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com

[toc] | [next] | [standalone]

#1609583

[Multipart message — attachments visible in raw view] — view raw

Hi Chen-Yu,

On Wed, Mar 22, 2017 at 02:50:31PM +0800, Chen-Yu Tsai wrote:
> On Wed, Feb 15, 2017 at 5:49 PM, Maxime Ripard
> <maxime.ripard@free-electrons.com> wrote:
> > On Tue, Feb 14, 2017 at 06:26:39PM +0800, Chen-Yu Tsai wrote:
> >> On Tue, Feb 14, 2017 at 5:58 PM, Maxime Ripard
> >> <maxime.ripard@free-electrons.com> wrote:
> >> > On Tue, Feb 14, 2017 at 11:35:25AM +0800, Chen-Yu Tsai wrote:
> >> >> +/* Some PLLs are input * N / div1 / P. Model them as NKMP with no K */
> >> >
> >> > Is that even working?
> >>
> >> Looking at the nkmp clock code, only .recalc_rate will work properly though.
> >> Maybe I could fix up the code so it handles zero width factors.
> >>
> >> > I'm not quite sure we want to do that. We might model it as a NP clock
> >> > with a variable prediv?
> >>
> >> There's no NP clock type yet. And a problem with a variable prediv is that
> >> it doesn't participate in factor calculation. It's effectively fixed.
> >>
> >> I did this for the A80 as well though. Fixing up the NKMP clock might be
> >> easier.
> >
> > Then maybe we just need a NMP clock type then. What I'm really afraid
> > of is that we'll just end up in a clk-factors situation that was
> > simply impossible to maintain without breaking anything, hence why we
> > had different clock types then.
> 
> Upon further review, I think it will work. I did notice a discrepancy
> between .set_rate and .round_rate though. Will send fixes later.
> 
> About the old clk-factors situation, I'm not exactly sure what part
> you're referring to.

We need to be able to support old DTs, which will still use the old
clock code. Whatever solution we come up with need to take that into
account.

> To me it seems the "factors" bits are mostly the same. Differences
> are mostly with parent-specific pre-dividers, clock post-dividers,
> and non-standard factors. The first is nicely handled by the new mux
> wrapper, the second is currently only used with NK types, and the
> last is currently only supported by single factor divider or
> multiplier clocks with tables.
> 
> Non-standard factors are probably the trickiest one, but given we will
> support full factor tables for some of the tricky CPU PLLs, this is
> probably solved, even if not implemented yet.
> 
> I'll start with the NP style clocks, which only use P when the output
> is under a certain frequency.

Do we need to use a P factor? I mean, we can just create a custom
clock for that, I'd realy don't want to cripple the generic code for a
completely non-generic problem.

Maxime

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

[toc] | [prev] | [next] | [standalone]

#1609585

On Mon, Mar 27, 2017 at 4:51 AM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> Hi Chen-Yu,
>
> On Wed, Mar 22, 2017 at 02:50:31PM +0800, Chen-Yu Tsai wrote:
>> On Wed, Feb 15, 2017 at 5:49 PM, Maxime Ripard
>> <maxime.ripard@free-electrons.com> wrote:
>> > On Tue, Feb 14, 2017 at 06:26:39PM +0800, Chen-Yu Tsai wrote:
>> >> On Tue, Feb 14, 2017 at 5:58 PM, Maxime Ripard
>> >> <maxime.ripard@free-electrons.com> wrote:
>> >> > On Tue, Feb 14, 2017 at 11:35:25AM +0800, Chen-Yu Tsai wrote:
>> >> >> +/* Some PLLs are input * N / div1 / P. Model them as NKMP with no K */
>> >> >
>> >> > Is that even working?
>> >>
>> >> Looking at the nkmp clock code, only .recalc_rate will work properly though.
>> >> Maybe I could fix up the code so it handles zero width factors.
>> >>
>> >> > I'm not quite sure we want to do that. We might model it as a NP clock
>> >> > with a variable prediv?
>> >>
>> >> There's no NP clock type yet. And a problem with a variable prediv is that
>> >> it doesn't participate in factor calculation. It's effectively fixed.
>> >>
>> >> I did this for the A80 as well though. Fixing up the NKMP clock might be
>> >> easier.
>> >
>> > Then maybe we just need a NMP clock type then. What I'm really afraid
>> > of is that we'll just end up in a clk-factors situation that was
>> > simply impossible to maintain without breaking anything, hence why we
>> > had different clock types then.
>>
>> Upon further review, I think it will work. I did notice a discrepancy
>> between .set_rate and .round_rate though. Will send fixes later.
>>
>> About the old clk-factors situation, I'm not exactly sure what part
>> you're referring to.
>
> We need to be able to support old DTs, which will still use the old
> clock code. Whatever solution we come up with need to take that into
> account.
>
>> To me it seems the "factors" bits are mostly the same. Differences
>> are mostly with parent-specific pre-dividers, clock post-dividers,
>> and non-standard factors. The first is nicely handled by the new mux
>> wrapper, the second is currently only used with NK types, and the
>> last is currently only supported by single factor divider or
>> multiplier clocks with tables.
>>
>> Non-standard factors are probably the trickiest one, but given we will
>> support full factor tables for some of the tricky CPU PLLs, this is
>> probably solved, even if not implemented yet.
>>
>> I'll start with the NP style clocks, which only use P when the output
>> is under a certain frequency.
>
> Do we need to use a P factor? I mean, we can just create a custom
> clock for that, I'd realy don't want to cripple the generic code for a
> completely non-generic problem.

I'm not sure. AFAIK the vendor BSP cpufreq doesn't use frequencies
low enough to require the P divider, so we could just ignore it. But
then we need to make sure it's set to 1 at probe time, while keeping
the output frequency usable, which would kind of bloat the probe
function. FYI I'm in favor of doing it this way.

ChenYu

[toc] | [prev] | [next] | [standalone]

#1614957

[Multipart message — attachments visible in raw view] — view raw

On Mon, Mar 27, 2017 at 04:53:05PM +0800, Chen-Yu Tsai wrote:
> >> To me it seems the "factors" bits are mostly the same. Differences
> >> are mostly with parent-specific pre-dividers, clock post-dividers,
> >> and non-standard factors. The first is nicely handled by the new mux
> >> wrapper, the second is currently only used with NK types, and the
> >> last is currently only supported by single factor divider or
> >> multiplier clocks with tables.
> >>
> >> Non-standard factors are probably the trickiest one, but given we will
> >> support full factor tables for some of the tricky CPU PLLs, this is
> >> probably solved, even if not implemented yet.
> >>
> >> I'll start with the NP style clocks, which only use P when the output
> >> is under a certain frequency.
> >
> > Do we need to use a P factor? I mean, we can just create a custom
> > clock for that, I'd realy don't want to cripple the generic code for a
> > completely non-generic problem.
> 
> I'm not sure. AFAIK the vendor BSP cpufreq doesn't use frequencies
> low enough to require the P divider, so we could just ignore it. But
> then we need to make sure it's set to 1 at probe time, while keeping
> the output frequency usable, which would kind of bloat the probe
> function. FYI I'm in favor of doing it this way.

Hmmm, I don't know why I replied that anymore, I thought you were
still talking about MMC, while you were clearly talking about
CPU_PLLs...

The question still remains though. If we're not using P yet, and if
the BSP doesn't either, then we can just hardcode it.

We can always come up with something later if we need to use it,
either an NP-class, or a table. The only thing we need to care about
would be to pay attention to what the P factor already is, before
forcing it to 1. If it's set to 4, that would mean multiplying the CPU
clock by 4, which is probably not such a great idea.

Maxime

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web