On Tue, 22 Aug 2017, rui_f...@realsil.com.cn wrote:

> From: Rui Feng <rui_f...@realsil.com.cn>
> 
> Signed-off-by: Rui Feng <rui_f...@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

Reply via email to