Re: [Qemu-devel] [patch V5 2/5] Implement i.MX31 Clock Control Module

[Peter Maydell]

On 3 April 2012 02:55, Peter Chubb <address@hidden> wrote:
> ===================================================================
> --- qemu-working.orig/hw/imx.h  2012-04-03 11:48:48.088706634 +1000
> +++ qemu-working/hw/imx.h       2012-04-03 11:48:48.776708322 +1000
> @@ -13,4 +13,18 @@
>
>  void imx_serial_create(int uart, const target_phys_addr_t addr, qemu_irq 
> irq);
>
> +typedef enum  {
> +    NOCLK,
> +    MCU,
> +    HSP,
> +    IPG,
> +    CLK_32k
> +} IMXClk;
> +
> +uint32_t imx_timer_frequency(DeviceState *s, IMXClk clock);
> +void imx_timer_create(const char * const name,
> +                      const target_phys_addr_t addr,
> +                      qemu_irq irq,
> +                      DeviceState *ccm);
> +

These function prototypes should be in the other patch, surely?

> +/* PDR0 */
> +#define PDR0_MCU_PODF_SHIFT (0)
> +#define PDR0_MCU_PODF_MASK (0x7)
> +#define PDR0_MAX_PODF_SHIFT (3)
> +#define PDR0_MAX_PODF_MASK (0x7)
> +#define PDR0_IPG_PODF_SHIFT (6)
> +#define PDR0_IPG_PODF_MASK (0x3)
> +#define PDR0_NFC_PODF_SHIFT (8)
> +#define PDR0_NFC_PODF_MASK (7)

Some consistency about whether you're using 7 or 0x7 for
masks would be nice.
> +
> +/*
> + *  Set up the clocks the way U-Boot does.
> + */
> +static void imx_ccm_uboot_reset(IMXCCMState *s)
> +{
> +    s->ccmr = (0x074b0bf5 | CCMR_MPE) & ~CCMR_MDS;
> +    s->pdr0 = INSERT(0xff1, CSI) |
> +        INSERT(7, PER) | INSERT(3, HSP) | INSERT(5, NFC) | INSERT(1, IPG) |
> +        INSERT(3, MAX) | INSERT(0, MCU);
> +    s->mpctl = PLL_PD(0) | PLL_MFD(0xe) | PLL_MFI(9) | PLL_MFN(0xd);
> +}
> +
> +/*
> + * Set up the clocks the way a hardware reset or power-on does
> + */
> +static void imx_ccm_hw_reset(IMXCCMState *s)
> +{
> +    s->ccmr = 0x074b0b7b;
> +    s->pdr0 = 0xff870b48;
> +    s->mpctl = PLL_PD(1) | PLL_MFD(0) | PLL_MFI(6) | PLL_MFN(0);
> +}
> +
> +static void imx_ccm_reset(DeviceState *dev)
> +{
> +    IMXCCMState *s = container_of(dev, IMXCCMState, busdev.qdev);
> +
> +    s->cgr[0] = s->cgr[1] = s->cgr[2] = 0xffffffff;
> +    s->pmcr0 = 0x80209828;
> +    s->pdr1 = 0x49fcfe7f;
> +    s->spctl = PLL_PD(1) | PLL_MFD(4) | PLL_MFI(0xc) | PLL_MFN(1);
> +
> +    /*
> +     * Really should predicate this on arm_boot_info->is_linux
> +     * but I don't know how to do that.
> +     */
> +    if (1) {
> +        imx_ccm_uboot_reset(s);
> +    } else {
> +        imx_ccm_hw_reset(s);
> +    }
> +    update_clocks(s);
> +}

Urgh. Device models need to act like the device. Setting up
devices the way u-boot happens to initialise them should be
done somewhere else, ie driven by our built in bootloader,
if we do it at all.

However, the kernel's Documentation/arm/booting.txt says
nothing about the bootloader having to mess with clocks
so I would rather prefer to hold the line of:
 * fix your kernel not to care
 * or load an actual u-boot binary into emulated RAM
   or emulated flash, and let that do the setup
because I can see board-specific tweaks to the bootloader
getting rapidly out of hand if we're not careful...

-- PMM