| Message ID | 1402879613-21362-4-git-send-email-otavio@ossystems.com.br |
|---|---|
| State | Changes Requested |
| Delegated to: | Stefano Babic |
| Headers | show |
On Monday, June 16, 2014 at 02:46:51 AM, Otavio Salvador wrote: > There are board were Please fix your English and send a patch, thanks :) > we cannot do voltage negotiation but want to set > the VSELECT bit forcely to ensure it to work at 1.8V. > > This commit adds CONFIG_SYS_FSL_ESDHC_FORCE_VSELECT flag for this use. > > Signed-off-by: Otavio Salvador <otavio@ossystems.com.br> > --- > > drivers/mmc/fsl_esdhc.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c > index c75b38f..b3870e2 100644 > --- a/drivers/mmc/fsl_esdhc.c > +++ b/drivers/mmc/fsl_esdhc.c > @@ -517,6 +517,10 @@ static int esdhc_init(struct mmc *mmc) > /* Set timout to the maximum value */ > esdhc_clrsetbits32(®s->sysctl, SYSCTL_TIMEOUT_MASK, 14 << 16); > > +#ifdef CONFIG_SYS_FSL_ESDHC_FORCE_VSELECT > + esdhc_setbits32(®s->vendorspec, ESDHC_VENDORSPEC_VSELECT); > +#endif Documentation is missing. Best regards, Marek Vasut
On Sun, Jun 15, 2014 at 9:51 PM, Marek Vasut <marex@denx.de> wrote: > On Monday, June 16, 2014 at 02:46:51 AM, Otavio Salvador wrote: >> There are board were > > Please fix your English and send a patch, thanks :) I fixed the commit log, thanks. ... >> +#ifdef CONFIG_SYS_FSL_ESDHC_FORCE_VSELECT >> + esdhc_setbits32(®s->vendorspec, ESDHC_VENDORSPEC_VSELECT); >> +#endif > > Documentation is missing. There is no FSL ESDHC README file so that's why I didn't include it anywhere.
On Monday, June 16, 2014 at 03:22:22 AM, Otavio Salvador wrote: [...] > >> +#ifdef CONFIG_SYS_FSL_ESDHC_FORCE_VSELECT > >> + esdhc_setbits32(®s->vendorspec, ESDHC_VENDORSPEC_VSELECT); > >> +#endif > > > > Documentation is missing. > > There is no FSL ESDHC README file so that's why I didn't include it > anywhere. I'm at loss for words here, really... I think you know what needs to be done (hint: write the documentation), right ? Best regards, Marek Vasut
On Sun, Jun 15, 2014 at 10:27 PM, Marek Vasut <marex@denx.de> wrote: > On Monday, June 16, 2014 at 03:22:22 AM, Otavio Salvador wrote: > > [...] > >> >> +#ifdef CONFIG_SYS_FSL_ESDHC_FORCE_VSELECT >> >> + esdhc_setbits32(®s->vendorspec, ESDHC_VENDORSPEC_VSELECT); >> >> +#endif >> > >> > Documentation is missing. >> >> There is no FSL ESDHC README file so that's why I didn't include it >> anywhere. > > I'm at loss for words here, really... > > I think you know what needs to be done (hint: write the documentation), right ? I won't write the full documentation for it. I am sorry.
On Monday, June 16, 2014 at 03:39:08 AM, Otavio Salvador wrote: > On Sun, Jun 15, 2014 at 10:27 PM, Marek Vasut <marex@denx.de> wrote: > > On Monday, June 16, 2014 at 03:22:22 AM, Otavio Salvador wrote: > > > > [...] > > > >> >> +#ifdef CONFIG_SYS_FSL_ESDHC_FORCE_VSELECT > >> >> + esdhc_setbits32(®s->vendorspec, ESDHC_VENDORSPEC_VSELECT); > >> >> +#endif > >> > > >> > Documentation is missing. > >> > >> There is no FSL ESDHC README file so that's why I didn't include it > >> anywhere. > > > > I'm at loss for words here, really... > > > > I think you know what needs to be done (hint: write the documentation), > > right ? > > I won't write the full documentation for it. I am sorry. Undocumented configuration option is not acceptable, period. Best regards, Marek Vasut
On Sun, Jun 15, 2014 at 11:03 PM, Marek Vasut <marex@denx.de> wrote: > On Monday, June 16, 2014 at 03:39:08 AM, Otavio Salvador wrote: >> On Sun, Jun 15, 2014 at 10:27 PM, Marek Vasut <marex@denx.de> wrote: >> > On Monday, June 16, 2014 at 03:22:22 AM, Otavio Salvador wrote: >> > >> > [...] >> > >> >> >> +#ifdef CONFIG_SYS_FSL_ESDHC_FORCE_VSELECT >> >> >> + esdhc_setbits32(®s->vendorspec, ESDHC_VENDORSPEC_VSELECT); >> >> >> +#endif >> >> > >> >> > Documentation is missing. >> >> >> >> There is no FSL ESDHC README file so that's why I didn't include it >> >> anywhere. >> > >> > I'm at loss for words here, really... >> > >> > I think you know what needs to be done (hint: write the documentation), >> > right ? >> >> I won't write the full documentation for it. I am sorry. > > Undocumented configuration option is not acceptable, period. Who accepted the driver in the first version, without Doc? I am not in the position to write the full doc.
Hi Otavio, On 06/16/14 05:24, Otavio Salvador wrote: > On Sun, Jun 15, 2014 at 11:03 PM, Marek Vasut <marex@denx.de> wrote: >> On Monday, June 16, 2014 at 03:39:08 AM, Otavio Salvador wrote: >>> On Sun, Jun 15, 2014 at 10:27 PM, Marek Vasut <marex@denx.de> wrote: >>>> On Monday, June 16, 2014 at 03:22:22 AM, Otavio Salvador wrote: >>>> >>>> [...] >>>> >>>>>>> +#ifdef CONFIG_SYS_FSL_ESDHC_FORCE_VSELECT >>>>>>> + esdhc_setbits32(®s->vendorspec, ESDHC_VENDORSPEC_VSELECT); >>>>>>> +#endif >>>>>> >>>>>> Documentation is missing. >>>>> >>>>> There is no FSL ESDHC README file so that's why I didn't include it >>>>> anywhere. >>>> >>>> I'm at loss for words here, really... >>>> >>>> I think you know what needs to be done (hint: write the documentation), >>>> right ? >>> >>> I won't write the full documentation for it. I am sorry. >> >> Undocumented configuration option is not acceptable, period. > > Who accepted the driver in the first version, without Doc? > > I am not in the position to write the full doc. I think there is a misunderstanding here... I think Marek does not want to say that you need to write the full documentation for the driver, but only document the CONFIG_SYS_FSL_ESDHC_FORCE_VSELECT configuration option (what does it do when you define it and why should one define it).
On Mon, Jun 16, 2014 at 4:03 AM, Igor Grinberg <grinberg@compulab.co.il> wrote: > Hi Otavio, > > On 06/16/14 05:24, Otavio Salvador wrote: >> On Sun, Jun 15, 2014 at 11:03 PM, Marek Vasut <marex@denx.de> wrote: >>> On Monday, June 16, 2014 at 03:39:08 AM, Otavio Salvador wrote: >>>> On Sun, Jun 15, 2014 at 10:27 PM, Marek Vasut <marex@denx.de> wrote: >>>>> On Monday, June 16, 2014 at 03:22:22 AM, Otavio Salvador wrote: >>>>> >>>>> [...] >>>>> >>>>>>>> +#ifdef CONFIG_SYS_FSL_ESDHC_FORCE_VSELECT >>>>>>>> + esdhc_setbits32(®s->vendorspec, ESDHC_VENDORSPEC_VSELECT); >>>>>>>> +#endif >>>>>>> >>>>>>> Documentation is missing. >>>>>> >>>>>> There is no FSL ESDHC README file so that's why I didn't include it >>>>>> anywhere. >>>>> >>>>> I'm at loss for words here, really... >>>>> >>>>> I think you know what needs to be done (hint: write the documentation), >>>>> right ? >>>> >>>> I won't write the full documentation for it. I am sorry. >>> >>> Undocumented configuration option is not acceptable, period. >> >> Who accepted the driver in the first version, without Doc? >> >> I am not in the position to write the full doc. > > I think there is a misunderstanding here... > I think Marek does not want to say that you need to write the full > documentation for the driver, but only document the CONFIG_SYS_FSL_ESDHC_FORCE_VSELECT configuration option (what does it > do when you define it and why should one define it). Great but where if it does not exist? should I make a README.fsl-esdhc and include just it?
On Monday, June 16, 2014 at 01:48:11 PM, Otavio Salvador wrote: > On Mon, Jun 16, 2014 at 4:03 AM, Igor Grinberg <grinberg@compulab.co.il> wrote: > > Hi Otavio, > > > > On 06/16/14 05:24, Otavio Salvador wrote: > >> On Sun, Jun 15, 2014 at 11:03 PM, Marek Vasut <marex@denx.de> wrote: > >>> On Monday, June 16, 2014 at 03:39:08 AM, Otavio Salvador wrote: > >>>> On Sun, Jun 15, 2014 at 10:27 PM, Marek Vasut <marex@denx.de> wrote: > >>>>> On Monday, June 16, 2014 at 03:22:22 AM, Otavio Salvador wrote: > >>>>> > >>>>> [...] > >>>>> > >>>>>>>> +#ifdef CONFIG_SYS_FSL_ESDHC_FORCE_VSELECT > >>>>>>>> + esdhc_setbits32(®s->vendorspec, > >>>>>>>> ESDHC_VENDORSPEC_VSELECT); +#endif > >>>>>>> > >>>>>>> Documentation is missing. > >>>>>> > >>>>>> There is no FSL ESDHC README file so that's why I didn't include it > >>>>>> anywhere. > >>>>> > >>>>> I'm at loss for words here, really... > >>>>> > >>>>> I think you know what needs to be done (hint: write the > >>>>> documentation), right ? > >>>> > >>>> I won't write the full documentation for it. I am sorry. > >>> > >>> Undocumented configuration option is not acceptable, period. > >> > >> Who accepted the driver in the first version, without Doc? > >> > >> I am not in the position to write the full doc. > > > > I think there is a misunderstanding here... > > I think Marek does not want to say that you need to write the full > > documentation for the driver, but only document the > > CONFIG_SYS_FSL_ESDHC_FORCE_VSELECT configuration option (what does it do > > when you define it and why should one define it). > > Great but where if it does not exist? > > should I make a README.fsl-esdhc and include just it? I briefly looked over the options in the driver, prefixed with hash are the ones which are not driver specific: CONFIG_ESDHC_DETECT_8_BIT_QUIRK CONFIG_ESDHC_DETECT_QUIRK CONFIG_FSL_ESDHC_PIN_MUX CONFIG_FSL_USDHC #CONFIG_MX53 #CONFIG_OF_LIBFDT CONFIG_SYS_FSL_ERRATUM_ESDHC111 CONFIG_SYS_FSL_ERRATUM_ESDHC135 CONFIG_SYS_FSL_ERRATUM_ESDHC_A001 CONFIG_SYS_FSL_ESDHC_ADDR CONFIG_SYS_FSL_ESDHC_USE_PIO CONFIG_SYS_FSL_MMC_HAS_CAPBLT_VS33 #CONFIG_SYS_MMC_MAX_BLK_COUNT #CONFIG_SYS_SD_VOLTAGE #CONFIG_T4240QDS It looks like completely ad-hoc addition of new and new config options as whoever seen fit to me, both from their names and git log of the driver. This makes the driver completely unmaintainable. I would like to see them documented before any new config options are added, so please instead of fighting the mailing list, spend 10 minutes of your time and document them. Best regards, Marek Vasut
Hi Marek Il 17/giu/2014 08:03 "Marek Vasut" <marex@denx.de> ha scritto: > > On Monday, June 16, 2014 at 01:48:11 PM, Otavio Salvador wrote: > > On Mon, Jun 16, 2014 at 4:03 AM, Igor Grinberg <grinberg@compulab.co.il> > wrote: > > > Hi Otavio, > > > > > > On 06/16/14 05:24, Otavio Salvador wrote: > > >> On Sun, Jun 15, 2014 at 11:03 PM, Marek Vasut <marex@denx.de> wrote: > > >>> On Monday, June 16, 2014 at 03:39:08 AM, Otavio Salvador wrote: > > >>>> On Sun, Jun 15, 2014 at 10:27 PM, Marek Vasut <marex@denx.de> wrote: > > >>>>> On Monday, June 16, 2014 at 03:22:22 AM, Otavio Salvador wrote: > > >>>>> > > >>>>> [...] > > >>>>> > > >>>>>>>> +#ifdef CONFIG_SYS_FSL_ESDHC_FORCE_VSELECT > > >>>>>>>> + esdhc_setbits32(®s->vendorspec, > > >>>>>>>> ESDHC_VENDORSPEC_VSELECT); +#endif > > >>>>>>> > > >>>>>>> Documentation is missing. > > >>>>>> > > >>>>>> There is no FSL ESDHC README file so that's why I didn't include it > > >>>>>> anywhere. > > >>>>> > > >>>>> I'm at loss for words here, really... > > >>>>> > > >>>>> I think you know what needs to be done (hint: write the > > >>>>> documentation), right ? > > >>>> > > >>>> I won't write the full documentation for it. I am sorry. > > >>> > > >>> Undocumented configuration option is not acceptable, period. > > >> > > >> Who accepted the driver in the first version, without Doc? > > >> > > >> I am not in the position to write the full doc. > > > > > > I think there is a misunderstanding here... > > > I think Marek does not want to say that you need to write the full > > > documentation for the driver, but only document the > > > CONFIG_SYS_FSL_ESDHC_FORCE_VSELECT configuration option (what does it do > > > when you define it and why should one define it). > > > > Great but where if it does not exist? > > > > should I make a README.fsl-esdhc and include just it? > > I briefly looked over the options in the driver, prefixed with hash are the ones > which are not driver specific: > > CONFIG_ESDHC_DETECT_8_BIT_QUIRK > CONFIG_ESDHC_DETECT_QUIRK > CONFIG_FSL_ESDHC_PIN_MUX > CONFIG_FSL_USDHC > #CONFIG_MX53 > #CONFIG_OF_LIBFDT > CONFIG_SYS_FSL_ERRATUM_ESDHC111 > CONFIG_SYS_FSL_ERRATUM_ESDHC135 > CONFIG_SYS_FSL_ERRATUM_ESDHC_A001 > CONFIG_SYS_FSL_ESDHC_ADDR > CONFIG_SYS_FSL_ESDHC_USE_PIO > CONFIG_SYS_FSL_MMC_HAS_CAPBLT_VS33 > #CONFIG_SYS_MMC_MAX_BLK_COUNT > #CONFIG_SYS_SD_VOLTAGE > #CONFIG_T4240QDS > > It looks like completely ad-hoc addition of new and new config options as > whoever seen fit to me, both from their names and git log of the driver. This > makes the driver completely unmaintainable. I would like to see them documented Do you think that could be a better option to use flags and runtime detect instead having hundred of define? Michael > before any new config options are added, so please instead of fighting the > mailing list, spend 10 minutes of your time and document them. > > Best regards, > Marek Vasut > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot
Hi Otavio, On 06/16/14 14:48, Otavio Salvador wrote: > On Mon, Jun 16, 2014 at 4:03 AM, Igor Grinberg <grinberg@compulab.co.il> wrote: >> Hi Otavio, >> >> On 06/16/14 05:24, Otavio Salvador wrote: >>> On Sun, Jun 15, 2014 at 11:03 PM, Marek Vasut <marex@denx.de> wrote: >>>> On Monday, June 16, 2014 at 03:39:08 AM, Otavio Salvador wrote: >>>>> On Sun, Jun 15, 2014 at 10:27 PM, Marek Vasut <marex@denx.de> wrote: >>>>>> On Monday, June 16, 2014 at 03:22:22 AM, Otavio Salvador wrote: >>>>>> >>>>>> [...] >>>>>> >>>>>>>>> +#ifdef CONFIG_SYS_FSL_ESDHC_FORCE_VSELECT >>>>>>>>> + esdhc_setbits32(®s->vendorspec, ESDHC_VENDORSPEC_VSELECT); >>>>>>>>> +#endif >>>>>>>> >>>>>>>> Documentation is missing. >>>>>>> >>>>>>> There is no FSL ESDHC README file so that's why I didn't include it >>>>>>> anywhere. >>>>>> >>>>>> I'm at loss for words here, really... >>>>>> >>>>>> I think you know what needs to be done (hint: write the documentation), >>>>>> right ? >>>>> >>>>> I won't write the full documentation for it. I am sorry. >>>> >>>> Undocumented configuration option is not acceptable, period. >>> >>> Who accepted the driver in the first version, without Doc? >>> >>> I am not in the position to write the full doc. >> >> I think there is a misunderstanding here... >> I think Marek does not want to say that you need to write the full >> documentation for the driver, but only document the CONFIG_SYS_FSL_ESDHC_FORCE_VSELECT configuration option (what does it >> do when you define it and why should one define it). > > Great but where if it does not exist? > > should I make a README.fsl-esdhc and include just it? Hmmm... May be. I would make a decision and start something, then send an RFC. Writing some words should not be hard especially for configuration options that you introduce yourself, but if you find some stuff currently hard or time consuming for some reason, a "TODO: ..." might be an acceptable compromise.
Hi Otavio, On 16/06/2014 02:46, Otavio Salvador wrote: > There are board were we cannot do voltage negotiation but want to set > the VSELECT bit forcely to ensure it to work at 1.8V. > > This commit adds CONFIG_SYS_FSL_ESDHC_FORCE_VSELECT flag for this use. > > Signed-off-by: Otavio Salvador <otavio@ossystems.com.br> > --- > > drivers/mmc/fsl_esdhc.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c > index c75b38f..b3870e2 100644 > --- a/drivers/mmc/fsl_esdhc.c > +++ b/drivers/mmc/fsl_esdhc.c > @@ -517,6 +517,10 @@ static int esdhc_init(struct mmc *mmc) > /* Set timout to the maximum value */ > esdhc_clrsetbits32(®s->sysctl, SYSCTL_TIMEOUT_MASK, 14 << 16); > > +#ifdef CONFIG_SYS_FSL_ESDHC_FORCE_VSELECT > + esdhc_setbits32(®s->vendorspec, ESDHC_VENDORSPEC_VSELECT); > +#endif Instead of adding a new compiler switch that should be documented (I have already read Marek's comments), what do you think to extend struct fsl_esdhc_cfg, putting for exmaple an "options" field with this kind of specialization ? I see also a CONFIG_SYS_FSL_ESDHC_USE_PIO that slipped into mainline (you are both right : Otavio is not the first to add such kind of configuration switches that are still undocumented, but according to rules should be documented as Marek said). I would suggest to get rid as much as possible with these CONFIG_ switches. If we have some specialization for this driver before calling esdhc_init (better: fsl_esdhc_initialize), they are self explaining and we do not need further documentation. What do you think ? Best regards, Stefano
On Tue, Jun 17, 2014 at 12:11 PM, Stefano Babic <sbabic@denx.de> wrote: > On 16/06/2014 02:46, Otavio Salvador wrote: >> There are board were we cannot do voltage negotiation but want to set >> the VSELECT bit forcely to ensure it to work at 1.8V. >> >> This commit adds CONFIG_SYS_FSL_ESDHC_FORCE_VSELECT flag for this use. >> >> Signed-off-by: Otavio Salvador <otavio@ossystems.com.br> >> --- >> >> drivers/mmc/fsl_esdhc.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c >> index c75b38f..b3870e2 100644 >> --- a/drivers/mmc/fsl_esdhc.c >> +++ b/drivers/mmc/fsl_esdhc.c >> @@ -517,6 +517,10 @@ static int esdhc_init(struct mmc *mmc) >> /* Set timout to the maximum value */ >> esdhc_clrsetbits32(®s->sysctl, SYSCTL_TIMEOUT_MASK, 14 << 16); >> >> +#ifdef CONFIG_SYS_FSL_ESDHC_FORCE_VSELECT >> + esdhc_setbits32(®s->vendorspec, ESDHC_VENDORSPEC_VSELECT); >> +#endif > > Instead of adding a new compiler switch that should be documented (I > have already read Marek's comments), what do you think to extend struct > fsl_esdhc_cfg, putting for exmaple an "options" field with this kind of > specialization ? I will try to cook something in this direction.
Hi Stefano On Tue, Jun 17, 2014 at 5:12 PM, Otavio Salvador <otavio@ossystems.com.br> wrote: > On Tue, Jun 17, 2014 at 12:11 PM, Stefano Babic <sbabic@denx.de> wrote: >> On 16/06/2014 02:46, Otavio Salvador wrote: >>> There are board were we cannot do voltage negotiation but want to set >>> the VSELECT bit forcely to ensure it to work at 1.8V. >>> >>> This commit adds CONFIG_SYS_FSL_ESDHC_FORCE_VSELECT flag for this use. >>> >>> Signed-off-by: Otavio Salvador <otavio@ossystems.com.br> >>> --- >>> >>> drivers/mmc/fsl_esdhc.c | 4 ++++ >>> 1 file changed, 4 insertions(+) >>> >>> diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c >>> index c75b38f..b3870e2 100644 >>> --- a/drivers/mmc/fsl_esdhc.c >>> +++ b/drivers/mmc/fsl_esdhc.c >>> @@ -517,6 +517,10 @@ static int esdhc_init(struct mmc *mmc) >>> /* Set timout to the maximum value */ >>> esdhc_clrsetbits32(®s->sysctl, SYSCTL_TIMEOUT_MASK, 14 << 16); >>> >>> +#ifdef CONFIG_SYS_FSL_ESDHC_FORCE_VSELECT >>> + esdhc_setbits32(®s->vendorspec, ESDHC_VENDORSPEC_VSELECT); >>> +#endif >> >> Instead of adding a new compiler switch that should be documented (I >> have already read Marek's comments), what do you think to extend struct >> fsl_esdhc_cfg, putting for exmaple an "options" field with this kind of >> specialization ? > > I will try to cook something in this direction. > Well I already said some emails ago ;). Michael > -- > Otavio Salvador O.S. Systems > http://www.ossystems.com.br http://code.ossystems.com.br > Mobile: +55 (53) 9981-7854 Mobile: +1 (347) 903-9750 > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot
Hi Michael, On 17/06/2014 17:14, Michael Trimarchi wrote: >>> Instead of adding a new compiler switch that should be documented (I >>> have already read Marek's comments), what do you think to extend struct >>> fsl_esdhc_cfg, putting for exmaple an "options" field with this kind of >>> specialization ? >> >> I will try to cook something in this direction. >> > > Well I already said some emails ago ;). Nice I am not alone to think it in this way :-). Sorry, I have not read your answer, I was dropped from the CC list and I missed that you have already said the same. Best regards, Stefano
On Tuesday, June 17, 2014 at 08:06:38 AM, Michael Trimarchi wrote: > Hi Marek > > Il 17/giu/2014 08:03 "Marek Vasut" <marex@denx.de> ha scritto: > > On Monday, June 16, 2014 at 01:48:11 PM, Otavio Salvador wrote: > > > On Mon, Jun 16, 2014 at 4:03 AM, Igor Grinberg > > > <grinberg@compulab.co.il> > > > > wrote: > > > > Hi Otavio, > > > > > > > > On 06/16/14 05:24, Otavio Salvador wrote: > > > >> On Sun, Jun 15, 2014 at 11:03 PM, Marek Vasut <marex@denx.de> wrote: > > > >>> On Monday, June 16, 2014 at 03:39:08 AM, Otavio Salvador wrote: > > > >>>> On Sun, Jun 15, 2014 at 10:27 PM, Marek Vasut <marex@denx.de> > > wrote: > > > >>>>> On Monday, June 16, 2014 at 03:22:22 AM, Otavio Salvador wrote: > > > >>>>> > > > >>>>> [...] > > > >>>>> > > > >>>>>>>> +#ifdef CONFIG_SYS_FSL_ESDHC_FORCE_VSELECT > > > >>>>>>>> + esdhc_setbits32(®s->vendorspec, > > > >>>>>>>> ESDHC_VENDORSPEC_VSELECT); +#endif > > > >>>>>>> > > > >>>>>>> Documentation is missing. > > > >>>>>> > > > >>>>>> There is no FSL ESDHC README file so that's why I didn't include > > it > > > > >>>>>> anywhere. > > > >>>>> > > > >>>>> I'm at loss for words here, really... > > > >>>>> > > > >>>>> I think you know what needs to be done (hint: write the > > > >>>>> documentation), right ? > > > >>>> > > > >>>> I won't write the full documentation for it. I am sorry. > > > >>> > > > >>> Undocumented configuration option is not acceptable, period. > > > >> > > > >> Who accepted the driver in the first version, without Doc? > > > >> > > > >> I am not in the position to write the full doc. > > > > > > > > I think there is a misunderstanding here... > > > > I think Marek does not want to say that you need to write the full > > > > documentation for the driver, but only document the > > > > CONFIG_SYS_FSL_ESDHC_FORCE_VSELECT configuration option (what does it > > do > > > > > when you define it and why should one define it). > > > > > > Great but where if it does not exist? > > > > > > should I make a README.fsl-esdhc and include just it? > > > > I briefly looked over the options in the driver, prefixed with hash are > > the ones > > > which are not driver specific: > > > > CONFIG_ESDHC_DETECT_8_BIT_QUIRK > > CONFIG_ESDHC_DETECT_QUIRK > > CONFIG_FSL_ESDHC_PIN_MUX > > CONFIG_FSL_USDHC > > #CONFIG_MX53 > > #CONFIG_OF_LIBFDT > > CONFIG_SYS_FSL_ERRATUM_ESDHC111 > > CONFIG_SYS_FSL_ERRATUM_ESDHC135 > > CONFIG_SYS_FSL_ERRATUM_ESDHC_A001 > > CONFIG_SYS_FSL_ESDHC_ADDR > > CONFIG_SYS_FSL_ESDHC_USE_PIO > > CONFIG_SYS_FSL_MMC_HAS_CAPBLT_VS33 > > #CONFIG_SYS_MMC_MAX_BLK_COUNT > > #CONFIG_SYS_SD_VOLTAGE > > #CONFIG_T4240QDS > > > > It looks like completely ad-hoc addition of new and new config options as > > whoever seen fit to me, both from their names and git log of the driver. > > This > > > makes the driver completely unmaintainable. I would like to see them > > documented > > Do you think that could be a better option to use flags and runtime detect > instead having hundred of define? Eventually, when we transition to DT, this will be indeed needed anyway. But before that, we need to understand what those flags do (which we don't because previous patches neglected adding proper documentation).
Hi On Tue, Jun 17, 2014 at 5:49 PM, Marek Vasut <marex@denx.de> wrote: > On Tuesday, June 17, 2014 at 08:06:38 AM, Michael Trimarchi wrote: >> Hi Marek >> >> Il 17/giu/2014 08:03 "Marek Vasut" <marex@denx.de> ha scritto: >> > On Monday, June 16, 2014 at 01:48:11 PM, Otavio Salvador wrote: >> > > On Mon, Jun 16, 2014 at 4:03 AM, Igor Grinberg >> > > <grinberg@compulab.co.il> >> > >> > wrote: >> > > > Hi Otavio, >> > > > >> > > > On 06/16/14 05:24, Otavio Salvador wrote: >> > > >> On Sun, Jun 15, 2014 at 11:03 PM, Marek Vasut <marex@denx.de> wrote: >> > > >>> On Monday, June 16, 2014 at 03:39:08 AM, Otavio Salvador wrote: >> > > >>>> On Sun, Jun 15, 2014 at 10:27 PM, Marek Vasut <marex@denx.de> >> >> wrote: >> > > >>>>> On Monday, June 16, 2014 at 03:22:22 AM, Otavio Salvador wrote: >> > > >>>>> >> > > >>>>> [...] >> > > >>>>> >> > > >>>>>>>> +#ifdef CONFIG_SYS_FSL_ESDHC_FORCE_VSELECT >> > > >>>>>>>> + esdhc_setbits32(®s->vendorspec, >> > > >>>>>>>> ESDHC_VENDORSPEC_VSELECT); +#endif >> > > >>>>>>> >> > > >>>>>>> Documentation is missing. >> > > >>>>>> >> > > >>>>>> There is no FSL ESDHC README file so that's why I didn't include >> >> it >> >> > > >>>>>> anywhere. >> > > >>>>> >> > > >>>>> I'm at loss for words here, really... >> > > >>>>> >> > > >>>>> I think you know what needs to be done (hint: write the >> > > >>>>> documentation), right ? >> > > >>>> >> > > >>>> I won't write the full documentation for it. I am sorry. >> > > >>> >> > > >>> Undocumented configuration option is not acceptable, period. >> > > >> >> > > >> Who accepted the driver in the first version, without Doc? >> > > >> >> > > >> I am not in the position to write the full doc. >> > > > >> > > > I think there is a misunderstanding here... >> > > > I think Marek does not want to say that you need to write the full >> > > > documentation for the driver, but only document the >> > > > CONFIG_SYS_FSL_ESDHC_FORCE_VSELECT configuration option (what does it >> >> do >> >> > > > when you define it and why should one define it). >> > > >> > > Great but where if it does not exist? >> > > >> > > should I make a README.fsl-esdhc and include just it? >> > >> > I briefly looked over the options in the driver, prefixed with hash are >> >> the ones >> >> > which are not driver specific: >> > >> > CONFIG_ESDHC_DETECT_8_BIT_QUIRK >> > CONFIG_ESDHC_DETECT_QUIRK >> > CONFIG_FSL_ESDHC_PIN_MUX >> > CONFIG_FSL_USDHC >> > #CONFIG_MX53 >> > #CONFIG_OF_LIBFDT >> > CONFIG_SYS_FSL_ERRATUM_ESDHC111 >> > CONFIG_SYS_FSL_ERRATUM_ESDHC135 >> > CONFIG_SYS_FSL_ERRATUM_ESDHC_A001 >> > CONFIG_SYS_FSL_ESDHC_ADDR >> > CONFIG_SYS_FSL_ESDHC_USE_PIO >> > CONFIG_SYS_FSL_MMC_HAS_CAPBLT_VS33 >> > #CONFIG_SYS_MMC_MAX_BLK_COUNT >> > #CONFIG_SYS_SD_VOLTAGE >> > #CONFIG_T4240QDS >> > >> > It looks like completely ad-hoc addition of new and new config options as >> > whoever seen fit to me, both from their names and git log of the driver. >> >> This >> >> > makes the driver completely unmaintainable. I would like to see them >> >> documented >> >> Do you think that could be a better option to use flags and runtime detect >> instead having hundred of define? > > Eventually, when we transition to DT, this will be indeed needed anyway. But > before that, we need to understand what those flags do (which we don't because > previous patches neglected adding proper documentation). I suggest as Stefano said to add this option/flags now in preparation of DT transition and start to document flags as you said. Michael
diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c index c75b38f..b3870e2 100644 --- a/drivers/mmc/fsl_esdhc.c +++ b/drivers/mmc/fsl_esdhc.c @@ -517,6 +517,10 @@ static int esdhc_init(struct mmc *mmc) /* Set timout to the maximum value */ esdhc_clrsetbits32(®s->sysctl, SYSCTL_TIMEOUT_MASK, 14 << 16); +#ifdef CONFIG_SYS_FSL_ESDHC_FORCE_VSELECT + esdhc_setbits32(®s->vendorspec, ESDHC_VENDORSPEC_VSELECT); +#endif + return 0; }
There are board were we cannot do voltage negotiation but want to set the VSELECT bit forcely to ensure it to work at 1.8V. This commit adds CONFIG_SYS_FSL_ESDHC_FORCE_VSELECT flag for this use. Signed-off-by: Otavio Salvador <otavio@ossystems.com.br> --- drivers/mmc/fsl_esdhc.c | 4 ++++ 1 file changed, 4 insertions(+)