[PATCH 1/3] mmc: sdhci: sdhci-pci-o2micro: Correctly set bus width when tuning

[PATCH 1/3] mmc: sdhci: sdhci-pci-o2micro: Correctly set bus width when tuning

[Raul E Rangel]

sdhci_send_tuning uses mmc->ios.bus_width to determine the block size.
Without this patch the block size would be set incorrectly when the
bus_width == 8 which results in tuning failing.

Signed-off-by: Raul E Rangel <rrangel@chromium.org>
---

 drivers/mmc/host/sdhci-pci-o2micro.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci-pci-o2micro.c b/drivers/mmc/host/sdhci-pci-o2micro.c
index b29bf4e7dcb48..dd21315922c87 100644
--- a/drivers/mmc/host/sdhci-pci-o2micro.c
+++ b/drivers/mmc/host/sdhci-pci-o2micro.c
@@ -115,6 +115,7 @@ static int sdhci_o2_execute_tuning(struct mmc_host *mmc, u32 opcode)
 	 */
 	if (mmc->ios.bus_width == MMC_BUS_WIDTH_8) {
 		current_bus_width = mmc->ios.bus_width;
+		mmc->ios.bus_width = MMC_BUS_WIDTH_4;
 		sdhci_set_bus_width(host, MMC_BUS_WIDTH_4);
 	}
 
@@ -126,8 +127,10 @@ static int sdhci_o2_execute_tuning(struct mmc_host *mmc, u32 opcode)
 
 	sdhci_end_tuning(host);
 
-	if (current_bus_width == MMC_BUS_WIDTH_8)
+	if (current_bus_width == MMC_BUS_WIDTH_8) {
+		mmc->ios.bus_width = MMC_BUS_WIDTH_8;
 		sdhci_set_bus_width(host, current_bus_width);
+	}
 
 	host->flags &= ~SDHCI_HS400_TUNING;
 	return 0;
-- 
2.22.0.rc2.383.gf4fbbf30c2-goog

[PATCH 2/3] mmc: sdhci: sdhci-pci-o2micro: Check if controller supports 8-bit width

[Raul E Rangel]

The O2 controller supports 8-bit EMMC access. mmc_select_bus_width()
will be used to determine if the MMC supports 8-bit or 4-bit access.

Signed-off-by: Raul E Rangel <rrangel@chromium.org>
---
I tested this on an AMD chromebook.

$ cat /sys/kernel/debug/mmc1/ios
clock:          200000000 Hz
actual clock:   200000000 Hz
vdd:            21 (3.3 ~ 3.4 V)
bus mode:       2 (push-pull)
chip select:    0 (don't care)
power mode:     2 (on)
bus width:      3 (8 bits)
timing spec:    9 (mmc HS200)
signal voltage: 1 (1.80 V)
driver type:    0 (driver type B)

Before this patch only 4 bit was negotiated.

 drivers/mmc/host/sdhci-pci-o2micro.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci-pci-o2micro.c b/drivers/mmc/host/sdhci-pci-o2micro.c
index dd21315922c87..07bb91cbdf1f8 100644
--- a/drivers/mmc/host/sdhci-pci-o2micro.c
+++ b/drivers/mmc/host/sdhci-pci-o2micro.c
@@ -395,11 +395,16 @@ int sdhci_pci_o2_probe_slot(struct sdhci_pci_slot *slot)
 {
 	struct sdhci_pci_chip *chip;
 	struct sdhci_host *host;
-	u32 reg;
+	u32 reg, caps;
 	int ret;
 
 	chip = slot->chip;
 	host = slot->host;
+
+	caps = sdhci_readl(host, SDHCI_CAPABILITIES);
+	if (caps & SDHCI_CAN_DO_8BIT)
+		host->mmc->caps |= MMC_CAP_8_BIT_DATA;
+
 	switch (chip->pdev->device) {
 	case PCI_DEVICE_ID_O2_SDS0:
 	case PCI_DEVICE_ID_O2_SEABIRD0:
-- 
2.22.0.rc2.383.gf4fbbf30c2-goog

[PATCH 3/3] mmc: sdhci: Fix indenting on SDHCI_CTRL_8BITBUS

[Raul E Rangel]

The value is referring to SDHCI_HOST_CONTROL, not SDHCI_CTRL_DMA_MASK.

Signed-off-by: Raul E Rangel <rrangel@chromium.org>
---

 drivers/mmc/host/sdhci.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 199712e7adbb3..89fd96596a1f7 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -89,7 +89,7 @@
 #define   SDHCI_CTRL_ADMA32	0x10
 #define   SDHCI_CTRL_ADMA64	0x18
 #define   SDHCI_CTRL_ADMA3	0x18
-#define   SDHCI_CTRL_8BITBUS	0x20
+#define  SDHCI_CTRL_8BITBUS	0x20
 #define  SDHCI_CTRL_CDTEST_INS	0x40
 #define  SDHCI_CTRL_CDTEST_EN	0x80
 
-- 
2.22.0.rc2.383.gf4fbbf30c2-goog

Re: [PATCH 1/3] mmc: sdhci: sdhci-pci-o2micro: Correctly set bus width when tuning

[Adrian Hunter]

On 10/06/19 9:53 PM, Raul E Rangel wrote:
> sdhci_send_tuning uses mmc->ios.bus_width to determine the block size.
> Without this patch the block size would be set incorrectly when the
> bus_width == 8 which results in tuning failing.
> 
> Signed-off-by: Raul E Rangel <rrangel@chromium.org>

Acked-by: Adrian Hunter <adrian.hunter@intel.com>

> ---
> 
>  drivers/mmc/host/sdhci-pci-o2micro.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/host/sdhci-pci-o2micro.c b/drivers/mmc/host/sdhci-pci-o2micro.c
> index b29bf4e7dcb48..dd21315922c87 100644
> --- a/drivers/mmc/host/sdhci-pci-o2micro.c
> +++ b/drivers/mmc/host/sdhci-pci-o2micro.c
> @@ -115,6 +115,7 @@ static int sdhci_o2_execute_tuning(struct mmc_host *mmc, u32 opcode)
>  	 */
>  	if (mmc->ios.bus_width == MMC_BUS_WIDTH_8) {
>  		current_bus_width = mmc->ios.bus_width;
> +		mmc->ios.bus_width = MMC_BUS_WIDTH_4;
>  		sdhci_set_bus_width(host, MMC_BUS_WIDTH_4);
>  	}
>  
> @@ -126,8 +127,10 @@ static int sdhci_o2_execute_tuning(struct mmc_host *mmc, u32 opcode)
>  
>  	sdhci_end_tuning(host);
>  
> -	if (current_bus_width == MMC_BUS_WIDTH_8)
> +	if (current_bus_width == MMC_BUS_WIDTH_8) {
> +		mmc->ios.bus_width = MMC_BUS_WIDTH_8;
>  		sdhci_set_bus_width(host, current_bus_width);
> +	}
>  
>  	host->flags &= ~SDHCI_HS400_TUNING;
>  	return 0;
> 

Re: [PATCH 2/3] mmc: sdhci: sdhci-pci-o2micro: Check if controller supports 8-bit width

[Adrian Hunter]

On 10/06/19 9:53 PM, Raul E Rangel wrote:
> The O2 controller supports 8-bit EMMC access. mmc_select_bus_width()
> will be used to determine if the MMC supports 8-bit or 4-bit access.

The problem is that the bit indicates a host controller capability, not how
many data lines there actually are on the board.  Will this break something
that does not have 8 lines?

> 
> Signed-off-by: Raul E Rangel <rrangel@chromium.org>
> ---
> I tested this on an AMD chromebook.
> 
> $ cat /sys/kernel/debug/mmc1/ios
> clock:          200000000 Hz
> actual clock:   200000000 Hz
> vdd:            21 (3.3 ~ 3.4 V)
> bus mode:       2 (push-pull)
> chip select:    0 (don't care)
> power mode:     2 (on)
> bus width:      3 (8 bits)
> timing spec:    9 (mmc HS200)
> signal voltage: 1 (1.80 V)
> driver type:    0 (driver type B)
> 
> Before this patch only 4 bit was negotiated.
> 
>  drivers/mmc/host/sdhci-pci-o2micro.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/host/sdhci-pci-o2micro.c b/drivers/mmc/host/sdhci-pci-o2micro.c
> index dd21315922c87..07bb91cbdf1f8 100644
> --- a/drivers/mmc/host/sdhci-pci-o2micro.c
> +++ b/drivers/mmc/host/sdhci-pci-o2micro.c
> @@ -395,11 +395,16 @@ int sdhci_pci_o2_probe_slot(struct sdhci_pci_slot *slot)
>  {
>  	struct sdhci_pci_chip *chip;
>  	struct sdhci_host *host;
> -	u32 reg;
> +	u32 reg, caps;
>  	int ret;
>  
>  	chip = slot->chip;
>  	host = slot->host;
> +
> +	caps = sdhci_readl(host, SDHCI_CAPABILITIES);
> +	if (caps & SDHCI_CAN_DO_8BIT)
> +		host->mmc->caps |= MMC_CAP_8_BIT_DATA;
> +
>  	switch (chip->pdev->device) {
>  	case PCI_DEVICE_ID_O2_SDS0:
>  	case PCI_DEVICE_ID_O2_SEABIRD0:
> 

Re: [PATCH 3/3] mmc: sdhci: Fix indenting on SDHCI_CTRL_8BITBUS

[Adrian Hunter]

On 10/06/19 9:53 PM, Raul E Rangel wrote:
> The value is referring to SDHCI_HOST_CONTROL, not SDHCI_CTRL_DMA_MASK.
> 
> Signed-off-by: Raul E Rangel <rrangel@chromium.org>

Acked-by: Adrian Hunter <adrian.hunter@intel.com>

> ---
> 
>  drivers/mmc/host/sdhci.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index 199712e7adbb3..89fd96596a1f7 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -89,7 +89,7 @@
>  #define   SDHCI_CTRL_ADMA32	0x10
>  #define   SDHCI_CTRL_ADMA64	0x18
>  #define   SDHCI_CTRL_ADMA3	0x18
> -#define   SDHCI_CTRL_8BITBUS	0x20
> +#define  SDHCI_CTRL_8BITBUS	0x20
>  #define  SDHCI_CTRL_CDTEST_INS	0x40
>  #define  SDHCI_CTRL_CDTEST_EN	0x80
>  
> 

Re: [PATCH 1/3] mmc: sdhci: sdhci-pci-o2micro: Correctly set bus width when tuning

[Ulf Hansson]

On Mon, 10 Jun 2019 at 20:54, Raul E Rangel <rrangel@chromium.org> wrote:
>
> sdhci_send_tuning uses mmc->ios.bus_width to determine the block size.
> Without this patch the block size would be set incorrectly when the
> bus_width == 8 which results in tuning failing.
>
> Signed-off-by: Raul E Rangel <rrangel@chromium.org>
> ---
>
>  drivers/mmc/host/sdhci-pci-o2micro.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/sdhci-pci-o2micro.c b/drivers/mmc/host/sdhci-pci-o2micro.c
> index b29bf4e7dcb48..dd21315922c87 100644
> --- a/drivers/mmc/host/sdhci-pci-o2micro.c
> +++ b/drivers/mmc/host/sdhci-pci-o2micro.c
> @@ -115,6 +115,7 @@ static int sdhci_o2_execute_tuning(struct mmc_host *mmc, u32 opcode)
>          */
>         if (mmc->ios.bus_width == MMC_BUS_WIDTH_8) {
>                 current_bus_width = mmc->ios.bus_width;
> +               mmc->ios.bus_width = MMC_BUS_WIDTH_4;

This looks wrong.

mmc->ios.bus_width is not supposed to be updated by a host driver, but
rather the value should only be read.

>                 sdhci_set_bus_width(host, MMC_BUS_WIDTH_4);
>         }
>
> @@ -126,8 +127,10 @@ static int sdhci_o2_execute_tuning(struct mmc_host *mmc, u32 opcode)
>
>         sdhci_end_tuning(host);
>
> -       if (current_bus_width == MMC_BUS_WIDTH_8)
> +       if (current_bus_width == MMC_BUS_WIDTH_8) {
> +               mmc->ios.bus_width = MMC_BUS_WIDTH_8;

Ditto.

>                 sdhci_set_bus_width(host, current_bus_width);
> +       }
>
>         host->flags &= ~SDHCI_HS400_TUNING;
>         return 0;
> --
> 2.22.0.rc2.383.gf4fbbf30c2-goog
>

Kind regards
Uffe

Re: [PATCH 3/3] mmc: sdhci: Fix indenting on SDHCI_CTRL_8BITBUS

[Ulf Hansson]

On Mon, 10 Jun 2019 at 20:55, Raul E Rangel <rrangel@chromium.org> wrote:
>
> The value is referring to SDHCI_HOST_CONTROL, not SDHCI_CTRL_DMA_MASK.

Perhaps re-phrase this changelog as to mention that you are removing a
white-space to fix alignment, as that was not so obvious.

Kind regards
Uffe

>
> Signed-off-by: Raul E Rangel <rrangel@chromium.org>
> ---
>
>  drivers/mmc/host/sdhci.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index 199712e7adbb3..89fd96596a1f7 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -89,7 +89,7 @@
>  #define   SDHCI_CTRL_ADMA32    0x10
>  #define   SDHCI_CTRL_ADMA64    0x18
>  #define   SDHCI_CTRL_ADMA3     0x18
> -#define   SDHCI_CTRL_8BITBUS   0x20
> +#define  SDHCI_CTRL_8BITBUS    0x20
>  #define  SDHCI_CTRL_CDTEST_INS 0x40
>  #define  SDHCI_CTRL_CDTEST_EN  0x80
>
> --
> 2.22.0.rc2.383.gf4fbbf30c2-goog
>

Re: [PATCH 1/3] mmc: sdhci: sdhci-pci-o2micro: Correctly set bus width when tuning

[Raul Rangel]

On Wed, Jun 12, 2019 at 03:36:25PM +0200, Ulf Hansson wrote:
> On Mon, 10 Jun 2019 at 20:54, Raul E Rangel <rrangel@chromium.org> wrote:
> >
> > sdhci_send_tuning uses mmc->ios.bus_width to determine the block size.
> > Without this patch the block size would be set incorrectly when the
> > bus_width == 8 which results in tuning failing.
> >
> > Signed-off-by: Raul E Rangel <rrangel@chromium.org>
> > ---
> >
> >  drivers/mmc/host/sdhci-pci-o2micro.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/mmc/host/sdhci-pci-o2micro.c b/drivers/mmc/host/sdhci-pci-o2micro.c
> > index b29bf4e7dcb48..dd21315922c87 100644
> > --- a/drivers/mmc/host/sdhci-pci-o2micro.c
> > +++ b/drivers/mmc/host/sdhci-pci-o2micro.c
> > @@ -115,6 +115,7 @@ static int sdhci_o2_execute_tuning(struct mmc_host *mmc, u32 opcode)
> >          */
> >         if (mmc->ios.bus_width == MMC_BUS_WIDTH_8) {
> >                 current_bus_width = mmc->ios.bus_width;
> > +               mmc->ios.bus_width = MMC_BUS_WIDTH_4;
> 
> This looks wrong.
> 
> mmc->ios.bus_width is not supposed to be updated by a host driver, but
I guess I left this part out: The O2Micro controller only supports
tuning at 4-bits. So the host driver needs to change the bus width while
tuning and then set it back when done. Ideally I would have used
`mmc_set_bus_width()`, but that is a core only function.

If `sdhci_send_tuning()` didn't rely on mmc->ios.bus_width to determine
the bus width, but instead read the HOST_CONTROL register then this
patch wouldn't be needed.
> rather the value should only be read.
> 
> >                 sdhci_set_bus_width(host, MMC_BUS_WIDTH_4);
> >         }
> >
> > @@ -126,8 +127,10 @@ static int sdhci_o2_execute_tuning(struct mmc_host *mmc, u32 opcode)
> >
> >         sdhci_end_tuning(host);
> >
> > -       if (current_bus_width == MMC_BUS_WIDTH_8)
> > +       if (current_bus_width == MMC_BUS_WIDTH_8) {
> > +               mmc->ios.bus_width = MMC_BUS_WIDTH_8;
> 
> Ditto.
> 
> >                 sdhci_set_bus_width(host, current_bus_width);
> > +       }
> >
> >         host->flags &= ~SDHCI_HS400_TUNING;
> >         return 0;
> > --
> > 2.22.0.rc2.383.gf4fbbf30c2-goog
> >
> 
> Kind regards
> Uffe
Thanks for the review!

Re: [PATCH 2/3] mmc: sdhci: sdhci-pci-o2micro: Check if controller supports 8-bit width

[Raul Rangel]

On Wed, Jun 12, 2019 at 04:09:47PM +0300, Adrian Hunter wrote:
> On 10/06/19 9:53 PM, Raul E Rangel wrote:
> > The O2 controller supports 8-bit EMMC access. mmc_select_bus_width()
> > will be used to determine if the MMC supports 8-bit or 4-bit access.
> 
> The problem is that the bit indicates a host controller capability, not how
> many data lines there actually are on the board.  Will this break something
> that does not have 8 lines?

So I asked the controller vendor about that:
> The capability shows the host controller can support 1,4,and 8 bit bus
> data transfer but it also depends on if HW can support it. Driver or FW
> should implement the bus testing procedure that is defined in A.6.3.a
> in JESD84-B51 spec to decide the real bus width that is supported in HW.

This seems to be what `mmc_select_bus_width()` is doing.

I don't actually have any 4-bit hardware to test with though.

Thanks for the review!

Re: [PATCH 2/3] mmc: sdhci: sdhci-pci-o2micro: Check if controller supports 8-bit width

[Adrian Hunter]

On 12/06/19 6:08 PM, Raul Rangel wrote:
> On Wed, Jun 12, 2019 at 04:09:47PM +0300, Adrian Hunter wrote:
>> On 10/06/19 9:53 PM, Raul E Rangel wrote:
>>> The O2 controller supports 8-bit EMMC access. mmc_select_bus_width()
>>> will be used to determine if the MMC supports 8-bit or 4-bit access.
>>
>> The problem is that the bit indicates a host controller capability, not how
>> many data lines there actually are on the board.  Will this break something
>> that does not have 8 lines?
> 
> So I asked the controller vendor about that:
>> The capability shows the host controller can support 1,4,and 8 bit bus
>> data transfer but it also depends on if HW can support it. Driver or FW
>> should implement the bus testing procedure that is defined in A.6.3.a
>> in JESD84-B51 spec to decide the real bus width that is supported in HW.
> 
> This seems to be what `mmc_select_bus_width()` is doing.

Good point.  Can you add this information to the commit message and add a
comment in the code.

> 
> I don't actually have any 4-bit hardware to test with though.
> 
> Thanks for the review!
> 

Re: [PATCH 1/3] mmc: sdhci: sdhci-pci-o2micro: Correctly set bus width when tuning

[Ulf Hansson]

On Wed, 12 Jun 2019 at 17:01, Raul Rangel <rrangel@chromium.org> wrote:
>
> On Wed, Jun 12, 2019 at 03:36:25PM +0200, Ulf Hansson wrote:
> > On Mon, 10 Jun 2019 at 20:54, Raul E Rangel <rrangel@chromium.org> wrote:
> > >
> > > sdhci_send_tuning uses mmc->ios.bus_width to determine the block size.
> > > Without this patch the block size would be set incorrectly when the
> > > bus_width == 8 which results in tuning failing.
> > >
> > > Signed-off-by: Raul E Rangel <rrangel@chromium.org>
> > > ---
> > >
> > >  drivers/mmc/host/sdhci-pci-o2micro.c | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/mmc/host/sdhci-pci-o2micro.c b/drivers/mmc/host/sdhci-pci-o2micro.c
> > > index b29bf4e7dcb48..dd21315922c87 100644
> > > --- a/drivers/mmc/host/sdhci-pci-o2micro.c
> > > +++ b/drivers/mmc/host/sdhci-pci-o2micro.c
> > > @@ -115,6 +115,7 @@ static int sdhci_o2_execute_tuning(struct mmc_host *mmc, u32 opcode)
> > >          */
> > >         if (mmc->ios.bus_width == MMC_BUS_WIDTH_8) {
> > >                 current_bus_width = mmc->ios.bus_width;
> > > +               mmc->ios.bus_width = MMC_BUS_WIDTH_4;
> >
> > This looks wrong.
> >
> > mmc->ios.bus_width is not supposed to be updated by a host driver, but
> I guess I left this part out: The O2Micro controller only supports
> tuning at 4-bits. So the host driver needs to change the bus width while
> tuning and then set it back when done.

Thanks for clarifying. Please add that information to the changelog
and a minor comment in the code as well.

> Ideally I would have used
> `mmc_set_bus_width()`, but that is a core only function.

Well, that function also calls mmc_set_ios(), which is not what you
want, or is it?

>
> If `sdhci_send_tuning()` didn't rely on mmc->ios.bus_width to determine
> the bus width, but instead read the HOST_CONTROL register then this
> patch wouldn't be needed.

Could you elaborate on that. Perhaps there are variants that have a
similar constraint and in such case, this solution could possibly help
them as well.

> > rather the value should only be read.
> >
> > >                 sdhci_set_bus_width(host, MMC_BUS_WIDTH_4);
> > >         }
> > >
> > > @@ -126,8 +127,10 @@ static int sdhci_o2_execute_tuning(struct mmc_host *mmc, u32 opcode)
> > >
> > >         sdhci_end_tuning(host);
> > >
> > > -       if (current_bus_width == MMC_BUS_WIDTH_8)
> > > +       if (current_bus_width == MMC_BUS_WIDTH_8) {
> > > +               mmc->ios.bus_width = MMC_BUS_WIDTH_8;
> >
> > Ditto.
> >
> > >                 sdhci_set_bus_width(host, current_bus_width);
> > > +       }
> > >
> > >         host->flags &= ~SDHCI_HS400_TUNING;
> > >         return 0;
> > > --
> > > 2.22.0.rc2.383.gf4fbbf30c2-goog
> > >
> >

Kind regards
Uffe