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

[åé]

Dear Jones,

> +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.

If val is not pre-allocated, what will the val be if it is not assigned before using?

> +	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.

> +	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.

> +	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.