[Qemu-devel] [PATCH] hw/misc/zynq_slcr: Change CPU clock rate

[Qemu-devel] [PATCH] hw/misc/zynq_slcr: Change CPU clock rate

[Guenter Roeck]

The Linux kernel only accepts 333334 Khz and 666667 Khz clock rates, and
may crash if the actual clock rate is too low. The clock rate used to be
(ps-clk-frequency * 26 / 4), which resulted in a CPU frequency of
216666 Khz if ps-clk-frequency was set to 33333333 Hz. Change it to
(ps-clk-frequency * 20 / 2) = 333333 Khz to make Linux happy.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 hw/misc/zynq_slcr.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/misc/zynq_slcr.c b/hw/misc/zynq_slcr.c
index 964f253..d3e1ce0 100644
--- a/hw/misc/zynq_slcr.c
+++ b/hw/misc/zynq_slcr.c
@@ -189,7 +189,7 @@ static void zynq_slcr_reset(DeviceState *d)
 
     s->regs[LOCKSTA] = 1;
     /* 0x100 - 0x11C */
-    s->regs[ARM_PLL_CTRL]   = 0x0001A008;
+    s->regs[ARM_PLL_CTRL]   = 0x00014008;
     s->regs[DDR_PLL_CTRL]   = 0x0001A008;
     s->regs[IO_PLL_CTRL]    = 0x0001A008;
     s->regs[PLL_STATUS]     = 0x0000003F;
@@ -198,7 +198,7 @@ static void zynq_slcr_reset(DeviceState *d)
     s->regs[IO_PLL_CFG]     = 0x00014000;
 
     /* 0x120 - 0x16C */
-    s->regs[ARM_CLK_CTRL]   = 0x1F000400;
+    s->regs[ARM_CLK_CTRL]   = 0x1F000200;
     s->regs[DDR_CLK_CTRL]   = 0x18400003;
     s->regs[DCI_CLK_CTRL]   = 0x01E03201;
     s->regs[APER_CLK_CTRL]  = 0x01FFCCCD;
-- 
2.1.4

Re: [Qemu-devel] [PATCH] hw/misc/zynq_slcr: Change CPU clock rate

[Peter Crosthwaite]

On Wed, Aug 12, 2015 at 7:24 AM, Guenter Roeck <linux@roeck-us.net> wrote:
> The Linux kernel only accepts 333334 Khz and 666667 Khz clock rates, and
> may crash if the actual clock rate is too low. The clock rate used to be
> (ps-clk-frequency * 26 / 4), which resulted in a CPU frequency of
> 216666 Khz if ps-clk-frequency was set to 33333333 Hz. Change it to
> (ps-clk-frequency * 20 / 2) = 333333 Khz to make Linux happy.
>

This is a long known problem that is solved via patch in the yocto
meta-layer via a similar hack to slcr.

Ideally SLCR should follow the TRM reset value by default and this is
really setting up post bootloader state.

But PMM recently upstreamed the needed core patches to have a device
register Linux specific setup:

Search for:

"hw/arm: new interface for devices which need to behave differently
for kernel boot"

and following patches that show how we can achieve best of both words,
with Linux specific device setup as well keep the ability to boot raw
(for testing firmware such as FSBL itself).

Regards,
Peter

P.S. Sorry about the delay (and to PMM on missed review of that
series). I dropped off the radar for a month or so July->Aug.

> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
>  hw/misc/zynq_slcr.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/misc/zynq_slcr.c b/hw/misc/zynq_slcr.c
> index 964f253..d3e1ce0 100644
> --- a/hw/misc/zynq_slcr.c
> +++ b/hw/misc/zynq_slcr.c
> @@ -189,7 +189,7 @@ static void zynq_slcr_reset(DeviceState *d)
>
>      s->regs[LOCKSTA] = 1;
>      /* 0x100 - 0x11C */
> -    s->regs[ARM_PLL_CTRL]   = 0x0001A008;
> +    s->regs[ARM_PLL_CTRL]   = 0x00014008;
>      s->regs[DDR_PLL_CTRL]   = 0x0001A008;
>      s->regs[IO_PLL_CTRL]    = 0x0001A008;
>      s->regs[PLL_STATUS]     = 0x0000003F;
> @@ -198,7 +198,7 @@ static void zynq_slcr_reset(DeviceState *d)
>      s->regs[IO_PLL_CFG]     = 0x00014000;
>
>      /* 0x120 - 0x16C */
> -    s->regs[ARM_CLK_CTRL]   = 0x1F000400;
> +    s->regs[ARM_CLK_CTRL]   = 0x1F000200;
>      s->regs[DDR_CLK_CTRL]   = 0x18400003;
>      s->regs[DCI_CLK_CTRL]   = 0x01E03201;
>      s->regs[APER_CLK_CTRL]  = 0x01FFCCCD;
> --
> 2.1.4
>
>

Re: [Qemu-devel] [PATCH] hw/misc/zynq_slcr: Change CPU clock rate

[Guenter Roeck]

Hi Peter,

On Thu, Sep 10, 2015 at 04:26:13PM -0700, Peter Crosthwaite wrote:
> On Wed, Aug 12, 2015 at 7:24 AM, Guenter Roeck <linux@roeck-us.net> wrote:
> > The Linux kernel only accepts 333334 Khz and 666667 Khz clock rates, and
> > may crash if the actual clock rate is too low. The clock rate used to be
> > (ps-clk-frequency * 26 / 4), which resulted in a CPU frequency of
> > 216666 Khz if ps-clk-frequency was set to 33333333 Hz. Change it to
> > (ps-clk-frequency * 20 / 2) = 333333 Khz to make Linux happy.
> >
> 
> This is a long known problem that is solved via patch in the yocto
> meta-layer via a similar hack to slcr.
> 
Doesn't sound like a solution to me :-(.

> Ideally SLCR should follow the TRM reset value by default and this is
> really setting up post bootloader state.
> 
Which sclr ? The Linux driver ?

> But PMM recently upstreamed the needed core patches to have a device
> register Linux specific setup:
> 
> Search for:
> 
> "hw/arm: new interface for devices which need to behave differently
> for kernel boot"
> 
> and following patches that show how we can achieve best of both words,
> with Linux specific device setup as well keep the ability to boot raw
> (for testing firmware such as FSBL itself).
> 
So what would be the correct solution here ? Implement the arm_linux_init
callback for this driver, which would set the register values below, or
some other acceptable register values ?

Or something else ?

> Regards,
> Peter
> 
> P.S. Sorry about the delay (and to PMM on missed review of that
> series). I dropped off the radar for a month or so July->Aug.
> 
No worries.

Thanks,
Guenter

> > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > ---
> >  hw/misc/zynq_slcr.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/misc/zynq_slcr.c b/hw/misc/zynq_slcr.c
> > index 964f253..d3e1ce0 100644
> > --- a/hw/misc/zynq_slcr.c
> > +++ b/hw/misc/zynq_slcr.c
> > @@ -189,7 +189,7 @@ static void zynq_slcr_reset(DeviceState *d)
> >
> >      s->regs[LOCKSTA] = 1;
> >      /* 0x100 - 0x11C */
> > -    s->regs[ARM_PLL_CTRL]   = 0x0001A008;
> > +    s->regs[ARM_PLL_CTRL]   = 0x00014008;
> >      s->regs[DDR_PLL_CTRL]   = 0x0001A008;
> >      s->regs[IO_PLL_CTRL]    = 0x0001A008;
> >      s->regs[PLL_STATUS]     = 0x0000003F;
> > @@ -198,7 +198,7 @@ static void zynq_slcr_reset(DeviceState *d)
> >      s->regs[IO_PLL_CFG]     = 0x00014000;
> >
> >      /* 0x120 - 0x16C */
> > -    s->regs[ARM_CLK_CTRL]   = 0x1F000400;
> > +    s->regs[ARM_CLK_CTRL]   = 0x1F000200;
> >      s->regs[DDR_CLK_CTRL]   = 0x18400003;
> >      s->regs[DCI_CLK_CTRL]   = 0x01E03201;
> >      s->regs[APER_CLK_CTRL]  = 0x01FFCCCD;
> > --
> > 2.1.4
> >
> >

Re: [Qemu-devel] [PATCH] hw/misc/zynq_slcr: Change CPU clock rate

[Peter Crosthwaite]

On Thu, Sep 10, 2015 at 6:57 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> Hi Peter,
>
> On Thu, Sep 10, 2015 at 04:26:13PM -0700, Peter Crosthwaite wrote:
>> On Wed, Aug 12, 2015 at 7:24 AM, Guenter Roeck <linux@roeck-us.net> wrote:
>> > The Linux kernel only accepts 333334 Khz and 666667 Khz clock rates, and
>> > may crash if the actual clock rate is too low. The clock rate used to be
>> > (ps-clk-frequency * 26 / 4), which resulted in a CPU frequency of
>> > 216666 Khz if ps-clk-frequency was set to 33333333 Hz. Change it to
>> > (ps-clk-frequency * 20 / 2) = 333333 Khz to make Linux happy.
>> >
>>
>> This is a long known problem that is solved via patch in the yocto
>> meta-layer via a similar hack to slcr.
>>
> Doesn't sound like a solution to me :-(.
>

It's not :)

>> Ideally SLCR should follow the TRM reset value by default and this is
>> really setting up post bootloader state.
>>
> Which sclr ? The Linux driver ?
>

No I mean this QEMU device model. It would be nice if the Kernel was
able to deal with absent clocking (perhaps configured via DTS?) but in
reality I think we have to work around this from the QEMU side. We
just need to make your changes in this patch Linux specific rather
than absolute.

>> But PMM recently upstreamed the needed core patches to have a device
>> register Linux specific setup:
>>
>> Search for:
>>
>> "hw/arm: new interface for devices which need to behave differently
>> for kernel boot"
>>
>> and following patches that show how we can achieve best of both words,
>> with Linux specific device setup as well keep the ability to boot raw
>> (for testing firmware such as FSBL itself).
>>
> So what would be the correct solution here ? Implement the arm_linux_init
> callback for this driver, which would set the register values below, or
> some other acceptable register values ?
>

Yes I think so.

Regards,
Peter

> Or something else ?
>
>> Regards,
>> Peter
>>
>> P.S. Sorry about the delay (and to PMM on missed review of that
>> series). I dropped off the radar for a month or so July->Aug.
>>
> No worries.
>
> Thanks,
> Guenter
>
>> > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> > ---
>> >  hw/misc/zynq_slcr.c | 4 ++--
>> >  1 file changed, 2 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/hw/misc/zynq_slcr.c b/hw/misc/zynq_slcr.c
>> > index 964f253..d3e1ce0 100644
>> > --- a/hw/misc/zynq_slcr.c
>> > +++ b/hw/misc/zynq_slcr.c
>> > @@ -189,7 +189,7 @@ static void zynq_slcr_reset(DeviceState *d)
>> >
>> >      s->regs[LOCKSTA] = 1;
>> >      /* 0x100 - 0x11C */
>> > -    s->regs[ARM_PLL_CTRL]   = 0x0001A008;
>> > +    s->regs[ARM_PLL_CTRL]   = 0x00014008;
>> >      s->regs[DDR_PLL_CTRL]   = 0x0001A008;
>> >      s->regs[IO_PLL_CTRL]    = 0x0001A008;
>> >      s->regs[PLL_STATUS]     = 0x0000003F;
>> > @@ -198,7 +198,7 @@ static void zynq_slcr_reset(DeviceState *d)
>> >      s->regs[IO_PLL_CFG]     = 0x00014000;
>> >
>> >      /* 0x120 - 0x16C */
>> > -    s->regs[ARM_CLK_CTRL]   = 0x1F000400;
>> > +    s->regs[ARM_CLK_CTRL]   = 0x1F000200;
>> >      s->regs[DDR_CLK_CTRL]   = 0x18400003;
>> >      s->regs[DCI_CLK_CTRL]   = 0x01E03201;
>> >      s->regs[APER_CLK_CTRL]  = 0x01FFCCCD;
>> > --
>> > 2.1.4
>> >
>> >