| Message ID | 20230411111034.1473044-4-ajayagarwal@google.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | ASPM: aspm_disable/default/support state handling fixes | expand |
On Tue, Apr 11, 2023 at 04:40:34PM +0530, Ajay Agarwal wrote: > Currently the driver checks if ASPM_STATE_L1SS is supported > before calling aspm_calc_l1ss_info(), only for this function to > return if ASPM_STATE_L1_2_MASK is not supported. Simplify the > logic by directly checking for L1.2 mask. > > Signed-off-by: Ajay Agarwal <ajayagarwal@google.com> > --- > drivers/pci/pcie/aspm.c | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c > index 7c9935f331f1..8c45835e8016 100644 > --- a/drivers/pci/pcie/aspm.c > +++ b/drivers/pci/pcie/aspm.c > @@ -481,9 +481,6 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link, > u32 pctl1, pctl2, cctl1, cctl2; > u32 pl1_2_enables, cl1_2_enables; > > - if (!(link->aspm_support & ASPM_STATE_L1_2_MASK)) > - return; > - > /* Choose the greater of the two Port Common_Mode_Restore_Times */ > val1 = (parent_l1ss_cap & PCI_L1SS_CAP_CM_RESTORE_TIME) >> 8; > val2 = (child_l1ss_cap & PCI_L1SS_CAP_CM_RESTORE_TIME) >> 8; > @@ -616,7 +613,7 @@ static void aspm_l1ss_init(struct pcie_link_state *link) > if (parent_l1ss_ctl1 & child_l1ss_ctl1 & PCI_L1SS_CTL1_PCIPM_L1_2) > link->aspm_enabled |= ASPM_STATE_L1_2_PCIPM; > > - if (link->aspm_support & ASPM_STATE_L1SS) > + if (link->aspm_support & ASPM_STATE_L1_2_MASK) > aspm_calc_l1ss_info(link, parent_l1ss_cap, child_l1ss_cap); I think the reason it was this way is because several of the relevant names use "l1ss": ASPM_STATE_L1SS aspm_calc_l1ss_info calc_l1ss_pwron But everything in aspm_calc_l1ss_info() is L1.2-specific, so I think it would make sense to use your patch, and at the same time, rename aspm_calc_l1ss_info() and calc_l1ss_pwron() to aspm_calc_l12_info() and calc_l12_pwron() to match. Bjorn
On Mon, May 01, 2023 at 12:55:18PM -0500, Bjorn Helgaas wrote: > On Tue, Apr 11, 2023 at 04:40:34PM +0530, Ajay Agarwal wrote: > > Currently the driver checks if ASPM_STATE_L1SS is supported > > before calling aspm_calc_l1ss_info(), only for this function to > > return if ASPM_STATE_L1_2_MASK is not supported. Simplify the > > logic by directly checking for L1.2 mask. > > > > Signed-off-by: Ajay Agarwal <ajayagarwal@google.com> > > --- > > drivers/pci/pcie/aspm.c | 5 +---- > > 1 file changed, 1 insertion(+), 4 deletions(-) > > > > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c > > index 7c9935f331f1..8c45835e8016 100644 > > --- a/drivers/pci/pcie/aspm.c > > +++ b/drivers/pci/pcie/aspm.c > > @@ -481,9 +481,6 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link, > > u32 pctl1, pctl2, cctl1, cctl2; > > u32 pl1_2_enables, cl1_2_enables; > > > > - if (!(link->aspm_support & ASPM_STATE_L1_2_MASK)) > > - return; > > - > > /* Choose the greater of the two Port Common_Mode_Restore_Times */ > > val1 = (parent_l1ss_cap & PCI_L1SS_CAP_CM_RESTORE_TIME) >> 8; > > val2 = (child_l1ss_cap & PCI_L1SS_CAP_CM_RESTORE_TIME) >> 8; > > @@ -616,7 +613,7 @@ static void aspm_l1ss_init(struct pcie_link_state *link) > > if (parent_l1ss_ctl1 & child_l1ss_ctl1 & PCI_L1SS_CTL1_PCIPM_L1_2) > > link->aspm_enabled |= ASPM_STATE_L1_2_PCIPM; > > > > - if (link->aspm_support & ASPM_STATE_L1SS) > > + if (link->aspm_support & ASPM_STATE_L1_2_MASK) > > aspm_calc_l1ss_info(link, parent_l1ss_cap, child_l1ss_cap); > > I think the reason it was this way is because several of the relevant > names use "l1ss": > > ASPM_STATE_L1SS > aspm_calc_l1ss_info > calc_l1ss_pwron > > But everything in aspm_calc_l1ss_info() is L1.2-specific, so I think > it would make sense to use your patch, and at the same time, rename > aspm_calc_l1ss_info() and calc_l1ss_pwron() to aspm_calc_l12_info() > and calc_l12_pwron() to match. > > Bjorn Sure, will incorporate your suggestions in the next version.
diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c index 7c9935f331f1..8c45835e8016 100644 --- a/drivers/pci/pcie/aspm.c +++ b/drivers/pci/pcie/aspm.c @@ -481,9 +481,6 @@ static void aspm_calc_l1ss_info(struct pcie_link_state *link, u32 pctl1, pctl2, cctl1, cctl2; u32 pl1_2_enables, cl1_2_enables; - if (!(link->aspm_support & ASPM_STATE_L1_2_MASK)) - return; - /* Choose the greater of the two Port Common_Mode_Restore_Times */ val1 = (parent_l1ss_cap & PCI_L1SS_CAP_CM_RESTORE_TIME) >> 8; val2 = (child_l1ss_cap & PCI_L1SS_CAP_CM_RESTORE_TIME) >> 8; @@ -616,7 +613,7 @@ static void aspm_l1ss_init(struct pcie_link_state *link) if (parent_l1ss_ctl1 & child_l1ss_ctl1 & PCI_L1SS_CTL1_PCIPM_L1_2) link->aspm_enabled |= ASPM_STATE_L1_2_PCIPM; - if (link->aspm_support & ASPM_STATE_L1SS) + if (link->aspm_support & ASPM_STATE_L1_2_MASK) aspm_calc_l1ss_info(link, parent_l1ss_cap, child_l1ss_cap); }
Currently the driver checks if ASPM_STATE_L1SS is supported before calling aspm_calc_l1ss_info(), only for this function to return if ASPM_STATE_L1_2_MASK is not supported. Simplify the logic by directly checking for L1.2 mask. Signed-off-by: Ajay Agarwal <ajayagarwal@google.com> --- drivers/pci/pcie/aspm.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)