Re: çå: çå: [PATCH v4] mfd: Add support for RTS5250S power saving

[Lee Jones]

On Tue, 05 Sep 2017, Lee Jones wrote:

> On Tue, 05 Sep 2017, åé wrote:
> 
> > > > +static void rts5250_set_l1off_cfg_sub_d0(struct rtsx_pcr *pcr, int
> > > > +active) {
> > > > +	struct rtsx_cr_option *option = &(pcr->option);
> > > > +
> > > > +	u32 interrupt = rtsx_pci_readl(pcr, RTSX_BIPR);
> > > > +	int card_exist = (interrupt & SD_EXIST) | (interrupt & MS_EXIST);
> > > > +	int aspm_L1_1, aspm_L1_2;
> > > > +	u8 val = 0;
> > > 
> > > No need to pre-allocate.
> > 
> > All of my responses should now be quotes, but they are not.
> > 
> > This makes replying to your messages difficult.
> > 
> > Please fix your editor.  
> > 
> > > If val is not pre-allocated, what will the val be if it is not assigned before using?
> > 
> > You do not use it before assigning it.
> 
> Your mailer is still broken.  Please fix it.
> 
> > If aspm_L1_1 == 0 && aspm_L1_2 == 0, the val will be used before assigned.
> 
> Ah yes, I see.  Leave it as it is then.
> 
> > > > +	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;
> > > > +	}
> > > > +
> > > > +	if (aspm_L1_1 || aspm_L1_2) {
> > > > +		if (rtsx_check_dev_flag(pcr,
> > > > +					LTR_L1SS_PWR_GATE_CHECK_CARD_EN)) {
> > > > +			if (card_exist)
> > > > +				val &= ~L1OFF_MBIAS2_EN_5250;
> > > > +			else
> > > > +				val |= L1OFF_MBIAS2_EN_5250;
> > > > +		}
> > > > +	}
> > > > +	rtsx_set_l1off_sub(pcr, val);
> > > > +}
> > > > +
> > > >  static const struct pcr_ops rts524a_pcr_ops = {
> > > >  	.write_phy = rts524a_write_phy,
> > > >  	.read_phy = rts524a_read_phy,
> > > > @@ -473,11 +617,16 @@ static int rts524a_extra_init_hw(struct rtsx_pcr *pcr)
> > > >  	.card_power_off = rtsx_base_card_power_off,
> > > >  	.switch_output_voltage = rtsx_base_switch_output_voltage,
> > > >  	.force_power_down = rtsx_base_force_power_down,
> > > > +	.set_l1off_cfg_sub_d0 = rts5250_set_l1off_cfg_sub_d0,
> > > > +	.set_aspm = rts5249_set_aspm,
> > > >  };
> > > >  
> > > >  void rts524a_init_params(struct rtsx_pcr *pcr)  {
> > > >  	rts5249_init_params(pcr);
> > > > +	pcr->option.ltr_l1off_sspwrgate = LTR_L1OFF_SSPWRGATE_5250_DEFAULT;
> > > > +	pcr->option.ltr_l1off_snooze_sspwrgate =
> > > > +		LTR_L1OFF_SNOOZE_SSPWRGATE_5250_DEFAULT;
> > > >  
> > > >  	pcr->reg_pm_ctrl3 = RTS524A_PM_CTRL3;
> > > >  	pcr->ops = &rts524a_pcr_ops;
> > > > @@ -576,11 +725,16 @@ static int rts525a_extra_init_hw(struct rtsx_pcr *pcr)
> > > >  	.card_power_off = rtsx_base_card_power_off,
> > > >  	.switch_output_voltage = rts525a_switch_output_voltage,
> > > >  	.force_power_down = rtsx_base_force_power_down,
> > > > +	.set_l1off_cfg_sub_d0 = rts5250_set_l1off_cfg_sub_d0,
> > > > +	.set_aspm = rts5249_set_aspm,
> > > >  };
> > > >  
> > > >  void rts525a_init_params(struct rtsx_pcr *pcr)  {
> > > >  	rts5249_init_params(pcr);
> > > > +	pcr->option.ltr_l1off_sspwrgate = LTR_L1OFF_SSPWRGATE_5250_DEFAULT;
> > > > +	pcr->option.ltr_l1off_snooze_sspwrgate =
> > > > +		LTR_L1OFF_SNOOZE_SSPWRGATE_5250_DEFAULT;
> > > >  
> > > >  	pcr->reg_pm_ctrl3 = RTS524A_PM_CTRL3;
> > > >  	pcr->ops = &rts525a_pcr_ops;
> > > > diff --git a/drivers/mfd/rtsx_pcr.c b/drivers/mfd/rtsx_pcr.c index
> > > > a0ac89d..50a6e67 100644
> > > > --- a/drivers/mfd/rtsx_pcr.c
> > > > +++ b/drivers/mfd/rtsx_pcr.c
> > > > @@ -79,6 +79,131 @@ static inline void rtsx_pci_disable_aspm(struct rtsx_pcr *pcr)
> > > >  		0xFC, 0);
> > > >  }
> > > >  
> > > > +int rtsx_comm_set_ltr_latency(struct rtsx_pcr *pcr, u32 latency) {
> > > > +	rtsx_pci_write_register(pcr, MSGTXDATA0, 0xFF, (u8) (latency & 
> > > > +0xFF));
> > > 
> > > What is (the first) 0xFF?
> > > 
> > > 0xFF is just 8 bit mask.
> > 
> > I mean the first one.
> > 
> > The first 0xFF is also 8 bit mask.

Please define them for clarity.

> > > > +	rtsx_pci_write_register(pcr, MSGTXDATA1,
> > > > +				0xFF, (u8)((latency >> 8) & 0xFF));
> > > > +	rtsx_pci_write_register(pcr, MSGTXDATA2,
> > > > +				0xFF, (u8)((latency >> 16) & 0xFF));
> > > > +	rtsx_pci_write_register(pcr, MSGTXDATA3,
> > > > +				0xFF, (u8)((latency >> 24) & 0xFF));
> > > > +	rtsx_pci_write_register(pcr, LTR_CTL, LTR_TX_EN_MASK |
> > > > +		LTR_LATENCY_MODE_MASK, LTR_TX_EN_1 | LTR_LATENCY_MODE_SW);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +int rtsx_set_ltr_latency(struct rtsx_pcr *pcr, u32 latency) {
> > > > +	if (pcr->ops->set_ltr_latency)
> > > > +		return pcr->ops->set_ltr_latency(pcr, latency);
> > > > +	else
> > > > +		return rtsx_comm_set_ltr_latency(pcr, latency); }
> > > > +
> > > > +static void rtsx_comm_set_aspm(struct rtsx_pcr *pcr, bool enable) {
> > > > +	struct rtsx_cr_option *option = &pcr->option;
> > > > +
> > > > +	if (pcr->aspm_enabled == enable)
> > > > +		return;
> > > > +
> > > > +	if (option->dev_aspm_mode == DEV_ASPM_DYNAMIC) {
> > > > +		if (enable)
> > > > +			rtsx_pci_enable_aspm(pcr);
> > > > +		else
> > > > +			rtsx_pci_disable_aspm(pcr);
> > > > +	} else if (option->dev_aspm_mode == DEV_ASPM_BACKDOOR) {
> > > > +		u8 mask = FORCE_ASPM_VAL_MASK;
> > > > +		u8 val = 0;
> > > > +
> > > > +		if (enable)
> > > > +			val = pcr->aspm_en;
> > > > +		rtsx_pci_write_register(pcr, ASPM_FORCE_CTL,  mask, val);
> > > > +	}
> > > > +
> > > > +	pcr->aspm_enabled = enable;
> > > > +}
> > > > +
> > > > +static void rtsx_disable_aspm(struct rtsx_pcr *pcr) {
> > > > +	if (pcr->ops->set_aspm)
> > > > +		pcr->ops->set_aspm(pcr, false);
> > > > +	else
> > > > +		rtsx_comm_set_aspm(pcr, false);
> > > > +}
> > > > +
> > > > +int rtsx_set_l1off_sub(struct rtsx_pcr *pcr, u8 val) {
> > > > +	rtsx_pci_write_register(pcr, L1SUB_CONFIG3, 0xFF, val);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static void rtsx_comm_set_l1off_cfg_sub_d0(struct rtsx_pcr *pcr, 
> > > > +int
> > > > +active) {
> > > > +	struct rtsx_cr_option *option = &(pcr->option);
> > > > +
> > > > +	u32 interrupt = rtsx_pci_readl(pcr, RTSX_BIPR);
> > > > +	int card_exist = (interrupt & SD_EXIST) | (interrupt & MS_EXIST);
> > > > +	int aspm_L1_1, aspm_L1_2;
> > > > +	u8 val = 0;
> > > 
> > > No need to pre-initialise.
> > > 
> > > Same above.
> > 
> > Same answer.
> > 
> > Same above.
> > 
> > > > +	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;
> > > > +	}
> > > > +
> > > > +	if (aspm_L1_1 || aspm_L1_2) {
> > > > +		if (rtsx_check_dev_flag(pcr,
> > > > +					LTR_L1SS_PWR_GATE_CHECK_CARD_EN)) {
> > > > +			if (card_exist)
> > > > +				val &= ~L1OFF_MBIAS2_EN_COMM;
> > > > +			else
> > > > +				val |= L1OFF_MBIAS2_EN_COMM;
> > > > +		}
> > > > +	}
> > > > +	rtsx_set_l1off_sub(pcr, val);
> > > > +}
> > > 
> > > This function looks very similar to the one above.
> > > 
> > > Any way you can integrate the two?
> > > 
> > > The two functions are callback functions in different files for 
> > > different chips, in the future maybe need to support other chips, so I think separating them is low coupling.
> > 
> > Are they identical?
> > 
> > No. Different chips maybe different.

But are the functions *currently* identical?

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org â Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog