Message-ID: <20170821085351.ci7cvz3nkqvfmxst@dell>
Date:   Mon, 21 Aug 2017 09:53:51 +0100
From:   Lee Jones <lee.jones@...aro.org>
To:     rui_feng@...lsil.com.cn
Cc:     linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] mfd: Add support for RTS5250S power saving

On Mon, 21 Aug 2017, rui_feng@...lsil.com.cn wrote:

> From: rui_feng <rui_feng@...lsil.com.cn>

Please use the format "First Last" for you name.

> Signed-off-by: rui_feng <rui_feng@...lsil.com.cn>

Same.

You should also send patches using `git format patch` and `git send-email`.

> ---
>  drivers/mfd/rts5249.c        | 183 ++++++++++++++++++++++++++++++++++++++
>  drivers/mfd/rtsx_pcr.c       | 206 +++++++++++++++++++++++++++++++++++++++++--
>  drivers/mfd/rtsx_pcr.h       |   2 +
>  include/linux/mfd/rtsx_pci.h |  47 ++++++++++
>  4 files changed, 433 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/mfd/rts5249.c b/drivers/mfd/rts5249.c
> index 40f8bb1..a560b5a 100644
> --- a/drivers/mfd/rts5249.c
> +++ b/drivers/mfd/rts5249.c
> @@ -103,8 +103,70 @@ 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 cr_option *option = &(pcr->option);
> +	u32 lval;
> +
> +	if (CHK_PCI_PID(pcr, 0x524A))

No magic numbers please.

Almost all of the numbers below should be defined.

> +		rtsx_pci_read_config_dword(pcr, 0x160, &lval);
> +	else
> +		rtsx_pci_read_config_dword(pcr, 0x168, &lval);
> +
> +	if (lval & (1 << 3))

... including these.

> +		set_dev_flag(pcr, ASPM_L1_1_EN);
> +	else
> +		clear_dev_flag(pcr, ASPM_L1_1_EN);
> +	if (lval & (1 << 2))
> +		set_dev_flag(pcr, ASPM_L1_2_EN);
> +	else
> +		clear_dev_flag(pcr, ASPM_L1_2_EN);
> +
> +	if (lval & (1 << 1))
> +		set_dev_flag(pcr, PM_L1_1_EN);
> +	else
> +		clear_dev_flag(pcr, PM_L1_1_EN);
> +	if (lval & (1 << 0))
> +		set_dev_flag(pcr, PM_L1_2_EN);
> +	else
> +		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 = 1;
> +			option->ltr_active = 1;

These should be bool.  Then use 'true' instead of '1'.

Same for all the other 'yes/no' variables used here.

> +			rtsx_set_ltr_latency(pcr, option->ltr_active_latency);
> +		} else {
> +			option->ltr_enabled = 0;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int rts5249_init_from_hw(struct rtsx_pcr *pcr)
> +{
> +	struct cr_option *option = &(pcr->option);
> +
> +	if (check_dev_flag(pcr, ASPM_L1_1_EN | ASPM_L1_2_EN
> +				| PM_L1_1_EN | PM_L1_2_EN))
> +		option->force_clkreq_0 = 0;
> +	else
> +		option->force_clkreq_0 = 1;
> +
> +	return 0;
> +}
> +
>  static int rts5249_extra_init_hw(struct rtsx_pcr *pcr)
>  {
> +	struct 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 +187,14 @@ 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

The first line of a multi-line comment should be blank.

> +	 * If 1, CLKREQ# PIN will be forced to drive 0 to request clock

"If 1"?  That's a very ambiguous comment.

Leave coding nuances out of comments.  Just use plain English.

> +	 */
> +	if (option->force_clkreq_0)
> +		rtsx_pci_add_cmd(pcr, WRITE_REG_CMD, PETXCFG, 0x80, 0x80);
> +	else
> +		rtsx_pci_add_cmd(pcr, WRITE_REG_CMD, PETXCFG, 0x80, 0x00);

More magic numbers to define.

>  	return rtsx_pci_send_cmd(pcr, 100);
>  }
>  
> @@ -353,6 +423,8 @@ static int rtsx_base_switch_output_voltage(struct rtsx_pcr *pcr, u8 voltage)
>  
>  void rts5249_init_params(struct rtsx_pcr *pcr)
>  {
> +	struct 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 +444,19 @@ 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;
> +	option->dev_flags |= LTR_L1SS_PWR_GATE_EN;

It's more common to do both of these using a single statement.

> +	option->ltr_en = 1;

Bool.

> +	/* init latency of active, idle, L1OFF to 60us, 300us, 3ms */
> +	/* 60us:0x883C, 300us:0x892C, 3ms:0x9003 */

No need for this comment, just define them all.

> +	option->ltr_active_latency = 0x883C;
> +	option->ltr_idle_latency = 0x892C;
> +	option->ltr_l1off_latency = 0x9003;
> +	option->dev_aspm_mode = DEV_ASPM_DYNAMIC;
> +	option->l1_snooze_delay = 1;
> +	option->ltr_l1off_sspwrgate = 0xAF;
> +	option->ltr_l1off_snooze_sspwrgate = 0xAC;
>  }
>  
>  static int rts524a_write_phy(struct rtsx_pcr *pcr, u8 addr, u16 val)
> @@ -459,6 +544,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 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 = check_dev_flag(pcr, ASPM_L1_1_EN);
> +	aspm_L1_2 = 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 (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);
> +		}
> +	} else {
> +		/* l1off, latency: 300us */
> +		if (aspm_L1_2) {
> +			u8 val = option->ltr_l1off_sspwrgate;
> +
> +			if (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 +606,14 @@ 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 = 0xFF;
> +	pcr->option.ltr_l1off_snooze_sspwrgate = 0xF8;
>  
>  	pcr->reg_pm_ctrl3 = RTS524A_PM_CTRL3;
>  	pcr->ops = &rts524a_pcr_ops;
> @@ -564,6 +700,48 @@ static int rts525a_extra_init_hw(struct rtsx_pcr *pcr)
>  	return 0;
>  }
>  
> +static void rts5249_enable_aspm(struct rtsx_pcr *pcr)
> +{
> +	struct cr_option *option = &pcr->option;
> +
> +	if (!pcr->aspm_enabled) {

If you negate this and just return, you can save yourself from a tab.

> +		if (option->dev_aspm_mode != DEV_ASPM_DISABLE) {

Again, negate this and if true, set pcr->aspm_enabled and leave.

> +			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 = 1;
> +	}
> +}
> +
> +static void rts5249_disable_aspm(struct rtsx_pcr *pcr)
> +{
> +	struct cr_option *option = &pcr->option;
> +
> +	if (pcr->aspm_enabled) {
> +		if (option->dev_aspm_mode != DEV_ASPM_DISABLE) {
> +			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 = 0;
> +	}
> +}

Same logic here.

>  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 +754,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,
> +	.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 = 0xFF;
> +	pcr->option.ltr_l1off_snooze_sspwrgate = 0xF8;
>  
>  	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..59af7c2 100644
> --- a/drivers/mfd/rtsx_pcr.c
> +++ b/drivers/mfd/rtsx_pcr.c
> @@ -79,6 +79,143 @@ 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));
> +	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;
> +}
> +
> +static void rtsx_comm_disable_aspm(struct rtsx_pcr *pcr)
> +{
> +	struct cr_option *option = &pcr->option;
> +
> +	if (pcr->aspm_enabled) {
> +		if (option->dev_aspm_mode != DEV_ASPM_DISABLE) {
> +			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);
> +			}
> +			msleep(20);
> +		}
> +		pcr->aspm_enabled = 0;
> +	}
> +}

And again.

> +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);

'\n' here.

> +	return 0;
> +}
> +
> +static void rtsx_comm_set_l1off_cfg_sub_D0(struct rtsx_pcr *pcr, int active)

Keep to lower case for function names.

> +{
> +	struct 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 = check_dev_flag(pcr, ASPM_L1_1_EN);
> +	aspm_L1_2 = 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 (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);
> +		}
> +	} else {
> +		/* l1off, latency: 300us */
> +		if (aspm_L1_2) {
> +			u8 val = option->ltr_l1off_sspwrgate;
> +
> +			if (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);
> +		}
> +	}
> +}
> +
> +void rtsx_set_l1off_sub_cfg_D0(struct rtsx_pcr *pcr, int active)

Here too (and everywhere else).

> +{
> +	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 cr_option *option = &pcr->option;
> +
> +	rtsx_disable_aspm(pcr);
> +
> +	if (option->ltr_enabled) {
> +		u32 latency = option->ltr_active_latency;

No need for this variable.

> +		rtsx_set_ltr_latency(pcr, latency);
> +	}
> +
> +	if (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 +226,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 +1093,63 @@ static int rtsx_pci_acquire_irq(struct rtsx_pcr *pcr)
>  	return 0;
>  }
>  
> +static void rtsx_comm_enable_aspm(struct rtsx_pcr *pcr)
> +{
> +	struct cr_option *option = &pcr->option;
> +
> +	if (!pcr->aspm_enabled) {
> +		if (option->dev_aspm_mode != DEV_ASPM_DISABLE) {
> +			u8 val = 0;
> +
> +			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;
> +
> +				val = pcr->aspm_en;
> +				rtsx_pci_write_register(pcr, ASPM_FORCE_CTL,
> +							mask, val);
> +			}
> +		}
> +		pcr->aspm_enabled = 1;
> +	}
> +}

Use the name logic as before.

Are any of these functions duplicated, they look very similar?

> +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 cr_option *option = &pcr->option;
> +
> +	if (option->ltr_enabled) {
> +		u32 latency = option->ltr_l1off_latency;
> +
> +		if (check_dev_flag(pcr, L1_SNOOZE_TEST_EN))
> +			mdelay(option->l1_snooze_delay);
> +
> +		rtsx_set_ltr_latency(pcr, latency);
> +	}
> +
> +	if (check_dev_flag(pcr, LTR_L1SS_PWR_GATE_EN))
> +		rtsx_set_l1off_sub_cfg_D0(pcr, 0);
> +
> +	rtsx_enable_aspm(pcr);
> +}

Here too.

> +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 +1166,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 +1254,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);
> +
>  	/* 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..544f82b 100644
> --- a/drivers/mfd/rtsx_pcr.h
> +++ b/drivers/mfd/rtsx_pcr.h
> @@ -85,5 +85,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..f7efb0f 100644
> --- a/include/linux/mfd/rtsx_pci.h
> +++ b/include/linux/mfd/rtsx_pci.h
> @@ -876,14 +876,60 @@ 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			(1 << 0)
> +#define ASPM_L1_2_EN			(1 << 1)
> +#define PM_L1_1_EN			(1 << 2)
> +#define PM_L1_2_EN			(1 << 3)
> +#define LTR_L1SS_PWR_GATE_EN            (1 << 4)
> +#define L1_SNOOZE_TEST_EN		(1 << 5)
> +#define LTR_L1SS_PWR_GATE_CHECK_CARD_EN	(1 << 6)

Use BIT()

> +enum dev_aspm_mode {
> +	DEV_ASPM_DISABLE = 0,
> +	DEV_ASPM_DYNAMIC = 1,
> +	DEV_ASPM_BACKDOOR = 2,
> +	DEV_ASPM_STATIC	= 3,

You only need to initialise the first entry.

> +};
> +
> +struct cr_option {
> +	u32 dev_flags;
> +	int force_clkreq_0;
> +	int ltr_en;
> +	int ltr_enabled;
> +	int 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;
> +};

You need to author a kerneldoc header for this structure.

> +#define set_dev_flag(cr, flag) \
> +	((cr)->option.dev_flags |= (flag))
> +#define clear_dev_flag(cr, flag) \
> +	((cr)->option.dev_flags &= ~(flag))
> +#define check_dev_flag(cr, flag) \
> +	((cr)->option.dev_flags & (flag))

These are very generic sounding.

Prefix with rtsx_*.

>  struct rtsx_pcr {
>  	struct pci_dev			*pci;
>  	unsigned int			id;
>  	int				pcie_cap;
> +	struct cr_option option;

Tabbing.

>  	/* pci resources */
>  	unsigned long			addr;
> @@ -940,6 +986,7 @@ struct rtsx_pcr {
>  	u8				card_drive_sel;
>  #define ASPM_L1_EN			0x02
>  	u8				aspm_en;
> +	int				aspm_enabled;
>  
>  #define PCR_MS_PMOS			(1 << 0)
>  #define PCR_REVERSE_SOCKET		(1 << 1)

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

Powered by blists - more mailing lists