| Message ID | 1414517144-2183-1-git-send-email-contact@paulk.fr |
|---|---|
| State | Changes Requested |
| Delegated to: | Pantelis Antoniou |
| Headers | show |
Hi Paul, On 10/28/14 19:25, Paul Kocialkowski wrote: > Some devices may use non-standard combinations of regulators to power MMC: > this allows these devices to provide a board-specific MMC power init function > to set everything up in their own way. > > Signed-off-by: Paul Kocialkowski <contact@paulk.fr> > --- > arch/arm/include/asm/omap_mmc.h | 4 +++- > drivers/mmc/omap_hsmmc.c | 4 +++- > 2 files changed, 6 insertions(+), 2 deletions(-) > > diff --git a/arch/arm/include/asm/omap_mmc.h b/arch/arm/include/asm/omap_mmc.h > index 617e22f..b6a8325 100644 > --- a/arch/arm/include/asm/omap_mmc.h > +++ b/arch/arm/include/asm/omap_mmc.h > @@ -164,5 +164,7 @@ struct hsmmc { > int omap_mmc_init(int dev_index, uint host_caps_mask, uint f_max, int cd_gpio, > int wp_gpio); > > - > +#ifdef CONFIG_OMAP_HSMMC_BOARD_POWER_INIT I'm not a huge fan of that approach, but if you add yet another CONFIG_ option, I think it is a requirement to add a documentation for it. > +void omap_hsmmc_board_power_init(void); Anyway, I would suggest adding a default __weak board_mmc_power_init() or something like this (which would be transfered into a callback in pdata once omap_hsmmc.c is). Or... just no need for this patch at all, as board_mmc_init() can be used for this... > +#endif > #endif /* OMAP_MMC_H_ */ > diff --git a/drivers/mmc/omap_hsmmc.c b/drivers/mmc/omap_hsmmc.c > index ef2cbf9..ef4c5cf 100644 > --- a/drivers/mmc/omap_hsmmc.c > +++ b/drivers/mmc/omap_hsmmc.c > @@ -136,7 +136,9 @@ static unsigned char mmc_board_init(struct mmc *mmc) > pbias_lite &= ~(PBIASLITEPWRDNZ1 | PBIASLITEPWRDNZ0); > writel(pbias_lite, &t2_base->pbias_lite); > #endif > -#if defined(CONFIG_TWL4030_POWER) > +#if defined(CONFIG_OMAP_HSMMC_BOARD_POWER_INIT) > + omap_hsmmc_board_power_init(); > +#elif defined(CONFIG_TWL4030_POWER) > twl4030_power_mmc_init(); > mdelay(100); /* ramp-up delay from Linux code */ > #endif >
Le mardi 28 octobre 2014 à 20:02 +0200, Igor Grinberg a écrit : > Hi Paul, > > On 10/28/14 19:25, Paul Kocialkowski wrote: > > Some devices may use non-standard combinations of regulators to power MMC: > > this allows these devices to provide a board-specific MMC power init function > > to set everything up in their own way. > > > > Signed-off-by: Paul Kocialkowski <contact@paulk.fr> > > --- > > arch/arm/include/asm/omap_mmc.h | 4 +++- > > drivers/mmc/omap_hsmmc.c | 4 +++- > > 2 files changed, 6 insertions(+), 2 deletions(-) > > > > diff --git a/arch/arm/include/asm/omap_mmc.h b/arch/arm/include/asm/omap_mmc.h > > index 617e22f..b6a8325 100644 > > --- a/arch/arm/include/asm/omap_mmc.h > > +++ b/arch/arm/include/asm/omap_mmc.h > > @@ -164,5 +164,7 @@ struct hsmmc { > > int omap_mmc_init(int dev_index, uint host_caps_mask, uint f_max, int cd_gpio, > > int wp_gpio); > > > > - > > +#ifdef CONFIG_OMAP_HSMMC_BOARD_POWER_INIT > > I'm not a huge fan of that approach, but if you add > yet another CONFIG_ option, I think it is a requirement to add > a documentation for it. That saddens me too, but I don't see how to do this in a better way for now. > > +void omap_hsmmc_board_power_init(void); > > Anyway, I would suggest adding a default > __weak board_mmc_power_init() or something like this > (which would be transfered into a callback in pdata once omap_hsmmc.c is). Well, there are two distinct scenarios here: 1. the board follows some reference design and VMMC1 is used to power MMC1, VMMC2 for MMC2, then twl4030 has a function that will set everything up correctly (with the addition I added earlier) 2. the board uses the regulators in a more or less random way and VMMC1 is not mapped to MMC1 power at all. This is the case on the LG Optimus Black (P970). There is actually an auxiliary regulator IC that is in charge of powering MMC1. Hence the need for board-specific behavior. I think it's perfectly fine to keep using the twl4030 function when the board follows the reference design. Would that board_mmc_power_init you suggest be generic to all MMC drivers? Then at which point should it be called? What would a generic board_mmc_power_init have? TWL4030 is specific to OMAP3. Then perhaps there should be a board_mmc_power_init implementation for OMAP3, that can be overridden by a board-specific function? I'm a bit new to the U-Boot code, so I'd appreciate pointers on what should go where. > Or... just no need for this patch at all, as board_mmc_init() > can be used for this... Not in the context of the SPL, which is precisely why I needed this. Thanks for your review! > > +#endif > > #endif /* OMAP_MMC_H_ */ > > diff --git a/drivers/mmc/omap_hsmmc.c b/drivers/mmc/omap_hsmmc.c > > index ef2cbf9..ef4c5cf 100644 > > --- a/drivers/mmc/omap_hsmmc.c > > +++ b/drivers/mmc/omap_hsmmc.c > > @@ -136,7 +136,9 @@ static unsigned char mmc_board_init(struct mmc *mmc) > > pbias_lite &= ~(PBIASLITEPWRDNZ1 | PBIASLITEPWRDNZ0); > > writel(pbias_lite, &t2_base->pbias_lite); > > #endif > > -#if defined(CONFIG_TWL4030_POWER) > > +#if defined(CONFIG_OMAP_HSMMC_BOARD_POWER_INIT) > > + omap_hsmmc_board_power_init(); > > +#elif defined(CONFIG_TWL4030_POWER) > > twl4030_power_mmc_init(); > > mdelay(100); /* ramp-up delay from Linux code */ > > #endif > > >
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Hi Paul, On 10/28/14 20:11, Paul Kocialkowski wrote: > Le mardi 28 octobre 2014 à 20:02 +0200, Igor Grinberg a écrit : >> Hi Paul, >> >> On 10/28/14 19:25, Paul Kocialkowski wrote: >>> Some devices may use non-standard combinations of regulators to power MMC: >>> this allows these devices to provide a board-specific MMC power init function >>> to set everything up in their own way. >>> >>> Signed-off-by: Paul Kocialkowski <contact@paulk.fr> >>> --- >>> arch/arm/include/asm/omap_mmc.h | 4 +++- >>> drivers/mmc/omap_hsmmc.c | 4 +++- >>> 2 files changed, 6 insertions(+), 2 deletions(-) >>> >>> diff --git a/arch/arm/include/asm/omap_mmc.h b/arch/arm/include/asm/omap_mmc.h >>> index 617e22f..b6a8325 100644 >>> --- a/arch/arm/include/asm/omap_mmc.h >>> +++ b/arch/arm/include/asm/omap_mmc.h >>> @@ -164,5 +164,7 @@ struct hsmmc { >>> int omap_mmc_init(int dev_index, uint host_caps_mask, uint f_max, int cd_gpio, >>> int wp_gpio); >>> >>> - >>> +#ifdef CONFIG_OMAP_HSMMC_BOARD_POWER_INIT >> >> I'm not a huge fan of that approach, but if you add >> yet another CONFIG_ option, I think it is a requirement to add >> a documentation for it. > > That saddens me too, but I don't see how to do this in a better way for > now. > >>> +void omap_hsmmc_board_power_init(void); >> >> Anyway, I would suggest adding a default >> __weak board_mmc_power_init() or something like this >> (which would be transfered into a callback in pdata once omap_hsmmc.c is). > > Well, there are two distinct scenarios here: > 1. the board follows some reference design and VMMC1 is used to power > MMC1, VMMC2 for MMC2, then twl4030 has a function that will set > everything up correctly (with the addition I added earlier) > 2. the board uses the regulators in a more or less random way and VMMC1 > is not mapped to MMC1 power at all. This is the case on the LG Optimus > Black (P970). There is actually an auxiliary regulator IC that is in > charge of powering MMC1. Hence the need for board-specific behavior. > > I think it's perfectly fine to keep using the twl4030 function when the > board follows the reference design. > > Would that board_mmc_power_init you suggest be generic to all MMC > drivers? That would be perfect, I think. > Then at which point should it be called? What would a generic > board_mmc_power_init have? Well, those are the question you usually should answer when you write common code. I hopefully will have time to look at this next week. > TWL4030 is specific to OMAP3. Well, in practice, it is, but it does not have to be... Also, it is a question of how the board is wired... > Then perhaps > there should be a board_mmc_power_init implementation for OMAP3, that > can be overridden by a board-specific function? Or, as you have proposed already, a common board_mmc_power_init() function and a board can choose to run the twl4030_power_mmc_init()? > > I'm a bit new to the U-Boot code, so I'd appreciate pointers on what > should go where. > >> Or... just no need for this patch at all, as board_mmc_init() >> can be used for this... > > Not in the context of the SPL, which is precisely why I needed this. This might lead to a question - is current spl flexible enough to let a board deal with the mmc power? Tom? > > Thanks for your review! > >>> +#endif >>> #endif /* OMAP_MMC_H_ */ >>> diff --git a/drivers/mmc/omap_hsmmc.c b/drivers/mmc/omap_hsmmc.c >>> index ef2cbf9..ef4c5cf 100644 >>> --- a/drivers/mmc/omap_hsmmc.c >>> +++ b/drivers/mmc/omap_hsmmc.c >>> @@ -136,7 +136,9 @@ static unsigned char mmc_board_init(struct mmc *mmc) >>> pbias_lite &= ~(PBIASLITEPWRDNZ1 | PBIASLITEPWRDNZ0); >>> writel(pbias_lite, &t2_base->pbias_lite); >>> #endif >>> -#if defined(CONFIG_TWL4030_POWER) >>> +#if defined(CONFIG_OMAP_HSMMC_BOARD_POWER_INIT) >>> + omap_hsmmc_board_power_init(); >>> +#elif defined(CONFIG_TWL4030_POWER) >>> twl4030_power_mmc_init(); >>> mdelay(100); /* ramp-up delay from Linux code */ >>> #endif >>> >> > - -- Regards, Igor. -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBAgAGBQJUUOcyAAoJEBDE8YO64EfaA00P/RRjiz3AyVqGJdVcN85eRim4 oYxr2/q5jaj67zD6x6UD4eT/VSnJQXaxNEmFQVNjoZc4U6U0V7qjrTC2/USh6v6Q GROIH7zK/uP9+rRnJWJYdjAjZMJNShNmUvP9/oIb8ZtvVKG1aEuRoRWYwlyirJr/ cmtzeFS/TwaWSgFrtDWmCXuXzyKUdB+A/AOjAIj7HMZjKV+gaSZMSSyv5H6uQK+3 eI7KUskYvTJFCKKez0jnXv3IkhKjeJxy7eKQ147nfQMcZJ+dby3cbJuS3aZSG2xI jx+w6ruSm4DMYJeCsEoWhW1wFJtKrtqZ4TyYEQeC9PWoON/8b67Jy+SYTHnVBkJk qR7UFEyttUIE6zulD5rf2SlxS0e96HkLqNOfaI2K8rqbKuNOy+ZlA8TKsaVx5QAH Fsxn494TY38vgBTYQbf74pv7psw2mPT//FUF//HqTuAID9ne6pMgM+j+WE0GwWOI CZw9ddutxj1ksFu0kvnldvfZ6F4yd4aJFPUPQEDa6/qkVrKEW7Y3Alov5gHQMHQ8 zkhyv60s3owlmtcvkRtmb4LcRh+/lm8+gO9u7jmFmtiIhkCuEQA7IopqqYRdibXC SiKyGH53TTLg/zocixO1/vY/a5C2YHADaVJ6Rzzea6IDUeHBoWTKhU4vUudjOb5z NCH2ncJjDN8moX9CBENr =3a3G -----END PGP SIGNATURE-----
Hi Paul, > On Oct 28, 2014, at 19:25 , Paul Kocialkowski <contact@paulk.fr> wrote: > > Some devices may use non-standard combinations of regulators to power MMC: > this allows these devices to provide a board-specific MMC power init function > to set everything up in their own way. > > Signed-off-by: Paul Kocialkowski <contact@paulk.fr> > --- > arch/arm/include/asm/omap_mmc.h | 4 +++- > drivers/mmc/omap_hsmmc.c | 4 +++- > 2 files changed, 6 insertions(+), 2 deletions(-) > > diff --git a/arch/arm/include/asm/omap_mmc.h b/arch/arm/include/asm/omap_mmc.h > index 617e22f..b6a8325 100644 > --- a/arch/arm/include/asm/omap_mmc.h > +++ b/arch/arm/include/asm/omap_mmc.h > @@ -164,5 +164,7 @@ struct hsmmc { > int omap_mmc_init(int dev_index, uint host_caps_mask, uint f_max, int cd_gpio, > int wp_gpio); > > - > +#ifdef CONFIG_OMAP_HSMMC_BOARD_POWER_INIT > +void omap_hsmmc_board_power_init(void); > +#endif > #endif /* OMAP_MMC_H_ */ > diff --git a/drivers/mmc/omap_hsmmc.c b/drivers/mmc/omap_hsmmc.c > index ef2cbf9..ef4c5cf 100644 > --- a/drivers/mmc/omap_hsmmc.c > +++ b/drivers/mmc/omap_hsmmc.c > @@ -136,7 +136,9 @@ static unsigned char mmc_board_init(struct mmc *mmc) > pbias_lite &= ~(PBIASLITEPWRDNZ1 | PBIASLITEPWRDNZ0); > writel(pbias_lite, &t2_base->pbias_lite); > #endif > -#if defined(CONFIG_TWL4030_POWER) > +#if defined(CONFIG_OMAP_HSMMC_BOARD_POWER_INIT) > + omap_hsmmc_board_power_init(); > +#elif defined(CONFIG_TWL4030_POWER) > twl4030_power_mmc_init(); > mdelay(100); /* ramp-up delay from Linux code */ > #endif > -- > 1.7.9.5 > Igor covered most of what I was going to whine about. I’m not a big fun of the define maze here, let’s not add to it, shall we? A common board_mmc_power_init() function would be fine in this occasion. Regards — Pantelis
diff --git a/arch/arm/include/asm/omap_mmc.h b/arch/arm/include/asm/omap_mmc.h index 617e22f..b6a8325 100644 --- a/arch/arm/include/asm/omap_mmc.h +++ b/arch/arm/include/asm/omap_mmc.h @@ -164,5 +164,7 @@ struct hsmmc { int omap_mmc_init(int dev_index, uint host_caps_mask, uint f_max, int cd_gpio, int wp_gpio); - +#ifdef CONFIG_OMAP_HSMMC_BOARD_POWER_INIT +void omap_hsmmc_board_power_init(void); +#endif #endif /* OMAP_MMC_H_ */ diff --git a/drivers/mmc/omap_hsmmc.c b/drivers/mmc/omap_hsmmc.c index ef2cbf9..ef4c5cf 100644 --- a/drivers/mmc/omap_hsmmc.c +++ b/drivers/mmc/omap_hsmmc.c @@ -136,7 +136,9 @@ static unsigned char mmc_board_init(struct mmc *mmc) pbias_lite &= ~(PBIASLITEPWRDNZ1 | PBIASLITEPWRDNZ0); writel(pbias_lite, &t2_base->pbias_lite); #endif -#if defined(CONFIG_TWL4030_POWER) +#if defined(CONFIG_OMAP_HSMMC_BOARD_POWER_INIT) + omap_hsmmc_board_power_init(); +#elif defined(CONFIG_TWL4030_POWER) twl4030_power_mmc_init(); mdelay(100); /* ramp-up delay from Linux code */ #endif
Some devices may use non-standard combinations of regulators to power MMC: this allows these devices to provide a board-specific MMC power init function to set everything up in their own way. Signed-off-by: Paul Kocialkowski <contact@paulk.fr> --- arch/arm/include/asm/omap_mmc.h | 4 +++- drivers/mmc/omap_hsmmc.c | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-)