| Message ID | 1385014699-7257-11-git-send-email-lokeshvutla@ti.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
On Thu, Nov 21, 2013 at 1:18 AM, Lokesh Vutla <lokeshvutla@ti.com> wrote: > Updating the Multiplier and Dividers values for all DPLLs for EPOS EVM. > Following are the DPLL locking frequencies at OPP NOM: > MPU locks at 600MHz > Core locks at 1000MHz > Per locks at 960MHz > DDR locks at 266MHz > As mentioned earlier, this hardcoded frequency approach is really not scalable when you have more device variants coming in. Just look at the AM335x changes on how this gets complicated. > Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com> > --- > arch/arm/cpu/armv7/am33xx/clock.c | 12 +++++++--- > arch/arm/cpu/armv7/am33xx/clock_am33xx.c | 15 ++++++++++++ > arch/arm/cpu/armv7/am33xx/clock_am43xx.c | 8 +------ > arch/arm/include/asm/arch-am33xx/clock.h | 7 +++--- > board/ti/am43xx/board.c | 38 +++++++++++++++++++++++++++--- > board/ti/am43xx/board.h | 1 + > board/ti/am43xx/mux.c | 5 ++++ > 7 files changed, 69 insertions(+), 17 deletions(-) > > diff --git a/arch/arm/cpu/armv7/am33xx/clock.c b/arch/arm/cpu/armv7/am33xx/clock.c > index 8e5f3c6..0672798 100644 > --- a/arch/arm/cpu/armv7/am33xx/clock.c > +++ b/arch/arm/cpu/armv7/am33xx/clock.c > @@ -101,9 +101,15 @@ void do_setup_dpll(const struct dpll_regs *dpll_regs, > static void setup_dplls(void) > { > const struct dpll_params *params; > - do_setup_dpll(&dpll_core_regs, &dpll_core); > - do_setup_dpll(&dpll_mpu_regs, &dpll_mpu); > - do_setup_dpll(&dpll_per_regs, &dpll_per); > + > + params = get_dpll_core_params(); > + do_setup_dpll(&dpll_core_regs, params); > + > + params = get_dpll_mpu_params(); > + do_setup_dpll(&dpll_mpu_regs, params); > + > + params = get_dpll_per_params(); > + do_setup_dpll(&dpll_per_regs, params); > writel(0x300, &cmwkup->clkdcoldodpllper); > > params = get_dpll_ddr_params(); > diff --git a/arch/arm/cpu/armv7/am33xx/clock_am33xx.c b/arch/arm/cpu/armv7/am33xx/clock_am33xx.c > index fabe259..92142c8 100644 > --- a/arch/arm/cpu/armv7/am33xx/clock_am33xx.c > +++ b/arch/arm/cpu/armv7/am33xx/clock_am33xx.c > @@ -62,6 +62,21 @@ const struct dpll_params dpll_core = { > const struct dpll_params dpll_per = { > 960, OSC-1, 5, -1, -1, -1, -1}; > > +const struct dpll_params *get_dpll_mpu_params(void) > +{ > + return &dpll_mpu; > +} > + > +const struct dpll_params *get_dpll_core_params(void) > +{ > + return &dpll_core; > +} > + > +const struct dpll_params *get_dpll_per_params(void) > +{ > + return &dpll_per; > +} > + > void setup_clocks_for_console(void) > { > clrsetbits_le32(&cmwkup->wkclkstctrl, CD_CLKCTRL_CLKTRCTRL_MASK, > diff --git a/arch/arm/cpu/armv7/am33xx/clock_am43xx.c b/arch/arm/cpu/armv7/am33xx/clock_am43xx.c > index 22963b7..97c00b4 100644 > --- a/arch/arm/cpu/armv7/am33xx/clock_am43xx.c > +++ b/arch/arm/cpu/armv7/am33xx/clock_am43xx.c > @@ -48,15 +48,9 @@ const struct dpll_regs dpll_ddr_regs = { > .cm_idlest_dpll = CM_WKUP + 0x5A4, > .cm_clksel_dpll = CM_WKUP + 0x5AC, > .cm_div_m2_dpll = CM_WKUP + 0x5B0, > + .cm_div_m4_dpll = CM_WKUP + 0x5B8, > }; > > -const struct dpll_params dpll_mpu = { > - -1, -1, -1, -1, -1, -1, -1}; > -const struct dpll_params dpll_core = { > - -1, -1, -1, -1, -1, -1, -1}; > -const struct dpll_params dpll_per = { > - -1, -1, -1, -1, -1, -1, -1}; > - > void setup_clocks_for_console(void) > { > /* Do not add any spl_debug prints in this function */ > diff --git a/arch/arm/include/asm/arch-am33xx/clock.h b/arch/arm/include/asm/arch-am33xx/clock.h > index 519249e..7637457 100644 > --- a/arch/arm/include/asm/arch-am33xx/clock.h > +++ b/arch/arm/include/asm/arch-am33xx/clock.h > @@ -98,13 +98,12 @@ extern const struct dpll_regs dpll_mpu_regs; > extern const struct dpll_regs dpll_core_regs; > extern const struct dpll_regs dpll_per_regs; > extern const struct dpll_regs dpll_ddr_regs; > -extern const struct dpll_params dpll_mpu; > -extern const struct dpll_params dpll_core; > -extern const struct dpll_params dpll_per; > -extern const struct dpll_params dpll_ddr; > > extern struct cm_wkuppll *const cmwkup; > > +const struct dpll_params *get_dpll_mpu_params(void); > +const struct dpll_params *get_dpll_core_params(void); > +const struct dpll_params *get_dpll_per_params(void); > const struct dpll_params *get_dpll_ddr_params(void); > void do_setup_dpll(const struct dpll_regs *, const struct dpll_params *); > void prcm_init(void); > diff --git a/board/ti/am43xx/board.c b/board/ti/am43xx/board.c > index 723d0ca..2d1b8f9 100644 > --- a/board/ti/am43xx/board.c > +++ b/board/ti/am43xx/board.c > @@ -65,12 +65,44 @@ static int read_eeprom(struct am43xx_board_id *header) > > #ifdef CONFIG_SPL_BUILD > > -const struct dpll_params dpll_ddr = { > - -1, -1, -1, -1, -1, -1, -1}; > +const struct dpll_params epos_evm_dpll_ddr = { > + 266, 24, 1, -1, 1, -1, -1}; > +const struct dpll_params epos_evm_dpll_mpu = { > + 600, 24, 1, -1, -1, -1, -1}; > +const struct dpll_params epos_evm_dpll_core = { > + 1000, 24, -1, -1, 10, 8, 4}; > +const struct dpll_params epos_evm_dpll_per = { > + 960, 24, 5, -1, -1, -1, -1}; > > const struct dpll_params *get_dpll_ddr_params(void) > { > - return &dpll_ddr; > + if (board_is_eposevm()) > + return &epos_evm_dpll_ddr; > +} > + > +const struct dpll_params *get_dpll_mpu_params(void) > +{ > + if (board_is_eposevm()) > + return &epos_evm_dpll_mpu; > +} > + > +const struct dpll_params *get_dpll_core_params(void) > +{ > + struct am43xx_board_id header; > + > + enable_i2c0_pin_mux(); > + i2c_init(CONFIG_SYS_OMAP24_I2C_SPEED, CONFIG_SYS_OMAP24_I2C_SLAVE); > + if (read_eeprom(&header) < 0) > + puts("Could not get board ID.\n"); > + Reading the EEPROM again? Why don't you just read it once and update some common structure? Regards, Vaibhav
On Friday 22 November 2013 02:07 AM, Vaibhav Bedia wrote: > On Thu, Nov 21, 2013 at 1:18 AM, Lokesh Vutla <lokeshvutla@ti.com> wrote: >> Updating the Multiplier and Dividers values for all DPLLs for EPOS EVM. >> Following are the DPLL locking frequencies at OPP NOM: >> MPU locks at 600MHz >> Core locks at 1000MHz >> Per locks at 960MHz >> DDR locks at 266MHz >> > > As mentioned earlier, this hardcoded frequency approach is really not > scalable when you have > more device variants coming in. Just look at the AM335x changes on how > this gets complicated. We already had a discussion on this during V1 of this series. Sekhar and Tom replied to you comments. What is the point in asking the same question again? Since you are very concerned here. Why are you feeling it so complicated when new variants come? We can always differentiate the new variant from the old ones and can pass dpll structure accordingly. It is just a matter a one if condition. It ll be better to look at the current code and see how cleanly it is done. Thanks Lokesh > >> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com> >> --- >> arch/arm/cpu/armv7/am33xx/clock.c | 12 +++++++--- >> arch/arm/cpu/armv7/am33xx/clock_am33xx.c | 15 ++++++++++++ >> arch/arm/cpu/armv7/am33xx/clock_am43xx.c | 8 +------ >> arch/arm/include/asm/arch-am33xx/clock.h | 7 +++--- >> board/ti/am43xx/board.c | 38 +++++++++++++++++++++++++++--- >> board/ti/am43xx/board.h | 1 + >> board/ti/am43xx/mux.c | 5 ++++ >> 7 files changed, 69 insertions(+), 17 deletions(-) >> >> diff --git a/arch/arm/cpu/armv7/am33xx/clock.c b/arch/arm/cpu/armv7/am33xx/clock.c >> index 8e5f3c6..0672798 100644 >> --- a/arch/arm/cpu/armv7/am33xx/clock.c >> +++ b/arch/arm/cpu/armv7/am33xx/clock.c >> @@ -101,9 +101,15 @@ void do_setup_dpll(const struct dpll_regs *dpll_regs, >> static void setup_dplls(void) >> { >> const struct dpll_params *params; >> - do_setup_dpll(&dpll_core_regs, &dpll_core); >> - do_setup_dpll(&dpll_mpu_regs, &dpll_mpu); >> - do_setup_dpll(&dpll_per_regs, &dpll_per); >> + >> + params = get_dpll_core_params(); >> + do_setup_dpll(&dpll_core_regs, params); >> + >> + params = get_dpll_mpu_params(); >> + do_setup_dpll(&dpll_mpu_regs, params); >> + >> + params = get_dpll_per_params(); >> + do_setup_dpll(&dpll_per_regs, params); >> writel(0x300, &cmwkup->clkdcoldodpllper); >> >> params = get_dpll_ddr_params(); >> diff --git a/arch/arm/cpu/armv7/am33xx/clock_am33xx.c b/arch/arm/cpu/armv7/am33xx/clock_am33xx.c >> index fabe259..92142c8 100644 >> --- a/arch/arm/cpu/armv7/am33xx/clock_am33xx.c >> +++ b/arch/arm/cpu/armv7/am33xx/clock_am33xx.c >> @@ -62,6 +62,21 @@ const struct dpll_params dpll_core = { >> const struct dpll_params dpll_per = { >> 960, OSC-1, 5, -1, -1, -1, -1}; >> >> +const struct dpll_params *get_dpll_mpu_params(void) >> +{ >> + return &dpll_mpu; >> +} >> + >> +const struct dpll_params *get_dpll_core_params(void) >> +{ >> + return &dpll_core; >> +} >> + >> +const struct dpll_params *get_dpll_per_params(void) >> +{ >> + return &dpll_per; >> +} >> + >> void setup_clocks_for_console(void) >> { >> clrsetbits_le32(&cmwkup->wkclkstctrl, CD_CLKCTRL_CLKTRCTRL_MASK, >> diff --git a/arch/arm/cpu/armv7/am33xx/clock_am43xx.c b/arch/arm/cpu/armv7/am33xx/clock_am43xx.c >> index 22963b7..97c00b4 100644 >> --- a/arch/arm/cpu/armv7/am33xx/clock_am43xx.c >> +++ b/arch/arm/cpu/armv7/am33xx/clock_am43xx.c >> @@ -48,15 +48,9 @@ const struct dpll_regs dpll_ddr_regs = { >> .cm_idlest_dpll = CM_WKUP + 0x5A4, >> .cm_clksel_dpll = CM_WKUP + 0x5AC, >> .cm_div_m2_dpll = CM_WKUP + 0x5B0, >> + .cm_div_m4_dpll = CM_WKUP + 0x5B8, >> }; >> >> -const struct dpll_params dpll_mpu = { >> - -1, -1, -1, -1, -1, -1, -1}; >> -const struct dpll_params dpll_core = { >> - -1, -1, -1, -1, -1, -1, -1}; >> -const struct dpll_params dpll_per = { >> - -1, -1, -1, -1, -1, -1, -1}; >> - >> void setup_clocks_for_console(void) >> { >> /* Do not add any spl_debug prints in this function */ >> diff --git a/arch/arm/include/asm/arch-am33xx/clock.h b/arch/arm/include/asm/arch-am33xx/clock.h >> index 519249e..7637457 100644 >> --- a/arch/arm/include/asm/arch-am33xx/clock.h >> +++ b/arch/arm/include/asm/arch-am33xx/clock.h >> @@ -98,13 +98,12 @@ extern const struct dpll_regs dpll_mpu_regs; >> extern const struct dpll_regs dpll_core_regs; >> extern const struct dpll_regs dpll_per_regs; >> extern const struct dpll_regs dpll_ddr_regs; >> -extern const struct dpll_params dpll_mpu; >> -extern const struct dpll_params dpll_core; >> -extern const struct dpll_params dpll_per; >> -extern const struct dpll_params dpll_ddr; >> >> extern struct cm_wkuppll *const cmwkup; >> >> +const struct dpll_params *get_dpll_mpu_params(void); >> +const struct dpll_params *get_dpll_core_params(void); >> +const struct dpll_params *get_dpll_per_params(void); >> const struct dpll_params *get_dpll_ddr_params(void); >> void do_setup_dpll(const struct dpll_regs *, const struct dpll_params *); >> void prcm_init(void); >> diff --git a/board/ti/am43xx/board.c b/board/ti/am43xx/board.c >> index 723d0ca..2d1b8f9 100644 >> --- a/board/ti/am43xx/board.c >> +++ b/board/ti/am43xx/board.c >> @@ -65,12 +65,44 @@ static int read_eeprom(struct am43xx_board_id *header) >> >> #ifdef CONFIG_SPL_BUILD >> >> -const struct dpll_params dpll_ddr = { >> - -1, -1, -1, -1, -1, -1, -1}; >> +const struct dpll_params epos_evm_dpll_ddr = { >> + 266, 24, 1, -1, 1, -1, -1}; >> +const struct dpll_params epos_evm_dpll_mpu = { >> + 600, 24, 1, -1, -1, -1, -1}; >> +const struct dpll_params epos_evm_dpll_core = { >> + 1000, 24, -1, -1, 10, 8, 4}; >> +const struct dpll_params epos_evm_dpll_per = { >> + 960, 24, 5, -1, -1, -1, -1}; >> >> const struct dpll_params *get_dpll_ddr_params(void) >> { >> - return &dpll_ddr; >> + if (board_is_eposevm()) >> + return &epos_evm_dpll_ddr; >> +} >> + >> +const struct dpll_params *get_dpll_mpu_params(void) >> +{ >> + if (board_is_eposevm()) >> + return &epos_evm_dpll_mpu; >> +} >> + >> +const struct dpll_params *get_dpll_core_params(void) >> +{ >> + struct am43xx_board_id header; >> + >> + enable_i2c0_pin_mux(); >> + i2c_init(CONFIG_SYS_OMAP24_I2C_SPEED, CONFIG_SYS_OMAP24_I2C_SLAVE); >> + if (read_eeprom(&header) < 0) >> + puts("Could not get board ID.\n"); >> + > > Reading the EEPROM again? Why don't you just read it once and update some > common structure? > > Regards, > Vaibhav >
On Mon, Nov 25, 2013 at 12:08 AM, Lokesh Vutla <lokeshvutla@ti.com> wrote: > On Friday 22 November 2013 02:07 AM, Vaibhav Bedia wrote: >> On Thu, Nov 21, 2013 at 1:18 AM, Lokesh Vutla <lokeshvutla@ti.com> wrote: >>> Updating the Multiplier and Dividers values for all DPLLs for EPOS EVM. >>> Following are the DPLL locking frequencies at OPP NOM: >>> MPU locks at 600MHz >>> Core locks at 1000MHz >>> Per locks at 960MHz >>> DDR locks at 266MHz >>> >> >> As mentioned earlier, this hardcoded frequency approach is really not >> scalable when you have >> more device variants coming in. Just look at the AM335x changes on how >> this gets complicated. > We already had a discussion on this during V1 of this series. > Sekhar and Tom replied to you comments. What is the point in asking the same question again? > Because i don't recall any conclusion being reached on that thread? Because the objective of a patch review is to come up with a solution which learns from the past and doesn't try to brush the past issues under the carpet under the assumption that the shiny new device will never have bugs? > Since you are very concerned here. Why are you feeling it so complicated when new variants come? > We can always differentiate the new variant from the old ones and can pass dpll structure accordingly. It is just a matter a one if condition. > It ll be better to look at the current code and see how cleanly it is done. > As the OPP decoder table in the AM335x datasheet will tell you it's not that simple. Frankly, i could care less about this change...
On Wednesday 27 November 2013 05:36 AM, Vaibhav Bedia wrote: > On Mon, Nov 25, 2013 at 12:08 AM, Lokesh Vutla <lokeshvutla@ti.com> wrote: >> On Friday 22 November 2013 02:07 AM, Vaibhav Bedia wrote: >>> On Thu, Nov 21, 2013 at 1:18 AM, Lokesh Vutla <lokeshvutla@ti.com> wrote: >>>> Updating the Multiplier and Dividers values for all DPLLs for EPOS EVM. >>>> Following are the DPLL locking frequencies at OPP NOM: >>>> MPU locks at 600MHz >>>> Core locks at 1000MHz >>>> Per locks at 960MHz >>>> DDR locks at 266MHz >>>> >>> >>> As mentioned earlier, this hardcoded frequency approach is really not >>> scalable when you have >>> more device variants coming in. Just look at the AM335x changes on how >>> this gets complicated. >> We already had a discussion on this during V1 of this series. >> Sekhar and Tom replied to you comments. What is the point in asking the same question again? >> > > Because i don't recall any conclusion being reached on that thread? Because the > objective of a patch review is to come up with a solution which learns > from the past > and doesn't try to brush the past issues under the carpet under the > assumption that > the shiny new device will never have bugs? Yes I agree with you point. But I have replied to this thread saying that "Currently these values are not blown in eFuse. Both EPOS and GP evms support OPP NOM. So there is no harm in booting at OPP NOM here." You haven't come back on that. > >> Since you are very concerned here. Why are you feeling it so complicated when new variants come? >> We can always differentiate the new variant from the old ones and can pass dpll structure accordingly. It is just a matter a one if condition. >> It ll be better to look at the current code and see how cleanly it is done. >> > > As the OPP decoder table in the AM335x datasheet will tell you it's > not that simple. I guess your previous comment was about the code complexity when a new variant of the board comes. Coming to OPP tables: Frankly, I don't know what happened during AM33xx times. I am just trying to make things simpler and cleaner. Do you mean here, we need to support all OPPs in U-Boot? Are you expecting complete OPP table to get from e-fuse values for AM43xx? As per my understanding from working on OMAP, U-Boot supports only OPP_NOM( or the safe OPP to boot). Please correct me if I am wrong. Thanks and regards, Lokesh > Frankly, i could care less about this change... >
On Wed, Nov 27, 2013 at 1:58 AM, Lokesh Vutla <lokeshvutla@ti.com> wrote: > On Wednesday 27 November 2013 05:36 AM, Vaibhav Bedia wrote: >> On Mon, Nov 25, 2013 at 12:08 AM, Lokesh Vutla <lokeshvutla@ti.com> wrote: >>> On Friday 22 November 2013 02:07 AM, Vaibhav Bedia wrote: >>>> On Thu, Nov 21, 2013 at 1:18 AM, Lokesh Vutla <lokeshvutla@ti.com> wrote: >>>>> Updating the Multiplier and Dividers values for all DPLLs for EPOS EVM. >>>>> Following are the DPLL locking frequencies at OPP NOM: >>>>> MPU locks at 600MHz >>>>> Core locks at 1000MHz >>>>> Per locks at 960MHz >>>>> DDR locks at 266MHz >>>>> >>>> >>>> As mentioned earlier, this hardcoded frequency approach is really not >>>> scalable when you have >>>> more device variants coming in. Just look at the AM335x changes on how >>>> this gets complicated. >>> We already had a discussion on this during V1 of this series. >>> Sekhar and Tom replied to you comments. What is the point in asking the same question again? >>> >> >> Because i don't recall any conclusion being reached on that thread? Because the >> objective of a patch review is to come up with a solution which learns >> from the past >> and doesn't try to brush the past issues under the carpet under the >> assumption that >> the shiny new device will never have bugs? > Yes I agree with you point. > But I have replied to this thread saying that > "Currently these values are not blown in eFuse. Both EPOS and GP evms support > OPP NOM. So there is no harm in booting at OPP NOM here." > You haven't come back on that. > Sorry i somehow missed replying to that. So here's the thing. Right now you are told that all the devices support OPP NOM and hence as the U-Boot maintainer for AM437x you go ahead and use that information. However, few days down the line you'll have different speed grades coming out of the fab and some of them are going to have a max OPP that's lower than what OPP_NOM means. After a few devices blow up and some mails go around the world your code gets blamed and you hastily come up with a patch which tries to address the slower devices. The future you then realizes that you need some mechanism the differentiate the old devices without the eFuse information and the new which do. You might discover that there's some board rev check or something like that can be used. But that's only if you are lucky. I hope you see where this is going. So what can you do? A. Go ahead with OPP NOM and be prepared to debug boot failures on random boards only to discover that the devices are not meant to operate at OPP_NOM. B. Find the lowest common denominator for the supported OPPs and use that in here. This might end up making some folks unhappy but then you need to make a choice between fast and reliable and i would personally go for the latter. If folks really care about boot times someone needs to start putting the right information in the eFuses at the right time. >> >>> Since you are very concerned here. Why are you feeling it so complicated when new variants come? >>> We can always differentiate the new variant from the old ones and can pass dpll structure accordingly. It is just a matter a one if condition. >>> It ll be better to look at the current code and see how cleanly it is done. >>> >> >> As the OPP decoder table in the AM335x datasheet will tell you it's >> not that simple. > I guess your previous comment was about the code complexity when a new variant of the board comes. > Coming to OPP tables: > Frankly, I don't know what happened during AM33xx times. I am just trying to make things simpler and cleaner. > Do you mean here, we need to support all OPPs in U-Boot? > Are you expecting complete OPP table to get from e-fuse values for AM43xx? > > As per my understanding from working on OMAP, U-Boot supports only OPP_NOM( or the safe OPP to boot). > > Please correct me if I am wrong. > I hope the verbiage above helps you get an idea of why i am insisting on a change here. I am not asking U-Boot to have the complete OPP table. That's for the kernel + device-tree to handle. If there's no eFuse data in the devices, hard luck. We do the only thing we can for reliable boots and that's to use an OPP which is reliable all the time. If you still think OPP_NOM is the right choice please go ahead and use it. Regards, Vaibhav
On Thursday 28 November 2013 04:18 AM, Vaibhav Bedia wrote: > On Wed, Nov 27, 2013 at 1:58 AM, Lokesh Vutla <lokeshvutla@ti.com> wrote: >> On Wednesday 27 November 2013 05:36 AM, Vaibhav Bedia wrote: >>> On Mon, Nov 25, 2013 at 12:08 AM, Lokesh Vutla <lokeshvutla@ti.com> wrote: >>>> On Friday 22 November 2013 02:07 AM, Vaibhav Bedia wrote: >>>>> On Thu, Nov 21, 2013 at 1:18 AM, Lokesh Vutla <lokeshvutla@ti.com> wrote: >>>>>> Updating the Multiplier and Dividers values for all DPLLs for EPOS EVM. >>>>>> Following are the DPLL locking frequencies at OPP NOM: >>>>>> MPU locks at 600MHz >>>>>> Core locks at 1000MHz >>>>>> Per locks at 960MHz >>>>>> DDR locks at 266MHz >>>>>> >>>>> >>>>> As mentioned earlier, this hardcoded frequency approach is really not >>>>> scalable when you have >>>>> more device variants coming in. Just look at the AM335x changes on how >>>>> this gets complicated. >>>> We already had a discussion on this during V1 of this series. >>>> Sekhar and Tom replied to you comments. What is the point in asking the same question again? >>>> >>> >>> Because i don't recall any conclusion being reached on that thread? Because the >>> objective of a patch review is to come up with a solution which learns >>> from the past >>> and doesn't try to brush the past issues under the carpet under the >>> assumption that >>> the shiny new device will never have bugs? >> Yes I agree with you point. >> But I have replied to this thread saying that >> "Currently these values are not blown in eFuse. Both EPOS and GP evms support >> OPP NOM. So there is no harm in booting at OPP NOM here." >> You haven't come back on that. >> > > Sorry i somehow missed replying to that. > > So here's the thing. Right now you are told that all the devices > support OPP NOM and hence as the U-Boot maintainer for AM437x > you go ahead and use that information. However, few days down the > line you'll have different speed grades coming out of the fab and some > of them are going to have a max OPP that's lower than what OPP_NOM > means. After a few devices blow up and some mails go around the > world your code gets blamed and you hastily come up with a patch > which tries to address the slower devices. The future you then realizes > that you need some mechanism the differentiate the old devices without > the eFuse information and the new which do. You might discover that > there's some board rev check or something like that can be used. But that's > only if you are lucky. I hope you see where this is going. Thanks for the clear explanation. Now I get the point. > > So what can you do? > A. Go ahead with OPP NOM and be prepared to debug boot failures on > random boards only to discover that the devices are not meant to operate > at OPP_NOM. > > B. Find the lowest common denominator for the supported OPPs and use > that in here. This might end up making some folks unhappy but then you > need to make a choice between fast and reliable and i would personally go > for the latter. If folks really care about boot times someone needs to start > putting the right information in the eFuses at the right time. > >>> >>>> Since you are very concerned here. Why are you feeling it so complicated when new variants come? >>>> We can always differentiate the new variant from the old ones and can pass dpll structure accordingly. It is just a matter a one if condition. >>>> It ll be better to look at the current code and see how cleanly it is done. >>>> >>> >>> As the OPP decoder table in the AM335x datasheet will tell you it's >>> not that simple. >> I guess your previous comment was about the code complexity when a new variant of the board comes. >> Coming to OPP tables: >> Frankly, I don't know what happened during AM33xx times. I am just trying to make things simpler and cleaner. >> Do you mean here, we need to support all OPPs in U-Boot? >> Are you expecting complete OPP table to get from e-fuse values for AM43xx? >> >> As per my understanding from working on OMAP, U-Boot supports only OPP_NOM( or the safe OPP to boot). >> >> Please correct me if I am wrong. >> > > I hope the verbiage above helps you get an idea of why i am insisting on > a change here. > > I am not asking U-Boot to have the complete OPP table. That's for the kernel > + device-tree to handle. If there's no eFuse data in the devices, hard luck. > We do the only thing we can for reliable boots and that's to use an OPP which > is reliable all the time. I agree with you, we need to have EEPROM to have the safe OPP. Unfortunately we don't have this information. I am sure people will not accept to boot at lowest OPP. I read more about it and got inputs from Sekhar. I came to know that there is a DEV_ATTRIBUTE register which tells about the safest OPP to boot with. Looks like this should be sufficient to get the values. Ill add this code and update the patch. Thanks and regards, Lokesh > > If you still think OPP_NOM is the right choice please go ahead and use it. > > Regards, > Vaibhav >
On Sun, Dec 1, 2013 at 10:53 PM, Lokesh Vutla <lokeshvutla@ti.com> wrote: [...] > I read more about it and got inputs from Sekhar. I came to know that there is a DEV_ATTRIBUTE register > which tells about the safest OPP to boot with. Looks like this should be sufficient to get the values. > Ill add this code and update the patch. Hmm i don't know how that field gets populated. Unless it's coming from the eFuse i really doubt it would serve the purpose.
On 12/4/2013 8:50 AM, Vaibhav Bedia wrote: > On Sun, Dec 1, 2013 at 10:53 PM, Lokesh Vutla <lokeshvutla@ti.com> wrote: > [...] >> I read more about it and got inputs from Sekhar. I came to know that there is a DEV_ATTRIBUTE register >> which tells about the safest OPP to boot with. Looks like this should be sufficient to get the values. >> Ill add this code and update the patch. > > Hmm i don't know how that field gets populated. Unless it's coming > from the eFuse i really doubt > it would serve the purpose. Yes, that register is a reflection of efuse settings. Thanks, Sekhar
diff --git a/arch/arm/cpu/armv7/am33xx/clock.c b/arch/arm/cpu/armv7/am33xx/clock.c index 8e5f3c6..0672798 100644 --- a/arch/arm/cpu/armv7/am33xx/clock.c +++ b/arch/arm/cpu/armv7/am33xx/clock.c @@ -101,9 +101,15 @@ void do_setup_dpll(const struct dpll_regs *dpll_regs, static void setup_dplls(void) { const struct dpll_params *params; - do_setup_dpll(&dpll_core_regs, &dpll_core); - do_setup_dpll(&dpll_mpu_regs, &dpll_mpu); - do_setup_dpll(&dpll_per_regs, &dpll_per); + + params = get_dpll_core_params(); + do_setup_dpll(&dpll_core_regs, params); + + params = get_dpll_mpu_params(); + do_setup_dpll(&dpll_mpu_regs, params); + + params = get_dpll_per_params(); + do_setup_dpll(&dpll_per_regs, params); writel(0x300, &cmwkup->clkdcoldodpllper); params = get_dpll_ddr_params(); diff --git a/arch/arm/cpu/armv7/am33xx/clock_am33xx.c b/arch/arm/cpu/armv7/am33xx/clock_am33xx.c index fabe259..92142c8 100644 --- a/arch/arm/cpu/armv7/am33xx/clock_am33xx.c +++ b/arch/arm/cpu/armv7/am33xx/clock_am33xx.c @@ -62,6 +62,21 @@ const struct dpll_params dpll_core = { const struct dpll_params dpll_per = { 960, OSC-1, 5, -1, -1, -1, -1}; +const struct dpll_params *get_dpll_mpu_params(void) +{ + return &dpll_mpu; +} + +const struct dpll_params *get_dpll_core_params(void) +{ + return &dpll_core; +} + +const struct dpll_params *get_dpll_per_params(void) +{ + return &dpll_per; +} + void setup_clocks_for_console(void) { clrsetbits_le32(&cmwkup->wkclkstctrl, CD_CLKCTRL_CLKTRCTRL_MASK, diff --git a/arch/arm/cpu/armv7/am33xx/clock_am43xx.c b/arch/arm/cpu/armv7/am33xx/clock_am43xx.c index 22963b7..97c00b4 100644 --- a/arch/arm/cpu/armv7/am33xx/clock_am43xx.c +++ b/arch/arm/cpu/armv7/am33xx/clock_am43xx.c @@ -48,15 +48,9 @@ const struct dpll_regs dpll_ddr_regs = { .cm_idlest_dpll = CM_WKUP + 0x5A4, .cm_clksel_dpll = CM_WKUP + 0x5AC, .cm_div_m2_dpll = CM_WKUP + 0x5B0, + .cm_div_m4_dpll = CM_WKUP + 0x5B8, }; -const struct dpll_params dpll_mpu = { - -1, -1, -1, -1, -1, -1, -1}; -const struct dpll_params dpll_core = { - -1, -1, -1, -1, -1, -1, -1}; -const struct dpll_params dpll_per = { - -1, -1, -1, -1, -1, -1, -1}; - void setup_clocks_for_console(void) { /* Do not add any spl_debug prints in this function */ diff --git a/arch/arm/include/asm/arch-am33xx/clock.h b/arch/arm/include/asm/arch-am33xx/clock.h index 519249e..7637457 100644 --- a/arch/arm/include/asm/arch-am33xx/clock.h +++ b/arch/arm/include/asm/arch-am33xx/clock.h @@ -98,13 +98,12 @@ extern const struct dpll_regs dpll_mpu_regs; extern const struct dpll_regs dpll_core_regs; extern const struct dpll_regs dpll_per_regs; extern const struct dpll_regs dpll_ddr_regs; -extern const struct dpll_params dpll_mpu; -extern const struct dpll_params dpll_core; -extern const struct dpll_params dpll_per; -extern const struct dpll_params dpll_ddr; extern struct cm_wkuppll *const cmwkup; +const struct dpll_params *get_dpll_mpu_params(void); +const struct dpll_params *get_dpll_core_params(void); +const struct dpll_params *get_dpll_per_params(void); const struct dpll_params *get_dpll_ddr_params(void); void do_setup_dpll(const struct dpll_regs *, const struct dpll_params *); void prcm_init(void); diff --git a/board/ti/am43xx/board.c b/board/ti/am43xx/board.c index 723d0ca..2d1b8f9 100644 --- a/board/ti/am43xx/board.c +++ b/board/ti/am43xx/board.c @@ -65,12 +65,44 @@ static int read_eeprom(struct am43xx_board_id *header) #ifdef CONFIG_SPL_BUILD -const struct dpll_params dpll_ddr = { - -1, -1, -1, -1, -1, -1, -1}; +const struct dpll_params epos_evm_dpll_ddr = { + 266, 24, 1, -1, 1, -1, -1}; +const struct dpll_params epos_evm_dpll_mpu = { + 600, 24, 1, -1, -1, -1, -1}; +const struct dpll_params epos_evm_dpll_core = { + 1000, 24, -1, -1, 10, 8, 4}; +const struct dpll_params epos_evm_dpll_per = { + 960, 24, 5, -1, -1, -1, -1}; const struct dpll_params *get_dpll_ddr_params(void) { - return &dpll_ddr; + if (board_is_eposevm()) + return &epos_evm_dpll_ddr; +} + +const struct dpll_params *get_dpll_mpu_params(void) +{ + if (board_is_eposevm()) + return &epos_evm_dpll_mpu; +} + +const struct dpll_params *get_dpll_core_params(void) +{ + struct am43xx_board_id header; + + enable_i2c0_pin_mux(); + i2c_init(CONFIG_SYS_OMAP24_I2C_SPEED, CONFIG_SYS_OMAP24_I2C_SLAVE); + if (read_eeprom(&header) < 0) + puts("Could not get board ID.\n"); + + if (board_is_eposevm()) + return &epos_evm_dpll_core; +} + +const struct dpll_params *get_dpll_per_params(void) +{ + if (board_is_eposevm()) + return &epos_evm_dpll_per; } void set_uart_mux_conf(void) diff --git a/board/ti/am43xx/board.h b/board/ti/am43xx/board.h index 9268895..beee770 100644 --- a/board/ti/am43xx/board.h +++ b/board/ti/am43xx/board.h @@ -46,4 +46,5 @@ static inline int board_is_gpevm(void) void enable_uart0_pin_mux(void); void enable_board_pin_mux(void); +void enable_i2c0_pin_mux(void); #endif diff --git a/board/ti/am43xx/mux.c b/board/ti/am43xx/mux.c index 46bad01..a2d72dd 100644 --- a/board/ti/am43xx/mux.c +++ b/board/ti/am43xx/mux.c @@ -43,3 +43,8 @@ void enable_board_pin_mux(void) configure_module_pin_mux(mmc0_pin_mux); configure_module_pin_mux(i2c0_pin_mux); } + +void enable_i2c0_pin_mux(void) +{ + configure_module_pin_mux(i2c0_pin_mux); +}
Updating the Multiplier and Dividers values for all DPLLs for EPOS EVM. Following are the DPLL locking frequencies at OPP NOM: MPU locks at 600MHz Core locks at 1000MHz Per locks at 960MHz DDR locks at 266MHz Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com> --- arch/arm/cpu/armv7/am33xx/clock.c | 12 +++++++--- arch/arm/cpu/armv7/am33xx/clock_am33xx.c | 15 ++++++++++++ arch/arm/cpu/armv7/am33xx/clock_am43xx.c | 8 +------ arch/arm/include/asm/arch-am33xx/clock.h | 7 +++--- board/ti/am43xx/board.c | 38 +++++++++++++++++++++++++++--- board/ti/am43xx/board.h | 1 + board/ti/am43xx/mux.c | 5 ++++ 7 files changed, 69 insertions(+), 17 deletions(-)