| Message ID | 1408453153-8125-1-git-send-email-21cnbao@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
["Followup-To:" header set to gmane.linux.kernel.mmc.] On 2014-08-19, Barry Song <21cnbao@gmail.com> wrote: > From: Minda Chen <Minda.Chen@csr.com> > > 8bit-width enable bit of CSR MMC hosts is 3, while stardard hosts use > bit 5. this patch fixes the functionality of 8bit transfer in CSR mmc > controllers and improve performance for mmc0 a lot. > > Signed-off-by: Minda Chen <Minda.Chen@csr.com> > Signed-off-by: Barry Song <Baohua.Song@csr.com> > --- > -v2: check for host->version >= SDHCI_SPEC_300 > Hi Barry, Did you check the runtime behaviour of the device after this change ? From the SiRFprimaII/SiRFatlasVI data sheets, it appears that the implementation of the SDHCI controller is a modified version of the one described in the 1.0 specification, and not a normal 3.0 controller. This explains why the independent development of the 8-bit wide transfer bus does not use the same bit as the standard one. As a result, the description of the SPEC_VER field in the SD_SLOT_INT_STATUS register indicates a read-only value of 0, which corresponds to SDHCI_SPEC_100 in the "sdhci.h" header. On a real SiRFatlasVI chip, the value is 0 as well. I believe the correct change would be to remove the control on the version >= SDHCI_SPEC_300 in both 4-bit and 8-bit cases, instead of adding it in both cases. There are no supported SiRF SoCs where the 8-bit bus is not supported at the controller level. In my opinion, this should look like this: +static void sdhci_sirf_set_bus_width(struct sdhci_host *host, int width) +{ + u8 ctrl; + + ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL); + /* The 8-bit bus width configuration bit is not standard */ + ctrl &= ~(SDHCI_CTRL_4BITBUS | SDHCI_SIRF_8BITBUS); + if (width == MMC_BUS_WIDTH_8) { + ctrl |= SDHCI_SIRF_8BITBUS; + } else if (width == MMC_BUS_WIDTH_4) + ctrl |= SDHCI_CTRL_4BITBUS; + } + sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL); +} + You can use it verbatim with my signoff if you want. Signed-off-by: Romain Izard <romain.izard.pro@gmail.com> But you should try it on your board, as I do not have a setup to test the mainline kernel on SiRFatlasVI available. Best regards,
2014-08-20 22:25 GMT+08:00 Romain Izard <romain.izard.pro@gmail.com>: > ["Followup-To:" header set to gmane.linux.kernel.mmc.] > On 2014-08-19, Barry Song <21cnbao@gmail.com> wrote: >> From: Minda Chen <Minda.Chen@csr.com> >> >> 8bit-width enable bit of CSR MMC hosts is 3, while stardard hosts use >> bit 5. this patch fixes the functionality of 8bit transfer in CSR mmc >> controllers and improve performance for mmc0 a lot. >> >> Signed-off-by: Minda Chen <Minda.Chen@csr.com> >> Signed-off-by: Barry Song <Baohua.Song@csr.com> >> --- >> -v2: check for host->version >= SDHCI_SPEC_300 >> > > Hi Barry, > > Did you check the runtime behaviour of the device after this change ? > > From the SiRFprimaII/SiRFatlasVI data sheets, it appears that the > implementation of the SDHCI controller is a modified version of the one > described in the 1.0 specification, and not a normal 3.0 controller. > This explains why the independent development of the 8-bit wide transfer > bus does not use the same bit as the standard one. > > As a result, the description of the SPEC_VER field in the > SD_SLOT_INT_STATUS register indicates a read-only value of 0, which > corresponds to SDHCI_SPEC_100 in the "sdhci.h" header. On a real > SiRFatlasVI chip, the value is 0 as well. > > I believe the correct change would be to remove the control on the > version >= SDHCI_SPEC_300 in both 4-bit and 8-bit cases, instead of > adding it in both cases. There are no supported SiRF SoCs where the 8-bit > bus is not supported at the controller level. > > In my opinion, this should look like this: > > +static void sdhci_sirf_set_bus_width(struct sdhci_host *host, int width) > +{ > + u8 ctrl; > + > + ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL); > + /* The 8-bit bus width configuration bit is not standard */ > + ctrl &= ~(SDHCI_CTRL_4BITBUS | SDHCI_SIRF_8BITBUS); > + if (width == MMC_BUS_WIDTH_8) { > + ctrl |= SDHCI_SIRF_8BITBUS; > + } else if (width == MMC_BUS_WIDTH_4) > + ctrl |= SDHCI_CTRL_4BITBUS; > + } > + sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL); > +} > + hi Romain, thanks very much! Minda will double-check the HW behaviour and confirm what you said. > > You can use it verbatim with my signoff if you want. > Signed-off-by: Romain Izard <romain.izard.pro@gmail.com> > > > But you should try it on your board, as I do not have a setup to test > the mainline kernel on SiRFatlasVI available. > > Best regards, > -- > Romain Izard > -barry -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Barry Song [mailto:21cnbao@gmail.com] > Sent: Thursday, August 21, 2014 6:09 PM > To: Romain Izard > Cc: linux-mmc@vger.kernel.org; Minda Chen; DL-SHA-WorkGroupLinux > Subject: Re: [PATCH v2] mmc: sdhci-sirf: fix 8bit width enable by overwriting > set_bus_width > > 2014-08-20 22:25 GMT+08:00 Romain Izard <romain.izard.pro@gmail.com>: > > ["Followup-To:" header set to gmane.linux.kernel.mmc.] On 2014-08-19, > > Barry Song <21cnbao@gmail.com> wrote: > >> From: Minda Chen <Minda.Chen@csr.com> > >> > >> 8bit-width enable bit of CSR MMC hosts is 3, while stardard hosts use > >> bit 5. this patch fixes the functionality of 8bit transfer in CSR mmc > >> controllers and improve performance for mmc0 a lot. > >> > >> Signed-off-by: Minda Chen <Minda.Chen@csr.com> > >> Signed-off-by: Barry Song <Baohua.Song@csr.com> > >> --- > >> -v2: check for host->version >= SDHCI_SPEC_300 > >> > > > > Hi Barry, > > > > Did you check the runtime behaviour of the device after this change ? > > > > From the SiRFprimaII/SiRFatlasVI data sheets, it appears that the > > implementation of the SDHCI controller is a modified version of the > > one described in the 1.0 specification, and not a normal 3.0 controller. > > This explains why the independent development of the 8-bit wide > > transfer bus does not use the same bit as the standard one. > > > > As a result, the description of the SPEC_VER field in the > > SD_SLOT_INT_STATUS register indicates a read-only value of 0, which > > corresponds to SDHCI_SPEC_100 in the "sdhci.h" header. On a real > > SiRFatlasVI chip, the value is 0 as well. > > > > I believe the correct change would be to remove the control on the > > version >= SDHCI_SPEC_300 in both 4-bit and 8-bit cases, instead of > > adding it in both cases. There are no supported SiRF SoCs where the > > 8-bit bus is not supported at the controller level. > > > > In my opinion, this should look like this: > > > > +static void sdhci_sirf_set_bus_width(struct sdhci_host *host, int > > +width) { > > + u8 ctrl; > > + > > + ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL); > > + /* The 8-bit bus width configuration bit is not standard */ > > + ctrl &= ~(SDHCI_CTRL_4BITBUS | SDHCI_SIRF_8BITBUS); > > + if (width == MMC_BUS_WIDTH_8) { > > + ctrl |= SDHCI_SIRF_8BITBUS; > > + } else if (width == MMC_BUS_WIDTH_4) > > + ctrl |= SDHCI_CTRL_4BITBUS; > > + } > > + sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL); } > > + > > hi Romain, > thanks very much! Minda will double-check the HW behaviour and confirm > what you said. [Barry Song] Romain, Minda's version is: static void sdhci_sirf_set_bus_width(struct sdhci_host *host, int width) { u8 ctrl; ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL); ctrl &= ~(SDHCI_CTRL_4BITBUS | SDHCI_SIRF_8BITBUS); if (width == MMC_BUS_WIDTH_8) { /* * CSR atlas7 and prima2 SD host version is not 3.0 * 8bit-width enable bit of CSR SD hosts is 3, * while stardard hosts use bit 5 */ ctrl |= SDHCI_SIRF_8BITBUS; } else if (width == MMC_BUS_WIDTH_4) ctrl |= SDHCI_CTRL_4BITBUS; sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL); } It is basically same with you. > > > > > You can use it verbatim with my signoff if you want. > > Signed-off-by: Romain Izard <romain.izard.pro@gmail.com> [Barry Song] do you mean a Reviewed-by? -barry Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom More information can be found at www.csr.com. Keep up to date with CSR on our technical blog, www.csr.com/blog, CSR people blog, www.csr.com/people, YouTube, www.youtube.com/user/CSRplc, Facebook, www.facebook.com/pages/CSR/191038434253534, or follow us on Twitter at www.twitter.com/CSR_plc. New for 2014, you can now access the wide range of products powered by aptX at www.aptx.com.
2014-08-22 8:48 GMT+02:00 Barry Song <Barry.Song@csr.com>: > > Minda's version is: > static void sdhci_sirf_set_bus_width(struct sdhci_host *host, int width) > { > u8 ctrl; > > ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL); > ctrl &= ~(SDHCI_CTRL_4BITBUS | SDHCI_SIRF_8BITBUS); > > if (width == MMC_BUS_WIDTH_8) { > /* > * CSR atlas7 and prima2 SD host version is not 3.0 > * 8bit-width enable bit of CSR SD hosts is 3, > * while stardard hosts use bit 5 > */ > ctrl |= SDHCI_SIRF_8BITBUS; > } else if (width == MMC_BUS_WIDTH_4) > ctrl |= SDHCI_CTRL_4BITBUS; Here, the braces are not right. The coding style requires braces for both sides of an 'else', or none of them > > sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL); > } > > It is basically same with you. > Yes. >> > You can use it verbatim with my signoff if you want. >> > Signed-off-by: Romain Izard <romain.izard.pro@gmail.com> > > do you mean a Reviewed-by? > In case you wanted to take the code snipplet I provided without changing it, I provided you my Signed-off-by to be sure that there is no issue. Since Minda wrote it on his/her own, it's not necessary. But now, you can use: Reviewed-by: Romain Izard <romain.izard.pro@gmail.com> Best regards,
diff --git a/drivers/mmc/host/sdhci-sirf.c b/drivers/mmc/host/sdhci-sirf.c index 1700453..a7224e5 100644 --- a/drivers/mmc/host/sdhci-sirf.c +++ b/drivers/mmc/host/sdhci-sirf.c @@ -15,6 +15,8 @@ #include <linux/mmc/slot-gpio.h> #include "sdhci-pltfm.h" +#define SDHCI_SIRF_8BITBUS BIT(3) + struct sdhci_sirf_priv { struct clk *clk; int gpio_cd; @@ -27,10 +29,34 @@ static unsigned int sdhci_sirf_get_max_clk(struct sdhci_host *host) return clk_get_rate(priv->clk); } +static void sdhci_sirf_set_bus_width(struct sdhci_host *host, int width) +{ + u8 ctrl; + + ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL); + if (width == MMC_BUS_WIDTH_8) { + ctrl &= ~SDHCI_CTRL_4BITBUS; + /* + * 8bit-width enable bit of CSR MMC hosts is 3, + * while stardard hosts use bit 5 + */ + if (host->version >= SDHCI_SPEC_300) + ctrl |= SDHCI_SIRF_8BITBUS; + } else { + if (host->version >= SDHCI_SPEC_300) + ctrl &= ~SDHCI_SIRF_8BITBUS; + if (width == MMC_BUS_WIDTH_4) + ctrl |= SDHCI_CTRL_4BITBUS; + else + ctrl &= ~SDHCI_CTRL_4BITBUS; + } + sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL); +} + static struct sdhci_ops sdhci_sirf_ops = { .set_clock = sdhci_set_clock, .get_max_clock = sdhci_sirf_get_max_clk, - .set_bus_width = sdhci_set_bus_width, + .set_bus_width = sdhci_sirf_set_bus_width, .reset = sdhci_reset, .set_uhs_signaling = sdhci_set_uhs_signaling, };