[PATCH v1 1/2] x86: tangier: Enable support for SD/SDIO family in the pinmux driver

[PATCH v1 1/2] x86: tangier: Enable support for SD/SDIO family in the pinmux driver

[Andy Shevchenko]

We would need to quirk out Card Detect case and for that we allow
configuring SD/SDIO family of pins.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 arch/x86/cpu/tangier/pinmux.c | 39 ++++++++++++++++++++++++++++++-----
 1 file changed, 34 insertions(+), 5 deletions(-)

diff --git a/arch/x86/cpu/tangier/pinmux.c b/arch/x86/cpu/tangier/pinmux.c
index acf97e3af51d..8385167b2b6b 100644
--- a/arch/x86/cpu/tangier/pinmux.c
+++ b/arch/x86/cpu/tangier/pinmux.c
@@ -37,8 +37,9 @@ struct mrfld_family {
 		.npins = (e) - (s) + 1,			\
 	}
 
-/* Now we only support I2C family of pins */
+/* Now we only support SD/SDIO and I2C families of pins */
 static struct mrfld_family mrfld_families[] = {
+	MRFLD_FAMILY(3, 37, 56),
 	MRFLD_FAMILY(7, 101, 114),
 };
 
@@ -125,6 +126,34 @@ static int mrfld_pinconfig_protected(unsigned int pin, u32 mask, u32 bits)
 	return ret;
 }
 
+static int mrfld_pinconfig(unsigned int pin, u32 mask, u32 bits)
+{
+	struct mrfld_pinctrl *pinctrl;
+	struct udevice *dev;
+	void __iomem *bufcfg;
+	u32 v, value;
+	int ret;
+
+	ret = syscon_get_by_driver_data(X86_SYSCON_PINCONF, &dev);
+	if (ret)
+		return ret;
+
+	pinctrl = dev_get_priv(dev);
+
+	bufcfg = mrfld_get_bufcfg(pinctrl, pin);
+	if (!bufcfg)
+		return -EINVAL;
+
+	value = readl(bufcfg);
+	v = (value & ~mask) | (bits & mask);
+	writel(v, bufcfg);
+
+	debug("v: 0x%x p: 0x%x bits: %d, mask: %d bufcfg: 0x%p\n",
+	      v, (u32)bufcfg, bits, mask, bufcfg);
+
+	return 0;
+}
+
 static int mrfld_pinctrl_cfg_pin(ofnode pin_node)
 {
 	bool is_protected;
@@ -133,10 +162,7 @@ static int mrfld_pinctrl_cfg_pin(ofnode pin_node)
 	u32 mask;
 	int ret;
 
-	/* For now we only support just protected Family of pins */
 	is_protected = ofnode_read_bool(pin_node, "protected");
-	if (!is_protected)
-		return -ENOTSUPP;
 
 	pad_offset = ofnode_read_s32_default(pin_node, "pad-offset", -1);
 	if (pad_offset == -1)
@@ -152,7 +178,10 @@ static int mrfld_pinctrl_cfg_pin(ofnode pin_node)
 	if (mode & ~mask)
 		return -ENOTSUPP;
 
-	ret = mrfld_pinconfig_protected(pad_offset, mask, mode);
+	if (is_protected)
+		ret = mrfld_pinconfig_protected(pad_offset, mask, mode);
+	else
+		ret = mrfld_pinconfig(pad_offset, mask, mode);
 
 	return ret;
 }
-- 
2.33.0

[PATCH v1 2/2] x86: edison: Don't take SD card detect pin into consideration

[Andy Shevchenko]

There are two PCB designs in the wild which use the opposite
signaling for SD card detect. This makes U-Boot working in one case
and failing in the other. Quirk this out by disconnecting SD card
detect pin from the PCB by switching to mode 3.

BugLink: https://github.com/edison-fw/meta-intel-edison/issues/136
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 arch/x86/dts/edison.dts | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/x86/dts/edison.dts b/arch/x86/dts/edison.dts
index 2c8cf6c07102..04e8a4e457c8 100644
--- a/arch/x86/dts/edison.dts
+++ b/arch/x86/dts/edison.dts
@@ -94,6 +94,7 @@
 	sdcard: mmc@ff3fa000 {
 		compatible = "intel,sdhci-tangier";
 		reg = <0xff3fa000 0x1000>;
+		cd-inverted;
 	};
 
 	pmu: power@ff00b000 {
@@ -131,6 +132,17 @@
 		compatible = "intel,pinctrl-tangier";
 		reg = <0xff0c0000 0x8000>;
 
+		/*
+		 * Disconnect SD card detect, so it won't affect the reality
+		 * on two different PCB designs where it's using the opposite
+		 * signaling: Edison/Arduino uses Active Low, while SparkFun
+		 * went with Active High.
+		 */
+		sd_cd@0 {
+			pad-offset = <37>;
+			mode-func = <3>;
+		};
+
 		/*
 		 * Initial configuration came from the firmware.
 		 * Which quite likely has been used in the phones, where I2C #8,
-- 
2.33.0

Re: [PATCH v1 2/2] x86: edison: Don't take SD card detect pin into consideration

[Ferry Toth]

Hi,
Op 15-10-2021 om 19:11 schreef Andy Shevchenko:
> There are two PCB designs in the wild which use the opposite
> signaling for SD card detect. This makes U-Boot working in one case
> and failing in the other. Quirk this out by disconnecting SD card
> detect pin from the PCB by switching to mode 3.
> 
Tested-by: Ferry Toth <ftoth@exalondelft.nl> @ Intel Edison-Arduino board

> BugLink: https://github.com/edison-fw/meta-intel-edison/issues/136
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>   arch/x86/dts/edison.dts | 12 ++++++++++++
>   1 file changed, 12 insertions(+)
> 
> diff --git a/arch/x86/dts/edison.dts b/arch/x86/dts/edison.dts
> index 2c8cf6c07102..04e8a4e457c8 100644
> --- a/arch/x86/dts/edison.dts
> +++ b/arch/x86/dts/edison.dts
> @@ -94,6 +94,7 @@
>   	sdcard: mmc@ff3fa000 {
>   		compatible = "intel,sdhci-tangier";
>   		reg = <0xff3fa000 0x1000>;
> +		cd-inverted;
>   	};
>   
>   	pmu: power@ff00b000 {
> @@ -131,6 +132,17 @@
>   		compatible = "intel,pinctrl-tangier";
>   		reg = <0xff0c0000 0x8000>;
>   
> +		/*
> +		 * Disconnect SD card detect, so it won't affect the reality
> +		 * on two different PCB designs where it's using the opposite
> +		 * signaling: Edison/Arduino uses Active Low, while SparkFun
> +		 * went with Active High.
> +		 */
> +		sd_cd@0 {
> +			pad-offset = <37>;
> +			mode-func = <3>;
> +		};
> +
>   		/*
>   		 * Initial configuration came from the firmware.
>   		 * Which quite likely has been used in the phones, where I2C #8,
> 

Re: [PATCH v1 2/2] x86: edison: Don't take SD card detect pin into consideration

[Andy Shevchenko]

On Thu, Oct 21, 2021 at 12:41 AM Ferry Toth <fntoth@gmail.com> wrote:
> Op 15-10-2021 om 19:11 schreef Andy Shevchenko:
> > There are two PCB designs in the wild which use the opposite
> > signaling for SD card detect. This makes U-Boot working in one case
> > and failing in the other. Quirk this out by disconnecting SD card
> > detect pin from the PCB by switching to mode 3.
> >
> Tested-by: Ferry Toth <ftoth@exalondelft.nl> @ Intel Edison-Arduino board

Thank you, Ferry. And AFAICS the original reporter had confirmed that
this fixes the issue on his SparkFun board.

Bin, can this be reviewed and applied?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/2] x86: tangier: Enable support for SD/SDIO family in the pinmux driver

[Bin Meng]

Hi Andy,

On Sat, Oct 16, 2021 at 1:27 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> We would need to quirk out Card Detect case and for that we allow
> configuring SD/SDIO family of pins.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  arch/x86/cpu/tangier/pinmux.c | 39 ++++++++++++++++++++++++++++++-----
>  1 file changed, 34 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/cpu/tangier/pinmux.c b/arch/x86/cpu/tangier/pinmux.c
> index acf97e3af51d..8385167b2b6b 100644
> --- a/arch/x86/cpu/tangier/pinmux.c
> +++ b/arch/x86/cpu/tangier/pinmux.c
> @@ -37,8 +37,9 @@ struct mrfld_family {
>                 .npins = (e) - (s) + 1,                 \
>         }
>
> -/* Now we only support I2C family of pins */
> +/* Now we only support SD/SDIO and I2C families of pins */
>  static struct mrfld_family mrfld_families[] = {
> +       MRFLD_FAMILY(3, 37, 56),
>         MRFLD_FAMILY(7, 101, 114),
>  };
>
> @@ -125,6 +126,34 @@ static int mrfld_pinconfig_protected(unsigned int pin, u32 mask, u32 bits)
>         return ret;
>  }
>
> +static int mrfld_pinconfig(unsigned int pin, u32 mask, u32 bits)
> +{
> +       struct mrfld_pinctrl *pinctrl;
> +       struct udevice *dev;
> +       void __iomem *bufcfg;
> +       u32 v, value;
> +       int ret;
> +
> +       ret = syscon_get_by_driver_data(X86_SYSCON_PINCONF, &dev);
> +       if (ret)
> +               return ret;
> +
> +       pinctrl = dev_get_priv(dev);
> +
> +       bufcfg = mrfld_get_bufcfg(pinctrl, pin);
> +       if (!bufcfg)
> +               return -EINVAL;
> +
> +       value = readl(bufcfg);
> +       v = (value & ~mask) | (bits & mask);
> +       writel(v, bufcfg);
> +
> +       debug("v: 0x%x p: 0x%x bits: %d, mask: %d bufcfg: 0x%p\n",
> +             v, (u32)bufcfg, bits, mask, bufcfg);
> +
> +       return 0;

This function can be consolidated with mrfld_pinconfig_protected(),
ie: updating mrfld_pinconfig_protected() to call mrfld_pinconfig().

> +}
> +
>  static int mrfld_pinctrl_cfg_pin(ofnode pin_node)
>  {
>         bool is_protected;
> @@ -133,10 +162,7 @@ static int mrfld_pinctrl_cfg_pin(ofnode pin_node)
>         u32 mask;
>         int ret;
>
> -       /* For now we only support just protected Family of pins */
>         is_protected = ofnode_read_bool(pin_node, "protected");
> -       if (!is_protected)
> -               return -ENOTSUPP;
>
>         pad_offset = ofnode_read_s32_default(pin_node, "pad-offset", -1);
>         if (pad_offset == -1)
> @@ -152,7 +178,10 @@ static int mrfld_pinctrl_cfg_pin(ofnode pin_node)
>         if (mode & ~mask)
>                 return -ENOTSUPP;
>
> -       ret = mrfld_pinconfig_protected(pad_offset, mask, mode);
> +       if (is_protected)
> +               ret = mrfld_pinconfig_protected(pad_offset, mask, mode);
> +       else
> +               ret = mrfld_pinconfig(pad_offset, mask, mode);
>
>         return ret;
>  }

Regards,
Bin

Re: [PATCH v1 2/2] x86: edison: Don't take SD card detect pin into consideration

[Bin Meng]

Hi Andy,

On Sat, Oct 16, 2021 at 1:27 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> There are two PCB designs in the wild which use the opposite
> signaling for SD card detect. This makes U-Boot working in one case
> and failing in the other. Quirk this out by disconnecting SD card
> detect pin from the PCB by switching to mode 3.
>
> BugLink: https://github.com/edison-fw/meta-intel-edison/issues/136
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  arch/x86/dts/edison.dts | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/arch/x86/dts/edison.dts b/arch/x86/dts/edison.dts
> index 2c8cf6c07102..04e8a4e457c8 100644
> --- a/arch/x86/dts/edison.dts
> +++ b/arch/x86/dts/edison.dts
> @@ -94,6 +94,7 @@
>         sdcard: mmc@ff3fa000 {
>                 compatible = "intel,sdhci-tangier";
>                 reg = <0xff3fa000 0x1000>;
> +               cd-inverted;

So I wonder how sd card ever worked without the "cd-inverted" property?

>         };
>
>         pmu: power@ff00b000 {
> @@ -131,6 +132,17 @@
>                 compatible = "intel,pinctrl-tangier";
>                 reg = <0xff0c0000 0x8000>;
>
> +               /*
> +                * Disconnect SD card detect, so it won't affect the reality
> +                * on two different PCB designs where it's using the opposite
> +                * signaling: Edison/Arduino uses Active Low, while SparkFun
> +                * went with Active High.
> +                */
> +               sd_cd@0 {
> +                       pad-offset = <37>;
> +                       mode-func = <3>;
> +               };
> +
>                 /*
>                  * Initial configuration came from the firmware.
>                  * Which quite likely has been used in the phones, where I2C #8,
> --

Regards,
Bin

Re: [PATCH v1 2/2] x86: edison: Don't take SD card detect pin into consideration

[Andy Shevchenko]

On Wed, Oct 27, 2021 at 5:57 AM Bin Meng <bmeng.cn@gmail.com> wrote:
> On Sat, Oct 16, 2021 at 1:27 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > There are two PCB designs in the wild which use the opposite
> > signaling for SD card detect. This makes U-Boot working in one case
> > and failing in the other. Quirk this out by disconnecting SD card
> > detect pin from the PCB by switching to mode 3.
> >
> > BugLink: https://github.com/edison-fw/meta-intel-edison/issues/136

...

> >         sdcard: mmc@ff3fa000 {
> >                 compatible = "intel,sdhci-tangier";
> >                 reg = <0xff3fa000 0x1000>;
> > +               cd-inverted;
>
> So I wonder how sd card ever worked without the "cd-inverted" property?

TL;DR: it worked on one PCB design and did not work on the other. When
we disconnect the pin the read value is always the same and inverted
to what we expected in the code.

Have you read the commit message and the comment below? Have you read
the bug report?

> > +               /*
> > +                * Disconnect SD card detect, so it won't affect the reality
> > +                * on two different PCB designs where it's using the opposite
> > +                * signaling: Edison/Arduino uses Active Low, while SparkFun
> > +                * went with Active High.
> > +                */

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/2] x86: tangier: Enable support for SD/SDIO family in the pinmux driver

[Andy Shevchenko]

On Wed, Oct 27, 2021 at 5:49 AM Bin Meng <bmeng.cn@gmail.com> wrote:
> On Sat, Oct 16, 2021 at 1:27 AM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > We would need to quirk out Card Detect case and for that we allow
> > configuring SD/SDIO family of pins.

...

> > +static int mrfld_pinconfig(unsigned int pin, u32 mask, u32 bits)
> > +{

> This function can be consolidated with mrfld_pinconfig_protected(),
> ie: updating mrfld_pinconfig_protected() to call mrfld_pinconfig().

Can it be done in a separate patch? This is a fix related and tested,
it will take time to retest this by all parties.

> > +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 2/2] x86: edison: Don't take SD card detect pin into consideration

[Bin Meng]

On Wed, Oct 27, 2021 at 4:52 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Wed, Oct 27, 2021 at 5:57 AM Bin Meng <bmeng.cn@gmail.com> wrote:
> > On Sat, Oct 16, 2021 at 1:27 AM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > >
> > > There are two PCB designs in the wild which use the opposite
> > > signaling for SD card detect. This makes U-Boot working in one case
> > > and failing in the other. Quirk this out by disconnecting SD card
> > > detect pin from the PCB by switching to mode 3.
> > >
> > > BugLink: https://github.com/edison-fw/meta-intel-edison/issues/136
>
> ...
>
> > >         sdcard: mmc@ff3fa000 {
> > >                 compatible = "intel,sdhci-tangier";
> > >                 reg = <0xff3fa000 0x1000>;
> > > +               cd-inverted;
> >
> > So I wonder how sd card ever worked without the "cd-inverted" property?
>
> TL;DR: it worked on one PCB design and did not work on the other. When
> we disconnect the pin the read value is always the same and inverted
> to what we expected in the code.

Great. That explains. Although I read the below comments a couple of
times, it just did not come to my head that "when we disconnect the
pin the read value is always the same and inverted to what we expected
in the code."

>
> Have you read the commit message and the comment below? Have you read
> the bug report?

Yes, but it's not crystal clear hence the ask. So maybe we can update
the comments to make such clearer?

>
> > > +               /*
> > > +                * Disconnect SD card detect, so it won't affect the reality
> > > +                * on two different PCB designs where it's using the opposite
> > > +                * signaling: Edison/Arduino uses Active Low, while SparkFun
> > > +                * went with Active High.
> > > +                */

FWIW,
Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

Regards,
Bin

Re: [PATCH v1 1/2] x86: tangier: Enable support for SD/SDIO family in the pinmux driver

[Bin Meng]

On Wed, Oct 27, 2021 at 4:54 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Wed, Oct 27, 2021 at 5:49 AM Bin Meng <bmeng.cn@gmail.com> wrote:
> > On Sat, Oct 16, 2021 at 1:27 AM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > >
> > > We would need to quirk out Card Detect case and for that we allow
> > > configuring SD/SDIO family of pins.
>
> ...
>
> > > +static int mrfld_pinconfig(unsigned int pin, u32 mask, u32 bits)
> > > +{
>
> > This function can be consolidated with mrfld_pinconfig_protected(),
> > ie: updating mrfld_pinconfig_protected() to call mrfld_pinconfig().
>
> Can it be done in a separate patch? This is a fix related and tested,
> it will take time to retest this by all parties.
>

Sure please send a follow-up patch when you are free.

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

Regards,
Bin

Re: [PATCH v1 1/2] x86: tangier: Enable support for SD/SDIO family in the pinmux driver

[Andy Shevchenko]

On Wed, Oct 27, 2021 at 1:14 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> On Wed, Oct 27, 2021 at 4:54 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Wed, Oct 27, 2021 at 5:49 AM Bin Meng <bmeng.cn@gmail.com> wrote:
> > > On Sat, Oct 16, 2021 at 1:27 AM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:

...

> > > This function can be consolidated with mrfld_pinconfig_protected(),
> > > ie: updating mrfld_pinconfig_protected() to call mrfld_pinconfig().
> >
> > Can it be done in a separate patch? This is a fix related and tested,
> > it will take time to retest this by all parties.
>
> Sure please send a follow-up patch when you are free.

I will do it later on.

> Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

Thanks!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 2/2] x86: edison: Don't take SD card detect pin into consideration

[Andy Shevchenko]

On Wed, Oct 27, 2021 at 06:10:59PM +0800, Bin Meng wrote:
> On Wed, Oct 27, 2021 at 4:52 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Wed, Oct 27, 2021 at 5:57 AM Bin Meng <bmeng.cn@gmail.com> wrote:
> > > On Sat, Oct 16, 2021 at 1:27 AM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:

...

> > > > +               cd-inverted;
> > >
> > > So I wonder how sd card ever worked without the "cd-inverted" property?
> >
> > TL;DR: it worked on one PCB design and did not work on the other. When
> > we disconnect the pin the read value is always the same and inverted
> > to what we expected in the code.
> 
> Great. That explains. Although I read the below comments a couple of
> times, it just did not come to my head that "when we disconnect the
> pin the read value is always the same and inverted to what we expected
> in the code."

Sure!

> > Have you read the commit message and the comment below? Have you read
> > the bug report?
> 
> Yes, but it's not crystal clear hence the ask. So maybe we can update
> the comments to make such clearer?

Yep, I'll do it for the next version.

> > > > +               /*
> > > > +                * Disconnect SD card detect, so it won't affect the reality
> > > > +                * on two different PCB designs where it's using the opposite
> > > > +                * signaling: Edison/Arduino uses Active Low, while SparkFun
> > > > +                * went with Active High.
> > > > +                */
> 
> FWIW,
> Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

Thanks!

-- 
With Best Regards,
Andy Shevchenko