Re: [RFC PATCH v1 1/4] [PCI/ASPM:] Remove struct pcie_link_state.clkpm_default

[Bjorn Helgaas <helgaas@xxxxxxxxxx>]

On Wed, Sep 29, 2021 at 02:43:57AM +0200, Saheed O. Bolarinwa wrote:
> From: "Bolarinwa O. Saheed" <refactormyself@xxxxxxxxx>
> 
> The clkpm_default member of the struct pcie_link_state stores the
> value of the default clkpm state as it is in the BIOS.
> 
> This patch:
> - Removes clkpm_default from struct pcie_link_state
> - Creates pcie_get_clkpm_state() which return the clkpm state
>   obtained the BIOS
> - Replaces references to clkpm_default with call to
>   pcie_get_clkpm_state()
> 
> Signed-off-by: Bolarinwa O. Saheed <refactormyself@xxxxxxxxx>
> ---
>  drivers/pci/pcie/aspm.c | 37 +++++++++++++++++++++++++++----------
>  1 file changed, 27 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 013a47f587ce..c23da9a4e2fb 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -63,7 +63,6 @@ struct pcie_link_state {
>  	/* Clock PM state */
>  	u32 clkpm_capable:1;		/* Clock PM capable? */
>  	u32 clkpm_enabled:1;		/* Current Clock PM state */
> -	u32 clkpm_default:1;		/* Default Clock PM state by BIOS */
>  	u32 clkpm_disable:1;		/* Clock PM disabled */
>  
>  	/* Exit latencies */
> @@ -123,6 +122,30 @@ static int policy_to_aspm_state(struct pcie_link_state *link)
>  	return 0;
>  }
>  
> +static int pcie_get_clkpm_state(struct pci_dev *pdev)
> +{
> +	int enabled = 1;
> +	u32 reg32;
> +	u16 reg16;
> +	struct pci_dev *child;
> +	struct pci_bus *linkbus = pdev->subordinate;
> +
> +	/* All functions should have the same clkpm state, take the worst */
> +	list_for_each_entry(child, &linkbus->devices, bus_list) {
> +		pcie_capability_read_dword(child, PCI_EXP_LNKCAP, &reg32);
> +		if (!(reg32 & PCI_EXP_LNKCAP_CLKPM)) {
> +			enabled = 0;
> +			break;
> +		}
> +
> +		pcie_capability_read_word(child, PCI_EXP_LNKCTL, &reg16);
> +		if (!(reg16 & PCI_EXP_LNKCTL_CLKREQ_EN))
> +			enabled = 0;
> +	}
> +
> +	return enabled;
> +}
> +
>  static int policy_to_clkpm_state(struct pcie_link_state *link)
>  {
>  	switch (aspm_policy) {
> @@ -134,7 +157,7 @@ static int policy_to_clkpm_state(struct pcie_link_state *link)
>  		/* Enable Clock PM */
>  		return 1;
>  	case POLICY_DEFAULT:
> -		return link->clkpm_default;
> +		return pcie_get_clkpm_state(link->pdev);
>  	}
>  	return 0;
>  }
> @@ -168,9 +191,8 @@ static void pcie_set_clkpm(struct pcie_link_state *link, int enable)
>  
>  static void pcie_clkpm_cap_init(struct pcie_link_state *link, int blacklist)
>  {
> -	int capable = 1, enabled = 1;
> +	int capable = 1;
>  	u32 reg32;
> -	u16 reg16;
>  	struct pci_dev *child;
>  	struct pci_bus *linkbus = link->pdev->subordinate;
>  
> @@ -179,15 +201,10 @@ static void pcie_clkpm_cap_init(struct pcie_link_state *link, int blacklist)
>  		pcie_capability_read_dword(child, PCI_EXP_LNKCAP, &reg32);
>  		if (!(reg32 & PCI_EXP_LNKCAP_CLKPM)) {
>  			capable = 0;
> -			enabled = 0;
>  			break;
>  		}
> -		pcie_capability_read_word(child, PCI_EXP_LNKCTL, &reg16);
> -		if (!(reg16 & PCI_EXP_LNKCTL_CLKREQ_EN))
> -			enabled = 0;
>  	}
> -	link->clkpm_enabled = enabled;
> -	link->clkpm_default = enabled;
> +	link->clkpm_enabled = pcie_get_clkpm_state(link->pdev);

I love the idea of removing clkpm_default, but I need a little more
convincing.  Before this patch, this code computes clkpm_default from
PCI_EXP_LNKCAP_CLKPM and PCI_EXP_LNKCTL_CLKREQ_EN of all the functions
of the device.

PCI_EXP_LNKCAP_CLKPM is a read-only value, so we can re-read that any
time.  But PCI_EXP_LNKCTL_CLKREQ_EN is writable, so if we want to know
the value that firmware put there, we need to read and save it before
we modify it.

Why is it safe to remove this init-time read of
PCI_EXP_LNKCTL_CLKREQ_EN and instead re-read it any time we need the
"default" settings from firmware?

>  	link->clkpm_capable = capable;
>  	link->clkpm_disable = blacklist ? 1 : 0;
>  }
> -- 
> 2.20.1
>