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

[Bjorn Helgaas]

On Mon, Sep 07, 2020 at 06:07:31PM +0800, ricky_wu@xxxxxxxxxxx wrote:
> From: Ricky Wu <ricky_wu@xxxxxxxxxxx>
> 
> 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 <ricky_wu@xxxxxxxxxxx>
> ---
>  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;
> +
> +}