* [PATCH] misc: rtsx: rts5249 support runtime PM @ 2020-11-24 6:02 ricky_wu 2020-11-24 8:40 ` kernel test robot ` (2 more replies) 0 siblings, 3 replies; 8+ messages in thread From: ricky_wu @ 2020-11-24 6:02 UTC (permalink / raw) To: arnd, gregkh, ulf.hansson, bhelgaas, ricky_wu, vaibhavgupta40, kdlnx, dianders, rmfrfs, lee.jones, linux-kernel, linux-mmc From: Ricky Wu <ricky_wu@realtek.com> rtsx_pci_sdmmc: add to support autosuspend when the rtd3_en is set rtsx_pcr: add callback functions about runtime PM add delay_work(rtd3_work) to decrease usage count to 0 when staying at idle over 10 sec rts5249: add extra flow at init function to support wakeup from d3 and set rtd3_en from register setting Signed-off-by: Ricky Wu <ricky_wu@realtek.com> --- drivers/misc/cardreader/rts5249.c | 25 ++++-- drivers/misc/cardreader/rtsx_pcr.c | 122 +++++++++++++++++++++++++++-- drivers/misc/cardreader/rtsx_pcr.h | 1 + drivers/mmc/host/rtsx_pci_sdmmc.c | 18 ++++- include/linux/rtsx_pci.h | 1 + 5 files changed, 152 insertions(+), 15 deletions(-) diff --git a/drivers/misc/cardreader/rts5249.c b/drivers/misc/cardreader/rts5249.c index b85279f1fc5e..1da3b1ca1121 100644 --- a/drivers/misc/cardreader/rts5249.c +++ b/drivers/misc/cardreader/rts5249.c @@ -65,7 +65,6 @@ static void rtsx_base_fetch_vendor_settings(struct rtsx_pcr *pcr) pcr_dbg(pcr, "skip fetch vendor setting\n"); return; } - pcr->aspm_en = rtsx_reg_to_aspm(reg); pcr->sd30_drive_sel_1v8 = rtsx_reg_to_sd30_drive_sel_1v8(reg); pcr->card_drive_sel &= 0x3F; @@ -73,6 +72,8 @@ static void rtsx_base_fetch_vendor_settings(struct rtsx_pcr *pcr) pci_read_config_dword(pdev, PCR_SETTING_REG2, ®); pcr_dbg(pcr, "Cfg 0x%x: 0x%x\n", PCR_SETTING_REG2, reg); + + pcr->rtd3_en = rtsx_reg_to_rtd3_uhsii(reg); if (rtsx_check_mmc_support(reg)) pcr->extra_caps |= EXTRA_CAPS_NO_MMC; pcr->sd30_drive_sel_3v3 = rtsx_reg_to_sd30_drive_sel_3v3(reg); @@ -278,13 +279,25 @@ static int rts5249_extra_init_hw(struct rtsx_pcr *pcr) rtsx_pci_send_cmd(pcr, CMD_TIMEOUT_DEF); - if (CHK_PCI_PID(pcr, PID_524A) || CHK_PCI_PID(pcr, PID_525A)) { + if (CHK_PCI_PID(pcr, PID_524A) || CHK_PCI_PID(pcr, PID_525A)) rtsx_pci_write_register(pcr, REG_VREF, PWD_SUSPND_EN, PWD_SUSPND_EN); - rtsx_pci_write_register(pcr, RTS524A_PM_CTRL3, 0x01, 0x00); - rtsx_pci_write_register(pcr, RTS524A_PME_FORCE_CTL, 0x30, 0x20); + + if (pcr->rtd3_en) { + if (CHK_PCI_PID(pcr, PID_524A) || CHK_PCI_PID(pcr, PID_525A)) { + rtsx_pci_write_register(pcr, RTS524A_PM_CTRL3, 0x01, 0x01); + rtsx_pci_write_register(pcr, RTS524A_PME_FORCE_CTL, 0x30, 0x30); + } else { + rtsx_pci_write_register(pcr, PM_CTRL3, 0x01, 0x01); + rtsx_pci_write_register(pcr, PME_FORCE_CTL, 0xFF, 0x33); + } } else { - rtsx_pci_write_register(pcr, PME_FORCE_CTL, 0xFF, 0x30); - rtsx_pci_write_register(pcr, PM_CTRL3, 0x01, 0x00); + if (CHK_PCI_PID(pcr, PID_524A) || CHK_PCI_PID(pcr, PID_525A)) { + rtsx_pci_write_register(pcr, RTS524A_PM_CTRL3, 0x01, 0x00); + rtsx_pci_write_register(pcr, RTS524A_PME_FORCE_CTL, 0x30, 0x20); + } else { + rtsx_pci_write_register(pcr, PME_FORCE_CTL, 0xFF, 0x30); + rtsx_pci_write_register(pcr, PM_CTRL3, 0x01, 0x00); + } } /* diff --git a/drivers/misc/cardreader/rtsx_pcr.c b/drivers/misc/cardreader/rtsx_pcr.c index 5d15607027e9..cb105563bde7 100644 --- a/drivers/misc/cardreader/rtsx_pcr.c +++ b/drivers/misc/cardreader/rtsx_pcr.c @@ -20,6 +20,8 @@ #include <linux/rtsx_pci.h> #include <linux/mmc/card.h> #include <asm/unaligned.h> +#include <linux/pm.h> +#include <linux/pm_runtime.h> #include "rtsx_pcr.h" #include "rts5261.h" @@ -89,9 +91,16 @@ static void rtsx_comm_set_aspm(struct rtsx_pcr *pcr, bool enable) if (pcr->aspm_enabled == enable) return; - pcie_capability_clear_and_set_word(pcr->pci, PCI_EXP_LNKCTL, - PCI_EXP_LNKCTL_ASPMC, - enable ? pcr->aspm_en : 0); + if (pcr->aspm_en & 0x02) + rtsx_pci_write_register(pcr, ASPM_FORCE_CTL, FORCE_ASPM_CTL0 | + FORCE_ASPM_CTL1, enable ? 0 : FORCE_ASPM_CTL0 | FORCE_ASPM_CTL1); + else + rtsx_pci_write_register(pcr, ASPM_FORCE_CTL, FORCE_ASPM_CTL0 | + FORCE_ASPM_CTL1, FORCE_ASPM_CTL0 | FORCE_ASPM_CTL1); + + if (!enable && (pcr->aspm_en & 0x02)) + mdelay(10); + pcr->aspm_enabled = enable; } @@ -143,6 +152,9 @@ void rtsx_pci_start_run(struct rtsx_pcr *pcr) /* If pci device removed, don't queue idle work any more */ if (pcr->remove_pci) return; + if (pcr->rtd3_en) + if (pcr->pci->dev.power.usage_count.counter == 0) + pm_runtime_get(&(pcr->pci->dev)); if (pcr->state != PDEV_STAT_RUN) { pcr->state = PDEV_STAT_RUN; @@ -1075,6 +1087,19 @@ static void rtsx_pm_power_saving(struct rtsx_pcr *pcr) rtsx_comm_pm_power_saving(pcr); } +static void rtsx_pci_rtd3_work(struct work_struct *work) +{ + struct delayed_work *dwork = to_delayed_work(work); + struct rtsx_pcr *pcr = container_of(dwork, struct rtsx_pcr, rtd3_work); + + pcr_dbg(pcr, "--> %s\n", __func__); + + while (pcr->pci->dev.power.usage_count.counter > 0) { + if (pm_runtime_active(&(pcr->pci->dev))) + pm_runtime_put(&(pcr->pci->dev)); + } +} + static void rtsx_pci_idle_work(struct work_struct *work) { struct delayed_work *dwork = to_delayed_work(work); @@ -1094,6 +1119,9 @@ static void rtsx_pci_idle_work(struct work_struct *work) rtsx_pm_power_saving(pcr); mutex_unlock(&pcr->pcr_mutex); + + if (pcr->rtd3_en) + mod_delayed_work(system_wq, &pcr->rtd3_work, msecs_to_jiffies(10000)); } static void rtsx_base_force_power_down(struct rtsx_pcr *pcr, u8 pm_state) @@ -1283,7 +1311,7 @@ static int rtsx_pci_init_hw(struct rtsx_pcr *pcr) /* Wait SSC power stable */ udelay(200); - rtsx_pci_disable_aspm(pcr); + rtsx_disable_aspm(pcr); if (pcr->ops->optimize_phy) { err = pcr->ops->optimize_phy(pcr); if (err < 0) @@ -1357,8 +1385,7 @@ static int rtsx_pci_init_hw(struct rtsx_pcr *pcr) rtsx_pci_init_ocp(pcr); /* Enable clk_request_n to enable clock power management */ - pcie_capability_write_word(pdev, PCI_EXP_LNKCTL, - PCI_EXP_LNKCTL_CLKREQ_EN); + pci_write_config_byte(pdev, PCI_EXP_LNKCTL+1, 0x01); /* Enter L1 when host tx idle */ pci_write_config_byte(pdev, 0x70F, 0x5B); @@ -1368,6 +1395,8 @@ static int rtsx_pci_init_hw(struct rtsx_pcr *pcr) return err; } + rtsx_pci_write_register(pcr, ASPM_FORCE_CTL, 0x30, 0x30); + /* No CD interrupt if probing driver with card inserted. * So we need to initialize pcr->card_exist here. */ @@ -1571,6 +1600,12 @@ static int rtsx_pci_probe(struct pci_dev *pcidev, rtsx_pcr_cells[i].platform_data = handle; rtsx_pcr_cells[i].pdata_size = sizeof(*handle); } + + if (pcr->rtd3_en) { + INIT_DELAYED_WORK(&pcr->rtd3_work, rtsx_pci_rtd3_work); + pm_runtime_enable(&pcidev->dev); + } + ret = mfd_add_devices(&pcidev->dev, pcr->id, rtsx_pcr_cells, ARRAY_SIZE(rtsx_pcr_cells), NULL, 0, NULL); if (ret < 0) @@ -1610,6 +1645,9 @@ static void rtsx_pci_remove(struct pci_dev *pcidev) pcr->remove_pci = true; + if (pcr->rtd3_en) + pm_runtime_get_noresume(&pcr->pci->dev); + /* Disable interrupts at the pcr level */ spin_lock_irq(&pcr->lock); rtsx_pci_writel(pcr, RTSX_BIER, 0); @@ -1619,6 +1657,9 @@ static void rtsx_pci_remove(struct pci_dev *pcidev) cancel_delayed_work_sync(&pcr->carddet_work); cancel_delayed_work_sync(&pcr->idle_work); + if (pcr->rtd3_en) + cancel_delayed_work_sync(&pcr->rtd3_work); + mfd_remove_devices(&pcidev->dev); dma_free_coherent(&(pcr->pci->dev), RTSX_RESV_BUF_LEN, @@ -1635,6 +1676,11 @@ static void rtsx_pci_remove(struct pci_dev *pcidev) idr_remove(&rtsx_pci_idr, pcr->id); spin_unlock(&rtsx_pci_lock); + if (pcr->rtd3_en) { + pm_runtime_disable(&pcr->pci->dev); + pm_runtime_put_noidle(&pcr->pci->dev); + } + kfree(pcr->slots); kfree(pcr); kfree(handle); @@ -1716,13 +1762,75 @@ static void rtsx_pci_shutdown(struct pci_dev *pcidev) pci_disable_msi(pcr->pci); } +static int rtsx_pci_runtime_suspend(struct device *device) +{ + struct pci_dev *pcidev = to_pci_dev(device); + struct pcr_handle *handle; + struct rtsx_pcr *pcr; + + handle = pci_get_drvdata(pcidev); + pcr = handle->pcr; + dev_dbg(&(pcidev->dev), "--> %s\n", __func__); + + cancel_delayed_work(&pcr->carddet_work); + cancel_delayed_work(&pcr->rtd3_work); + cancel_delayed_work(&pcr->idle_work); + + mutex_lock(&pcr->pcr_mutex); + rtsx_pci_power_off(pcr, HOST_ENTER_S3); + + free_irq(pcr->irq, (void *)pcr); + + mutex_unlock(&pcr->pcr_mutex); + + return 0; +} + +static int rtsx_pci_runtime_resume(struct device *device) +{ + struct pci_dev *pcidev = to_pci_dev(device); + struct pcr_handle *handle; + struct rtsx_pcr *pcr; + int ret = 0; + + handle = pci_get_drvdata(pcidev); + pcr = handle->pcr; + dev_dbg(&(pcidev->dev), "--> %s\n", __func__); + + mutex_lock(&pcr->pcr_mutex); + + rtsx_pci_write_register(pcr, HOST_SLEEP_STATE, 0x03, 0x00); + rtsx_pci_acquire_irq(pcr); + synchronize_irq(pcr->irq); + + if (pcr->ops->fetch_vendor_settings) + pcr->ops->fetch_vendor_settings(pcr); + + rtsx_pci_init_hw(pcr); + + if (pcr->slots[RTSX_SD_CARD].p_dev != NULL) { + pcr->slots[RTSX_SD_CARD].card_event( + pcr->slots[RTSX_SD_CARD].p_dev); + } + + schedule_delayed_work(&pcr->idle_work, msecs_to_jiffies(200)); + + mutex_unlock(&pcr->pcr_mutex); + return ret; +} + #else /* CONFIG_PM */ #define rtsx_pci_shutdown NULL +#define rtsx_pci_runtime_suspend NULL +#define rtsx_pic_runtime_resume NULL #endif /* CONFIG_PM */ -static SIMPLE_DEV_PM_OPS(rtsx_pci_pm_ops, rtsx_pci_suspend, rtsx_pci_resume); +static const struct dev_pm_ops rtsx_pci_pm_ops = { + SET_SYSTEM_SLEEP_PM_OPS(rtsx_pci_suspend, rtsx_pci_resume) + SET_RUNTIME_PM_OPS(rtsx_pci_runtime_suspend, rtsx_pci_runtime_resume, NULL) +}; static struct pci_driver rtsx_pci_driver = { .name = DRV_NAME_RTSX_PCI, diff --git a/drivers/misc/cardreader/rtsx_pcr.h b/drivers/misc/cardreader/rtsx_pcr.h index fe5f4ca0f937..daf057c4eea6 100644 --- a/drivers/misc/cardreader/rtsx_pcr.h +++ b/drivers/misc/cardreader/rtsx_pcr.h @@ -90,6 +90,7 @@ static inline u8 map_sd_drive(int idx) #define rtsx_check_mmc_support(reg) ((reg) & 0x10) #define rtsx_reg_to_rtd3(reg) ((reg) & 0x02) +#define rtsx_reg_to_rtd3_uhsii(reg) ((reg) & 0x04) #define rtsx_reg_to_aspm(reg) (((reg) >> 28) & 0x03) #define rtsx_reg_to_sd30_drive_sel_1v8(reg) (((reg) >> 26) & 0x03) #define rtsx_reg_to_sd30_drive_sel_3v3(reg) (((reg) >> 5) & 0x03) diff --git a/drivers/mmc/host/rtsx_pci_sdmmc.c b/drivers/mmc/host/rtsx_pci_sdmmc.c index eb395e144207..02f31f4d0944 100644 --- a/drivers/mmc/host/rtsx_pci_sdmmc.c +++ b/drivers/mmc/host/rtsx_pci_sdmmc.c @@ -20,6 +20,7 @@ #include <linux/mmc/card.h> #include <linux/rtsx_pci.h> #include <asm/unaligned.h> +#include <linux/pm_runtime.h> struct realtek_pci_sdmmc { struct platform_device *pdev; @@ -953,7 +954,6 @@ static int sd_set_power_mode(struct realtek_pci_sdmmc *host, unsigned char power_mode) { int err; - if (power_mode == MMC_POWER_OFF) err = sd_power_off(host); else @@ -1249,7 +1249,7 @@ static int sdmmc_switch_voltage(struct mmc_host *mmc, struct mmc_ios *ios) out: /* Stop toggle SD clock in idle */ - err = rtsx_pci_write_register(pcr, SD_BUS_STAT, + rtsx_pci_write_register(pcr, SD_BUS_STAT, SD_CLK_TOGGLE_EN | SD_CLK_FORCE_STOP, 0); mutex_unlock(&pcr->pcr_mutex); @@ -1343,6 +1343,7 @@ static void init_extra_caps(struct realtek_pci_sdmmc *host) static void realtek_init_host(struct realtek_pci_sdmmc *host) { struct mmc_host *mmc = host->mmc; + struct rtsx_pcr *pcr = host->pcr; mmc->f_min = 250000; mmc->f_max = 208000000; @@ -1350,6 +1351,8 @@ static void realtek_init_host(struct realtek_pci_sdmmc *host) mmc->caps = MMC_CAP_4_BIT_DATA | MMC_CAP_SD_HIGHSPEED | MMC_CAP_MMC_HIGHSPEED | MMC_CAP_BUS_WIDTH_TEST | MMC_CAP_UHS_SDR12 | MMC_CAP_UHS_SDR25; + if (pcr->rtd3_en) + mmc->caps = mmc->caps | MMC_CAP_AGGRESSIVE_PM; mmc->caps2 = MMC_CAP2_NO_PRESCAN_POWERUP | MMC_CAP2_FULL_PWR_CYCLE; mmc->max_current_330 = 400; mmc->max_current_180 = 800; @@ -1407,6 +1410,12 @@ static int rtsx_pci_sdmmc_drv_probe(struct platform_device *pdev) realtek_init_host(host); + if (pcr->rtd3_en) { + pm_runtime_use_autosuspend(&pdev->dev); + pm_runtime_set_autosuspend_delay(&pdev->dev, 5000); + pm_runtime_enable(&pdev->dev); + } + mmc_add_host(mmc); return 0; @@ -1426,6 +1435,11 @@ static int rtsx_pci_sdmmc_drv_remove(struct platform_device *pdev) pcr->slots[RTSX_SD_CARD].card_event = NULL; mmc = host->mmc; + if (pcr->rtd3_en) { + pm_runtime_dont_use_autosuspend(&pdev->dev); + pm_runtime_disable(&pdev->dev); + } + cancel_work_sync(&host->work); mutex_lock(&host->host_mutex); diff --git a/include/linux/rtsx_pci.h b/include/linux/rtsx_pci.h index 745f5e73f99a..689d955c2246 100644 --- a/include/linux/rtsx_pci.h +++ b/include/linux/rtsx_pci.h @@ -1174,6 +1174,7 @@ struct rtsx_pcr { struct delayed_work carddet_work; struct delayed_work idle_work; + struct delayed_work rtd3_work; spinlock_t lock; struct mutex pcr_mutex; -- 2.17.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] misc: rtsx: rts5249 support runtime PM 2020-11-24 6:02 [PATCH] misc: rtsx: rts5249 support runtime PM ricky_wu @ 2020-11-24 8:40 ` kernel test robot 2020-11-24 8:55 ` kernel test robot 2020-11-24 20:49 ` Bjorn Helgaas 2 siblings, 0 replies; 8+ messages in thread From: kernel test robot @ 2020-11-24 8:40 UTC (permalink / raw) To: kbuild-all [-- Attachment #1: Type: text/plain, Size: 2998 bytes --] Hi, Thank you for the patch! Yet something to improve: [auto build test ERROR on char-misc/char-misc-testing] [also build test ERROR on soc/for-next linux/master linus/master v5.10-rc5 next-20201123] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/ricky_wu-realtek-com/misc-rtsx-rts5249-support-runtime-PM/20201124-140617 base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git 93c69b2d17372463ae33b79b3002c22a208945b3 config: s390-randconfig-r004-20201124 (attached as .config) compiler: s390-linux-gcc (GCC) 9.3.0 reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/0day-ci/linux/commit/2e168305df88dec50654147a4a7a29b390629ec8 git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review ricky_wu-realtek-com/misc-rtsx-rts5249-support-runtime-PM/20201124-140617 git checkout 2e168305df88dec50654147a4a7a29b390629ec8 # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=s390 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> All errors (new ones prefixed by >>): drivers/misc/cardreader/rtsx_pcr.c: In function 'rtsx_pci_start_run': >> drivers/misc/cardreader/rtsx_pcr.c:156:26: error: 'struct dev_pm_info' has no member named 'usage_count' 156 | if (pcr->pci->dev.power.usage_count.counter == 0) | ^ drivers/misc/cardreader/rtsx_pcr.c: In function 'rtsx_pci_rtd3_work': drivers/misc/cardreader/rtsx_pcr.c:1097:28: error: 'struct dev_pm_info' has no member named 'usage_count' 1097 | while (pcr->pci->dev.power.usage_count.counter > 0) { | ^ vim +156 drivers/misc/cardreader/rtsx_pcr.c 149 150 void rtsx_pci_start_run(struct rtsx_pcr *pcr) 151 { 152 /* If pci device removed, don't queue idle work any more */ 153 if (pcr->remove_pci) 154 return; 155 if (pcr->rtd3_en) > 156 if (pcr->pci->dev.power.usage_count.counter == 0) 157 pm_runtime_get(&(pcr->pci->dev)); 158 159 if (pcr->state != PDEV_STAT_RUN) { 160 pcr->state = PDEV_STAT_RUN; 161 if (pcr->ops->enable_auto_blink) 162 pcr->ops->enable_auto_blink(pcr); 163 rtsx_pm_full_on(pcr); 164 } 165 166 mod_delayed_work(system_wq, &pcr->idle_work, msecs_to_jiffies(200)); 167 } 168 EXPORT_SYMBOL_GPL(rtsx_pci_start_run); 169 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org [-- Attachment #2: config.gz --] [-- Type: application/gzip, Size: 27354 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] misc: rtsx: rts5249 support runtime PM 2020-11-24 6:02 [PATCH] misc: rtsx: rts5249 support runtime PM ricky_wu 2020-11-24 8:40 ` kernel test robot @ 2020-11-24 8:55 ` kernel test robot 2020-11-24 20:49 ` Bjorn Helgaas 2 siblings, 0 replies; 8+ messages in thread From: kernel test robot @ 2020-11-24 8:55 UTC (permalink / raw) To: kbuild-all [-- Attachment #1: Type: text/plain, Size: 10049 bytes --] Hi, Thank you for the patch! Yet something to improve: [auto build test ERROR on char-misc/char-misc-testing] [also build test ERROR on soc/for-next linux/master linus/master v5.10-rc5 next-20201123] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/ricky_wu-realtek-com/misc-rtsx-rts5249-support-runtime-PM/20201124-140617 base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git 93c69b2d17372463ae33b79b3002c22a208945b3 config: riscv-randconfig-r016-20201124 (attached as .config) compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project df9ae5992889560a8f3c6760b54d5051b47c7bf5) reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # install riscv cross compiling tool for clang build # apt-get install binutils-riscv64-linux-gnu # https://github.com/0day-ci/linux/commit/2e168305df88dec50654147a4a7a29b390629ec8 git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review ricky_wu-realtek-com/misc-rtsx-rts5249-support-runtime-PM/20201124-140617 git checkout 2e168305df88dec50654147a4a7a29b390629ec8 # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=riscv If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> All errors (new ones prefixed by >>): In file included from arch/riscv/include/asm/io.h:149: include/asm-generic/io.h:564:9: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] return inw(addr); ^~~~~~~~~ arch/riscv/include/asm/io.h:56:76: note: expanded from macro 'inw' #define inw(c) ({ u16 __v; __io_pbr(); __v = readw_cpu((void*)(PCI_IOBASE + (c))); __io_par(__v); __v; }) ~~~~~~~~~~ ^ arch/riscv/include/asm/mmio.h:88:76: note: expanded from macro 'readw_cpu' #define readw_cpu(c) ({ u16 __r = le16_to_cpu((__force __le16)__raw_readw(c)); __r; }) ^ include/uapi/linux/byteorder/little_endian.h:36:51: note: expanded from macro '__le16_to_cpu' #define __le16_to_cpu(x) ((__force __u16)(__le16)(x)) ^ In file included from drivers/misc/cardreader/rtsx_pcr.c:10: In file included from include/linux/pci.h:38: In file included from include/linux/interrupt.h:11: In file included from include/linux/hardirq.h:10: In file included from ./arch/riscv/include/generated/asm/hardirq.h:1: In file included from include/asm-generic/hardirq.h:13: In file included from include/linux/irq.h:20: In file included from include/linux/io.h:13: In file included from arch/riscv/include/asm/io.h:149: include/asm-generic/io.h:572:9: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] return inl(addr); ^~~~~~~~~ arch/riscv/include/asm/io.h:57:76: note: expanded from macro 'inl' #define inl(c) ({ u32 __v; __io_pbr(); __v = readl_cpu((void*)(PCI_IOBASE + (c))); __io_par(__v); __v; }) ~~~~~~~~~~ ^ arch/riscv/include/asm/mmio.h:89:76: note: expanded from macro 'readl_cpu' #define readl_cpu(c) ({ u32 __r = le32_to_cpu((__force __le32)__raw_readl(c)); __r; }) ^ include/uapi/linux/byteorder/little_endian.h:34:51: note: expanded from macro '__le32_to_cpu' #define __le32_to_cpu(x) ((__force __u32)(__le32)(x)) ^ In file included from drivers/misc/cardreader/rtsx_pcr.c:10: In file included from include/linux/pci.h:38: In file included from include/linux/interrupt.h:11: In file included from include/linux/hardirq.h:10: In file included from ./arch/riscv/include/generated/asm/hardirq.h:1: In file included from include/asm-generic/hardirq.h:13: In file included from include/linux/irq.h:20: In file included from include/linux/io.h:13: In file included from arch/riscv/include/asm/io.h:149: include/asm-generic/io.h:580:2: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] outb(value, addr); ^~~~~~~~~~~~~~~~~ arch/riscv/include/asm/io.h:59:68: note: expanded from macro 'outb' #define outb(v,c) ({ __io_pbw(); writeb_cpu((v),(void*)(PCI_IOBASE + (c))); __io_paw(); }) ~~~~~~~~~~ ^ arch/riscv/include/asm/mmio.h:91:52: note: expanded from macro 'writeb_cpu' #define writeb_cpu(v, c) ((void)__raw_writeb((v), (c))) ^ In file included from drivers/misc/cardreader/rtsx_pcr.c:10: In file included from include/linux/pci.h:38: In file included from include/linux/interrupt.h:11: In file included from include/linux/hardirq.h:10: In file included from ./arch/riscv/include/generated/asm/hardirq.h:1: In file included from include/asm-generic/hardirq.h:13: In file included from include/linux/irq.h:20: In file included from include/linux/io.h:13: In file included from arch/riscv/include/asm/io.h:149: include/asm-generic/io.h:588:2: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] outw(value, addr); ^~~~~~~~~~~~~~~~~ arch/riscv/include/asm/io.h:60:68: note: expanded from macro 'outw' #define outw(v,c) ({ __io_pbw(); writew_cpu((v),(void*)(PCI_IOBASE + (c))); __io_paw(); }) ~~~~~~~~~~ ^ arch/riscv/include/asm/mmio.h:92:76: note: expanded from macro 'writew_cpu' #define writew_cpu(v, c) ((void)__raw_writew((__force u16)cpu_to_le16(v), (c))) ^ In file included from drivers/misc/cardreader/rtsx_pcr.c:10: In file included from include/linux/pci.h:38: In file included from include/linux/interrupt.h:11: In file included from include/linux/hardirq.h:10: In file included from ./arch/riscv/include/generated/asm/hardirq.h:1: In file included from include/asm-generic/hardirq.h:13: In file included from include/linux/irq.h:20: In file included from include/linux/io.h:13: In file included from arch/riscv/include/asm/io.h:149: include/asm-generic/io.h:596:2: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] outl(value, addr); ^~~~~~~~~~~~~~~~~ arch/riscv/include/asm/io.h:61:68: note: expanded from macro 'outl' #define outl(v,c) ({ __io_pbw(); writel_cpu((v),(void*)(PCI_IOBASE + (c))); __io_paw(); }) ~~~~~~~~~~ ^ arch/riscv/include/asm/mmio.h:93:76: note: expanded from macro 'writel_cpu' #define writel_cpu(v, c) ((void)__raw_writel((__force u32)cpu_to_le32(v), (c))) ^ In file included from drivers/misc/cardreader/rtsx_pcr.c:10: In file included from include/linux/pci.h:38: In file included from include/linux/interrupt.h:11: In file included from include/linux/hardirq.h:10: In file included from ./arch/riscv/include/generated/asm/hardirq.h:1: In file included from include/asm-generic/hardirq.h:13: In file included from include/linux/irq.h:20: In file included from include/linux/io.h:13: In file included from arch/riscv/include/asm/io.h:149: include/asm-generic/io.h:1005:55: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic] return (port > MMIO_UPPER_LIMIT) ? NULL : PCI_IOBASE + port; ~~~~~~~~~~ ^ >> drivers/misc/cardreader/rtsx_pcr.c:156:27: error: no member named 'usage_count' in 'struct dev_pm_info' if (pcr->pci->dev.power.usage_count.counter == 0) ~~~~~~~~~~~~~~~~~~~ ^ drivers/misc/cardreader/rtsx_pcr.c:1097:29: error: no member named 'usage_count' in 'struct dev_pm_info' while (pcr->pci->dev.power.usage_count.counter > 0) { ~~~~~~~~~~~~~~~~~~~ ^ 7 warnings and 2 errors generated. vim +156 drivers/misc/cardreader/rtsx_pcr.c 149 150 void rtsx_pci_start_run(struct rtsx_pcr *pcr) 151 { 152 /* If pci device removed, don't queue idle work any more */ 153 if (pcr->remove_pci) 154 return; 155 if (pcr->rtd3_en) > 156 if (pcr->pci->dev.power.usage_count.counter == 0) 157 pm_runtime_get(&(pcr->pci->dev)); 158 159 if (pcr->state != PDEV_STAT_RUN) { 160 pcr->state = PDEV_STAT_RUN; 161 if (pcr->ops->enable_auto_blink) 162 pcr->ops->enable_auto_blink(pcr); 163 rtsx_pm_full_on(pcr); 164 } 165 166 mod_delayed_work(system_wq, &pcr->idle_work, msecs_to_jiffies(200)); 167 } 168 EXPORT_SYMBOL_GPL(rtsx_pci_start_run); 169 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org [-- Attachment #2: config.gz --] [-- Type: application/gzip, Size: 27129 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] misc: rtsx: rts5249 support runtime PM 2020-11-24 6:02 [PATCH] misc: rtsx: rts5249 support runtime PM ricky_wu 2020-11-24 8:40 ` kernel test robot 2020-11-24 8:55 ` kernel test robot @ 2020-11-24 20:49 ` Bjorn Helgaas 2020-11-25 14:04 ` Rafael J. Wysocki 2 siblings, 1 reply; 8+ messages in thread From: Bjorn Helgaas @ 2020-11-24 20:49 UTC (permalink / raw) To: ricky_wu Cc: arnd, gregkh, ulf.hansson, bhelgaas, vaibhavgupta40, kdlnx, dianders, rmfrfs, lee.jones, linux-kernel, linux-mmc, Rafael J. Wysocki, linux-pm [+cc Rafael, linux-pm for runtime PM question below] On Tue, Nov 24, 2020 at 02:02:02PM +0800, ricky_wu@realtek.com wrote: > From: Ricky Wu <ricky_wu@realtek.com> > > rtsx_pci_sdmmc: add to support autosuspend when the rtd3_en is set > > rtsx_pcr: add callback functions about runtime PM > add delay_work(rtd3_work) to decrease usage count to 0 when staying > at idle over 10 sec > > rts5249: add extra flow at init function to support wakeup from d3 > and set rtd3_en from register setting This looks like several patches that should be split up. The ASPM changes should be unrelated to runtime PM. > Signed-off-by: Ricky Wu <ricky_wu@realtek.com> > --- > drivers/misc/cardreader/rts5249.c | 25 ++++-- > drivers/misc/cardreader/rtsx_pcr.c | 122 +++++++++++++++++++++++++++-- > drivers/misc/cardreader/rtsx_pcr.h | 1 + > drivers/mmc/host/rtsx_pci_sdmmc.c | 18 ++++- > include/linux/rtsx_pci.h | 1 + > 5 files changed, 152 insertions(+), 15 deletions(-) > > diff --git a/drivers/misc/cardreader/rts5249.c b/drivers/misc/cardreader/rts5249.c > index b85279f1fc5e..1da3b1ca1121 100644 > --- a/drivers/misc/cardreader/rts5249.c > +++ b/drivers/misc/cardreader/rts5249.c > @@ -65,7 +65,6 @@ static void rtsx_base_fetch_vendor_settings(struct rtsx_pcr *pcr) > pcr_dbg(pcr, "skip fetch vendor setting\n"); > return; > } > - Doesn't look like an improvement to me. > pcr->aspm_en = rtsx_reg_to_aspm(reg); > pcr->sd30_drive_sel_1v8 = rtsx_reg_to_sd30_drive_sel_1v8(reg); > pcr->card_drive_sel &= 0x3F; > @@ -73,6 +72,8 @@ static void rtsx_base_fetch_vendor_settings(struct rtsx_pcr *pcr) > > pci_read_config_dword(pdev, PCR_SETTING_REG2, ®); > pcr_dbg(pcr, "Cfg 0x%x: 0x%x\n", PCR_SETTING_REG2, reg); > + > + pcr->rtd3_en = rtsx_reg_to_rtd3_uhsii(reg); > if (rtsx_check_mmc_support(reg)) > pcr->extra_caps |= EXTRA_CAPS_NO_MMC; > pcr->sd30_drive_sel_3v3 = rtsx_reg_to_sd30_drive_sel_3v3(reg); > @@ -278,13 +279,25 @@ static int rts5249_extra_init_hw(struct rtsx_pcr *pcr) > > rtsx_pci_send_cmd(pcr, CMD_TIMEOUT_DEF); > > - if (CHK_PCI_PID(pcr, PID_524A) || CHK_PCI_PID(pcr, PID_525A)) { > + if (CHK_PCI_PID(pcr, PID_524A) || CHK_PCI_PID(pcr, PID_525A)) > rtsx_pci_write_register(pcr, REG_VREF, PWD_SUSPND_EN, PWD_SUSPND_EN); > - rtsx_pci_write_register(pcr, RTS524A_PM_CTRL3, 0x01, 0x00); > - rtsx_pci_write_register(pcr, RTS524A_PME_FORCE_CTL, 0x30, 0x20); > + > + if (pcr->rtd3_en) { > + if (CHK_PCI_PID(pcr, PID_524A) || CHK_PCI_PID(pcr, PID_525A)) { > + rtsx_pci_write_register(pcr, RTS524A_PM_CTRL3, 0x01, 0x01); > + rtsx_pci_write_register(pcr, RTS524A_PME_FORCE_CTL, 0x30, 0x30); > + } else { > + rtsx_pci_write_register(pcr, PM_CTRL3, 0x01, 0x01); > + rtsx_pci_write_register(pcr, PME_FORCE_CTL, 0xFF, 0x33); > + } > } else { > - rtsx_pci_write_register(pcr, PME_FORCE_CTL, 0xFF, 0x30); > - rtsx_pci_write_register(pcr, PM_CTRL3, 0x01, 0x00); > + if (CHK_PCI_PID(pcr, PID_524A) || CHK_PCI_PID(pcr, PID_525A)) { > + rtsx_pci_write_register(pcr, RTS524A_PM_CTRL3, 0x01, 0x00); > + rtsx_pci_write_register(pcr, RTS524A_PME_FORCE_CTL, 0x30, 0x20); > + } else { > + rtsx_pci_write_register(pcr, PME_FORCE_CTL, 0xFF, 0x30); > + rtsx_pci_write_register(pcr, PM_CTRL3, 0x01, 0x00); > + } > } > > /* > diff --git a/drivers/misc/cardreader/rtsx_pcr.c b/drivers/misc/cardreader/rtsx_pcr.c > index 5d15607027e9..cb105563bde7 100644 > --- a/drivers/misc/cardreader/rtsx_pcr.c > +++ b/drivers/misc/cardreader/rtsx_pcr.c > @@ -20,6 +20,8 @@ > #include <linux/rtsx_pci.h> > #include <linux/mmc/card.h> > #include <asm/unaligned.h> > +#include <linux/pm.h> > +#include <linux/pm_runtime.h> > > #include "rtsx_pcr.h" > #include "rts5261.h" > @@ -89,9 +91,16 @@ static void rtsx_comm_set_aspm(struct rtsx_pcr *pcr, bool enable) > if (pcr->aspm_enabled == enable) > return; > > - pcie_capability_clear_and_set_word(pcr->pci, PCI_EXP_LNKCTL, > - PCI_EXP_LNKCTL_ASPMC, > - enable ? pcr->aspm_en : 0); > + if (pcr->aspm_en & 0x02) > + rtsx_pci_write_register(pcr, ASPM_FORCE_CTL, FORCE_ASPM_CTL0 | > + FORCE_ASPM_CTL1, enable ? 0 : FORCE_ASPM_CTL0 | FORCE_ASPM_CTL1); > + else > + rtsx_pci_write_register(pcr, ASPM_FORCE_CTL, FORCE_ASPM_CTL0 | > + FORCE_ASPM_CTL1, FORCE_ASPM_CTL0 | FORCE_ASPM_CTL1); > + > + if (!enable && (pcr->aspm_en & 0x02)) > + mdelay(10); This is a significant change that should be in its own patch and explained. Previously we did standard PCI config reads/writes to the PCIe Link Control register. After this change we'll use rtsx_pci_write_register(), which looks like it writes to an MMIO register in a BAR: rtsx_pci_probe pcr->remap_addr = ioremap(base, len) rtsx_pci_write_register rtsx_pci_writel(pcr, RTSX_HAIMR, val) iowrite32(val, pcr->remap_addr + reg) It's not clear that the MMIO register in the BAR is the same as the one in config space. And we still write the Link Control register in config space below and other places. How are these supposed to be coordinated? Drivers should not change ASPM configuration directly. Especially not in obfuscated ways like this. > pcr->aspm_enabled = enable; > } > @@ -143,6 +152,9 @@ void rtsx_pci_start_run(struct rtsx_pcr *pcr) > /* If pci device removed, don't queue idle work any more */ > if (pcr->remove_pci) > return; > + if (pcr->rtd3_en) > + if (pcr->pci->dev.power.usage_count.counter == 0) > + pm_runtime_get(&(pcr->pci->dev)); > > if (pcr->state != PDEV_STAT_RUN) { > pcr->state = PDEV_STAT_RUN; > @@ -1075,6 +1087,19 @@ static void rtsx_pm_power_saving(struct rtsx_pcr *pcr) > rtsx_comm_pm_power_saving(pcr); > } > > +static void rtsx_pci_rtd3_work(struct work_struct *work) > +{ > + struct delayed_work *dwork = to_delayed_work(work); > + struct rtsx_pcr *pcr = container_of(dwork, struct rtsx_pcr, rtd3_work); > + > + pcr_dbg(pcr, "--> %s\n", __func__); > + > + while (pcr->pci->dev.power.usage_count.counter > 0) { > + if (pm_runtime_active(&(pcr->pci->dev))) > + pm_runtime_put(&(pcr->pci->dev)); I'm not a runtime PM expert, but this looks fishy. AFAICT this is the only driver in the tree that uses usage_count.counter this way, which is a pretty big hint that this needs a closer look. Cc'd Rafael. > + } > +} > + > static void rtsx_pci_idle_work(struct work_struct *work) > { > struct delayed_work *dwork = to_delayed_work(work); > @@ -1094,6 +1119,9 @@ static void rtsx_pci_idle_work(struct work_struct *work) > rtsx_pm_power_saving(pcr); > > mutex_unlock(&pcr->pcr_mutex); > + > + if (pcr->rtd3_en) > + mod_delayed_work(system_wq, &pcr->rtd3_work, msecs_to_jiffies(10000)); > } > > static void rtsx_base_force_power_down(struct rtsx_pcr *pcr, u8 pm_state) > @@ -1283,7 +1311,7 @@ static int rtsx_pci_init_hw(struct rtsx_pcr *pcr) > /* Wait SSC power stable */ > udelay(200); > > - rtsx_pci_disable_aspm(pcr); > + rtsx_disable_aspm(pcr); > if (pcr->ops->optimize_phy) { > err = pcr->ops->optimize_phy(pcr); > if (err < 0) > @@ -1357,8 +1385,7 @@ static int rtsx_pci_init_hw(struct rtsx_pcr *pcr) > rtsx_pci_init_ocp(pcr); > > /* Enable clk_request_n to enable clock power management */ > - pcie_capability_write_word(pdev, PCI_EXP_LNKCTL, > - PCI_EXP_LNKCTL_CLKREQ_EN); > + pci_write_config_byte(pdev, PCI_EXP_LNKCTL+1, 0x01); Drivers should not write PCI_EXP_LNKCTL directly because that clobbers things done by the PCI core, e.g., ASPM and common clock configuration, Read Completion Boundary setting, etc. But if you must, you should at least use this: pcie_capability_clear_and_set_word(pdev, PCI_EXP_LNKCTL, 0, PCI_EXP_LNKCTL_CLKREQ_EN); > /* Enter L1 when host tx idle */ > pci_write_config_byte(pdev, 0x70F, 0x5B); > > @@ -1368,6 +1395,8 @@ static int rtsx_pci_init_hw(struct rtsx_pcr *pcr) > return err; > } > > + rtsx_pci_write_register(pcr, ASPM_FORCE_CTL, 0x30, 0x30); > + > /* No CD interrupt if probing driver with card inserted. > * So we need to initialize pcr->card_exist here. > */ > @@ -1571,6 +1600,12 @@ static int rtsx_pci_probe(struct pci_dev *pcidev, > rtsx_pcr_cells[i].platform_data = handle; > rtsx_pcr_cells[i].pdata_size = sizeof(*handle); > } > + > + if (pcr->rtd3_en) { > + INIT_DELAYED_WORK(&pcr->rtd3_work, rtsx_pci_rtd3_work); > + pm_runtime_enable(&pcidev->dev); > + } > + > ret = mfd_add_devices(&pcidev->dev, pcr->id, rtsx_pcr_cells, > ARRAY_SIZE(rtsx_pcr_cells), NULL, 0, NULL); > if (ret < 0) > @@ -1610,6 +1645,9 @@ static void rtsx_pci_remove(struct pci_dev *pcidev) > > pcr->remove_pci = true; > > + if (pcr->rtd3_en) > + pm_runtime_get_noresume(&pcr->pci->dev); > + > /* Disable interrupts at the pcr level */ > spin_lock_irq(&pcr->lock); > rtsx_pci_writel(pcr, RTSX_BIER, 0); > @@ -1619,6 +1657,9 @@ static void rtsx_pci_remove(struct pci_dev *pcidev) > cancel_delayed_work_sync(&pcr->carddet_work); > cancel_delayed_work_sync(&pcr->idle_work); > > + if (pcr->rtd3_en) > + cancel_delayed_work_sync(&pcr->rtd3_work); > + > mfd_remove_devices(&pcidev->dev); > > dma_free_coherent(&(pcr->pci->dev), RTSX_RESV_BUF_LEN, > @@ -1635,6 +1676,11 @@ static void rtsx_pci_remove(struct pci_dev *pcidev) > idr_remove(&rtsx_pci_idr, pcr->id); > spin_unlock(&rtsx_pci_lock); > > + if (pcr->rtd3_en) { > + pm_runtime_disable(&pcr->pci->dev); > + pm_runtime_put_noidle(&pcr->pci->dev); > + } > + > kfree(pcr->slots); > kfree(pcr); > kfree(handle); > @@ -1716,13 +1762,75 @@ static void rtsx_pci_shutdown(struct pci_dev *pcidev) > pci_disable_msi(pcr->pci); > } > > +static int rtsx_pci_runtime_suspend(struct device *device) > +{ > + struct pci_dev *pcidev = to_pci_dev(device); > + struct pcr_handle *handle; > + struct rtsx_pcr *pcr; > + > + handle = pci_get_drvdata(pcidev); > + pcr = handle->pcr; > + dev_dbg(&(pcidev->dev), "--> %s\n", __func__); > + > + cancel_delayed_work(&pcr->carddet_work); > + cancel_delayed_work(&pcr->rtd3_work); > + cancel_delayed_work(&pcr->idle_work); > + > + mutex_lock(&pcr->pcr_mutex); > + rtsx_pci_power_off(pcr, HOST_ENTER_S3); > + > + free_irq(pcr->irq, (void *)pcr); > + > + mutex_unlock(&pcr->pcr_mutex); > + > + return 0; > +} > + > +static int rtsx_pci_runtime_resume(struct device *device) > +{ > + struct pci_dev *pcidev = to_pci_dev(device); > + struct pcr_handle *handle; > + struct rtsx_pcr *pcr; > + int ret = 0; > + > + handle = pci_get_drvdata(pcidev); > + pcr = handle->pcr; > + dev_dbg(&(pcidev->dev), "--> %s\n", __func__); > + > + mutex_lock(&pcr->pcr_mutex); > + > + rtsx_pci_write_register(pcr, HOST_SLEEP_STATE, 0x03, 0x00); > + rtsx_pci_acquire_irq(pcr); > + synchronize_irq(pcr->irq); > + > + if (pcr->ops->fetch_vendor_settings) > + pcr->ops->fetch_vendor_settings(pcr); > + > + rtsx_pci_init_hw(pcr); > + > + if (pcr->slots[RTSX_SD_CARD].p_dev != NULL) { > + pcr->slots[RTSX_SD_CARD].card_event( > + pcr->slots[RTSX_SD_CARD].p_dev); > + } > + > + schedule_delayed_work(&pcr->idle_work, msecs_to_jiffies(200)); > + > + mutex_unlock(&pcr->pcr_mutex); > + return ret; > +} > + > #else /* CONFIG_PM */ > > #define rtsx_pci_shutdown NULL > +#define rtsx_pci_runtime_suspend NULL > +#define rtsx_pic_runtime_resume NULL > > #endif /* CONFIG_PM */ > > -static SIMPLE_DEV_PM_OPS(rtsx_pci_pm_ops, rtsx_pci_suspend, rtsx_pci_resume); > +static const struct dev_pm_ops rtsx_pci_pm_ops = { > + SET_SYSTEM_SLEEP_PM_OPS(rtsx_pci_suspend, rtsx_pci_resume) > + SET_RUNTIME_PM_OPS(rtsx_pci_runtime_suspend, rtsx_pci_runtime_resume, NULL) > +}; > > static struct pci_driver rtsx_pci_driver = { > .name = DRV_NAME_RTSX_PCI, > diff --git a/drivers/misc/cardreader/rtsx_pcr.h b/drivers/misc/cardreader/rtsx_pcr.h > index fe5f4ca0f937..daf057c4eea6 100644 > --- a/drivers/misc/cardreader/rtsx_pcr.h > +++ b/drivers/misc/cardreader/rtsx_pcr.h > @@ -90,6 +90,7 @@ static inline u8 map_sd_drive(int idx) > > #define rtsx_check_mmc_support(reg) ((reg) & 0x10) > #define rtsx_reg_to_rtd3(reg) ((reg) & 0x02) > +#define rtsx_reg_to_rtd3_uhsii(reg) ((reg) & 0x04) > #define rtsx_reg_to_aspm(reg) (((reg) >> 28) & 0x03) > #define rtsx_reg_to_sd30_drive_sel_1v8(reg) (((reg) >> 26) & 0x03) > #define rtsx_reg_to_sd30_drive_sel_3v3(reg) (((reg) >> 5) & 0x03) > diff --git a/drivers/mmc/host/rtsx_pci_sdmmc.c b/drivers/mmc/host/rtsx_pci_sdmmc.c > index eb395e144207..02f31f4d0944 100644 > --- a/drivers/mmc/host/rtsx_pci_sdmmc.c > +++ b/drivers/mmc/host/rtsx_pci_sdmmc.c > @@ -20,6 +20,7 @@ > #include <linux/mmc/card.h> > #include <linux/rtsx_pci.h> > #include <asm/unaligned.h> > +#include <linux/pm_runtime.h> > > struct realtek_pci_sdmmc { > struct platform_device *pdev; > @@ -953,7 +954,6 @@ static int sd_set_power_mode(struct realtek_pci_sdmmc *host, > unsigned char power_mode) > { > int err; > - Not an improvement. Looks like accidental whitespace change. But if you wanted to improve this function, you could (in a separate patch) make it look like this: static int sd_set_power_mode(...) { if (power_mode == MMC_POWER_OFF) return sd_power_off(host); else return sd_power_on(host); } > if (power_mode == MMC_POWER_OFF) > err = sd_power_off(host); > else > @@ -1249,7 +1249,7 @@ static int sdmmc_switch_voltage(struct mmc_host *mmc, struct mmc_ios *ios) > > out: > /* Stop toggle SD clock in idle */ > - err = rtsx_pci_write_register(pcr, SD_BUS_STAT, > + rtsx_pci_write_register(pcr, SD_BUS_STAT, > SD_CLK_TOGGLE_EN | SD_CLK_FORCE_STOP, 0); > > mutex_unlock(&pcr->pcr_mutex); > @@ -1343,6 +1343,7 @@ static void init_extra_caps(struct realtek_pci_sdmmc *host) > static void realtek_init_host(struct realtek_pci_sdmmc *host) > { > struct mmc_host *mmc = host->mmc; > + struct rtsx_pcr *pcr = host->pcr; > > mmc->f_min = 250000; > mmc->f_max = 208000000; > @@ -1350,6 +1351,8 @@ static void realtek_init_host(struct realtek_pci_sdmmc *host) > mmc->caps = MMC_CAP_4_BIT_DATA | MMC_CAP_SD_HIGHSPEED | > MMC_CAP_MMC_HIGHSPEED | MMC_CAP_BUS_WIDTH_TEST | > MMC_CAP_UHS_SDR12 | MMC_CAP_UHS_SDR25; > + if (pcr->rtd3_en) > + mmc->caps = mmc->caps | MMC_CAP_AGGRESSIVE_PM; > mmc->caps2 = MMC_CAP2_NO_PRESCAN_POWERUP | MMC_CAP2_FULL_PWR_CYCLE; > mmc->max_current_330 = 400; > mmc->max_current_180 = 800; > @@ -1407,6 +1410,12 @@ static int rtsx_pci_sdmmc_drv_probe(struct platform_device *pdev) > > realtek_init_host(host); > > + if (pcr->rtd3_en) { > + pm_runtime_use_autosuspend(&pdev->dev); > + pm_runtime_set_autosuspend_delay(&pdev->dev, 5000); I think you should swap the order of these so the delay is set first. Maybe it doesn't matter because both are before pm_runtime_enable(), but it's always good style to set parameters before enabling a feature. > + pm_runtime_enable(&pdev->dev); > + } > + > mmc_add_host(mmc); > > return 0; > @@ -1426,6 +1435,11 @@ static int rtsx_pci_sdmmc_drv_remove(struct platform_device *pdev) > pcr->slots[RTSX_SD_CARD].card_event = NULL; > mmc = host->mmc; > > + if (pcr->rtd3_en) { > + pm_runtime_dont_use_autosuspend(&pdev->dev); > + pm_runtime_disable(&pdev->dev); > + } > + > cancel_work_sync(&host->work); > > mutex_lock(&host->host_mutex); > diff --git a/include/linux/rtsx_pci.h b/include/linux/rtsx_pci.h > index 745f5e73f99a..689d955c2246 100644 > --- a/include/linux/rtsx_pci.h > +++ b/include/linux/rtsx_pci.h > @@ -1174,6 +1174,7 @@ struct rtsx_pcr { > > struct delayed_work carddet_work; > struct delayed_work idle_work; > + struct delayed_work rtd3_work; > > spinlock_t lock; > struct mutex pcr_mutex; > -- > 2.17.1 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] misc: rtsx: rts5249 support runtime PM 2020-11-24 20:49 ` Bjorn Helgaas @ 2020-11-25 14:04 ` Rafael J. Wysocki 2020-11-26 3:07 ` 吳昊澄 Ricky 0 siblings, 1 reply; 8+ messages in thread From: Rafael J. Wysocki @ 2020-11-25 14:04 UTC (permalink / raw) To: Bjorn Helgaas, ricky_wu Cc: Arnd Bergmann, Greg Kroah-Hartman, Ulf Hansson, Bjorn Helgaas, vaibhavgupta40, kdlnx, Doug Anderson, rmfrfs, Lee Jones, Linux Kernel Mailing List, linux-mmc, Rafael J. Wysocki, Linux PM On Tue, Nov 24, 2020 at 9:49 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > [+cc Rafael, linux-pm for runtime PM question below] > > On Tue, Nov 24, 2020 at 02:02:02PM +0800, ricky_wu@realtek.com wrote: > > From: Ricky Wu <ricky_wu@realtek.com> > > > > rtsx_pci_sdmmc: add to support autosuspend when the rtd3_en is set > > > > rtsx_pcr: add callback functions about runtime PM > > add delay_work(rtd3_work) to decrease usage count to 0 when staying > > at idle over 10 sec > > > > rts5249: add extra flow at init function to support wakeup from d3 > > and set rtd3_en from register setting > > This looks like several patches that should be split up. The ASPM > changes should be unrelated to runtime PM. > > > Signed-off-by: Ricky Wu <ricky_wu@realtek.com> > > --- > > drivers/misc/cardreader/rts5249.c | 25 ++++-- > > drivers/misc/cardreader/rtsx_pcr.c | 122 +++++++++++++++++++++++++++-- > > drivers/misc/cardreader/rtsx_pcr.h | 1 + > > drivers/mmc/host/rtsx_pci_sdmmc.c | 18 ++++- > > include/linux/rtsx_pci.h | 1 + > > 5 files changed, 152 insertions(+), 15 deletions(-) > > > > diff --git a/drivers/misc/cardreader/rts5249.c b/drivers/misc/cardreader/rts5249.c > > index b85279f1fc5e..1da3b1ca1121 100644 > > --- a/drivers/misc/cardreader/rts5249.c > > +++ b/drivers/misc/cardreader/rts5249.c > > @@ -65,7 +65,6 @@ static void rtsx_base_fetch_vendor_settings(struct rtsx_pcr *pcr) > > pcr_dbg(pcr, "skip fetch vendor setting\n"); > > return; > > } > > - > > Doesn't look like an improvement to me. +1 > > pcr->aspm_en = rtsx_reg_to_aspm(reg); > > pcr->sd30_drive_sel_1v8 = rtsx_reg_to_sd30_drive_sel_1v8(reg); > > pcr->card_drive_sel &= 0x3F; > > @@ -73,6 +72,8 @@ static void rtsx_base_fetch_vendor_settings(struct rtsx_pcr *pcr) > > > > pci_read_config_dword(pdev, PCR_SETTING_REG2, ®); > > pcr_dbg(pcr, "Cfg 0x%x: 0x%x\n", PCR_SETTING_REG2, reg); > > + > > + pcr->rtd3_en = rtsx_reg_to_rtd3_uhsii(reg); > > if (rtsx_check_mmc_support(reg)) > > pcr->extra_caps |= EXTRA_CAPS_NO_MMC; > > pcr->sd30_drive_sel_3v3 = rtsx_reg_to_sd30_drive_sel_3v3(reg); > > @@ -278,13 +279,25 @@ static int rts5249_extra_init_hw(struct rtsx_pcr *pcr) > > > > rtsx_pci_send_cmd(pcr, CMD_TIMEOUT_DEF); > > > > - if (CHK_PCI_PID(pcr, PID_524A) || CHK_PCI_PID(pcr, PID_525A)) { > > + if (CHK_PCI_PID(pcr, PID_524A) || CHK_PCI_PID(pcr, PID_525A)) > > rtsx_pci_write_register(pcr, REG_VREF, PWD_SUSPND_EN, PWD_SUSPND_EN); > > - rtsx_pci_write_register(pcr, RTS524A_PM_CTRL3, 0x01, 0x00); > > - rtsx_pci_write_register(pcr, RTS524A_PME_FORCE_CTL, 0x30, 0x20); > > + > > + if (pcr->rtd3_en) { > > + if (CHK_PCI_PID(pcr, PID_524A) || CHK_PCI_PID(pcr, PID_525A)) { > > + rtsx_pci_write_register(pcr, RTS524A_PM_CTRL3, 0x01, 0x01); > > + rtsx_pci_write_register(pcr, RTS524A_PME_FORCE_CTL, 0x30, 0x30); > > + } else { > > + rtsx_pci_write_register(pcr, PM_CTRL3, 0x01, 0x01); > > + rtsx_pci_write_register(pcr, PME_FORCE_CTL, 0xFF, 0x33); > > + } > > } else { > > - rtsx_pci_write_register(pcr, PME_FORCE_CTL, 0xFF, 0x30); > > - rtsx_pci_write_register(pcr, PM_CTRL3, 0x01, 0x00); > > + if (CHK_PCI_PID(pcr, PID_524A) || CHK_PCI_PID(pcr, PID_525A)) { > > + rtsx_pci_write_register(pcr, RTS524A_PM_CTRL3, 0x01, 0x00); > > + rtsx_pci_write_register(pcr, RTS524A_PME_FORCE_CTL, 0x30, 0x20); > > + } else { > > + rtsx_pci_write_register(pcr, PME_FORCE_CTL, 0xFF, 0x30); > > + rtsx_pci_write_register(pcr, PM_CTRL3, 0x01, 0x00); > > + } > > } > > > > /* > > diff --git a/drivers/misc/cardreader/rtsx_pcr.c b/drivers/misc/cardreader/rtsx_pcr.c > > index 5d15607027e9..cb105563bde7 100644 > > --- a/drivers/misc/cardreader/rtsx_pcr.c > > +++ b/drivers/misc/cardreader/rtsx_pcr.c > > @@ -20,6 +20,8 @@ > > #include <linux/rtsx_pci.h> > > #include <linux/mmc/card.h> > > #include <asm/unaligned.h> > > +#include <linux/pm.h> > > +#include <linux/pm_runtime.h> > > > > #include "rtsx_pcr.h" > > #include "rts5261.h" > > @@ -89,9 +91,16 @@ static void rtsx_comm_set_aspm(struct rtsx_pcr *pcr, bool enable) > > if (pcr->aspm_enabled == enable) > > return; > > > > - pcie_capability_clear_and_set_word(pcr->pci, PCI_EXP_LNKCTL, > > - PCI_EXP_LNKCTL_ASPMC, > > - enable ? pcr->aspm_en : 0); > > + if (pcr->aspm_en & 0x02) > > + rtsx_pci_write_register(pcr, ASPM_FORCE_CTL, FORCE_ASPM_CTL0 | > > + FORCE_ASPM_CTL1, enable ? 0 : FORCE_ASPM_CTL0 | FORCE_ASPM_CTL1); > > + else > > + rtsx_pci_write_register(pcr, ASPM_FORCE_CTL, FORCE_ASPM_CTL0 | > > + FORCE_ASPM_CTL1, FORCE_ASPM_CTL0 | FORCE_ASPM_CTL1); > > + > > + if (!enable && (pcr->aspm_en & 0x02)) > > + mdelay(10); > > This is a significant change that should be in its own patch and > explained. Previously we did standard PCI config reads/writes to the > PCIe Link Control register. After this change we'll use > rtsx_pci_write_register(), which looks like it writes to an MMIO > register in a BAR: > > rtsx_pci_probe > pcr->remap_addr = ioremap(base, len) > > rtsx_pci_write_register > rtsx_pci_writel(pcr, RTSX_HAIMR, val) > iowrite32(val, pcr->remap_addr + reg) > > It's not clear that the MMIO register in the BAR is the same as the > one in config space. And we still write the Link Control register in > config space below and other places. How are these supposed to be > coordinated? > > Drivers should not change ASPM configuration directly. Especially not > in obfuscated ways like this. Indeed. > > pcr->aspm_enabled = enable; > > } > > @@ -143,6 +152,9 @@ void rtsx_pci_start_run(struct rtsx_pcr *pcr) > > /* If pci device removed, don't queue idle work any more */ > > if (pcr->remove_pci) > > return; > > + if (pcr->rtd3_en) > > + if (pcr->pci->dev.power.usage_count.counter == 0) > > + pm_runtime_get(&(pcr->pci->dev)); > > > > if (pcr->state != PDEV_STAT_RUN) { > > pcr->state = PDEV_STAT_RUN; > > @@ -1075,6 +1087,19 @@ static void rtsx_pm_power_saving(struct rtsx_pcr *pcr) > > rtsx_comm_pm_power_saving(pcr); > > } > > > > +static void rtsx_pci_rtd3_work(struct work_struct *work) > > +{ > > + struct delayed_work *dwork = to_delayed_work(work); > > + struct rtsx_pcr *pcr = container_of(dwork, struct rtsx_pcr, rtd3_work); > > + > > + pcr_dbg(pcr, "--> %s\n", __func__); > > + > > + while (pcr->pci->dev.power.usage_count.counter > 0) { > > + if (pm_runtime_active(&(pcr->pci->dev))) > > + pm_runtime_put(&(pcr->pci->dev)); > > I'm not a runtime PM expert, but this looks fishy. AFAICT this is the > only driver in the tree that uses usage_count.counter this way, which > is a pretty big hint that this needs a closer look. Cc'd Rafael. You are right, this is not correct from the PM-runtime POV. It looks like this attempts to force the PM-runtime usage counter down to 0 and it's kind of hard to say why this is done (and it shouldn't be done in the first place, because it destroys the usage counter balance). Ricky, is this an attempt to work around an issue of some sort? ^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH] misc: rtsx: rts5249 support runtime PM 2020-11-25 14:04 ` Rafael J. Wysocki @ 2020-11-26 3:07 ` 吳昊澄 Ricky 2020-11-26 14:19 ` Rafael J. Wysocki 0 siblings, 1 reply; 8+ messages in thread From: 吳昊澄 Ricky @ 2020-11-26 3:07 UTC (permalink / raw) To: Rafael J. Wysocki, Bjorn Helgaas Cc: Arnd Bergmann, Greg Kroah-Hartman, Ulf Hansson, Bjorn Helgaas, vaibhavgupta40, kdlnx, Doug Anderson, rmfrfs, Lee Jones, Linux Kernel Mailing List, linux-mmc, Rafael J. Wysocki, Linux PM > -----Original Message----- > From: Rafael J. Wysocki [mailto:rafael@kernel.org] > Sent: Wednesday, November 25, 2020 10:04 PM > To: Bjorn Helgaas; 吳昊澄 Ricky > Cc: Arnd Bergmann; Greg Kroah-Hartman; Ulf Hansson; Bjorn Helgaas; > vaibhavgupta40@gmail.com; kdlnx@doth.eu; Doug Anderson; > rmfrfs@gmail.com; Lee Jones; Linux Kernel Mailing List; linux-mmc; Rafael J. > Wysocki; Linux PM > Subject: Re: [PATCH] misc: rtsx: rts5249 support runtime PM > > On Tue, Nov 24, 2020 at 9:49 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > > > [+cc Rafael, linux-pm for runtime PM question below] > > > > On Tue, Nov 24, 2020 at 02:02:02PM +0800, ricky_wu@realtek.com wrote: > > > From: Ricky Wu <ricky_wu@realtek.com> > > > > > > rtsx_pci_sdmmc: add to support autosuspend when the rtd3_en is set > > > > > > rtsx_pcr: add callback functions about runtime PM > > > add delay_work(rtd3_work) to decrease usage count to 0 when staying > > > at idle over 10 sec > > > > > > rts5249: add extra flow at init function to support wakeup from d3 > > > and set rtd3_en from register setting > > > > This looks like several patches that should be split up. The ASPM > > changes should be unrelated to runtime PM. > > Thanks for your reply.. Ok I will split up ASPM changes and runtime PM > > > Signed-off-by: Ricky Wu <ricky_wu@realtek.com> > > > --- > > > drivers/misc/cardreader/rts5249.c | 25 ++++-- > > > drivers/misc/cardreader/rtsx_pcr.c | 122 > +++++++++++++++++++++++++++-- > > > drivers/misc/cardreader/rtsx_pcr.h | 1 + > > > drivers/mmc/host/rtsx_pci_sdmmc.c | 18 ++++- > > > include/linux/rtsx_pci.h | 1 + > > > 5 files changed, 152 insertions(+), 15 deletions(-) > > > > > > diff --git a/drivers/misc/cardreader/rts5249.c > b/drivers/misc/cardreader/rts5249.c > > > index b85279f1fc5e..1da3b1ca1121 100644 > > > --- a/drivers/misc/cardreader/rts5249.c > > > +++ b/drivers/misc/cardreader/rts5249.c > > > @@ -65,7 +65,6 @@ static void rtsx_base_fetch_vendor_settings(struct > rtsx_pcr *pcr) > > > pcr_dbg(pcr, "skip fetch vendor setting\n"); > > > return; > > > } > > > - > > > > Doesn't look like an improvement to me. > > +1 > > > > pcr->aspm_en = rtsx_reg_to_aspm(reg); > > > pcr->sd30_drive_sel_1v8 = rtsx_reg_to_sd30_drive_sel_1v8(reg); > > > pcr->card_drive_sel &= 0x3F; > > > @@ -73,6 +72,8 @@ static void rtsx_base_fetch_vendor_settings(struct > rtsx_pcr *pcr) > > > > > > pci_read_config_dword(pdev, PCR_SETTING_REG2, ®); > > > pcr_dbg(pcr, "Cfg 0x%x: 0x%x\n", PCR_SETTING_REG2, reg); > > > + > > > + pcr->rtd3_en = rtsx_reg_to_rtd3_uhsii(reg); > > > if (rtsx_check_mmc_support(reg)) > > > pcr->extra_caps |= EXTRA_CAPS_NO_MMC; > > > pcr->sd30_drive_sel_3v3 = rtsx_reg_to_sd30_drive_sel_3v3(reg); > > > @@ -278,13 +279,25 @@ static int rts5249_extra_init_hw(struct rtsx_pcr > *pcr) > > > > > > rtsx_pci_send_cmd(pcr, CMD_TIMEOUT_DEF); > > > > > > - if (CHK_PCI_PID(pcr, PID_524A) || CHK_PCI_PID(pcr, PID_525A)) { > > > + if (CHK_PCI_PID(pcr, PID_524A) || CHK_PCI_PID(pcr, PID_525A)) > > > rtsx_pci_write_register(pcr, REG_VREF, PWD_SUSPND_EN, > PWD_SUSPND_EN); > > > - rtsx_pci_write_register(pcr, RTS524A_PM_CTRL3, 0x01, > 0x00); > > > - rtsx_pci_write_register(pcr, RTS524A_PME_FORCE_CTL, > 0x30, 0x20); > > > + > > > + if (pcr->rtd3_en) { > > > + if (CHK_PCI_PID(pcr, PID_524A) || CHK_PCI_PID(pcr, > PID_525A)) { > > > + rtsx_pci_write_register(pcr, RTS524A_PM_CTRL3, > 0x01, 0x01); > > > + rtsx_pci_write_register(pcr, > RTS524A_PME_FORCE_CTL, 0x30, 0x30); > > > + } else { > > > + rtsx_pci_write_register(pcr, PM_CTRL3, 0x01, > 0x01); > > > + rtsx_pci_write_register(pcr, PME_FORCE_CTL, > 0xFF, 0x33); > > > + } > > > } else { > > > - rtsx_pci_write_register(pcr, PME_FORCE_CTL, 0xFF, 0x30); > > > - rtsx_pci_write_register(pcr, PM_CTRL3, 0x01, 0x00); > > > + if (CHK_PCI_PID(pcr, PID_524A) || CHK_PCI_PID(pcr, > PID_525A)) { > > > + rtsx_pci_write_register(pcr, RTS524A_PM_CTRL3, > 0x01, 0x00); > > > + rtsx_pci_write_register(pcr, > RTS524A_PME_FORCE_CTL, 0x30, 0x20); > > > + } else { > > > + rtsx_pci_write_register(pcr, PME_FORCE_CTL, > 0xFF, 0x30); > > > + rtsx_pci_write_register(pcr, PM_CTRL3, 0x01, > 0x00); > > > + } > > > } > > > > > > /* > > > diff --git a/drivers/misc/cardreader/rtsx_pcr.c > b/drivers/misc/cardreader/rtsx_pcr.c > > > index 5d15607027e9..cb105563bde7 100644 > > > --- a/drivers/misc/cardreader/rtsx_pcr.c > > > +++ b/drivers/misc/cardreader/rtsx_pcr.c > > > @@ -20,6 +20,8 @@ > > > #include <linux/rtsx_pci.h> > > > #include <linux/mmc/card.h> > > > #include <asm/unaligned.h> > > > +#include <linux/pm.h> > > > +#include <linux/pm_runtime.h> > > > > > > #include "rtsx_pcr.h" > > > #include "rts5261.h" > > > @@ -89,9 +91,16 @@ static void rtsx_comm_set_aspm(struct rtsx_pcr *pcr, > bool enable) > > > if (pcr->aspm_enabled == enable) > > > return; > > > > > > - pcie_capability_clear_and_set_word(pcr->pci, PCI_EXP_LNKCTL, > > > - PCI_EXP_LNKCTL_ASPMC, > > > - enable ? pcr->aspm_en : > 0); > > > + if (pcr->aspm_en & 0x02) > > > + rtsx_pci_write_register(pcr, ASPM_FORCE_CTL, > FORCE_ASPM_CTL0 | > > > + FORCE_ASPM_CTL1, enable ? 0 : > FORCE_ASPM_CTL0 | FORCE_ASPM_CTL1); > > > + else > > > + rtsx_pci_write_register(pcr, ASPM_FORCE_CTL, > FORCE_ASPM_CTL0 | > > > + FORCE_ASPM_CTL1, FORCE_ASPM_CTL0 | > FORCE_ASPM_CTL1); > > > + > > > + if (!enable && (pcr->aspm_en & 0x02)) > > > + mdelay(10); > > > > This is a significant change that should be in its own patch and > > explained. Previously we did standard PCI config reads/writes to the > > PCIe Link Control register. After this change we'll use > > rtsx_pci_write_register(), which looks like it writes to an MMIO > > register in a BAR: > > > > rtsx_pci_probe > > pcr->remap_addr = ioremap(base, len) > > > > rtsx_pci_write_register > > rtsx_pci_writel(pcr, RTSX_HAIMR, val) > > iowrite32(val, pcr->remap_addr + reg) > > > > It's not clear that the MMIO register in the BAR is the same as the > > one in config space. And we still write the Link Control register in > > config space below and other places. How are these supposed to be > > coordinated? > > > > Drivers should not change ASPM configuration directly. Especially not > > in obfuscated ways like this. > > Indeed. > We used this write register to disable/enable ASPM for our IC, we don't want to enter ASPM when IC in R/W time and initial time so we need to write this register and we think this way not conflict the " Drivers should not change ASPM configuration directly " so we remove pcie_capability_clear_and_set_word(pcr->pci, PCI_EXP_LNKCTL...) to follow not change ASPM configuration directly > > > pcr->aspm_enabled = enable; > > > } > > > @@ -143,6 +152,9 @@ void rtsx_pci_start_run(struct rtsx_pcr *pcr) > > > /* If pci device removed, don't queue idle work any more */ > > > if (pcr->remove_pci) > > > return; > > > + if (pcr->rtd3_en) > > > + if (pcr->pci->dev.power.usage_count.counter == 0) > > > + pm_runtime_get(&(pcr->pci->dev)); > > > > > > if (pcr->state != PDEV_STAT_RUN) { > > > pcr->state = PDEV_STAT_RUN; > > > @@ -1075,6 +1087,19 @@ static void rtsx_pm_power_saving(struct > rtsx_pcr *pcr) > > > rtsx_comm_pm_power_saving(pcr); > > > } > > > > > > +static void rtsx_pci_rtd3_work(struct work_struct *work) > > > +{ > > > + struct delayed_work *dwork = to_delayed_work(work); > > > + struct rtsx_pcr *pcr = container_of(dwork, struct rtsx_pcr, > rtd3_work); > > > + > > > + pcr_dbg(pcr, "--> %s\n", __func__); > > > + > > > + while (pcr->pci->dev.power.usage_count.counter > 0) { > > > + if (pm_runtime_active(&(pcr->pci->dev))) > > > + pm_runtime_put(&(pcr->pci->dev)); > > > > I'm not a runtime PM expert, but this looks fishy. AFAICT this is the > > only driver in the tree that uses usage_count.counter this way, which > > is a pretty big hint that this needs a closer look. Cc'd Rafael. > > You are right, this is not correct from the PM-runtime POV. > > It looks like this attempts to force the PM-runtime usage counter down > to 0 and it's kind of hard to say why this is done (and it shouldn't > be done in the first place, because it destroys the usage counter > balance). > > Ricky, is this an attempt to work around an issue of some sort? > Thanks Bjorn and Rafael I found when we boot up, our dev pcr->pci->dev.power.usage_count.counter always is 2, Don’t know how to make it to 0 because we need to support D3 and run runtime_suspended callback function Is there something wrong with us to enable runtime PM? > > static void rtsx_pci_idle_work(struct work_struct *work) > > { > > struct delayed_work *dwork = to_delayed_work(work); > > @@ -1094,6 +1119,9 @@ static void rtsx_pci_idle_work(struct work_struct > *work) > > rtsx_pm_power_saving(pcr); > > > > mutex_unlock(&pcr->pcr_mutex); > > + > > + if (pcr->rtd3_en) > > + mod_delayed_work(system_wq, &pcr->rtd3_work, > msecs_to_jiffies(10000)); > > } > > > > static void rtsx_base_force_power_down(struct rtsx_pcr *pcr, u8 pm_state) > > @@ -1283,7 +1311,7 @@ static int rtsx_pci_init_hw(struct rtsx_pcr *pcr) > > /* Wait SSC power stable */ > > udelay(200); > > > > - rtsx_pci_disable_aspm(pcr); > > + rtsx_disable_aspm(pcr); > > if (pcr->ops->optimize_phy) { > > err = pcr->ops->optimize_phy(pcr); > > if (err < 0) > > @@ -1357,8 +1385,7 @@ static int rtsx_pci_init_hw(struct rtsx_pcr *pcr) > > rtsx_pci_init_ocp(pcr); > > > > /* Enable clk_request_n to enable clock power management */ > > - pcie_capability_write_word(pdev, PCI_EXP_LNKCTL, > > - PCI_EXP_LNKCTL_CLKREQ_EN); > > + pci_write_config_byte(pdev, PCI_EXP_LNKCTL+1, 0x01); > > Drivers should not write PCI_EXP_LNKCTL directly because that clobbers > things done by the PCI core, e.g., ASPM and common clock > configuration, Read Completion Boundary setting, etc. > > But if you must, you should at least use this: > > pcie_capability_clear_and_set_word(pdev, PCI_EXP_LNKCTL, 0, > PCI_EXP_LNKCTL_CLKREQ_EN); > OK thanks for your advice > > /* Enter L1 when host tx idle */ > > pci_write_config_byte(pdev, 0x70F, 0x5B); > > > > @@ -1368,6 +1395,8 @@ static int rtsx_pci_init_hw(struct rtsx_pcr *pcr) > > return err; > > } > > > > + rtsx_pci_write_register(pcr, ASPM_FORCE_CTL, 0x30, 0x30); > > + > > /* No CD interrupt if probing driver with card inserted. > > * So we need to initialize pcr->card_exist here. > > */ > > @@ -1571,6 +1600,12 @@ static int rtsx_pci_probe(struct pci_dev *pcidev, > > rtsx_pcr_cells[i].platform_data = handle; > > rtsx_pcr_cells[i].pdata_size = sizeof(*handle); > > } > > + > > + if (pcr->rtd3_en) { > > + INIT_DELAYED_WORK(&pcr->rtd3_work, rtsx_pci_rtd3_work); > > + pm_runtime_enable(&pcidev->dev); > > + } > > + > > ret = mfd_add_devices(&pcidev->dev, pcr->id, rtsx_pcr_cells, > > ARRAY_SIZE(rtsx_pcr_cells), NULL, 0, NULL); > > if (ret < 0) > > @@ -1610,6 +1645,9 @@ static void rtsx_pci_remove(struct pci_dev *pcidev) > > > > pcr->remove_pci = true; > > > > + if (pcr->rtd3_en) > > + pm_runtime_get_noresume(&pcr->pci->dev); > > + > > /* Disable interrupts at the pcr level */ > > spin_lock_irq(&pcr->lock); > > rtsx_pci_writel(pcr, RTSX_BIER, 0); > > @@ -1619,6 +1657,9 @@ static void rtsx_pci_remove(struct pci_dev *pcidev) > > cancel_delayed_work_sync(&pcr->carddet_work); > > cancel_delayed_work_sync(&pcr->idle_work); > > > > + if (pcr->rtd3_en) > > + cancel_delayed_work_sync(&pcr->rtd3_work); > > + > > mfd_remove_devices(&pcidev->dev); > > > > dma_free_coherent(&(pcr->pci->dev), RTSX_RESV_BUF_LEN, > > @@ -1635,6 +1676,11 @@ static void rtsx_pci_remove(struct pci_dev > *pcidev) > > idr_remove(&rtsx_pci_idr, pcr->id); > > spin_unlock(&rtsx_pci_lock); > > > > + if (pcr->rtd3_en) { > > + pm_runtime_disable(&pcr->pci->dev); > > + pm_runtime_put_noidle(&pcr->pci->dev); > > + } > > + > > kfree(pcr->slots); > > kfree(pcr); > > kfree(handle); > > @@ -1716,13 +1762,75 @@ static void rtsx_pci_shutdown(struct pci_dev > *pcidev) > > pci_disable_msi(pcr->pci); > > } > > > > +static int rtsx_pci_runtime_suspend(struct device *device) > > +{ > > + struct pci_dev *pcidev = to_pci_dev(device); > > + struct pcr_handle *handle; > > + struct rtsx_pcr *pcr; > > + > > + handle = pci_get_drvdata(pcidev); > > + pcr = handle->pcr; > > + dev_dbg(&(pcidev->dev), "--> %s\n", __func__); > > + > > + cancel_delayed_work(&pcr->carddet_work); > > + cancel_delayed_work(&pcr->rtd3_work); > > + cancel_delayed_work(&pcr->idle_work); > > + > > + mutex_lock(&pcr->pcr_mutex); > > + rtsx_pci_power_off(pcr, HOST_ENTER_S3); > > + > > + free_irq(pcr->irq, (void *)pcr); > > + > > + mutex_unlock(&pcr->pcr_mutex); > > + > > + return 0; > > +} > > + > > +static int rtsx_pci_runtime_resume(struct device *device) > > +{ > > + struct pci_dev *pcidev = to_pci_dev(device); > > + struct pcr_handle *handle; > > + struct rtsx_pcr *pcr; > > + int ret = 0; > > + > > + handle = pci_get_drvdata(pcidev); > > + pcr = handle->pcr; > > + dev_dbg(&(pcidev->dev), "--> %s\n", __func__); > > + > > + mutex_lock(&pcr->pcr_mutex); > > + > > + rtsx_pci_write_register(pcr, HOST_SLEEP_STATE, 0x03, 0x00); > > + rtsx_pci_acquire_irq(pcr); > > + synchronize_irq(pcr->irq); > > + > > + if (pcr->ops->fetch_vendor_settings) > > + pcr->ops->fetch_vendor_settings(pcr); > > + > > + rtsx_pci_init_hw(pcr); > > + > > + if (pcr->slots[RTSX_SD_CARD].p_dev != NULL) { > > + pcr->slots[RTSX_SD_CARD].card_event( > > + pcr->slots[RTSX_SD_CARD].p_dev); > > + } > > + > > + schedule_delayed_work(&pcr->idle_work, msecs_to_jiffies(200)); > > + > > + mutex_unlock(&pcr->pcr_mutex); > > + return ret; > > +} > > + > > #else /* CONFIG_PM */ > > > > #define rtsx_pci_shutdown NULL > > +#define rtsx_pci_runtime_suspend NULL > > +#define rtsx_pic_runtime_resume NULL > > > > #endif /* CONFIG_PM */ > > > > -static SIMPLE_DEV_PM_OPS(rtsx_pci_pm_ops, rtsx_pci_suspend, > rtsx_pci_resume); > > +static const struct dev_pm_ops rtsx_pci_pm_ops = { > > + SET_SYSTEM_SLEEP_PM_OPS(rtsx_pci_suspend, rtsx_pci_resume) > > + SET_RUNTIME_PM_OPS(rtsx_pci_runtime_suspend, > rtsx_pci_runtime_resume, NULL) > > +}; > > > > static struct pci_driver rtsx_pci_driver = { > > .name = DRV_NAME_RTSX_PCI, > > diff --git a/drivers/misc/cardreader/rtsx_pcr.h > b/drivers/misc/cardreader/rtsx_pcr.h > > index fe5f4ca0f937..daf057c4eea6 100644 > > --- a/drivers/misc/cardreader/rtsx_pcr.h > > +++ b/drivers/misc/cardreader/rtsx_pcr.h > > @@ -90,6 +90,7 @@ static inline u8 map_sd_drive(int idx) > > > > #define rtsx_check_mmc_support(reg) ((reg) & 0x10) > > #define rtsx_reg_to_rtd3(reg) ((reg) & 0x02) > > +#define rtsx_reg_to_rtd3_uhsii(reg) ((reg) & 0x04) > > #define rtsx_reg_to_aspm(reg) (((reg) >> 28) & 0x03) > > #define rtsx_reg_to_sd30_drive_sel_1v8(reg) (((reg) >> 26) & 0x03) > > #define rtsx_reg_to_sd30_drive_sel_3v3(reg) (((reg) >> 5) & 0x03) > > diff --git a/drivers/mmc/host/rtsx_pci_sdmmc.c > b/drivers/mmc/host/rtsx_pci_sdmmc.c > > index eb395e144207..02f31f4d0944 100644 > > --- a/drivers/mmc/host/rtsx_pci_sdmmc.c > > +++ b/drivers/mmc/host/rtsx_pci_sdmmc.c > > @@ -20,6 +20,7 @@ > > #include <linux/mmc/card.h> > > #include <linux/rtsx_pci.h> > > #include <asm/unaligned.h> > > +#include <linux/pm_runtime.h> > > > > struct realtek_pci_sdmmc { > > struct platform_device *pdev; > > @@ -953,7 +954,6 @@ static int sd_set_power_mode(struct > realtek_pci_sdmmc *host, > > unsigned char power_mode) > > { > > int err; > > - > > Not an improvement. Looks like accidental whitespace change. But if > you wanted to improve this function, you could (in a separate patch) > make it look like this: > > static int sd_set_power_mode(...) > { > if (power_mode == MMC_POWER_OFF) > return sd_power_off(host); > else > return sd_power_on(host); > } > OK thanks for your advice > > if (power_mode == MMC_POWER_OFF) > > err = sd_power_off(host); > > else > > @@ -1249,7 +1249,7 @@ static int sdmmc_switch_voltage(struct mmc_host > *mmc, struct mmc_ios *ios) > > > > out: > > /* Stop toggle SD clock in idle */ > > - err = rtsx_pci_write_register(pcr, SD_BUS_STAT, > > + rtsx_pci_write_register(pcr, SD_BUS_STAT, > > SD_CLK_TOGGLE_EN | SD_CLK_FORCE_STOP, 0); > > > > mutex_unlock(&pcr->pcr_mutex); > > @@ -1343,6 +1343,7 @@ static void init_extra_caps(struct > realtek_pci_sdmmc *host) > > static void realtek_init_host(struct realtek_pci_sdmmc *host) > > { > > struct mmc_host *mmc = host->mmc; > > + struct rtsx_pcr *pcr = host->pcr; > > > > mmc->f_min = 250000; > > mmc->f_max = 208000000; > > @@ -1350,6 +1351,8 @@ static void realtek_init_host(struct > realtek_pci_sdmmc *host) > > mmc->caps = MMC_CAP_4_BIT_DATA | MMC_CAP_SD_HIGHSPEED | > > MMC_CAP_MMC_HIGHSPEED | MMC_CAP_BUS_WIDTH_TEST | > > MMC_CAP_UHS_SDR12 | MMC_CAP_UHS_SDR25; > > + if (pcr->rtd3_en) > > + mmc->caps = mmc->caps | MMC_CAP_AGGRESSIVE_PM; > > mmc->caps2 = MMC_CAP2_NO_PRESCAN_POWERUP | > MMC_CAP2_FULL_PWR_CYCLE; > > mmc->max_current_330 = 400; > > mmc->max_current_180 = 800; > > @@ -1407,6 +1410,12 @@ static int rtsx_pci_sdmmc_drv_probe(struct > platform_device *pdev) > > > > realtek_init_host(host); > > > > + if (pcr->rtd3_en) { > > + pm_runtime_use_autosuspend(&pdev->dev); > > + pm_runtime_set_autosuspend_delay(&pdev->dev, 5000); > > I think you should swap the order of these so the delay is set first. > Maybe it doesn't matter because both are before pm_runtime_enable(), > but it's always good style to set parameters before enabling a > feature. > OK thanks for your advice > > + pm_runtime_enable(&pdev->dev); > > + } > > + > > mmc_add_host(mmc); > > > > return 0; > > @@ -1426,6 +1435,11 @@ static int rtsx_pci_sdmmc_drv_remove(struct > platform_device *pdev) > > pcr->slots[RTSX_SD_CARD].card_event = NULL; > > mmc = host->mmc; > > > > + if (pcr->rtd3_en) { > > + pm_runtime_dont_use_autosuspend(&pdev->dev); > > + pm_runtime_disable(&pdev->dev); > > + } > > + > > cancel_work_sync(&host->work); > > > > mutex_lock(&host->host_mutex); > > diff --git a/include/linux/rtsx_pci.h b/include/linux/rtsx_pci.h > > index 745f5e73f99a..689d955c2246 100644 > > --- a/include/linux/rtsx_pci.h > > +++ b/include/linux/rtsx_pci.h > > @@ -1174,6 +1174,7 @@ struct rtsx_pcr { > > > > struct delayed_work carddet_work; > > struct delayed_work idle_work; > > + struct delayed_work rtd3_work; > > > > spinlock_t lock; > > struct mutex pcr_mutex; > > -- > > 2.17.1 > ------Please consider the environment before printing this e-mail. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] misc: rtsx: rts5249 support runtime PM 2020-11-26 3:07 ` 吳昊澄 Ricky @ 2020-11-26 14:19 ` Rafael J. Wysocki 2020-11-26 15:02 ` Ulf Hansson 0 siblings, 1 reply; 8+ messages in thread From: Rafael J. Wysocki @ 2020-11-26 14:19 UTC (permalink / raw) To: 吳昊澄 Ricky Cc: Rafael J. Wysocki, Bjorn Helgaas, Arnd Bergmann, Greg Kroah-Hartman, Ulf Hansson, Bjorn Helgaas, vaibhavgupta40, kdlnx, Doug Anderson, rmfrfs, Lee Jones, Linux Kernel Mailing List, linux-mmc, Rafael J. Wysocki, Linux PM On Thu, Nov 26, 2020 at 4:07 AM 吳昊澄 Ricky <ricky_wu@realtek.com> wrote: > > > -----Original Message----- > > From: Rafael J. Wysocki [mailto:rafael@kernel.org] > > Sent: Wednesday, November 25, 2020 10:04 PM > > To: Bjorn Helgaas; 吳昊澄 Ricky [cut] > > > > +static void rtsx_pci_rtd3_work(struct work_struct *work) > > > > +{ > > > > + struct delayed_work *dwork = to_delayed_work(work); > > > > + struct rtsx_pcr *pcr = container_of(dwork, struct rtsx_pcr, > > rtd3_work); > > > > + > > > > + pcr_dbg(pcr, "--> %s\n", __func__); > > > > + > > > > + while (pcr->pci->dev.power.usage_count.counter > 0) { > > > > + if (pm_runtime_active(&(pcr->pci->dev))) > > > > + pm_runtime_put(&(pcr->pci->dev)); > > > > > > I'm not a runtime PM expert, but this looks fishy. AFAICT this is the > > > only driver in the tree that uses usage_count.counter this way, which > > > is a pretty big hint that this needs a closer look. Cc'd Rafael. > > > > You are right, this is not correct from the PM-runtime POV. > > > > It looks like this attempts to force the PM-runtime usage counter down > > to 0 and it's kind of hard to say why this is done (and it shouldn't > > be done in the first place, because it destroys the usage counter > > balance). > > > > Ricky, is this an attempt to work around an issue of some sort? > > > > Thanks Bjorn and Rafael > I found when we boot up, our dev pcr->pci->dev.power.usage_count.counter always is 2, > Don’t know how to make it to 0 because we need to support D3 and run runtime_suspended callback function > Is there something wrong with us to enable runtime PM? That is possible. If you want it to be enabled by default, you need to call pm_runtime_allow() from the driver at probe time, in addition to pm_runtime_enable(), in the first place, but that only drops one reference, so question is where the other one comes from. Are the pm_runtime_get*() and pm_runtime_put*() calls balanced? ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] misc: rtsx: rts5249 support runtime PM 2020-11-26 14:19 ` Rafael J. Wysocki @ 2020-11-26 15:02 ` Ulf Hansson 0 siblings, 0 replies; 8+ messages in thread From: Ulf Hansson @ 2020-11-26 15:02 UTC (permalink / raw) To: Rafael J. Wysocki, 吳昊澄 Ricky Cc: Bjorn Helgaas, Arnd Bergmann, Greg Kroah-Hartman, Bjorn Helgaas, vaibhavgupta40, kdlnx, Doug Anderson, rmfrfs, Lee Jones, Linux Kernel Mailing List, linux-mmc, Rafael J. Wysocki, Linux PM On Thu, 26 Nov 2020 at 15:19, Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Thu, Nov 26, 2020 at 4:07 AM 吳昊澄 Ricky <ricky_wu@realtek.com> wrote: > > > > > -----Original Message----- > > > From: Rafael J. Wysocki [mailto:rafael@kernel.org] > > > Sent: Wednesday, November 25, 2020 10:04 PM > > > To: Bjorn Helgaas; 吳昊澄 Ricky > > [cut] > > > > > > +static void rtsx_pci_rtd3_work(struct work_struct *work) > > > > > +{ > > > > > + struct delayed_work *dwork = to_delayed_work(work); > > > > > + struct rtsx_pcr *pcr = container_of(dwork, struct rtsx_pcr, > > > rtd3_work); > > > > > + > > > > > + pcr_dbg(pcr, "--> %s\n", __func__); > > > > > + > > > > > + while (pcr->pci->dev.power.usage_count.counter > 0) { > > > > > + if (pm_runtime_active(&(pcr->pci->dev))) > > > > > + pm_runtime_put(&(pcr->pci->dev)); > > > > > > > > I'm not a runtime PM expert, but this looks fishy. AFAICT this is the > > > > only driver in the tree that uses usage_count.counter this way, which > > > > is a pretty big hint that this needs a closer look. Cc'd Rafael. > > > > > > You are right, this is not correct from the PM-runtime POV. > > > > > > It looks like this attempts to force the PM-runtime usage counter down > > > to 0 and it's kind of hard to say why this is done (and it shouldn't > > > be done in the first place, because it destroys the usage counter > > > balance). > > > > > > Ricky, is this an attempt to work around an issue of some sort? > > > > > > > Thanks Bjorn and Rafael > > I found when we boot up, our dev pcr->pci->dev.power.usage_count.counter always is 2, > > Don’t know how to make it to 0 because we need to support D3 and run runtime_suspended callback function > > Is there something wrong with us to enable runtime PM? > > That is possible. > > If you want it to be enabled by default, you need to call > pm_runtime_allow() from the driver at probe time, in addition to > pm_runtime_enable(), in the first place, but that only drops one > reference, so question is where the other one comes from. Yes, good point. Moreover, I am wondering whether you also need to deploy support for runtime PM for the child device (managed by drivers/mmc/host/rtsx_pci_sdmmc.c driver), as to make the support complete. > > Are the pm_runtime_get*() and pm_runtime_put*() calls balanced? Perhaps have a look at how the drivers/misc/cardreader/rtsx_usb.c and drivers/mmc/host/rtsx_usb_sdmmc.c have implemented this could help. I know it's USB, but it should work quite similar in regards to runtime PM, I think. Kind regards Uffe ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-11-26 15:03 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-11-24 6:02 [PATCH] misc: rtsx: rts5249 support runtime PM ricky_wu 2020-11-24 8:40 ` kernel test robot 2020-11-24 8:55 ` kernel test robot 2020-11-24 20:49 ` Bjorn Helgaas 2020-11-25 14:04 ` Rafael J. Wysocki 2020-11-26 3:07 ` 吳昊澄 Ricky 2020-11-26 14:19 ` Rafael J. Wysocki 2020-11-26 15:02 ` Ulf Hansson
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.