| Message ID | 20220928121911.14994-1-pali@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | PCI: tegra: Use PCI_CONF1_EXT_ADDRESS() macro | expand |
On Wed, Sep 28, 2022 at 02:19:11PM +0200, Pali Rohár wrote: > Simplify pci-tegra.c driver code and use new PCI_CONF1_EXT_ADDRESS() macro > for accessing PCI config space. > > Signed-off-by: Pali Rohár <pali@kernel.org> > --- > Please look also at this related patch: > https://patchwork.kernel.org/project/linux-pci/patch/20220911113216.14892-1-pali@kernel.org/ > --- > drivers/pci/controller/pci-tegra.c | 11 +++-------- > 1 file changed, 3 insertions(+), 8 deletions(-) I had to go chase down the patch that introduces PCI_CONF1_EXT_ADDRESS. It would've been easier if this had been part of the series that introduced that, or if you had provided a link to that patch here. Anyway, looks like this is equivalent to the existing inline function, so: Acked-by: Thierry Reding <treding@nvidia.com>
On Wed, Sep 28, 2022 at 04:35:10PM +0200, Thierry Reding wrote: > On Wed, Sep 28, 2022 at 02:19:11PM +0200, Pali Rohár wrote: > > Simplify pci-tegra.c driver code and use new PCI_CONF1_EXT_ADDRESS() macro > > for accessing PCI config space. > > > > Signed-off-by: Pali Rohár <pali@kernel.org> > > --- > > Please look also at this related patch: > > https://patchwork.kernel.org/project/linux-pci/patch/20220911113216.14892-1-pali@kernel.org/ > > --- > > drivers/pci/controller/pci-tegra.c | 11 +++-------- > > 1 file changed, 3 insertions(+), 8 deletions(-) > > I had to go chase down the patch that introduces PCI_CONF1_EXT_ADDRESS. > It would've been easier if this had been part of the series that > introduced that, or if you had provided a link to that patch here. > > Anyway, looks like this is equivalent to the existing inline function, > so: > > Acked-by: Thierry Reding <treding@nvidia.com> After looking at the linked patch, perhaps revise this one more time and remove the comment above the removed helper since it's now just a duplication of what the PCI_CONF1_EXT_ADDRESS comments say. Thierry
On Wed, 28 Sep 2022 14:19:11 +0200, Pali Rohár wrote: > Simplify pci-tegra.c driver code and use new PCI_CONF1_EXT_ADDRESS() macro > for accessing PCI config space. > > Applied to pci/misc, thanks! [1/1] PCI: tegra: Use PCI_CONF1_EXT_ADDRESS() macro https://git.kernel.org/lpieralisi/pci/c/8bb7ff12a914 Thanks, Lorenzo
On 28/09/2022 13:19, Pali Rohár wrote: > Simplify pci-tegra.c driver code and use new PCI_CONF1_EXT_ADDRESS() macro > for accessing PCI config space. > > Signed-off-by: Pali Rohár <pali@kernel.org> > --- > Please look also at this related patch: > https://patchwork.kernel.org/project/linux-pci/patch/20220911113216.14892-1-pali@kernel.org/ > --- > drivers/pci/controller/pci-tegra.c | 11 +++-------- > 1 file changed, 3 insertions(+), 8 deletions(-) > > diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c > index 5df90d183526..c9924e75e597 100644 > --- a/drivers/pci/controller/pci-tegra.c > +++ b/drivers/pci/controller/pci-tegra.c > @@ -417,13 +417,6 @@ static inline u32 pads_readl(struct tegra_pcie *pcie, unsigned long offset) > * address (access to which generates correct config transaction) falls in > * this 4 KiB region. > */ > -static unsigned int tegra_pcie_conf_offset(u8 bus, unsigned int devfn, > - unsigned int where) > -{ > - return ((where & 0xf00) << 16) | (bus << 16) | (PCI_SLOT(devfn) << 11) | > - (PCI_FUNC(devfn) << 8) | (where & 0xff); > -} > - > static void __iomem *tegra_pcie_map_bus(struct pci_bus *bus, > unsigned int devfn, > int where) > @@ -445,7 +438,9 @@ static void __iomem *tegra_pcie_map_bus(struct pci_bus *bus, > unsigned int offset; > u32 base; > > - offset = tegra_pcie_conf_offset(bus->number, devfn, where); > + offset = PCI_CONF1_EXT_ADDRESS(bus->number, PCI_SLOT(devfn), > + PCI_FUNC(devfn), where) & > + ~PCI_CONF1_ENABLE; > > /* move 4 KiB window to offset within the FPCI region */ > base = 0xfe100000 + ((offset & ~(SZ_4K - 1)) >> 8); Our PCIe test on Tegra124 Jetson TK1 is currently failing on -next and bisect points to this commit. Looking at bit closer, the problem appears to be the PCI_CONF1_REG_MASK which has a value of 0xfc. Before this patch was applied a mask of 0xff was applied to the lower 8-bits of 'where' and now it is 0xfc. So this does not work for Tegra as it is. Let me know if you have any thoughts? Jon
On Tuesday 11 October 2022 16:42:34 Jon Hunter wrote: > On 28/09/2022 13:19, Pali Rohár wrote: > > Simplify pci-tegra.c driver code and use new PCI_CONF1_EXT_ADDRESS() macro > > for accessing PCI config space. > > > > Signed-off-by: Pali Rohár <pali@kernel.org> > > --- > > Please look also at this related patch: > > https://patchwork.kernel.org/project/linux-pci/patch/20220911113216.14892-1-pali@kernel.org/ > > --- > > drivers/pci/controller/pci-tegra.c | 11 +++-------- > > 1 file changed, 3 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c > > index 5df90d183526..c9924e75e597 100644 > > --- a/drivers/pci/controller/pci-tegra.c > > +++ b/drivers/pci/controller/pci-tegra.c > > @@ -417,13 +417,6 @@ static inline u32 pads_readl(struct tegra_pcie *pcie, unsigned long offset) > > * address (access to which generates correct config transaction) falls in > > * this 4 KiB region. > > */ > > -static unsigned int tegra_pcie_conf_offset(u8 bus, unsigned int devfn, > > - unsigned int where) > > -{ > > - return ((where & 0xf00) << 16) | (bus << 16) | (PCI_SLOT(devfn) << 11) | > > - (PCI_FUNC(devfn) << 8) | (where & 0xff); > > -} > > - > > static void __iomem *tegra_pcie_map_bus(struct pci_bus *bus, > > unsigned int devfn, > > int where) > > @@ -445,7 +438,9 @@ static void __iomem *tegra_pcie_map_bus(struct pci_bus *bus, > > unsigned int offset; > > u32 base; > > - offset = tegra_pcie_conf_offset(bus->number, devfn, where); > > + offset = PCI_CONF1_EXT_ADDRESS(bus->number, PCI_SLOT(devfn), > > + PCI_FUNC(devfn), where) & > > + ~PCI_CONF1_ENABLE; > > /* move 4 KiB window to offset within the FPCI region */ > > base = 0xfe100000 + ((offset & ~(SZ_4K - 1)) >> 8); > > > Our PCIe test on Tegra124 Jetson TK1 is currently failing on -next and > bisect points to this commit. Looking at bit closer, the problem appears to > be the PCI_CONF1_REG_MASK which has a value of 0xfc. Before this patch was > applied a mask of 0xff was applied to the lower 8-bits of 'where' and now it > is 0xfc. So this does not work for Tegra as it is. > > Let me know if you have any thoughts? > > Jon > > -- > nvpublic I see, this is stupid mistake. PCIe config read and write operations needs to be 4-byte aligned, so normally it is done by calculating 4-byte aligned base address and then using appropriate cpu load/store instruction to access just defined size/offset of 4-byte config space register. pci-tegra.c is using common helper functions pci_generic_config_read() and pci_generic_config_write(), which expects final address with offset, and not 4-byte aligned address. I'm not sure what should be the proper fix, but for me it looks like that pci_generic_config_read() and pci_generic_config_write() could be adjusted to handle it. In any case, above patch is a regressions and I see there two options for now: 1) Reverting that patch 2) Adding "offset |= where & 0x3;" after the PCI_CONF1_EXT_ADDRESS() macro to set also lower 2 bits of accessed register. Jon, Lorenzo, what do you think? Could you test if 2) is working fine?
On 11/10/2022 17:16, Pali Rohár wrote: ... > I see, this is stupid mistake. PCIe config read and write operations > needs to be 4-byte aligned, so normally it is done by calculating 4-byte > aligned base address and then using appropriate cpu load/store > instruction to access just defined size/offset of 4-byte config space > register. > > pci-tegra.c is using common helper functions pci_generic_config_read() > and pci_generic_config_write(), which expects final address with offset, > and not 4-byte aligned address. > > I'm not sure what should be the proper fix, but for me it looks like > that pci_generic_config_read() and pci_generic_config_write() could be > adjusted to handle it. > > In any case, above patch is a regressions and I see there two options > for now: > > 1) Reverting that patch > > 2) Adding "offset |= where & 0x3;" after the PCI_CONF1_EXT_ADDRESS() > macro to set also lower 2 bits of accessed register. > > Jon, Lorenzo, what do you think? Could you test if 2) is working fine? I tested 'offset |= where & 0xff' which is essentially the same as the above and that is working and so I am sure that the above works too. However, I do wonder if reverting is simpler because we already have a '& ~PCI_CONF1_ENABLE' and now adding '| where & 0x3' seems to diminish the value of this change. Cheers Jon
On Tuesday 11 October 2022 17:47:50 Jon Hunter wrote: > On 11/10/2022 17:16, Pali Rohár wrote: > > ... > > > I see, this is stupid mistake. PCIe config read and write operations > > needs to be 4-byte aligned, so normally it is done by calculating 4-byte > > aligned base address and then using appropriate cpu load/store > > instruction to access just defined size/offset of 4-byte config space > > register. > > > > pci-tegra.c is using common helper functions pci_generic_config_read() > > and pci_generic_config_write(), which expects final address with offset, > > and not 4-byte aligned address. > > > > I'm not sure what should be the proper fix, but for me it looks like > > that pci_generic_config_read() and pci_generic_config_write() could be > > adjusted to handle it. > > > > In any case, above patch is a regressions and I see there two options > > for now: > > > > 1) Reverting that patch > > > > 2) Adding "offset |= where & 0x3;" after the PCI_CONF1_EXT_ADDRESS() > > macro to set also lower 2 bits of accessed register. > > > > Jon, Lorenzo, what do you think? Could you test if 2) is working fine? > > > I tested 'offset |= where & 0xff' which is essentially the same as the above > and that is working and so I am sure that the above works too. However, I do > wonder if reverting is simpler because we already have a '& > ~PCI_CONF1_ENABLE' and now adding '| where & 0x3' seems to diminish the > value of this change. > > Cheers > Jon > > -- > nvpublic Well, if decision would be that pci_generic_config_read() could be modified in the future to handle aligning, then '|= where & 0x3' block would be moved from driver to generic function... I'm really not sure which option to choose.
On Tue, Oct 11, 2022 at 05:47:50PM +0100, Jon Hunter wrote: > > On 11/10/2022 17:16, Pali Rohár wrote: > > ... > > > I see, this is stupid mistake. PCIe config read and write operations > > needs to be 4-byte aligned, so normally it is done by calculating 4-byte > > aligned base address and then using appropriate cpu load/store > > instruction to access just defined size/offset of 4-byte config space > > register. > > > > pci-tegra.c is using common helper functions pci_generic_config_read() > > and pci_generic_config_write(), which expects final address with offset, > > and not 4-byte aligned address. > > > > I'm not sure what should be the proper fix, but for me it looks like > > that pci_generic_config_read() and pci_generic_config_write() could be > > adjusted to handle it. > > > > In any case, above patch is a regressions and I see there two options > > for now: > > > > 1) Reverting that patch > > > > 2) Adding "offset |= where & 0x3;" after the PCI_CONF1_EXT_ADDRESS() > > macro to set also lower 2 bits of accessed register. > > > > Jon, Lorenzo, what do you think? Could you test if 2) is working fine? > > > I tested 'offset |= where & 0xff' which is essentially the same as the above > and that is working and so I am sure that the above works too. However, I do > wonder if reverting is simpler because we already have a '& > ~PCI_CONF1_ENABLE' and now adding '| where & 0x3' seems to diminish the > value of this change. Hi Jon, it is unfortunate but I think we should proceed with a revert, please send it and I shall ask Bjorn to send it for one of the upcoming -rcX. Thanks, Lorenzo
On 17/10/2022 08:43, Lorenzo Pieralisi wrote: > On Tue, Oct 11, 2022 at 05:47:50PM +0100, Jon Hunter wrote: >> >> On 11/10/2022 17:16, Pali Rohár wrote: >> >> ... >> >>> I see, this is stupid mistake. PCIe config read and write operations >>> needs to be 4-byte aligned, so normally it is done by calculating 4-byte >>> aligned base address and then using appropriate cpu load/store >>> instruction to access just defined size/offset of 4-byte config space >>> register. >>> >>> pci-tegra.c is using common helper functions pci_generic_config_read() >>> and pci_generic_config_write(), which expects final address with offset, >>> and not 4-byte aligned address. >>> >>> I'm not sure what should be the proper fix, but for me it looks like >>> that pci_generic_config_read() and pci_generic_config_write() could be >>> adjusted to handle it. >>> >>> In any case, above patch is a regressions and I see there two options >>> for now: >>> >>> 1) Reverting that patch >>> >>> 2) Adding "offset |= where & 0x3;" after the PCI_CONF1_EXT_ADDRESS() >>> macro to set also lower 2 bits of accessed register. >>> >>> Jon, Lorenzo, what do you think? Could you test if 2) is working fine? >> >> >> I tested 'offset |= where & 0xff' which is essentially the same as the above >> and that is working and so I am sure that the above works too. However, I do >> wonder if reverting is simpler because we already have a '& >> ~PCI_CONF1_ENABLE' and now adding '| where & 0x3' seems to diminish the >> value of this change. > > Hi Jon, > > it is unfortunate but I think we should proceed with a revert, please > send it and I shall ask Bjorn to send it for one of the upcoming -rcX. I have sent a revert for this change. Thanks Jon
diff --git a/drivers/pci/controller/pci-tegra.c b/drivers/pci/controller/pci-tegra.c index 5df90d183526..c9924e75e597 100644 --- a/drivers/pci/controller/pci-tegra.c +++ b/drivers/pci/controller/pci-tegra.c @@ -417,13 +417,6 @@ static inline u32 pads_readl(struct tegra_pcie *pcie, unsigned long offset) * address (access to which generates correct config transaction) falls in * this 4 KiB region. */ -static unsigned int tegra_pcie_conf_offset(u8 bus, unsigned int devfn, - unsigned int where) -{ - return ((where & 0xf00) << 16) | (bus << 16) | (PCI_SLOT(devfn) << 11) | - (PCI_FUNC(devfn) << 8) | (where & 0xff); -} - static void __iomem *tegra_pcie_map_bus(struct pci_bus *bus, unsigned int devfn, int where) @@ -445,7 +438,9 @@ static void __iomem *tegra_pcie_map_bus(struct pci_bus *bus, unsigned int offset; u32 base; - offset = tegra_pcie_conf_offset(bus->number, devfn, where); + offset = PCI_CONF1_EXT_ADDRESS(bus->number, PCI_SLOT(devfn), + PCI_FUNC(devfn), where) & + ~PCI_CONF1_ENABLE; /* move 4 KiB window to offset within the FPCI region */ base = 0xfe100000 + ((offset & ~(SZ_4K - 1)) >> 8);
Simplify pci-tegra.c driver code and use new PCI_CONF1_EXT_ADDRESS() macro for accessing PCI config space. Signed-off-by: Pali Rohár <pali@kernel.org> --- Please look also at this related patch: https://patchwork.kernel.org/project/linux-pci/patch/20220911113216.14892-1-pali@kernel.org/ --- drivers/pci/controller/pci-tegra.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-)