[PATCH 1/2] sdhci-of-arasan: Add quirk and device tree parameter to fake CD bit

[PATCH 1/2] sdhci-of-arasan: Add quirk and device tree parameter to fake CD bit

[Zach Brown]

The sdhci controller on xilinx zynq devices will not function unless
the cd bit is provided. http://www.xilinx.com/support/answers/61064.html
In cases where it is impossible to provide the cd bit in hardware,
setting the controller to test mode and then setting inserted to true
will get the controller to function with out the cd bit.

The device property "fake-cd" will let the arasan driver know it needs
to fake the cd bit for the controller inorder for the controller to
function with a SD card that does not provide the CD bit.

Signed-off-by: Zach Brown <zach.brown@ni.com>
---
 Documentation/devicetree/bindings/mmc/arasan,sdhci.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
index 3404afa..3b9f406 100644
--- a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
+++ b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
@@ -21,6 +21,10 @@ Required Properties:
   - interrupts: Interrupt specifier
   - interrupt-parent: Phandle for the interrupt controller that services
 		      interrupts for this device.
+Optional Properties:
+- fake-cd: On Zynq Devices the SDHCI Controller will not work without the cd
+  bit. When this option is set the driver will put the controller in test mode
+  and fake the cd bit so it will function.
 
 Required Properties for "arasan,sdhci-5.1":
   - phys: From PHY bindings: Phandle for the Generic PHY for arasan.
-- 
2.7.4

[PATCH 2/2] sdhci-of-arasan: Add quirk and device tree parameter to fake CD bit

[Zach Brown]

The sdhci controller on xilinx zynq devices will not function unless
the cd bit is provided. http://www.xilinx.com/support/answers/61064.html
In cases where it is impossible to provide the cd bit in hardware,
setting the controller to test mode and then setting inserted to true
will get the controller to function with out the cd bit.

The device property "fake-cd" will let the arasan driver know it needs
to fake the cd bit for the controller inorder for the controller to
function with a SD card that does not provide the CD bit.

Signed-off-by: Zach Brown <zach.brown@ni.com>
---
 drivers/mmc/host/sdhci-of-arasan.c | 23 ++++++++++++++++++++++-
 drivers/mmc/host/sdhci.h           |  4 ++++
 2 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
index e0f193f..a74bcf5 100644
--- a/drivers/mmc/host/sdhci-of-arasan.c
+++ b/drivers/mmc/host/sdhci-of-arasan.c
@@ -26,6 +26,7 @@
 #include <linux/phy/phy.h>
 #include <linux/regmap.h>
 #include "sdhci-pltfm.h"
+#include <linux/of.h>
 
 #define SDHCI_ARASAN_CLK_CTRL_OFFSET	0x2c
 #define SDHCI_ARASAN_VENDOR_REGISTER	0x78
@@ -203,12 +204,26 @@ static void sdhci_arasan_hs400_enhanced_strobe(struct mmc_host *mmc,
 	writel(vendor, host->ioaddr + SDHCI_ARASAN_VENDOR_REGISTER);
 }
 
+void sdhci_arasan_reset(struct sdhci_host *host, u8 mask)
+{
+	u8 ctrl;
+
+	sdhci_reset(host, mask);
+
+	if (host->quirks2 & SDHCI_QUIRK2_NEED_FAKE_CD) {
+		ctrl = sdhci_readl(host, SDHCI_HOST_CONTROL);
+		ctrl |= SDHCI_CTRL_CD_TEST_INSERTED |
+		SDHCI_CTRL_CD_TEST_ENABLE;
+		sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL);
+	}
+}
+
 static struct sdhci_ops sdhci_arasan_ops = {
 	.set_clock = sdhci_arasan_set_clock,
 	.get_max_clock = sdhci_pltfm_clk_get_max_clock,
 	.get_timeout_clock = sdhci_arasan_get_timeout_clock,
 	.set_bus_width = sdhci_set_bus_width,
-	.reset = sdhci_reset,
+	.reset = sdhci_arasan_reset,
 	.set_uhs_signaling = sdhci_set_uhs_signaling,
 };
 
@@ -516,6 +531,12 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
 	}
 
 	sdhci_get_of_property(pdev);
+
+	if (of_get_property(pdev->dev.of_node, "fake-cd", NULL))
+		host->quirks2 |= SDHCI_QUIRK2_NEED_FAKE_CD;
+
+	pltfm_host = sdhci_priv(host);
+	pltfm_host->priv = sdhci_arasan;
 	pltfm_host->clk = clk_xin;
 
 	sdhci_arasan_update_baseclkfreq(host);
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index 0411c9f..ff3c8ba 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -84,6 +84,8 @@
 #define   SDHCI_CTRL_ADMA32	0x10
 #define   SDHCI_CTRL_ADMA64	0x18
 #define   SDHCI_CTRL_8BITBUS	0x20
+#define  SDHCI_CTRL_CD_TEST_INSERTED   0x40
+#define  SDHCI_CTRL_CD_TEST_ENABLE     0x80
 
 #define SDHCI_POWER_CONTROL	0x29
 #define  SDHCI_POWER_ON		0x01
@@ -422,6 +424,8 @@ struct sdhci_host {
 #define SDHCI_QUIRK2_ACMD23_BROKEN			(1<<14)
 /* Broken Clock divider zero in controller */
 #define SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN		(1<<15)
+/* Controller needs the cd bit faked */
+#define SDHCI_QUIRK2_NEED_FAKE_CD (1<<16)
 
 	int irq;		/* Device IRQ */
 	void __iomem *ioaddr;	/* Mapped address */
-- 
2.7.4

Re: [PATCH 2/2] sdhci-of-arasan: Add quirk and device tree parameter to fake CD bit

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1700 bytes --]

Hi Zach,

[auto build test ERROR on robh/for-next]
[also build test ERROR on v4.8-rc3 next-20160824]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
[Suggest to use git(>=2.9.0) format-patch --base=<commit> (or --base=auto for convenience) to record what (public, well-known) commit your patch series was built on]
[Check https://git-scm.com/docs/git-format-patch for more information]

url:    https://github.com/0day-ci/linux/commits/Zach-Brown/sdhci-of-arasan-Add-quirk-and-device-tree-parameter-to-fake-CD-bit/20160825-072554
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: x86_64-allmodconfig (attached as .config)
compiler: gcc-6 (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers/mmc/host/sdhci-of-arasan.c: In function 'sdhci_arasan_probe':
>> drivers/mmc/host/sdhci-of-arasan.c:539:12: error: 'struct sdhci_pltfm_host' has no member named 'priv'; did you mean 'private'?
     pltfm_host->priv = sdhci_arasan;
               ^~

vim +539 drivers/mmc/host/sdhci-of-arasan.c

   533		sdhci_get_of_property(pdev);
   534	
   535		if (of_get_property(pdev->dev.of_node, "fake-cd", NULL))
   536			host->quirks2 |= SDHCI_QUIRK2_NEED_FAKE_CD;
   537	
   538		pltfm_host = sdhci_priv(host);
 > 539		pltfm_host->priv = sdhci_arasan;
   540		pltfm_host->clk = clk_xin;
   541	
   542		sdhci_arasan_update_baseclkfreq(host);

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 55503 bytes --]

Re: [PATCH 2/2] sdhci-of-arasan: Add quirk and device tree parameter to fake CD bit

[Adrian Hunter]

On 25/08/16 02:23, Zach Brown wrote:
> The sdhci controller on xilinx zynq devices will not function unless
> the cd bit is provided. http://www.xilinx.com/support/answers/61064.html
> In cases where it is impossible to provide the cd bit in hardware,
> setting the controller to test mode and then setting inserted to true
> will get the controller to function with out the cd bit.

with out -> without

> 
> The device property "fake-cd" will let the arasan driver know it needs
> to fake the cd bit for the controller inorder for the controller to

inorder -> in order

> function with a SD card that does not provide the CD bit.

Please use either 'cd' or 'CD' consistently.

> 
> Signed-off-by: Zach Brown <zach.brown@ni.com>
> ---
>  drivers/mmc/host/sdhci-of-arasan.c | 23 ++++++++++++++++++++++-
>  drivers/mmc/host/sdhci.h           |  4 ++++
>  2 files changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
> index e0f193f..a74bcf5 100644
> --- a/drivers/mmc/host/sdhci-of-arasan.c
> +++ b/drivers/mmc/host/sdhci-of-arasan.c
> @@ -26,6 +26,7 @@
>  #include <linux/phy/phy.h>
>  #include <linux/regmap.h>
>  #include "sdhci-pltfm.h"
> +#include <linux/of.h>
>  
>  #define SDHCI_ARASAN_CLK_CTRL_OFFSET	0x2c
>  #define SDHCI_ARASAN_VENDOR_REGISTER	0x78
> @@ -203,12 +204,26 @@ static void sdhci_arasan_hs400_enhanced_strobe(struct mmc_host *mmc,
>  	writel(vendor, host->ioaddr + SDHCI_ARASAN_VENDOR_REGISTER);
>  }
>  
> +void sdhci_arasan_reset(struct sdhci_host *host, u8 mask)
> +{
> +	u8 ctrl;
> +
> +	sdhci_reset(host, mask);
> +
> +	if (host->quirks2 & SDHCI_QUIRK2_NEED_FAKE_CD) {
> +		ctrl = sdhci_readl(host, SDHCI_HOST_CONTROL);

Is there a reason this is sdhci_readl instead of sdhci_readb

> +		ctrl |= SDHCI_CTRL_CD_TEST_INSERTED |
> +		SDHCI_CTRL_CD_TEST_ENABLE;

Please indent SDHCI_CTRL_CD_TEST_ENABLE

> +		sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL);
> +	}
> +}
> +
>  static struct sdhci_ops sdhci_arasan_ops = {
>  	.set_clock = sdhci_arasan_set_clock,
>  	.get_max_clock = sdhci_pltfm_clk_get_max_clock,
>  	.get_timeout_clock = sdhci_arasan_get_timeout_clock,
>  	.set_bus_width = sdhci_set_bus_width,
> -	.reset = sdhci_reset,
> +	.reset = sdhci_arasan_reset,
>  	.set_uhs_signaling = sdhci_set_uhs_signaling,
>  };
>  
> @@ -516,6 +531,12 @@ static int sdhci_arasan_probe(struct platform_device *pdev)
>  	}
>  
>  	sdhci_get_of_property(pdev);
> +
> +	if (of_get_property(pdev->dev.of_node, "fake-cd", NULL))
> +		host->quirks2 |= SDHCI_QUIRK2_NEED_FAKE_CD;

Please don't use a quirk bit for this.  Instead add a flag to, for example,
struct sdhci_arasan_data.

> +
> +	pltfm_host = sdhci_priv(host);
> +	pltfm_host->priv = sdhci_arasan;

These 2 lines don't belong.

>  	pltfm_host->clk = clk_xin;
>  
>  	sdhci_arasan_update_baseclkfreq(host);
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index 0411c9f..ff3c8ba 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -84,6 +84,8 @@
>  #define   SDHCI_CTRL_ADMA32	0x10
>  #define   SDHCI_CTRL_ADMA64	0x18
>  #define   SDHCI_CTRL_8BITBUS	0x20
> +#define  SDHCI_CTRL_CD_TEST_INSERTED   0x40
> +#define  SDHCI_CTRL_CD_TEST_ENABLE     0x80

For the sake of neatness, let's make these a little more abbreviated, say:

	SDHCI_CTRL_CDTEST_INS
	SDHCI_CTRL_CDTEST_EN

>  
>  #define SDHCI_POWER_CONTROL	0x29
>  #define  SDHCI_POWER_ON		0x01
> @@ -422,6 +424,8 @@ struct sdhci_host {
>  #define SDHCI_QUIRK2_ACMD23_BROKEN			(1<<14)
>  /* Broken Clock divider zero in controller */
>  #define SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN		(1<<15)
> +/* Controller needs the cd bit faked */
> +#define SDHCI_QUIRK2_NEED_FAKE_CD (1<<16)
>  
>  	int irq;		/* Device IRQ */
>  	void __iomem *ioaddr;	/* Mapped address */
> 

Re: [PATCH 2/2] sdhci-of-arasan: Add quirk and device tree parameter to fake CD bit

[Lars-Peter Clausen]

On 08/25/2016 01:23 AM, Zach Brown wrote:
> The sdhci controller on xilinx zynq devices will not function unless
> the cd bit is provided. http://www.xilinx.com/support/answers/61064.html
> In cases where it is impossible to provide the cd bit in hardware,
> setting the controller to test mode and then setting inserted to true
> will get the controller to function with out the cd bit.
> 
> The device property "fake-cd" will let the arasan driver know it needs
> to fake the cd bit for the controller inorder for the controller to
> function with a SD card that does not provide the CD bit.

The devicetree is supposed to be descriptive rather than imperative. fake-cd
is a bit more on the later side. There is already a standard property for
describing that the card-detect pin is not connected, called 'broken-cd'.
Can that be used instead?

Re: [PATCH 1/2] sdhci-of-arasan: Add quirk and device tree parameter to fake CD bit

[Mark Rutland]

On Wed, Aug 24, 2016 at 06:23:03PM -0500, Zach Brown wrote:
> The sdhci controller on xilinx zynq devices will not function unless
> the cd bit is provided. http://www.xilinx.com/support/answers/61064.html
> In cases where it is impossible to provide the cd bit in hardware,
> setting the controller to test mode and then setting inserted to true
> will get the controller to function with out the cd bit.
> 
> The device property "fake-cd" will let the arasan driver know it needs
> to fake the cd bit for the controller inorder for the controller to

Nit: s/inorder/in order/

Comments on the actual patch below.

> function with a SD card that does not provide the CD bit.
> 
> Signed-off-by: Zach Brown <zach.brown@ni.com>
> ---
>  Documentation/devicetree/bindings/mmc/arasan,sdhci.txt | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> index 3404afa..3b9f406 100644
> --- a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> +++ b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> @@ -21,6 +21,10 @@ Required Properties:
>    - interrupts: Interrupt specifier
>    - interrupt-parent: Phandle for the interrupt controller that services
>  		      interrupts for this device.
> +Optional Properties:
> +- fake-cd: On Zynq Devices the SDHCI Controller will not work without the cd
> +  bit. When this option is set the driver will put the controller in test mode
> +  and fake the cd bit so it will function.

As Lars noted, the DT should describe the HW, and the policy of how to deal
with that should be left to the kernel. So from a DT perspective the above is
not correct.

If I understand the linked documentation, this is slightly different to typical
uses of broken-cd in that in the absence of a card detect signal the HW will
not be able to access the SD card at all, even if requested to. Is that correct?

If so, perhaps a better option is to have the combination of broken-cd and the
compatible string for this IP block imply that the test mode workaround is
required. Obviously that requires a fixup to the usual broken-cd binding to
remove the implication that polling alone must be used.

Thanks,
Mark.

Re: [PATCH 1/2] sdhci-of-arasan: Add quirk and device tree parameter to fake CD bit

[Zach Brown]

On Thu, Aug 25, 2016 at 11:56:55AM +0100, Mark Rutland wrote:
> On Wed, Aug 24, 2016 at 06:23:03PM -0500, Zach Brown wrote:
> > The sdhci controller on xilinx zynq devices will not function unless
> > the cd bit is provided. http://www.xilinx.com/support/answers/61064.html
> > In cases where it is impossible to provide the cd bit in hardware,
> > setting the controller to test mode and then setting inserted to true
> > will get the controller to function with out the cd bit.
> > 
> > The device property "fake-cd" will let the arasan driver know it needs
> > to fake the cd bit for the controller inorder for the controller to
> 
> Nit: s/inorder/in order/
> 
> Comments on the actual patch below.
> 
> > function with a SD card that does not provide the CD bit.
> > 
> > Signed-off-by: Zach Brown <zach.brown@ni.com>
> > ---
> >  Documentation/devicetree/bindings/mmc/arasan,sdhci.txt | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> > index 3404afa..3b9f406 100644
> > --- a/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> > +++ b/Documentation/devicetree/bindings/mmc/arasan,sdhci.txt
> > @@ -21,6 +21,10 @@ Required Properties:
> >    - interrupts: Interrupt specifier
> >    - interrupt-parent: Phandle for the interrupt controller that services
> >  		      interrupts for this device.
> > +Optional Properties:
> > +- fake-cd: On Zynq Devices the SDHCI Controller will not work without the cd
> > +  bit. When this option is set the driver will put the controller in test mode
> > +  and fake the cd bit so it will function.
> 
> As Lars noted, the DT should describe the HW, and the policy of how to deal
> with that should be left to the kernel. So from a DT perspective the above is
> not correct.
> 
> If I understand the linked documentation, this is slightly different to typical
> uses of broken-cd in that in the absence of a card detect signal the HW will
> not be able to access the SD card at all, even if requested to. Is that correct?
> 
> If so, perhaps a better option is to have the combination of broken-cd and the
> compatible string for this IP block imply that the test mode workaround is
> required. Obviously that requires a fixup to the usual broken-cd binding to
> remove the implication that polling alone must be used.
> 
> Thanks,
> Mark.

In cases where the card is non-removable then polling doesn't make sense. So it
doesn't make sense to tie the test mode workaround into the broken-cd property,
even though I agree the nature of the defect fits under the notion of the CD 
being broken.

Re: [PATCH 1/2] sdhci-of-arasan: Add quirk and device tree parameter to fake CD bit

[Mark Rutland]

On Thu, Aug 25, 2016 at 12:15:44PM -0500, Zach Brown wrote:
> On Thu, Aug 25, 2016 at 11:56:55AM +0100, Mark Rutland wrote:
> > On Wed, Aug 24, 2016 at 06:23:03PM -0500, Zach Brown wrote:
> > > +- fake-cd: On Zynq Devices the SDHCI Controller will not work without the cd
> > > +  bit. When this option is set the driver will put the controller in test mode
> > > +  and fake the cd bit so it will function.
> > 
> > As Lars noted, the DT should describe the HW, and the policy of how to deal
> > with that should be left to the kernel. So from a DT perspective the above is
> > not correct.
> > 
> > If I understand the linked documentation, this is slightly different to typical
> > uses of broken-cd in that in the absence of a card detect signal the HW will
> > not be able to access the SD card at all, even if requested to. Is that correct?
> > 
> > If so, perhaps a better option is to have the combination of broken-cd and the
> > compatible string for this IP block imply that the test mode workaround is
> > required. Obviously that requires a fixup to the usual broken-cd binding to
> > remove the implication that polling alone must be used.
> > 
> > Thanks,
> > Mark.
> 
> In cases where the card is non-removable then polling doesn't make sense.

We have the non-removable property to describe that, so we can also look at that.

> So it doesn't make sense to tie the test mode workaround into the broken-cd
> property, even though I agree the nature of the defect fits under the notion
> of the CD being broken.

Maybe not solely on broken-cd, but I think that we dont necessarily need a new
DT property. As above, broken-cd, non-removable, and the compatible string may
together give the kernel enough information to choose the right thing to do.

Thanks,
Mark.

Re: [PATCH 1/2] sdhci-of-arasan: Add quirk and device tree parameter to fake CD bit

[Zach Brown]

On Thu, Aug 25, 2016 at 07:10:00PM +0100, Mark Rutland wrote:
> On Thu, Aug 25, 2016 at 12:15:44PM -0500, Zach Brown wrote:
> > On Thu, Aug 25, 2016 at 11:56:55AM +0100, Mark Rutland wrote:
> > > On Wed, Aug 24, 2016 at 06:23:03PM -0500, Zach Brown wrote:
> > > > +- fake-cd: On Zynq Devices the SDHCI Controller will not work without the cd
> > > > +  bit. When this option is set the driver will put the controller in test mode
> > > > +  and fake the cd bit so it will function.
> > > 
> > > As Lars noted, the DT should describe the HW, and the policy of how to deal
> > > with that should be left to the kernel. So from a DT perspective the above is
> > > not correct.
> > > 
> > > If I understand the linked documentation, this is slightly different to typical
> > > uses of broken-cd in that in the absence of a card detect signal the HW will
> > > not be able to access the SD card at all, even if requested to. Is that correct?
> > > 
> > > If so, perhaps a better option is to have the combination of broken-cd and the
> > > compatible string for this IP block imply that the test mode workaround is
> > > required. Obviously that requires a fixup to the usual broken-cd binding to
> > > remove the implication that polling alone must be used.
> > > 
> > > Thanks,
> > > Mark.
> > 
> > In cases where the card is non-removable then polling doesn't make sense.
> 
> We have the non-removable property to describe that, so we can also look at that.
> 
> > So it doesn't make sense to tie the test mode workaround into the broken-cd
> > property, even though I agree the nature of the defect fits under the notion
> > of the CD being broken.
> 
> Maybe not solely on broken-cd, but I think that we dont necessarily need a new
> DT property. As above, broken-cd, non-removable, and the compatible string may
> together give the kernel enough information to choose the right thing to do.
> 
> Thanks,
> Mark.

I'm not sure if I understand your suggestion completely. Are you suggesting
setting both the broken-cd and non-removable properties? That would make sense,
but my understanding was that the two properities are not meant to co-exist. In
/Documentation/devicetree/bindings/mmc/mmc.txt it states that only one should
be supplied. Don't the two properties conflict with each other?

Re: [PATCH 1/2] sdhci-of-arasan: Add quirk and device tree parameter to fake CD bit

[Mark Rutland]

On Thu, Aug 25, 2016 at 01:26:22PM -0500, Zach Brown wrote:
> On Thu, Aug 25, 2016 at 07:10:00PM +0100, Mark Rutland wrote:
> > On Thu, Aug 25, 2016 at 12:15:44PM -0500, Zach Brown wrote:
> > > In cases where the card is non-removable then polling doesn't make sense.
> > 
> > We have the non-removable property to describe that, so we can also look at that.
> > 
> > > So it doesn't make sense to tie the test mode workaround into the broken-cd
> > > property, even though I agree the nature of the defect fits under the notion
> > > of the CD being broken.
> > 
> > Maybe not solely on broken-cd, but I think that we dont necessarily need a new
> > DT property. As above, broken-cd, non-removable, and the compatible string may
> > together give the kernel enough information to choose the right thing to do.
> > 
> > Thanks,
> > Mark.
> 
> I'm not sure if I understand your suggestion completely. Are you suggesting
> setting both the broken-cd and non-removable properties? That would make sense,
> but my understanding was that the two properities are not meant to co-exist. In
> /Documentation/devicetree/bindings/mmc/mmc.txt it states that only one should
> be supplied. Don't the two properties conflict with each other?

They do for the cases that exist today, but given we're updating the document
anyway, we could simply clarify the cases in which the two can sanely co-exist
(e.g.  for this particular IP block).

Thanks,
Mark.

Re: [PATCH 1/2] sdhci-of-arasan: Add quirk and device tree parameter to fake CD bit

[Zach Brown]

On Thu, Aug 25, 2016 at 07:28:55PM +0100, Mark Rutland wrote:
> On Thu, Aug 25, 2016 at 01:26:22PM -0500, Zach Brown wrote:
> > On Thu, Aug 25, 2016 at 07:10:00PM +0100, Mark Rutland wrote:
> > > On Thu, Aug 25, 2016 at 12:15:44PM -0500, Zach Brown wrote:
> > > > In cases where the card is non-removable then polling doesn't make sense.
> > > 
> > > We have the non-removable property to describe that, so we can also look at that.
> > > 
> > > > So it doesn't make sense to tie the test mode workaround into the broken-cd
> > > > property, even though I agree the nature of the defect fits under the notion
> > > > of the CD being broken.
> > > 
> > > Maybe not solely on broken-cd, but I think that we dont necessarily need a new
> > > DT property. As above, broken-cd, non-removable, and the compatible string may
> > > together give the kernel enough information to choose the right thing to do.
> > > 
> > > Thanks,
> > > Mark.
> > 
> > I'm not sure if I understand your suggestion completely. Are you suggesting
> > setting both the broken-cd and non-removable properties? That would make sense,
> > but my understanding was that the two properities are not meant to co-exist. In
> > /Documentation/devicetree/bindings/mmc/mmc.txt it states that only one should
> > be supplied. Don't the two properties conflict with each other?
> 
> They do for the cases that exist today, but given we're updating the document
> anyway, we could simply clarify the cases in which the two can sanely co-exist
> (e.g.  for this particular IP block).
> 
> Thanks,
> Mark.

That makes sense. I'll change the documentation for broken-cd and non-removable
in the IP specific document and change the driver accordingly.

Re: [PATCH 1/2] sdhci-of-arasan: Add quirk and device tree parameter to fake CD bit

[Ulf Hansson]

On 25 August 2016 at 22:46, Zach Brown <zach.brown@ni.com> wrote:
> On Thu, Aug 25, 2016 at 07:28:55PM +0100, Mark Rutland wrote:
>> On Thu, Aug 25, 2016 at 01:26:22PM -0500, Zach Brown wrote:
>> > On Thu, Aug 25, 2016 at 07:10:00PM +0100, Mark Rutland wrote:
>> > > On Thu, Aug 25, 2016 at 12:15:44PM -0500, Zach Brown wrote:
>> > > > In cases where the card is non-removable then polling doesn't make sense.
>> > >
>> > > We have the non-removable property to describe that, so we can also look at that.
>> > >
>> > > > So it doesn't make sense to tie the test mode workaround into the broken-cd
>> > > > property, even though I agree the nature of the defect fits under the notion
>> > > > of the CD being broken.
>> > >
>> > > Maybe not solely on broken-cd, but I think that we dont necessarily need a new
>> > > DT property. As above, broken-cd, non-removable, and the compatible string may
>> > > together give the kernel enough information to choose the right thing to do.
>> > >
>> > > Thanks,
>> > > Mark.
>> >
>> > I'm not sure if I understand your suggestion completely. Are you suggesting
>> > setting both the broken-cd and non-removable properties? That would make sense,
>> > but my understanding was that the two properities are not meant to co-exist. In
>> > /Documentation/devicetree/bindings/mmc/mmc.txt it states that only one should
>> > be supplied. Don't the two properties conflict with each other?
>>
>> They do for the cases that exist today, but given we're updating the document
>> anyway, we could simply clarify the cases in which the two can sanely co-exist
>> (e.g.  for this particular IP block).

No, please!

Depending on the SDHCI variant there is already some difference on how
broken-cd is treated.

Let's not add yet another, as I think it will be too complicated for
people to understand the bindings.

>>
>> Thanks,
>> Mark.
>
> That makes sense. I'll change the documentation for broken-cd and non-removable
> in the IP specific document and change the driver accordingly.

I rather have a new DT binding specific for this case.

Perhaps there's a better name than "fake-cd". How about "force-cd", or
if someone can come up with a better name.

Kind regards
Uffe

Re: [PATCH 1/2] sdhci-of-arasan: Add quirk and device tree parameter to fake CD bit

[Zach Brown]

On Fri, Aug 26, 2016 at 09:42:38AM +0200, Ulf Hansson wrote:
> On 25 August 2016 at 22:46, Zach Brown <zach.brown@ni.com> wrote:
> > On Thu, Aug 25, 2016 at 07:28:55PM +0100, Mark Rutland wrote:
> >> On Thu, Aug 25, 2016 at 01:26:22PM -0500, Zach Brown wrote:
> >> > On Thu, Aug 25, 2016 at 07:10:00PM +0100, Mark Rutland wrote:
> >> > > On Thu, Aug 25, 2016 at 12:15:44PM -0500, Zach Brown wrote:
> >> > > > In cases where the card is non-removable then polling doesn't make sense.
> >> > >
> >> > > We have the non-removable property to describe that, so we can also look at that.
> >> > >
> >> > > > So it doesn't make sense to tie the test mode workaround into the broken-cd
> >> > > > property, even though I agree the nature of the defect fits under the notion
> >> > > > of the CD being broken.
> >> > >
> >> > > Maybe not solely on broken-cd, but I think that we dont necessarily need a new
> >> > > DT property. As above, broken-cd, non-removable, and the compatible string may
> >> > > together give the kernel enough information to choose the right thing to do.
> >> > >
> >> > > Thanks,
> >> > > Mark.
> >> >
> >> > I'm not sure if I understand your suggestion completely. Are you suggesting
> >> > setting both the broken-cd and non-removable properties? That would make sense,
> >> > but my understanding was that the two properities are not meant to co-exist. In
> >> > /Documentation/devicetree/bindings/mmc/mmc.txt it states that only one should
> >> > be supplied. Don't the two properties conflict with each other?
> >>
> >> They do for the cases that exist today, but given we're updating the document
> >> anyway, we could simply clarify the cases in which the two can sanely co-exist
> >> (e.g.  for this particular IP block).
> 
> No, please!
> 
> Depending on the SDHCI variant there is already some difference on how
> broken-cd is treated.
> 
> Let's not add yet another, as I think it will be too complicated for
> people to understand the bindings.
> 

Shawn Lin pointed out that there might be instances of the arasan controller 
that don't have the behavior the patch addresses. Having a new DT binding
specific for this case would avoid needing to maintain a list of controllers
that need the fix.  

> >>
> >> Thanks,
> >> Mark.
> >
> > That makes sense. I'll change the documentation for broken-cd and non-removable
> > in the IP specific document and change the driver accordingly.
> 
> I rather have a new DT binding specific for this case.
> 
> Perhaps there's a better name than "fake-cd". How about "force-cd", or
> if someone can come up with a better name.
> 
> Kind regards
> Uffe

I've been trying to come up with a better name. Here are a few ideas
cd-not-wired
needs-test-cd
fails-without-force-cd

Re: [PATCH 1/2] sdhci-of-arasan: Add quirk and device tree parameter to fake CD bit

[Ulf Hansson]

On 27 August 2016 at 00:29, Zach Brown <zach.brown@ni.com> wrote:
> On Fri, Aug 26, 2016 at 09:42:38AM +0200, Ulf Hansson wrote:
>> On 25 August 2016 at 22:46, Zach Brown <zach.brown@ni.com> wrote:
>> > On Thu, Aug 25, 2016 at 07:28:55PM +0100, Mark Rutland wrote:
>> >> On Thu, Aug 25, 2016 at 01:26:22PM -0500, Zach Brown wrote:
>> >> > On Thu, Aug 25, 2016 at 07:10:00PM +0100, Mark Rutland wrote:
>> >> > > On Thu, Aug 25, 2016 at 12:15:44PM -0500, Zach Brown wrote:
>> >> > > > In cases where the card is non-removable then polling doesn't make sense.
>> >> > >
>> >> > > We have the non-removable property to describe that, so we can also look at that.
>> >> > >
>> >> > > > So it doesn't make sense to tie the test mode workaround into the broken-cd
>> >> > > > property, even though I agree the nature of the defect fits under the notion
>> >> > > > of the CD being broken.
>> >> > >
>> >> > > Maybe not solely on broken-cd, but I think that we dont necessarily need a new
>> >> > > DT property. As above, broken-cd, non-removable, and the compatible string may
>> >> > > together give the kernel enough information to choose the right thing to do.
>> >> > >
>> >> > > Thanks,
>> >> > > Mark.
>> >> >
>> >> > I'm not sure if I understand your suggestion completely. Are you suggesting
>> >> > setting both the broken-cd and non-removable properties? That would make sense,
>> >> > but my understanding was that the two properities are not meant to co-exist. In
>> >> > /Documentation/devicetree/bindings/mmc/mmc.txt it states that only one should
>> >> > be supplied. Don't the two properties conflict with each other?
>> >>
>> >> They do for the cases that exist today, but given we're updating the document
>> >> anyway, we could simply clarify the cases in which the two can sanely co-exist
>> >> (e.g.  for this particular IP block).
>>
>> No, please!
>>
>> Depending on the SDHCI variant there is already some difference on how
>> broken-cd is treated.
>>
>> Let's not add yet another, as I think it will be too complicated for
>> people to understand the bindings.
>>
>
> Shawn Lin pointed out that there might be instances of the arasan controller
> that don't have the behavior the patch addresses. Having a new DT binding
> specific for this case would avoid needing to maintain a list of controllers
> that need the fix.
>

Okay!

>> >>
>> >> Thanks,
>> >> Mark.
>> >
>> > That makes sense. I'll change the documentation for broken-cd and non-removable
>> > in the IP specific document and change the driver accordingly.
>>
>> I rather have a new DT binding specific for this case.
>>
>> Perhaps there's a better name than "fake-cd". How about "force-cd", or
>> if someone can come up with a better name.
>>
>> Kind regards
>> Uffe
>
> I've been trying to come up with a better name. Here are a few ideas
> cd-not-wired
> needs-test-cd
> fails-without-force-cd

Yeah, we could probably announce a naming competition for this. :-)

Just select one of them and try to get an ack from Rob/Mark then I
will pick up the change.

Kind regards
Uffe

Re: [PATCH 1/2] sdhci-of-arasan: Add quirk and device tree parameter to fake CD bit

[Sören Brinkmann]

On Wed, 2016-08-24 at 18:23:03 -0500, Zach Brown wrote:
> The sdhci controller on xilinx zynq devices will not function unless
> the cd bit is provided. http://www.xilinx.com/support/answers/61064.html
> In cases where it is impossible to provide the cd bit in hardware,
> setting the controller to test mode and then setting inserted to true
> will get the controller to function with out the cd bit.
> 
> The device property "fake-cd" will let the arasan driver know it needs
> to fake the cd bit for the controller inorder for the controller to
> function with a SD card that does not provide the CD bit.

I thought the CD is, if not pinned out, tied off to some valid logic
level. Isn't it enough to specify cd-inverted if needed to make it work
in those cases?

	Sören

Re: [PATCH 1/2] sdhci-of-arasan: Add quirk and device tree parameter to fake CD bit

[Lars-Peter Clausen]

On 08/25/2016 05:10 PM, Sören Brinkmann wrote:
> On Wed, 2016-08-24 at 18:23:03 -0500, Zach Brown wrote:
>> The sdhci controller on xilinx zynq devices will not function unless
>> the cd bit is provided. http://www.xilinx.com/support/answers/61064.html
>> In cases where it is impossible to provide the cd bit in hardware,
>> setting the controller to test mode and then setting inserted to true
>> will get the controller to function with out the cd bit.
>>
>> The device property "fake-cd" will let the arasan driver know it needs
>> to fake the cd bit for the controller inorder for the controller to
>> function with a SD card that does not provide the CD bit.
> 
> I thought the CD is, if not pinned out, tied off to some valid logic
> level. Isn't it enough to specify cd-inverted if needed to make it work
> in those cases?

It is always brought out to some pin, that is the problem on the Zynq. This
means you'd have to set at least one pin aside as dummy CD or WP pin. Which
is not always possible when you are tight on available pins.

Re: [PATCH 1/2] sdhci-of-arasan: Add quirk and device tree parameter to fake CD bit

[Sören Brinkmann]

On Thu, 2016-08-25 at 17:23:47 +0200, Lars-Peter Clausen wrote:
> On 08/25/2016 05:10 PM, Sören Brinkmann wrote:
> > On Wed, 2016-08-24 at 18:23:03 -0500, Zach Brown wrote:
> >> The sdhci controller on xilinx zynq devices will not function unless
> >> the cd bit is provided. http://www.xilinx.com/support/answers/61064.html
> >> In cases where it is impossible to provide the cd bit in hardware,
> >> setting the controller to test mode and then setting inserted to true
> >> will get the controller to function with out the cd bit.
> >>
> >> The device property "fake-cd" will let the arasan driver know it needs
> >> to fake the cd bit for the controller inorder for the controller to
> >> function with a SD card that does not provide the CD bit.
> > 
> > I thought the CD is, if not pinned out, tied off to some valid logic
> > level. Isn't it enough to specify cd-inverted if needed to make it work
> > in those cases?
> 
> It is always brought out to some pin, that is the problem on the Zynq. This
> means you'd have to set at least one pin aside as dummy CD or WP pin. Which
> is not always possible when you are tight on available pins.

I have to admit that I haven't looked at Vivado for quite a while. Is it
possible to select EMIO for those pins? If those are not routed anything
they should be tied to some logic level, I believe.
If they are always forced to be on a physical pin, do you let that pin
just float? Otherwise, the logic level should also be defined, give and
take a logic inversion.

	Sören

Re: [PATCH 1/2] sdhci-of-arasan: Add quirk and device tree parameter to fake CD bit

[Lars-Peter Clausen]

On 08/25/2016 05:29 PM, Sören Brinkmann wrote:
> On Thu, 2016-08-25 at 17:23:47 +0200, Lars-Peter Clausen wrote:
>> On 08/25/2016 05:10 PM, Sören Brinkmann wrote:
>>> On Wed, 2016-08-24 at 18:23:03 -0500, Zach Brown wrote:
>>>> The sdhci controller on xilinx zynq devices will not function unless
>>>> the cd bit is provided. http://www.xilinx.com/support/answers/61064.html
>>>> In cases where it is impossible to provide the cd bit in hardware,
>>>> setting the controller to test mode and then setting inserted to true
>>>> will get the controller to function with out the cd bit.
>>>>
>>>> The device property "fake-cd" will let the arasan driver know it needs
>>>> to fake the cd bit for the controller inorder for the controller to
>>>> function with a SD card that does not provide the CD bit.
>>>
>>> I thought the CD is, if not pinned out, tied off to some valid logic
>>> level. Isn't it enough to specify cd-inverted if needed to make it work
>>> in those cases?
>>
>> It is always brought out to some pin, that is the problem on the Zynq. This
>> means you'd have to set at least one pin aside as dummy CD or WP pin. Which
>> is not always possible when you are tight on available pins.
> 
> I have to admit that I haven't looked at Vivado for quite a while. Is it
> possible to select EMIO for those pins? If those are not routed anything
> they should be tied to some logic level, I believe.
> If they are always forced to be on a physical pin, do you let that pin
> just float? Otherwise, the logic level should also be defined, give and
> take a logic inversion.

Yes, it is possible to source them from EMIO. We had the same issue with WP
on one of our boards and the routing to EMIO approach was one of the
solutions I considered. But I could not find documentation on whether and
how the pins are tied when the FPGA is not configured or what happens during
reconfiguration, is there a chance of glitches.

So I went with the software solution of specifying disable-wp for the slot
instead.

But even if using EMIO works I think correctly describing the hardware,
which is that there is no external WP or CD pin connected, is the better
approach rather than playing around with invert flags, which has a different
semantical meaning. Software can than work with that information and take
the approach it is best to handle the situation. And this approach might
change if we find better ways to handle it, whereas the hardware description
stays static.