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

[Peter Crosthwaite]

On Sat, Sep 12, 2015 at 2:06 PM, Guenter Roeck <address@hidden> 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 for to make Linux happy.
> Limit the change to Linux boots only.
>
> Signed-off-by: Guenter Roeck <address@hidden>
>
> ---
> v2: Limit scope of change to Linux boots.
>
>  hw/misc/zynq_slcr.c | 29 +++++++++++++++++++++++++++--
>  1 file changed, 27 insertions(+), 2 deletions(-)
>
> diff --git a/hw/misc/zynq_slcr.c b/hw/misc/zynq_slcr.c
> index 964f253..ed510fb 100644
> --- a/hw/misc/zynq_slcr.c
> +++ b/hw/misc/zynq_slcr.c
> @@ -14,6 +14,7 @@
>   * with this program; if not, see <http://www.gnu.org/licenses/>.
>   */
>
> +#include "hw/arm/linux-boot-if.h"
>  #include "hw/hw.h"
>  #include "qemu/timer.h"
>  #include "hw/sysbus.h"
> @@ -177,6 +178,8 @@ typedef struct ZynqSLCRState {
>
>      MemoryRegion iomem;
>
> +    bool is_linux;
> +
>      uint32_t regs[ZYNQ_SLCR_NUM_REGS];
>  } ZynqSLCRState;
>
> @@ -189,7 +192,11 @@ static void zynq_slcr_reset(DeviceState *d)
>
>      s->regs[LOCKSTA] = 1;
>      /* 0x100 - 0x11C */
> -    s->regs[ARM_PLL_CTRL]   = 0x0001A008;
> +    if (!s->is_linux) {
> +        s->regs[ARM_PLL_CTRL]   = 0x0001A008;
> +    } else {
> +        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 +205,11 @@ static void zynq_slcr_reset(DeviceState *d)
>      s->regs[IO_PLL_CFG]     = 0x00014000;
>
>      /* 0x120 - 0x16C */
> -    s->regs[ARM_CLK_CTRL]   = 0x1F000400;
> +    if (!s->is_linux) {
> +        s->regs[ARM_CLK_CTRL]   = 0x1F000400;
> +    } else {
> +        s->regs[ARM_CLK_CTRL]   = 0x1F000200;
> +    }

This hunk is not needed. The CLK_CTRL is the property of the Kernel so
cpufreq drivers should be able to set this to whatever it wants.

Your change is actually compensating for an existing bug. Heres the fix:

@@ -393,12 +393,12 @@ static void zynq_slcr_write(void *opaque, hwaddr offset,
         return;
     }

-    if (!s->regs[LOCKSTA]) {
-        s->regs[offset / 4] = val;
-    } else {
-        DB_PRINT("SCLR registers are locked. Unlock them first\n");
+    if (s->regs[LOCKSTA]) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "SCLR registers are locked. Unlock them first\n");
         return;
     }
+    s->regs[offset] = val;

There's a little bit of extra cleanup there while I was touching the
code, but the issue is that /4 in original code is bogus. This means
SLCR writes have not worked for sometime (doh). I'll spin this as a
patch soon.

I have replicated this (as well as the ADC hang) and am working on a
machine-level firmwareish version of your PLL change. I'll keep
everyone posted.

Regards,
Peter

>      s->regs[DDR_CLK_CTRL]   = 0x18400003;
>      s->regs[DCI_CLK_CTRL]   = 0x01E03201;
>      s->regs[APER_CLK_CTRL]  = 0x01FFCCCD;
> @@ -429,17 +440,27 @@ static const VMStateDescription vmstate_zynq_slcr = {
>      .version_id = 2,
>      .minimum_version_id = 2,
>      .fields = (VMStateField[]) {
> +        VMSTATE_BOOL(is_linux, ZynqSLCRState),
>          VMSTATE_UINT32_ARRAY(regs, ZynqSLCRState, ZYNQ_SLCR_NUM_REGS),
>          VMSTATE_END_OF_LIST()
>      }
>  };
>
> +static void zynq_sclr_linux_init(ARMLinuxBootIf *obj, bool secure_boot)
> +{
> +    ZynqSLCRState *s = ZYNQ_SLCR(obj);
> +
> +    s->is_linux = true;
> +}
> +
>  static void zynq_slcr_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> +    ARMLinuxBootIfClass *albifc = ARM_LINUX_BOOT_IF_CLASS(klass);
>
>      dc->vmsd = &vmstate_zynq_slcr;
>      dc->reset = zynq_slcr_reset;
> +    albifc->arm_linux_init = zynq_sclr_linux_init;
>  }
>
>  static const TypeInfo zynq_slcr_info = {
> @@ -448,6 +469,10 @@ static const TypeInfo zynq_slcr_info = {
>      .parent = TYPE_SYS_BUS_DEVICE,
>      .instance_size  = sizeof(ZynqSLCRState),
>      .instance_init = zynq_slcr_init,
> +    .interfaces = (InterfaceInfo []) {
> +        { TYPE_ARM_LINUX_BOOT_IF },
> +        { },
> +    },
>  };
>
>  static void zynq_slcr_register_types(void)
> --
> 2.1.4
>