diff mbox series

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

Message ID 20210929004400.25717-2-refactormyself@gmail.com (mailing list archive)
State Superseded
Delegated to: Bjorn Helgaas
Headers show
Series Remove struct pcie_link_state.clkpm_* | expand

Commit Message

Saheed O. Bolarinwa Sept. 29, 2021, 12:43 a.m. UTC 
From: "Bolarinwa O. Saheed" <refactormyself@gmail.com>

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@gmail.com>
---
 drivers/pci/pcie/aspm.c | 37 +++++++++++++++++++++++++++----------
 1 file changed, 27 insertions(+), 10 deletions(-)

Comments

Bjorn Helgaas Sept. 30, 2021, 3:54 p.m. UTC | #1 
On Wed, Sep 29, 2021 at 02:43:57AM +0200, Saheed O. Bolarinwa wrote:
> From: "Bolarinwa O. Saheed" <refactormyself@gmail.com>
> 
> 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@gmail.com>
> ---
>  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
>
Saheed O. Bolarinwa Oct. 11, 2021, 6:05 a.m. UTC | #2 
On Thu, Sep 30, 2021 at 5:54 PM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Wed, Sep 29, 2021 at 02:43:57AM +0200, Saheed O. Bolarinwa wrote:
> > From: "Bolarinwa O. Saheed" <refactormyself@gmail.com>
> >
> > 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@gmail.com>
> > ---
> >  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?
After another look, it "may" not be safe, but then clkpm_default
should be documented
as /* Clock PM state at last boot */ because I don't think it is the
*default* state. Please,
let me know what I missing, I list below my reasons: (pardon the repetitions)

- I agree that clkpm_default current stores the boot time value.
- currently, the value of clkpm_default reflect the value of
PCI_EXP_LNKCAP_CLKPM
   (read-only) and PCI_EXP_LNKCTL_CLKREQ_EN (writable)
- calling pcie_set_clkpm() can change the "default" state in the
firmware and it is stored
   in clkpm_enable until the next boot, when clkpm_enable = clkpm_default
- if the "default" state is changed after boot then its initial value
stored in clkpm_default is
  lost at reboot.
- IMO the intention of calling pcie_set_clkpm() is to set the default
value on the firmware.
  We may need another function to set clkpm to a *temporary state*
that may at any time
  be different from the value in the firmware.
- Currently, clkpm_enable always reflect the value in the firmware and
the may be different
  from the value of clkpm_default.
- If clkpm_default does not reflect the value in the firmware after
boot, it feels to me like it is
  not the *default* value
- I also think that clkpm_enabled is supposed to be a sort of
*temporary/current state* that
  may or may not be stored as the default. Although, I am not sure why
it will be needed!


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

Patch

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);
 	link->clkpm_capable = capable;
 	link->clkpm_disable = blacklist ? 1 : 0;
 }