| Message ID | 20200506151429.12255-4-kishon@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add PCIe support to TI's J721E SoC | expand |
On Wed, 6 May 2020 20:44:18 +0530, Kishon Vijay Abraham I wrote: > Add support to use custom read and write accessors. Platforms that > don't support half word or byte access or any other constraint > while accessing registers can use this feature to populate custom > read and write accessors. These custom accessors are used for both > standard register access and configuration space register access of > the PCIe host bridge. > > Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> > --- > drivers/pci/controller/cadence/pcie-cadence.h | 107 +++++++++++++++--- > 1 file changed, 94 insertions(+), 13 deletions(-) > Reviewed-by: Rob Herring <robh@kernel.org>
On Wed, May 06, 2020 at 08:44:18PM +0530, Kishon Vijay Abraham I wrote: > Add support to use custom read and write accessors. Platforms that > don't support half word or byte access or any other constraint > while accessing registers can use this feature to populate custom > read and write accessors. These custom accessors are used for both > standard register access and configuration space register access of > the PCIe host bridge. > > Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> > --- > drivers/pci/controller/cadence/pcie-cadence.h | 107 +++++++++++++++--- > 1 file changed, 94 insertions(+), 13 deletions(-) Actually, take back my R-by... > > diff --git a/drivers/pci/controller/cadence/pcie-cadence.h b/drivers/pci/controller/cadence/pcie-cadence.h > index df14ad002fe9..70b6b25153e8 100644 > --- a/drivers/pci/controller/cadence/pcie-cadence.h > +++ b/drivers/pci/controller/cadence/pcie-cadence.h > @@ -223,6 +223,11 @@ enum cdns_pcie_msg_routing { > MSG_ROUTING_GATHER, > }; > > +struct cdns_pcie_ops { > + u32 (*read)(void __iomem *addr, int size); > + void (*write)(void __iomem *addr, int size, u32 value); > +}; > + > /** > * struct cdns_pcie - private data for Cadence PCIe controller drivers > * @reg_base: IO mapped register base > @@ -239,7 +244,7 @@ struct cdns_pcie { > int phy_count; > struct phy **phy; > struct device_link **link; > - const struct cdns_pcie_common_ops *ops; > + const struct cdns_pcie_ops *ops; > }; > > /** > @@ -299,69 +304,145 @@ struct cdns_pcie_ep { > /* Register access */ > static inline void cdns_pcie_writeb(struct cdns_pcie *pcie, u32 reg, u8 value) > { > - writeb(value, pcie->reg_base + reg); > + void __iomem *addr = pcie->reg_base + reg; > + > + if (pcie->ops && pcie->ops->write) { > + pcie->ops->write(addr, 0x1, value); > + return; > + } > + > + writeb(value, addr); > } > > static inline void cdns_pcie_writew(struct cdns_pcie *pcie, u32 reg, u16 value) > { > - writew(value, pcie->reg_base + reg); > + void __iomem *addr = pcie->reg_base + reg; > + > + if (pcie->ops && pcie->ops->write) { > + pcie->ops->write(addr, 0x2, value); > + return; > + } > + > + writew(value, addr); > } cdns_pcie_writeb and cdns_pcie_writew are used, so remove them. > > static inline void cdns_pcie_writel(struct cdns_pcie *pcie, u32 reg, u32 value) > { > - writel(value, pcie->reg_base + reg); > + void __iomem *addr = pcie->reg_base + reg; > + > + if (pcie->ops && pcie->ops->write) { > + pcie->ops->write(addr, 0x4, value); > + return; > + } > + > + writel(value, addr); writel isn't broken for you, so you don't need this either. > } > > static inline u32 cdns_pcie_readl(struct cdns_pcie *pcie, u32 reg) > { > - return readl(pcie->reg_base + reg); > + void __iomem *addr = pcie->reg_base + reg; > + > + if (pcie->ops && pcie->ops->read) > + return pcie->ops->read(addr, 0x4); > + > + return readl(addr); And neither is readl. > } > > /* Root Port register access */ > static inline void cdns_pcie_rp_writeb(struct cdns_pcie *pcie, > u32 reg, u8 value) > { > - writeb(value, pcie->reg_base + CDNS_PCIE_RP_BASE + reg); > + void __iomem *addr = pcie->reg_base + CDNS_PCIE_RP_BASE + reg; > + > + if (pcie->ops && pcie->ops->write) { > + pcie->ops->write(addr, 0x1, value); > + return; > + } > + > + writeb(value, addr); > } > > static inline void cdns_pcie_rp_writew(struct cdns_pcie *pcie, > u32 reg, u16 value) > { > - writew(value, pcie->reg_base + CDNS_PCIE_RP_BASE + reg); > + void __iomem *addr = pcie->reg_base + CDNS_PCIE_RP_BASE + reg; > + > + if (pcie->ops && pcie->ops->write) { > + pcie->ops->write(addr, 0x2, value); > + return; > + } > + > + writew(value, addr); You removed 2 out of 3 calls to this. I think I'd just make the root port writes always be 32-bit. It is all just one time init stuff anyways. Either rework the calls to assemble the data into 32-bits or keep these functions and do the RMW here. > } > > /* Endpoint Function register access */ > static inline void cdns_pcie_ep_fn_writeb(struct cdns_pcie *pcie, u8 fn, > u32 reg, u8 value) > { > - writeb(value, pcie->reg_base + CDNS_PCIE_EP_FUNC_BASE(fn) + reg); > + void __iomem *addr = pcie->reg_base + CDNS_PCIE_EP_FUNC_BASE(fn) + reg; > + > + if (pcie->ops && pcie->ops->write) { > + pcie->ops->write(addr, 0x1, value); > + return; > + } > + > + writeb(value, addr); Same for these EP functions. Unless there are places where doing a RMW is fundamentally broken like in config space (not counting the one time init stuff). Rob
Hi Rob, On 5/21/2020 3:37 AM, Rob Herring wrote: > On Wed, May 06, 2020 at 08:44:18PM +0530, Kishon Vijay Abraham I wrote: >> Add support to use custom read and write accessors. Platforms that >> don't support half word or byte access or any other constraint >> while accessing registers can use this feature to populate custom >> read and write accessors. These custom accessors are used for both >> standard register access and configuration space register access of >> the PCIe host bridge. >> >> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> >> --- >> drivers/pci/controller/cadence/pcie-cadence.h | 107 +++++++++++++++--- >> 1 file changed, 94 insertions(+), 13 deletions(-) > > Actually, take back my R-by... > >> >> diff --git a/drivers/pci/controller/cadence/pcie-cadence.h b/drivers/pci/controller/cadence/pcie-cadence.h >> index df14ad002fe9..70b6b25153e8 100644 >> --- a/drivers/pci/controller/cadence/pcie-cadence.h >> +++ b/drivers/pci/controller/cadence/pcie-cadence.h >> @@ -223,6 +223,11 @@ enum cdns_pcie_msg_routing { >> MSG_ROUTING_GATHER, >> }; >> >> +struct cdns_pcie_ops { >> + u32 (*read)(void __iomem *addr, int size); >> + void (*write)(void __iomem *addr, int size, u32 value); >> +}; >> + >> /** >> * struct cdns_pcie - private data for Cadence PCIe controller drivers >> * @reg_base: IO mapped register base >> @@ -239,7 +244,7 @@ struct cdns_pcie { >> int phy_count; >> struct phy **phy; >> struct device_link **link; >> - const struct cdns_pcie_common_ops *ops; >> + const struct cdns_pcie_ops *ops; >> }; >> >> /** >> @@ -299,69 +304,145 @@ struct cdns_pcie_ep { >> /* Register access */ >> static inline void cdns_pcie_writeb(struct cdns_pcie *pcie, u32 reg, u8 value) >> { >> - writeb(value, pcie->reg_base + reg); >> + void __iomem *addr = pcie->reg_base + reg; >> + >> + if (pcie->ops && pcie->ops->write) { >> + pcie->ops->write(addr, 0x1, value); >> + return; >> + } >> + >> + writeb(value, addr); >> } >> >> static inline void cdns_pcie_writew(struct cdns_pcie *pcie, u32 reg, u16 value) >> { >> - writew(value, pcie->reg_base + reg); >> + void __iomem *addr = pcie->reg_base + reg; >> + >> + if (pcie->ops && pcie->ops->write) { >> + pcie->ops->write(addr, 0x2, value); >> + return; >> + } >> + >> + writew(value, addr); >> } > > cdns_pcie_writeb and cdns_pcie_writew are used, so remove them. > >> >> static inline void cdns_pcie_writel(struct cdns_pcie *pcie, u32 reg, u32 value) >> { >> - writel(value, pcie->reg_base + reg); >> + void __iomem *addr = pcie->reg_base + reg; >> + >> + if (pcie->ops && pcie->ops->write) { >> + pcie->ops->write(addr, 0x4, value); >> + return; >> + } >> + >> + writel(value, addr); > > writel isn't broken for you, so you don't need this either. > >> } >> >> static inline u32 cdns_pcie_readl(struct cdns_pcie *pcie, u32 reg) >> { >> - return readl(pcie->reg_base + reg); >> + void __iomem *addr = pcie->reg_base + reg; >> + >> + if (pcie->ops && pcie->ops->read) >> + return pcie->ops->read(addr, 0x4); >> + >> + return readl(addr); > > And neither is readl. Sure, I'll remove all the unused functions and avoid using ops for readl and writel. > >> } >> >> /* Root Port register access */ >> static inline void cdns_pcie_rp_writeb(struct cdns_pcie *pcie, >> u32 reg, u8 value) >> { >> - writeb(value, pcie->reg_base + CDNS_PCIE_RP_BASE + reg); >> + void __iomem *addr = pcie->reg_base + CDNS_PCIE_RP_BASE + reg; >> + >> + if (pcie->ops && pcie->ops->write) { >> + pcie->ops->write(addr, 0x1, value); >> + return; >> + } >> + >> + writeb(value, addr); >> } >> >> static inline void cdns_pcie_rp_writew(struct cdns_pcie *pcie, >> u32 reg, u16 value) >> { >> - writew(value, pcie->reg_base + CDNS_PCIE_RP_BASE + reg); >> + void __iomem *addr = pcie->reg_base + CDNS_PCIE_RP_BASE + reg; >> + >> + if (pcie->ops && pcie->ops->write) { >> + pcie->ops->write(addr, 0x2, value); >> + return; >> + } >> + >> + writew(value, addr); > > You removed 2 out of 3 calls to this. I think I'd just make the root > port writes always be 32-bit. It is all just one time init stuff > anyways. > > Either rework the calls to assemble the data into 32-bits or keep these > functions and do the RMW here. The problem with assembling data into 32-bits is we have to read/write with different offsets. We'll give PCI_VENDOR_ID offset for modifying deviceID, PCI_INTERRUPT_LINE for modifying INTERRUPT_PIN which might get non-intuitive. Similarly in endpoint we read and write to MSI_FLAGS (which is at offset 2) we have to directly use MSI capability offset. And doing RMW in the accessors would mean the same RMW op is repeated. So if we just have cdns_pcie_rp_writeb() and cdns_pcie_rp_writew(), the same code will be repeated here twice. IMHO using ops is a lot cleaner for these cases. IMHO except for removing unused functions and not changing readl/writel, others should use ops. Kindly let me know what you think. Thanks Kishon
On Thu, May 21, 2020 at 7:33 AM Kishon Vijay Abraham I <kishon@ti.com> wrote: > > Hi Rob, > > On 5/21/2020 3:37 AM, Rob Herring wrote: > > On Wed, May 06, 2020 at 08:44:18PM +0530, Kishon Vijay Abraham I wrote: > >> Add support to use custom read and write accessors. Platforms that > >> don't support half word or byte access or any other constraint > >> while accessing registers can use this feature to populate custom > >> read and write accessors. These custom accessors are used for both > >> standard register access and configuration space register access of > >> the PCIe host bridge. > >> > >> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> > >> --- > >> drivers/pci/controller/cadence/pcie-cadence.h | 107 +++++++++++++++--- > >> 1 file changed, 94 insertions(+), 13 deletions(-) > > > > Actually, take back my R-by... > > > >> > >> diff --git a/drivers/pci/controller/cadence/pcie-cadence.h b/drivers/pci/controller/cadence/pcie-cadence.h > >> index df14ad002fe9..70b6b25153e8 100644 > >> --- a/drivers/pci/controller/cadence/pcie-cadence.h > >> +++ b/drivers/pci/controller/cadence/pcie-cadence.h > >> @@ -223,6 +223,11 @@ enum cdns_pcie_msg_routing { > >> MSG_ROUTING_GATHER, > >> }; > >> > >> +struct cdns_pcie_ops { > >> + u32 (*read)(void __iomem *addr, int size); > >> + void (*write)(void __iomem *addr, int size, u32 value); > >> +}; > >> + > >> /** > >> * struct cdns_pcie - private data for Cadence PCIe controller drivers > >> * @reg_base: IO mapped register base > >> @@ -239,7 +244,7 @@ struct cdns_pcie { > >> int phy_count; > >> struct phy **phy; > >> struct device_link **link; > >> - const struct cdns_pcie_common_ops *ops; > >> + const struct cdns_pcie_ops *ops; > >> }; > >> > >> /** > >> @@ -299,69 +304,145 @@ struct cdns_pcie_ep { > >> /* Register access */ > >> static inline void cdns_pcie_writeb(struct cdns_pcie *pcie, u32 reg, u8 value) > >> { > >> - writeb(value, pcie->reg_base + reg); > >> + void __iomem *addr = pcie->reg_base + reg; > >> + > >> + if (pcie->ops && pcie->ops->write) { > >> + pcie->ops->write(addr, 0x1, value); > >> + return; > >> + } > >> + > >> + writeb(value, addr); > >> } > >> > >> static inline void cdns_pcie_writew(struct cdns_pcie *pcie, u32 reg, u16 value) > >> { > >> - writew(value, pcie->reg_base + reg); > >> + void __iomem *addr = pcie->reg_base + reg; > >> + > >> + if (pcie->ops && pcie->ops->write) { > >> + pcie->ops->write(addr, 0x2, value); > >> + return; > >> + } > >> + > >> + writew(value, addr); > >> } > > > > cdns_pcie_writeb and cdns_pcie_writew are used, so remove them. > > > >> > >> static inline void cdns_pcie_writel(struct cdns_pcie *pcie, u32 reg, u32 value) > >> { > >> - writel(value, pcie->reg_base + reg); > >> + void __iomem *addr = pcie->reg_base + reg; > >> + > >> + if (pcie->ops && pcie->ops->write) { > >> + pcie->ops->write(addr, 0x4, value); > >> + return; > >> + } > >> + > >> + writel(value, addr); > > > > writel isn't broken for you, so you don't need this either. > > > >> } > >> > >> static inline u32 cdns_pcie_readl(struct cdns_pcie *pcie, u32 reg) > >> { > >> - return readl(pcie->reg_base + reg); > >> + void __iomem *addr = pcie->reg_base + reg; > >> + > >> + if (pcie->ops && pcie->ops->read) > >> + return pcie->ops->read(addr, 0x4); > >> + > >> + return readl(addr); > > > > And neither is readl. > > Sure, I'll remove all the unused functions and avoid using ops for readl and > writel. > > > >> } > >> > >> /* Root Port register access */ > >> static inline void cdns_pcie_rp_writeb(struct cdns_pcie *pcie, > >> u32 reg, u8 value) > >> { > >> - writeb(value, pcie->reg_base + CDNS_PCIE_RP_BASE + reg); > >> + void __iomem *addr = pcie->reg_base + CDNS_PCIE_RP_BASE + reg; > >> + > >> + if (pcie->ops && pcie->ops->write) { > >> + pcie->ops->write(addr, 0x1, value); > >> + return; > >> + } > >> + > >> + writeb(value, addr); > >> } > >> > >> static inline void cdns_pcie_rp_writew(struct cdns_pcie *pcie, > >> u32 reg, u16 value) > >> { > >> - writew(value, pcie->reg_base + CDNS_PCIE_RP_BASE + reg); > >> + void __iomem *addr = pcie->reg_base + CDNS_PCIE_RP_BASE + reg; > >> + > >> + if (pcie->ops && pcie->ops->write) { > >> + pcie->ops->write(addr, 0x2, value); > >> + return; > >> + } > >> + > >> + writew(value, addr); > > > > You removed 2 out of 3 calls to this. I think I'd just make the root > > port writes always be 32-bit. It is all just one time init stuff > > anyways. > > > > Either rework the calls to assemble the data into 32-bits or keep these > > functions and do the RMW here. > > The problem with assembling data into 32-bits is we have to read/write with > different offsets. We'll give PCI_VENDOR_ID offset for modifying deviceID, > PCI_INTERRUPT_LINE for modifying INTERRUPT_PIN which might get non-intuitive. > Similarly in endpoint we read and write to MSI_FLAGS (which is at offset 2) we > have to directly use MSI capability offset. > > And doing RMW in the accessors would mean the same RMW op is repeated. So if we > just have cdns_pcie_rp_writeb() and cdns_pcie_rp_writew(), the same code will > be repeated here twice. Why repeated? Just copy what the config accessors do: static inline void cdns_pcie_write_sz(u32 val, void __iomem *addr, int size) { u32 tmp, mask, where = (unsigned long)addr & 0x3; addr -= where; mask = ~(((1 << (size * 8)) - 1) << (where * 8)); tmp = readl(addr) & mask; tmp |= val << (where * 8); writel(tmp, addr); } /* Root Port register access */ static inline void cdns_pcie_rp_writeb(struct cdns_pcie *pcie, u32 reg, u8 value) { cdns_pcie_write_sz(value, pcie->reg_base + CDNS_PCIE_RP_BASE + reg, 1); } static inline void cdns_pcie_rp_writew(struct cdns_pcie *pcie, u32 reg, u16 value) { cdns_pcie_write_sz(value, pcie->reg_base + CDNS_PCIE_RP_BASE + reg, 2); } > > IMHO using ops is a lot cleaner for these cases. IMHO except for removing > unused functions and not changing readl/writel, others should use ops. Trying to read the DW PCI driver I don't agree... Again, unless doing RMW is fundamentally broken (like it is for config space at runtime), then you only want to do it on broken h/w and ops would be appropriate. It is all mostly one time setup, so doing a few extra reads isn't a big deal. If you really care about speed on that, probably should use the _relaxed accessor variants. I'm being hopeful the Cadence IP doesn't become the mess that DW is. Rob
Hi Rob, On 5/22/2020 3:47 AM, Rob Herring wrote: > On Thu, May 21, 2020 at 7:33 AM Kishon Vijay Abraham I <kishon@ti.com> wrote: >> >> Hi Rob, >> >> On 5/21/2020 3:37 AM, Rob Herring wrote: >>> On Wed, May 06, 2020 at 08:44:18PM +0530, Kishon Vijay Abraham I wrote: >>>> Add support to use custom read and write accessors. Platforms that >>>> don't support half word or byte access or any other constraint >>>> while accessing registers can use this feature to populate custom >>>> read and write accessors. These custom accessors are used for both >>>> standard register access and configuration space register access of >>>> the PCIe host bridge. >>>> >>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> >>>> --- >>>> drivers/pci/controller/cadence/pcie-cadence.h | 107 +++++++++++++++--- >>>> 1 file changed, 94 insertions(+), 13 deletions(-) >>> >>> Actually, take back my R-by... >>> >>>> >>>> diff --git a/drivers/pci/controller/cadence/pcie-cadence.h b/drivers/pci/controller/cadence/pcie-cadence.h >>>> index df14ad002fe9..70b6b25153e8 100644 >>>> --- a/drivers/pci/controller/cadence/pcie-cadence.h >>>> +++ b/drivers/pci/controller/cadence/pcie-cadence.h >>>> @@ -223,6 +223,11 @@ enum cdns_pcie_msg_routing { >>>> MSG_ROUTING_GATHER, >>>> }; >>>> >>>> +struct cdns_pcie_ops { >>>> + u32 (*read)(void __iomem *addr, int size); >>>> + void (*write)(void __iomem *addr, int size, u32 value); >>>> +}; >>>> + >>>> /** >>>> * struct cdns_pcie - private data for Cadence PCIe controller drivers >>>> * @reg_base: IO mapped register base >>>> @@ -239,7 +244,7 @@ struct cdns_pcie { >>>> int phy_count; >>>> struct phy **phy; >>>> struct device_link **link; >>>> - const struct cdns_pcie_common_ops *ops; >>>> + const struct cdns_pcie_ops *ops; >>>> }; >>>> >>>> /** >>>> @@ -299,69 +304,145 @@ struct cdns_pcie_ep { >>>> /* Register access */ >>>> static inline void cdns_pcie_writeb(struct cdns_pcie *pcie, u32 reg, u8 value) >>>> { >>>> - writeb(value, pcie->reg_base + reg); >>>> + void __iomem *addr = pcie->reg_base + reg; >>>> + >>>> + if (pcie->ops && pcie->ops->write) { >>>> + pcie->ops->write(addr, 0x1, value); >>>> + return; >>>> + } >>>> + >>>> + writeb(value, addr); >>>> } >>>> >>>> static inline void cdns_pcie_writew(struct cdns_pcie *pcie, u32 reg, u16 value) >>>> { >>>> - writew(value, pcie->reg_base + reg); >>>> + void __iomem *addr = pcie->reg_base + reg; >>>> + >>>> + if (pcie->ops && pcie->ops->write) { >>>> + pcie->ops->write(addr, 0x2, value); >>>> + return; >>>> + } >>>> + >>>> + writew(value, addr); >>>> } >>> >>> cdns_pcie_writeb and cdns_pcie_writew are used, so remove them. >>> >>>> >>>> static inline void cdns_pcie_writel(struct cdns_pcie *pcie, u32 reg, u32 value) >>>> { >>>> - writel(value, pcie->reg_base + reg); >>>> + void __iomem *addr = pcie->reg_base + reg; >>>> + >>>> + if (pcie->ops && pcie->ops->write) { >>>> + pcie->ops->write(addr, 0x4, value); >>>> + return; >>>> + } >>>> + >>>> + writel(value, addr); >>> >>> writel isn't broken for you, so you don't need this either. >>> >>>> } >>>> >>>> static inline u32 cdns_pcie_readl(struct cdns_pcie *pcie, u32 reg) >>>> { >>>> - return readl(pcie->reg_base + reg); >>>> + void __iomem *addr = pcie->reg_base + reg; >>>> + >>>> + if (pcie->ops && pcie->ops->read) >>>> + return pcie->ops->read(addr, 0x4); >>>> + >>>> + return readl(addr); >>> >>> And neither is readl. >> >> Sure, I'll remove all the unused functions and avoid using ops for readl and >> writel. >>> >>>> } >>>> >>>> /* Root Port register access */ >>>> static inline void cdns_pcie_rp_writeb(struct cdns_pcie *pcie, >>>> u32 reg, u8 value) >>>> { >>>> - writeb(value, pcie->reg_base + CDNS_PCIE_RP_BASE + reg); >>>> + void __iomem *addr = pcie->reg_base + CDNS_PCIE_RP_BASE + reg; >>>> + >>>> + if (pcie->ops && pcie->ops->write) { >>>> + pcie->ops->write(addr, 0x1, value); >>>> + return; >>>> + } >>>> + >>>> + writeb(value, addr); >>>> } >>>> >>>> static inline void cdns_pcie_rp_writew(struct cdns_pcie *pcie, >>>> u32 reg, u16 value) >>>> { >>>> - writew(value, pcie->reg_base + CDNS_PCIE_RP_BASE + reg); >>>> + void __iomem *addr = pcie->reg_base + CDNS_PCIE_RP_BASE + reg; >>>> + >>>> + if (pcie->ops && pcie->ops->write) { >>>> + pcie->ops->write(addr, 0x2, value); >>>> + return; >>>> + } >>>> + >>>> + writew(value, addr); >>> >>> You removed 2 out of 3 calls to this. I think I'd just make the root >>> port writes always be 32-bit. It is all just one time init stuff >>> anyways. >>> >>> Either rework the calls to assemble the data into 32-bits or keep these >>> functions and do the RMW here. >> >> The problem with assembling data into 32-bits is we have to read/write with >> different offsets. We'll give PCI_VENDOR_ID offset for modifying deviceID, >> PCI_INTERRUPT_LINE for modifying INTERRUPT_PIN which might get non-intuitive. >> Similarly in endpoint we read and write to MSI_FLAGS (which is at offset 2) we >> have to directly use MSI capability offset. >> >> And doing RMW in the accessors would mean the same RMW op is repeated. So if we >> just have cdns_pcie_rp_writeb() and cdns_pcie_rp_writew(), the same code will >> be repeated here twice. > > Why repeated? Just copy what the config accessors do: > > static inline void cdns_pcie_write_sz(u32 val, void __iomem *addr, int size) > { > u32 tmp, mask, where = (unsigned long)addr & 0x3; > > addr -= where; > > mask = ~(((1 << (size * 8)) - 1) << (where * 8)); > tmp = readl(addr) & mask; > tmp |= val << (where * 8); > writel(tmp, addr); > } > > /* Root Port register access */ > static inline void cdns_pcie_rp_writeb(struct cdns_pcie *pcie, > u32 reg, u8 value) > { > cdns_pcie_write_sz(value, pcie->reg_base + CDNS_PCIE_RP_BASE + reg, 1); > } > > static inline void cdns_pcie_rp_writew(struct cdns_pcie *pcie, > u32 reg, u16 value) > { > cdns_pcie_write_sz(value, pcie->reg_base + CDNS_PCIE_RP_BASE + reg, 2); > } > >> >> IMHO using ops is a lot cleaner for these cases. IMHO except for removing >> unused functions and not changing readl/writel, others should use ops. > > Trying to read the DW PCI driver I don't agree... > > Again, unless doing RMW is fundamentally broken (like it is for config > space at runtime), then you only want to do it on broken h/w and ops > would be appropriate. It is all mostly one time setup, so doing a few > extra reads isn't a big deal. If you really care about speed on that, > probably should use the _relaxed accessor variants. > > I'm being hopeful the Cadence IP doesn't become the mess that DW is. Okay, I'll remove ops for read/write accessors. Thanks Kishon
diff --git a/drivers/pci/controller/cadence/pcie-cadence.h b/drivers/pci/controller/cadence/pcie-cadence.h index df14ad002fe9..70b6b25153e8 100644 --- a/drivers/pci/controller/cadence/pcie-cadence.h +++ b/drivers/pci/controller/cadence/pcie-cadence.h @@ -223,6 +223,11 @@ enum cdns_pcie_msg_routing { MSG_ROUTING_GATHER, }; +struct cdns_pcie_ops { + u32 (*read)(void __iomem *addr, int size); + void (*write)(void __iomem *addr, int size, u32 value); +}; + /** * struct cdns_pcie - private data for Cadence PCIe controller drivers * @reg_base: IO mapped register base @@ -239,7 +244,7 @@ struct cdns_pcie { int phy_count; struct phy **phy; struct device_link **link; - const struct cdns_pcie_common_ops *ops; + const struct cdns_pcie_ops *ops; }; /** @@ -299,69 +304,145 @@ struct cdns_pcie_ep { /* Register access */ static inline void cdns_pcie_writeb(struct cdns_pcie *pcie, u32 reg, u8 value) { - writeb(value, pcie->reg_base + reg); + void __iomem *addr = pcie->reg_base + reg; + + if (pcie->ops && pcie->ops->write) { + pcie->ops->write(addr, 0x1, value); + return; + } + + writeb(value, addr); } static inline void cdns_pcie_writew(struct cdns_pcie *pcie, u32 reg, u16 value) { - writew(value, pcie->reg_base + reg); + void __iomem *addr = pcie->reg_base + reg; + + if (pcie->ops && pcie->ops->write) { + pcie->ops->write(addr, 0x2, value); + return; + } + + writew(value, addr); } static inline void cdns_pcie_writel(struct cdns_pcie *pcie, u32 reg, u32 value) { - writel(value, pcie->reg_base + reg); + void __iomem *addr = pcie->reg_base + reg; + + if (pcie->ops && pcie->ops->write) { + pcie->ops->write(addr, 0x4, value); + return; + } + + writel(value, addr); } static inline u32 cdns_pcie_readl(struct cdns_pcie *pcie, u32 reg) { - return readl(pcie->reg_base + reg); + void __iomem *addr = pcie->reg_base + reg; + + if (pcie->ops && pcie->ops->read) + return pcie->ops->read(addr, 0x4); + + return readl(addr); } /* Root Port register access */ static inline void cdns_pcie_rp_writeb(struct cdns_pcie *pcie, u32 reg, u8 value) { - writeb(value, pcie->reg_base + CDNS_PCIE_RP_BASE + reg); + void __iomem *addr = pcie->reg_base + CDNS_PCIE_RP_BASE + reg; + + if (pcie->ops && pcie->ops->write) { + pcie->ops->write(addr, 0x1, value); + return; + } + + writeb(value, addr); } static inline void cdns_pcie_rp_writew(struct cdns_pcie *pcie, u32 reg, u16 value) { - writew(value, pcie->reg_base + CDNS_PCIE_RP_BASE + reg); + void __iomem *addr = pcie->reg_base + CDNS_PCIE_RP_BASE + reg; + + if (pcie->ops && pcie->ops->write) { + pcie->ops->write(addr, 0x2, value); + return; + } + + writew(value, addr); } /* Endpoint Function register access */ static inline void cdns_pcie_ep_fn_writeb(struct cdns_pcie *pcie, u8 fn, u32 reg, u8 value) { - writeb(value, pcie->reg_base + CDNS_PCIE_EP_FUNC_BASE(fn) + reg); + void __iomem *addr = pcie->reg_base + CDNS_PCIE_EP_FUNC_BASE(fn) + reg; + + if (pcie->ops && pcie->ops->write) { + pcie->ops->write(addr, 0x1, value); + return; + } + + writeb(value, addr); } static inline void cdns_pcie_ep_fn_writew(struct cdns_pcie *pcie, u8 fn, u32 reg, u16 value) { - writew(value, pcie->reg_base + CDNS_PCIE_EP_FUNC_BASE(fn) + reg); + void __iomem *addr = pcie->reg_base + CDNS_PCIE_EP_FUNC_BASE(fn) + reg; + + if (pcie->ops && pcie->ops->write) { + pcie->ops->write(addr, 0x2, value); + return; + } + + writew(value, addr); } static inline void cdns_pcie_ep_fn_writel(struct cdns_pcie *pcie, u8 fn, u32 reg, u32 value) { - writel(value, pcie->reg_base + CDNS_PCIE_EP_FUNC_BASE(fn) + reg); + void __iomem *addr = pcie->reg_base + CDNS_PCIE_EP_FUNC_BASE(fn) + reg; + + if (pcie->ops && pcie->ops->write) { + pcie->ops->write(addr, 0x4, value); + return; + } + + writel(value, addr); } static inline u8 cdns_pcie_ep_fn_readb(struct cdns_pcie *pcie, u8 fn, u32 reg) { - return readb(pcie->reg_base + CDNS_PCIE_EP_FUNC_BASE(fn) + reg); + void __iomem *addr = pcie->reg_base + CDNS_PCIE_EP_FUNC_BASE(fn) + reg; + + if (pcie->ops && pcie->ops->read) + return pcie->ops->read(addr, 0x1); + + return readb(addr); } static inline u16 cdns_pcie_ep_fn_readw(struct cdns_pcie *pcie, u8 fn, u32 reg) { - return readw(pcie->reg_base + CDNS_PCIE_EP_FUNC_BASE(fn) + reg); + void __iomem *addr = pcie->reg_base + CDNS_PCIE_EP_FUNC_BASE(fn) + reg; + + if (pcie->ops && pcie->ops->read) + return pcie->ops->read(addr, 0x2); + + return readw(addr); } static inline u32 cdns_pcie_ep_fn_readl(struct cdns_pcie *pcie, u8 fn, u32 reg) { - return readl(pcie->reg_base + CDNS_PCIE_EP_FUNC_BASE(fn) + reg); + void __iomem *addr = pcie->reg_base + CDNS_PCIE_EP_FUNC_BASE(fn) + reg; + + if (pcie->ops && pcie->ops->read) + return pcie->ops->read(addr, 0x4); + + return readl(addr); } #ifdef CONFIG_PCIE_CADENCE_HOST
Add support to use custom read and write accessors. Platforms that don't support half word or byte access or any other constraint while accessing registers can use this feature to populate custom read and write accessors. These custom accessors are used for both standard register access and configuration space register access of the PCIe host bridge. Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> --- drivers/pci/controller/cadence/pcie-cadence.h | 107 +++++++++++++++--- 1 file changed, 94 insertions(+), 13 deletions(-)