| Message ID | 1453844698-26375-1-git-send-email-jcd@tribudubois.net |
|---|---|
| State | New |
| Headers | show |
On 26 January 2016 at 21:44, Jean-Christophe Dubois <jcd@tribudubois.net> wrote: > Various i.MX timers (GPT, EPIT, PWM, ...) are only requesting 4 clocks > from the system. > * CLK_NONE, > * CLK_IPG, > * CLK_IPG_HIGH, > * CLK_32k > > Other "clocks" are not required by the qemu framework so far. This patch confuses me. You don't say why we're removing a bunch of code or why it's OK to do that. > Signed-off-by: Jean-Christophe Dubois <jcd@tribudubois.net> > --- > hw/misc/imx25_ccm.c | 35 +++++------------------------------ > hw/misc/imx31_ccm.c | 38 ++++---------------------------------- > hw/timer/imx_epit.c | 8 ++++---- > hw/timer/imx_gpt.c | 16 ++++++++-------- > include/hw/misc/imx_ccm.h | 10 ++-------- > 5 files changed, 23 insertions(+), 84 deletions(-) > > diff --git a/hw/misc/imx25_ccm.c b/hw/misc/imx25_ccm.c > index 23759ca..38599f2 100644 > --- a/hw/misc/imx25_ccm.c > +++ b/hw/misc/imx25_ccm.c > @@ -119,20 +119,6 @@ static uint32_t imx25_ccm_get_mpll_clk(IMXCCMState *dev) > return freq; > } > > -static uint32_t imx25_ccm_get_upll_clk(IMXCCMState *dev) > -{ > - uint32_t freq = 0; > - IMX25CCMState *s = IMX25_CCM(dev); > - > - if (!EXTRACT(s->reg[IMX25_CCM_CCTL_REG], UPLL_DIS)) { > - freq = imx_ccm_calc_pll(s->reg[IMX25_CCM_UPCTL_REG], CKIH_FREQ); > - } > - > - DPRINTF("freq = %d\n", freq); > - > - return freq; > -} > - > static uint32_t imx25_ccm_get_mcu_clk(IMXCCMState *dev) > { > uint32_t freq; > @@ -181,21 +167,10 @@ static uint32_t imx25_ccm_get_clock_frequency(IMXCCMState *dev, IMXClk clock) > DPRINTF("Clock = %d)\n", clock); > > switch (clock) { > - case NOCLK: > - break; > - case CLK_MPLL: > - freq = imx25_ccm_get_mpll_clk(dev); > - break; > - case CLK_UPLL: > - freq = imx25_ccm_get_upll_clk(dev); > - break; > - case CLK_MCU: > - freq = imx25_ccm_get_mcu_clk(dev); > - break; > - case CLK_AHB: > - freq = imx25_ccm_get_ahb_clk(dev); > + case CLK_NONE: > break; > case CLK_IPG: > + case CLK_IPG_HIGH: > freq = imx25_ccm_get_ipg_clk(dev); > break; > case CLK_32k: > @@ -221,9 +196,9 @@ static void imx25_ccm_reset(DeviceState *dev) > memset(s->reg, 0, IMX25_CCM_MAX_REG * sizeof(uint32_t)); > s->reg[IMX25_CCM_MPCTL_REG] = 0x800b2c01; > s->reg[IMX25_CCM_UPCTL_REG] = 0x84042800; > - /* > + /* > * The value below gives: > - * CPU = 133 MHz, AHB = 66,5 MHz, IPG = 33 MHz. > + * CPU = 133 MHz, AHB = 66,5 MHz, IPG = 33 MHz. > */ > s->reg[IMX25_CCM_CCTL_REG] = 0xd0030000; > s->reg[IMX25_CCM_CGCR0_REG] = 0x028A0100; > @@ -240,7 +215,7 @@ static void imx25_ccm_reset(DeviceState *dev) > > /* > * default boot will change the reset values to allow: > - * CPU = 399 MHz, AHB = 133 MHz, IPG = 66,5 MHz. > + * CPU = 399 MHz, AHB = 133 MHz, IPG = 66,5 MHz. > * For some reason, this doesn't work. With the value below, linux > * detects a 88 MHz IPG CLK instead of 66,5 MHz. > s->reg[IMX25_CCM_CCTL_REG] = 0x20032000; > diff --git a/hw/misc/imx31_ccm.c b/hw/misc/imx31_ccm.c > index 47d6ead..3876cc2 100644 > --- a/hw/misc/imx31_ccm.c > +++ b/hw/misc/imx31_ccm.c > @@ -111,7 +111,7 @@ static uint32_t imx31_ccm_get_pll_ref_clk(IMXCCMState *dev) > if (s->reg[IMX31_CCM_CCMR_REG] & CCMR_FPMF) { > freq *= 1024; > } > - } > + } What are all these whitespace only changes? If you need to do a whitespace cleanup please put it in its own patch. > case CLK_32k: > diff --git a/hw/timer/imx_epit.c b/hw/timer/imx_epit.c > index 50bf83c..a67bcb5 100644 > --- a/hw/timer/imx_epit.c > +++ b/hw/timer/imx_epit.c > @@ -51,10 +51,10 @@ static char const *imx_epit_reg_name(uint32_t reg) > * These are typical. > */ > static const IMXClk imx_epit_clocks[] = { > - NOCLK, /* 00 disabled */ > - CLK_IPG, /* 01 ipg_clk, ~532MHz */ > - CLK_IPG, /* 10 ipg_clk_highfreq */ > - CLK_32k, /* 11 ipg_clk_32k -- ~32kHz */ > + CLK_NONE, /* 00 disabled */ > + CLK_IPG, /* 01 ipg_clk, ~532MHz */ > + CLK_IPG_HIGH, /* 10 ipg_clk_highfreq */ > + CLK_32k, /* 11 ipg_clk_32k -- ~32kHz */ > }; > > /* > diff --git a/hw/timer/imx_gpt.c b/hw/timer/imx_gpt.c > index b227256..b2f03b0 100644 > --- a/hw/timer/imx_gpt.c > +++ b/hw/timer/imx_gpt.c > @@ -80,14 +80,14 @@ static const VMStateDescription vmstate_imx_timer_gpt = { > }; > > static const IMXClk imx_gpt_clocks[] = { > - NOCLK, /* 000 No clock source */ > - CLK_IPG, /* 001 ipg_clk, 532MHz*/ > - CLK_IPG, /* 010 ipg_clk_highfreq */ > - NOCLK, /* 011 not defined */ > - CLK_32k, /* 100 ipg_clk_32k */ > - NOCLK, /* 101 not defined */ > - NOCLK, /* 110 not defined */ > - NOCLK, /* 111 not defined */ > + CLK_NONE, /* 000 No clock source */ > + CLK_IPG, /* 001 ipg_clk, 532MHz*/ > + CLK_IPG_HIGH, /* 010 ipg_clk_highfreq */ > + CLK_NONE, /* 011 not defined */ > + CLK_32k, /* 100 ipg_clk_32k */ > + CLK_NONE, /* 101 not defined */ > + CLK_NONE, /* 110 not defined */ > + CLK_NONE, /* 111 not defined */ > }; These are just renaming NOCLK to CLK_NONE and fixing formatting? Again, please don't put that in the same patch as substantive code changes. thanks -- PMM
Le 02/02/2016 17:22, Peter Maydell a écrit : > On 26 January 2016 at 21:44, Jean-Christophe Dubois <jcd@tribudubois.net> wrote: >> Various i.MX timers (GPT, EPIT, PWM, ...) are only requesting 4 clocks >> from the system. >> * CLK_NONE, >> * CLK_IPG, >> * CLK_IPG_HIGH, >> * CLK_32k >> >> Other "clocks" are not required by the qemu framework so far. > This patch confuses me. You don't say why we're removing a bunch > of code or why it's OK to do that. I added these other clocks in a previous patch but I believe it was a mistake I made. These additional clocks (MCU, AHB, PER, ...) will likely never be user inside QEMU. And each new version of SOC will just continue to add to this list of clocks making things always more confusing. In fact, the only interesting clocks are the ones that are used by timer devices. And timer devices don't directly use SOC named clocks (PER, AHB, ...), but they use only (for now) the 4 abstract clocks listed above. Each SOC seems then free to map these abstract clocks (generally mostly IPG and IPG_HIGH) to any of its real clocks. So I decided to just remove the useless code in relation with SOC clocks that are not otherwise used in the rest of the QEMU code. I added this mess so I should clean it. And this is really what this patch is: A cleanup of something that should not have been added in the first place because it has no use. Sorry about that. > >> Signed-off-by: Jean-Christophe Dubois <jcd@tribudubois.net> >> --- >> hw/misc/imx25_ccm.c | 35 +++++------------------------------ >> hw/misc/imx31_ccm.c | 38 ++++---------------------------------- >> hw/timer/imx_epit.c | 8 ++++---- >> hw/timer/imx_gpt.c | 16 ++++++++-------- >> include/hw/misc/imx_ccm.h | 10 ++-------- >> 5 files changed, 23 insertions(+), 84 deletions(-) >> >> diff --git a/hw/misc/imx25_ccm.c b/hw/misc/imx25_ccm.c >> index 23759ca..38599f2 100644 >> --- a/hw/misc/imx25_ccm.c >> +++ b/hw/misc/imx25_ccm.c >> @@ -119,20 +119,6 @@ static uint32_t imx25_ccm_get_mpll_clk(IMXCCMState *dev) >> return freq; >> } >> >> -static uint32_t imx25_ccm_get_upll_clk(IMXCCMState *dev) >> -{ >> - uint32_t freq = 0; >> - IMX25CCMState *s = IMX25_CCM(dev); >> - >> - if (!EXTRACT(s->reg[IMX25_CCM_CCTL_REG], UPLL_DIS)) { >> - freq = imx_ccm_calc_pll(s->reg[IMX25_CCM_UPCTL_REG], CKIH_FREQ); >> - } >> - >> - DPRINTF("freq = %d\n", freq); >> - >> - return freq; >> -} >> - >> static uint32_t imx25_ccm_get_mcu_clk(IMXCCMState *dev) >> { >> uint32_t freq; >> @@ -181,21 +167,10 @@ static uint32_t imx25_ccm_get_clock_frequency(IMXCCMState *dev, IMXClk clock) >> DPRINTF("Clock = %d)\n", clock); >> >> switch (clock) { >> - case NOCLK: >> - break; >> - case CLK_MPLL: >> - freq = imx25_ccm_get_mpll_clk(dev); >> - break; >> - case CLK_UPLL: >> - freq = imx25_ccm_get_upll_clk(dev); >> - break; >> - case CLK_MCU: >> - freq = imx25_ccm_get_mcu_clk(dev); >> - break; >> - case CLK_AHB: >> - freq = imx25_ccm_get_ahb_clk(dev); >> + case CLK_NONE: >> break; >> case CLK_IPG: >> + case CLK_IPG_HIGH: >> freq = imx25_ccm_get_ipg_clk(dev); >> break; >> case CLK_32k: >> @@ -221,9 +196,9 @@ static void imx25_ccm_reset(DeviceState *dev) >> memset(s->reg, 0, IMX25_CCM_MAX_REG * sizeof(uint32_t)); >> s->reg[IMX25_CCM_MPCTL_REG] = 0x800b2c01; >> s->reg[IMX25_CCM_UPCTL_REG] = 0x84042800; >> - /* >> + /* >> * The value below gives: >> - * CPU = 133 MHz, AHB = 66,5 MHz, IPG = 33 MHz. >> + * CPU = 133 MHz, AHB = 66,5 MHz, IPG = 33 MHz. >> */ >> s->reg[IMX25_CCM_CCTL_REG] = 0xd0030000; >> s->reg[IMX25_CCM_CGCR0_REG] = 0x028A0100; >> @@ -240,7 +215,7 @@ static void imx25_ccm_reset(DeviceState *dev) >> >> /* >> * default boot will change the reset values to allow: >> - * CPU = 399 MHz, AHB = 133 MHz, IPG = 66,5 MHz. >> + * CPU = 399 MHz, AHB = 133 MHz, IPG = 66,5 MHz. >> * For some reason, this doesn't work. With the value below, linux >> * detects a 88 MHz IPG CLK instead of 66,5 MHz. >> s->reg[IMX25_CCM_CCTL_REG] = 0x20032000; >> diff --git a/hw/misc/imx31_ccm.c b/hw/misc/imx31_ccm.c >> index 47d6ead..3876cc2 100644 >> --- a/hw/misc/imx31_ccm.c >> +++ b/hw/misc/imx31_ccm.c >> @@ -111,7 +111,7 @@ static uint32_t imx31_ccm_get_pll_ref_clk(IMXCCMState *dev) >> if (s->reg[IMX31_CCM_CCMR_REG] & CCMR_FPMF) { >> freq *= 1024; >> } >> - } >> + } > > What are all these whitespace only changes? If you need to do a > whitespace cleanup please put it in its own patch. OK > >> case CLK_32k: >> diff --git a/hw/timer/imx_epit.c b/hw/timer/imx_epit.c >> index 50bf83c..a67bcb5 100644 >> --- a/hw/timer/imx_epit.c >> +++ b/hw/timer/imx_epit.c >> @@ -51,10 +51,10 @@ static char const *imx_epit_reg_name(uint32_t reg) >> * These are typical. >> */ >> static const IMXClk imx_epit_clocks[] = { >> - NOCLK, /* 00 disabled */ >> - CLK_IPG, /* 01 ipg_clk, ~532MHz */ >> - CLK_IPG, /* 10 ipg_clk_highfreq */ >> - CLK_32k, /* 11 ipg_clk_32k -- ~32kHz */ >> + CLK_NONE, /* 00 disabled */ >> + CLK_IPG, /* 01 ipg_clk, ~532MHz */ >> + CLK_IPG_HIGH, /* 10 ipg_clk_highfreq */ >> + CLK_32k, /* 11 ipg_clk_32k -- ~32kHz */ >> }; >> >> /* >> diff --git a/hw/timer/imx_gpt.c b/hw/timer/imx_gpt.c >> index b227256..b2f03b0 100644 >> --- a/hw/timer/imx_gpt.c >> +++ b/hw/timer/imx_gpt.c >> @@ -80,14 +80,14 @@ static const VMStateDescription vmstate_imx_timer_gpt = { >> }; >> >> static const IMXClk imx_gpt_clocks[] = { >> - NOCLK, /* 000 No clock source */ >> - CLK_IPG, /* 001 ipg_clk, 532MHz*/ >> - CLK_IPG, /* 010 ipg_clk_highfreq */ >> - NOCLK, /* 011 not defined */ >> - CLK_32k, /* 100 ipg_clk_32k */ >> - NOCLK, /* 101 not defined */ >> - NOCLK, /* 110 not defined */ >> - NOCLK, /* 111 not defined */ >> + CLK_NONE, /* 000 No clock source */ >> + CLK_IPG, /* 001 ipg_clk, 532MHz*/ >> + CLK_IPG_HIGH, /* 010 ipg_clk_highfreq */ >> + CLK_NONE, /* 011 not defined */ >> + CLK_32k, /* 100 ipg_clk_32k */ >> + CLK_NONE, /* 101 not defined */ >> + CLK_NONE, /* 110 not defined */ >> + CLK_NONE, /* 111 not defined */ >> }; > These are just renaming NOCLK to CLK_NONE and fixing formatting? > Again, please don't put that in the same patch as substantive > code changes. I just wanted to make things more coherent at the naming convention level. But if you prefer NOCLK, I'll put it back. > > thanks > -- PMM >
On 2 February 2016 at 22:22, Jean-Christophe DUBOIS <jcd@tribudubois.net> wrote: > Peter Maydell wrote: >> These are just renaming NOCLK to CLK_NONE and fixing formatting? >> Again, please don't put that in the same patch as substantive >> code changes. > > > I just wanted to make things more coherent at the naming convention level. > > But if you prefer NOCLK, I'll put it back. I have no preference either way about the name. It's just hard to review patches if they mix lots of cleanups in at once, and especially if they mix stylistic changes in with behaviour changes. It's probably sufficient just to split this patch up into logically distinct changes with suitable commit messages. thanks -- PMM
diff --git a/hw/misc/imx25_ccm.c b/hw/misc/imx25_ccm.c index 23759ca..38599f2 100644 --- a/hw/misc/imx25_ccm.c +++ b/hw/misc/imx25_ccm.c @@ -119,20 +119,6 @@ static uint32_t imx25_ccm_get_mpll_clk(IMXCCMState *dev) return freq; } -static uint32_t imx25_ccm_get_upll_clk(IMXCCMState *dev) -{ - uint32_t freq = 0; - IMX25CCMState *s = IMX25_CCM(dev); - - if (!EXTRACT(s->reg[IMX25_CCM_CCTL_REG], UPLL_DIS)) { - freq = imx_ccm_calc_pll(s->reg[IMX25_CCM_UPCTL_REG], CKIH_FREQ); - } - - DPRINTF("freq = %d\n", freq); - - return freq; -} - static uint32_t imx25_ccm_get_mcu_clk(IMXCCMState *dev) { uint32_t freq; @@ -181,21 +167,10 @@ static uint32_t imx25_ccm_get_clock_frequency(IMXCCMState *dev, IMXClk clock) DPRINTF("Clock = %d)\n", clock); switch (clock) { - case NOCLK: - break; - case CLK_MPLL: - freq = imx25_ccm_get_mpll_clk(dev); - break; - case CLK_UPLL: - freq = imx25_ccm_get_upll_clk(dev); - break; - case CLK_MCU: - freq = imx25_ccm_get_mcu_clk(dev); - break; - case CLK_AHB: - freq = imx25_ccm_get_ahb_clk(dev); + case CLK_NONE: break; case CLK_IPG: + case CLK_IPG_HIGH: freq = imx25_ccm_get_ipg_clk(dev); break; case CLK_32k: @@ -221,9 +196,9 @@ static void imx25_ccm_reset(DeviceState *dev) memset(s->reg, 0, IMX25_CCM_MAX_REG * sizeof(uint32_t)); s->reg[IMX25_CCM_MPCTL_REG] = 0x800b2c01; s->reg[IMX25_CCM_UPCTL_REG] = 0x84042800; - /* + /* * The value below gives: - * CPU = 133 MHz, AHB = 66,5 MHz, IPG = 33 MHz. + * CPU = 133 MHz, AHB = 66,5 MHz, IPG = 33 MHz. */ s->reg[IMX25_CCM_CCTL_REG] = 0xd0030000; s->reg[IMX25_CCM_CGCR0_REG] = 0x028A0100; @@ -240,7 +215,7 @@ static void imx25_ccm_reset(DeviceState *dev) /* * default boot will change the reset values to allow: - * CPU = 399 MHz, AHB = 133 MHz, IPG = 66,5 MHz. + * CPU = 399 MHz, AHB = 133 MHz, IPG = 66,5 MHz. * For some reason, this doesn't work. With the value below, linux * detects a 88 MHz IPG CLK instead of 66,5 MHz. s->reg[IMX25_CCM_CCTL_REG] = 0x20032000; diff --git a/hw/misc/imx31_ccm.c b/hw/misc/imx31_ccm.c index 47d6ead..3876cc2 100644 --- a/hw/misc/imx31_ccm.c +++ b/hw/misc/imx31_ccm.c @@ -111,7 +111,7 @@ static uint32_t imx31_ccm_get_pll_ref_clk(IMXCCMState *dev) if (s->reg[IMX31_CCM_CCMR_REG] & CCMR_FPMF) { freq *= 1024; } - } + } } else { freq = CKIH_FREQ; } @@ -134,6 +134,7 @@ static uint32_t imx31_ccm_get_mpll_clk(IMXCCMState *dev) return freq; } + static uint32_t imx31_ccm_get_mcu_main_clk(IMXCCMState *dev) { uint32_t freq; @@ -151,32 +152,6 @@ static uint32_t imx31_ccm_get_mcu_main_clk(IMXCCMState *dev) return freq; } -static uint32_t imx31_ccm_get_mcu_clk(IMXCCMState *dev) -{ - uint32_t freq; - IMX31CCMState *s = IMX31_CCM(dev); - - freq = imx31_ccm_get_mcu_main_clk(dev) - / (1 + EXTRACT(s->reg[IMX31_CCM_PDR0_REG], MCU)); - - DPRINTF("freq = %d\n", freq); - - return freq; -} - -static uint32_t imx31_ccm_get_hsp_clk(IMXCCMState *dev) -{ - uint32_t freq; - IMX31CCMState *s = IMX31_CCM(dev); - - freq = imx31_ccm_get_mcu_main_clk(dev) - / (1 + EXTRACT(s->reg[IMX31_CCM_PDR0_REG], HSP)); - - DPRINTF("freq = %d\n", freq); - - return freq; -} - static uint32_t imx31_ccm_get_hclk_clk(IMXCCMState *dev) { uint32_t freq; @@ -208,15 +183,10 @@ static uint32_t imx31_ccm_get_clock_frequency(IMXCCMState *dev, IMXClk clock) uint32_t freq = 0; switch (clock) { - case NOCLK: - break; - case CLK_MCU: - freq = imx31_ccm_get_mcu_clk(dev); - break; - case CLK_HSP: - freq = imx31_ccm_get_hsp_clk(dev); + case CLK_NONE: break; case CLK_IPG: + case CLK_IPG_HIGH: freq = imx31_ccm_get_ipg_clk(dev); break; case CLK_32k: diff --git a/hw/timer/imx_epit.c b/hw/timer/imx_epit.c index 50bf83c..a67bcb5 100644 --- a/hw/timer/imx_epit.c +++ b/hw/timer/imx_epit.c @@ -51,10 +51,10 @@ static char const *imx_epit_reg_name(uint32_t reg) * These are typical. */ static const IMXClk imx_epit_clocks[] = { - NOCLK, /* 00 disabled */ - CLK_IPG, /* 01 ipg_clk, ~532MHz */ - CLK_IPG, /* 10 ipg_clk_highfreq */ - CLK_32k, /* 11 ipg_clk_32k -- ~32kHz */ + CLK_NONE, /* 00 disabled */ + CLK_IPG, /* 01 ipg_clk, ~532MHz */ + CLK_IPG_HIGH, /* 10 ipg_clk_highfreq */ + CLK_32k, /* 11 ipg_clk_32k -- ~32kHz */ }; /* diff --git a/hw/timer/imx_gpt.c b/hw/timer/imx_gpt.c index b227256..b2f03b0 100644 --- a/hw/timer/imx_gpt.c +++ b/hw/timer/imx_gpt.c @@ -80,14 +80,14 @@ static const VMStateDescription vmstate_imx_timer_gpt = { }; static const IMXClk imx_gpt_clocks[] = { - NOCLK, /* 000 No clock source */ - CLK_IPG, /* 001 ipg_clk, 532MHz*/ - CLK_IPG, /* 010 ipg_clk_highfreq */ - NOCLK, /* 011 not defined */ - CLK_32k, /* 100 ipg_clk_32k */ - NOCLK, /* 101 not defined */ - NOCLK, /* 110 not defined */ - NOCLK, /* 111 not defined */ + CLK_NONE, /* 000 No clock source */ + CLK_IPG, /* 001 ipg_clk, 532MHz*/ + CLK_IPG_HIGH, /* 010 ipg_clk_highfreq */ + CLK_NONE, /* 011 not defined */ + CLK_32k, /* 100 ipg_clk_32k */ + CLK_NONE, /* 101 not defined */ + CLK_NONE, /* 110 not defined */ + CLK_NONE, /* 111 not defined */ }; static void imx_gpt_set_freq(IMXGPTState *s) diff --git a/include/hw/misc/imx_ccm.h b/include/hw/misc/imx_ccm.h index 5c4b795..48a7afa 100644 --- a/include/hw/misc/imx_ccm.h +++ b/include/hw/misc/imx_ccm.h @@ -43,15 +43,9 @@ typedef struct IMXCCMState { } IMXCCMState; typedef enum { - NOCLK, - CLK_MPLL, - CLK_UPLL, - CLK_MCU, - CLK_HSP, - CLK_MAX, - CLK_AHB, + CLK_NONE, CLK_IPG, - CLK_PER, + CLK_IPG_HIGH, CLK_32k } IMXClk;
Various i.MX timers (GPT, EPIT, PWM, ...) are only requesting 4 clocks from the system. * CLK_NONE, * CLK_IPG, * CLK_IPG_HIGH, * CLK_32k Other "clocks" are not required by the qemu framework so far. Signed-off-by: Jean-Christophe Dubois <jcd@tribudubois.net> --- hw/misc/imx25_ccm.c | 35 +++++------------------------------ hw/misc/imx31_ccm.c | 38 ++++---------------------------------- hw/timer/imx_epit.c | 8 ++++---- hw/timer/imx_gpt.c | 16 ++++++++-------- include/hw/misc/imx_ccm.h | 10 ++-------- 5 files changed, 23 insertions(+), 84 deletions(-)