mmc: sdhci-pci: Add some CD GPIO related quirks
cover mmc: sdhci-pci: Add some CD GPIO related quirks
It appears that one of the supported platform magically worked with the custom IRQ handler (any hints how?) while having two PCB designs with an opposite CD sense level. Here is an attempt to fix it by quirking out CD GPIO.
Patch 1 introduces two additional quirks (it's done this way due to patch 3, see below). Patch 2 is an actual fix for the mentioned platform. If backported need to be taken with patch 1 together. Patch 3 is (RFT) clean up. The questionable part here is the locking scheme. Shouldn't we do something similar in the generic IRQ handler of SDHCI? Or Broxton case has something quite different in mind?
Patches 4-6 are dead-code removals. Patch 4 accompanying patch 2, patches 5-6 just similar to it, but (functionally) independent. Would like to hear if it's okay to do.
Any comments, hints, advice are welcome!
Andy Shevchenko (6):
mmc: sdhci: Introduce couple of quirks to ignore particular state of
CD GPIO
mmc: sdhci-pci: Read card detect from ACPI for Intel Merrifield
mmc: sdhci: Replace bxt_get_cd() with
SDHCI_QUIRK_CARD_DETECTION_IF_GPIO_HIGH
mmc: sdhci-pci: Remove dead code (struct sdhci_pci_data et al)
mmc: sdhci-pci: Remove dead code (cd_gpio, cd_irq et al)
mmc: sdhci-pci: Remove dead code (rst_n_gpio et al)
drivers/mmc/host/Makefile | 1 - drivers/mmc/host/sdhci-acpi.c | 24 +---- drivers/mmc/host/sdhci-pci-core.c | 166 +++-------------------------- drivers/mmc/host/sdhci-pci-data.c | 6 -- drivers/mmc/host/sdhci-pci.h | 5 - drivers/mmc/host/sdhci.c | 13 ++- drivers/mmc/host/sdhci.h | 4 + include/linux/mmc/sdhci-pci-data.h | 18 ---- 8 files changed, 27 insertions(+), 210 deletions(-) delete mode 100644 drivers/mmc/host/sdhci-pci-data.c delete mode 100644 include/linux/mmc/sdhci-pci-data.h
Signature · 1 line
2.33.0
PATCH 1/6 mmc: sdhci: Introduce couple of quirks to ignore particular state of CD GPIO
Some platforms may provide contradictory info in some states of CD GPIO line, and hence that state or states should be ignored. Introduce couple of quirks for that.
--- drivers/mmc/host/sdhci.c | 13 ++++++++----- drivers/mmc/host/sdhci.h | 4 ++++ 2 files changed, 12 insertions(+), 5 deletions(-)
drivers/mmc/host/sdhci.c +8 -5
@@ -2395,7 +2395,7 @@ EXPORT_SYMBOL_GPL(sdhci_set_ios);static int sdhci_get_cd (struct mmc_host *mmc) {struct sdhci_host *host =mmc_priv (mmc); -int gpio_cd =mmc_gpio_get_cd (mmc); +int gpio_cd;if (host->flags &SDHCI_DEVICE_DEAD )return 0 ;
@@ -2405,11 +2405,14 @@ static int sdhci_get_cd(struct mmc_host *mmc)return 1 ;/* -* Try slot gpio detect, if defined it take precedence -* over build in controller functionality +* Try slot GPIO detect, if defined it take precedence +* over build in controller functionality. */ -if (gpio_cd >=0 ) -return !!gpio_cd; + gpio_cd =mmc_gpio_get_cd (mmc); +if (gpio_cd ==0 && !(host->quirks2 &SDHCI_QUIRK_CARD_DETECTION_IF_GPIO_LOW )) +return 0 ; +if (gpio_cd >0 && !(host->quirks2 &SDHCI_QUIRK_CARD_DETECTION_IF_GPIO_HIGH )) +return 1 ;/* If polling, assume that the card is always present. */ if (host->quirks &SDHCI_QUIRK_BROKEN_CARD_DETECTION )
drivers/mmc/host/sdhci.h +4 -0
@@ -464,6 +464,10 @@ struct sdhci_host {#define SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN (1<<15)/* Controller has CRC in 136 bit Command Response */ #define SDHCI_QUIRK2_RSP_136_HAS_CRC (1<<16) +/* Controller requires additional card detection test on GPIO low */ +#define SDHCI_QUIRK_CARD_DETECTION_IF_GPIO_LOW (1<<17) +/* Controller requires additional card detection test on GPIO high */ +#define SDHCI_QUIRK_CARD_DETECTION_IF_GPIO_HIGH (1<<18)/* * Disable HW timeout if the requested timeout is more than the maximum * obtainable timeout.
Signature · 1 line
2.33.0
Adrian Hunter
maintainerRe: [PATCH v1 1/6] mmc: sdhci: Introduce couple of quirks to ignore particular state of CD GPIO
Quoted 3 lines · Andy Shevchenko
Some platforms may provide contradictory info in some states of CD GPIO line, and hence that state or states should be ignored. Introduce couple of quirks for that.
OK, I have looked closer now :-)
Hooking ->get_cd() should work, and that is preferred to new quirks. Please try that.
Quoted 55 lines
Signed-off-by Andy Shevchenko <[email protected]>--- drivers/mmc/host/sdhci.c | 13 ++++++++----- drivers/mmc/host/sdhci.h | 4 ++++ 2 files changed, 12 insertions(+), 5 deletions(-)drivers/mmc/host/sdhci.c +8
-5@@ -2395,7 +2395,7 @@ EXPORT_SYMBOL_GPL(sdhci_set_ios);static int sdhci_get_cd (struct mmc_host *mmc) {struct sdhci_host *host =mmc_priv (mmc); -int gpio_cd =mmc_gpio_get_cd (mmc); +int gpio_cd;if (host->flags &SDHCI_DEVICE_DEAD )return 0 ;@@ -2405,11 +2405,14 @@ static int sdhci_get_cd(struct mmc_host *mmc)return 1 ;/* -* Try slot gpio detect, if defined it take precedence -* over build in controller functionality +* Try slot GPIO detect, if defined it take precedence +* over build in controller functionality. */ -if (gpio_cd >=0 ) -return !!gpio_cd; + gpio_cd =mmc_gpio_get_cd (mmc); +if (gpio_cd ==0 && !(host->quirks2 &SDHCI_QUIRK_CARD_DETECTION_IF_GPIO_LOW )) +return 0 ; +if (gpio_cd >0 && !(host->quirks2 &SDHCI_QUIRK_CARD_DETECTION_IF_GPIO_HIGH )) +return 1 ;/* If polling, assume that the card is always present. */ if (host->quirks &SDHCI_QUIRK_BROKEN_CARD_DETECTION )drivers/mmc/host/sdhci.h +4
-0@@ -464,6 +464,10 @@ struct sdhci_host {#define SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN (1<<15)/* Controller has CRC in 136 bit Command Response */ #define SDHCI_QUIRK2_RSP_136_HAS_CRC (1<<16) +/* Controller requires additional card detection test on GPIO low */ +#define SDHCI_QUIRK_CARD_DETECTION_IF_GPIO_LOW (1<<17) +/* Controller requires additional card detection test on GPIO high */ +#define SDHCI_QUIRK_CARD_DETECTION_IF_GPIO_HIGH (1<<18)/* * Disable HW timeout if the requested timeout is more than the maximum * obtainable timeout.
PATCH 2/6 mmc: sdhci-pci: Read card detect from ACPI for Intel Merrifield
In reply to Andy ShevchenkoIntel Merrifield platform had been converted to use ACPI enumeration. However, the driver missed an update to retrieve card detect GPIO. Fix it here.
Unfortunately we can't rely on CD GPIO state because there are two different PCB designs in the wild that are using an opposite card detection sense and there is no way to distinguish those platforms, that's why ignore CD GPIO completely and use it only as an card plug or unplug event.
--- drivers/mmc/host/sdhci-pci-core.c | 8 ++++++++ 1 file changed, 8 insertions(+)
drivers/mmc/host/sdhci-pci-core.c +8 -0
@@ -1341,6 +1341,14 @@ static int intel_mrfld_mmc_probe_slot(struct sdhci_pci_slot *slot)MMC_CAP_1_8V_DDR ;break ;case INTEL_MRFLD_SD : + slot->cd_idx =0 ; +/* +* There are two PCB designs of SD card slot with the opposite +* card detection sense. Quirk this out by ignoring GPIO state +* completely. +*/ + slot->host->quirks2 |=SDHCI_QUIRK_CARD_DETECTION_IF_GPIO_LOW | +SDHCI_QUIRK_CARD_DETECTION_IF_GPIO_HIGH ; slot->host->quirks2 |=SDHCI_QUIRK2_NO_1_8_V ;break ;case INTEL_MRFLD_SDIO :
Signature · 1 line
2.33.0
PATCH 3/6 mmc: sdhci: Replace bxt_get_cd() with SDHCI_QUIRK_CARD_DETECTION_IF_GPIO_HIGH
In reply to Andy ShevchenkoThe bxt_get_cd() repeats the functionality of sdhci_get_cd() when SDHCI_QUIRK_CARD_DETECTION_IF_GPIO_HIGH is set. Thus, replace the method with the setting of the quirk.
--- drivers/mmc/host/sdhci-acpi.c | 24 +----------------------- drivers/mmc/host/sdhci-pci-core.c | 24 +----------------------- 2 files changed, 2 insertions(+), 46 deletions(-)
drivers/mmc/host/sdhci-acpi.c +1 -23
@@ -359,28 +359,6 @@ static inline bool sdhci_acpi_no_fixup_child_power(struct acpi_device *adev)#endif -static int bxt_get_cd (struct mmc_host *mmc) -{ -int gpio_cd =mmc_gpio_get_cd (mmc); -struct sdhci_host *host =mmc_priv (mmc); -unsigned long flags; -int ret =0 ; - -if (!gpio_cd) -return 0 ; - -spin_lock_irqsave (&host->lock, flags); - -if (host->flags &SDHCI_DEVICE_DEAD ) - goto out; - - ret = !!(sdhci_readl (host,SDHCI_PRESENT_STATE ) &SDHCI_CARD_PRESENT ); -out: -spin_unlock_irqrestore (&host->lock, flags); - -return ret; -} -static int intel_probe_slot (struct platform_device *pdev,struct acpi_device *adev) {struct sdhci_acpi_host *c =platform_get_drvdata (pdev);
@@ -393,7 +371,7 @@ static int intel_probe_slot(struct platform_device *pdev, struct acpi_device *ad host->timeout_clk =1000 ;/* 1000 kHz i.e. 1 MHz */ if (acpi_dev_hid_uid_match (adev,"80865ACA" ,NULL )) - host->mmc_host_ops.get_cd = bxt_get_cd; + host->quirks2 |=SDHCI_QUIRK_CARD_DETECTION_IF_GPIO_HIGH ;intel_dsm_init (intel_host, &pdev->dev, host->mmc);
drivers/mmc/host/sdhci-pci-core.c +1 -23
@@ -616,28 +616,6 @@ static int intel_select_drive_strength(struct mmc_card *card,return intel_host->drv_strength; } -static int bxt_get_cd (struct mmc_host *mmc) -{ -int gpio_cd =mmc_gpio_get_cd (mmc); -struct sdhci_host *host =mmc_priv (mmc); -unsigned long flags; -int ret =0 ; - -if (!gpio_cd) -return 0 ; - -spin_lock_irqsave (&host->lock, flags); - -if (host->flags &SDHCI_DEVICE_DEAD ) - goto out; - - ret = !!(sdhci_readl (host,SDHCI_PRESENT_STATE ) &SDHCI_CARD_PRESENT ); -out: -spin_unlock_irqrestore (&host->lock, flags); - -return ret; -} -#define SDHCI_INTEL_PWR_TIMEOUT_CNT 20#define SDHCI_INTEL_PWR_TIMEOUT_UDELAY 100
@@ -1171,7 +1149,7 @@ static int byt_sd_probe_slot(struct sdhci_pci_slot *slot) slot->chip->pdev->device ==PCI_DEVICE_ID_INTEL_BXTM_SD || slot->chip->pdev->device ==PCI_DEVICE_ID_INTEL_APL_SD || slot->chip->pdev->device == PCI_DEVICE_ID_INTEL_GLK_SD) - slot->host->mmc_host_ops.get_cd = bxt_get_cd; + slot->host->quirks2 |= SDHCI_QUIRK_CARD_DETECTION_IF_GPIO_HIGH;if (slot->chip->pdev->subsystem_vendor ==PCI_VENDOR_ID_NI && slot->chip->pdev->subsystem_device ==PCI_SUBDEVICE_ID_NI_78E3 )
Signature · 1 line
2.33.0
PATCH 4/6 mmc: sdhci-pci: Remove dead code (struct sdhci_pci_data et al)
In reply to Andy ShevchenkoThe last user of this struct gone couple of releases ago. Besides that there were not so many users of this API for 10+ years: one is implied above Intel Merrifield (added 2016-08-31, removed 2021-02-11), and another is Intel Sunrisepoint (added 2015-02-06, removed 2017-03-20).
Effectively this is a revert of the commit 52c506f0bc72 ("mmc: sdhci-pci:
add platform data").
--- drivers/mmc/host/Makefile | 1 - drivers/mmc/host/sdhci-pci-core.c | 31 ++++-------------------------- drivers/mmc/host/sdhci-pci-data.c | 6 ------ drivers/mmc/host/sdhci-pci.h | 1 - include/linux/mmc/sdhci-pci-data.h | 18 ----------------- 5 files changed, 4 insertions(+), 53 deletions(-) delete mode 100644 drivers/mmc/host/sdhci-pci-data.c delete mode 100644 include/linux/mmc/sdhci-pci-data.h
drivers/mmc/host/Makefile +0 -1
@@ -14,7 +14,6 @@ obj-$(CONFIG_MMC_SDHCI) += sdhci.o obj-$(CONFIG_MMC_SDHCI_PCI) += sdhci-pci.o sdhci-pci-y += sdhci-pci-core.o sdhci-pci-o2micro.o sdhci-pci-arasan.o \ sdhci-pci-dwc-mshc.o sdhci-pci-gli.o -obj-$(subst m,y,$(CONFIG_MMC_SDHCI_PCI)) += sdhci-pci-data.o obj-$(CONFIG_MMC_SDHCI_ACPI) += sdhci-acpi.o obj-$(CONFIG_MMC_SDHCI_PXAV3) += sdhci-pxav3.o obj-$(CONFIG_MMC_SDHCI_PXAV2) += sdhci-pxav2.o
drivers/mmc/host/sdhci-pci-core.c +4 -27
@@ -17,8 +17,6 @@#include <linux/dma-mapping.h> #include <linux/slab.h> #include <linux/device.h> -#include <linux/mmc/host.h> -#include <linux/mmc/mmc.h> #include <linux/scatterlist.h> #include <linux/io.h> #include <linux/iopoll.h>
@@ -26,11 +24,13 @@#include <linux/pm_runtime.h> #include <linux/pm_qos.h> #include <linux/debugfs.h> -#include <linux/mmc/slot-gpio.h> -#include <linux/mmc/sdhci-pci-data.h> #include <linux/acpi.h> #include <linux/dmi.h> +#include <linux/mmc/host.h> +#include <linux/mmc/mmc.h> +#include <linux/mmc/slot-gpio.h> +#ifdef CONFIG_X86 #include <asm/iosf_mbi.h> #endif
@@ -2116,22 +2116,6 @@ static struct sdhci_pci_slot *sdhci_pci_probe_slot( slot->cd_gpio = -EINVAL ; slot->cd_idx =-1 ; -/* Retrieve platform data if there is any */ -if (*sdhci_pci_get_data) - slot->data =sdhci_pci_get_data (pdev, slotno); - -if (slot->data) { -if (slot->data->setup) { - ret = slot->data->setup (slot->data); -if (ret) { -dev_err (&pdev->dev,"platform setup failed\n" ); - goto free; - } - } - slot->rst_n_gpio = slot->data->rst_n_gpio; - slot->cd_gpio = slot->data->cd_gpio; - } - host->hw_name ="PCI" ; host->ops = chip->fixes && chip->fixes->ops ? chip->fixes->ops :
@@ -2218,10 +2202,6 @@ static struct sdhci_pci_slot *sdhci_pci_probe_slot( chip->fixes->remove_slot (slot,0 ); cleanup: -if (slot->data && slot->data->cleanup) - slot->data->cleanup (slot->data); - -free:sdhci_free_host (host);return ERR_PTR (ret);
@@ -2244,9 +2224,6 @@ static void sdhci_pci_remove_slot(struct sdhci_pci_slot *slot)if (slot->chip->fixes && slot->chip->fixes->remove_slot) slot->chip->fixes->remove_slot (slot, dead); -if (slot->data && slot->data->cleanup) - slot->data->cleanup (slot->data); -sdhci_free_host (slot->host); }
drivers/mmc/host/sdhci-pci-data.c deleted +0 -6
@@ -1,6 +0,0 @@ -// SPDX-License-Identifier: GPL-2.0-only -#include <linux/module.h> -#include <linux/mmc/sdhci-pci-data.h> - -struct sdhci_pci_data *(*sdhci_pci_get_data)(struct pci_dev *pdev,int slotno); -EXPORT_SYMBOL_GPL (sdhci_pci_get_data);
drivers/mmc/host/sdhci-pci.h +0 -1
@@ -156,7 +156,6 @@ struct sdhci_pci_fixes {struct sdhci_pci_slot {struct sdhci_pci_chip *chip;struct sdhci_host *host; -struct sdhci_pci_data *data;int rst_n_gpio;int cd_gpio;
include/linux/mmc/sdhci-pci-data.h deleted +0 -18
@@ -1,18 +0,0 @@ -/* SPDX-License-Identifier: GPL-2.0 */ -#ifndef LINUX_MMC_SDHCI_PCI_DATA_H -#define LINUX_MMC_SDHCI_PCI_DATA_H - -struct pci_dev ; - -struct sdhci_pci_data { -struct pci_dev *pdev; -int slotno; -int rst_n_gpio;/* Set to -EINVAL if unused */ -int cd_gpio;/* Set to -EINVAL if unused */ -int (*setup)(struct sdhci_pci_data *data); -void (*cleanup)(struct sdhci_pci_data *data); -}; - -extern struct sdhci_pci_data *(*sdhci_pci_get_data)(struct pci_dev *pdev, -int slotno); -#endif
Signature · 1 line
2.33.0
Adrian Hunter
maintainerRe: [PATCH v1 4/6] mmc: sdhci-pci: Remove dead code (struct sdhci_pci_data et al)
Quoted 4 lines · Andy Shevchenko
The last user of this struct gone couple of releases ago. Besides that there were not so many users of this API for 10+ years: one is implied above Intel Merrifield (added 2016-08-31, removed 2021-02-11), and another is Intel Sunrisepoint (added 2015-02-06, removed 2017-03-20).
Wouldn't hurt to identify the commits here if it is not too much trouble.
Quoted 149 lines
Effectively this is a revert of the commit
52c506f0bc72("mmc: sdhci-pci:
add platform data").Signed-off-by Andy Shevchenko <[email protected]>--- drivers/mmc/host/Makefile | 1 - drivers/mmc/host/sdhci-pci-core.c | 31 ++++-------------------------- drivers/mmc/host/sdhci-pci-data.c | 6 ------ drivers/mmc/host/sdhci-pci.h | 1 - include/linux/mmc/sdhci-pci-data.h | 18 ----------------- 5 files changed, 4 insertions(+), 53 deletions(-) delete mode 100644 drivers/mmc/host/sdhci-pci-data.c delete mode 100644 include/linux/mmc/sdhci-pci-data.hdrivers/mmc/host/Makefile +0
-1@@ -14,7 +14,6 @@ obj-$(CONFIG_MMC_SDHCI) += sdhci.o obj-$(CONFIG_MMC_SDHCI_PCI) += sdhci-pci.o sdhci-pci-y += sdhci-pci-core.o sdhci-pci-o2micro.o sdhci-pci-arasan.o \ sdhci-pci-dwc-mshc.o sdhci-pci-gli.o -obj-$(subst m,y,$(CONFIG_MMC_SDHCI_PCI)) += sdhci-pci-data.o obj-$(CONFIG_MMC_SDHCI_ACPI) += sdhci-acpi.o obj-$(CONFIG_MMC_SDHCI_PXAV3) += sdhci-pxav3.o obj-$(CONFIG_MMC_SDHCI_PXAV2) += sdhci-pxav2.odrivers/mmc/host/sdhci-pci-core.c +4
-27@@ -17,8 +17,6 @@#include <linux/dma-mapping.h> #include <linux/slab.h> #include <linux/device.h> -#include <linux/mmc/host.h> -#include <linux/mmc/mmc.h> #include <linux/scatterlist.h> #include <linux/io.h> #include <linux/iopoll.h> @@ -26,11 +24,13 @@#include <linux/pm_runtime.h> #include <linux/pm_qos.h> #include <linux/debugfs.h> -#include <linux/mmc/slot-gpio.h> -#include <linux/mmc/sdhci-pci-data.h> #include <linux/acpi.h> #include <linux/dmi.h> +#include <linux/mmc/host.h> +#include <linux/mmc/mmc.h> +#include <linux/mmc/slot-gpio.h> +#ifdef CONFIG_X86 #include <asm/iosf_mbi.h> #endif @@ -2116,22 +2116,6 @@ static struct sdhci_pci_slot *sdhci_pci_probe_slot( slot->cd_gpio = -EINVAL ; slot->cd_idx =-1 ; -/* Retrieve platform data if there is any */ -if (*sdhci_pci_get_data) - slot->data =sdhci_pci_get_data (pdev, slotno); - -if (slot->data) { -if (slot->data->setup) { - ret = slot->data->setup (slot->data); -if (ret) { -dev_err (&pdev->dev,"platform setup failed\n" ); - goto free; - } - } - slot->rst_n_gpio = slot->data->rst_n_gpio; - slot->cd_gpio = slot->data->cd_gpio; - } - host->hw_name ="PCI" ; host->ops = chip->fixes && chip->fixes->ops ? chip->fixes->ops :@@ -2218,10 +2202,6 @@ static struct sdhci_pci_slot *sdhci_pci_probe_slot( chip->fixes->remove_slot (slot,0 ); cleanup: -if (slot->data && slot->data->cleanup) - slot->data->cleanup (slot->data); - -free:sdhci_free_host (host);return ERR_PTR (ret);@@ -2244,9 +2224,6 @@ static void sdhci_pci_remove_slot(struct sdhci_pci_slot *slot)if (slot->chip->fixes && slot->chip->fixes->remove_slot) slot->chip->fixes->remove_slot (slot, dead); -if (slot->data && slot->data->cleanup) - slot->data->cleanup (slot->data); -sdhci_free_host (slot->host); }drivers/mmc/host/sdhci-pci-data.c deleted +0
-6@@ -1,6 +0,0 @@ -// SPDX-License-Identifier: GPL-2.0-only -#include <linux/module.h> -#include <linux/mmc/sdhci-pci-data.h> - -struct sdhci_pci_data *(*sdhci_pci_get_data)(struct pci_dev *pdev,int slotno); -EXPORT_SYMBOL_GPL (sdhci_pci_get_data);drivers/mmc/host/sdhci-pci.h +0
-1@@ -156,7 +156,6 @@ struct sdhci_pci_fixes {struct sdhci_pci_slot {struct sdhci_pci_chip *chip;struct sdhci_host *host; -struct sdhci_pci_data *data;int rst_n_gpio;int cd_gpio;include/linux/mmc/sdhci-pci-data.h deleted +0
-18@@ -1,18 +0,0 @@ -/* SPDX-License-Identifier: GPL-2.0 */ -#ifndef LINUX_MMC_SDHCI_PCI_DATA_H -#define LINUX_MMC_SDHCI_PCI_DATA_H - -struct pci_dev ; - -struct sdhci_pci_data { -struct pci_dev *pdev; -int slotno; -int rst_n_gpio;/* Set to -EINVAL if unused */ -int cd_gpio;/* Set to -EINVAL if unused */ -int (*setup)(struct sdhci_pci_data *data); -void (*cleanup)(struct sdhci_pci_data *data); -}; - -extern struct sdhci_pci_data *(*sdhci_pci_get_data)(struct pci_dev *pdev, -int slotno); -#endif
PATCH 5/6 mmc: sdhci-pci: Remove dead code (cd_gpio, cd_irq et al)
In reply to Andy ShevchenkoThe last user of this struct gone couple of releases ago. Remove the dead code for good and encourage people to use MMC core functionality for that.
--- drivers/mmc/host/sdhci-pci-core.c | 76 +------------------------------ drivers/mmc/host/sdhci-pci.h | 2 - 2 files changed, 1 insertion(+), 77 deletions(-)
drivers/mmc/host/sdhci-pci-core.c +1 -75
@@ -345,73 +345,6 @@ static int pch_hc_probe_slot(struct sdhci_pci_slot *slot)return 0 ; } -#ifdef CONFIG_PM - -static irqreturn_t sdhci_pci_sd_cd (int irq,void *dev_id) -{ -struct sdhci_pci_slot *slot = dev_id; -struct sdhci_host *host = slot->host; - -mmc_detect_change (host->mmc,msecs_to_jiffies (200 )); -return IRQ_HANDLED ; -} - -static void sdhci_pci_add_own_cd (struct sdhci_pci_slot *slot) -{ -int err, irq, gpio = slot->cd_gpio; - - slot->cd_gpio = -EINVAL ; - slot->cd_irq = -EINVAL ; - -if (!gpio_is_valid (gpio)) -return ; - - err =devm_gpio_request (&slot->chip->pdev->dev, gpio,"sd_cd" ); -if (err <0 ) - goto out; - - err =gpio_direction_input (gpio); -if (err <0 ) - goto out_free; - - irq =gpio_to_irq (gpio); -if (irq <0 ) - goto out_free; - - err =request_irq (irq, sdhci_pci_sd_cd,IRQF_TRIGGER_RISING | -IRQF_TRIGGER_FALLING ,"sd_cd" , slot); -if (err) - goto out_free; - - slot->cd_gpio = gpio; - slot->cd_irq = irq; - -return ; - -out_free: -devm_gpio_free (&slot->chip->pdev->dev, gpio); -out: -dev_warn (&slot->chip->pdev->dev,"failed to setup card detect wake up\n" ); -} - -static void sdhci_pci_remove_own_cd (struct sdhci_pci_slot *slot) -{ -if (slot->cd_irq >=0 ) -free_irq (slot->cd_irq, slot); -} - -#else - -static inline void sdhci_pci_add_own_cd (struct sdhci_pci_slot *slot) -{ -} - -static inline void sdhci_pci_remove_own_cd (struct sdhci_pci_slot *slot) -{ -} - -#endif -static int mfd_emmc_probe_slot (struct sdhci_pci_slot *slot) { slot->host->mmc->caps |=MMC_CAP_8_BIT_DATA |MMC_CAP_NONREMOVABLE ;
@@ -2113,7 +2046,6 @@ static struct sdhci_pci_slot *sdhci_pci_probe_slot( slot->chip = chip; slot->host = host; slot->rst_n_gpio = -EINVAL ; - slot->cd_gpio = -EINVAL ; slot->cd_idx =-1 ; host->hw_name ="PCI" ;
@@ -2184,15 +2116,11 @@ static struct sdhci_pci_slot *sdhci_pci_probe_slot(if (ret) goto remove; -sdhci_pci_add_own_cd (slot); -/* * Check if the chip needs a separate GPIO for card detect to wake up * from runtime suspend. If it is not there, don't allow runtime PM. -* Note sdhci_pci_add_own_cd() sets slot->cd_gpio to -EINVAL on failure. */ -if (chip->fixes && chip->fixes->own_cd_for_runtime_pm && - !gpio_is_valid (slot->cd_gpio) && slot->cd_idx <0 ) +if (chip->fixes && chip->fixes->own_cd_for_runtime_pm && slot->cd_idx <0 ) chip->allow_runtime_pm = false;return slot;
@@ -2212,8 +2140,6 @@ static void sdhci_pci_remove_slot(struct sdhci_pci_slot *slot)int dead;u32 scratch; -sdhci_pci_remove_own_cd (slot); - dead =0 ; scratch =readl (slot->host->ioaddr +SDHCI_INT_STATUS );if (scratch == (u32)-1 )
drivers/mmc/host/sdhci-pci.h +0 -2
@@ -158,8 +158,6 @@ struct sdhci_pci_slot {struct sdhci_host *host;int rst_n_gpio; -int cd_gpio; -int cd_irq;int cd_idx;bool cd_override_level;
Signature · 1 line
2.33.0
Adrian Hunter
maintainerRe: [PATCH v1 5/6] mmc: sdhci-pci: Remove dead code (cd_gpio, cd_irq et al)
Quoted 3 lines · Andy Shevchenko
The last user of this struct gone couple of releases ago. Remove the dead code for good and encourage people to use MMC core functionality for that.
So this is dependent on patch 4 i.e. that patch removed the references to the code being removed in this patch. Please make a note of that in the commit message.
Quoted 133 lines
Signed-off-by Andy Shevchenko <[email protected]>--- drivers/mmc/host/sdhci-pci-core.c | 76 +------------------------------ drivers/mmc/host/sdhci-pci.h | 2 - 2 files changed, 1 insertion(+), 77 deletions(-)drivers/mmc/host/sdhci-pci-core.c +1
-75@@ -345,73 +345,6 @@ static int pch_hc_probe_slot(struct sdhci_pci_slot *slot)return 0 ; } -#ifdef CONFIG_PM - -static irqreturn_t sdhci_pci_sd_cd (int irq,void *dev_id) -{ -struct sdhci_pci_slot *slot = dev_id; -struct sdhci_host *host = slot->host; - -mmc_detect_change (host->mmc,msecs_to_jiffies (200 )); -return IRQ_HANDLED ; -} - -static void sdhci_pci_add_own_cd (struct sdhci_pci_slot *slot) -{ -int err, irq, gpio = slot->cd_gpio; - - slot->cd_gpio = -EINVAL ; - slot->cd_irq = -EINVAL ; - -if (!gpio_is_valid (gpio)) -return ; - - err =devm_gpio_request (&slot->chip->pdev->dev, gpio,"sd_cd" ); -if (err <0 ) - goto out; - - err =gpio_direction_input (gpio); -if (err <0 ) - goto out_free; - - irq =gpio_to_irq (gpio); -if (irq <0 ) - goto out_free; - - err =request_irq (irq, sdhci_pci_sd_cd,IRQF_TRIGGER_RISING | -IRQF_TRIGGER_FALLING ,"sd_cd" , slot); -if (err) - goto out_free; - - slot->cd_gpio = gpio; - slot->cd_irq = irq; - -return ; - -out_free: -devm_gpio_free (&slot->chip->pdev->dev, gpio); -out: -dev_warn (&slot->chip->pdev->dev,"failed to setup card detect wake up\n" ); -} - -static void sdhci_pci_remove_own_cd (struct sdhci_pci_slot *slot) -{ -if (slot->cd_irq >=0 ) -free_irq (slot->cd_irq, slot); -} - -#else - -static inline void sdhci_pci_add_own_cd (struct sdhci_pci_slot *slot) -{ -} - -static inline void sdhci_pci_remove_own_cd (struct sdhci_pci_slot *slot) -{ -} - -#endif -static int mfd_emmc_probe_slot (struct sdhci_pci_slot *slot) { slot->host->mmc->caps |=MMC_CAP_8_BIT_DATA |MMC_CAP_NONREMOVABLE ;@@ -2113,7 +2046,6 @@ static struct sdhci_pci_slot *sdhci_pci_probe_slot( slot->chip = chip; slot->host = host; slot->rst_n_gpio = -EINVAL ; - slot->cd_gpio = -EINVAL ; slot->cd_idx =-1 ; host->hw_name ="PCI" ;@@ -2184,15 +2116,11 @@ static struct sdhci_pci_slot *sdhci_pci_probe_slot(if (ret) goto remove; -sdhci_pci_add_own_cd (slot); -/* * Check if the chip needs a separate GPIO for card detect to wake up * from runtime suspend. If it is not there, don't allow runtime PM. -* Note sdhci_pci_add_own_cd() sets slot->cd_gpio to -EINVAL on failure. */ -if (chip->fixes && chip->fixes->own_cd_for_runtime_pm && - !gpio_is_valid (slot->cd_gpio) && slot->cd_idx <0 ) +if (chip->fixes && chip->fixes->own_cd_for_runtime_pm && slot->cd_idx <0 ) chip->allow_runtime_pm = false;return slot;@@ -2212,8 +2140,6 @@ static void sdhci_pci_remove_slot(struct sdhci_pci_slot *slot)int dead;u32 scratch; -sdhci_pci_remove_own_cd (slot); - dead =0 ; scratch =readl (slot->host->ioaddr +SDHCI_INT_STATUS );if (scratch == (u32)-1 )drivers/mmc/host/sdhci-pci.h +0
-2@@ -158,8 +158,6 @@ struct sdhci_pci_slot {struct sdhci_host *host;int rst_n_gpio; -int cd_gpio; -int cd_irq;int cd_idx;bool cd_override_level;
PATCH 6/6 mmc: sdhci-pci: Remove dead code (rst_n_gpio et al)
In reply to Andy ShevchenkoThere is no user of this member. Remove the dead code for good.
--- drivers/mmc/host/sdhci-pci-core.c | 27 --------------------------- drivers/mmc/host/sdhci-pci.h | 2 -- 2 files changed, 29 deletions(-)
drivers/mmc/host/sdhci-pci-core.c +0 -27
@@ -1900,21 +1900,6 @@ int sdhci_pci_enable_dma(struct sdhci_host *host)return 0 ; } -static void sdhci_pci_gpio_hw_reset (struct sdhci_host *host) -{ -struct sdhci_pci_slot *slot =sdhci_priv (host); -int rst_n_gpio = slot->rst_n_gpio; - -if (!gpio_is_valid (rst_n_gpio)) -return ; -gpio_set_value_cansleep (rst_n_gpio,0 ); -/* For eMMC, minimum is 1us but give it 10us for good measure */ -udelay (10 ); -gpio_set_value_cansleep (rst_n_gpio,1 ); -/* For eMMC, minimum is 200us but give it 300us for good measure */ -usleep_range (300 ,1000 ); -} -static void sdhci_pci_hw_reset (struct sdhci_host *host) {struct sdhci_pci_slot *slot =sdhci_priv (host);
@@ -2045,7 +2030,6 @@ static struct sdhci_pci_slot *sdhci_pci_probe_slot( slot->chip = chip; slot->host = host; - slot->rst_n_gpio = -EINVAL ; slot->cd_idx =-1 ; host->hw_name ="PCI" ;
@@ -2071,17 +2055,6 @@ static struct sdhci_pci_slot *sdhci_pci_probe_slot( goto cleanup; } -if (gpio_is_valid (slot->rst_n_gpio)) { -if (!devm_gpio_request (&pdev->dev, slot->rst_n_gpio,"eMMC_reset" )) { -gpio_direction_output (slot->rst_n_gpio,1 ); - slot->host->mmc->caps |=MMC_CAP_HW_RESET ; - slot->hw_reset = sdhci_pci_gpio_hw_reset; - }else { -dev_warn (&pdev->dev,"failed to request rst_n_gpio\n" ); - slot->rst_n_gpio = -EINVAL ; - } - } - host->mmc->pm_caps =MMC_PM_KEEP_POWER ; host->mmc->slotno = slotno; host->mmc->caps2 |=MMC_CAP2_NO_PRESCAN_POWERUP ;
drivers/mmc/host/sdhci-pci.h +0 -2
@@ -157,8 +157,6 @@ struct sdhci_pci_slot {struct sdhci_pci_chip *chip;struct sdhci_host *host; -int rst_n_gpio; -int cd_idx;bool cd_override_level;
Signature · 1 line
2.33.0
Adrian Hunter
maintainerRe: [PATCH v1 6/6] mmc: sdhci-pci: Remove dead code (rst_n_gpio et al)
Quoted 1 line · Andy Shevchenko
There is no user of this member. Remove the dead code for good.
So this is dependent on patch 5 i.e. that patch removed the references to the code being removed in this patch. Please make a note of that in the commit message.
Quoted 73 lines
Signed-off-by Andy Shevchenko <[email protected]>--- drivers/mmc/host/sdhci-pci-core.c | 27 --------------------------- drivers/mmc/host/sdhci-pci.h | 2 -- 2 files changed, 29 deletions(-)drivers/mmc/host/sdhci-pci-core.c +0
-27@@ -1900,21 +1900,6 @@ int sdhci_pci_enable_dma(struct sdhci_host *host)return 0 ; } -static void sdhci_pci_gpio_hw_reset (struct sdhci_host *host) -{ -struct sdhci_pci_slot *slot =sdhci_priv (host); -int rst_n_gpio = slot->rst_n_gpio; - -if (!gpio_is_valid (rst_n_gpio)) -return ; -gpio_set_value_cansleep (rst_n_gpio,0 ); -/* For eMMC, minimum is 1us but give it 10us for good measure */ -udelay (10 ); -gpio_set_value_cansleep (rst_n_gpio,1 ); -/* For eMMC, minimum is 200us but give it 300us for good measure */ -usleep_range (300 ,1000 ); -} -static void sdhci_pci_hw_reset (struct sdhci_host *host) {struct sdhci_pci_slot *slot =sdhci_priv (host);@@ -2045,7 +2030,6 @@ static struct sdhci_pci_slot *sdhci_pci_probe_slot( slot->chip = chip; slot->host = host; - slot->rst_n_gpio = -EINVAL ; slot->cd_idx =-1 ; host->hw_name ="PCI" ;@@ -2071,17 +2055,6 @@ static struct sdhci_pci_slot *sdhci_pci_probe_slot( goto cleanup; } -if (gpio_is_valid (slot->rst_n_gpio)) { -if (!devm_gpio_request (&pdev->dev, slot->rst_n_gpio,"eMMC_reset" )) { -gpio_direction_output (slot->rst_n_gpio,1 ); - slot->host->mmc->caps |=MMC_CAP_HW_RESET ; - slot->hw_reset = sdhci_pci_gpio_hw_reset; - }else { -dev_warn (&pdev->dev,"failed to request rst_n_gpio\n" ); - slot->rst_n_gpio = -EINVAL ; - } - } - host->mmc->pm_caps =MMC_PM_KEEP_POWER ; host->mmc->slotno = slotno; host->mmc->caps2 |=MMC_CAP2_NO_PRESCAN_POWERUP ;drivers/mmc/host/sdhci-pci.h +0
-2@@ -157,8 +157,6 @@ struct sdhci_pci_slot {struct sdhci_pci_chip *chip;struct sdhci_host *host; -int rst_n_gpio; -int cd_idx;bool cd_override_level;
Quoted 12 lines · Andy Shevchenko
It appears that one of the supported platform magically worked with the custom IRQ handler (any hints how?) while having two PCB designs with an opposite CD sense level. Here is an attempt to fix it by quirking out CD GPIO.
Patch 1 introduces two additional quirks (it's done this way due to patch 3, see below). Patch 2 is an actual fix for the mentioned platform. If backported need to be taken with patch 1 together. Patch 3 is (RFT) clean up. The questionable part here is the locking scheme. Shouldn't we do something similar in the generic IRQ handler of SDHCI? Or Broxton case has something quite different in mind?
Quoted 5 lines
Patches 4-6 are dead-code removals. Patch 4 accompanying patch 2, patches 5-6 just similar to it, but (functionally) independent. Would like to hear if it's okay to do.
Any comments, hints, advice are welcome!
Adrian, Ulf, any comments on this? At least first two fix the regression and would be nice to have them in sooner than later (I can split them separately if required).
Signature · 2 lines
With Best Regards, Andy Shevchenko
Adrian Hunter
maintainerQuoted 17 lines · Andy Shevchenko
Quoted 16 lines · Andy Shevchenko
It appears that one of the supported platform magically worked with the custom IRQ handler (any hints how?) while having two PCB designs with an opposite CD sense level. Here is an attempt to fix it by quirking out CD GPIO.
Patch 1 introduces two additional quirks (it's done this way due to patch 3, see below). Patch 2 is an actual fix for the mentioned platform. If backported need to be taken with patch 1 together. Patch 3 is (RFT) clean up. The questionable part here is the locking scheme. Shouldn't we do something similar in the generic IRQ handler of SDHCI? Or Broxton case has something quite different in mind?
Patches 4-6 are dead-code removals. Patch 4 accompanying patch 2, patches 5-6 just similar to it, but (functionally) independent. Would like to hear if it's okay to do.
Quoted 5 lines
Quoted 1 line
Any comments, hints, advice are welcome!
Adrian, Ulf, any comments on this? At least first two fix the regression and would be nice to have them in sooner than later (I can split them separately if required).
I am not sure we need new quirks, given that we can just hook the callback and do anything that way. However I really haven't had time to look closely yet, sorry.
Quoted 2 lines · Adrian Hunter
Quoted 1 line · Andy Shevchenko
On Tue, Oct 05, 2021 at 01:24:24PM +0300, Andy Shevchenko wrote:
...
Quoted 7 lines
Quoted 3 lines
Adrian, Ulf, any comments on this? At least first two fix the regression and would be nice to have them in sooner than later (I can split them separately if required).
I am not sure we need new quirks, given that we can just hook the callback and do anything that way. However I really haven't had time to look closely yet, sorry.
Adrian, thanks for review, I'll address your comments in v2.
Signature · 2 lines
With Best Regards, Andy Shevchenko
It looks like you were previously using the cd-gpios DT property to determine this. It also sounds like you switched from DT to ACPI so you lost the ability to use this field?
Can you not use something like the following:
https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/kernel/v5.10/drivers/mmc/host/sdhci-acpi.c;l=945
p.s.,
Sorry for resending. I sent it as rich text before.
Quoted 29 lines · Andy Shevchenko
Quoted 12 lines · Andy Shevchenko
It appears that one of the supported platform magically worked with the custom IRQ handler (any hints how?) while having two PCB designs with an opposite CD sense level. Here is an attempt to fix it by quirking out CD GPIO.
Patch 1 introduces two additional quirks (it's done this way due to patch 3, see below). Patch 2 is an actual fix for the mentioned platform. If backported need to be taken with patch 1 together. Patch 3 is (RFT) clean up. The questionable part here is the locking scheme. Shouldn't we do something similar in the generic IRQ handler of SDHCI? Or Broxton case has something quite different in mind?
Quoted 5 lines
Patches 4-6 are dead-code removals. Patch 4 accompanying patch 2, patches 5-6 just similar to it, but (functionally) independent. Would like to hear if it's okay to do.
Any comments, hints, advice are welcome!
Adrian, Ulf, any comments on this? At least first two fix the regression and would be nice to have them in sooner than later (I can split them separately if required).
--
With Best Regards,
Andy Shevchenko
Quoted 3 lines · Raul Rangel
It looks like you were previously using the
cd-gpiosDT property to determine this.
No, it's not the case. The case is that we switched from hard coded stuff (board files) to ACPI.
Quoted 5 lines
It also sounds like you switched from DT to ACPI so you lost the ability to use this field?
Can you not use something like the following:
https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/kernel/v5.10/drivers/mmc/host/sdhci-acpi.c;l=945
No. It won't work. Anything like this won't work.
The problem here is on PCB level due to different uSD connectors () being use: one is Active High, the other is Active Low for CD signal.
...
Quoted 1 line
Quoted 1 line
Quoted 1 line
Any comments, hints, advice are welcome!
Signature · 2 lines
With Best Regards, Andy Shevchenko