| Message ID | 1321094190-8108-6-git-send-email-jason.hui@linaro.org |
|---|---|
| State | Changes Requested |
| Headers | show |
> The mmc host controller on the i.mx6q is called usdhc which > is redesigned based on the freescale esdhc controller. > > The usdhc controller is almost compatible with esdhc except > it adds one misc control register from user using experience. > > Signed-off-by: Jason Liu <jason.hui@linaro.org> > --- > drivers/mmc/fsl_esdhc.c | 14 +++++++++++++- > 1 files changed, 13 insertions(+), 1 deletions(-) > > diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c > index ec953f0..cd17ef2 100644 > --- a/drivers/mmc/fsl_esdhc.c > +++ b/drivers/mmc/fsl_esdhc.c > @@ -58,7 +58,12 @@ struct fsl_esdhc { > uint autoc12err; > uint hostcapblt; > uint wml; > - char reserved1[8]; > +#if defined(CONFIG_FSL_USDHC) > + uint mixctrl; > + char reserved1[4]; > +#else > + char reserved1[8]; > +#endif Hi Jason, can't we just drop this ifdef ? > uint fevt; > char reserved2[168]; > uint hostver; > @@ -298,6 +303,9 @@ esdhc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, > struct mmc_data *data) > > /* Send the command */ > esdhc_write32(®s->cmdarg, cmd->cmdarg); > +#if defined(CONFIG_FSL_USDHC) > + esdhc_write32(®s->mixctrl, xfertyp & 0xFFFF); > +#endif > esdhc_write32(®s->xfertyp, xfertyp); Why is this duplicated? This seems like a huge user-experience nonsense to me :) If you write to xfertyp register, you still have to write the same thing to mixctrl? Or if you do it vice versa, won't it work ? Why did you add the register? > > /* Wait for the command to complete */ > @@ -482,7 +490,11 @@ int fsl_esdhc_initialize(bd_t *bis, struct > fsl_esdhc_cfg *cfg) > > mmc = malloc(sizeof(struct mmc)); > > +#if defined(CONFIG_FSL_USDHC) > + sprintf(mmc->name, "FSL_USDHC"); > +#else > sprintf(mmc->name, "FSL_ESDHC"); > +#endif Why not just rename it to FSL_SDHC and be done with it ? > regs = (struct fsl_esdhc *)cfg->esdhc_base; > > /* First reset the eSDHC controller */
On Sun, Nov 13, 2011 at 12:35 AM, Marek Vasut <marek.vasut@gmail.com> wrote: >> The mmc host controller on the i.mx6q is called usdhc which >> is redesigned based on the freescale esdhc controller. >> >> The usdhc controller is almost compatible with esdhc except >> it adds one misc control register from user using experience. >> >> Signed-off-by: Jason Liu <jason.hui@linaro.org> >> --- >> drivers/mmc/fsl_esdhc.c | 14 +++++++++++++- >> 1 files changed, 13 insertions(+), 1 deletions(-) >> >> diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c >> index ec953f0..cd17ef2 100644 >> --- a/drivers/mmc/fsl_esdhc.c >> +++ b/drivers/mmc/fsl_esdhc.c >> @@ -58,7 +58,12 @@ struct fsl_esdhc { >> uint autoc12err; >> uint hostcapblt; >> uint wml; >> - char reserved1[8]; >> +#if defined(CONFIG_FSL_USDHC) >> + uint mixctrl; >> + char reserved1[4]; >> +#else >> + char reserved1[8]; >> +#endif > > Hi Jason, > > can't we just drop this ifdef ? Yes, I think we can. I will drop it later. > >> uint fevt; >> char reserved2[168]; >> uint hostver; >> @@ -298,6 +303,9 @@ esdhc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, >> struct mmc_data *data) >> >> /* Send the command */ >> esdhc_write32(®s->cmdarg, cmd->cmdarg); >> +#if defined(CONFIG_FSL_USDHC) >> + esdhc_write32(®s->mixctrl, xfertyp & 0xFFFF); >> +#endif >> esdhc_write32(®s->xfertyp, xfertyp); > > Why is this duplicated? This seems like a huge user-experience nonsense to me :) > > If you write to xfertyp register, you still have to write the same thing to > mixctrl? Or if you do it vice versa, won't it work ? Why did you add the > register? This is due to that on i.mx6q usdhc, it adds one extra mixctrl(offset 0x48) register to accommodate the low 16-bits of tranfertype and the high 16-bits of tranfertype will still go via the xfertyp register. On the i.mx6q usdhc, the xfertyp(offset 0xc) register, the low 16-bits is not used and only high 16-bits have been used. I can make the above code clear as the followings, #if defined(CONFIG_FSL_USDHC) esdhc_write32(®s->mixctrl, xfertyp & 0x0000FFFF); esdhc_write32(®s->xfertyp, xfertyp & 0xFFFF0000); #else esdhc_write32(®s->xfertyp, xfertyp); #endif > >> >> /* Wait for the command to complete */ >> @@ -482,7 +490,11 @@ int fsl_esdhc_initialize(bd_t *bis, struct >> fsl_esdhc_cfg *cfg) >> >> mmc = malloc(sizeof(struct mmc)); >> >> +#if defined(CONFIG_FSL_USDHC) >> + sprintf(mmc->name, "FSL_USDHC"); >> +#else >> sprintf(mmc->name, "FSL_ESDHC"); >> +#endif > > Why not just rename it to FSL_SDHC and be done with it ? OK, I will do it. > >> regs = (struct fsl_esdhc *)cfg->esdhc_base; >> >> /* First reset the eSDHC controller */ > Jason Liu
On 11/12/2011 11:36 AM, Jason Liu wrote: > The mmc host controller on the i.mx6q is called usdhc which > is redesigned based on the freescale esdhc controller. > > The usdhc controller is almost compatible with esdhc except > it adds one misc control register from user using experience. > > Signed-off-by: Jason Liu <jason.hui@linaro.org> > --- Hi Jason, I put Andy in CC (MMC custodian). > drivers/mmc/fsl_esdhc.c | 14 +++++++++++++- > 1 files changed, 13 insertions(+), 1 deletions(-) > > diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c > index ec953f0..cd17ef2 100644 > --- a/drivers/mmc/fsl_esdhc.c > +++ b/drivers/mmc/fsl_esdhc.c > @@ -58,7 +58,12 @@ struct fsl_esdhc { > uint autoc12err; > uint hostcapblt; > uint wml; > - char reserved1[8]; > +#if defined(CONFIG_FSL_USDHC) > + uint mixctrl; > + char reserved1[4]; > +#else > + char reserved1[8]; > +#endif > uint fevt; > char reserved2[168]; > uint hostver; > @@ -298,6 +303,9 @@ esdhc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data) > > /* Send the command */ > esdhc_write32(®s->cmdarg, cmd->cmdarg); > +#if defined(CONFIG_FSL_USDHC) > + esdhc_write32(®s->mixctrl, xfertyp & 0xFFFF); > +#endif > esdhc_write32(®s->xfertyp, xfertyp); > > /* Wait for the command to complete */ > @@ -482,7 +490,11 @@ int fsl_esdhc_initialize(bd_t *bis, struct fsl_esdhc_cfg *cfg) > > mmc = malloc(sizeof(struct mmc)); > > +#if defined(CONFIG_FSL_USDHC) > + sprintf(mmc->name, "FSL_USDHC"); > +#else > sprintf(mmc->name, "FSL_ESDHC"); > +#endif Acked-by: Stefano Babic <sbabic@denx.de> Best regards, Stefano Babic
> On Sun, Nov 13, 2011 at 12:35 AM, Marek Vasut <marek.vasut@gmail.com> wrote: > >> The mmc host controller on the i.mx6q is called usdhc which > >> is redesigned based on the freescale esdhc controller. > >> > >> The usdhc controller is almost compatible with esdhc except > >> it adds one misc control register from user using experience. > >> > >> Signed-off-by: Jason Liu <jason.hui@linaro.org> > >> --- > >> drivers/mmc/fsl_esdhc.c | 14 +++++++++++++- > >> 1 files changed, 13 insertions(+), 1 deletions(-) > >> > >> diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c > >> index ec953f0..cd17ef2 100644 > >> --- a/drivers/mmc/fsl_esdhc.c > >> +++ b/drivers/mmc/fsl_esdhc.c > >> @@ -58,7 +58,12 @@ struct fsl_esdhc { > >> uint autoc12err; > >> uint hostcapblt; > >> uint wml; > >> - char reserved1[8]; > >> +#if defined(CONFIG_FSL_USDHC) > >> + uint mixctrl; > >> + char reserved1[4]; > >> +#else > >> + char reserved1[8]; > >> +#endif > > > > Hi Jason, > > > > can't we just drop this ifdef ? > > Yes, I think we can. I will drop it later. Please drop now so it's not forgotten ;-) > > >> uint fevt; > >> char reserved2[168]; > >> uint hostver; > >> @@ -298,6 +303,9 @@ esdhc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, > >> struct mmc_data *data) > >> > >> /* Send the command */ > >> esdhc_write32(®s->cmdarg, cmd->cmdarg); > >> +#if defined(CONFIG_FSL_USDHC) > >> + esdhc_write32(®s->mixctrl, xfertyp & 0xFFFF); > >> +#endif > >> esdhc_write32(®s->xfertyp, xfertyp); > > > > Why is this duplicated? This seems like a huge user-experience nonsense > > to me :) > > > > If you write to xfertyp register, you still have to write the same thing > > to mixctrl? Or if you do it vice versa, won't it work ? Why did you add > > the register? > > This is due to that on i.mx6q usdhc, it adds one extra mixctrl(offset > 0x48) register to accommodate the > low 16-bits of tranfertype and the high 16-bits of tranfertype will > still go via the xfertyp register. > On the i.mx6q usdhc, the xfertyp(offset 0xc) register, the low 16-bits > is not used and only > high 16-bits have been used. I can make the above code clear as the > followings, > > #if defined(CONFIG_FSL_USDHC) > esdhc_write32(®s->mixctrl, xfertyp & 0x0000FFFF); > esdhc_write32(®s->xfertyp, xfertyp & 0xFFFF0000); > #else > esdhc_write32(®s->xfertyp, xfertyp); > #endif Well this is insane, is the datasheet for mx6q available already so I can check for myself? > > >> /* Wait for the command to complete */ > >> @@ -482,7 +490,11 @@ int fsl_esdhc_initialize(bd_t *bis, struct > >> fsl_esdhc_cfg *cfg) > >> > >> mmc = malloc(sizeof(struct mmc)); > >> > >> +#if defined(CONFIG_FSL_USDHC) > >> + sprintf(mmc->name, "FSL_USDHC"); > >> +#else > >> sprintf(mmc->name, "FSL_ESDHC"); > >> +#endif > > > > Why not just rename it to FSL_SDHC and be done with it ? > > OK, I will do it. > > >> regs = (struct fsl_esdhc *)cfg->esdhc_base; > >> > >> /* First reset the eSDHC controller */ > > Jason Liu
On Mon, Nov 14, 2011 at 6:42 PM, Marek Vasut <marek.vasut@gmail.com> wrote: >> On Sun, Nov 13, 2011 at 12:35 AM, Marek Vasut <marek.vasut@gmail.com> wrote: >> >> The mmc host controller on the i.mx6q is called usdhc which >> >> is redesigned based on the freescale esdhc controller. >> >> >> >> The usdhc controller is almost compatible with esdhc except >> >> it adds one misc control register from user using experience. >> >> >> >> Signed-off-by: Jason Liu <jason.hui@linaro.org> >> >> --- >> >> drivers/mmc/fsl_esdhc.c | 14 +++++++++++++- >> >> 1 files changed, 13 insertions(+), 1 deletions(-) >> >> >> >> diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c >> >> index ec953f0..cd17ef2 100644 >> >> --- a/drivers/mmc/fsl_esdhc.c >> >> +++ b/drivers/mmc/fsl_esdhc.c >> >> @@ -58,7 +58,12 @@ struct fsl_esdhc { >> >> uint autoc12err; >> >> uint hostcapblt; >> >> uint wml; >> >> - char reserved1[8]; >> >> +#if defined(CONFIG_FSL_USDHC) >> >> + uint mixctrl; >> >> + char reserved1[4]; >> >> +#else >> >> + char reserved1[8]; >> >> +#endif >> > >> > Hi Jason, >> > >> > can't we just drop this ifdef ? >> >> Yes, I think we can. I will drop it later. > > Please drop now so it's not forgotten ;-) > >> >> >> uint fevt; >> >> char reserved2[168]; >> >> uint hostver; >> >> @@ -298,6 +303,9 @@ esdhc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, >> >> struct mmc_data *data) >> >> >> >> /* Send the command */ >> >> esdhc_write32(®s->cmdarg, cmd->cmdarg); >> >> +#if defined(CONFIG_FSL_USDHC) >> >> + esdhc_write32(®s->mixctrl, xfertyp & 0xFFFF); >> >> +#endif >> >> esdhc_write32(®s->xfertyp, xfertyp); >> > >> > Why is this duplicated? This seems like a huge user-experience nonsense >> > to me :) >> > >> > If you write to xfertyp register, you still have to write the same thing >> > to mixctrl? Or if you do it vice versa, won't it work ? Why did you add >> > the register? >> >> This is due to that on i.mx6q usdhc, it adds one extra mixctrl(offset >> 0x48) register to accommodate the >> low 16-bits of tranfertype and the high 16-bits of tranfertype will >> still go via the xfertyp register. >> On the i.mx6q usdhc, the xfertyp(offset 0xc) register, the low 16-bits >> is not used and only >> high 16-bits have been used. I can make the above code clear as the >> followings, >> >> #if defined(CONFIG_FSL_USDHC) >> esdhc_write32(®s->mixctrl, xfertyp & 0x0000FFFF); >> esdhc_write32(®s->xfertyp, xfertyp & 0xFFFF0000); >> #else >> esdhc_write32(®s->xfertyp, xfertyp); >> #endif > > Well this is insane, is the datasheet for mx6q available already so I can check > for myself? This is mx6q usdhc design and datasheet is not public now. Without this, the mmc driver does not work. Jason Liu > >> >> >> /* Wait for the command to complete */ >> >> @@ -482,7 +490,11 @@ int fsl_esdhc_initialize(bd_t *bis, struct >> >> fsl_esdhc_cfg *cfg) >> >> >> >> mmc = malloc(sizeof(struct mmc)); >> >> >> >> +#if defined(CONFIG_FSL_USDHC) >> >> + sprintf(mmc->name, "FSL_USDHC"); >> >> +#else >> >> sprintf(mmc->name, "FSL_ESDHC"); >> >> +#endif >> > >> > Why not just rename it to FSL_SDHC and be done with it ? >> >> OK, I will do it. >> >> >> regs = (struct fsl_esdhc *)cfg->esdhc_base; >> >> >> >> /* First reset the eSDHC controller */ >> >> Jason Liu >
On 11/15/2011 10:46 AM, Jason Hui wrote: >> Well this is insane, is the datasheet for mx6q available already so I can check >> for myself? > > This is mx6q usdhc design and datasheet is not public now. > Without this, the mmc driver does not work. Can you at least add a useful comment explaining why it is required, maybe copying only the few lines from the manual where it is described, or adding the errata number if this is due to a bug ? Best regards, Stefano
On Tue, Nov 15, 2011 at 7:56 PM, Stefano Babic <sbabic@denx.de> wrote: > On 11/15/2011 10:46 AM, Jason Hui wrote: > >>> Well this is insane, is the datasheet for mx6q available already so I can check >>> for myself? >> >> This is mx6q usdhc design and datasheet is not public now. >> Without this, the mmc driver does not work. > > Can you at least add a useful comment explaining why it is required, > maybe copying only the few lines from the manual where it is described, > or adding the errata number if this is due to a bug ? It's not the bug but the design. I will extend the commit message. > > Best regards, > Stefano > > -- > ===================================================================== > DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office@denx.de > ===================================================================== >
> On Tue, Nov 15, 2011 at 7:56 PM, Stefano Babic <sbabic@denx.de> wrote: > > On 11/15/2011 10:46 AM, Jason Hui wrote: > >>> Well this is insane, is the datasheet for mx6q available already so I > >>> can check for myself? > >> > >> This is mx6q usdhc design and datasheet is not public now. > >> Without this, the mmc driver does not work. > > > > Can you at least add a useful comment explaining why it is required, > > maybe copying only the few lines from the manual where it is described, > > or adding the errata number if this is due to a bug ? > > It's not the bug but the design. Yep, I fully understand, you had to come with something new for mx6q :-) But please reconsider letting your hardware dudes inovating their drug arsenal next time you introduce new line of CPUs, it's really hard to understand their reasons for introducing such changes ;-D > I will extend the commit message. Thanks! > > > Best regards, > > Stefano > > > > -- > > ===================================================================== > > DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel > > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > > Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office@denx.de > > =====================================================================
diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c index ec953f0..cd17ef2 100644 --- a/drivers/mmc/fsl_esdhc.c +++ b/drivers/mmc/fsl_esdhc.c @@ -58,7 +58,12 @@ struct fsl_esdhc { uint autoc12err; uint hostcapblt; uint wml; - char reserved1[8]; +#if defined(CONFIG_FSL_USDHC) + uint mixctrl; + char reserved1[4]; +#else + char reserved1[8]; +#endif uint fevt; char reserved2[168]; uint hostver; @@ -298,6 +303,9 @@ esdhc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data) /* Send the command */ esdhc_write32(®s->cmdarg, cmd->cmdarg); +#if defined(CONFIG_FSL_USDHC) + esdhc_write32(®s->mixctrl, xfertyp & 0xFFFF); +#endif esdhc_write32(®s->xfertyp, xfertyp); /* Wait for the command to complete */ @@ -482,7 +490,11 @@ int fsl_esdhc_initialize(bd_t *bis, struct fsl_esdhc_cfg *cfg) mmc = malloc(sizeof(struct mmc)); +#if defined(CONFIG_FSL_USDHC) + sprintf(mmc->name, "FSL_USDHC"); +#else sprintf(mmc->name, "FSL_ESDHC"); +#endif regs = (struct fsl_esdhc *)cfg->esdhc_base; /* First reset the eSDHC controller */
The mmc host controller on the i.mx6q is called usdhc which is redesigned based on the freescale esdhc controller. The usdhc controller is almost compatible with esdhc except it adds one misc control register from user using experience. Signed-off-by: Jason Liu <jason.hui@linaro.org> --- drivers/mmc/fsl_esdhc.c | 14 +++++++++++++- 1 files changed, 13 insertions(+), 1 deletions(-)