[PATCH v2 0/2] Hi folks,

[PATCH v2 0/2] Hi folks,

[Hector Martin]

Hi folks,

This short series adds a few quirks needed to make the card readers in
Apple M1 Pro/Max MacBook laptops work properly.

The first patch should be straightforward; it just allows configuring
the CD/WP polarity based on device tree settings. There is already a
standard DT binding for this.

The second patch works around an issue with 8/16-bit MMIO reads that
only affects these platforms, for some reason.

Changes since v1:

- Also applied workaround to GL9750
- Fixed checkpatch warnings

Hector Martin (2):
  mmc: sdhci-pci-gli: GL9755: Support for CD/WP inversion on OF
    platforms
  mmc: sdhci-pci-gli: GL975[50]: Issue 8/16-bit MMIO reads as 32-bit
    reads.

 drivers/mmc/host/sdhci-pci-gli.c | 42 ++++++++++++++++++++++++++++++--
 1 file changed, 40 insertions(+), 2 deletions(-)

-- 
2.33.0

[PATCH v2 1/2] mmc: sdhci-pci-gli: GL9755: Support for CD/WP inversion on OF platforms

[Hector Martin]

This is required on some Apple ARM64 laptops using this controller.
As is typical on DT platforms, pull these quirks from the device tree
using the standard mmc bindings.

See Documentation/devicetree/bindings/mmc/mmc-controller.yaml

Signed-off-by: Hector Martin <marcan@marcan.st>
---
 drivers/mmc/host/sdhci-pci-gli.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/sdhci-pci-gli.c b/drivers/mmc/host/sdhci-pci-gli.c
index 4fd99c1e82ba..ad742743a494 100644
--- a/drivers/mmc/host/sdhci-pci-gli.c
+++ b/drivers/mmc/host/sdhci-pci-gli.c
@@ -12,6 +12,7 @@
 #include <linux/pci.h>
 #include <linux/mmc/mmc.h>
 #include <linux/delay.h>
+#include <linux/of.h>
 #include "sdhci.h"
 #include "sdhci-pci.h"
 #include "cqhci.h"
@@ -114,8 +115,10 @@
 #define   GLI_9755_WT_EN_OFF    0x0
 
 #define PCI_GLI_9755_PECONF   0x44
-#define   PCI_GLI_9755_LFCLK    GENMASK(14, 12)
-#define   PCI_GLI_9755_DMACLK   BIT(29)
+#define   PCI_GLI_9755_LFCLK          GENMASK(14, 12)
+#define   PCI_GLI_9755_DMACLK         BIT(29)
+#define   PCI_GLI_9755_INVERT_CD      BIT(30)
+#define   PCI_GLI_9755_INVERT_WP      BIT(31)
 
 #define PCI_GLI_9755_CFG2          0x48
 #define   PCI_GLI_9755_CFG2_L1DLY    GENMASK(28, 24)
@@ -570,6 +573,18 @@ static void gl9755_hw_setting(struct sdhci_pci_slot *slot)
 	gl9755_wt_on(pdev);
 
 	pci_read_config_dword(pdev, PCI_GLI_9755_PECONF, &value);
+#ifdef CONFIG_OF
+	if (pdev->dev.of_node) {
+		/*
+		 * Apple ARM64 platforms using these chips may have
+		 * inverted CD/WP detection.
+		 */
+		if (of_property_read_bool(pdev->dev.of_node, "cd-inverted"))
+			value |= PCI_GLI_9755_INVERT_CD;
+		if (of_property_read_bool(pdev->dev.of_node, "wp-inverted"))
+			value |= PCI_GLI_9755_INVERT_WP;
+	}
+#endif
 	value &= ~PCI_GLI_9755_LFCLK;
 	value &= ~PCI_GLI_9755_DMACLK;
 	pci_write_config_dword(pdev, PCI_GLI_9755_PECONF, value);
-- 
2.33.0

[PATCH v2 2/2] mmc: sdhci-pci-gli: GL975[50]: Issue 8/16-bit MMIO reads as 32-bit reads.

[Hector Martin]

For some reason, <32-bit reads do not work on Apple ARM64 platforms with
these chips (even though they do on other PCIe devices). Issue them as
32-bit reads instead. This is done unconditionally, as it shouldn't hurt
even if not necessary.

Signed-off-by: Hector Martin <marcan@marcan.st>
---
 drivers/mmc/host/sdhci-pci-gli.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/mmc/host/sdhci-pci-gli.c b/drivers/mmc/host/sdhci-pci-gli.c
index ad742743a494..c6828e84db31 100644
--- a/drivers/mmc/host/sdhci-pci-gli.c
+++ b/drivers/mmc/host/sdhci-pci-gli.c
@@ -906,7 +906,28 @@ static int gli_probe_slot_gl9763e(struct sdhci_pci_slot *slot)
 	return 0;
 }
 
+#define REG_OFFSET_IN_BITS(reg) ((reg) << 3 & 0x18)
+
+static u16 sdhci_gli_readw(struct sdhci_host *host, int reg)
+{
+	u32 val = readl(host->ioaddr + (reg & ~3));
+	u16 word;
+
+	word = (val >> REG_OFFSET_IN_BITS(reg)) & 0xffff;
+	return word;
+}
+
+static u8 sdhci_gli_readb(struct sdhci_host *host, int reg)
+{
+	u32 val = readl(host->ioaddr + (reg & ~3));
+	u8 byte = (val >> REG_OFFSET_IN_BITS(reg)) & 0xff;
+
+	return byte;
+}
+
 static const struct sdhci_ops sdhci_gl9755_ops = {
+	.read_w			= sdhci_gli_readw,
+	.read_b			= sdhci_gli_readb,
 	.set_clock		= sdhci_gl9755_set_clock,
 	.enable_dma		= sdhci_pci_enable_dma,
 	.set_bus_width		= sdhci_set_bus_width,
@@ -926,6 +947,8 @@ const struct sdhci_pci_fixes sdhci_gl9755 = {
 };
 
 static const struct sdhci_ops sdhci_gl9750_ops = {
+	.read_w			= sdhci_gli_readw,
+	.read_b			= sdhci_gli_readb,
 	.read_l                 = sdhci_gl9750_readl,
 	.set_clock		= sdhci_gl9750_set_clock,
 	.enable_dma		= sdhci_pci_enable_dma,
-- 
2.33.0

Re: [PATCH v2 0/2] (mmc: sdhci-pci-gli: GL9755: Quirks for Apple ARM platforms)

[Hector Martin]

On 12/12/2021 16.02, Hector Martin wrote:
> Hi folks,
> 
> This short series adds a few quirks needed to make the card readers in
> Apple M1 Pro/Max MacBook laptops work properly.
> 
> The first patch should be straightforward; it just allows configuring
> the CD/WP polarity based on device tree settings. There is already a
> standard DT binding for this.
> 
> The second patch works around an issue with 8/16-bit MMIO reads that
> only affects these platforms, for some reason.
> 
> Changes since v1:
> 
> - Also applied workaround to GL9750
> - Fixed checkpatch warnings
> 
> Hector Martin (2):
>    mmc: sdhci-pci-gli: GL9755: Support for CD/WP inversion on OF
>      platforms
>    mmc: sdhci-pci-gli: GL975[50]: Issue 8/16-bit MMIO reads as 32-bit
>      reads.
> 
>   drivers/mmc/host/sdhci-pci-gli.c | 42 ++++++++++++++++++++++++++++++--
>   1 file changed, 40 insertions(+), 2 deletions(-)
> 

Argh, sorry about the bad subject. Copy&paste mishap while editing the
cover letter. Not doing great on the emailing for this one... :(

-- 
Hector Martin (marcan@marcan.st)
Public Key: https://mrcn.st/pub

Re: [PATCH v2 1/2] mmc: sdhci-pci-gli: GL9755: Support for CD/WP inversion on OF platforms

[Robin Murphy]

On 2021-12-12 07:02, Hector Martin wrote:
> This is required on some Apple ARM64 laptops using this controller.
> As is typical on DT platforms, pull these quirks from the device tree
> using the standard mmc bindings.
> 
> See Documentation/devicetree/bindings/mmc/mmc-controller.yaml
> 
> Signed-off-by: Hector Martin <marcan@marcan.st>
> ---
>   drivers/mmc/host/sdhci-pci-gli.c | 19 +++++++++++++++++--
>   1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-pci-gli.c b/drivers/mmc/host/sdhci-pci-gli.c
> index 4fd99c1e82ba..ad742743a494 100644
> --- a/drivers/mmc/host/sdhci-pci-gli.c
> +++ b/drivers/mmc/host/sdhci-pci-gli.c
> @@ -12,6 +12,7 @@
>   #include <linux/pci.h>
>   #include <linux/mmc/mmc.h>
>   #include <linux/delay.h>
> +#include <linux/of.h>
>   #include "sdhci.h"
>   #include "sdhci-pci.h"
>   #include "cqhci.h"
> @@ -114,8 +115,10 @@
>   #define   GLI_9755_WT_EN_OFF    0x0
>   
>   #define PCI_GLI_9755_PECONF   0x44
> -#define   PCI_GLI_9755_LFCLK    GENMASK(14, 12)
> -#define   PCI_GLI_9755_DMACLK   BIT(29)
> +#define   PCI_GLI_9755_LFCLK          GENMASK(14, 12)
> +#define   PCI_GLI_9755_DMACLK         BIT(29)
> +#define   PCI_GLI_9755_INVERT_CD      BIT(30)
> +#define   PCI_GLI_9755_INVERT_WP      BIT(31)
>   
>   #define PCI_GLI_9755_CFG2          0x48
>   #define   PCI_GLI_9755_CFG2_L1DLY    GENMASK(28, 24)
> @@ -570,6 +573,18 @@ static void gl9755_hw_setting(struct sdhci_pci_slot *slot)
>   	gl9755_wt_on(pdev);
>   
>   	pci_read_config_dword(pdev, PCI_GLI_9755_PECONF, &value);
> +#ifdef CONFIG_OF
> +	if (pdev->dev.of_node) {

FWIW, of_property_read_bool() should handle a NULL node correctly, and 
its stub be optimised away for !OF, so you don't really need the 
explicit check or the #ifdef.

Robin.

> +		/*
> +		 * Apple ARM64 platforms using these chips may have
> +		 * inverted CD/WP detection.
> +		 */
> +		if (of_property_read_bool(pdev->dev.of_node, "cd-inverted"))
> +			value |= PCI_GLI_9755_INVERT_CD;
> +		if (of_property_read_bool(pdev->dev.of_node, "wp-inverted"))
> +			value |= PCI_GLI_9755_INVERT_WP;
> +	}
> +#endif
>   	value &= ~PCI_GLI_9755_LFCLK;
>   	value &= ~PCI_GLI_9755_DMACLK;
>   	pci_write_config_dword(pdev, PCI_GLI_9755_PECONF, value);
> 

Re: [PATCH v2 1/2] mmc: sdhci-pci-gli: GL9755: Support for CD/WP inversion on OF platforms

[Adrian Hunter]

On 12/12/2021 09:02, Hector Martin wrote:
> This is required on some Apple ARM64 laptops using this controller.
> As is typical on DT platforms, pull these quirks from the device tree
> using the standard mmc bindings.
> 
> See Documentation/devicetree/bindings/mmc/mmc-controller.yaml
> 
> Signed-off-by: Hector Martin <marcan@marcan.st>

A couple of kernel style issues, but fix those and you can add:

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

> ---
>  drivers/mmc/host/sdhci-pci-gli.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-pci-gli.c b/drivers/mmc/host/sdhci-pci-gli.c
> index 4fd99c1e82ba..ad742743a494 100644
> --- a/drivers/mmc/host/sdhci-pci-gli.c
> +++ b/drivers/mmc/host/sdhci-pci-gli.c
> @@ -12,6 +12,7 @@
>  #include <linux/pci.h>
>  #include <linux/mmc/mmc.h>
>  #include <linux/delay.h>
> +#include <linux/of.h>
>  #include "sdhci.h"
>  #include "sdhci-pci.h"
>  #include "cqhci.h"
> @@ -114,8 +115,10 @@
>  #define   GLI_9755_WT_EN_OFF    0x0
>  
>  #define PCI_GLI_9755_PECONF   0x44
> -#define   PCI_GLI_9755_LFCLK    GENMASK(14, 12)
> -#define   PCI_GLI_9755_DMACLK   BIT(29)
> +#define   PCI_GLI_9755_LFCLK          GENMASK(14, 12)
> +#define   PCI_GLI_9755_DMACLK         BIT(29)

Please don't mix in white space changes.

> +#define   PCI_GLI_9755_INVERT_CD      BIT(30)
> +#define   PCI_GLI_9755_INVERT_WP      BIT(31)
>  
>  #define PCI_GLI_9755_CFG2          0x48
>  #define   PCI_GLI_9755_CFG2_L1DLY    GENMASK(28, 24)
> @@ -570,6 +573,18 @@ static void gl9755_hw_setting(struct sdhci_pci_slot *slot)
>  	gl9755_wt_on(pdev);
>  
>  	pci_read_config_dword(pdev, PCI_GLI_9755_PECONF, &value);
> +#ifdef CONFIG_OF
> +	if (pdev->dev.of_node) {

As Robin wrote, please remove #ifdef and if (pdev->dev.of_node)
because they are not needed.

> +		/*
> +		 * Apple ARM64 platforms using these chips may have
> +		 * inverted CD/WP detection.
> +		 */
> +		if (of_property_read_bool(pdev->dev.of_node, "cd-inverted"))
> +			value |= PCI_GLI_9755_INVERT_CD;
> +		if (of_property_read_bool(pdev->dev.of_node, "wp-inverted"))
> +			value |= PCI_GLI_9755_INVERT_WP;
> +	}
> +#endif
>  	value &= ~PCI_GLI_9755_LFCLK;
>  	value &= ~PCI_GLI_9755_DMACLK;
>  	pci_write_config_dword(pdev, PCI_GLI_9755_PECONF, value);
> 

Re: [PATCH v2 2/2] mmc: sdhci-pci-gli: GL975[50]: Issue 8/16-bit MMIO reads as 32-bit reads.

[Adrian Hunter]

On 12/12/2021 09:02, Hector Martin wrote:
> For some reason, <32-bit reads do not work on Apple ARM64 platforms with
> these chips (even though they do on other PCIe devices). Issue them as
> 32-bit reads instead. This is done unconditionally, as it shouldn't hurt
> even if not necessary.
> 
> Signed-off-by: Hector Martin <marcan@marcan.st>

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

> ---
>  drivers/mmc/host/sdhci-pci-gli.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci-pci-gli.c b/drivers/mmc/host/sdhci-pci-gli.c
> index ad742743a494..c6828e84db31 100644
> --- a/drivers/mmc/host/sdhci-pci-gli.c
> +++ b/drivers/mmc/host/sdhci-pci-gli.c
> @@ -906,7 +906,28 @@ static int gli_probe_slot_gl9763e(struct sdhci_pci_slot *slot)
>  	return 0;
>  }
>  
> +#define REG_OFFSET_IN_BITS(reg) ((reg) << 3 & 0x18)
> +
> +static u16 sdhci_gli_readw(struct sdhci_host *host, int reg)
> +{
> +	u32 val = readl(host->ioaddr + (reg & ~3));
> +	u16 word;
> +
> +	word = (val >> REG_OFFSET_IN_BITS(reg)) & 0xffff;
> +	return word;
> +}
> +
> +static u8 sdhci_gli_readb(struct sdhci_host *host, int reg)
> +{
> +	u32 val = readl(host->ioaddr + (reg & ~3));
> +	u8 byte = (val >> REG_OFFSET_IN_BITS(reg)) & 0xff;
> +
> +	return byte;
> +}
> +
>  static const struct sdhci_ops sdhci_gl9755_ops = {
> +	.read_w			= sdhci_gli_readw,
> +	.read_b			= sdhci_gli_readb,
>  	.set_clock		= sdhci_gl9755_set_clock,
>  	.enable_dma		= sdhci_pci_enable_dma,
>  	.set_bus_width		= sdhci_set_bus_width,
> @@ -926,6 +947,8 @@ const struct sdhci_pci_fixes sdhci_gl9755 = {
>  };
>  
>  static const struct sdhci_ops sdhci_gl9750_ops = {
> +	.read_w			= sdhci_gli_readw,
> +	.read_b			= sdhci_gli_readb,
>  	.read_l                 = sdhci_gl9750_readl,
>  	.set_clock		= sdhci_gl9750_set_clock,
>  	.enable_dma		= sdhci_pci_enable_dma,
> 

Re: [PATCH v2 1/2] mmc: sdhci-pci-gli: GL9755: Support for CD/WP inversion on OF platforms

[Hector Martin]

On 14/12/2021 19.41, Adrian Hunter wrote:
>>   #define PCI_GLI_9755_PECONF   0x44
>> -#define   PCI_GLI_9755_LFCLK    GENMASK(14, 12)
>> -#define   PCI_GLI_9755_DMACLK   BIT(29)
>> +#define   PCI_GLI_9755_LFCLK          GENMASK(14, 12)
>> +#define   PCI_GLI_9755_DMACLK         BIT(29)
> 
> Please don't mix in white space changes.

This is aligning the existing code with the additions; is it preferable 
to have the new ifdefs below misaligned?

>> +#define   PCI_GLI_9755_INVERT_CD      BIT(30)
>> +#define   PCI_GLI_9755_INVERT_WP      BIT(31)
>>   
>>   #define PCI_GLI_9755_CFG2          0x48
>>   #define   PCI_GLI_9755_CFG2_L1DLY    GENMASK(28, 24)
>> @@ -570,6 +573,18 @@ static void gl9755_hw_setting(struct sdhci_pci_slot *slot)
>>   	gl9755_wt_on(pdev);
>>   
>>   	pci_read_config_dword(pdev, PCI_GLI_9755_PECONF, &value);
>> +#ifdef CONFIG_OF
>> +	if (pdev->dev.of_node) {
> 
> As Robin wrote, please remove #ifdef and if (pdev->dev.of_node)
> because they are not needed.

Ack, will send out a v3 soon with the requested changes and hopefully it 
should be good to go :)

-- 
Hector Martin (marcan@marcan.st)
Public Key: https://mrcn.st/pub

Re: [PATCH v2 1/2] mmc: sdhci-pci-gli: GL9755: Support for CD/WP inversion on OF platforms

[Adrian Hunter]

On 14/12/2021 13:17, Hector Martin wrote:
> On 14/12/2021 19.41, Adrian Hunter wrote:
>>>   #define PCI_GLI_9755_PECONF   0x44
>>> -#define   PCI_GLI_9755_LFCLK    GENMASK(14, 12)
>>> -#define   PCI_GLI_9755_DMACLK   BIT(29)
>>> +#define   PCI_GLI_9755_LFCLK          GENMASK(14, 12)
>>> +#define   PCI_GLI_9755_DMACLK         BIT(29)
>>
>> Please don't mix in white space changes.
> 
> This is aligning the existing code with the additions; is it preferable to have the new ifdefs below misaligned?

White space changes should be a separate patch

> 
>>> +#define   PCI_GLI_9755_INVERT_CD      BIT(30)
>>> +#define   PCI_GLI_9755_INVERT_WP      BIT(31)
>>>     #define PCI_GLI_9755_CFG2          0x48
>>>   #define   PCI_GLI_9755_CFG2_L1DLY    GENMASK(28, 24)
>>> @@ -570,6 +573,18 @@ static void gl9755_hw_setting(struct sdhci_pci_slot *slot)
>>>       gl9755_wt_on(pdev);
>>>         pci_read_config_dword(pdev, PCI_GLI_9755_PECONF, &value);
>>> +#ifdef CONFIG_OF
>>> +    if (pdev->dev.of_node) {
>>
>> As Robin wrote, please remove #ifdef and if (pdev->dev.of_node)
>> because they are not needed.
> 
> Ack, will send out a v3 soon with the requested changes and hopefully it should be good to go :)
> 

Re: [PATCH v2 1/2] mmc: sdhci-pci-gli: GL9755: Support for CD/WP inversion on OF platforms

[Hector Martin]

On 13/12/2021 20.45, Robin Murphy wrote:
> On 2021-12-12 07:02, Hector Martin wrote:
>> This is required on some Apple ARM64 laptops using this controller.
>> As is typical on DT platforms, pull these quirks from the device tree
>> using the standard mmc bindings.
>>
>> See Documentation/devicetree/bindings/mmc/mmc-controller.yaml
>>
>> Signed-off-by: Hector Martin <marcan@marcan.st>
>> ---
>>   drivers/mmc/host/sdhci-pci-gli.c | 19 +++++++++++++++++--
>>   1 file changed, 17 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-pci-gli.c b/drivers/mmc/host/sdhci-pci-gli.c
>> index 4fd99c1e82ba..ad742743a494 100644
>> --- a/drivers/mmc/host/sdhci-pci-gli.c
>> +++ b/drivers/mmc/host/sdhci-pci-gli.c
>> @@ -12,6 +12,7 @@
>>   #include <linux/pci.h>
>>   #include <linux/mmc/mmc.h>
>>   #include <linux/delay.h>
>> +#include <linux/of.h>
>>   #include "sdhci.h"
>>   #include "sdhci-pci.h"
>>   #include "cqhci.h"
>> @@ -114,8 +115,10 @@
>>   #define   GLI_9755_WT_EN_OFF    0x0
>>   
>>   #define PCI_GLI_9755_PECONF   0x44
>> -#define   PCI_GLI_9755_LFCLK    GENMASK(14, 12)
>> -#define   PCI_GLI_9755_DMACLK   BIT(29)
>> +#define   PCI_GLI_9755_LFCLK          GENMASK(14, 12)
>> +#define   PCI_GLI_9755_DMACLK         BIT(29)
>> +#define   PCI_GLI_9755_INVERT_CD      BIT(30)
>> +#define   PCI_GLI_9755_INVERT_WP      BIT(31)
>>   
>>   #define PCI_GLI_9755_CFG2          0x48
>>   #define   PCI_GLI_9755_CFG2_L1DLY    GENMASK(28, 24)
>> @@ -570,6 +573,18 @@ static void gl9755_hw_setting(struct sdhci_pci_slot *slot)
>>   	gl9755_wt_on(pdev);
>>   
>>   	pci_read_config_dword(pdev, PCI_GLI_9755_PECONF, &value);
>> +#ifdef CONFIG_OF
>> +	if (pdev->dev.of_node) {
> 
> FWIW, of_property_read_bool() should handle a NULL node correctly, and 
> its stub be optimised away for !OF, so you don't really need the 
> explicit check or the #ifdef.

Thanks, removed it for v3!

-- 
Hector Martin (marcan@marcan.st)
Public Key: https://mrcn.st/pub