2020-09-07 10:12:04

by Ricky Wu

Subject: [PATCH v5 2/2] misc: rtsx: Add power saving functions and fix driving parameter

From: Ricky Wu <[email protected]>

v4:
split power down flow and power saving function to two patch

v5:
fix up modified change under the --- line

Add rts522a L1 sub-state support
Save more power on rts5227 rts5249 rts525a rts5260
Fix rts5260 driving parameter

Signed-off-by: Ricky Wu <[email protected]>
---
drivers/misc/cardreader/rts5227.c | 112 +++++++++++++++++++++-
drivers/misc/cardreader/rts5249.c | 145 ++++++++++++++++++++++++++++-
drivers/misc/cardreader/rts5260.c | 28 +++---
drivers/misc/cardreader/rtsx_pcr.h | 17 ++++
4 files changed, 283 insertions(+), 19 deletions(-)

diff --git a/drivers/misc/cardreader/rts5227.c b/drivers/misc/cardreader/rts5227.c
index 747391e3fb5d..8859011672cb 100644
--- a/drivers/misc/cardreader/rts5227.c
+++ b/drivers/misc/cardreader/rts5227.c
@@ -72,15 +72,80 @@ static void rts5227_fetch_vendor_settings(struct rtsx_pcr *pcr)

pci_read_config_dword(pdev, PCR_SETTING_REG2, &reg);
pcr_dbg(pcr, "Cfg 0x%x: 0x%x\n", PCR_SETTING_REG2, 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);
if (rtsx_reg_check_reverse_socket(reg))
pcr->flags |= PCR_REVERSE_SOCKET;
}

+static void rts5227_init_from_cfg(struct rtsx_pcr *pcr)
+{
+ struct pci_dev *pdev = pcr->pci;
+ int l1ss;
+ u32 lval;
+ struct rtsx_cr_option *option = &pcr->option;
+
+ l1ss = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_L1SS);
+ if (!l1ss)
+ return;
+
+ pci_read_config_dword(pdev, l1ss + PCI_L1SS_CTL1, &lval);
+
+ if (CHK_PCI_PID(pcr, 0x522A)) {
+ if (0 == (lval & 0x0F))
+ rtsx_pci_enable_oobs_polling(pcr);
+ else
+ rtsx_pci_disable_oobs_polling(pcr);
+ }
+
+ if (lval & PCI_L1SS_CTL1_ASPM_L1_1)
+ rtsx_set_dev_flag(pcr, ASPM_L1_1_EN);
+ else
+ rtsx_clear_dev_flag(pcr, ASPM_L1_1_EN);
+
+ if (lval & PCI_L1SS_CTL1_ASPM_L1_2)
+ rtsx_set_dev_flag(pcr, ASPM_L1_2_EN);
+ else
+ rtsx_clear_dev_flag(pcr, ASPM_L1_2_EN);
+
+ if (lval & PCI_L1SS_CTL1_PCIPM_L1_1)
+ rtsx_set_dev_flag(pcr, PM_L1_1_EN);
+ else
+ rtsx_clear_dev_flag(pcr, PM_L1_1_EN);
+
+ if (lval & PCI_L1SS_CTL1_PCIPM_L1_2)
+ rtsx_set_dev_flag(pcr, PM_L1_2_EN);
+ else
+ rtsx_clear_dev_flag(pcr, PM_L1_2_EN);
+
+ if (option->ltr_en) {
+ u16 val;
+
+ pcie_capability_read_word(pcr->pci, PCI_EXP_DEVCTL2, &val);
+ if (val & PCI_EXP_DEVCTL2_LTR_EN) {
+ option->ltr_enabled = true;
+ option->ltr_active = true;
+ rtsx_set_ltr_latency(pcr, option->ltr_active_latency);
+ } else {
+ option->ltr_enabled = false;
+ }
+ }
+
+ if (rtsx_check_dev_flag(pcr, ASPM_L1_1_EN | ASPM_L1_2_EN
+ | PM_L1_1_EN | PM_L1_2_EN))
+ option->force_clkreq_0 = false;
+ else
+ option->force_clkreq_0 = true;
+
+}
+
static int rts5227_extra_init_hw(struct rtsx_pcr *pcr)
{
u16 cap;
+ struct rtsx_cr_option *option = &pcr->option;

+ rts5227_init_from_cfg(pcr);
rtsx_pci_init_cmd(pcr);

/* Configure GPIO as output */
@@ -102,9 +167,17 @@ static int rts5227_extra_init_hw(struct rtsx_pcr *pcr)
rts5227_fill_driving(pcr, OUTPUT_3V3);
/* Configure force_clock_req */
if (pcr->flags & PCR_REVERSE_SOCKET)
- rtsx_pci_add_cmd(pcr, WRITE_REG_CMD, PETXCFG, 0xB8, 0xB8);
+ rtsx_pci_add_cmd(pcr, WRITE_REG_CMD, PETXCFG, 0x30, 0x30);
else
- rtsx_pci_add_cmd(pcr, WRITE_REG_CMD, PETXCFG, 0xB8, 0x88);
+ rtsx_pci_add_cmd(pcr, WRITE_REG_CMD, PETXCFG, 0x30, 0x00);
+
+ if (option->force_clkreq_0)
+ rtsx_pci_add_cmd(pcr, WRITE_REG_CMD, PETXCFG,
+ FORCE_CLKREQ_DELINK_MASK, FORCE_CLKREQ_LOW);
+ else
+ rtsx_pci_add_cmd(pcr, WRITE_REG_CMD, PETXCFG,
+ FORCE_CLKREQ_DELINK_MASK, FORCE_CLKREQ_HIGH);
+
rtsx_pci_add_cmd(pcr, WRITE_REG_CMD, pcr->reg_pm_ctrl3, 0x10, 0x00);

return rtsx_pci_send_cmd(pcr, 100);
@@ -359,6 +432,27 @@ static int rts522a_switch_output_voltage(struct rtsx_pcr *pcr, u8 voltage)
return rtsx_pci_send_cmd(pcr, 100);
}

+static void rts522a_set_l1off_cfg_sub_d0(struct rtsx_pcr *pcr, int active)
+{
+ struct rtsx_cr_option *option = &pcr->option;
+ int aspm_L1_1, aspm_L1_2;
+ u8 val = 0;
+
+ aspm_L1_1 = rtsx_check_dev_flag(pcr, ASPM_L1_1_EN);
+ aspm_L1_2 = rtsx_check_dev_flag(pcr, ASPM_L1_2_EN);
+
+ if (active) {
+ /* run, latency: 60us */
+ if (aspm_L1_1)
+ val = option->ltr_l1off_snooze_sspwrgate;
+ } else {
+ /* l1off, latency: 300us */
+ if (aspm_L1_2)
+ val = option->ltr_l1off_sspwrgate;
+ }
+
+ rtsx_set_l1off_sub(pcr, val);
+}

/* rts522a operations mainly derived from rts5227, except phy/hw init setting.
*/
@@ -375,15 +469,29 @@ static const struct pcr_ops rts522a_pcr_ops = {
.switch_output_voltage = rts522a_switch_output_voltage,
.cd_deglitch = NULL,
.conv_clk_and_div_n = NULL,
+ .set_l1off_cfg_sub_d0 = rts522a_set_l1off_cfg_sub_d0,
};

void rts522a_init_params(struct rtsx_pcr *pcr)
{
+ struct rtsx_cr_option *option = &pcr->option;
+
rts5227_init_params(pcr);
pcr->ops = &rts522a_pcr_ops;
pcr->tx_initial_phase = SET_CLOCK_PHASE(20, 20, 11);
pcr->reg_pm_ctrl3 = RTS522A_PM_CTRL3;

+ option->dev_flags = LTR_L1SS_PWR_GATE_EN;
+ option->ltr_en = true;
+
+ /* init latency of active, idle, L1OFF to 60us, 300us, 3ms */
+ option->ltr_active_latency = LTR_ACTIVE_LATENCY_DEF;
+ option->ltr_idle_latency = LTR_IDLE_LATENCY_DEF;
+ option->ltr_l1off_latency = LTR_L1OFF_LATENCY_DEF;
+ option->l1_snooze_delay = L1_SNOOZE_DELAY_DEF;
+ option->ltr_l1off_sspwrgate = 0x7F;
+ option->ltr_l1off_snooze_sspwrgate = 0x78;
+
pcr->option.ocp_en = 1;
if (pcr->option.ocp_en)
pcr->hw_param.interrupt_en |= SD_OC_INT_EN;
diff --git a/drivers/misc/cardreader/rts5249.c b/drivers/misc/cardreader/rts5249.c
index 719aa2d61919..b85279f1fc5e 100644
--- a/drivers/misc/cardreader/rts5249.c
+++ b/drivers/misc/cardreader/rts5249.c
@@ -73,6 +73,8 @@ static void rtsx_base_fetch_vendor_settings(struct rtsx_pcr *pcr)

pci_read_config_dword(pdev, PCR_SETTING_REG2, &reg);
pcr_dbg(pcr, "Cfg 0x%x: 0x%x\n", PCR_SETTING_REG2, 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);
if (rtsx_reg_check_reverse_socket(reg))
pcr->flags |= PCR_REVERSE_SOCKET;
@@ -91,6 +93,14 @@ static void rts5249_init_from_cfg(struct rtsx_pcr *pcr)

pci_read_config_dword(pdev, l1ss + PCI_L1SS_CTL1, &lval);

+ if (CHK_PCI_PID(pcr, PID_524A) || CHK_PCI_PID(pcr, PID_525A)) {
+ if (0 == (lval & 0x0F))
+ rtsx_pci_enable_oobs_polling(pcr);
+ else
+ rtsx_pci_disable_oobs_polling(pcr);
+ }
+
+
if (lval & PCI_L1SS_CTL1_ASPM_L1_1)
rtsx_set_dev_flag(pcr, ASPM_L1_1_EN);

@@ -130,6 +140,112 @@ static int rts5249_init_from_hw(struct rtsx_pcr *pcr)
return 0;
}

+static void rts52xa_save_content_from_efuse(struct rtsx_pcr *pcr)
+{
+ u8 cnt, sv;
+ u16 j = 0;
+ u8 tmp;
+ u8 val;
+ int i;
+
+ rtsx_pci_write_register(pcr, RTS524A_PME_FORCE_CTL,
+ REG_EFUSE_BYPASS | REG_EFUSE_POR, REG_EFUSE_POR);
+ udelay(1);
+
+ pcr_dbg(pcr, "Enable efuse por!");
+ pcr_dbg(pcr, "save efuse to autoload");
+
+ rtsx_pci_write_register(pcr, RTS525A_EFUSE_ADD, REG_EFUSE_ADD_MASK, 0x00);
+ rtsx_pci_write_register(pcr, RTS525A_EFUSE_CTL,
+ REG_EFUSE_ENABLE | REG_EFUSE_MODE, REG_EFUSE_ENABLE);
+ /* Wait transfer end */
+ for (j = 0; j < 1024; j++) {
+ rtsx_pci_read_register(pcr, RTS525A_EFUSE_CTL, &tmp);
+ if ((tmp & 0x80) == 0)
+ break;
+ }
+ rtsx_pci_read_register(pcr, RTS525A_EFUSE_DATA, &val);
+ cnt = val & 0x0F;
+ sv = val & 0x10;
+
+ if (sv) {
+ for (i = 0; i < 4; i++) {
+ rtsx_pci_write_register(pcr, RTS525A_EFUSE_ADD,
+ REG_EFUSE_ADD_MASK, 0x04 + i);
+ rtsx_pci_write_register(pcr, RTS525A_EFUSE_CTL,
+ REG_EFUSE_ENABLE | REG_EFUSE_MODE, REG_EFUSE_ENABLE);
+ /* Wait transfer end */
+ for (j = 0; j < 1024; j++) {
+ rtsx_pci_read_register(pcr, RTS525A_EFUSE_CTL, &tmp);
+ if ((tmp & 0x80) == 0)
+ break;
+ }
+ rtsx_pci_read_register(pcr, RTS525A_EFUSE_DATA, &val);
+ rtsx_pci_write_register(pcr, 0xFF04 + i, 0xFF, val);
+ }
+ } else {
+ rtsx_pci_write_register(pcr, 0xFF04, 0xFF, (u8)PCI_VID(pcr));
+ rtsx_pci_write_register(pcr, 0xFF05, 0xFF, (u8)(PCI_VID(pcr) >> 8));
+ rtsx_pci_write_register(pcr, 0xFF06, 0xFF, (u8)PCI_PID(pcr));
+ rtsx_pci_write_register(pcr, 0xFF07, 0xFF, (u8)(PCI_PID(pcr) >> 8));
+ }
+
+ for (i = 0; i < cnt * 4; i++) {
+ if (sv)
+ rtsx_pci_write_register(pcr, RTS525A_EFUSE_ADD,
+ REG_EFUSE_ADD_MASK, 0x08 + i);
+ else
+ rtsx_pci_write_register(pcr, RTS525A_EFUSE_ADD,
+ REG_EFUSE_ADD_MASK, 0x04 + i);
+ rtsx_pci_write_register(pcr, RTS525A_EFUSE_CTL,
+ REG_EFUSE_ENABLE | REG_EFUSE_MODE, REG_EFUSE_ENABLE);
+ /* Wait transfer end */
+ for (j = 0; j < 1024; j++) {
+ rtsx_pci_read_register(pcr, RTS525A_EFUSE_CTL, &tmp);
+ if ((tmp & 0x80) == 0)
+ break;
+ }
+ rtsx_pci_read_register(pcr, RTS525A_EFUSE_DATA, &val);
+ rtsx_pci_write_register(pcr, 0xFF08 + i, 0xFF, val);
+ }
+ rtsx_pci_write_register(pcr, 0xFF00, 0xFF, (cnt & 0x7F) | 0x80);
+ rtsx_pci_write_register(pcr, RTS524A_PME_FORCE_CTL,
+ REG_EFUSE_BYPASS | REG_EFUSE_POR, REG_EFUSE_BYPASS);
+ pcr_dbg(pcr, "Disable efuse por!");
+}
+
+static void rts52xa_save_content_to_autoload_space(struct rtsx_pcr *pcr)
+{
+ u8 val;
+
+ rtsx_pci_read_register(pcr, RESET_LOAD_REG, &val);
+ if (val & 0x02) {
+ rtsx_pci_read_register(pcr, RTS525A_BIOS_CFG, &val);
+ if (val & RTS525A_LOAD_BIOS_FLAG) {
+ rtsx_pci_write_register(pcr, RTS525A_BIOS_CFG,
+ RTS525A_LOAD_BIOS_FLAG, RTS525A_CLEAR_BIOS_FLAG);
+
+ rtsx_pci_write_register(pcr, RTS524A_PME_FORCE_CTL,
+ REG_EFUSE_POWER_MASK, REG_EFUSE_POWERON);
+
+ pcr_dbg(pcr, "Power ON efuse!");
+ mdelay(1);
+ rts52xa_save_content_from_efuse(pcr);
+ } else {
+ rtsx_pci_read_register(pcr, RTS524A_PME_FORCE_CTL, &val);
+ if (!(val & 0x08))
+ rts52xa_save_content_from_efuse(pcr);
+ }
+ } else {
+ pcr_dbg(pcr, "Load from autoload");
+ rtsx_pci_write_register(pcr, 0xFF00, 0xFF, 0x80);
+ rtsx_pci_write_register(pcr, 0xFF04, 0xFF, (u8)PCI_VID(pcr));
+ rtsx_pci_write_register(pcr, 0xFF05, 0xFF, (u8)(PCI_VID(pcr) >> 8));
+ rtsx_pci_write_register(pcr, 0xFF06, 0xFF, (u8)PCI_PID(pcr));
+ rtsx_pci_write_register(pcr, 0xFF07, 0xFF, (u8)(PCI_PID(pcr) >> 8));
+ }
+}
+
static int rts5249_extra_init_hw(struct rtsx_pcr *pcr)
{
struct rtsx_cr_option *option = &(pcr->option);
@@ -139,6 +255,9 @@ static int rts5249_extra_init_hw(struct rtsx_pcr *pcr)

rtsx_pci_init_cmd(pcr);

+ if (CHK_PCI_PID(pcr, PID_524A) || CHK_PCI_PID(pcr, PID_525A))
+ rts52xa_save_content_to_autoload_space(pcr);
+
/* Rest L1SUB Config */
rtsx_pci_add_cmd(pcr, WRITE_REG_CMD, L1SUB_CONFIG3, 0xFF, 0x00);
/* Configure GPIO as output */
@@ -157,18 +276,36 @@ static int rts5249_extra_init_hw(struct rtsx_pcr *pcr)
else
rtsx_pci_add_cmd(pcr, WRITE_REG_CMD, PETXCFG, 0xB0, 0x80);

+ rtsx_pci_send_cmd(pcr, CMD_TIMEOUT_DEF);
+
+ 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);
+ } else {
+ rtsx_pci_write_register(pcr, PME_FORCE_CTL, 0xFF, 0x30);
+ rtsx_pci_write_register(pcr, PM_CTRL3, 0x01, 0x00);
+ }
+
/*
* If u_force_clkreq_0 is enabled, CLKREQ# PIN will be forced
* to drive low, and we forcibly request clock.
*/
if (option->force_clkreq_0)
- rtsx_pci_add_cmd(pcr, WRITE_REG_CMD, PETXCFG,
+ rtsx_pci_write_register(pcr, PETXCFG,
FORCE_CLKREQ_DELINK_MASK, FORCE_CLKREQ_LOW);
else
- rtsx_pci_add_cmd(pcr, WRITE_REG_CMD, PETXCFG,
+ rtsx_pci_write_register(pcr, PETXCFG,
FORCE_CLKREQ_DELINK_MASK, FORCE_CLKREQ_HIGH);

- return rtsx_pci_send_cmd(pcr, CMD_TIMEOUT_DEF);
+ rtsx_pci_write_register(pcr, pcr->reg_pm_ctrl3, 0x10, 0x00);
+ if (CHK_PCI_PID(pcr, PID_524A) || CHK_PCI_PID(pcr, PID_525A)) {
+ rtsx_pci_write_register(pcr, RTS524A_PME_FORCE_CTL,
+ REG_EFUSE_POWER_MASK, REG_EFUSE_POWEROFF);
+ pcr_dbg(pcr, "Power OFF efuse!");
+ }
+
+ return 0;
}

static int rts5249_optimize_phy(struct rtsx_pcr *pcr)
@@ -652,6 +789,8 @@ static int rts525a_extra_init_hw(struct rtsx_pcr *pcr)
{
rts5249_extra_init_hw(pcr);

+ rtsx_pci_write_register(pcr, RTS5250_CLK_CFG3, RTS525A_CFG_MEM_PD, RTS525A_CFG_MEM_PD);
+
rtsx_pci_write_register(pcr, PCLK_CTL, PCLK_MODE_SEL, PCLK_MODE_SEL);
if (is_version(pcr, 0x525A, IC_VER_A)) {
rtsx_pci_write_register(pcr, L1SUB_CONFIG2,
diff --git a/drivers/misc/cardreader/rts5260.c b/drivers/misc/cardreader/rts5260.c
index 897cfee350e7..080a7d67a8e1 100644
--- a/drivers/misc/cardreader/rts5260.c
+++ b/drivers/misc/cardreader/rts5260.c
@@ -26,21 +26,17 @@ static u8 rts5260_get_ic_version(struct rtsx_pcr *pcr)

static void rts5260_fill_driving(struct rtsx_pcr *pcr, u8 voltage)
{
- u8 driving_3v3[6][3] = {
- {0x94, 0x94, 0x94},
- {0x11, 0x11, 0x18},
- {0x55, 0x55, 0x5C},
- {0x94, 0x94, 0x94},
- {0x94, 0x94, 0x94},
- {0xFF, 0xFF, 0xFF},
+ u8 driving_3v3[4][3] = {
+ {0x11, 0x11, 0x11},
+ {0x22, 0x22, 0x22},
+ {0x55, 0x55, 0x55},
+ {0x33, 0x33, 0x33},
};
- u8 driving_1v8[6][3] = {
- {0x9A, 0x89, 0x89},
- {0xC4, 0xC4, 0xC4},
- {0x3C, 0x3C, 0x3C},
+ u8 driving_1v8[4][3] = {
+ {0x35, 0x33, 0x33},
+ {0x8A, 0x88, 0x88},
+ {0xBD, 0xBB, 0xBB},
{0x9B, 0x99, 0x99},
- {0x9A, 0x89, 0x89},
- {0xFE, 0xFE, 0xFE},
};
u8 (*driving)[3], drive_sel;

@@ -58,7 +54,7 @@ static void rts5260_fill_driving(struct rtsx_pcr *pcr, u8 voltage)
rtsx_pci_write_register(pcr, SD30_CMD_DRIVE_SEL,
0xFF, driving[drive_sel][1]);

- rtsx_pci_write_register(pcr, SD30_CMD_DRIVE_SEL,
+ rtsx_pci_write_register(pcr, SD30_DAT_DRIVE_SEL,
0xFF, driving[drive_sel][2]);
}

@@ -82,6 +78,8 @@ static void rtsx_base_fetch_vendor_settings(struct rtsx_pcr *pcr)

pci_read_config_dword(pdev, PCR_SETTING_REG2, &reg);
pcr_dbg(pcr, "Cfg 0x%x: 0x%x\n", PCR_SETTING_REG2, 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);
if (rtsx_reg_check_reverse_socket(reg))
pcr->flags |= PCR_REVERSE_SOCKET;
@@ -559,6 +557,8 @@ static int rts5260_extra_init_hw(struct rtsx_pcr *pcr)
rtsx_pci_write_register(pcr, PETXCFG,
FORCE_CLKREQ_DELINK_MASK, FORCE_CLKREQ_HIGH);

+ rtsx_pci_write_register(pcr, pcr->reg_pm_ctrl3, 0x10, 0x00);
+
return 0;
}

diff --git a/drivers/misc/cardreader/rtsx_pcr.h b/drivers/misc/cardreader/rtsx_pcr.h
index 6b322db8738e..fe5f4ca0f937 100644
--- a/drivers/misc/cardreader/rtsx_pcr.h
+++ b/drivers/misc/cardreader/rtsx_pcr.h
@@ -18,7 +18,24 @@
#define RTS522A_PM_CTRL3 0xFF7E

#define RTS524A_PME_FORCE_CTL 0xFF78
+#define REG_EFUSE_BYPASS 0x08
+#define REG_EFUSE_POR 0x04
+#define REG_EFUSE_POWER_MASK 0x03
+#define REG_EFUSE_POWERON 0x03
+#define REG_EFUSE_POWEROFF 0x00
+#define RTS5250_CLK_CFG3 0xFF79
+#define RTS525A_CFG_MEM_PD 0xF0
#define RTS524A_PM_CTRL3 0xFF7E
+#define RTS525A_BIOS_CFG 0xFF2D
+#define RTS525A_LOAD_BIOS_FLAG 0x01
+#define RTS525A_CLEAR_BIOS_FLAG 0x00
+
+#define RTS525A_EFUSE_CTL 0xFC32
+#define REG_EFUSE_ENABLE 0x80
+#define REG_EFUSE_MODE 0x40
+#define RTS525A_EFUSE_ADD 0xFC33
+#define REG_EFUSE_ADD_MASK 0x3F
+#define RTS525A_EFUSE_DATA 0xFC35

#define LTR_ACTIVE_LATENCY_DEF 0x883C
#define LTR_IDLE_LATENCY_DEF 0x892C
--
2.17.1

2020-09-08 22:31:54

by Bjorn Helgaas

Subject: Re: [PATCH v5 2/2] misc: rtsx: Add power saving functions and fix driving parameter

On Mon, Sep 07, 2020 at 06:07:31PM +0800, [email protected] wrote:
> From: Ricky Wu <[email protected]>
>
> v4:
> split power down flow and power saving function to two patch
>
> v5:
> fix up modified change under the --- line

Hehe, this came out *above* the "---" line :)

> Add rts522a L1 sub-state support
> Save more power on rts5227 rts5249 rts525a rts5260
> Fix rts5260 driving parameter
>
> Signed-off-by: Ricky Wu <[email protected]>
> ---
> drivers/misc/cardreader/rts5227.c | 112 +++++++++++++++++++++-
> drivers/misc/cardreader/rts5249.c | 145 ++++++++++++++++++++++++++++-
> drivers/misc/cardreader/rts5260.c | 28 +++---
> drivers/misc/cardreader/rtsx_pcr.h | 17 ++++
> 4 files changed, 283 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/misc/cardreader/rts5227.c b/drivers/misc/cardreader/rts5227.c
> index 747391e3fb5d..8859011672cb 100644
> --- a/drivers/misc/cardreader/rts5227.c
> +++ b/drivers/misc/cardreader/rts5227.c
> @@ -72,15 +72,80 @@ static void rts5227_fetch_vendor_settings(struct rtsx_pcr *pcr)
>
> pci_read_config_dword(pdev, PCR_SETTING_REG2, &reg);
> pcr_dbg(pcr, "Cfg 0x%x: 0x%x\n", PCR_SETTING_REG2, 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);
> if (rtsx_reg_check_reverse_socket(reg))
> pcr->flags |= PCR_REVERSE_SOCKET;
> }
>
> +static void rts5227_init_from_cfg(struct rtsx_pcr *pcr)
> +{
> + struct pci_dev *pdev = pcr->pci;
> + int l1ss;
> + u32 lval;
> + struct rtsx_cr_option *option = &pcr->option;
> +
> + l1ss = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_L1SS);
> + if (!l1ss)
> + return;
> +
> + pci_read_config_dword(pdev, l1ss + PCI_L1SS_CTL1, &lval);

This looks a little problematic. PCI_L1SS_CTL1 is an architected
register in the ASPM L1 PM Substates capability, and its value may
change at runtime because drivers/pci/pcie/aspm.c manages it.

It looks like the code below does device-specific configuration based
on the current PCI_L1SS_CTL1 value. But what happens if aspm.c
changes PCI_L1SS_CTL1 later?

> + if (CHK_PCI_PID(pcr, 0x522A)) {
> + if (0 == (lval & 0x0F))
> + rtsx_pci_enable_oobs_polling(pcr);
> + else
> + rtsx_pci_disable_oobs_polling(pcr);
> + }
> +
> + if (lval & PCI_L1SS_CTL1_ASPM_L1_1)
> + rtsx_set_dev_flag(pcr, ASPM_L1_1_EN);
> + else
> + rtsx_clear_dev_flag(pcr, ASPM_L1_1_EN);
> +
> + if (lval & PCI_L1SS_CTL1_ASPM_L1_2)
> + rtsx_set_dev_flag(pcr, ASPM_L1_2_EN);
> + else
> + rtsx_clear_dev_flag(pcr, ASPM_L1_2_EN);
> +
> + if (lval & PCI_L1SS_CTL1_PCIPM_L1_1)
> + rtsx_set_dev_flag(pcr, PM_L1_1_EN);
> + else
> + rtsx_clear_dev_flag(pcr, PM_L1_1_EN);
> +
> + if (lval & PCI_L1SS_CTL1_PCIPM_L1_2)
> + rtsx_set_dev_flag(pcr, PM_L1_2_EN);
> + else
> + rtsx_clear_dev_flag(pcr, PM_L1_2_EN);
> +
> + if (option->ltr_en) {
> + u16 val;
> +
> + pcie_capability_read_word(pcr->pci, PCI_EXP_DEVCTL2, &val);

Same thing here. I don't think the PCI core currently changes
PCI_EXP_DEVCTL2 after boot, but it's not a good idea to assume it's
going to be constant.

> + if (val & PCI_EXP_DEVCTL2_LTR_EN) {
> + option->ltr_enabled = true;
> + option->ltr_active = true;
> + rtsx_set_ltr_latency(pcr, option->ltr_active_latency);
> + } else {
> + option->ltr_enabled = false;
> + }
> + }
> +
> + if (rtsx_check_dev_flag(pcr, ASPM_L1_1_EN | ASPM_L1_2_EN
> + | PM_L1_1_EN | PM_L1_2_EN))
> + option->force_clkreq_0 = false;
> + else
> + option->force_clkreq_0 = true;
> +
> +}

2020-09-09 07:14:15

by Ricky Wu

Subject: RE: [PATCH v5 2/2] misc: rtsx: Add power saving functions and fix driving parameter

> -----Original Message-----
> From: Bjorn Helgaas [mailto:[email protected]]
> Sent: Wednesday, September 09, 2020 6:29 AM
> To: ?d???? Ricky
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]
> Subject: Re: [PATCH v5 2/2] misc: rtsx: Add power saving functions and fix driving
> parameter
>
> On Mon, Sep 07, 2020 at 06:07:31PM +0800, [email protected] wrote:
> > From: Ricky Wu <[email protected]>
> >
> > v4:
> > split power down flow and power saving function to two patch
> >
> > v5:
> > fix up modified change under the --- line
>
> Hehe, this came out *above* the "---" line :)
>
> > Add rts522a L1 sub-state support
> > Save more power on rts5227 rts5249 rts525a rts5260
> > Fix rts5260 driving parameter
> >
> > Signed-off-by: Ricky Wu <[email protected]>
> > ---
> > drivers/misc/cardreader/rts5227.c | 112 +++++++++++++++++++++-
> > drivers/misc/cardreader/rts5249.c | 145
> ++++++++++++++++++++++++++++-
> > drivers/misc/cardreader/rts5260.c | 28 +++---
> > drivers/misc/cardreader/rtsx_pcr.h | 17 ++++
> > 4 files changed, 283 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/misc/cardreader/rts5227.c
> b/drivers/misc/cardreader/rts5227.c
> > index 747391e3fb5d..8859011672cb 100644
> > --- a/drivers/misc/cardreader/rts5227.c
> > +++ b/drivers/misc/cardreader/rts5227.c
> > @@ -72,15 +72,80 @@ static void rts5227_fetch_vendor_settings(struct
> rtsx_pcr *pcr)
> >
> > pci_read_config_dword(pdev, PCR_SETTING_REG2, &reg);
> > pcr_dbg(pcr, "Cfg 0x%x: 0x%x\n", PCR_SETTING_REG2, 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);
> > if (rtsx_reg_check_reverse_socket(reg))
> > pcr->flags |= PCR_REVERSE_SOCKET;
> > }
> >
> > +static void rts5227_init_from_cfg(struct rtsx_pcr *pcr)
> > +{
> > + struct pci_dev *pdev = pcr->pci;
> > + int l1ss;
> > + u32 lval;
> > + struct rtsx_cr_option *option = &pcr->option;
> > +
> > + l1ss = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_L1SS);
> > + if (!l1ss)
> > + return;
> > +
> > + pci_read_config_dword(pdev, l1ss + PCI_L1SS_CTL1, &lval);
>
> This looks a little problematic. PCI_L1SS_CTL1 is an architected
> register in the ASPM L1 PM Substates capability, and its value may
> change at runtime because drivers/pci/pcie/aspm.c manages it.
>
> It looks like the code below does device-specific configuration based
> on the current PCI_L1SS_CTL1 value. But what happens if aspm.c
> changes PCI_L1SS_CTL1 later?
>

We are going to make sure and set the best configuration on the current time,
if host change the capability later, it doesn't affect function, only affect a little power saving

> > + if (CHK_PCI_PID(pcr, 0x522A)) {
> > + if (0 == (lval & 0x0F))
> > + rtsx_pci_enable_oobs_polling(pcr);
> > + else
> > + rtsx_pci_disable_oobs_polling(pcr);
> > + }
> > +
> > + if (lval & PCI_L1SS_CTL1_ASPM_L1_1)
> > + rtsx_set_dev_flag(pcr, ASPM_L1_1_EN);
> > + else
> > + rtsx_clear_dev_flag(pcr, ASPM_L1_1_EN);
> > +
> > + if (lval & PCI_L1SS_CTL1_ASPM_L1_2)
> > + rtsx_set_dev_flag(pcr, ASPM_L1_2_EN);
> > + else
> > + rtsx_clear_dev_flag(pcr, ASPM_L1_2_EN);
> > +
> > + if (lval & PCI_L1SS_CTL1_PCIPM_L1_1)
> > + rtsx_set_dev_flag(pcr, PM_L1_1_EN);
> > + else
> > + rtsx_clear_dev_flag(pcr, PM_L1_1_EN);
> > +
> > + if (lval & PCI_L1SS_CTL1_PCIPM_L1_2)
> > + rtsx_set_dev_flag(pcr, PM_L1_2_EN);
> > + else
> > + rtsx_clear_dev_flag(pcr, PM_L1_2_EN);
> > +
> > + if (option->ltr_en) {
> > + u16 val;
> > +
> > + pcie_capability_read_word(pcr->pci, PCI_EXP_DEVCTL2, &val);
>
> Same thing here. I don't think the PCI core currently changes
> PCI_EXP_DEVCTL2 after boot, but it's not a good idea to assume it's
> going to be constant.
>

The same reply

> > + if (val & PCI_EXP_DEVCTL2_LTR_EN) {
> > + option->ltr_enabled = true;
> > + option->ltr_active = true;
> > + rtsx_set_ltr_latency(pcr, option->ltr_active_latency);
> > + } else {
> > + option->ltr_enabled = false;
> > + }
> > + }
> > +
> > + if (rtsx_check_dev_flag(pcr, ASPM_L1_1_EN | ASPM_L1_2_EN
> > + | PM_L1_1_EN | PM_L1_2_EN))
> > + option->force_clkreq_0 = false;
> > + else
> > + option->force_clkreq_0 = true;
> > +
> > +}
>
> ------Please consider the environment before printing this e-mail.

2020-09-09 17:47:41

by Bjorn Helgaas

Subject: Re: [PATCH v5 2/2] misc: rtsx: Add power saving functions and fix driving parameter

On Wed, Sep 09, 2020 at 07:10:19AM +0000, 吳昊澄 Ricky wrote:
> > -----Original Message-----
> > From: Bjorn Helgaas [mailto:[email protected]]
> > Sent: Wednesday, September 09, 2020 6:29 AM
> > To: 吳昊澄 Ricky
> > Cc: [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected];
> > [email protected]
> > Subject: Re: [PATCH v5 2/2] misc: rtsx: Add power saving functions and fix driving
> > parameter
> >
> > On Mon, Sep 07, 2020 at 06:07:31PM +0800, [email protected] wrote:
> > > From: Ricky Wu <[email protected]>
> > >
> > > v4:
> > > split power down flow and power saving function to two patch
> > >
> > > v5:
> > > fix up modified change under the --- line
> >
> > Hehe, this came out *above* the "---" line :)
> >
> > > Add rts522a L1 sub-state support
> > > Save more power on rts5227 rts5249 rts525a rts5260
> > > Fix rts5260 driving parameter
> > >
> > > Signed-off-by: Ricky Wu <[email protected]>
> > > ---
> > > drivers/misc/cardreader/rts5227.c | 112 +++++++++++++++++++++-
> > > drivers/misc/cardreader/rts5249.c | 145
> > ++++++++++++++++++++++++++++-
> > > drivers/misc/cardreader/rts5260.c | 28 +++---
> > > drivers/misc/cardreader/rtsx_pcr.h | 17 ++++
> > > 4 files changed, 283 insertions(+), 19 deletions(-)
> > >
> > > diff --git a/drivers/misc/cardreader/rts5227.c
> > b/drivers/misc/cardreader/rts5227.c
> > > index 747391e3fb5d..8859011672cb 100644
> > > --- a/drivers/misc/cardreader/rts5227.c
> > > +++ b/drivers/misc/cardreader/rts5227.c
> > > @@ -72,15 +72,80 @@ static void rts5227_fetch_vendor_settings(struct
> > rtsx_pcr *pcr)
> > >
> > > pci_read_config_dword(pdev, PCR_SETTING_REG2, &reg);
> > > pcr_dbg(pcr, "Cfg 0x%x: 0x%x\n", PCR_SETTING_REG2, 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);
> > > if (rtsx_reg_check_reverse_socket(reg))
> > > pcr->flags |= PCR_REVERSE_SOCKET;
> > > }
> > >
> > > +static void rts5227_init_from_cfg(struct rtsx_pcr *pcr)
> > > +{
> > > + struct pci_dev *pdev = pcr->pci;
> > > + int l1ss;
> > > + u32 lval;
> > > + struct rtsx_cr_option *option = &pcr->option;
> > > +
> > > + l1ss = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_L1SS);
> > > + if (!l1ss)
> > > + return;
> > > +
> > > + pci_read_config_dword(pdev, l1ss + PCI_L1SS_CTL1, &lval);
> >
> > This looks a little problematic. PCI_L1SS_CTL1 is an architected
> > register in the ASPM L1 PM Substates capability, and its value may
> > change at runtime because drivers/pci/pcie/aspm.c manages it.
> >
> > It looks like the code below does device-specific configuration based
> > on the current PCI_L1SS_CTL1 value. But what happens if aspm.c
> > changes PCI_L1SS_CTL1 later?
>
> We are going to make sure and set the best configuration on the
> current time, if host change the capability later, it doesn't affect
> function, only affect a little power saving

Why don't you unconditionally do the following?

rtsx_set_dev_flag(pcr, ASPM_L1_1_EN);
rtsx_set_dev_flag(pcr, ASPM_L1_2_EN);
rtsx_set_dev_flag(pcr, PM_L1_1_EN);
rtsx_set_dev_flag(pcr, PM_L1_2_EN);

Let's assume the generic code in aspm.c has cleared all these bits:

PCI_L1SS_CTL1_ASPM_L1_1
PCI_L1SS_CTL1_ASPM_L1_2
PCI_L1SS_CTL1_PCIPM_L1_1
PCI_L1SS_CTL1_PCIPM_L1_2

in the L1 PM Substates capability.

I think you are saying that if you *also* clear ASPM_L1_1_EN,
ASPM_L1_2_EN, PM_L1_1_EN, and PM_L1_2_EN in your device-specific
registers, it uses less power than if you set those device-specific
bits. Right?

And moreover, I think you're saying that if aspm.c subsequently *sets*
some of those bits in the L1 PM Substates capability, those substates
*work* even though the device-specific ASPM_L1_1_EN, ASPM_L1_2_EN,
PM_L1_1_EN, and PM_L1_2_EN bits are not set. Right?

I do not feel good about this as a general strategy. I think we
should program the device so the behavior is completely predictable,
regardless of the generic enable bits happened to be set at
probe-time.

The current approach means that if we enable L1 substates after the
driver probe, the device is configured differently than if we enabled
L1 substates before probe. That's not a reliable way to operate it.

> > > + if (CHK_PCI_PID(pcr, 0x522A)) {
> > > + if (0 == (lval & 0x0F))
> > > + rtsx_pci_enable_oobs_polling(pcr);
> > > + else
> > > + rtsx_pci_disable_oobs_polling(pcr);
> > > + }
> > > +
> > > + if (lval & PCI_L1SS_CTL1_ASPM_L1_1)
> > > + rtsx_set_dev_flag(pcr, ASPM_L1_1_EN);
> > > + else
> > > + rtsx_clear_dev_flag(pcr, ASPM_L1_1_EN);
> > > +
> > > + if (lval & PCI_L1SS_CTL1_ASPM_L1_2)
> > > + rtsx_set_dev_flag(pcr, ASPM_L1_2_EN);
> > > + else
> > > + rtsx_clear_dev_flag(pcr, ASPM_L1_2_EN);
> > > +
> > > + if (lval & PCI_L1SS_CTL1_PCIPM_L1_1)
> > > + rtsx_set_dev_flag(pcr, PM_L1_1_EN);
> > > + else
> > > + rtsx_clear_dev_flag(pcr, PM_L1_1_EN);
> > > +
> > > + if (lval & PCI_L1SS_CTL1_PCIPM_L1_2)
> > > + rtsx_set_dev_flag(pcr, PM_L1_2_EN);
> > > + else
> > > + rtsx_clear_dev_flag(pcr, PM_L1_2_EN);
> > > +
> > > + if (option->ltr_en) {
> > > + u16 val;
> > > +
> > > + pcie_capability_read_word(pcr->pci, PCI_EXP_DEVCTL2, &val);
> >
> > Same thing here. I don't think the PCI core currently changes
> > PCI_EXP_DEVCTL2 after boot, but it's not a good idea to assume it's
> > going to be constant.
> >
>
> The same reply
>
> > > + if (val & PCI_EXP_DEVCTL2_LTR_EN) {
> > > + option->ltr_enabled = true;
> > > + option->ltr_active = true;
> > > + rtsx_set_ltr_latency(pcr, option->ltr_active_latency);
> > > + } else {
> > > + option->ltr_enabled = false;
> > > + }
> > > + }
> > > +
> > > + if (rtsx_check_dev_flag(pcr, ASPM_L1_1_EN | ASPM_L1_2_EN
> > > + | PM_L1_1_EN | PM_L1_2_EN))
> > > + option->force_clkreq_0 = false;
> > > + else
> > > + option->force_clkreq_0 = true;
> > > +
> > > +}
> >
> > ------Please consider the environment before printing this e-mail.

2020-09-11 08:22:25

by Ricky Wu

Subject: RE: [PATCH v5 2/2] misc: rtsx: Add power saving functions and fix driving parameter

> -----Original Message-----
> From: Bjorn Helgaas [mailto:[email protected]]
> Sent: Thursday, September 10, 2020 1:44 AM
> To: 吳昊澄 Ricky
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]
> Subject: Re: [PATCH v5 2/2] misc: rtsx: Add power saving functions and fix driving
> parameter
>
> On Wed, Sep 09, 2020 at 07:10:19AM +0000, 吳昊澄 Ricky wrote:
> > > -----Original Message-----
> > > From: Bjorn Helgaas [mailto:[email protected]]
> > > Sent: Wednesday, September 09, 2020 6:29 AM
> > > To: 吳昊澄 Ricky
> > > Cc: [email protected]; [email protected]; [email protected];
> > > [email protected]; [email protected];
> [email protected];
> > > [email protected]; [email protected];
> > > [email protected]
> > > Subject: Re: [PATCH v5 2/2] misc: rtsx: Add power saving functions and fix
> driving
> > > parameter
> > >
> > > On Mon, Sep 07, 2020 at 06:07:31PM +0800, [email protected] wrote:
> > > > From: Ricky Wu <[email protected]>
> > > >
> > > > v4:
> > > > split power down flow and power saving function to two patch
> > > >
> > > > v5:
> > > > fix up modified change under the --- line
> > >
> > > Hehe, this came out *above* the "---" line :)
> > >
> > > > Add rts522a L1 sub-state support
> > > > Save more power on rts5227 rts5249 rts525a rts5260
> > > > Fix rts5260 driving parameter
> > > >
> > > > Signed-off-by: Ricky Wu <[email protected]>
> > > > ---
> > > > drivers/misc/cardreader/rts5227.c | 112 +++++++++++++++++++++-
> > > > drivers/misc/cardreader/rts5249.c | 145
> > > ++++++++++++++++++++++++++++-
> > > > drivers/misc/cardreader/rts5260.c | 28 +++---
> > > > drivers/misc/cardreader/rtsx_pcr.h | 17 ++++
> > > > 4 files changed, 283 insertions(+), 19 deletions(-)
> > > >
> > > > diff --git a/drivers/misc/cardreader/rts5227.c
> > > b/drivers/misc/cardreader/rts5227.c
> > > > index 747391e3fb5d..8859011672cb 100644
> > > > --- a/drivers/misc/cardreader/rts5227.c
> > > > +++ b/drivers/misc/cardreader/rts5227.c
> > > > @@ -72,15 +72,80 @@ static void rts5227_fetch_vendor_settings(struct
> > > rtsx_pcr *pcr)
> > > >
> > > > pci_read_config_dword(pdev, PCR_SETTING_REG2, &reg);
> > > > pcr_dbg(pcr, "Cfg 0x%x: 0x%x\n", PCR_SETTING_REG2, 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);
> > > > if (rtsx_reg_check_reverse_socket(reg))
> > > > pcr->flags |= PCR_REVERSE_SOCKET;
> > > > }
> > > >
> > > > +static void rts5227_init_from_cfg(struct rtsx_pcr *pcr)
> > > > +{
> > > > + struct pci_dev *pdev = pcr->pci;
> > > > + int l1ss;
> > > > + u32 lval;
> > > > + struct rtsx_cr_option *option = &pcr->option;
> > > > +
> > > > + l1ss = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_L1SS);
> > > > + if (!l1ss)
> > > > + return;
> > > > +
> > > > + pci_read_config_dword(pdev, l1ss + PCI_L1SS_CTL1, &lval);
> > >
> > > This looks a little problematic. PCI_L1SS_CTL1 is an architected
> > > register in the ASPM L1 PM Substates capability, and its value may
> > > change at runtime because drivers/pci/pcie/aspm.c manages it.
> > >
> > > It looks like the code below does device-specific configuration based
> > > on the current PCI_L1SS_CTL1 value. But what happens if aspm.c
> > > changes PCI_L1SS_CTL1 later?
> >
> > We are going to make sure and set the best configuration on the
> > current time, if host change the capability later, it doesn't affect
> > function, only affect a little power saving
>
> Why don't you unconditionally do the following?
>
> rtsx_set_dev_flag(pcr, ASPM_L1_1_EN);
> rtsx_set_dev_flag(pcr, ASPM_L1_2_EN);
> rtsx_set_dev_flag(pcr, PM_L1_1_EN);
> rtsx_set_dev_flag(pcr, PM_L1_2_EN);
>

Our power saving function have 2 different flow L1 and L1substate, so we need to check it for which flow we are going to
Detail to see below reply

> Let's assume the generic code in aspm.c has cleared all these bits:
>
> PCI_L1SS_CTL1_ASPM_L1_1
> PCI_L1SS_CTL1_ASPM_L1_2
> PCI_L1SS_CTL1_PCIPM_L1_1
> PCI_L1SS_CTL1_PCIPM_L1_2
>
> in the L1 PM Substates capability.
>
> I think you are saying that if you *also* clear ASPM_L1_1_EN,
> ASPM_L1_2_EN, PM_L1_1_EN, and PM_L1_2_EN in your device-specific
> registers, it uses less power than if you set those device-specific
> bits. Right?
>
> And moreover, I think you're saying that if aspm.c subsequently *sets*
> some of those bits in the L1 PM Substates capability, those substates
> *work* even though the device-specific ASPM_L1_1_EN, ASPM_L1_2_EN,
> PM_L1_1_EN, and PM_L1_2_EN bits are not set. Right?
>
> I do not feel good about this as a general strategy. I think we
> should program the device so the behavior is completely predictable,
> regardless of the generic enable bits happened to be set at
> probe-time.
>
> The current approach means that if we enable L1 substates after the
> driver probe, the device is configured differently than if we enabled
> L1 substates before probe. That's not a reliable way to operate it.
>

Talk about our power saving function
a) basic L1 power saving
b) advance L1 power saving
c) advance L1 substate power saving

at initial , we check pci port support L1 subs or not, if not we are going to b) otherwise going to c).
Assume aspm.c change bit of L1 PM Substates capability after our driver probe, we are going to a)

So far we did not see any platform change L1 PM Substates capability after our driver probe.


> > > > + if (CHK_PCI_PID(pcr, 0x522A)) {
> > > > + if (0 == (lval & 0x0F))
> > > > + rtsx_pci_enable_oobs_polling(pcr);
> > > > + else
> > > > + rtsx_pci_disable_oobs_polling(pcr);
> > > > + }
> > > > +
> > > > + if (lval & PCI_L1SS_CTL1_ASPM_L1_1)
> > > > + rtsx_set_dev_flag(pcr, ASPM_L1_1_EN);
> > > > + else
> > > > + rtsx_clear_dev_flag(pcr, ASPM_L1_1_EN);
> > > > +
> > > > + if (lval & PCI_L1SS_CTL1_ASPM_L1_2)
> > > > + rtsx_set_dev_flag(pcr, ASPM_L1_2_EN);
> > > > + else
> > > > + rtsx_clear_dev_flag(pcr, ASPM_L1_2_EN);
> > > > +
> > > > + if (lval & PCI_L1SS_CTL1_PCIPM_L1_1)
> > > > + rtsx_set_dev_flag(pcr, PM_L1_1_EN);
> > > > + else
> > > > + rtsx_clear_dev_flag(pcr, PM_L1_1_EN);
> > > > +
> > > > + if (lval & PCI_L1SS_CTL1_PCIPM_L1_2)
> > > > + rtsx_set_dev_flag(pcr, PM_L1_2_EN);
> > > > + else
> > > > + rtsx_clear_dev_flag(pcr, PM_L1_2_EN);
> > > > +
> > > > + if (option->ltr_en) {
> > > > + u16 val;
> > > > +
> > > > + pcie_capability_read_word(pcr->pci, PCI_EXP_DEVCTL2, &val);
> > >
> > > Same thing here. I don't think the PCI core currently changes
> > > PCI_EXP_DEVCTL2 after boot, but it's not a good idea to assume it's
> > > going to be constant.
> > >
> >
> > The same reply
> >
> > > > + if (val & PCI_EXP_DEVCTL2_LTR_EN) {
> > > > + option->ltr_enabled = true;
> > > > + option->ltr_active = true;
> > > > + rtsx_set_ltr_latency(pcr, option->ltr_active_latency);
> > > > + } else {
> > > > + option->ltr_enabled = false;
> > > > + }
> > > > + }
> > > > +
> > > > + if (rtsx_check_dev_flag(pcr, ASPM_L1_1_EN | ASPM_L1_2_EN
> > > > + | PM_L1_1_EN | PM_L1_2_EN))
> > > > + option->force_clkreq_0 = false;
> > > > + else
> > > > + option->force_clkreq_0 = true;
> > > > +
> > > > +}
> > >
> > > ------Please consider the environment before printing this e-mail.

2020-09-11 16:58:14

by Bjorn Helgaas

Subject: Re: [PATCH v5 2/2] misc: rtsx: Add power saving functions and fix driving parameter

On Fri, Sep 11, 2020 at 08:18:22AM +0000, 吳昊澄 Ricky wrote:
> > -----Original Message-----
> > From: Bjorn Helgaas [mailto:[email protected]]
> > Sent: Thursday, September 10, 2020 1:44 AM
> > To: 吳昊澄 Ricky
> > Cc: [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected];
> > [email protected]
> > Subject: Re: [PATCH v5 2/2] misc: rtsx: Add power saving functions and fix driving
> > parameter
> >
> > On Wed, Sep 09, 2020 at 07:10:19AM +0000, 吳昊澄 Ricky wrote:
> > > > -----Original Message-----
> > > > From: Bjorn Helgaas [mailto:[email protected]]
> > > > Sent: Wednesday, September 09, 2020 6:29 AM
> > > > To: 吳昊澄 Ricky
> > > > Cc: [email protected]; [email protected]; [email protected];
> > > > [email protected]; [email protected];
> > [email protected];
> > > > [email protected]; [email protected];
> > > > [email protected]
> > > > Subject: Re: [PATCH v5 2/2] misc: rtsx: Add power saving functions and fix
> > driving
> > > > parameter
> > > >
> > > > On Mon, Sep 07, 2020 at 06:07:31PM +0800, [email protected] wrote:
> > > > > From: Ricky Wu <[email protected]>
> > > > >
> > > > > v4:
> > > > > split power down flow and power saving function to two patch
> > > > >
> > > > > v5:
> > > > > fix up modified change under the --- line
> > > >
> > > > Hehe, this came out *above* the "---" line :)
> > > >
> > > > > Add rts522a L1 sub-state support
> > > > > Save more power on rts5227 rts5249 rts525a rts5260
> > > > > Fix rts5260 driving parameter
> > > > >
> > > > > Signed-off-by: Ricky Wu <[email protected]>
> > > > > ---
> > > > > drivers/misc/cardreader/rts5227.c | 112 +++++++++++++++++++++-
> > > > > drivers/misc/cardreader/rts5249.c | 145
> > > > ++++++++++++++++++++++++++++-
> > > > > drivers/misc/cardreader/rts5260.c | 28 +++---
> > > > > drivers/misc/cardreader/rtsx_pcr.h | 17 ++++
> > > > > 4 files changed, 283 insertions(+), 19 deletions(-)
> > > > >
> > > > > diff --git a/drivers/misc/cardreader/rts5227.c
> > > > b/drivers/misc/cardreader/rts5227.c
> > > > > index 747391e3fb5d..8859011672cb 100644
> > > > > --- a/drivers/misc/cardreader/rts5227.c
> > > > > +++ b/drivers/misc/cardreader/rts5227.c
> > > > > @@ -72,15 +72,80 @@ static void rts5227_fetch_vendor_settings(struct
> > > > rtsx_pcr *pcr)
> > > > >
> > > > > pci_read_config_dword(pdev, PCR_SETTING_REG2, &reg);
> > > > > pcr_dbg(pcr, "Cfg 0x%x: 0x%x\n", PCR_SETTING_REG2, 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);
> > > > > if (rtsx_reg_check_reverse_socket(reg))
> > > > > pcr->flags |= PCR_REVERSE_SOCKET;
> > > > > }
> > > > >
> > > > > +static void rts5227_init_from_cfg(struct rtsx_pcr *pcr)
> > > > > +{
> > > > > + struct pci_dev *pdev = pcr->pci;
> > > > > + int l1ss;
> > > > > + u32 lval;
> > > > > + struct rtsx_cr_option *option = &pcr->option;
> > > > > +
> > > > > + l1ss = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_L1SS);
> > > > > + if (!l1ss)
> > > > > + return;
> > > > > +
> > > > > + pci_read_config_dword(pdev, l1ss + PCI_L1SS_CTL1, &lval);
> > > >
> > > > This looks a little problematic. PCI_L1SS_CTL1 is an architected
> > > > register in the ASPM L1 PM Substates capability, and its value may
> > > > change at runtime because drivers/pci/pcie/aspm.c manages it.
> > > >
> > > > It looks like the code below does device-specific configuration based
> > > > on the current PCI_L1SS_CTL1 value. But what happens if aspm.c
> > > > changes PCI_L1SS_CTL1 later?
> > >
> > > We are going to make sure and set the best configuration on the
> > > current time, if host change the capability later, it doesn't affect
> > > function, only affect a little power saving
> >
> > Why don't you unconditionally do the following?
> >
> > rtsx_set_dev_flag(pcr, ASPM_L1_1_EN);
> > rtsx_set_dev_flag(pcr, ASPM_L1_2_EN);
> > rtsx_set_dev_flag(pcr, PM_L1_1_EN);
> > rtsx_set_dev_flag(pcr, PM_L1_2_EN);
>
> Our power saving function have 2 different flow L1 and L1substate,
> so we need to check it for which flow we are going to
> Detail to see below reply
>
> > Let's assume the generic code in aspm.c has cleared all these bits:
> >
> > PCI_L1SS_CTL1_ASPM_L1_1
> > PCI_L1SS_CTL1_ASPM_L1_2
> > PCI_L1SS_CTL1_PCIPM_L1_1
> > PCI_L1SS_CTL1_PCIPM_L1_2
> >
> > in the L1 PM Substates capability.
> >
> > I think you are saying that if you *also* clear ASPM_L1_1_EN,
> > ASPM_L1_2_EN, PM_L1_1_EN, and PM_L1_2_EN in your device-specific
> > registers, it uses less power than if you set those device-specific
> > bits. Right?
> >
> > And moreover, I think you're saying that if aspm.c subsequently *sets*
> > some of those bits in the L1 PM Substates capability, those substates
> > *work* even though the device-specific ASPM_L1_1_EN, ASPM_L1_2_EN,
> > PM_L1_1_EN, and PM_L1_2_EN bits are not set. Right?
> >
> > I do not feel good about this as a general strategy. I think we
> > should program the device so the behavior is completely predictable,
> > regardless of the generic enable bits happened to be set at
> > probe-time.
> >
> > The current approach means that if we enable L1 substates after the
> > driver probe, the device is configured differently than if we enabled
> > L1 substates before probe. That's not a reliable way to operate it.
>
> Talk about our power saving function
> a) basic L1 power saving
> b) advance L1 power saving
> c) advance L1 substate power saving

I have no idea what the difference between "basic L1 power saving" and
"advance L1 power saving" is, so I assume those are device-specific
things. If not, please use the same terminology as the PCIe spec.

> at initial, we check pci port support L1 subs or not, if not we are
> going to b) otherwise going to c).

You're not checking whether the port *supports* L1 substates. You
would look at PCI_L1SS_CAP to learn that. You're looking at
PCI_L1SS_CTL1, which tells you whether L1 substates are *enabled*.

> Assume aspm.c change bit of L1 PM Substates capability after our
> driver probe, we are going to a)
>
> So far we did not see any platform change L1 PM Substates capability
> after our driver probe.

You should expect that aspm.c will change bits in the L1 PM *control*
register (PCI_L1SS_CTL1) after probe.

You might not actually see it change, depending on how you tested, but
you cannot rely on PCI_L1SS_CTL1 being constant. It may change based
on power/performance tradeoffs, e.g., whether the system is plugged
into AC power, whether it's idle, etc.

> > > > > + if (CHK_PCI_PID(pcr, 0x522A)) {
> > > > > + if (0 == (lval & 0x0F))
> > > > > + rtsx_pci_enable_oobs_polling(pcr);
> > > > > + else
> > > > > + rtsx_pci_disable_oobs_polling(pcr);
> > > > > + }
> > > > > +
> > > > > + if (lval & PCI_L1SS_CTL1_ASPM_L1_1)
> > > > > + rtsx_set_dev_flag(pcr, ASPM_L1_1_EN);
> > > > > + else
> > > > > + rtsx_clear_dev_flag(pcr, ASPM_L1_1_EN);
> > > > > +
> > > > > + if (lval & PCI_L1SS_CTL1_ASPM_L1_2)
> > > > > + rtsx_set_dev_flag(pcr, ASPM_L1_2_EN);
> > > > > + else
> > > > > + rtsx_clear_dev_flag(pcr, ASPM_L1_2_EN);
> > > > > +
> > > > > + if (lval & PCI_L1SS_CTL1_PCIPM_L1_1)
> > > > > + rtsx_set_dev_flag(pcr, PM_L1_1_EN);
> > > > > + else
> > > > > + rtsx_clear_dev_flag(pcr, PM_L1_1_EN);
> > > > > +
> > > > > + if (lval & PCI_L1SS_CTL1_PCIPM_L1_2)
> > > > > + rtsx_set_dev_flag(pcr, PM_L1_2_EN);
> > > > > + else
> > > > > + rtsx_clear_dev_flag(pcr, PM_L1_2_EN);
> > > > > +
> > > > > + if (option->ltr_en) {
> > > > > + u16 val;
> > > > > +
> > > > > + pcie_capability_read_word(pcr->pci, PCI_EXP_DEVCTL2, &val);
> > > >
> > > > Same thing here. I don't think the PCI core currently changes
> > > > PCI_EXP_DEVCTL2 after boot, but it's not a good idea to assume it's
> > > > going to be constant.
> > > >
> > >
> > > The same reply
> > >
> > > > > + if (val & PCI_EXP_DEVCTL2_LTR_EN) {
> > > > > + option->ltr_enabled = true;
> > > > > + option->ltr_active = true;
> > > > > + rtsx_set_ltr_latency(pcr, option->ltr_active_latency);
> > > > > + } else {
> > > > > + option->ltr_enabled = false;
> > > > > + }
> > > > > + }
> > > > > +
> > > > > + if (rtsx_check_dev_flag(pcr, ASPM_L1_1_EN | ASPM_L1_2_EN
> > > > > + | PM_L1_1_EN | PM_L1_2_EN))
> > > > > + option->force_clkreq_0 = false;
> > > > > + else
> > > > > + option->force_clkreq_0 = true;
> > > > > +
> > > > > +}
> > > >
> > > > ------Please consider the environment before printing this e-mail.

2020-09-15 08:29:17

by Ricky Wu

Subject: RE: [PATCH v5 2/2] misc: rtsx: Add power saving functions and fix driving parameter

> -----Original Message-----
> From: Bjorn Helgaas [mailto:[email protected]]
> Sent: Friday, September 11, 2020 10:56 PM
> To: 吳昊澄 Ricky
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]
> Subject: Re: [PATCH v5 2/2] misc: rtsx: Add power saving functions and fix driving
> parameter
>
> On Fri, Sep 11, 2020 at 08:18:22AM +0000, 吳昊澄 Ricky wrote:
> > > -----Original Message-----
> > > From: Bjorn Helgaas [mailto:[email protected]]
> > > Sent: Thursday, September 10, 2020 1:44 AM
> > > To: 吳昊澄 Ricky
> > > Cc: [email protected]; [email protected]; [email protected];
> > > [email protected]; [email protected];
> [email protected];
> > > [email protected]; [email protected];
> > > [email protected]
> > > Subject: Re: [PATCH v5 2/2] misc: rtsx: Add power saving functions and fix
> driving
> > > parameter
> > >
> > > On Wed, Sep 09, 2020 at 07:10:19AM +0000, 吳昊澄 Ricky wrote:
> > > > > -----Original Message-----
> > > > > From: Bjorn Helgaas [mailto:[email protected]]
> > > > > Sent: Wednesday, September 09, 2020 6:29 AM
> > > > > To: 吳昊澄 Ricky
> > > > > Cc: [email protected]; [email protected];
> [email protected];
> > > > > [email protected]; [email protected];
> > > [email protected];
> > > > > [email protected]; [email protected];
> > > > > [email protected]
> > > > > Subject: Re: [PATCH v5 2/2] misc: rtsx: Add power saving functions and fix
> > > driving
> > > > > parameter
> > > > >
> > > > > On Mon, Sep 07, 2020 at 06:07:31PM +0800, [email protected]
> wrote:
> > > > > > From: Ricky Wu <[email protected]>
> > > > > >
> > > > > > v4:
> > > > > > split power down flow and power saving function to two patch
> > > > > >
> > > > > > v5:
> > > > > > fix up modified change under the --- line
> > > > >
> > > > > Hehe, this came out *above* the "---" line :)
> > > > >
> > > > > > Add rts522a L1 sub-state support
> > > > > > Save more power on rts5227 rts5249 rts525a rts5260
> > > > > > Fix rts5260 driving parameter
> > > > > >
> > > > > > Signed-off-by: Ricky Wu <[email protected]>
> > > > > > ---
> > > > > > drivers/misc/cardreader/rts5227.c | 112 +++++++++++++++++++++-
> > > > > > drivers/misc/cardreader/rts5249.c | 145
> > > > > ++++++++++++++++++++++++++++-
> > > > > > drivers/misc/cardreader/rts5260.c | 28 +++---
> > > > > > drivers/misc/cardreader/rtsx_pcr.h | 17 ++++
> > > > > > 4 files changed, 283 insertions(+), 19 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/misc/cardreader/rts5227.c
> > > > > b/drivers/misc/cardreader/rts5227.c
> > > > > > index 747391e3fb5d..8859011672cb 100644
> > > > > > --- a/drivers/misc/cardreader/rts5227.c
> > > > > > +++ b/drivers/misc/cardreader/rts5227.c
> > > > > > @@ -72,15 +72,80 @@ static void
> rts5227_fetch_vendor_settings(struct
> > > > > rtsx_pcr *pcr)
> > > > > >
> > > > > > pci_read_config_dword(pdev, PCR_SETTING_REG2, &reg);
> > > > > > pcr_dbg(pcr, "Cfg 0x%x: 0x%x\n", PCR_SETTING_REG2, 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);
> > > > > > if (rtsx_reg_check_reverse_socket(reg))
> > > > > > pcr->flags |= PCR_REVERSE_SOCKET;
> > > > > > }
> > > > > >
> > > > > > +static void rts5227_init_from_cfg(struct rtsx_pcr *pcr)
> > > > > > +{
> > > > > > + struct pci_dev *pdev = pcr->pci;
> > > > > > + int l1ss;
> > > > > > + u32 lval;
> > > > > > + struct rtsx_cr_option *option = &pcr->option;
> > > > > > +
> > > > > > + l1ss = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_L1SS);
> > > > > > + if (!l1ss)
> > > > > > + return;
> > > > > > +
> > > > > > + pci_read_config_dword(pdev, l1ss + PCI_L1SS_CTL1, &lval);
> > > > >
> > > > > This looks a little problematic. PCI_L1SS_CTL1 is an architected
> > > > > register in the ASPM L1 PM Substates capability, and its value may
> > > > > change at runtime because drivers/pci/pcie/aspm.c manages it.
> > > > >
> > > > > It looks like the code below does device-specific configuration based
> > > > > on the current PCI_L1SS_CTL1 value. But what happens if aspm.c
> > > > > changes PCI_L1SS_CTL1 later?
> > > >
> > > > We are going to make sure and set the best configuration on the
> > > > current time, if host change the capability later, it doesn't affect
> > > > function, only affect a little power saving
> > >
> > > Why don't you unconditionally do the following?
> > >
> > > rtsx_set_dev_flag(pcr, ASPM_L1_1_EN);
> > > rtsx_set_dev_flag(pcr, ASPM_L1_2_EN);
> > > rtsx_set_dev_flag(pcr, PM_L1_1_EN);
> > > rtsx_set_dev_flag(pcr, PM_L1_2_EN);
> >
> > Our power saving function have 2 different flow L1 and L1substate,
> > so we need to check it for which flow we are going to
> > Detail to see below reply
> >
> > > Let's assume the generic code in aspm.c has cleared all these bits:
> > >
> > > PCI_L1SS_CTL1_ASPM_L1_1
> > > PCI_L1SS_CTL1_ASPM_L1_2
> > > PCI_L1SS_CTL1_PCIPM_L1_1
> > > PCI_L1SS_CTL1_PCIPM_L1_2
> > >
> > > in the L1 PM Substates capability.
> > >
> > > I think you are saying that if you *also* clear ASPM_L1_1_EN,
> > > ASPM_L1_2_EN, PM_L1_1_EN, and PM_L1_2_EN in your device-specific
> > > registers, it uses less power than if you set those device-specific
> > > bits. Right?
> > >
> > > And moreover, I think you're saying that if aspm.c subsequently *sets*
> > > some of those bits in the L1 PM Substates capability, those substates
> > > *work* even though the device-specific ASPM_L1_1_EN, ASPM_L1_2_EN,
> > > PM_L1_1_EN, and PM_L1_2_EN bits are not set. Right?
> > >
> > > I do not feel good about this as a general strategy. I think we
> > > should program the device so the behavior is completely predictable,
> > > regardless of the generic enable bits happened to be set at
> > > probe-time.
> > >
> > > The current approach means that if we enable L1 substates after the
> > > driver probe, the device is configured differently than if we enabled
> > > L1 substates before probe. That's not a reliable way to operate it.
> >
> > Talk about our power saving function
> > a) basic L1 power saving
> > b) advance L1 power saving
> > c) advance L1 substate power saving
>
> I have no idea what the difference between "basic L1 power saving" and
> "advance L1 power saving" is, so I assume those are device-specific
> things. If not, please use the same terminology as the PCIe spec.
>
> > at initial, we check pci port support L1 subs or not, if not we are
> > going to b) otherwise going to c).
>
> You're not checking whether the port *supports* L1 substates. You
> would look at PCI_L1SS_CAP to learn that. You're looking at
> PCI_L1SS_CTL1, which tells you whether L1 substates are *enabled*.
>
> > Assume aspm.c change bit of L1 PM Substates capability after our
> > driver probe, we are going to a)
> >
> > So far we did not see any platform change L1 PM Substates capability
> > after our driver probe.
>
> You should expect that aspm.c will change bits in the L1 PM *control*
> register (PCI_L1SS_CTL1) after probe.
>
> You might not actually see it change, depending on how you tested, but
> you cannot rely on PCI_L1SS_CTL1 being constant. It may change based
> on power/performance tradeoffs, e.g., whether the system is plugged
> into AC power, whether it's idle, etc.
>

Our ASPM function is a HW solution follow the PCIe SPEC. don’t worry about crash the system
If HOST change our device config space setting at run time our HW will do the corresponding things which follows the SPEC.
At initial time we set these parameter just good for more power saving

> > > > > > + if (CHK_PCI_PID(pcr, 0x522A)) {
> > > > > > + if (0 == (lval & 0x0F))
> > > > > > + rtsx_pci_enable_oobs_polling(pcr);
> > > > > > + else
> > > > > > + rtsx_pci_disable_oobs_polling(pcr);
> > > > > > + }
> > > > > > +
> > > > > > + if (lval & PCI_L1SS_CTL1_ASPM_L1_1)
> > > > > > + rtsx_set_dev_flag(pcr, ASPM_L1_1_EN);
> > > > > > + else
> > > > > > + rtsx_clear_dev_flag(pcr, ASPM_L1_1_EN);
> > > > > > +
> > > > > > + if (lval & PCI_L1SS_CTL1_ASPM_L1_2)
> > > > > > + rtsx_set_dev_flag(pcr, ASPM_L1_2_EN);
> > > > > > + else
> > > > > > + rtsx_clear_dev_flag(pcr, ASPM_L1_2_EN);
> > > > > > +
> > > > > > + if (lval & PCI_L1SS_CTL1_PCIPM_L1_1)
> > > > > > + rtsx_set_dev_flag(pcr, PM_L1_1_EN);
> > > > > > + else
> > > > > > + rtsx_clear_dev_flag(pcr, PM_L1_1_EN);
> > > > > > +
> > > > > > + if (lval & PCI_L1SS_CTL1_PCIPM_L1_2)
> > > > > > + rtsx_set_dev_flag(pcr, PM_L1_2_EN);
> > > > > > + else
> > > > > > + rtsx_clear_dev_flag(pcr, PM_L1_2_EN);
> > > > > > +
> > > > > > + if (option->ltr_en) {
> > > > > > + u16 val;
> > > > > > +
> > > > > > + pcie_capability_read_word(pcr->pci, PCI_EXP_DEVCTL2,
> &val);
> > > > >
> > > > > Same thing here. I don't think the PCI core currently changes
> > > > > PCI_EXP_DEVCTL2 after boot, but it's not a good idea to assume it's
> > > > > going to be constant.
> > > > >
> > > >
> > > > The same reply
> > > >
> > > > > > + if (val & PCI_EXP_DEVCTL2_LTR_EN) {
> > > > > > + option->ltr_enabled = true;
> > > > > > + option->ltr_active = true;
> > > > > > + rtsx_set_ltr_latency(pcr, option->ltr_active_latency);
> > > > > > + } else {
> > > > > > + option->ltr_enabled = false;
> > > > > > + }
> > > > > > + }
> > > > > > +
> > > > > > + if (rtsx_check_dev_flag(pcr, ASPM_L1_1_EN | ASPM_L1_2_EN
> > > > > > + | PM_L1_1_EN | PM_L1_2_EN))
> > > > > > + option->force_clkreq_0 = false;
> > > > > > + else
> > > > > > + option->force_clkreq_0 = true;
> > > > > > +
> > > > > > +}
> > > > >
> > > > > ------Please consider the environment before printing this e-mail.

2020-09-15 16:24:16

by Bjorn Helgaas

Subject: Re: [PATCH v5 2/2] misc: rtsx: Add power saving functions and fix driving parameter

On Tue, Sep 15, 2020 at 08:24:50AM +0000, 吳昊澄 Ricky wrote:
> > -----Original Message-----
> > From: Bjorn Helgaas [mailto:[email protected]]
> > Sent: Friday, September 11, 2020 10:56 PM
> > To: 吳昊澄 Ricky
> > Cc: [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected];
> > [email protected]
> > Subject: Re: [PATCH v5 2/2] misc: rtsx: Add power saving functions and fix driving
> > parameter
> >
> > On Fri, Sep 11, 2020 at 08:18:22AM +0000, 吳昊澄 Ricky wrote:
> > > > -----Original Message-----
> > > > From: Bjorn Helgaas [mailto:[email protected]]
> > > > Sent: Thursday, September 10, 2020 1:44 AM
> > > > To: 吳昊澄 Ricky
> > > > Cc: [email protected]; [email protected]; [email protected];
> > > > [email protected]; [email protected];
> > [email protected];
> > > > [email protected]; [email protected];
> > > > [email protected]
> > > > Subject: Re: [PATCH v5 2/2] misc: rtsx: Add power saving functions and fix
> > driving
> > > > parameter
> > > >
> > > > On Wed, Sep 09, 2020 at 07:10:19AM +0000, 吳昊澄 Ricky wrote:
> > > > > > -----Original Message-----
> > > > > > From: Bjorn Helgaas [mailto:[email protected]]
> > > > > > Sent: Wednesday, September 09, 2020 6:29 AM
> > > > > > To: 吳昊澄 Ricky
> > > > > > Cc: [email protected]; [email protected];
> > [email protected];
> > > > > > [email protected]; [email protected];
> > > > [email protected];
> > > > > > [email protected]; [email protected];
> > > > > > [email protected]
> > > > > > Subject: Re: [PATCH v5 2/2] misc: rtsx: Add power saving functions and fix
> > > > driving
> > > > > > parameter
> > > > > >
> > > > > > On Mon, Sep 07, 2020 at 06:07:31PM +0800, [email protected]
> > wrote:
> > > > > > > From: Ricky Wu <[email protected]>
> > > > > > >
> > > > > > > v4:
> > > > > > > split power down flow and power saving function to two patch
> > > > > > >
> > > > > > > v5:
> > > > > > > fix up modified change under the --- line
> > > > > >
> > > > > > Hehe, this came out *above* the "---" line :)
> > > > > >
> > > > > > > Add rts522a L1 sub-state support
> > > > > > > Save more power on rts5227 rts5249 rts525a rts5260
> > > > > > > Fix rts5260 driving parameter
> > > > > > >
> > > > > > > Signed-off-by: Ricky Wu <[email protected]>
> > > > > > > ---
> > > > > > > drivers/misc/cardreader/rts5227.c | 112 +++++++++++++++++++++-
> > > > > > > drivers/misc/cardreader/rts5249.c | 145
> > > > > > ++++++++++++++++++++++++++++-
> > > > > > > drivers/misc/cardreader/rts5260.c | 28 +++---
> > > > > > > drivers/misc/cardreader/rtsx_pcr.h | 17 ++++
> > > > > > > 4 files changed, 283 insertions(+), 19 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/misc/cardreader/rts5227.c
> > > > > > b/drivers/misc/cardreader/rts5227.c
> > > > > > > index 747391e3fb5d..8859011672cb 100644
> > > > > > > --- a/drivers/misc/cardreader/rts5227.c
> > > > > > > +++ b/drivers/misc/cardreader/rts5227.c
> > > > > > > @@ -72,15 +72,80 @@ static void
> > rts5227_fetch_vendor_settings(struct
> > > > > > rtsx_pcr *pcr)
> > > > > > >
> > > > > > > pci_read_config_dword(pdev, PCR_SETTING_REG2, &reg);
> > > > > > > pcr_dbg(pcr, "Cfg 0x%x: 0x%x\n", PCR_SETTING_REG2, 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);
> > > > > > > if (rtsx_reg_check_reverse_socket(reg))
> > > > > > > pcr->flags |= PCR_REVERSE_SOCKET;
> > > > > > > }
> > > > > > >
> > > > > > > +static void rts5227_init_from_cfg(struct rtsx_pcr *pcr)
> > > > > > > +{
> > > > > > > + struct pci_dev *pdev = pcr->pci;
> > > > > > > + int l1ss;
> > > > > > > + u32 lval;
> > > > > > > + struct rtsx_cr_option *option = &pcr->option;
> > > > > > > +
> > > > > > > + l1ss = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_L1SS);
> > > > > > > + if (!l1ss)
> > > > > > > + return;
> > > > > > > +
> > > > > > > + pci_read_config_dword(pdev, l1ss + PCI_L1SS_CTL1, &lval);
> > > > > >
> > > > > > This looks a little problematic. PCI_L1SS_CTL1 is an architected
> > > > > > register in the ASPM L1 PM Substates capability, and its value may
> > > > > > change at runtime because drivers/pci/pcie/aspm.c manages it.
> > > > > >
> > > > > > It looks like the code below does device-specific configuration based
> > > > > > on the current PCI_L1SS_CTL1 value. But what happens if aspm.c
> > > > > > changes PCI_L1SS_CTL1 later?
> > > > >
> > > > > We are going to make sure and set the best configuration on the
> > > > > current time, if host change the capability later, it doesn't affect
> > > > > function, only affect a little power saving
> > > >
> > > > Why don't you unconditionally do the following?
> > > >
> > > > rtsx_set_dev_flag(pcr, ASPM_L1_1_EN);
> > > > rtsx_set_dev_flag(pcr, ASPM_L1_2_EN);
> > > > rtsx_set_dev_flag(pcr, PM_L1_1_EN);
> > > > rtsx_set_dev_flag(pcr, PM_L1_2_EN);
> > >
> > > Our power saving function have 2 different flow L1 and L1substate,
> > > so we need to check it for which flow we are going to
> > > Detail to see below reply
> > >
> > > > Let's assume the generic code in aspm.c has cleared all these bits:
> > > >
> > > > PCI_L1SS_CTL1_ASPM_L1_1
> > > > PCI_L1SS_CTL1_ASPM_L1_2
> > > > PCI_L1SS_CTL1_PCIPM_L1_1
> > > > PCI_L1SS_CTL1_PCIPM_L1_2
> > > >
> > > > in the L1 PM Substates capability.
> > > >
> > > > I think you are saying that if you *also* clear ASPM_L1_1_EN,
> > > > ASPM_L1_2_EN, PM_L1_1_EN, and PM_L1_2_EN in your device-specific
> > > > registers, it uses less power than if you set those device-specific
> > > > bits. Right?
> > > >
> > > > And moreover, I think you're saying that if aspm.c subsequently *sets*
> > > > some of those bits in the L1 PM Substates capability, those substates
> > > > *work* even though the device-specific ASPM_L1_1_EN, ASPM_L1_2_EN,
> > > > PM_L1_1_EN, and PM_L1_2_EN bits are not set. Right?
> > > >
> > > > I do not feel good about this as a general strategy. I think we
> > > > should program the device so the behavior is completely predictable,
> > > > regardless of the generic enable bits happened to be set at
> > > > probe-time.
> > > >
> > > > The current approach means that if we enable L1 substates after the
> > > > driver probe, the device is configured differently than if we enabled
> > > > L1 substates before probe. That's not a reliable way to operate it.
> > >
> > > Talk about our power saving function
> > > a) basic L1 power saving
> > > b) advance L1 power saving
> > > c) advance L1 substate power saving
> >
> > I have no idea what the difference between "basic L1 power saving" and
> > "advance L1 power saving" is, so I assume those are device-specific
> > things. If not, please use the same terminology as the PCIe spec.
> >
> > > at initial, we check pci port support L1 subs or not, if not we are
> > > going to b) otherwise going to c).
> >
> > You're not checking whether the port *supports* L1 substates. You
> > would look at PCI_L1SS_CAP to learn that. You're looking at
> > PCI_L1SS_CTL1, which tells you whether L1 substates are *enabled*.
> >
> > > Assume aspm.c change bit of L1 PM Substates capability after our
> > > driver probe, we are going to a)
> > >
> > > So far we did not see any platform change L1 PM Substates capability
> > > after our driver probe.
> >
> > You should expect that aspm.c will change bits in the L1 PM *control*
> > register (PCI_L1SS_CTL1) after probe.
> >
> > You might not actually see it change, depending on how you tested, but
> > you cannot rely on PCI_L1SS_CTL1 being constant. It may change based
> > on power/performance tradeoffs, e.g., whether the system is plugged
> > into AC power, whether it's idle, etc.
> >
>
> Our ASPM function is a HW solution follow the PCIe SPEC. don’t worry
> about crash the system If HOST change our device config space
> setting at run time our HW will do the corresponding things which
> follows the SPEC. At initial time we set these parameter just good
> for more power saving

OK. It would be better if your hardware would notice the
PCI_L1SS_CTL1 change and set its own device-specific power-saving
parameters. The drivers would be simpler, and the device behavior
would be more consistent.

Drivers *writing* to standard PCI config things (64-byte config header
or Capabilities like PCIe, PM, ASPM, L1 Substates) is a definite
problem because the PCI core owns those and writes from drivers need
to be mediated somehow. AFAICT your drivers don't write to these
things.

Drivers *reading* these things (as your drivers do) shouldn't cause
problems, but it does violate the interface abstractions that the PCI
core should provide.

Bjorn

2020-09-16 17:46:26

by Greg Kroah-Hartman

Subject: Re: [PATCH v5 2/2] misc: rtsx: Add power saving functions and fix driving parameter

On Tue, Sep 15, 2020 at 10:28:11AM -0500, Bjorn Helgaas wrote:
> On Tue, Sep 15, 2020 at 08:24:50AM +0000, 吳昊澄 Ricky wrote:
> > > -----Original Message-----
> > > From: Bjorn Helgaas [mailto:[email protected]]
> > > Sent: Friday, September 11, 2020 10:56 PM
> > > To: 吳昊澄 Ricky
> > > Cc: [email protected]; [email protected]; [email protected];
> > > [email protected]; [email protected]; [email protected];
> > > [email protected]; [email protected];
> > > [email protected]
> > > Subject: Re: [PATCH v5 2/2] misc: rtsx: Add power saving functions and fix driving
> > > parameter
> > >
> > > On Fri, Sep 11, 2020 at 08:18:22AM +0000, 吳昊澄 Ricky wrote:
> > > > > -----Original Message-----
> > > > > From: Bjorn Helgaas [mailto:[email protected]]
> > > > > Sent: Thursday, September 10, 2020 1:44 AM
> > > > > To: 吳昊澄 Ricky
> > > > > Cc: [email protected]; [email protected]; [email protected];
> > > > > [email protected]; [email protected];
> > > [email protected];
> > > > > [email protected]; [email protected];
> > > > > [email protected]
> > > > > Subject: Re: [PATCH v5 2/2] misc: rtsx: Add power saving functions and fix
> > > driving
> > > > > parameter
> > > > >
> > > > > On Wed, Sep 09, 2020 at 07:10:19AM +0000, 吳昊澄 Ricky wrote:
> > > > > > > -----Original Message-----
> > > > > > > From: Bjorn Helgaas [mailto:[email protected]]
> > > > > > > Sent: Wednesday, September 09, 2020 6:29 AM
> > > > > > > To: 吳昊澄 Ricky
> > > > > > > Cc: [email protected]; [email protected];
> > > [email protected];
> > > > > > > [email protected]; [email protected];
> > > > > [email protected];
> > > > > > > [email protected]; [email protected];
> > > > > > > [email protected]
> > > > > > > Subject: Re: [PATCH v5 2/2] misc: rtsx: Add power saving functions and fix
> > > > > driving
> > > > > > > parameter
> > > > > > >
> > > > > > > On Mon, Sep 07, 2020 at 06:07:31PM +0800, [email protected]
> > > wrote:
> > > > > > > > From: Ricky Wu <[email protected]>
> > > > > > > >
> > > > > > > > v4:
> > > > > > > > split power down flow and power saving function to two patch
> > > > > > > >
> > > > > > > > v5:
> > > > > > > > fix up modified change under the --- line
> > > > > > >
> > > > > > > Hehe, this came out *above* the "---" line :)
> > > > > > >
> > > > > > > > Add rts522a L1 sub-state support
> > > > > > > > Save more power on rts5227 rts5249 rts525a rts5260
> > > > > > > > Fix rts5260 driving parameter
> > > > > > > >
> > > > > > > > Signed-off-by: Ricky Wu <[email protected]>
> > > > > > > > ---
> > > > > > > > drivers/misc/cardreader/rts5227.c | 112 +++++++++++++++++++++-
> > > > > > > > drivers/misc/cardreader/rts5249.c | 145
> > > > > > > ++++++++++++++++++++++++++++-
> > > > > > > > drivers/misc/cardreader/rts5260.c | 28 +++---
> > > > > > > > drivers/misc/cardreader/rtsx_pcr.h | 17 ++++
> > > > > > > > 4 files changed, 283 insertions(+), 19 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/misc/cardreader/rts5227.c
> > > > > > > b/drivers/misc/cardreader/rts5227.c
> > > > > > > > index 747391e3fb5d..8859011672cb 100644
> > > > > > > > --- a/drivers/misc/cardreader/rts5227.c
> > > > > > > > +++ b/drivers/misc/cardreader/rts5227.c
> > > > > > > > @@ -72,15 +72,80 @@ static void
> > > rts5227_fetch_vendor_settings(struct
> > > > > > > rtsx_pcr *pcr)
> > > > > > > >
> > > > > > > > pci_read_config_dword(pdev, PCR_SETTING_REG2, &reg);
> > > > > > > > pcr_dbg(pcr, "Cfg 0x%x: 0x%x\n", PCR_SETTING_REG2, 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);
> > > > > > > > if (rtsx_reg_check_reverse_socket(reg))
> > > > > > > > pcr->flags |= PCR_REVERSE_SOCKET;
> > > > > > > > }
> > > > > > > >
> > > > > > > > +static void rts5227_init_from_cfg(struct rtsx_pcr *pcr)
> > > > > > > > +{
> > > > > > > > + struct pci_dev *pdev = pcr->pci;
> > > > > > > > + int l1ss;
> > > > > > > > + u32 lval;
> > > > > > > > + struct rtsx_cr_option *option = &pcr->option;
> > > > > > > > +
> > > > > > > > + l1ss = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_L1SS);
> > > > > > > > + if (!l1ss)
> > > > > > > > + return;
> > > > > > > > +
> > > > > > > > + pci_read_config_dword(pdev, l1ss + PCI_L1SS_CTL1, &lval);
> > > > > > >
> > > > > > > This looks a little problematic. PCI_L1SS_CTL1 is an architected
> > > > > > > register in the ASPM L1 PM Substates capability, and its value may
> > > > > > > change at runtime because drivers/pci/pcie/aspm.c manages it.
> > > > > > >
> > > > > > > It looks like the code below does device-specific configuration based
> > > > > > > on the current PCI_L1SS_CTL1 value. But what happens if aspm.c
> > > > > > > changes PCI_L1SS_CTL1 later?
> > > > > >
> > > > > > We are going to make sure and set the best configuration on the
> > > > > > current time, if host change the capability later, it doesn't affect
> > > > > > function, only affect a little power saving
> > > > >
> > > > > Why don't you unconditionally do the following?
> > > > >
> > > > > rtsx_set_dev_flag(pcr, ASPM_L1_1_EN);
> > > > > rtsx_set_dev_flag(pcr, ASPM_L1_2_EN);
> > > > > rtsx_set_dev_flag(pcr, PM_L1_1_EN);
> > > > > rtsx_set_dev_flag(pcr, PM_L1_2_EN);
> > > >
> > > > Our power saving function have 2 different flow L1 and L1substate,
> > > > so we need to check it for which flow we are going to
> > > > Detail to see below reply
> > > >
> > > > > Let's assume the generic code in aspm.c has cleared all these bits:
> > > > >
> > > > > PCI_L1SS_CTL1_ASPM_L1_1
> > > > > PCI_L1SS_CTL1_ASPM_L1_2
> > > > > PCI_L1SS_CTL1_PCIPM_L1_1
> > > > > PCI_L1SS_CTL1_PCIPM_L1_2
> > > > >
> > > > > in the L1 PM Substates capability.
> > > > >
> > > > > I think you are saying that if you *also* clear ASPM_L1_1_EN,
> > > > > ASPM_L1_2_EN, PM_L1_1_EN, and PM_L1_2_EN in your device-specific
> > > > > registers, it uses less power than if you set those device-specific
> > > > > bits. Right?
> > > > >
> > > > > And moreover, I think you're saying that if aspm.c subsequently *sets*
> > > > > some of those bits in the L1 PM Substates capability, those substates
> > > > > *work* even though the device-specific ASPM_L1_1_EN, ASPM_L1_2_EN,
> > > > > PM_L1_1_EN, and PM_L1_2_EN bits are not set. Right?
> > > > >
> > > > > I do not feel good about this as a general strategy. I think we
> > > > > should program the device so the behavior is completely predictable,
> > > > > regardless of the generic enable bits happened to be set at
> > > > > probe-time.
> > > > >
> > > > > The current approach means that if we enable L1 substates after the
> > > > > driver probe, the device is configured differently than if we enabled
> > > > > L1 substates before probe. That's not a reliable way to operate it.
> > > >
> > > > Talk about our power saving function
> > > > a) basic L1 power saving
> > > > b) advance L1 power saving
> > > > c) advance L1 substate power saving
> > >
> > > I have no idea what the difference between "basic L1 power saving" and
> > > "advance L1 power saving" is, so I assume those are device-specific
> > > things. If not, please use the same terminology as the PCIe spec.
> > >
> > > > at initial, we check pci port support L1 subs or not, if not we are
> > > > going to b) otherwise going to c).
> > >
> > > You're not checking whether the port *supports* L1 substates. You
> > > would look at PCI_L1SS_CAP to learn that. You're looking at
> > > PCI_L1SS_CTL1, which tells you whether L1 substates are *enabled*.
> > >
> > > > Assume aspm.c change bit of L1 PM Substates capability after our
> > > > driver probe, we are going to a)
> > > >
> > > > So far we did not see any platform change L1 PM Substates capability
> > > > after our driver probe.
> > >
> > > You should expect that aspm.c will change bits in the L1 PM *control*
> > > register (PCI_L1SS_CTL1) after probe.
> > >
> > > You might not actually see it change, depending on how you tested, but
> > > you cannot rely on PCI_L1SS_CTL1 being constant. It may change based
> > > on power/performance tradeoffs, e.g., whether the system is plugged
> > > into AC power, whether it's idle, etc.
> > >
> >
> > Our ASPM function is a HW solution follow the PCIe SPEC. don’t worry
> > about crash the system If HOST change our device config space
> > setting at run time our HW will do the corresponding things which
> > follows the SPEC. At initial time we set these parameter just good
> > for more power saving
>
> OK. It would be better if your hardware would notice the
> PCI_L1SS_CTL1 change and set its own device-specific power-saving
> parameters. The drivers would be simpler, and the device behavior
> would be more consistent.
>
> Drivers *writing* to standard PCI config things (64-byte config header
> or Capabilities like PCIe, PM, ASPM, L1 Substates) is a definite
> problem because the PCI core owns those and writes from drivers need
> to be mediated somehow. AFAICT your drivers don't write to these
> things.
>
> Drivers *reading* these things (as your drivers do) shouldn't cause
> problems, but it does violate the interface abstractions that the PCI
> core should provide.

So is it ok to take this patch now, or does it need to be changed any?

thanks,

greg k-h

2020-09-16 19:01:23

by Bjorn Helgaas

Subject: Re: [PATCH v5 2/2] misc: rtsx: Add power saving functions and fix driving parameter

On Wed, Sep 16, 2020 at 02:30:20PM +0200, [email protected] wrote:
> On Tue, Sep 15, 2020 at 10:28:11AM -0500, Bjorn Helgaas wrote:
> > On Tue, Sep 15, 2020 at 08:24:50AM +0000, 吳昊澄 Ricky wrote:
> > > > -----Original Message-----
> > > > From: Bjorn Helgaas [mailto:[email protected]]
> > > > Sent: Friday, September 11, 2020 10:56 PM
> > > > To: 吳昊澄 Ricky
> > > > Cc: [email protected]; [email protected]; [email protected];
> > > > [email protected]; [email protected]; [email protected];
> > > > [email protected]; [email protected];
> > > > [email protected]
> > > > Subject: Re: [PATCH v5 2/2] misc: rtsx: Add power saving functions and fix driving
> > > > parameter
> > > >
> > > > On Fri, Sep 11, 2020 at 08:18:22AM +0000, 吳昊澄 Ricky wrote:
> > > > > > -----Original Message-----
> > > > > > From: Bjorn Helgaas [mailto:[email protected]]
> > > > > > Sent: Thursday, September 10, 2020 1:44 AM
> > > > > > To: 吳昊澄 Ricky
> > > > > > Cc: [email protected]; [email protected]; [email protected];
> > > > > > [email protected]; [email protected];
> > > > [email protected];
> > > > > > [email protected]; [email protected];
> > > > > > [email protected]
> > > > > > Subject: Re: [PATCH v5 2/2] misc: rtsx: Add power saving functions and fix
> > > > driving
> > > > > > parameter
> > > > > >
> > > > > > On Wed, Sep 09, 2020 at 07:10:19AM +0000, 吳昊澄 Ricky wrote:
> > > > > > > > -----Original Message-----
> > > > > > > > From: Bjorn Helgaas [mailto:[email protected]]
> > > > > > > > Sent: Wednesday, September 09, 2020 6:29 AM
> > > > > > > > To: 吳昊澄 Ricky
> > > > > > > > Cc: [email protected]; [email protected];
> > > > [email protected];
> > > > > > > > [email protected]; [email protected];
> > > > > > [email protected];
> > > > > > > > [email protected]; [email protected];
> > > > > > > > [email protected]
> > > > > > > > Subject: Re: [PATCH v5 2/2] misc: rtsx: Add power saving functions and fix
> > > > > > driving
> > > > > > > > parameter
> > > > > > > >
> > > > > > > > On Mon, Sep 07, 2020 at 06:07:31PM +0800, [email protected]
> > > > wrote:
> > > > > > > > > From: Ricky Wu <[email protected]>
> > > > > > > > >
> > > > > > > > > v4:
> > > > > > > > > split power down flow and power saving function to two patch
> > > > > > > > >
> > > > > > > > > v5:
> > > > > > > > > fix up modified change under the --- line
> > > > > > > >
> > > > > > > > Hehe, this came out *above* the "---" line :)
> > > > > > > >
> > > > > > > > > Add rts522a L1 sub-state support
> > > > > > > > > Save more power on rts5227 rts5249 rts525a rts5260
> > > > > > > > > Fix rts5260 driving parameter
> > > > > > > > >
> > > > > > > > > Signed-off-by: Ricky Wu <[email protected]>
> > > > > > > > > ---
> > > > > > > > > drivers/misc/cardreader/rts5227.c | 112 +++++++++++++++++++++-
> > > > > > > > > drivers/misc/cardreader/rts5249.c | 145
> > > > > > > > ++++++++++++++++++++++++++++-
> > > > > > > > > drivers/misc/cardreader/rts5260.c | 28 +++---
> > > > > > > > > drivers/misc/cardreader/rtsx_pcr.h | 17 ++++
> > > > > > > > > 4 files changed, 283 insertions(+), 19 deletions(-)
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/misc/cardreader/rts5227.c
> > > > > > > > b/drivers/misc/cardreader/rts5227.c
> > > > > > > > > index 747391e3fb5d..8859011672cb 100644
> > > > > > > > > --- a/drivers/misc/cardreader/rts5227.c
> > > > > > > > > +++ b/drivers/misc/cardreader/rts5227.c
> > > > > > > > > @@ -72,15 +72,80 @@ static void
> > > > rts5227_fetch_vendor_settings(struct
> > > > > > > > rtsx_pcr *pcr)
> > > > > > > > >
> > > > > > > > > pci_read_config_dword(pdev, PCR_SETTING_REG2, &reg);
> > > > > > > > > pcr_dbg(pcr, "Cfg 0x%x: 0x%x\n", PCR_SETTING_REG2, 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);
> > > > > > > > > if (rtsx_reg_check_reverse_socket(reg))
> > > > > > > > > pcr->flags |= PCR_REVERSE_SOCKET;
> > > > > > > > > }
> > > > > > > > >
> > > > > > > > > +static void rts5227_init_from_cfg(struct rtsx_pcr *pcr)
> > > > > > > > > +{
> > > > > > > > > + struct pci_dev *pdev = pcr->pci;
> > > > > > > > > + int l1ss;
> > > > > > > > > + u32 lval;
> > > > > > > > > + struct rtsx_cr_option *option = &pcr->option;
> > > > > > > > > +
> > > > > > > > > + l1ss = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_L1SS);
> > > > > > > > > + if (!l1ss)
> > > > > > > > > + return;
> > > > > > > > > +
> > > > > > > > > + pci_read_config_dword(pdev, l1ss + PCI_L1SS_CTL1, &lval);
> > > > > > > >
> > > > > > > > This looks a little problematic. PCI_L1SS_CTL1 is an architected
> > > > > > > > register in the ASPM L1 PM Substates capability, and its value may
> > > > > > > > change at runtime because drivers/pci/pcie/aspm.c manages it.
> > > > > > > >
> > > > > > > > It looks like the code below does device-specific configuration based
> > > > > > > > on the current PCI_L1SS_CTL1 value. But what happens if aspm.c
> > > > > > > > changes PCI_L1SS_CTL1 later?
> > > > > > >
> > > > > > > We are going to make sure and set the best configuration on the
> > > > > > > current time, if host change the capability later, it doesn't affect
> > > > > > > function, only affect a little power saving
> > > > > >
> > > > > > Why don't you unconditionally do the following?
> > > > > >
> > > > > > rtsx_set_dev_flag(pcr, ASPM_L1_1_EN);
> > > > > > rtsx_set_dev_flag(pcr, ASPM_L1_2_EN);
> > > > > > rtsx_set_dev_flag(pcr, PM_L1_1_EN);
> > > > > > rtsx_set_dev_flag(pcr, PM_L1_2_EN);
> > > > >
> > > > > Our power saving function have 2 different flow L1 and L1substate,
> > > > > so we need to check it for which flow we are going to
> > > > > Detail to see below reply
> > > > >
> > > > > > Let's assume the generic code in aspm.c has cleared all these bits:
> > > > > >
> > > > > > PCI_L1SS_CTL1_ASPM_L1_1
> > > > > > PCI_L1SS_CTL1_ASPM_L1_2
> > > > > > PCI_L1SS_CTL1_PCIPM_L1_1
> > > > > > PCI_L1SS_CTL1_PCIPM_L1_2
> > > > > >
> > > > > > in the L1 PM Substates capability.
> > > > > >
> > > > > > I think you are saying that if you *also* clear ASPM_L1_1_EN,
> > > > > > ASPM_L1_2_EN, PM_L1_1_EN, and PM_L1_2_EN in your device-specific
> > > > > > registers, it uses less power than if you set those device-specific
> > > > > > bits. Right?
> > > > > >
> > > > > > And moreover, I think you're saying that if aspm.c subsequently *sets*
> > > > > > some of those bits in the L1 PM Substates capability, those substates
> > > > > > *work* even though the device-specific ASPM_L1_1_EN, ASPM_L1_2_EN,
> > > > > > PM_L1_1_EN, and PM_L1_2_EN bits are not set. Right?
> > > > > >
> > > > > > I do not feel good about this as a general strategy. I think we
> > > > > > should program the device so the behavior is completely predictable,
> > > > > > regardless of the generic enable bits happened to be set at
> > > > > > probe-time.
> > > > > >
> > > > > > The current approach means that if we enable L1 substates after the
> > > > > > driver probe, the device is configured differently than if we enabled
> > > > > > L1 substates before probe. That's not a reliable way to operate it.
> > > > >
> > > > > Talk about our power saving function
> > > > > a) basic L1 power saving
> > > > > b) advance L1 power saving
> > > > > c) advance L1 substate power saving
> > > >
> > > > I have no idea what the difference between "basic L1 power saving" and
> > > > "advance L1 power saving" is, so I assume those are device-specific
> > > > things. If not, please use the same terminology as the PCIe spec.
> > > >
> > > > > at initial, we check pci port support L1 subs or not, if not we are
> > > > > going to b) otherwise going to c).
> > > >
> > > > You're not checking whether the port *supports* L1 substates. You
> > > > would look at PCI_L1SS_CAP to learn that. You're looking at
> > > > PCI_L1SS_CTL1, which tells you whether L1 substates are *enabled*.
> > > >
> > > > > Assume aspm.c change bit of L1 PM Substates capability after our
> > > > > driver probe, we are going to a)
> > > > >
> > > > > So far we did not see any platform change L1 PM Substates capability
> > > > > after our driver probe.
> > > >
> > > > You should expect that aspm.c will change bits in the L1 PM *control*
> > > > register (PCI_L1SS_CTL1) after probe.
> > > >
> > > > You might not actually see it change, depending on how you tested, but
> > > > you cannot rely on PCI_L1SS_CTL1 being constant. It may change based
> > > > on power/performance tradeoffs, e.g., whether the system is plugged
> > > > into AC power, whether it's idle, etc.
> > > >
> > >
> > > Our ASPM function is a HW solution follow the PCIe SPEC. don’t worry
> > > about crash the system If HOST change our device config space
> > > setting at run time our HW will do the corresponding things which
> > > follows the SPEC. At initial time we set these parameter just good
> > > for more power saving
> >
> > OK. It would be better if your hardware would notice the
> > PCI_L1SS_CTL1 change and set its own device-specific power-saving
> > parameters. The drivers would be simpler, and the device behavior
> > would be more consistent.
> >
> > Drivers *writing* to standard PCI config things (64-byte config header
> > or Capabilities like PCIe, PM, ASPM, L1 Substates) is a definite
> > problem because the PCI core owns those and writes from drivers need
> > to be mediated somehow. AFAICT your drivers don't write to these
> > things.
> >
> > Drivers *reading* these things (as your drivers do) shouldn't cause
> > problems, but it does violate the interface abstractions that the PCI
> > core should provide.
>
> So is it ok to take this patch now, or does it need to be changed any?

Yes, it's OK with me if you take this patch.

The ASPM hardware feature is designed to work without any driver
support. It does need to be configured, which involves both the
device and the upstream bridge, so it should be done by the BIOS or
the PCI core. There are a few drivers (amdgpu, radeon, hfi1, e1000e,
iwlegacy, ath10k, ath9k, mt76, rtlwifi, rtw88, and these rts
cardreader drivers) that do it themselves, incorrectly.

But this particular patch only *reads* the ASPM control registers,
without writing them, so it shouldn't make anything worse.

Bjorn

2020-09-16 19:17:34

by Greg Kroah-Hartman

Subject: Re: [PATCH v5 2/2] misc: rtsx: Add power saving functions and fix driving parameter

On Wed, Sep 16, 2020 at 08:32:26AM -0500, Bjorn Helgaas wrote:
> > So is it ok to take this patch now, or does it need to be changed any?
>
> Yes, it's OK with me if you take this patch.
>
> The ASPM hardware feature is designed to work without any driver
> support. It does need to be configured, which involves both the
> device and the upstream bridge, so it should be done by the BIOS or
> the PCI core. There are a few drivers (amdgpu, radeon, hfi1, e1000e,
> iwlegacy, ath10k, ath9k, mt76, rtlwifi, rtw88, and these rts
> cardreader drivers) that do it themselves, incorrectly.
>
> But this particular patch only *reads* the ASPM control registers,
> without writing them, so it shouldn't make anything worse.

Ok, thanks for the review, now queued up.

greg k-h

2021-03-25 19:37:25

by Marius Bakke

Subject: Sudden poweroffs when idle on ThinkPad x290s since v5.10-rc1 (bisected to 7c33e3c4c79a)

Hello,

After updating to the 5.10 kernel, my laptop will spontaneously poweroff
when idle after some time (between 30 minutes and three days).

I have bisected it down to a rtsx driver update in 7c33e3c4c79a:

commit 7c33e3c4c79ac5def79e7c773e38a7113eb14204
Author: Ricky Wu <[email protected]>
Date: Mon Sep 7 18:07:31 2020 +0800

misc: rtsx: Add power saving functions and fix driving parameter

v4:
split power down flow and power saving function to two patch

v5:
fix up modified change under the --- line

Add rts522a L1 sub-state support
Save more power on rts5227 rts5249 rts525a rts5260
Fix rts5260 driving parameter

Signed-off-by: Ricky Wu <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Greg Kroah-Hartman <[email protected]>

drivers/misc/cardreader/rts5227.c | 112 +++++++++++++++++++++++++++-
drivers/misc/cardreader/rts5249.c | 145 ++++++++++++++++++++++++++++++++++++-
drivers/misc/cardreader/rts5260.c | 28 +++----
drivers/misc/cardreader/rtsx_pcr.h | 17 +++++
4 files changed, 283 insertions(+), 19 deletions(-)

(See also <https://bugzilla.kernel.org/show_bug.cgi?id=211809>.)

I'm not sure whether this is a driver or hardware problem and am seeking
feedback on how to further debug the issue.

FWIW the problem does not seem to reproduce on an AMD ThinkPad T14 Gen 1
with the same RTS522A PCIe card reader (on stock Arch 5.10.16 kernel).

Thanks & happy hacking,
Marius

Attachments: