diff mbox series

[2/2] drivers: mmc: sdhci_am654: Add the quirk to set TESTCD bit

Message ID 20220418102040.4993-3-a-govindraju@ti.com
State Superseded
Headers show
Series MMC: Add quirk to set the TESTCD bit | expand

Commit Message

Aswath Govindraju April 18, 2022, 10:20 a.m. UTC 
From: Vignesh Raghavendra <vigneshr@ti.com>

The ARASAN MMC controller on Keystone 3 class of devices need the SDCD
line to be connected for proper functioning. Similar to the issue pointed
out in sdhci-of-arasan.c driver, commit 3794c542641f ("mmc:
sdhci-of-arasan: Set controller to test mode when no CD bit").

In cases where this can't be connected, add a quirk to force the
controller into test mode and set the TESTCD bit. Use the flag
"ti,fails-without-test-cd", to implement this above quirk when required.

Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com>
Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
---
 drivers/mmc/host/sdhci_am654.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

Comments

kernel test robot April 18, 2022, 11:55 a.m. UTC | #1 
Hi Aswath,

I love your patch! Perhaps something to improve:

[auto build test WARNING on robh/for-next]
[also build test WARNING on linus/master ulf-hansson-mmc-mirror/next v5.18-rc3 next-20220414]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Aswath-Govindraju/MMC-Add-quirk-to-set-the-TESTCD-bit/20220418-182325
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: arc-allyesconfig (https://download.01.org/0day-ci/archive/20220418/202204181920.iopT8zQA-lkp@intel.com/config)
compiler: arceb-elf-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/a7d917691f55e240b1ab0abf36b0b39d1194a323
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Aswath-Govindraju/MMC-Add-quirk-to-set-the-TESTCD-bit/20220418-182325
        git checkout a7d917691f55e240b1ab0abf36b0b39d1194a323
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=arc SHELL=/bin/bash drivers/mmc/host/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/mmc/host/sdhci_am654.c:375:6: warning: no previous prototype for 'sdhci_am654_reset' [-Wmissing-prototypes]
     375 | void sdhci_am654_reset(struct sdhci_host *host, u8 mask)
         |      ^~~~~~~~~~~~~~~~~


vim +/sdhci_am654_reset +375 drivers/mmc/host/sdhci_am654.c

   374	
 > 375	void sdhci_am654_reset(struct sdhci_host *host, u8 mask)
   376	{
   377		u8 ctrl;
   378		struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
   379		struct sdhci_am654_data *sdhci_am654 = sdhci_pltfm_priv(pltfm_host);
   380	
   381		sdhci_reset(host, mask);
   382	
   383		if (sdhci_am654->quirks & SDHCI_AM654_QUIRK_FORCE_CDTEST) {
   384			ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL);
   385			ctrl |= SDHCI_CTRL_CDTEST_INS | SDHCI_CTRL_CDTEST_EN;
   386			sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL);
   387		}
   388	}
   389
kernel test robot April 18, 2022, 2:49 p.m. UTC | #2 
Hi Aswath,

I love your patch! Perhaps something to improve:

[auto build test WARNING on robh/for-next]
[also build test WARNING on linus/master ulf-hansson-mmc-mirror/next v5.18-rc3 next-20220414]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Aswath-Govindraju/MMC-Add-quirk-to-set-the-TESTCD-bit/20220418-182325
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: riscv-randconfig-r016-20220418 (https://download.01.org/0day-ci/archive/20220418/202204182202.yPPV6YZI-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 429cbac0390654f90bba18a41799464adf31a5ec)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install riscv cross compiling tool for clang build
        # apt-get install binutils-riscv64-linux-gnu
        # https://github.com/intel-lab-lkp/linux/commit/a7d917691f55e240b1ab0abf36b0b39d1194a323
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Aswath-Govindraju/MMC-Add-quirk-to-set-the-TESTCD-bit/20220418-182325
        git checkout a7d917691f55e240b1ab0abf36b0b39d1194a323
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=riscv SHELL=/bin/bash drivers/mmc/host/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from drivers/mmc/host/sdhci_am654.c:9:
   In file included from include/linux/iopoll.h:14:
   In file included from include/linux/io.h:13:
   In file included from arch/riscv/include/asm/io.h:136:
   include/asm-generic/io.h:464:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __raw_readb(PCI_IOBASE + addr);
                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:477:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
   #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
                                                     ^
   In file included from drivers/mmc/host/sdhci_am654.c:9:
   In file included from include/linux/iopoll.h:14:
   In file included from include/linux/io.h:13:
   In file included from arch/riscv/include/asm/io.h:136:
   include/asm-generic/io.h:490:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
                                                     ^
   In file included from drivers/mmc/host/sdhci_am654.c:9:
   In file included from include/linux/iopoll.h:14:
   In file included from include/linux/io.h:13:
   In file included from arch/riscv/include/asm/io.h:136:
   include/asm-generic/io.h:501:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writeb(value, PCI_IOBASE + addr);
                               ~~~~~~~~~~ ^
   include/asm-generic/io.h:511:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:521:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:1024:55: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           return (port > MMIO_UPPER_LIMIT) ? NULL : PCI_IOBASE + port;
                                                     ~~~~~~~~~~ ^
>> drivers/mmc/host/sdhci_am654.c:375:6: warning: no previous prototype for function 'sdhci_am654_reset' [-Wmissing-prototypes]
   void sdhci_am654_reset(struct sdhci_host *host, u8 mask)
        ^
   drivers/mmc/host/sdhci_am654.c:375:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   void sdhci_am654_reset(struct sdhci_host *host, u8 mask)
   ^
   static 
   8 warnings generated.


vim +/sdhci_am654_reset +375 drivers/mmc/host/sdhci_am654.c

   374	
 > 375	void sdhci_am654_reset(struct sdhci_host *host, u8 mask)
   376	{
   377		u8 ctrl;
   378		struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
   379		struct sdhci_am654_data *sdhci_am654 = sdhci_pltfm_priv(pltfm_host);
   380	
   381		sdhci_reset(host, mask);
   382	
   383		if (sdhci_am654->quirks & SDHCI_AM654_QUIRK_FORCE_CDTEST) {
   384			ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL);
   385			ctrl |= SDHCI_CTRL_CDTEST_INS | SDHCI_CTRL_CDTEST_EN;
   386			sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL);
   387		}
   388	}
   389
Ulf Hansson April 21, 2022, 12:11 p.m. UTC | #3 
On Mon, 18 Apr 2022 at 12:21, Aswath Govindraju <a-govindraju@ti.com> wrote:
>
> From: Vignesh Raghavendra <vigneshr@ti.com>
>
> The ARASAN MMC controller on Keystone 3 class of devices need the SDCD
> line to be connected for proper functioning. Similar to the issue pointed
> out in sdhci-of-arasan.c driver, commit 3794c542641f ("mmc:
> sdhci-of-arasan: Set controller to test mode when no CD bit").
>
> In cases where this can't be connected, add a quirk to force the
> controller into test mode and set the TESTCD bit. Use the flag
> "ti,fails-without-test-cd", to implement this above quirk when required.
>
> Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com>
> Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
> ---
>  drivers/mmc/host/sdhci_am654.c | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
> index e54fe24d47e7..c36b969ed1b6 100644
> --- a/drivers/mmc/host/sdhci_am654.c
> +++ b/drivers/mmc/host/sdhci_am654.c
> @@ -147,6 +147,9 @@ struct sdhci_am654_data {
>         int drv_strength;
>         int strb_sel;
>         u32 flags;
> +       u32 quirks;
> +
> +#define SDHCI_AM654_QUIRK_FORCE_CDTEST BIT(0)
>  };
>
>  struct sdhci_am654_driver_data {
> @@ -369,6 +372,21 @@ static void sdhci_am654_write_b(struct sdhci_host *host, u8 val, int reg)
>         }
>  }
>
> +void sdhci_am654_reset(struct sdhci_host *host, u8 mask)

This should be a static function.

> +{
> +       u8 ctrl;
> +       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +       struct sdhci_am654_data *sdhci_am654 = sdhci_pltfm_priv(pltfm_host);
> +
> +       sdhci_reset(host, mask);
> +
> +       if (sdhci_am654->quirks & SDHCI_AM654_QUIRK_FORCE_CDTEST) {
> +               ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL);
> +               ctrl |= SDHCI_CTRL_CDTEST_INS | SDHCI_CTRL_CDTEST_EN;
> +               sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL);
> +       }
> +}
> +
>  static int sdhci_am654_execute_tuning(struct mmc_host *mmc, u32 opcode)
>  {
>         struct sdhci_host *host = mmc_priv(mmc);
> @@ -500,7 +518,7 @@ static struct sdhci_ops sdhci_j721e_4bit_ops = {
>         .set_clock = sdhci_j721e_4bit_set_clock,
>         .write_b = sdhci_am654_write_b,
>         .irq = sdhci_am654_cqhci_irq,
> -       .reset = sdhci_reset,
> +       .reset = sdhci_am654_reset,
>  };
>
>  static const struct sdhci_pltfm_data sdhci_j721e_4bit_pdata = {
> @@ -719,6 +737,9 @@ static int sdhci_am654_get_of_property(struct platform_device *pdev,
>         device_property_read_u32(dev, "ti,clkbuf-sel",
>                                  &sdhci_am654->clkbuf_sel);
>
> +       if (device_property_read_bool(dev, "ti,fails-without-test-cd"))
> +               sdhci_am654->quirks |= SDHCI_AM654_QUIRK_FORCE_CDTEST;
> +
>         sdhci_get_of_property(pdev);
>
>         return 0;
> --
> 2.17.1
>

Other than the minor thing above, this looks good to me.

Kind regards
Uffe
Aswath Govindraju April 25, 2022, 6:44 a.m. UTC | #4 
Hi Uffe,

On 21/04/22 17:41, Ulf Hansson wrote:
> On Mon, 18 Apr 2022 at 12:21, Aswath Govindraju <a-govindraju@ti.com> wrote:
>>
>> From: Vignesh Raghavendra <vigneshr@ti.com>
>>
>> The ARASAN MMC controller on Keystone 3 class of devices need the SDCD
>> line to be connected for proper functioning. Similar to the issue pointed
>> out in sdhci-of-arasan.c driver, commit 3794c542641f ("mmc:
>> sdhci-of-arasan: Set controller to test mode when no CD bit").
>>
>> In cases where this can't be connected, add a quirk to force the
>> controller into test mode and set the TESTCD bit. Use the flag
>> "ti,fails-without-test-cd", to implement this above quirk when required.
>>
>> Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com>
>> Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
>> ---
>>  drivers/mmc/host/sdhci_am654.c | 23 ++++++++++++++++++++++-
>>  1 file changed, 22 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
>> index e54fe24d47e7..c36b969ed1b6 100644
>> --- a/drivers/mmc/host/sdhci_am654.c
>> +++ b/drivers/mmc/host/sdhci_am654.c
>> @@ -147,6 +147,9 @@ struct sdhci_am654_data {
>>         int drv_strength;
>>         int strb_sel;
>>         u32 flags;
>> +       u32 quirks;
>> +
>> +#define SDHCI_AM654_QUIRK_FORCE_CDTEST BIT(0)
>>  };
>>
>>  struct sdhci_am654_driver_data {
>> @@ -369,6 +372,21 @@ static void sdhci_am654_write_b(struct sdhci_host *host, u8 val, int reg)
>>         }
>>  }
>>
>> +void sdhci_am654_reset(struct sdhci_host *host, u8 mask)
> 
> This should be a static function.
> 

Fixed this in the respin.

>> +{
>> +       u8 ctrl;
>> +       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> +       struct sdhci_am654_data *sdhci_am654 = sdhci_pltfm_priv(pltfm_host);
>> +
>> +       sdhci_reset(host, mask);
>> +
>> +       if (sdhci_am654->quirks & SDHCI_AM654_QUIRK_FORCE_CDTEST) {
>> +               ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL);
>> +               ctrl |= SDHCI_CTRL_CDTEST_INS | SDHCI_CTRL_CDTEST_EN;
>> +               sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL);
>> +       }
>> +}
>> +
>>  static int sdhci_am654_execute_tuning(struct mmc_host *mmc, u32 opcode)
>>  {
>>         struct sdhci_host *host = mmc_priv(mmc);
>> @@ -500,7 +518,7 @@ static struct sdhci_ops sdhci_j721e_4bit_ops = {
>>         .set_clock = sdhci_j721e_4bit_set_clock,
>>         .write_b = sdhci_am654_write_b,
>>         .irq = sdhci_am654_cqhci_irq,
>> -       .reset = sdhci_reset,
>> +       .reset = sdhci_am654_reset,
>>  };
>>
>>  static const struct sdhci_pltfm_data sdhci_j721e_4bit_pdata = {
>> @@ -719,6 +737,9 @@ static int sdhci_am654_get_of_property(struct platform_device *pdev,
>>         device_property_read_u32(dev, "ti,clkbuf-sel",
>>                                  &sdhci_am654->clkbuf_sel);
>>
>> +       if (device_property_read_bool(dev, "ti,fails-without-test-cd"))
>> +               sdhci_am654->quirks |= SDHCI_AM654_QUIRK_FORCE_CDTEST;
>> +
>>         sdhci_get_of_property(pdev);
>>
>>         return 0;
>> --
>> 2.17.1
>>
> 
> Other than the minor thing above, this looks good to me.
> 

Thank you for the review.

Link to the v2 of this patch series,

https://patchwork.kernel.org/project/linux-mmc/list/?series=635179
diff mbox series

Patch

diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
index e54fe24d47e7..c36b969ed1b6 100644
--- a/drivers/mmc/host/sdhci_am654.c
+++ b/drivers/mmc/host/sdhci_am654.c
@@ -147,6 +147,9 @@  struct sdhci_am654_data {
 	int drv_strength;
 	int strb_sel;
 	u32 flags;
+	u32 quirks;
+
+#define SDHCI_AM654_QUIRK_FORCE_CDTEST BIT(0)
 };
 
 struct sdhci_am654_driver_data {
@@ -369,6 +372,21 @@  static void sdhci_am654_write_b(struct sdhci_host *host, u8 val, int reg)
 	}
 }
 
+void sdhci_am654_reset(struct sdhci_host *host, u8 mask)
+{
+	u8 ctrl;
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_am654_data *sdhci_am654 = sdhci_pltfm_priv(pltfm_host);
+
+	sdhci_reset(host, mask);
+
+	if (sdhci_am654->quirks & SDHCI_AM654_QUIRK_FORCE_CDTEST) {
+		ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL);
+		ctrl |= SDHCI_CTRL_CDTEST_INS | SDHCI_CTRL_CDTEST_EN;
+		sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL);
+	}
+}
+
 static int sdhci_am654_execute_tuning(struct mmc_host *mmc, u32 opcode)
 {
 	struct sdhci_host *host = mmc_priv(mmc);
@@ -500,7 +518,7 @@  static struct sdhci_ops sdhci_j721e_4bit_ops = {
 	.set_clock = sdhci_j721e_4bit_set_clock,
 	.write_b = sdhci_am654_write_b,
 	.irq = sdhci_am654_cqhci_irq,
-	.reset = sdhci_reset,
+	.reset = sdhci_am654_reset,
 };
 
 static const struct sdhci_pltfm_data sdhci_j721e_4bit_pdata = {
@@ -719,6 +737,9 @@  static int sdhci_am654_get_of_property(struct platform_device *pdev,
 	device_property_read_u32(dev, "ti,clkbuf-sel",
 				 &sdhci_am654->clkbuf_sel);
 
+	if (device_property_read_bool(dev, "ti,fails-without-test-cd"))
+		sdhci_am654->quirks |= SDHCI_AM654_QUIRK_FORCE_CDTEST;
+
 	sdhci_get_of_property(pdev);
 
 	return 0;