On Thu, Sep 30, 2021 at 5:54 PM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
>
> 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, ®32);
> > + if (!(reg32 & PCI_EXP_LNKCAP_CLKPM)) {
> > + enabled = 0;
> > + break;
> > + }
> > +
> > + pcie_capability_read_word(child, PCI_EXP_LNKCTL, ®16);
> > + 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, ®32);
> > if (!(reg32 & PCI_EXP_LNKCAP_CLKPM)) {
> > capable = 0;
> > - enabled = 0;
> > break;
> > }
> > - pcie_capability_read_word(child, PCI_EXP_LNKCTL, ®16);
> > - 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
> >
