Problem with Allwinner H3 clocks

[Jean-Francois Moine]

Hi Jens,

My H3 machine (OPI2) cannot boot with the PLL6 (periph0) as defined
in the kernel 4.5-rc1. As there is no UART, I don't know what is wrong.

But, applying your old patch

[PATCH v4 1/6] clk: sunxi: Let divs clocks read the base factor clock name from devicetree
(https://lkml.org/lkml/2015/10/27/532)

makes everything work correctly again (thanks to other patches, I have
4 CPUs, USB, thermal sensor and video).

Any idea?

--
Ken ar c'hentañ | ** Breizh ha Linux atav! **
Jef | http://moinejf.free.fr/

[Chen-Yu Tsai]

Hi,

On Wed, Jan 27, 2016 at 3:46 PM, Jean-Francois Moine <moi...@free.fr> wrote:
> Hi Jens,
>
> My H3 machine (OPI2) cannot boot with the PLL6 (periph0) as defined
> in the kernel 4.5-rc1. As there is no UART, I don't know what is wrong.
>
> But, applying your old patch
>
> [PATCH v4 1/6] clk: sunxi: Let divs clocks read the base factor clock name from devicetree
> (https://lkml.org/lkml/2015/10/27/532)
>
> makes everything work correctly again (thanks to other patches, I have
> 4 CPUs, USB, thermal sensor and video).
>
> Any idea?

What kernel and DTS are you using? What other patches have you applied?

Patches for H3 USB, thermal sensor and video have not been merged, so it's
likely to have some integration problems. FYI you should always check the
result after self-applying or rebasing DT patches, as there isn't enough
context for git to know if a patch has been applied or not.

4.5-rc1 + sunxi-next boots fine on my Orange PI PC, using the Orange PI Plus
DTS for now.


Regards
ChenYu

[Jean-Francois Moine]

On Wed, 27 Jan 2016 16:18:53 +0800
Chen-Yu Tsai <we...@csie.org> wrote:

> Hi,

Hi ChenYu,

> On Wed, Jan 27, 2016 at 3:46 PM, Jean-Francois Moine <moi...@free.fr> wrote:
> > Hi Jens,
> >
> > My H3 machine (OPI2) cannot boot with the PLL6 (periph0) as defined
> > in the kernel 4.5-rc1. As there is no UART, I don't know what is wrong.
> >
> > But, applying your old patch
> >
> > [PATCH v4 1/6] clk: sunxi: Let divs clocks read the base factor clock name from devicetree
> > (https://lkml.org/lkml/2015/10/27/532)
> >
> > makes everything work correctly again (thanks to other patches, I have
> > 4 CPUs, USB, thermal sensor and video).
> >
> > Any idea?
>
> What kernel and DTS are you using? What other patches have you applied?

About the clock problem, I tried the 4.5-rc1 kernel with its included DTs
(sun8i-h3-orangepi-plus.dts) without patch. No UART.
Changing the PLL6 (and the phandles in the DT) makes the UART work.

> Patches for H3 USB, thermal sensor and video have not been merged, so it's
> likely to have some integration problems. FYI you should always check the
> result after self-applying or rebasing DT patches, as there isn't enough
> context for git to know if a patch has been applied or not.

Many patches (USB) are available in Hans de Goede's repository.
Some other ones either work fine directly from their submission
(thermal sensor) or ask for few changes (PRCM).
Video is my development. I will submit a new patch series as soon as
the hardware cursor works.

> 4.5-rc1 + sunxi-next boots fine on my Orange PI PC, using the Orange PI Plus
> DTS for now.

Strange! I already had this problem when Jens removed his work about
PLL6 in his H3 patch series. At this time, I was thinking about a merge
error of mine...

--
A galon | ** Breizh ha Linux atav! **
Jef | http://moinejf.free.fr/

[Jens Kuske]

On 27/01/16 10:37, Jean-Francois Moine wrote:
> On Wed, 27 Jan 2016 16:18:53 +0800
> Chen-Yu Tsai <we...@csie.org> wrote:
>
>> Hi,
>
> Hi ChenYu,
>
>> On Wed, Jan 27, 2016 at 3:46 PM, Jean-Francois Moine <moi...@free.fr> wrote:
>>> Hi Jens,
>>>
>>> My H3 machine (OPI2) cannot boot with the PLL6 (periph0) as defined
>>> in the kernel 4.5-rc1. As there is no UART, I don't know what is wrong.
>>>
>>> But, applying your old patch
>>>
>>> [PATCH v4 1/6] clk: sunxi: Let divs clocks read the base factor clock name from devicetree
>>> (https://lkml.org/lkml/2015/10/27/532)
>>>
>>> makes everything work correctly again (thanks to other patches, I have
>>> 4 CPUs, USB, thermal sensor and video).
>>>
>>> Any idea?
>>
>> What kernel and DTS are you using? What other patches have you applied?
>
> About the clock problem, I tried the 4.5-rc1 kernel with its included DTs
> (sun8i-h3-orangepi-plus.dts) without patch. No UART.
> Changing the PLL6 (and the phandles in the DT) makes the UART work.

Hi,

That sounds strange, 4.5-rc1 is working perfectly fine for me too.

I doubt the patch you linked is responsible for making it work, it only
removes the hardcoded output-names. If your DT isn't messed up this
isn't relevant at all since pll8, the initial reason for this patch, is
only a dummy clock for now. Are you sure you are using a clean v4.5-rc1
without any own modifications?

Jens

[Jean-Francois Moine]

On Wed, 27 Jan 2016 15:36:21 +0100
Jens Kuske <jens...@gmail.com> wrote:

> That sounds strange, 4.5-rc1 is working perfectly fine for me too.
>
> I doubt the patch you linked is responsible for making it work, it only
> removes the hardcoded output-names. If your DT isn't messed up this
> isn't relevant at all since pll8, the initial reason for this patch, is
> only a dummy clock for now. Are you sure you are using a clean v4.5-rc1
> without any own modifications?

To be sure, I generated a pure 4.5-rc1 kernel. Same result: no UART.
Maybe... one more information: I am using Allwinner's u-boot.

--
Ken ar c'hentañ | ** Breizh ha Linux atav! **
Jef | http://moinejf.free.fr/

[Hans de Goede]

Hi,

On 27-01-16 17:55, Jean-Francois Moine wrote:
> On Wed, 27 Jan 2016 15:36:21 +0100
> Jens Kuske <jens...@gmail.com> wrote:
>
>> That sounds strange, 4.5-rc1 is working perfectly fine for me too.
>>
>> I doubt the patch you linked is responsible for making it work, it only
>> removes the hardcoded output-names. If your DT isn't messed up this
>> isn't relevant at all since pll8, the initial reason for this patch, is
>> only a dummy clock for now. Are you sure you are using a clean v4.5-rc1
>> without any own modifications?
>
> To be sure, I generated a pure 4.5-rc1 kernel. Same result: no UART.
> Maybe... one more information: I am using Allwinner's u-boot.

Could be that that is the culprit, why are you not using upstream u-boot?
upstream u-boot has H3 and orangepi-pc support now.

Regards,

Hans

[Jean-Francois Moine]

On Wed, 27 Jan 2016 19:16:42 +0100
Hans de Goede <hdeg...@redhat.com> wrote:

> > To be sure, I generated a pure 4.5-rc1 kernel. Same result: no UART.
> > Maybe... one more information: I am using Allwinner's u-boot.
>
> Could be that that is the culprit, why are you not using upstream u-boot?
> upstream u-boot has H3 and orangepi-pc support now.

Yes, but using the upstream u-boot may hide the clock problem.

Otherwise, may the upstream u-boot boot the 3.4 kernel from Allwinner?

[Hans de Goede]

Hi,

On 27-01-16 20:02, Jean-Francois Moine wrote:
> On Wed, 27 Jan 2016 19:16:42 +0100
> Hans de Goede <hdeg...@redhat.com> wrote:
>
>>> To be sure, I generated a pure 4.5-rc1 kernel. Same result: no UART.
>>> Maybe... one more information: I am using Allwinner's u-boot.
>>
>> Could be that that is the culprit, why are you not using upstream u-boot?
>> upstream u-boot has H3 and orangepi-pc support now.
>
> Yes, but using the upstream u-boot may hide the clock problem.

I agree that that could be the case. Would be interesting to figure out
what exactly is needed to get the upstream kernel to work properly
with allwinner's u-boot.

p.s.

Did I understand correctly that you are working on a kms (or fbdev) driver
for the H3? A fellow Dutch hacker (Jelle van der Waa, added to the Cc) is
looking into adding H3 video console support to upstream u-boot. If you
already have a lit-up display with an upstream kernel it would be great
if you could share that, even if it is still wip.

Regards,

Hans

[Jens Kuske]

On 28/01/16 09:15, Hans de Goede wrote:
> Hi,
>
> On 27-01-16 20:02, Jean-Francois Moine wrote:
>> On Wed, 27 Jan 2016 19:16:42 +0100
>> Hans de Goede <hdeg...@redhat.com> wrote:
>>
>>>> To be sure, I generated a pure 4.5-rc1 kernel. Same result: no UART.
>>>> Maybe... one more information: I am using Allwinner's u-boot.
>>>
>>> Could be that that is the culprit, why are you not using upstream u-boot?
>>> upstream u-boot has H3 and orangepi-pc support now.
>>
>> Yes, but using the upstream u-boot may hide the clock problem.
>
> I agree that that could be the case. Would be interesting to figure out
> what exactly is needed to get the upstream kernel to work properly
> with allwinner's u-boot.

Hi,

after figuring out how to boot a devicetree kernel with allwinner's
u-boot I only had to add the mandatory

clock-frequency = <24000000>;
arm,cpu-registers-not-fw-configured;

to the timer node and v4.5-rc1 booted successfully. They are
intentionally left out in the official dt, because documentation says
one should prefer to fix the firmware -> mainline u-boot
No problems with clocks or uart here.

Jens

[Jean-Francois Moine]

Hi Hans and Jelle,

I submitted a (second) patch mid-january:

http://www.spinics.net/lists/dri-devel/msg98558.html

It has changed a bit since this time (towards simplification).
I was to submit a new patch, but the hardware cursor refuses to have
transparent pixels!
Otherwise, the TCON1 and DE2 are simple enough for the primary plane.
The problem is the HDMI driver...

[Jean-Francois Moine]

On Thu, 28 Jan 2016 14:16:26 +0100
Jens Kuske <jens...@gmail.com> wrote:

> after figuring out how to boot a devicetree kernel with allwinner's
> u-boot I only had to add the mandatory
>
> clock-frequency = <24000000>;
> arm,cpu-registers-not-fw-configured;
>
> to the timer node and v4.5-rc1 booted successfully. They are
> intentionally left out in the official dt, because documentation says
> one should prefer to fix the firmware -> mainline u-boot
> No problems with clocks or uart here.

You are right, I had these lines in my DT. Thanks.

But, now, what about the PLL8 (periph1) which should be used by the MMCs?

The A23/A33/H3 (and surely some other SoCs) documentations about
the peripheral/periph/periph0/periph1 PLLs say:

Note: The PLL Output should be fixed to 600MHz, it is not
recommended to vary this value arbitrarily.

(the value is 1.2GHz for the A64)

Could it be safer or simpler to define the frequency of these clocks in
the DT (with their x2/d2)?

[Maxime Ripard]

On Thu, Jan 28, 2016 at 09:15:57AM +0100, Hans de Goede wrote:
> Hi,
>
> On 27-01-16 20:02, Jean-Francois Moine wrote:
> >On Wed, 27 Jan 2016 19:16:42 +0100
> >Hans de Goede <hdeg...@redhat.com> wrote:
> >
> >>>To be sure, I generated a pure 4.5-rc1 kernel. Same result: no UART.
> >>>Maybe... one more information: I am using Allwinner's u-boot.
> >>
> >>Could be that that is the culprit, why are you not using upstream u-boot?
> >>upstream u-boot has H3 and orangepi-pc support now.
> >
> >Yes, but using the upstream u-boot may hide the clock problem.
>
> I agree that that could be the case. Would be interesting to figure out
> what exactly is needed to get the upstream kernel to work properly
> with allwinner's u-boot.

I'm guessing it's probably because of the arch timers that have not
been properly initialized.

Do you have any earlyprintk output?

Maxime

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

[Maxime Ripard]

On Thu, Jan 28, 2016 at 05:59:18PM +0100, Jean-Francois Moine wrote:
> On Thu, 28 Jan 2016 14:16:26 +0100
> Jens Kuske <jens...@gmail.com> wrote:
>
> > after figuring out how to boot a devicetree kernel with allwinner's
> > u-boot I only had to add the mandatory
> >
> > clock-frequency = <24000000>;
> > arm,cpu-registers-not-fw-configured;
> >
> > to the timer node and v4.5-rc1 booted successfully. They are
> > intentionally left out in the official dt, because documentation says
> > one should prefer to fix the firmware -> mainline u-boot
> > No problems with clocks or uart here.
>
> You are right, I had these lines in my DT. Thanks.

And even though you had these lines, it was still not working? Or is
it working now? I'm confused.

> But, now, what about the PLL8 (periph1) which should be used by the MMCs?

I just sent another version of the pll6 rework to enable it.

> The A23/A33/H3 (and surely some other SoCs) documentations about
> the peripheral/periph/periph0/periph1 PLLs say:
>
> Note: The PLL Output should be fixed to 600MHz, it is not
> recommended to vary this value arbitrarily.

I don't know if it's worth it at this point. The pll6 seems to work
fine at other rates. Have you experienced any breakage when running at
another frequency?

Thanks!

[Hans de Goede]

Hi,

On 01/28/2016 08:29 PM, Maxime Ripard wrote:
> On Thu, Jan 28, 2016 at 05:59:18PM +0100, Jean-Francois Moine wrote:

<snip>

>> The A23/A33/H3 (and surely some other SoCs) documentations about
>> the peripheral/periph/periph0/periph1 PLLs say:
>>
>> Note: The PLL Output should be fixed to 600MHz, it is not
>> recommended to vary this value arbitrarily.
>
> I don't know if it's worth it at this point. The pll6 seems to work
> fine at other rates. Have you experienced any breakage when running at
> another frequency?

Hmm, are we actually changing the freq of pll6 on any SoCs? I know we've
the code to it, but given that it is shared between many pheripherals,
I assume we end up never changing it. I assume / hope that the clock
framework protects against reclocking a clock with multiple users ...

Regards,

Hans

[Jean-Francois Moine]

On Thu, 28 Jan 2016 20:29:31 +0100
Maxime Ripard <maxime...@free-electrons.com> wrote:

> > You are right, I had these lines in my DT. Thanks.
>
> And even though you had these lines, it was still not working? Or is
> it working now? I'm confused.

It was not working with these lines because I was using the real pll8,
and, so, Jens' old patch was needed.

> > The A23/A33/H3 (and surely some other SoCs) documentations about
> > the peripheral/periph/periph0/periph1 PLLs say:
> >
> > Note: The PLL Output should be fixed to 600MHz, it is not
> > recommended to vary this value arbitrarily.
>
> I don't know if it's worth it at this point. The pll6 seems to work
> fine at other rates. Have you experienced any breakage when running at
> another frequency?

Each times I started my machine, the clocks (periph0 and 1) were 600MHz.
I think that this value should be OK for all subdevices (with the
kernel 3.4, I have 200MHz for AHB1 and underneath, 600MHz for
deinterlace and CSI, 300MHz and 50MHz for MMC).

[Chen-Yu Tsai]

No we're not. And none of the children of pll6 have CLK_SET_RATE_PARENT
set, so they won't change pll6. I think the point is pll6 itself _can_
be changed, but that would screw up all the peripherals depending on it.

There's also SATA and USB that might be driven by it.

ChenYu

[Maxime Ripard]

Hi,

On Fri, Jan 29, 2016 at 07:25:51AM +0100, Hans de Goede wrote:
> Hi,
>
> On 01/28/2016 08:29 PM, Maxime Ripard wrote:
> >On Thu, Jan 28, 2016 at 05:59:18PM +0100, Jean-Francois Moine wrote:
>
> <snip>
>
> >>The A23/A33/H3 (and surely some other SoCs) documentations about
> >>the peripheral/periph/periph0/periph1 PLLs say:
> >>
> >> Note: The PLL Output should be fixed to 600MHz, it is not
> >> recommended to vary this value arbitrarily.
> >
> >I don't know if it's worth it at this point. The pll6 seems to work
> >fine at other rates. Have you experienced any breakage when running at
> >another frequency?
>
> Hmm, are we actually changing the freq of pll6 on any SoCs? I know we've
> the code to it, but given that it is shared between many pheripherals,
> I assume we end up never changing it.

We don't, but it works. Back when I was debugging the A31 DMA
controller, I tried to do just that and nothing broke (at least as
long as you don't switch halfway through during the boot, but at the
time the clock is registered).

> I assume / hope that the clock framework protects against reclocking
> a clock with multiple users ...

There is, but it's opt-in, and we're not using it yet for anything but
the hstimers (and in that case, we don't prevent the reclocking, we
just take it into account).

[Hans de Goede]

Hi,

Shouldn't we be opt-ing in then ? At least the mmc driver makes
clk_set_rate calls, it would be bad if that somehow ends up changing the
pll6 rate while other peripherals are using it.

Regards,

Hans

[Chen-Yu Tsai]

Hi,

The mod clocks do not have CLK_SET_RATE_PARENT set, which prevents the
CCF from propagating clk_set_rate to its parent. As is for the other
child clocks of PLL6. The only exception is the SATA clock in PLL6.

Regards
ChenYu

[Maxime Ripard]

On Mon, Feb 01, 2016 at 10:37:45PM +0800, Chen-Yu Tsai wrote:
> >> There is, but it's opt-in, and we're not using it yet for anything but
> >> the hstimers (and in that case, we don't prevent the reclocking, we
> >> just take it into account).
> >
> > Shouldn't we be opt-ing in then ? At least the mmc driver makes
> > clk_set_rate calls, it would be bad if that somehow ends up changing the
> > pll6 rate while other peripherals are using it.
>
> The mod clocks do not have CLK_SET_RATE_PARENT set, which prevents the
> CCF from propagating clk_set_rate to its parent. As is for the other
> child clocks of PLL6. The only exception is the SATA clock in PLL6.

The PLL6 you're talking about is not the PLL6 we're talking about ;)

We're talking about the A31's, while the SATA on the A10 / A20 is
driven by the A10's (the equivalent for the A10 would be the PLL5)

[Maxime Ripard]

On Mon, Feb 01, 2016 at 03:26:43PM +0100, Hans de Goede wrote:
> >There is, but it's opt-in, and we're not using it yet for anything but
> >the hstimers (and in that case, we don't prevent the reclocking, we
> >just take it into account).
>
> Shouldn't we be opt-ing in then ? At least the mmc driver makes
> clk_set_rate calls, it would be bad if that somehow ends up changing the
> pll6 rate while other peripherals are using it.

Well, no one will change the pll6 rate at the moment.

Adding code for some case that will not arise doesn't seem very useful