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

[吳昊澄 Ricky]

> -----Original Message-----
> From: Bjorn Helgaas [mailto:helgaas@xxxxxxxxxx]
> Sent: Thursday, September 10, 2020 1:44 AM
> To: 吳昊澄 Ricky
> Cc: arnd@xxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx; bhelgaas@xxxxxxxxxx;
> ulf.hansson@xxxxxxxxxx; rui_feng@xxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> puranjay12@xxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx;
> vailbhavgupta40@xxxxxxxxxx
> 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:helgaas@xxxxxxxxxx]
> > > Sent: Wednesday, September 09, 2020 6:29 AM
> > > To: 吳昊澄 Ricky
> > > Cc: arnd@xxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx; bhelgaas@xxxxxxxxxx;
> > > ulf.hansson@xxxxxxxxxx; rui_feng@xxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx;
> > > puranjay12@xxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx;
> > > vailbhavgupta40@xxxxxxxxxx
> > > 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, 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?
> >
> > 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.