[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 3/6] i.MX: Add i.MX6 CCM and ANALOG device.
From: |
Peter Maydell |
Subject: |
Re: [Qemu-devel] [PATCH 3/6] i.MX: Add i.MX6 CCM and ANALOG device. |
Date: |
Tue, 2 Feb 2016 16:41:54 +0000 |
On 26 January 2016 at 21:45, Jean-Christophe Dubois <address@hidden> wrote:
> Signed-off-by: Jean-Christophe Dubois <address@hidden>
> +static uint32_t imx6_analog_get_pll2_clk(IMX6CCMState *dev)
> +{
> + uint32_t freq = 24000000;
> +
> + if (EXTRACT(dev->analog[CCM_ANALOG_PLL_SYS], DIV_SELECT)) {
> + freq *= 22;
> + } else {
> + freq *= 20;
> + }
> +
> + DPRINTF("freq = %d\n", freq);
> +
> + return freq;
> +}
> +
> +static uint32_t imx6_analog_get_pll2_pfd0_clk(IMX6CCMState *dev)
> +{
> + uint32_t freq = 0;
> +
> + freq = imx6_analog_get_pll2_clk(dev) * 18
> + / EXTRACT(dev->analog[CCM_ANALOG_PFD_528], PFD0_FRAC);
I think this multiplication can overflow. Are you sure you want
to do it at 32 bits?
(24000000 == 0x16E3600. 24000000 * 22 * 18 == 0x2367B8800.)
> +
> + DPRINTF("freq = %d\n", freq);
> +
> + return freq;
> +}
> +
> +static uint32_t imx6_analog_get_pll2_pfd2_clk(IMX6CCMState *dev)
> +{
> + uint32_t freq = 0;
> +
> + freq = imx6_analog_get_pll2_clk(dev) * 18
> + / EXTRACT(dev->analog[CCM_ANALOG_PFD_528], PFD2_FRAC);
Ditto.
> +
> + DPRINTF("freq = %d\n", freq);
All these debug printfs are identical, which seems to me like
it would make them not terribly useful, but I'm not the one that
would be debugging this :-)
> +
> + return freq;
> +}
> +
> +static uint32_t imx6_analog_get_periph_clk(IMX6CCMState *dev)
> +{
> + uint32_t freq = 0;
> +
> + switch (EXTRACT(dev->ccm[CCM_CBCMR], PRE_PERIPH_CLK_SEL)) {
> + case 0:
> + freq = imx6_analog_get_pll2_clk(dev);
> + break;
> + case 1:
> + freq = imx6_analog_get_pll2_pfd2_clk(dev);
> + break;
> + case 2:
> + freq = imx6_analog_get_pll2_pfd0_clk(dev);
> + break;
> + case 3:
> + freq = imx6_analog_get_pll2_pfd2_clk(dev) / 2;
> + break;
> + default:
> + /* We should never get there */
> + qemu_log_mask(LOG_GUEST_ERROR, "[%s]%s: unexpected clock selector\n",
> + TYPE_IMX6_CCM, __func__);
PRE_PERIPH_CLK_SEL is a two bit field, right? So this is actually
unreachable, not just "only reachable if guest misprograms a register".
That should be g_assert_not_reached().
> + break;
> + }
> +
> + DPRINTF("freq = %d\n", freq);
> +
> + return freq;
> +}
> +
> +static uint32_t imx6_ccm_get_ahb_clk(IMX6CCMState *dev)
> +{
> + uint32_t freq = 0;
> +
> + freq = imx6_analog_get_periph_clk(dev)
> + / (1 + EXTRACT(dev->ccm[CCM_CBCDR], AHB_PODF));;
Stray extra ';'.
> +
> + DPRINTF("freq = %d\n", freq);
> +
> + return freq;
> +}
> +
> +static uint32_t imx6_ccm_get_ipg_clk(IMX6CCMState *dev)
> +{
> + uint32_t freq = 0;
> +
> + freq = imx6_ccm_get_ahb_clk(dev)
> + / (1 + EXTRACT(dev->ccm[CCM_CBCDR], IPG_PODF));;
> +
> + DPRINTF("freq = %d\n", freq);
> +
> + return freq;
> +}
> +
> +static uint32_t imx6_ccm_get_per_clk(IMX6CCMState *dev)
> +{
> + uint32_t freq = 0;
> +
> + freq = imx6_ccm_get_ipg_clk(dev)
> + / (1 + EXTRACT(dev->ccm[CCM_CSCMR1], PERCLK_PODF));
> +
> + DPRINTF("freq = %d\n", freq);
> +
> + return freq;
> +}
> +
> +static uint32_t imx6_ccm_get_clock_frequency(IMXCCMState *dev, IMXClk clock)
> +{
> + uint32_t freq = 0;
> + IMX6CCMState *s = IMX6_CCM(dev);
> +
> + switch (clock) {
> + case CLK_NONE:
> + break;
> + case CLK_IPG:
> + freq = imx6_ccm_get_ipg_clk(s);
> + break;
> + case CLK_IPG_HIGH:
> + freq = imx6_ccm_get_per_clk(s);
> + break;
> + case CLK_32k:
> + freq = CKIL_FREQ;
> + break;
> + default:
> + qemu_log_mask(LOG_GUEST_ERROR, "[%s]%s: unsupported clock %d\n",
> + TYPE_IMX6_CCM, __func__, clock);
> + break;
> + }
> +
> + DPRINTF("Clock = %d) = %d\n", clock, freq);
> +
> + return freq;
> +}
> +
> +static void imx6_ccm_reset(DeviceState *dev)
> +{
> + IMX6CCMState *s = IMX6_CCM(dev);
> +
> + DPRINTF("\n");
What's this for?
> +
> + s->ccm[CCM_CCR] = 0x040116FF;
> + s->ccm[CCM_CCDR] = 0x00000000;
> + s->ccm[CCM_CSR] = 0x00000010;
> + s->ccm[CCM_CCSR] = 0x00000100;
> + s->ccm[CCM_CACRR] = 0x00000000;
> + s->ccm[CCM_CBCDR] = 0x00018D40;
> + s->ccm[CCM_CBCMR] = 0x00022324;
> + s->ccm[CCM_CSCMR1] = 0x00F00000;
> + s->ccm[CCM_CSCMR2] = 0x02B92F06;
> + s->ccm[CCM_CSCDR1] = 0x00490B00;
> + s->ccm[CCM_CS1CDR] = 0x0EC102C1;
> + s->ccm[CCM_CS2CDR] = 0x000736C1;
> + s->ccm[CCM_CDCDR] = 0x33F71F92;
> + s->ccm[CCM_CHSCCDR] = 0x0002A150;
> + s->ccm[CCM_CSCDR2] = 0x0002A150;
> + s->ccm[CCM_CSCDR3] = 0x00014841;
> + s->ccm[CCM_CDHIPR] = 0x00000000;
> + s->ccm[CCM_CTOR] = 0x00000000;
> + s->ccm[CCM_CLPCR] = 0x00000079;
> + s->ccm[CCM_CISR] = 0x00000000;
> + s->ccm[CCM_CIMR] = 0xFFFFFFFF;
> + s->ccm[CCM_CCOSR] = 0x000A0001;
> + s->ccm[CCM_CGPR] = 0x0000FE62;
> + s->ccm[CCM_CCGR0] = 0xFFFFFFFF;
> + s->ccm[CCM_CCGR1] = 0xFFFFFFFF;
> + s->ccm[CCM_CCGR2] = 0xFC3FFFFF;
> + s->ccm[CCM_CCGR3] = 0xFFFFFFFF;
> + s->ccm[CCM_CCGR4] = 0xFFFFFFFF;
> + s->ccm[CCM_CCGR5] = 0xFFFFFFFF;
> + s->ccm[CCM_CCGR6] = 0xFFFFFFFF;
> + s->ccm[CCM_CMEOR] = 0xFFFFFFFF;
> +
> + s->analog[CCM_ANALOG_PLL_ARM] = 0x00013042;
> + s->analog[CCM_ANALOG_PLL_USB1] = 0x00012000;
> + s->analog[CCM_ANALOG_PLL_USB2] = 0x00012000;
> + s->analog[CCM_ANALOG_PLL_SYS] = 0x00013001;
> + s->analog[CCM_ANALOG_PLL_SYS_SS] = 0x00000000;
> + s->analog[CCM_ANALOG_PLL_SYS_NUM] = 0x00000000;
> + s->analog[CCM_ANALOG_PLL_SYS_DENOM] = 0x00000012;
> + s->analog[CCM_ANALOG_PLL_AUDIO] = 0x00011006;
> + s->analog[CCM_ANALOG_PLL_AUDIO_NUM] = 0x05F5E100;
> + s->analog[CCM_ANALOG_PLL_AUDIO_DENOM] = 0x2964619C;
> + s->analog[CCM_ANALOG_PLL_VIDEO] = 0x0001100C;
> + s->analog[CCM_ANALOG_PLL_VIDEO_NUM] = 0x05F5E100;
> + s->analog[CCM_ANALOG_PLL_VIDEO_DENOM] = 0x10A24447;
> + s->analog[CCM_ANALOG_PLL_MLB] = 0x00010000;
> + s->analog[CCM_ANALOG_PLL_ENET] = 0x00011001;
> + s->analog[CCM_ANALOG_PFD_480] = 0x1311100C;
> + s->analog[CCM_ANALOG_PFD_528] = 0x1018101B;
> + /*
> + * This value is overrided by the PMU value
> + s->analog[CCM_ANALOG_MISC0] = 0x02000000;
> + */
"overridden", but please don't leave code in but commented out
like this.
> + s->analog[CCM_ANALOG_MISC2] = 0x00272727;
> +
> + s->analog[PMU_REG_1P1] = 0x00001073;
> + s->analog[PMU_REG_3P0] = 0x00000F74;
> + s->analog[PMU_REG_2P5] = 0x00005071;
> + s->analog[PMU_REG_CORE] = 0x00402010;
> + s->analog[PMU_MISC0] = 0x04000000;
> + s->analog[PMU_MISC1] = 0x00000000;
> + /*
> + * Same value as CCM_ANALOG_MISC2
> + s->analog[PMU_MISC2] = 0x00272727;
> + */
Ditto.
> +
> + s->analog[USB_ANALOG_DIGPROG] = 0x00000000;
> +
> + /* all PLL need to be locked */
"PLLs"
> + s->analog[CCM_ANALOG_PLL_ARM] |= CCM_ANALOG_PLL_LOCK;
> + s->analog[CCM_ANALOG_PLL_USB1] |= CCM_ANALOG_PLL_LOCK;
> + s->analog[CCM_ANALOG_PLL_USB2] |= CCM_ANALOG_PLL_LOCK;
> + s->analog[CCM_ANALOG_PLL_SYS] |= CCM_ANALOG_PLL_LOCK;
> + s->analog[CCM_ANALOG_PLL_AUDIO] |= CCM_ANALOG_PLL_LOCK;
> + s->analog[CCM_ANALOG_PLL_VIDEO] |= CCM_ANALOG_PLL_LOCK;
> + s->analog[CCM_ANALOG_PLL_MLB] |= CCM_ANALOG_PLL_LOCK;
> + s->analog[CCM_ANALOG_PLL_ENET] |= CCM_ANALOG_PLL_LOCK;
> +}
> +
> +static uint64_t imx6_ccm_read(void *opaque, uint32 index, unsigned size)
These are only called from within this driver code, so there's
no need to leave the first argument as a void*, you can make
it an IMX6CCMState *.
> +{
> + uint32 value = 0;
> + IMX6CCMState *s = (IMX6CCMState *)opaque;
> +
> + value = s->ccm[index];
> +
> + DPRINTF("reg[%s] => 0x%" PRIx32 "\n", imx6_ccm_reg_name(index), value);
> +
> + return (uint64_t)value;
> +}
> +
> +static void imx6_ccm_write(void *opaque, uint32 index, uint64_t value,
> + unsigned size)
> +{
> + IMX6CCMState *s = (IMX6CCMState *)opaque;
> +
> + DPRINTF("reg[%s] <= 0x%" PRIx32 "\n", imx6_ccm_reg_name(index),
> + (uint32_t)value);
> +
> + /*
> + * We will do a better implementation later. In particular some bits
> + * cannot be writen to.
"written"
> + */
> + s->ccm[index] = value;
> +}
> +
> +static uint64_t imx6_analog_read(void *opaque, uint32_t index, unsigned size)
> +{
> + uint32_t value;
> + IMX6CCMState *s = (IMX6CCMState *)opaque;
> +
> + switch (index) {
> + case CCM_ANALOG_PLL_ARM_SET:
> + case CCM_ANALOG_PLL_USB1_SET:
> + case CCM_ANALOG_PLL_USB2_SET:
> + case CCM_ANALOG_PLL_SYS_SET:
> + case CCM_ANALOG_PLL_AUDIO_SET:
> + case CCM_ANALOG_PLL_VIDEO_SET:
> + case CCM_ANALOG_PLL_MLB_SET:
> + case CCM_ANALOG_PLL_ENET_SET:
> + case CCM_ANALOG_PFD_480_SET:
> + case CCM_ANALOG_PFD_528_SET:
> + case CCM_ANALOG_MISC0_SET:
> + case PMU_MISC1_SET:
> + case CCM_ANALOG_MISC2_SET:
> + case USB_ANALOG_USB1_VBUS_DETECT_SET:
> + case USB_ANALOG_USB1_CHRG_DETECT_SET:
> + case USB_ANALOG_USB1_MISC_SET:
> + case USB_ANALOG_USB2_VBUS_DETECT_SET:
> + case USB_ANALOG_USB2_CHRG_DETECT_SET:
> + case USB_ANALOG_USB2_MISC_SET:
> + value = s->analog[index - 1];
I don't understand why these are referencing index -1 and index - 2;
could we have some explanation, please?
> + break;
> + case CCM_ANALOG_PLL_ARM_CLR:
> + case CCM_ANALOG_PLL_USB1_CLR:
> + case CCM_ANALOG_PLL_USB2_CLR:
> + case CCM_ANALOG_PLL_SYS_CLR:
> + case CCM_ANALOG_PLL_AUDIO_CLR:
> + case CCM_ANALOG_PLL_VIDEO_CLR:
> + case CCM_ANALOG_PLL_MLB_CLR:
> + case CCM_ANALOG_PLL_ENET_CLR:
> + case CCM_ANALOG_PFD_480_CLR:
> + case CCM_ANALOG_PFD_528_CLR:
> + case CCM_ANALOG_MISC0_CLR:
> + case PMU_MISC1_CLR:
> + case CCM_ANALOG_MISC2_CLR:
> + case USB_ANALOG_USB1_VBUS_DETECT_CLR:
> + case USB_ANALOG_USB1_CHRG_DETECT_CLR:
> + case USB_ANALOG_USB1_MISC_CLR:
> + case USB_ANALOG_USB2_VBUS_DETECT_CLR:
> + case USB_ANALOG_USB2_CHRG_DETECT_CLR:
> + case USB_ANALOG_USB2_MISC_CLR:
> + value = s->analog[index - 2];
> + break;
> + case CCM_ANALOG_PLL_ARM_TOG:
> + case CCM_ANALOG_PLL_USB1_TOG:
> + case CCM_ANALOG_PLL_USB2_TOG:
> + case CCM_ANALOG_PLL_SYS_TOG:
> + case CCM_ANALOG_PLL_AUDIO_TOG:
> + case CCM_ANALOG_PLL_VIDEO_TOG:
> + case CCM_ANALOG_PLL_MLB_TOG:
> + case CCM_ANALOG_PLL_ENET_TOG:
> + case CCM_ANALOG_PFD_480_TOG:
> + case CCM_ANALOG_PFD_528_TOG:
> + case CCM_ANALOG_MISC0_TOG:
> + case PMU_MISC1_TOG:
> + case CCM_ANALOG_MISC2_TOG:
> + case USB_ANALOG_USB1_VBUS_DETECT_TOG:
> + case USB_ANALOG_USB1_CHRG_DETECT_TOG:
> + case USB_ANALOG_USB1_MISC_TOG:
> + case USB_ANALOG_USB2_VBUS_DETECT_TOG:
> + case USB_ANALOG_USB2_CHRG_DETECT_TOG:
> + case USB_ANALOG_USB2_MISC_TOG:
> + value = s->analog[index - 3];
> + break;
> + default:
> + value = s->analog[index];
> + break;
> + }
> +
> + DPRINTF("reg[%s] => 0x%" PRIx32 "\n", imx6_analog_reg_name(index),
> value);
> +
> + return (uint64_t)value;
> +}
> +
> +static void imx6_analog_write(void *opaque, uint32_t index, uint64_t value,
> + unsigned size)
> +{
> + IMX6CCMState *s = (IMX6CCMState *)opaque;
> +
> + DPRINTF("reg[%s] <= 0x%" PRIx32 "\n", imx6_analog_reg_name(index),
> + (uint32_t)value);
> +
> + switch (index) {
> + case CCM_ANALOG_PLL_ARM_SET:
> + case CCM_ANALOG_PLL_USB1_SET:
> + case CCM_ANALOG_PLL_USB2_SET:
> + case CCM_ANALOG_PLL_SYS_SET:
> + case CCM_ANALOG_PLL_AUDIO_SET:
> + case CCM_ANALOG_PLL_VIDEO_SET:
> + case CCM_ANALOG_PLL_MLB_SET:
> + case CCM_ANALOG_PLL_ENET_SET:
> + case CCM_ANALOG_PFD_480_SET:
> + case CCM_ANALOG_PFD_528_SET:
> + case CCM_ANALOG_MISC0_SET:
> + case PMU_MISC1_SET:
> + case CCM_ANALOG_MISC2_SET:
> + case USB_ANALOG_USB1_VBUS_DETECT_SET:
> + case USB_ANALOG_USB1_CHRG_DETECT_SET:
> + case USB_ANALOG_USB1_MISC_SET:
> + case USB_ANALOG_USB2_VBUS_DETECT_SET:
> + case USB_ANALOG_USB2_CHRG_DETECT_SET:
> + case USB_ANALOG_USB2_MISC_SET:
> + /* Set bits in ref register */
> + s->analog[index - 1] |= value;
> + break;
> + case CCM_ANALOG_PLL_ARM_CLR:
> + case CCM_ANALOG_PLL_USB1_CLR:
> + case CCM_ANALOG_PLL_USB2_CLR:
> + case CCM_ANALOG_PLL_SYS_CLR:
> + case CCM_ANALOG_PLL_AUDIO_CLR:
> + case CCM_ANALOG_PLL_VIDEO_CLR:
> + case CCM_ANALOG_PLL_MLB_CLR:
> + case CCM_ANALOG_PLL_ENET_CLR:
> + case CCM_ANALOG_PFD_480_CLR:
> + case CCM_ANALOG_PFD_528_CLR:
> + case CCM_ANALOG_MISC0_CLR:
> + case PMU_MISC1_CLR:
> + case CCM_ANALOG_MISC2_CLR:
> + case USB_ANALOG_USB1_VBUS_DETECT_CLR:
> + case USB_ANALOG_USB1_CHRG_DETECT_CLR:
> + case USB_ANALOG_USB1_MISC_CLR:
> + case USB_ANALOG_USB2_VBUS_DETECT_CLR:
> + case USB_ANALOG_USB2_CHRG_DETECT_CLR:
> + case USB_ANALOG_USB2_MISC_CLR:
> + /* clear bits in ref register */
> + s->analog[index - 2] &= ~value;
> + break;
> + case CCM_ANALOG_PLL_ARM_TOG:
> + case CCM_ANALOG_PLL_USB1_TOG:
> + case CCM_ANALOG_PLL_USB2_TOG:
> + case CCM_ANALOG_PLL_SYS_TOG:
> + case CCM_ANALOG_PLL_AUDIO_TOG:
> + case CCM_ANALOG_PLL_VIDEO_TOG:
> + case CCM_ANALOG_PLL_MLB_TOG:
> + case CCM_ANALOG_PLL_ENET_TOG:
> + case CCM_ANALOG_PFD_480_TOG:
> + case CCM_ANALOG_PFD_528_TOG:
> + case CCM_ANALOG_MISC0_TOG:
> + case PMU_MISC1_TOG:
> + case CCM_ANALOG_MISC2_TOG:
> + case USB_ANALOG_USB1_VBUS_DETECT_TOG:
> + case USB_ANALOG_USB1_CHRG_DETECT_TOG:
> + case USB_ANALOG_USB1_MISC_TOG:
> + case USB_ANALOG_USB2_VBUS_DETECT_TOG:
> + case USB_ANALOG_USB2_CHRG_DETECT_TOG:
> + case USB_ANALOG_USB2_MISC_TOG:
> + /* toggle bits in ref register */
> + s->analog[index - 3] ^= value;
> + break;
> + default:
> + /*
> + * We will do a better implementation later. In particular some bits
> + * cannot be writen to.
> + */
> + s->analog[index] = value;
> + break;
> + }
> +}
> +
> +static uint64_t imx6_main_read(void *opaque, hwaddr offset, unsigned size)
> +{
> + uint64_t value = 0;
> + uint32_t index = offset >> 2;
> +
> + if (offset > (0x4000 + CCM_ANALOG_MAX * sizeof(uint32_t))) {
> + /* This is an unsupported area */
> + qemu_log_mask(LOG_GUEST_ERROR, "[%s]%s: Bad register at offset 0x%"
> + HWADDR_PRIx "\n", TYPE_IMX6_CCM, __func__, offset);
> + DPRINTF("Bad register at offset 0x%" HWADDR_PRIx "\n", offset);
> + } else if (offset >= 0x4000) {
> + /* This is an analog register */
> + value = imx6_analog_read(opaque, index - 0x1000, size);
> + } else if (offset < (CCM_MAX * sizeof(uint32_t))) {
> + /* This is a ccm register */
> + value = imx6_ccm_read(opaque, index, size);
Consider doing this with a set of subregions inside a container,
rather than an if-elseif chain.
> + } else {
> + /* This is an unsupported area */
> + qemu_log_mask(LOG_GUEST_ERROR, "[%s]%s: Bad register at offset 0x%"
> + HWADDR_PRIx "\n", TYPE_IMX6_CCM, __func__, offset);
> + DPRINTF("Bad register at offset 0x%" HWADDR_PRIx "\n", offset);
You can reshuffle the if..elseif chain to not have this and the first
if be duplicated code.
> + }
> +
> + return value;
> +}
> +#define EXTRACT(value, name) (((value) >> name##_SHIFT) & name##_MASK)
> +#define INSERT(value, name) (((value) & name##_MASK) << name##_SHIFT)
Can we not reinvent the extract32/deposit32 functions, please?
Just use the standard ones in bitops.h.
thanks
-- PMM
- Re: [Qemu-devel] [PATCH 3/6] i.MX: Add i.MX6 CCM and ANALOG device.,
Peter Maydell <=