Messages in this thread | | | Date | Tue, 5 Sep 2017 09:18:10 +0100 | From | Lee Jones <> | Subject | Re: 答复: 答复 : [PATCH v4] mfd: Add support for RTS5250S power saving |
| |
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. > > > > + 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. >
-- Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
| |