| Message ID | 20200414203005.5166-4-jonathan.derrick@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | PCI Bridge Emulation changes for v5.8 | expand |
On Tue, Apr 14, 2020 at 04:30:03PM -0400, Jon Derrick wrote: > In order to make pci-bridge-emul easier to keep up-to-date with new PCIe > features, convert all named register bits to GENMASK and BIT pairs. This > patch doesn't alter any of the PCI configuration space as these bits are > fully defined. > > Signed-off-by: Jon Derrick <jonathan.derrick@intel.com> > --- > drivers/pci/pci-bridge-emul.c | 17 ++++++----------- > 1 file changed, 6 insertions(+), 11 deletions(-) > > diff --git a/drivers/pci/pci-bridge-emul.c b/drivers/pci/pci-bridge-emul.c > index c00c30ffb198..bbcccadca85e 100644 > --- a/drivers/pci/pci-bridge-emul.c > +++ b/drivers/pci/pci-bridge-emul.c > @@ -221,11 +221,8 @@ static const struct pci_bridge_reg_behavior pcie_cap_regs_behavior[] = { > * as reserved bits. > */ > .rw = GENMASK(12, 0), > - .w1c = (PCI_EXP_SLTSTA_ABP | PCI_EXP_SLTSTA_PFD | > - PCI_EXP_SLTSTA_MRLSC | PCI_EXP_SLTSTA_PDC | > - PCI_EXP_SLTSTA_CC | PCI_EXP_SLTSTA_DLLSC) << 16, > - .ro = (PCI_EXP_SLTSTA_MRLSS | PCI_EXP_SLTSTA_PDS | > - PCI_EXP_SLTSTA_EIS) << 16, > + .w1c = (BIT(8) | GENMASK(4, 0)) << 16, > + .ro = GENMASK(7, 5) << 16, FYI, I find the previous version a lot more readable. Or rather I find it readable while the new one looks like intentionally obsfucated garbage to me.
On Thu, 2020-04-16 at 00:30 -0700, Christoph Hellwig wrote: > On Tue, Apr 14, 2020 at 04:30:03PM -0400, Jon Derrick wrote: > > In order to make pci-bridge-emul easier to keep up-to-date with new PCIe > > features, convert all named register bits to GENMASK and BIT pairs. This > > patch doesn't alter any of the PCI configuration space as these bits are > > fully defined. > > > > Signed-off-by: Jon Derrick <jonathan.derrick@intel.com> > > --- > > drivers/pci/pci-bridge-emul.c | 17 ++++++----------- > > 1 file changed, 6 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/pci/pci-bridge-emul.c b/drivers/pci/pci-bridge-emul.c > > index c00c30ffb198..bbcccadca85e 100644 > > --- a/drivers/pci/pci-bridge-emul.c > > +++ b/drivers/pci/pci-bridge-emul.c > > @@ -221,11 +221,8 @@ static const struct pci_bridge_reg_behavior pcie_cap_regs_behavior[] = { > > * as reserved bits. > > */ > > .rw = GENMASK(12, 0), > > - .w1c = (PCI_EXP_SLTSTA_ABP | PCI_EXP_SLTSTA_PFD | > > - PCI_EXP_SLTSTA_MRLSC | PCI_EXP_SLTSTA_PDC | > > - PCI_EXP_SLTSTA_CC | PCI_EXP_SLTSTA_DLLSC) << 16, > > - .ro = (PCI_EXP_SLTSTA_MRLSS | PCI_EXP_SLTSTA_PDS | > > - PCI_EXP_SLTSTA_EIS) << 16, > > + .w1c = (BIT(8) | GENMASK(4, 0)) << 16, > > + .ro = GENMASK(7, 5) << 16, > > FYI, I find the previous version a lot more readable. Or rather I find > it readable while the new one looks like intentionally obsfucated > garbage to me. Well I guess that's entirely subjective. But I do think if all the existing BIT and GENMASK were converted to named registers instead, it would be a lot easier to overlook mistakes.
On Thu, Apr 16, 2020 at 02:35:59PM +0000, Derrick, Jonathan wrote: > On Thu, 2020-04-16 at 00:30 -0700, Christoph Hellwig wrote: > > On Tue, Apr 14, 2020 at 04:30:03PM -0400, Jon Derrick wrote: > > > In order to make pci-bridge-emul easier to keep up-to-date with new PCIe > > > features, convert all named register bits to GENMASK and BIT pairs. This > > > patch doesn't alter any of the PCI configuration space as these bits are > > > fully defined. > > > > > > Signed-off-by: Jon Derrick <jonathan.derrick@intel.com> > > > --- > > > drivers/pci/pci-bridge-emul.c | 17 ++++++----------- > > > 1 file changed, 6 insertions(+), 11 deletions(-) > > > > > > diff --git a/drivers/pci/pci-bridge-emul.c b/drivers/pci/pci-bridge-emul.c > > > index c00c30ffb198..bbcccadca85e 100644 > > > --- a/drivers/pci/pci-bridge-emul.c > > > +++ b/drivers/pci/pci-bridge-emul.c > > > @@ -221,11 +221,8 @@ static const struct pci_bridge_reg_behavior pcie_cap_regs_behavior[] = { > > > * as reserved bits. > > > */ > > > .rw = GENMASK(12, 0), > > > - .w1c = (PCI_EXP_SLTSTA_ABP | PCI_EXP_SLTSTA_PFD | > > > - PCI_EXP_SLTSTA_MRLSC | PCI_EXP_SLTSTA_PDC | > > > - PCI_EXP_SLTSTA_CC | PCI_EXP_SLTSTA_DLLSC) << 16, > > > - .ro = (PCI_EXP_SLTSTA_MRLSS | PCI_EXP_SLTSTA_PDS | > > > - PCI_EXP_SLTSTA_EIS) << 16, > > > + .w1c = (BIT(8) | GENMASK(4, 0)) << 16, > > > + .ro = GENMASK(7, 5) << 16, > > > > FYI, I find the previous version a lot more readable. Or rather I find > > it readable while the new one looks like intentionally obsfucated > > garbage to me. > > Well I guess that's entirely subjective. But I do think if all the > existing BIT and GENMASK were converted to named registers instead, it > would be a lot easier to overlook mistakes. Hi Jon, where are we with this patch ? If we drop it I assume you will have to rebase subsequent patches - I do think Christoph has a point here. Thanks, Lorenzo
On Mon, 2020-05-11 at 11:06 +0100, Lorenzo Pieralisi wrote: > On Thu, Apr 16, 2020 at 02:35:59PM +0000, Derrick, Jonathan wrote: > > On Thu, 2020-04-16 at 00:30 -0700, Christoph Hellwig wrote: > > > On Tue, Apr 14, 2020 at 04:30:03PM -0400, Jon Derrick wrote: > > > > In order to make pci-bridge-emul easier to keep up-to-date with new PCIe > > > > features, convert all named register bits to GENMASK and BIT pairs. This > > > > patch doesn't alter any of the PCI configuration space as these bits are > > > > fully defined. > > > > > > > > Signed-off-by: Jon Derrick <jonathan.derrick@intel.com> > > > > --- > > > > drivers/pci/pci-bridge-emul.c | 17 ++++++----------- > > > > 1 file changed, 6 insertions(+), 11 deletions(-) > > > > > > > > diff --git a/drivers/pci/pci-bridge-emul.c b/drivers/pci/pci-bridge-emul.c > > > > index c00c30ffb198..bbcccadca85e 100644 > > > > --- a/drivers/pci/pci-bridge-emul.c > > > > +++ b/drivers/pci/pci-bridge-emul.c > > > > @@ -221,11 +221,8 @@ static const struct pci_bridge_reg_behavior pcie_cap_regs_behavior[] = { > > > > * as reserved bits. > > > > */ > > > > .rw = GENMASK(12, 0), > > > > - .w1c = (PCI_EXP_SLTSTA_ABP | PCI_EXP_SLTSTA_PFD | > > > > - PCI_EXP_SLTSTA_MRLSC | PCI_EXP_SLTSTA_PDC | > > > > - PCI_EXP_SLTSTA_CC | PCI_EXP_SLTSTA_DLLSC) << 16, > > > > - .ro = (PCI_EXP_SLTSTA_MRLSS | PCI_EXP_SLTSTA_PDS | > > > > - PCI_EXP_SLTSTA_EIS) << 16, > > > > + .w1c = (BIT(8) | GENMASK(4, 0)) << 16, > > > > + .ro = GENMASK(7, 5) << 16, > > > > > > FYI, I find the previous version a lot more readable. Or rather I find > > > it readable while the new one looks like intentionally obsfucated > > > garbage to me. > > > > Well I guess that's entirely subjective. But I do think if all the > > existing BIT and GENMASK were converted to named registers instead, it > > would be a lot easier to overlook mistakes. > > Hi Jon, > > where are we with this patch ? If we drop it I assume you will have > to rebase subsequent patches - I do think Christoph has a point here. > > Thanks, > Lorenzo I'll post v2 without 3 in a bit
diff --git a/drivers/pci/pci-bridge-emul.c b/drivers/pci/pci-bridge-emul.c index c00c30ffb198..bbcccadca85e 100644 --- a/drivers/pci/pci-bridge-emul.c +++ b/drivers/pci/pci-bridge-emul.c @@ -221,11 +221,8 @@ static const struct pci_bridge_reg_behavior pcie_cap_regs_behavior[] = { * as reserved bits. */ .rw = GENMASK(12, 0), - .w1c = (PCI_EXP_SLTSTA_ABP | PCI_EXP_SLTSTA_PFD | - PCI_EXP_SLTSTA_MRLSC | PCI_EXP_SLTSTA_PDC | - PCI_EXP_SLTSTA_CC | PCI_EXP_SLTSTA_DLLSC) << 16, - .ro = (PCI_EXP_SLTSTA_MRLSS | PCI_EXP_SLTSTA_PDS | - PCI_EXP_SLTSTA_EIS) << 16, + .w1c = (BIT(8) | GENMASK(4, 0)) << 16, + .ro = GENMASK(7, 5) << 16, .rsvd = GENMASK(15, 13) | (GENMASK(15, 9) << 16), }, @@ -236,10 +233,8 @@ static const struct pci_bridge_reg_behavior pcie_cap_regs_behavior[] = { * * Root capabilities has bit 0 RO, the rest is reserved. */ - .rw = (PCI_EXP_RTCTL_SECEE | PCI_EXP_RTCTL_SENFEE | - PCI_EXP_RTCTL_SEFEE | PCI_EXP_RTCTL_PMEIE | - PCI_EXP_RTCTL_CRSSVE), - .ro = PCI_EXP_RTCAP_CRSVIS << 16, + .rw = GENMASK(4, 0), + .ro = BIT(0) << 16, .rsvd = GENMASK(15, 5) | (GENMASK(15, 1) << 16), }, @@ -248,8 +243,8 @@ static const struct pci_bridge_reg_behavior pcie_cap_regs_behavior[] = { * Root status has bits 17 and [15:0] RO, bit 16 W1C, the rest * is reserved. */ - .ro = GENMASK(15, 0) | PCI_EXP_RTSTA_PENDING, - .w1c = PCI_EXP_RTSTA_PME, + .ro = BIT(17) | GENMASK(15, 0), + .w1c = BIT(16), .rsvd = GENMASK(31, 18), }, };
In order to make pci-bridge-emul easier to keep up-to-date with new PCIe features, convert all named register bits to GENMASK and BIT pairs. This patch doesn't alter any of the PCI configuration space as these bits are fully defined. Signed-off-by: Jon Derrick <jonathan.derrick@intel.com> --- drivers/pci/pci-bridge-emul.c | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-)