Messages in this thread Patch in this message | | | Date | Tue, 22 Aug 2017 11:30:43 +0100 | From | Lee Jones <> | Subject | Re: 答复 : [PATCH v3] mfd: Add support for RTS5250S power saving |
| |
You have dropped the Mailing List off.
Please don't do that.
On Tue, 22 Aug 2017, 冯锐 wrote:
> Dear Jones: > > > +struct rtsx_cr_option { > > + u32 dev_flags; > > + bool force_clkreq_0; > > + bool ltr_en; > > + bool ltr_enabled; > > + bool ltr_active; > > + u32 ltr_active_latency; > > + u32 ltr_idle_latency; > > + u32 ltr_l1off_latency; > > + enum dev_aspm_mode dev_aspm_mode; > > + u32 l1_snooze_delay; > > + u8 ltr_l1off_sspwrgate; > > + u8 ltr_l1off_snooze_sspwrgate; > > +}; > > This still needs a kerneldoc header.
You need to sort out your email client.
This line should have been quoted.
> Do you mean adding comment for each member of the struct?
Kerneldoc format looks like this:
/** * struct ccp_aes_engine - CCP AES operation * @type: AES operation key size * @mode: AES operation mode * @action: AES operation (decrypt/encrypt) * @key: key to be used for this AES operation * @key_len: length in bytes of key * @iv: IV to be used for this AES operation * @iv_len: length in bytes of iv * @src: data to be used for this operation * @dst: data produced by this operation * @src_len: length in bytes of data used for this operation * @cmac_final: indicates final operation when running in CMAC mode * @cmac_key: K1/K2 key used in final CMAC operation * @cmac_key_len: length in bytes of cmac_key */ > > + 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; > > + } > > No need for single line statements to be in {}'s. > > But according to the coding-style.rst, the {} is necessary. > > This does not apply if only one branch of a conditional statement is a single > statement; in the latter case use braces in both branches: > > .. code-block:: c > > if (condition) { > do_this(); > do_that(); > } else { > otherwise(); > }
Ah, I thought that was only for Zepher
Never mind then. Keep that as it is.
> -----邮件原件----- > 发件人: Lee Jones [mailto:lee.jones@linaro.org] > 发送时间: 2017年8月22日 15:33 > 收件人: 冯锐 > 抄送: linux-kernel@vger.kernel.org > 主题: Re: [PATCH v3] mfd: Add support for RTS5250S power saving
Ah, I see what you've done.
Please just respond in line in future.
Also, these headers should be removed.
> On Tue, 22 Aug 2017, rui_feng@realsil.com.cn wrote: > > > From: Rui Feng <rui_feng@realsil.com.cn> > > > > Signed-off-by: Rui Feng <rui_feng@realsil.com.cn> > > --- > > drivers/mfd/rts5249.c | 195 ++++++++++++++++++++++++++++++++++++++++ > > drivers/mfd/rtsx_pcr.c | 205 +++++++++++++++++++++++++++++++++++++++++-- > > drivers/mfd/rtsx_pcr.h | 11 +++ > > include/linux/mfd/rtsx_pci.h | 60 +++++++++++++ > > 4 files changed, 466 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/mfd/rts5249.c b/drivers/mfd/rts5249.c index > > 40f8bb1..3959ada 100644 > > --- a/drivers/mfd/rts5249.c > > +++ b/drivers/mfd/rts5249.c > > @@ -103,8 +103,72 @@ static void rtsx_base_force_power_down(struct rtsx_pcr *pcr, u8 pm_state) > > rtsx_pci_write_register(pcr, FPDCTL, 0x03, 0x03); } > > > > +static int rts5249_init_from_cfg(struct rtsx_pcr *pcr) { > > + struct rtsx_cr_option *option = &(pcr->option); > > + u32 lval; > > + > > + if (CHK_PCI_PID(pcr, PID_524A)) > > + rtsx_pci_read_config_dword(pcr, > > + PCR_ASPM_SETTING_REG1, &lval); > > + else > > + rtsx_pci_read_config_dword(pcr, > > + PCR_ASPM_SETTING_REG2, &lval); > > + > > + if (lval & ASPM_L1_1_EN_MASK) > > + rtsx_set_dev_flag(pcr, ASPM_L1_1_EN); > > + else > > + rtsx_clear_dev_flag(pcr, ASPM_L1_1_EN); > > '\n' here. > > > + if (lval & ASPM_L1_2_EN_MASK) > > + rtsx_set_dev_flag(pcr, ASPM_L1_2_EN); > > + else > > + rtsx_clear_dev_flag(pcr, ASPM_L1_2_EN); > > Why don't you ensure all flags are already clear? That way you can drop all of the clear flag calls and only set them when you know they are available. > > > + if (lval & PM_L1_1_EN_MASK) > > + rtsx_set_dev_flag(pcr, PM_L1_1_EN); > > + else > > + rtsx_clear_dev_flag(pcr, PM_L1_1_EN); > > '\n' here. > > > + if (lval & PM_L1_2_EN_MASK) > > + 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; > > + } > > No need for single line statements to be in {}'s. > > > + } > > + > > + return 0; > > Why does this function return int? > > You don't even check the return value. > > > +} > > + > > +static int rts5249_init_from_hw(struct rtsx_pcr *pcr) { > > + struct rtsx_cr_option *option = &(pcr->option); > > + > > + 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; > > + > > + return 0; > > +} > > + > > static int rts5249_extra_init_hw(struct rtsx_pcr *pcr) { > > + struct rtsx_cr_option *option = &(pcr->option); > > + > > + rts5249_init_from_cfg(pcr); > > + rts5249_init_from_hw(pcr); > > + > > rtsx_pci_init_cmd(pcr); > > > > /* Rest L1SUB Config */ > > @@ -125,6 +189,17 @@ static int rts5249_extra_init_hw(struct rtsx_pcr *pcr) > > else > > rtsx_pci_add_cmd(pcr, WRITE_REG_CMD, PETXCFG, 0xB0, 0x80); > > > > + /* > > + * u_force_clkreq_0 > > + * If true, CLKREQ# PIN will be forced to drive low to request clock > > + */ > > "If u_force_clkreq_0 is enabled, we forcibly request the XXX clock." > > > + 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); > > + > > return rtsx_pci_send_cmd(pcr, 100); > > } > > > > @@ -353,6 +428,8 @@ static int rtsx_base_switch_output_voltage(struct > > rtsx_pcr *pcr, u8 voltage) > > > > void rts5249_init_params(struct rtsx_pcr *pcr) { > > + struct rtsx_cr_option *option = &(pcr->option); > > + > > pcr->extra_caps = EXTRA_CAPS_SD_SDR50 | EXTRA_CAPS_SD_SDR104; > > pcr->num_slots = 2; > > pcr->ops = &rts5249_pcr_ops; > > @@ -372,6 +449,20 @@ void rts5249_init_params(struct rtsx_pcr *pcr) > > pcr->ms_pull_ctl_disable_tbl = rts5249_ms_pull_ctl_disable_tbl; > > > > pcr->reg_pm_ctrl3 = PM_CTRL3; > > + > > + option->dev_flags |= (LTR_L1SS_PWR_GATE_CHECK_CARD_EN > > + | 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_DEFAULT; > > + option->ltr_idle_latency = LTR_IDLE_LATENCY_DEFAULT; > > + option->ltr_l1off_latency = LTR_L1OFF_LATENCY_DEFAULT; > > + option->dev_aspm_mode = DEV_ASPM_DYNAMIC; > > + option->l1_snooze_delay = 1; > > 1 what? > > > + option->ltr_l1off_sspwrgate = LTR_L1OFF_SSPWRGATE_5249_DEFAULT; > > + option->ltr_l1off_snooze_sspwrgate = > > + LTR_L1OFF_SNOOZE_SSPWRGATE_5249_DEFAULT; > > } > > > > static int rts524a_write_phy(struct rtsx_pcr *pcr, u8 addr, u16 val) > > @@ -459,6 +550,54 @@ static int rts524a_extra_init_hw(struct rtsx_pcr *pcr) > > return 0; > > } > > > > +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; > > + > > + 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) { > > + u8 val = option->ltr_l1off_snooze_sspwrgate; > > + > > + if (rtsx_check_dev_flag(pcr, > > + LTR_L1SS_PWR_GATE_CHECK_CARD_EN)) { > > + if (card_exist) > > + val &= ~(1 << 7); > > + else > > + val |= (1 << 7); > > These need defining. > > > + } > > + > > + rtsx_set_l1off_sub(pcr, val); > > + } else { > > + rtsx_set_l1off_sub(pcr, 0); > > + } > > + } else { > > + /* l1off, latency: 300us */ > > + if (aspm_L1_2) { > > + u8 val = option->ltr_l1off_sspwrgate; > > + > > + if (rtsx_check_dev_flag(pcr, > > + LTR_L1SS_PWR_GATE_CHECK_CARD_EN)) { > > + if (card_exist) > > + val &= ~(1 << 7); > > + else > > + val |= (1 << 7); > > + } > > + > > + rtsx_set_l1off_sub(pcr, val); > > + } else { > > + rtsx_set_l1off_sub(pcr, 0); > > + } > > + } > > +} > > + > > static const struct pcr_ops rts524a_pcr_ops = { > > .write_phy = rts524a_write_phy, > > .read_phy = rts524a_read_phy, > > @@ -473,11 +612,15 @@ 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, > > }; > > > > 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; > > @@ -564,6 +707,52 @@ static int rts525a_extra_init_hw(struct rtsx_pcr *pcr) > > return 0; > > } > > > > +static void rts5249_enable_aspm(struct rtsx_pcr *pcr) { > > + struct rtsx_cr_option *option = &pcr->option; > > + > > + if (pcr->aspm_enabled) > > + return; > > + > > + if (option->dev_aspm_mode == DEV_ASPM_DISABLE) { > > + pcr->aspm_enabled = true; > > + return; > > + } > > '\n' here. > > > + if (option->dev_aspm_mode == DEV_ASPM_DYNAMIC) { > > + rtsx_pci_update_cfg_byte(pcr, > > + pcr->pcie_cap + PCI_EXP_LNKCTL, > > + 0xFC, pcr->aspm_en); > > + } else if (option->dev_aspm_mode == DEV_ASPM_BACKDOOR) { > > + u8 mask = FORCE_ASPM_VAL_MASK | FORCE_ASPM_CTL0; > > + > > + rtsx_pci_write_register(pcr, ASPM_FORCE_CTL, mask, 0); > > + } > > + pcr->aspm_enabled = true; > > Place this above the: > > if (option->dev_aspm_mode == DEV_ASPM_DISABLE) > > .. line. Then you can remove the duplicate. > > > +} > > + > > +static void rts5249_disable_aspm(struct rtsx_pcr *pcr) { > > + struct rtsx_cr_option *option = &pcr->option; > > + > > + if (!pcr->aspm_enabled) > > + return; > > '\n' here. > > > + if (option->dev_aspm_mode == DEV_ASPM_DISABLE) { > > + pcr->aspm_enabled = false; > > + return; > > + } > > + if (option->dev_aspm_mode == DEV_ASPM_DYNAMIC) { > > + rtsx_pci_update_cfg_byte(pcr, > > + pcr->pcie_cap + PCI_EXP_LNKCTL, > > + 0xFC, 0); > > + } else if (option->dev_aspm_mode == DEV_ASPM_BACKDOOR) { > > + u8 mask = FORCE_ASPM_VAL_MASK | FORCE_ASPM_CTL0; > > + > > + rtsx_pci_write_register(pcr, ASPM_FORCE_CTL, > > + mask, FORCE_ASPM_CTL0); > > + } > > + pcr->aspm_enabled = false; > > Place this above the: > > if (option->dev_aspm_mode == DEV_ASPM_DISABLE) > > .. line. Then you can remove the duplicate. > > OR, better still. Since most of this code is the same as the function above, please amalgamate them and figure a way to get them to share code. Whether that's by using a function argument or something else. > > Since there are only 2 pieces of information that differ, I would create a child function which takes 3 arguments (pcr and the differences between the functions), which both enable_* and disable_* call. > > > +} > > + > > static const struct pcr_ops rts525a_pcr_ops = { > > .fetch_vendor_settings = rtsx_base_fetch_vendor_settings, > > .extra_init_hw = rts525a_extra_init_hw, @@ -576,11 +765,17 @@ 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, > > + .enable_aspm = rts5249_enable_aspm, > > + .disable_aspm = rts5249_disable_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..2ff398d 100644 > > --- a/drivers/mfd/rtsx_pcr.c > > +++ b/drivers/mfd/rtsx_pcr.c > > @@ -79,6 +79,142 @@ 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)); > > + rtsx_pci_write_register(pcr, MSGTXDATA1, > > + 0xFF, (u8)((latency >> 8) & 0xFF)); > > Define these. > > > + 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, 0xC0, 0xC0); > > + > > + 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); > > + > > + return 0; > > This code is unreachable. > > > +} > > + > > +static void rtsx_comm_disable_aspm(struct rtsx_pcr *pcr) { > > + struct rtsx_cr_option *option = &pcr->option; > > + > > + if (!pcr->aspm_enabled) > > + return; > > '\n' here. > > > + if (option->dev_aspm_mode == DEV_ASPM_DISABLE) { > > + pcr->aspm_enabled = false; > > + return; > > + } > > '\n' here. > > > + if (option->dev_aspm_mode == DEV_ASPM_DYNAMIC) { > > + rtsx_pci_disable_aspm(pcr); > > + } else if (option->dev_aspm_mode == DEV_ASPM_BACKDOOR) { > > + u8 mask = FORCE_ASPM_VAL_MASK; > > + > > + rtsx_pci_write_register(pcr, ASPM_FORCE_CTL, mask, 0); > > + } > > '\n' here. > > > + msleep(20); > > + pcr->aspm_enabled = false; > > +} > > + > > +static void rtsx_disable_aspm(struct rtsx_pcr *pcr) { > > + if (pcr->ops->disable_aspm) > > + pcr->ops->disable_aspm(pcr); > > + else > > + rtsx_comm_disable_aspm(pcr); > > +} > > + > > +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; > > + > > + 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) { > > + u8 val = option->ltr_l1off_snooze_sspwrgate; > > + > > + if (rtsx_check_dev_flag(pcr, > > + LTR_L1SS_PWR_GATE_CHECK_CARD_EN)) { > > + if (card_exist) > > + val &= ~(1 << 4); > > + else > > + val |= (1 << 4); > > Define these. > > > + } > > + > > + rtsx_set_l1off_sub(pcr, val); > > + } else { > > + rtsx_set_l1off_sub(pcr, 0); > > + } > > + } else { > > + /* l1off, latency: 300us */ > > + if (aspm_L1_2) { > > + u8 val = option->ltr_l1off_sspwrgate; > > + > > + if (rtsx_check_dev_flag(pcr, > > + LTR_L1SS_PWR_GATE_CHECK_CARD_EN)) { > > + if (card_exist) > > + val &= ~(1 << 4); > > + else > > + val |= (1 << 4); > > + } > > + > > + rtsx_set_l1off_sub(pcr, val); > > + } else { > > + rtsx_set_l1off_sub(pcr, 0); > > + } > > + } > > +} > > Again, there is a lot of duplication here. Any way these can be amalgamated? > > > +void rtsx_set_l1off_sub_cfg_d0(struct rtsx_pcr *pcr, int active) { > > + if (pcr->ops->set_l1off_cfg_sub_d0) > > + pcr->ops->set_l1off_cfg_sub_d0(pcr, active); > > + else > > + rtsx_comm_set_l1off_cfg_sub_d0(pcr, active); } > > + > > +static void rtsx_comm_pm_full_on(struct rtsx_pcr *pcr) { > > + struct rtsx_cr_option *option = &pcr->option; > > + > > + rtsx_disable_aspm(pcr); > > + > > + if (option->ltr_enabled) > > + rtsx_set_ltr_latency(pcr, option->ltr_active_latency); > > + > > + if (rtsx_check_dev_flag(pcr, LTR_L1SS_PWR_GATE_EN)) > > + rtsx_set_l1off_sub_cfg_d0(pcr, 1); > > +} > > + > > +void rtsx_pm_full_on(struct rtsx_pcr *pcr) { > > + if (pcr->ops->full_on) > > + pcr->ops->full_on(pcr); > > + else > > + rtsx_comm_pm_full_on(pcr); > > +} > > + > > void rtsx_pci_start_run(struct rtsx_pcr *pcr) { > > /* If pci device removed, don't queue idle work any more */ @@ -89,9 > > +225,7 @@ void rtsx_pci_start_run(struct rtsx_pcr *pcr) > > pcr->state = PDEV_STAT_RUN; > > if (pcr->ops->enable_auto_blink) > > pcr->ops->enable_auto_blink(pcr); > > - > > - if (pcr->aspm_en) > > - rtsx_pci_disable_aspm(pcr); > > + rtsx_pm_full_on(pcr); > > } > > > > mod_delayed_work(system_wq, &pcr->idle_work, msecs_to_jiffies(200)); > > @@ -958,6 +1092,63 @@ static int rtsx_pci_acquire_irq(struct rtsx_pcr *pcr) > > return 0; > > } > > > > +static void rtsx_comm_enable_aspm(struct rtsx_pcr *pcr) { > > + struct rtsx_cr_option *option = &pcr->option; > > + > > + if (pcr->aspm_enabled) > > + return; > > '\n' here. > > > + if (option->dev_aspm_mode == DEV_ASPM_DISABLE) { > > + pcr->aspm_enabled = true; > > + return; > > + } > > + > > + if (option->dev_aspm_mode == DEV_ASPM_DYNAMIC) { > > + rtsx_pci_enable_aspm(pcr); > > + } else if (option->dev_aspm_mode == DEV_ASPM_BACKDOOR) { > > + u8 mask = FORCE_ASPM_VAL_MASK; > > + u8 val = pcr->aspm_en; > > + > > + rtsx_pci_write_register(pcr, ASPM_FORCE_CTL, mask, val); > > + } > > + pcr->aspm_enabled = true; > > Same comments as the code above that looks the same. > > > +} > > + > > +static void rtsx_enable_aspm(struct rtsx_pcr *pcr) { > > + if (pcr->ops->enable_aspm) > > + pcr->ops->enable_aspm(pcr); > > + else > > + rtsx_comm_enable_aspm(pcr); > > +} > > + > > +static void rtsx_comm_pm_power_saving(struct rtsx_pcr *pcr) { > > + struct rtsx_cr_option *option = &pcr->option; > > + > > + if (option->ltr_enabled) { > > + u32 latency = option->ltr_l1off_latency; > > + > > + if (rtsx_check_dev_flag(pcr, L1_SNOOZE_TEST_EN)) > > + mdelay(option->l1_snooze_delay); > > + > > + rtsx_set_ltr_latency(pcr, latency); > > + } > > + > > + if (rtsx_check_dev_flag(pcr, LTR_L1SS_PWR_GATE_EN)) > > + rtsx_set_l1off_sub_cfg_d0(pcr, 0); > > + > > + rtsx_enable_aspm(pcr); > > +} > > + > > +void rtsx_pm_power_saving(struct rtsx_pcr *pcr) { > > + if (pcr->ops->power_saving) > > + pcr->ops->power_saving(pcr); > > + else > > + rtsx_comm_pm_power_saving(pcr); > > +} > > + > > static void rtsx_pci_idle_work(struct work_struct *work) { > > struct delayed_work *dwork = to_delayed_work(work); @@ -974,8 > > +1165,7 @@ static void rtsx_pci_idle_work(struct work_struct *work) > > if (pcr->ops->turn_off_led) > > pcr->ops->turn_off_led(pcr); > > > > - if (pcr->aspm_en) > > - rtsx_pci_enable_aspm(pcr); > > + rtsx_pm_power_saving(pcr); > > > > mutex_unlock(&pcr->pcr_mutex); > > } > > @@ -1063,6 +1253,11 @@ static int rtsx_pci_init_hw(struct rtsx_pcr *pcr) > > if (err < 0) > > return err; > > > > + if (CHK_PCI_PID(pcr, 0x5250) || > > + CHK_PCI_PID(pcr, 0x524A) || > > + CHK_PCI_PID(pcr, 0x525A)) > > + rtsx_pci_write_register(pcr, PM_CLK_FORCE_CTL, 1, 1); > > This should probably be a switch() statement with fall-through. > > > /* Enable clk_request_n to enable clock power management */ > > rtsx_pci_write_config_byte(pcr, pcr->pcie_cap + PCI_EXP_LNKCTL + 1, 1); > > /* Enter L1 when host tx idle */ > > diff --git a/drivers/mfd/rtsx_pcr.h b/drivers/mfd/rtsx_pcr.h index > > 931d1ae..d3baaec 100644 > > --- a/drivers/mfd/rtsx_pcr.h > > +++ b/drivers/mfd/rtsx_pcr.h > > @@ -32,6 +32,15 @@ > > #define RTS524A_PME_FORCE_CTL 0xFF78 > > #define RTS524A_PM_CTRL3 0xFF7E > > > > +#define LTR_ACTIVE_LATENCY_DEFAULT 0x883C > > +#define LTR_IDLE_LATENCY_DEFAULT 0x892C > > +#define LTR_L1OFF_LATENCY_DEFAULT 0x9003 > > +#define LTR_L1OFF_SSPWRGATE_5249_DEFAULT 0xAF > > +#define LTR_L1OFF_SSPWRGATE_5250_DEFAULT 0xFF > > +#define LTR_L1OFF_SNOOZE_SSPWRGATE_5249_DEFAULT 0xAC > > +#define LTR_L1OFF_SNOOZE_SSPWRGATE_5250_DEFAULT 0xF8 > > + > > + > > int __rtsx_pci_write_phy_register(struct rtsx_pcr *pcr, u8 addr, u16 > > val); int __rtsx_pci_read_phy_register(struct rtsx_pcr *pcr, u8 addr, > > u16 *val); > > > > @@ -85,5 +94,7 @@ static inline u8 map_sd_drive(int idx) > > > > /* generic operations */ > > int rtsx_gops_pm_reset(struct rtsx_pcr *pcr); > > +int rtsx_set_ltr_latency(struct rtsx_pcr *pcr, u32 latency); int > > +rtsx_set_l1off_sub(struct rtsx_pcr *pcr, u8 val); > > > > #endif > > diff --git a/include/linux/mfd/rtsx_pci.h > > b/include/linux/mfd/rtsx_pci.h index 116816f..62b50e4 100644 > > --- a/include/linux/mfd/rtsx_pci.h > > +++ b/include/linux/mfd/rtsx_pci.h > > @@ -621,6 +621,9 @@ > > > > #define AUTOLOAD_CFG_BASE 0xFF00 > > #define PETXCFG 0xFF03 > > +#define FORCE_CLKREQ_DELINK_MASK BIT(7) > > +#define FORCE_CLKREQ_LOW 0x80 > > +#define FORCE_CLKREQ_HIGH 0x00 > > > > #define PM_CTRL1 0xFF44 > > #define CD_RESUME_EN_MASK 0xF0 > > @@ -844,6 +847,9 @@ > > #define PHY_DIG1E_RX_EN_KEEP 0x0001 > > #define PHY_DUM_REG 0x1F > > > > +#define PCR_ASPM_SETTING_REG1 0x160 > > +#define PCR_ASPM_SETTING_REG2 0x168 > > + > > #define PCR_SETTING_REG1 0x724 > > #define PCR_SETTING_REG2 0x814 > > #define PCR_SETTING_REG3 0x747 > > @@ -876,14 +882,65 @@ struct pcr_ops { > > int (*conv_clk_and_div_n)(int clk, int dir); > > void (*fetch_vendor_settings)(struct rtsx_pcr *pcr); > > void (*force_power_down)(struct rtsx_pcr *pcr, u8 pm_state); > > + > > + void (*enable_aspm)(struct rtsx_pcr *pcr); > > + void (*disable_aspm)(struct rtsx_pcr *pcr); > > + int (*set_ltr_latency)(struct rtsx_pcr *pcr, u32 latency); > > + int (*set_l1off_sub)(struct rtsx_pcr *pcr, u8 val); > > + void (*set_l1off_cfg_sub_d0)(struct rtsx_pcr *pcr, int active); > > + void (*full_on)(struct rtsx_pcr *pcr); > > + void (*power_saving)(struct rtsx_pcr *pcr); > > }; > > > > enum PDEV_STAT {PDEV_STAT_IDLE, PDEV_STAT_RUN}; > > > > +#define ASPM_L1_1_EN_MASK BIT(3) > > +#define ASPM_L1_2_EN_MASK BIT(2) > > +#define PM_L1_1_EN_MASK BIT(1) > > +#define PM_L1_2_EN_MASK BIT(0) > > + > > +#define ASPM_L1_1_EN BIT(0) > > +#define ASPM_L1_2_EN BIT(1) > > +#define PM_L1_1_EN BIT(2) > > +#define PM_L1_2_EN BIT(3) > > +#define LTR_L1SS_PWR_GATE_EN BIT(4) > > +#define L1_SNOOZE_TEST_EN BIT(5) > > +#define LTR_L1SS_PWR_GATE_CHECK_CARD_EN BIT(6) > > + > > +enum dev_aspm_mode { > > + DEV_ASPM_DISABLE = 0, > > + DEV_ASPM_DYNAMIC, > > + DEV_ASPM_BACKDOOR, > > + DEV_ASPM_STATIC, > > +}; > > + > > +struct rtsx_cr_option { > > + u32 dev_flags; > > + bool force_clkreq_0; > > + bool ltr_en; > > + bool ltr_enabled; > > + bool ltr_active; > > + u32 ltr_active_latency; > > + u32 ltr_idle_latency; > > + u32 ltr_l1off_latency; > > + enum dev_aspm_mode dev_aspm_mode; > > + u32 l1_snooze_delay; > > + u8 ltr_l1off_sspwrgate; > > + u8 ltr_l1off_snooze_sspwrgate; > > +}; > > This still needs a kerneldoc header. > > > +#define rtsx_set_dev_flag(cr, flag) \ > > + ((cr)->option.dev_flags |= (flag)) > > +#define rtsx_clear_dev_flag(cr, flag) \ > > + ((cr)->option.dev_flags &= ~(flag)) > > +#define rtsx_check_dev_flag(cr, flag) \ > > + ((cr)->option.dev_flags & (flag)) > > + > > struct rtsx_pcr { > > struct pci_dev *pci; > > unsigned int id; > > int pcie_cap; > > + struct rtsx_cr_option option; > > > > /* pci resources */ > > unsigned long addr; > > @@ -940,6 +997,7 @@ struct rtsx_pcr { > > u8 card_drive_sel; > > #define ASPM_L1_EN 0x02 > > u8 aspm_en; > > + bool aspm_enabled; > > > > #define PCR_MS_PMOS (1 << 0) > > #define PCR_REVERSE_SOCKET (1 << 1) > > @@ -964,6 +1022,8 @@ struct rtsx_pcr { > > u8 dma_error_count; > > }; > > > > +#define PID_524A 0x524A > > + > > #define CHK_PCI_PID(pcr, pid) ((pcr)->pci->device == (pid)) > > #define PCI_VID(pcr) ((pcr)->pci->vendor) > > #define PCI_PID(pcr) ((pcr)->pci->device) >
-- Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog
| |