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

Commit Message

Ricky WU Sept. 7, 2020, 10:07 a.m. UTC

From: Ricky Wu <[e-mail]>

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 <[e-mail]>
---
 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(-)

Comments

Bjorn Helgaas Sept. 8, 2020, 10:28 p.m. UTC | #1

On Mon, Sep 07, 2020 at 06:07:31PM +0800, [e-mail] wrote:
> From: Ricky Wu <[e-mail]>
> 
> 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 <[e-mail]>
> ---
>  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;
> +
> +}

Ricky WU Sept. 9, 2020, 7:10 a.m. UTC | #2

> -----Original Message-----
> From: Bjorn Helgaas [mailto:[e-mail]]
> Sent: Wednesday, September 09, 2020 6:29 AM
> To: 吳昊澄 Ricky
> Cc: [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]
> 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, [e-mail] wrote:
> > From: Ricky Wu <[e-mail]>
> >
> > 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 <[e-mail]>
> > ---
> >  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.

Bjorn Helgaas Sept. 9, 2020, 5:44 p.m. UTC | #3

On Wed, Sep 09, 2020 at 07:10:19AM +0000, 吳昊澄 Ricky wrote:
> > -----Original Message-----
> > From: Bjorn Helgaas [mailto:[e-mail]]
> > Sent: Wednesday, September 09, 2020 6:29 AM
> > To: 吳昊澄 Ricky
> > Cc: [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; [e-mail];
> > [e-mail]
> > 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, [e-mail] wrote:
> > > From: Ricky Wu <[e-mail]>
> > >
> > > 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 <[e-mail]>
> > > ---
> > >  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.

Ricky WU Sept. 11, 2020, 8:18 a.m. UTC | #4

> -----Original Message-----
> From: Bjorn Helgaas [mailto:[e-mail]]
> Sent: Thursday, September 10, 2020 1:44 AM
> To: 吳昊澄 Ricky
> Cc: [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]
> 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:[e-mail]]
> > > Sent: Wednesday, September 09, 2020 6:29 AM
> > > To: 吳昊澄 Ricky
> > > Cc: [e-mail]; [e-mail]; [e-mail];
> > > [e-mail]; [e-mail];
> [e-mail];
> > > [e-mail]; [e-mail];
> > > [e-mail]
> > > 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, [e-mail] wrote:
> > > > From: Ricky Wu <[e-mail]>
> > > >
> > > > 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 <[e-mail]>
> > > > ---
> > > >  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.

Bjorn Helgaas Sept. 11, 2020, 2:56 p.m. UTC | #5

On Fri, Sep 11, 2020 at 08:18:22AM +0000, 吳昊澄 Ricky wrote:
> > -----Original Message-----
> > From: Bjorn Helgaas [mailto:[e-mail]]
> > Sent: Thursday, September 10, 2020 1:44 AM
> > To: 吳昊澄 Ricky
> > Cc: [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; [e-mail];
> > [e-mail]
> > 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:[e-mail]]
> > > > Sent: Wednesday, September 09, 2020 6:29 AM
> > > > To: 吳昊澄 Ricky
> > > > Cc: [e-mail]; [e-mail]; [e-mail];
> > > > [e-mail]; [e-mail];
> > [e-mail];
> > > > [e-mail]; [e-mail];
> > > > [e-mail]
> > > > 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, [e-mail] wrote:
> > > > > From: Ricky Wu <[e-mail]>
> > > > >
> > > > > 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 <[e-mail]>
> > > > > ---
> > > > >  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.

Ricky WU Sept. 15, 2020, 8:24 a.m. UTC | #6

> -----Original Message-----
> From: Bjorn Helgaas [mailto:[e-mail]]
> Sent: Friday, September 11, 2020 10:56 PM
> To: 吳昊澄 Ricky
> Cc: [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]
> 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:[e-mail]]
> > > Sent: Thursday, September 10, 2020 1:44 AM
> > > To: 吳昊澄 Ricky
> > > Cc: [e-mail]; [e-mail]; [e-mail];
> > > [e-mail]; [e-mail];
> [e-mail];
> > > [e-mail]; [e-mail];
> > > [e-mail]
> > > 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:[e-mail]]
> > > > > Sent: Wednesday, September 09, 2020 6:29 AM
> > > > > To: 吳昊澄 Ricky
> > > > > Cc: [e-mail]; [e-mail];
> [e-mail];
> > > > > [e-mail]; [e-mail];
> > > [e-mail];
> > > > > [e-mail]; [e-mail];
> > > > > [e-mail]
> > > > > 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, [e-mail]
> wrote:
> > > > > > From: Ricky Wu <[e-mail]>
> > > > > >
> > > > > > 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 <[e-mail]>
> > > > > > ---
> > > > > >  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.

Bjorn Helgaas Sept. 15, 2020, 3:28 p.m. UTC | #7

On Tue, Sep 15, 2020 at 08:24:50AM +0000, 吳昊澄 Ricky wrote:
> > -----Original Message-----
> > From: Bjorn Helgaas [mailto:[e-mail]]
> > Sent: Friday, September 11, 2020 10:56 PM
> > To: 吳昊澄 Ricky
> > Cc: [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; [e-mail];
> > [e-mail]
> > 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:[e-mail]]
> > > > Sent: Thursday, September 10, 2020 1:44 AM
> > > > To: 吳昊澄 Ricky
> > > > Cc: [e-mail]; [e-mail]; [e-mail];
> > > > [e-mail]; [e-mail];
> > [e-mail];
> > > > [e-mail]; [e-mail];
> > > > [e-mail]
> > > > 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:[e-mail]]
> > > > > > Sent: Wednesday, September 09, 2020 6:29 AM
> > > > > > To: 吳昊澄 Ricky
> > > > > > Cc: [e-mail]; [e-mail];
> > [e-mail];
> > > > > > [e-mail]; [e-mail];
> > > > [e-mail];
> > > > > > [e-mail]; [e-mail];
> > > > > > [e-mail]
> > > > > > 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, [e-mail]
> > wrote:
> > > > > > > From: Ricky Wu <[e-mail]>
> > > > > > >
> > > > > > > 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 <[e-mail]>
> > > > > > > ---
> > > > > > >  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

Greg KH Sept. 16, 2020, 12:30 p.m. UTC | #8

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:[e-mail]]
> > > Sent: Friday, September 11, 2020 10:56 PM
> > > To: 吳昊澄 Ricky
> > > Cc: [e-mail]; [e-mail]; [e-mail];
> > > [e-mail]; [e-mail]; [e-mail];
> > > [e-mail]; [e-mail];
> > > [e-mail]
> > > 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:[e-mail]]
> > > > > Sent: Thursday, September 10, 2020 1:44 AM
> > > > > To: 吳昊澄 Ricky
> > > > > Cc: [e-mail]; [e-mail]; [e-mail];
> > > > > [e-mail]; [e-mail];
> > > [e-mail];
> > > > > [e-mail]; [e-mail];
> > > > > [e-mail]
> > > > > 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:[e-mail]]
> > > > > > > Sent: Wednesday, September 09, 2020 6:29 AM
> > > > > > > To: 吳昊澄 Ricky
> > > > > > > Cc: [e-mail]; [e-mail];
> > > [e-mail];
> > > > > > > [e-mail]; [e-mail];
> > > > > [e-mail];
> > > > > > > [e-mail]; [e-mail];
> > > > > > > [e-mail]
> > > > > > > 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, [e-mail]
> > > wrote:
> > > > > > > > From: Ricky Wu <[e-mail]>
> > > > > > > >
> > > > > > > > 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 <[e-mail]>
> > > > > > > > ---
> > > > > > > >  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

Patch

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