Re: [PATCH v5 1/3] PCI/ASPM: Add helper to encode L1SS T_POWER_ON fields

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 4418 bytes --]

On Tue, 28 Apr 2026, Krishna Chaitanya Chundru wrote:

> Add a shared helper to encode the PCIe L1 PM Substates T_POWER_ON
> parameter into the T_POWER_ON Scale and T_POWER_ON Value fields.
> 
> This helper can be used by the controller drivers to change the
> default/wrong value of T_POWER_ON in L1ss capability register to
> avoid incorrect calculation of LTR_L1.2_THRESHOLD value.
> 
> The helper converts a T_POWER_ON time specified in microseconds into
> the appropriate scale/value encoding defined by the PCIe spec r7.0,
> sec 7.8.3.2. Values that exceed the maximum encodable range are clamped
> to the largest representable encoding.
> 
> Tested-by: Shawn Lin <shawn.lin@rock-chips.com>
> Reviewed-by: Shawn Lin <shawn.lin@rock-chips.com>
> Signed-off-by: Krishna Chaitanya Chundru <krishna.chundru@oss.qualcomm.com>
> ---
>  drivers/pci/pci.h       |  6 ++++++
>  drivers/pci/pcie/aspm.c | 40 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 46 insertions(+)
> 
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 4a14f88e543a..c379befe1ebe 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -1110,6 +1110,7 @@ void pcie_aspm_pm_state_change(struct pci_dev *pdev, bool locked);
>  void pcie_aspm_powersave_config_link(struct pci_dev *pdev);
>  void pci_configure_ltr(struct pci_dev *pdev);
>  void pci_bridge_reconfigure_ltr(struct pci_dev *pdev);
> +void pcie_encode_t_power_on(u16 t_power_on_us, u8 *scale, u8 *value);
>  #else
>  static inline void pcie_aspm_remove_cap(struct pci_dev *pdev, u32 lnkcap) { }
>  static inline void pcie_aspm_init_link_state(struct pci_dev *pdev) { }
> @@ -1118,6 +1119,11 @@ static inline void pcie_aspm_pm_state_change(struct pci_dev *pdev, bool locked)
>  static inline void pcie_aspm_powersave_config_link(struct pci_dev *pdev) { }
>  static inline void pci_configure_ltr(struct pci_dev *pdev) { }
>  static inline void pci_bridge_reconfigure_ltr(struct pci_dev *pdev) { }
> +static inline void pcie_encode_t_power_on(u16 t_power_on_us, u8 *scale, u8 *value)
> +{
> +	*scale = 0;
> +	*value = 0;
> +}
>  #endif
>  
>  #ifdef CONFIG_PCIE_ECRC
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 925373b98dff..457d469b8d49 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -525,6 +525,46 @@ static u32 calc_l12_pwron(struct pci_dev *pdev, u32 scale, u32 val)
>  	return 0;
>  }
>  
> +/**
> + * pcie_encode_t_power_on - Encode T_POWER_ON into scale and value fields
> + * @t_power_on_us: T_POWER_ON time in microseconds
> + * @scale: Encoded T_POWER_ON Scale (0..2)
> + * @value: Encoded T_POWER_ON Value
> + *
> + * T_POWER_ON is encoded as:
> + *   T_POWER_ON(us) = scale_unit(us) * value
> + *
> + * where scale_unit is selected by @scale:
> + *   0: 2us
> + *   1: 10us
> + *   2: 100us
> + *
> + * If @t_power_on_us exceeds the maximum representable value, the result
> + * is clamped to the largest encodable T_POWER_ON.
> + *
> + * See PCIe r7.0, sec 7.8.3.2.
> + */
> +void pcie_encode_t_power_on(u16 t_power_on_us, u8 *scale, u8 *value)

Hi,

I don't know how the type for t_power_on_us was picked but if it was 
arbitrary decision, I suggest you just go with 32-bit input.

That would also remove the u32 -> u16 truncate done in the other patches 
of your series which would potentially corrupt the number (I assume 
numbers that big would be invalid but they could alias to small u16 
numbers).

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

> +{
> +	u8 maxv = FIELD_MAX(PCI_L1SS_CAP_P_PWR_ON_VALUE);
> +
> +	/* T_POWER_ON_Value ("value") is a 5-bit field with max value of 31. */
> +	if (t_power_on_us <= 2 * maxv) {
> +		*scale = 0; /* Value times 2us */
> +		*value = DIV_ROUND_UP(t_power_on_us, 2);
> +	} else if (t_power_on_us <= 10 * maxv) {
> +		*scale = 1; /* Value times 10us */
> +		*value = DIV_ROUND_UP(t_power_on_us, 10);
> +	} else if (t_power_on_us <= 100 * maxv) {
> +		*scale = 2; /* value times 100us */
> +		*value = DIV_ROUND_UP(t_power_on_us, 100);
> +	} else {
> +		*scale = 2;
> +		*value = maxv;
> +	}
> +}
> +EXPORT_SYMBOL(pcie_encode_t_power_on);
> +
>  /*
>   * Encode an LTR_L1.2_THRESHOLD value for the L1 PM Substates Control 1
>   * register.  Ports enter L1.2 when the most recent LTR value is greater
> 
> 

-- 
 i.