| Message ID | 1441964307-126942-3-git-send-email-gabriele.paoloni@huawei.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On Fri, Sep 11, 2015 at 05:38:26PM +0800, Gabriele Paoloni wrote: > From: gabriele paoloni <gabriele.paoloni@huawei.com> > > This patch changes the implementation of dw_pcie_cfg_read() and > dw_pcie_cfg_write() to improve the function usage from the callers > perspective. > Currently the callers are obliged to pass the 32bit aligned address > of the register that contains the field of the PCI header that they > want to read/write; also they have to pass the offset of the field > in that register. This is quite tricky to use as the callers are > obliged to sum the PCI header base address to the field offset > masked to retrieve the 32b aligned register address. > > With the new API the callers have to pass the base address of the > PCI header and the offset corresponding to the field they intend to > read/write. > > Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com> > int dw_pcie_cfg_read(void __iomem *addr, int where, int size, u32 *val) > { > + addr += (where & ~0x3); > *val = readl(addr); > + where &= 3; > > if (size == 1) > - *val = (*val >> (8 * (where & 3))) & 0xff; > + *val = (*val >> (8 * where)) & 0xff; > else if (size == 2) > - *val = (*val >> (8 * (where & 3))) & 0xffff; > + *val = (*val >> (8 * where)) & 0xffff; > else if (size != 4) > return PCIBIOS_BAD_REGISTER_NUMBER; > > @@ -96,12 +98,14 @@ int dw_pcie_cfg_read(void __iomem *addr, int where, int size, u32 *val) > > int dw_pcie_cfg_write(void __iomem *addr, int where, int size, u32 val) > { > + addr += where; > + > if (size == 4) > writel(val, addr); > else if (size == 2) > - writew(val, addr + (where & 2)); > + writew(val, addr); > else if (size == 1) > - writeb(val, addr + (where & 3)); > + writeb(val, addr); > else > return PCIBIOS_BAD_REGISTER_NUMBER; I just noticed the asymmetry between dw_pcie_cfg_read() and dw_pcie_cfg_write(): in dw_pcie_cfg_read() we always do 32-bit reads and mask out the parts we won't want, but in dw_pcie_cfg_write() we do 8-, 16-, or 32-byte writes. That was there even before your patch, but I wonder why. Either both should work the same way, or there should be a comment explaining why they are different. Jingoo, Pratyush? -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Sep 11, 2015 at 05:38:26PM +0800, Gabriele Paoloni wrote: > From: gabriele paoloni <gabriele.paoloni@huawei.com> > > This patch changes the implementation of dw_pcie_cfg_read() and > dw_pcie_cfg_write() to improve the function usage from the callers > perspective. > Currently the callers are obliged to pass the 32bit aligned address > of the register that contains the field of the PCI header that they > want to read/write; also they have to pass the offset of the field > in that register. This is quite tricky to use as the callers are > obliged to sum the PCI header base address to the field offset > masked to retrieve the 32b aligned register address. > > With the new API the callers have to pass the base address of the > PCI header and the offset corresponding to the field they intend to > read/write. > > Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com> > --- > drivers/pci/host/pci-exynos.c | 5 ++--- > drivers/pci/host/pci-keystone-dw.c | 4 ++-- > drivers/pci/host/pcie-designware.c | 28 ++++++++++++++-------------- > drivers/pci/host/pcie-spear13xx.c | 26 ++++++++++++-------------- > 4 files changed, 30 insertions(+), 33 deletions(-) > > diff --git a/drivers/pci/host/pci-exynos.c b/drivers/pci/host/pci-exynos.c > index f9f468d..8b0e04b 100644 > --- a/drivers/pci/host/pci-exynos.c > +++ b/drivers/pci/host/pci-exynos.c > @@ -454,7 +454,7 @@ static int exynos_pcie_rd_own_conf(struct pcie_port *pp, int where, int size, > int ret; > > exynos_pcie_sideband_dbi_r_mode(pp, true); > - ret = dw_pcie_cfg_read(pp->dbi_base + (where & ~0x3), where, size, val); > + ret = dw_pcie_cfg_read(pp->dbi_base, where, size, val); > exynos_pcie_sideband_dbi_r_mode(pp, false); > return ret; > } Is there really any value in keeping "addr" and "where" separate? dw_pcie_cfg_write() clearly doesn't care; it just adds them together. I don't think dw_pcie_cfg_read() needs to care either: it could round the address down to a 32-bit boundary and use the difference to compute the mask and shift. So I'm proposing something like this: int dw_pcie_cfg_read(void __iomem *addr, int size, u32 *val) int dw_pcie_cfg_write(void __iomem *addr, int size, u32 val) > int dw_pcie_cfg_read(void __iomem *addr, int where, int size, u32 *val) > { > + addr += (where & ~0x3); > *val = readl(addr); > + where &= 3; > > if (size == 1) > - *val = (*val >> (8 * (where & 3))) & 0xff; > + *val = (*val >> (8 * where)) & 0xff; > else if (size == 2) > - *val = (*val >> (8 * (where & 3))) & 0xffff; > + *val = (*val >> (8 * where)) & 0xffff; > else if (size != 4) > return PCIBIOS_BAD_REGISTER_NUMBER; > > @@ -96,12 +98,14 @@ int dw_pcie_cfg_read(void __iomem *addr, int where, int size, u32 *val) > > int dw_pcie_cfg_write(void __iomem *addr, int where, int size, u32 val) > { > + addr += where; > + > if (size == 4) > writel(val, addr); > else if (size == 2) > - writew(val, addr + (where & 2)); > + writew(val, addr); > else if (size == 1) > - writeb(val, addr + (where & 3)); > + writeb(val, addr); > else > return PCIBIOS_BAD_REGISTER_NUMBER; -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Sep 19, 2015 at 2:16 AM, Bjorn Helgaas <bhelgaas@google.com> wrote: > On Fri, Sep 11, 2015 at 05:38:26PM +0800, Gabriele Paoloni wrote: >> From: gabriele paoloni <gabriele.paoloni@huawei.com> >> >> This patch changes the implementation of dw_pcie_cfg_read() and >> dw_pcie_cfg_write() to improve the function usage from the callers >> perspective. >> Currently the callers are obliged to pass the 32bit aligned address >> of the register that contains the field of the PCI header that they >> want to read/write; also they have to pass the offset of the field >> in that register. This is quite tricky to use as the callers are >> obliged to sum the PCI header base address to the field offset >> masked to retrieve the 32b aligned register address. >> >> With the new API the callers have to pass the base address of the >> PCI header and the offset corresponding to the field they intend to >> read/write. >> >> Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com> > > >> int dw_pcie_cfg_read(void __iomem *addr, int where, int size, u32 *val) >> { >> + addr += (where & ~0x3); >> *val = readl(addr); >> + where &= 3; >> >> if (size == 1) >> - *val = (*val >> (8 * (where & 3))) & 0xff; >> + *val = (*val >> (8 * where)) & 0xff; >> else if (size == 2) >> - *val = (*val >> (8 * (where & 3))) & 0xffff; >> + *val = (*val >> (8 * where)) & 0xffff; >> else if (size != 4) >> return PCIBIOS_BAD_REGISTER_NUMBER; >> >> @@ -96,12 +98,14 @@ int dw_pcie_cfg_read(void __iomem *addr, int where, int size, u32 *val) >> >> int dw_pcie_cfg_write(void __iomem *addr, int where, int size, u32 val) >> { >> + addr += where; >> + >> if (size == 4) >> writel(val, addr); >> else if (size == 2) >> - writew(val, addr + (where & 2)); >> + writew(val, addr); >> else if (size == 1) >> - writeb(val, addr + (where & 3)); >> + writeb(val, addr); >> else >> return PCIBIOS_BAD_REGISTER_NUMBER; > > I just noticed the asymmetry between dw_pcie_cfg_read() and > dw_pcie_cfg_write(): in dw_pcie_cfg_read() we always do 32-bit reads and > mask out the parts we won't want, but in dw_pcie_cfg_write() we do 8-, 16-, > or 32-byte writes. > > That was there even before your patch, but I wonder why. Either both > should work the same way, or there should be a comment explaining why they > are different. > > Jingoo, Pratyush? As I said earlier, I just vaguely remember that there was some issue with a SOC in reading non word aligned addresses. (1) I do not have any reference for it and (2) even if some where there could be such issue they can always have platform specific accessor . So I support your idea and both read and write can be made symmetric. -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Sep 19, 2015 at 2:23 AM, Bjorn Helgaas <bhelgaas@google.com> wrote: > > Is there really any value in keeping "addr" and "where" separate? Now we do not care of buggy SOCs which could have issues with non word aligned address read. If there is any such SOC, then they can have their own accessor. So, I do not see any value of keeping "where". > dw_pcie_cfg_write() clearly doesn't care; it just adds them together. I > don't think dw_pcie_cfg_read() needs to care either: it could round the > address down to a 32-bit boundary and use the difference to compute the > mask and shift. > > So I'm proposing something like this: > > int dw_pcie_cfg_read(void __iomem *addr, int size, u32 *val) > int dw_pcie_cfg_write(void __iomem *addr, int size, u32 val) Agreed. ~Pratyush -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi guys sorry for the late reply I have been OOO for the last 5 days > -----Original Message----- > From: Pratyush Anand [mailto:pratyush.anand@gmail.com] > Sent: Saturday, September 19, 2015 4:04 AM > To: Bjorn Helgaas > Cc: Gabriele Paoloni; Jingoo Han; linux-pci@vger.kernel.org; Wangzhou > (B); Yuanzhichang; Zhudacai; zhangjukuo; qiuzhenfa; Liguozhu (Kenneth) > Subject: Re: [PATCH v3 2/3] PCI: designware: change dw_pcie_cfg_write() > and dw_pcie_cfg_read() > > On Sat, Sep 19, 2015 at 2:16 AM, Bjorn Helgaas <bhelgaas@google.com> > wrote: > > On Fri, Sep 11, 2015 at 05:38:26PM +0800, Gabriele Paoloni wrote: > >> From: gabriele paoloni <gabriele.paoloni@huawei.com> > >> > >> This patch changes the implementation of dw_pcie_cfg_read() and > >> dw_pcie_cfg_write() to improve the function usage from the callers > >> perspective. > >> Currently the callers are obliged to pass the 32bit aligned address > >> of the register that contains the field of the PCI header that they > >> want to read/write; also they have to pass the offset of the field > >> in that register. This is quite tricky to use as the callers are > >> obliged to sum the PCI header base address to the field offset > >> masked to retrieve the 32b aligned register address. > >> > >> With the new API the callers have to pass the base address of the > >> PCI header and the offset corresponding to the field they intend to > >> read/write. > >> > >> Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com> > > > > > >> int dw_pcie_cfg_read(void __iomem *addr, int where, int size, u32 > *val) > >> { > >> + addr += (where & ~0x3); > >> *val = readl(addr); > >> + where &= 3; > >> > >> if (size == 1) > >> - *val = (*val >> (8 * (where & 3))) & 0xff; > >> + *val = (*val >> (8 * where)) & 0xff; > >> else if (size == 2) > >> - *val = (*val >> (8 * (where & 3))) & 0xffff; > >> + *val = (*val >> (8 * where)) & 0xffff; > >> else if (size != 4) > >> return PCIBIOS_BAD_REGISTER_NUMBER; > >> > >> @@ -96,12 +98,14 @@ int dw_pcie_cfg_read(void __iomem *addr, int > where, int size, u32 *val) > >> > >> int dw_pcie_cfg_write(void __iomem *addr, int where, int size, u32 > val) > >> { > >> + addr += where; > >> + > >> if (size == 4) > >> writel(val, addr); > >> else if (size == 2) > >> - writew(val, addr + (where & 2)); > >> + writew(val, addr); > >> else if (size == 1) > >> - writeb(val, addr + (where & 3)); > >> + writeb(val, addr); > >> else > >> return PCIBIOS_BAD_REGISTER_NUMBER; > > > > I just noticed the asymmetry between dw_pcie_cfg_read() and > > dw_pcie_cfg_write(): in dw_pcie_cfg_read() we always do 32-bit reads > and > > mask out the parts we won't want, but in dw_pcie_cfg_write() we do 8-, > 16-, > > or 32-byte writes. > > > > That was there even before your patch, but I wonder why. Either both > > should work the same way, or there should be a comment explaining why > they > > are different. > > > > Jingoo, Pratyush? > > As I said earlier, I just vaguely remember that there was some issue > with a SOC in reading non word aligned addresses. > (1) I do not have any reference for it and (2) even if some where > there could be such issue they can always have platform specific > accessor . So I support your idea and both read and write can be made > symmetric. I agree with the idea but, what if doing so we break other drivers? Is the correct flow to break them first and let the single maintainers fix them then...?
On Wed, Sep 23, 2015 at 11:20 AM, Gabriele Paoloni <gabriele.paoloni@huawei.com> wrote: > Hi guys sorry for the late reply I have been OOO for the last 5 days > >> -----Original Message----- >> From: Pratyush Anand [mailto:pratyush.anand@gmail.com] >> Sent: Saturday, September 19, 2015 4:04 AM >> To: Bjorn Helgaas >> Cc: Gabriele Paoloni; Jingoo Han; linux-pci@vger.kernel.org; Wangzhou >> (B); Yuanzhichang; Zhudacai; zhangjukuo; qiuzhenfa; Liguozhu (Kenneth) >> Subject: Re: [PATCH v3 2/3] PCI: designware: change dw_pcie_cfg_write() >> and dw_pcie_cfg_read() >> >> On Sat, Sep 19, 2015 at 2:16 AM, Bjorn Helgaas <bhelgaas@google.com> >> wrote: >> > On Fri, Sep 11, 2015 at 05:38:26PM +0800, Gabriele Paoloni wrote: >> >> From: gabriele paoloni <gabriele.paoloni@huawei.com> >> >> >> >> This patch changes the implementation of dw_pcie_cfg_read() and >> >> dw_pcie_cfg_write() to improve the function usage from the callers >> >> perspective. >> >> Currently the callers are obliged to pass the 32bit aligned address >> >> of the register that contains the field of the PCI header that they >> >> want to read/write; also they have to pass the offset of the field >> >> in that register. This is quite tricky to use as the callers are >> >> obliged to sum the PCI header base address to the field offset >> >> masked to retrieve the 32b aligned register address. >> >> >> >> With the new API the callers have to pass the base address of the >> >> PCI header and the offset corresponding to the field they intend to >> >> read/write. >> >> >> >> Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com> >> > >> > >> >> int dw_pcie_cfg_read(void __iomem *addr, int where, int size, u32 >> *val) >> >> { >> >> + addr += (where & ~0x3); >> >> *val = readl(addr); >> >> + where &= 3; >> >> >> >> if (size == 1) >> >> - *val = (*val >> (8 * (where & 3))) & 0xff; >> >> + *val = (*val >> (8 * where)) & 0xff; >> >> else if (size == 2) >> >> - *val = (*val >> (8 * (where & 3))) & 0xffff; >> >> + *val = (*val >> (8 * where)) & 0xffff; >> >> else if (size != 4) >> >> return PCIBIOS_BAD_REGISTER_NUMBER; >> >> >> >> @@ -96,12 +98,14 @@ int dw_pcie_cfg_read(void __iomem *addr, int >> where, int size, u32 *val) >> >> >> >> int dw_pcie_cfg_write(void __iomem *addr, int where, int size, u32 >> val) >> >> { >> >> + addr += where; >> >> + >> >> if (size == 4) >> >> writel(val, addr); >> >> else if (size == 2) >> >> - writew(val, addr + (where & 2)); >> >> + writew(val, addr); >> >> else if (size == 1) >> >> - writeb(val, addr + (where & 3)); >> >> + writeb(val, addr); >> >> else >> >> return PCIBIOS_BAD_REGISTER_NUMBER; >> > >> > I just noticed the asymmetry between dw_pcie_cfg_read() and >> > dw_pcie_cfg_write(): in dw_pcie_cfg_read() we always do 32-bit reads >> and >> > mask out the parts we won't want, but in dw_pcie_cfg_write() we do 8-, >> 16-, >> > or 32-byte writes. >> > >> > That was there even before your patch, but I wonder why. Either both >> > should work the same way, or there should be a comment explaining why >> they >> > are different. >> > >> > Jingoo, Pratyush? >> >> As I said earlier, I just vaguely remember that there was some issue >> with a SOC in reading non word aligned addresses. >> (1) I do not have any reference for it and (2) even if some where >> there could be such issue they can always have platform specific >> accessor . So I support your idea and both read and write can be made >> symmetric. > > I agree with the idea but, what if doing so we break other drivers? > Is the correct flow to break them first and let the single maintainers fix them then...? Since we don't know which (if any) systems would break, I think the best we can do is see if anybody has objections, then put it in and see if anything breaks. Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Bjorn Helgaas [mailto:bhelgaas@google.com] > Sent: Wednesday, September 23, 2015 5:24 PM > To: Gabriele Paoloni > Cc: Pratyush Anand; Jingoo Han; linux-pci@vger.kernel.org; Wangzhou (B); > Yuanzhichang; Zhudacai; zhangjukuo; qiuzhenfa; Liguozhu (Kenneth) > Subject: Re: [PATCH v3 2/3] PCI: designware: change dw_pcie_cfg_write() > and dw_pcie_cfg_read() > > On Wed, Sep 23, 2015 at 11:20 AM, Gabriele Paoloni > <gabriele.paoloni@huawei.com> wrote: > > Hi guys sorry for the late reply I have been OOO for the last 5 days > > > >> -----Original Message----- > >> From: Pratyush Anand [mailto:pratyush.anand@gmail.com] > >> Sent: Saturday, September 19, 2015 4:04 AM > >> To: Bjorn Helgaas > >> Cc: Gabriele Paoloni; Jingoo Han; linux-pci@vger.kernel.org; > Wangzhou > >> (B); Yuanzhichang; Zhudacai; zhangjukuo; qiuzhenfa; Liguozhu > (Kenneth) > >> Subject: Re: [PATCH v3 2/3] PCI: designware: change > dw_pcie_cfg_write() > >> and dw_pcie_cfg_read() > >> > >> On Sat, Sep 19, 2015 at 2:16 AM, Bjorn Helgaas <bhelgaas@google.com> > >> wrote: > >> > On Fri, Sep 11, 2015 at 05:38:26PM +0800, Gabriele Paoloni wrote: > >> >> From: gabriele paoloni <gabriele.paoloni@huawei.com> > >> >> > >> >> This patch changes the implementation of dw_pcie_cfg_read() and > >> >> dw_pcie_cfg_write() to improve the function usage from the > callers > >> >> perspective. > >> >> Currently the callers are obliged to pass the 32bit aligned > address > >> >> of the register that contains the field of the PCI header that > they > >> >> want to read/write; also they have to pass the offset of the > field > >> >> in that register. This is quite tricky to use as the callers are > >> >> obliged to sum the PCI header base address to the field offset > >> >> masked to retrieve the 32b aligned register address. > >> >> > >> >> With the new API the callers have to pass the base address of the > >> >> PCI header and the offset corresponding to the field they intend > to > >> >> read/write. > >> >> > >> >> Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com> > >> > > >> > > >> >> int dw_pcie_cfg_read(void __iomem *addr, int where, int size, > u32 > >> *val) > >> >> { > >> >> + addr += (where & ~0x3); > >> >> *val = readl(addr); > >> >> + where &= 3; > >> >> > >> >> if (size == 1) > >> >> - *val = (*val >> (8 * (where & 3))) & 0xff; > >> >> + *val = (*val >> (8 * where)) & 0xff; > >> >> else if (size == 2) > >> >> - *val = (*val >> (8 * (where & 3))) & 0xffff; > >> >> + *val = (*val >> (8 * where)) & 0xffff; > >> >> else if (size != 4) > >> >> return PCIBIOS_BAD_REGISTER_NUMBER; > >> >> > >> >> @@ -96,12 +98,14 @@ int dw_pcie_cfg_read(void __iomem *addr, int > >> where, int size, u32 *val) > >> >> > >> >> int dw_pcie_cfg_write(void __iomem *addr, int where, int size, > u32 > >> val) > >> >> { > >> >> + addr += where; > >> >> + > >> >> if (size == 4) > >> >> writel(val, addr); > >> >> else if (size == 2) > >> >> - writew(val, addr + (where & 2)); > >> >> + writew(val, addr); > >> >> else if (size == 1) > >> >> - writeb(val, addr + (where & 3)); > >> >> + writeb(val, addr); > >> >> else > >> >> return PCIBIOS_BAD_REGISTER_NUMBER; > >> > > >> > I just noticed the asymmetry between dw_pcie_cfg_read() and > >> > dw_pcie_cfg_write(): in dw_pcie_cfg_read() we always do 32-bit > reads > >> and > >> > mask out the parts we won't want, but in dw_pcie_cfg_write() we do > 8-, > >> 16-, > >> > or 32-byte writes. > >> > > >> > That was there even before your patch, but I wonder why. Either > both > >> > should work the same way, or there should be a comment explaining > why > >> they > >> > are different. > >> > > >> > Jingoo, Pratyush? > >> > >> As I said earlier, I just vaguely remember that there was some issue > >> with a SOC in reading non word aligned addresses. > >> (1) I do not have any reference for it and (2) even if some where > >> there could be such issue they can always have platform specific > >> accessor . So I support your idea and both read and write can be > made > >> symmetric. > > > > I agree with the idea but, what if doing so we break other drivers? > > Is the correct flow to break them first and let the single > maintainers fix them then...? > > Since we don't know which (if any) systems would break, I think the > best we can do is see if anybody has objections, then put it in and > see if anything breaks. Right, I'll put it in v4 Many Thanks Gab > > Bjorn
> -----Original Message----- > From: linux-pci-owner@vger.kernel.org [mailto:linux-pci- > owner@vger.kernel.org] On Behalf Of Pratyush Anand > Sent: Saturday, September 19, 2015 4:11 AM > To: Bjorn Helgaas > Cc: Gabriele Paoloni; Jingoo Han; linux-pci@vger.kernel.org; Wangzhou > (B); Yuanzhichang; Zhudacai; zhangjukuo; qiuzhenfa; Liguozhu (Kenneth) > Subject: Re: [PATCH v3 2/3] PCI: designware: change dw_pcie_cfg_write() > and dw_pcie_cfg_read() > > On Sat, Sep 19, 2015 at 2:23 AM, Bjorn Helgaas <bhelgaas@google.com> > wrote: > > > > Is there really any value in keeping "addr" and "where" separate? > > Now we do not care of buggy SOCs which could have issues with non word > aligned address read. If there is any such SOC, then they can have > their own accessor. > So, I do not see any value of keeping "where". > > > dw_pcie_cfg_write() clearly doesn't care; it just adds them together. > I > > don't think dw_pcie_cfg_read() needs to care either: it could round > the > > address down to a 32-bit boundary and use the difference to compute > the > > mask and shift. > > > > So I'm proposing something like this: > > > > int dw_pcie_cfg_read(void __iomem *addr, int size, u32 *val) > > int dw_pcie_cfg_write(void __iomem *addr, int size, u32 val) Ok Agreed Will do in v4 Gab > > Agreed. > > ~Pratyush > -- > To unsubscribe from this list: send the line "unsubscribe linux-pci" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/pci/host/pci-exynos.c b/drivers/pci/host/pci-exynos.c index f9f468d..8b0e04b 100644 --- a/drivers/pci/host/pci-exynos.c +++ b/drivers/pci/host/pci-exynos.c @@ -454,7 +454,7 @@ static int exynos_pcie_rd_own_conf(struct pcie_port *pp, int where, int size, int ret; exynos_pcie_sideband_dbi_r_mode(pp, true); - ret = dw_pcie_cfg_read(pp->dbi_base + (where & ~0x3), where, size, val); + ret = dw_pcie_cfg_read(pp->dbi_base, where, size, val); exynos_pcie_sideband_dbi_r_mode(pp, false); return ret; } @@ -465,8 +465,7 @@ static int exynos_pcie_wr_own_conf(struct pcie_port *pp, int where, int size, int ret; exynos_pcie_sideband_dbi_w_mode(pp, true); - ret = dw_pcie_cfg_write(pp->dbi_base + (where & ~0x3), - where, size, val); + ret = dw_pcie_cfg_write(pp->dbi_base, where, size, val); exynos_pcie_sideband_dbi_w_mode(pp, false); return ret; } diff --git a/drivers/pci/host/pci-keystone-dw.c b/drivers/pci/host/pci-keystone-dw.c index e71da99..6648f9be 100644 --- a/drivers/pci/host/pci-keystone-dw.c +++ b/drivers/pci/host/pci-keystone-dw.c @@ -398,7 +398,7 @@ int ks_dw_pcie_rd_other_conf(struct pcie_port *pp, struct pci_bus *bus, addr = ks_pcie_cfg_setup(ks_pcie, bus_num, devfn); - return dw_pcie_cfg_read(addr + (where & ~0x3), where, size, val); + return dw_pcie_cfg_read(addr, where, size, val); } int ks_dw_pcie_wr_other_conf(struct pcie_port *pp, struct pci_bus *bus, @@ -410,7 +410,7 @@ int ks_dw_pcie_wr_other_conf(struct pcie_port *pp, struct pci_bus *bus, addr = ks_pcie_cfg_setup(ks_pcie, bus_num, devfn); - return dw_pcie_cfg_write(addr + (where & ~0x3), where, size, val); + return dw_pcie_cfg_write(addr, where, size, val); } /** diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c index 52aa6e3..cb23e31 100644 --- a/drivers/pci/host/pcie-designware.c +++ b/drivers/pci/host/pcie-designware.c @@ -82,12 +82,14 @@ static inline struct pcie_port *sys_to_pcie(struct pci_sys_data *sys) int dw_pcie_cfg_read(void __iomem *addr, int where, int size, u32 *val) { + addr += (where & ~0x3); *val = readl(addr); + where &= 3; if (size == 1) - *val = (*val >> (8 * (where & 3))) & 0xff; + *val = (*val >> (8 * where)) & 0xff; else if (size == 2) - *val = (*val >> (8 * (where & 3))) & 0xffff; + *val = (*val >> (8 * where)) & 0xffff; else if (size != 4) return PCIBIOS_BAD_REGISTER_NUMBER; @@ -96,12 +98,14 @@ int dw_pcie_cfg_read(void __iomem *addr, int where, int size, u32 *val) int dw_pcie_cfg_write(void __iomem *addr, int where, int size, u32 val) { + addr += where; + if (size == 4) writel(val, addr); else if (size == 2) - writew(val, addr + (where & 2)); + writew(val, addr); else if (size == 1) - writeb(val, addr + (where & 3)); + writeb(val, addr); else return PCIBIOS_BAD_REGISTER_NUMBER; @@ -132,8 +136,7 @@ static int dw_pcie_rd_own_conf(struct pcie_port *pp, int where, int size, if (pp->ops->rd_own_conf) ret = pp->ops->rd_own_conf(pp, where, size, val); else - ret = dw_pcie_cfg_read(pp->dbi_base + (where & ~0x3), where, - size, val); + ret = dw_pcie_cfg_read(pp->dbi_base, where, size, val); return ret; } @@ -146,8 +149,7 @@ static int dw_pcie_wr_own_conf(struct pcie_port *pp, int where, int size, if (pp->ops->wr_own_conf) ret = pp->ops->wr_own_conf(pp, where, size, val); else - ret = dw_pcie_cfg_write(pp->dbi_base + (where & ~0x3), where, - size, val); + ret = dw_pcie_cfg_write(pp->dbi_base, where, size, val); return ret; } @@ -539,13 +541,12 @@ static int dw_pcie_rd_other_conf(struct pcie_port *pp, struct pci_bus *bus, u32 devfn, int where, int size, u32 *val) { int ret, type; - u32 address, busdev, cfg_size; + u32 busdev, cfg_size; u64 cpu_addr; void __iomem *va_cfg_base; busdev = PCIE_ATU_BUS(bus->number) | PCIE_ATU_DEV(PCI_SLOT(devfn)) | PCIE_ATU_FUNC(PCI_FUNC(devfn)); - address = where & ~0x3; if (bus->parent->number == pp->root_bus_nr) { type = PCIE_ATU_TYPE_CFG0; @@ -562,7 +563,7 @@ static int dw_pcie_rd_other_conf(struct pcie_port *pp, struct pci_bus *bus, dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX0, type, cpu_addr, busdev, cfg_size); - ret = dw_pcie_cfg_read(va_cfg_base + address, where, size, val); + ret = dw_pcie_cfg_read(va_cfg_base, where, size, val); dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX0, PCIE_ATU_TYPE_IO, pp->io_mod_base, pp->io_bus_addr, pp->io_size); @@ -574,13 +575,12 @@ static int dw_pcie_wr_other_conf(struct pcie_port *pp, struct pci_bus *bus, u32 devfn, int where, int size, u32 val) { int ret, type; - u32 address, busdev, cfg_size; + u32 busdev, cfg_size; u64 cpu_addr; void __iomem *va_cfg_base; busdev = PCIE_ATU_BUS(bus->number) | PCIE_ATU_DEV(PCI_SLOT(devfn)) | PCIE_ATU_FUNC(PCI_FUNC(devfn)); - address = where & ~0x3; if (bus->parent->number == pp->root_bus_nr) { type = PCIE_ATU_TYPE_CFG0; @@ -597,7 +597,7 @@ static int dw_pcie_wr_other_conf(struct pcie_port *pp, struct pci_bus *bus, dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX0, type, cpu_addr, busdev, cfg_size); - ret = dw_pcie_cfg_write(va_cfg_base + address, where, size, val); + ret = dw_pcie_cfg_write(va_cfg_base, where, size, val); dw_pcie_prog_outbound_atu(pp, PCIE_ATU_REGION_INDEX0, PCIE_ATU_TYPE_IO, pp->io_mod_base, pp->io_bus_addr, pp->io_size); diff --git a/drivers/pci/host/pcie-spear13xx.c b/drivers/pci/host/pcie-spear13xx.c index 0754ea3..cd30d8a 100644 --- a/drivers/pci/host/pcie-spear13xx.c +++ b/drivers/pci/host/pcie-spear13xx.c @@ -163,36 +163,34 @@ static int spear13xx_pcie_establish_link(struct pcie_port *pp) * default value in capability register is 512 bytes. So force * it to 128 here. */ - dw_pcie_cfg_read(pp->dbi_base + exp_cap_off + PCI_EXP_DEVCTL, - 0, 2, &val); + dw_pcie_cfg_read(pp->dbi_base, exp_cap_off + PCI_EXP_DEVCTL, 2, &val); val &= ~PCI_EXP_DEVCTL_READRQ; - dw_pcie_cfg_write(pp->dbi_base + exp_cap_off + PCI_EXP_DEVCTL, - 0, 2, val); + dw_pcie_cfg_write(pp->dbi_base, exp_cap_off + PCI_EXP_DEVCTL, 2, val); - dw_pcie_cfg_write(pp->dbi_base + PCI_VENDOR_ID, 0, 2, 0x104A); - dw_pcie_cfg_write(pp->dbi_base + PCI_VENDOR_ID, 2, 2, 0xCD80); + dw_pcie_cfg_write(pp->dbi_base, PCI_VENDOR_ID, 2, 0x104A); + dw_pcie_cfg_write(pp->dbi_base, PCI_DEVICE_ID, 2, 0xCD80); /* * if is_gen1 is set then handle it, so that some buggy card * also works */ if (spear13xx_pcie->is_gen1) { - dw_pcie_cfg_read(pp->dbi_base + exp_cap_off + PCI_EXP_LNKCAP, - 0, 4, &val); + dw_pcie_cfg_read(pp->dbi_base, exp_cap_off + PCI_EXP_LNKCAP, + 4, &val); if ((val & PCI_EXP_LNKCAP_SLS) != PCI_EXP_LNKCAP_SLS_2_5GB) { val &= ~((u32)PCI_EXP_LNKCAP_SLS); val |= PCI_EXP_LNKCAP_SLS_2_5GB; - dw_pcie_cfg_write(pp->dbi_base + exp_cap_off + - PCI_EXP_LNKCAP, 0, 4, val); + dw_pcie_cfg_write(pp->dbi_base, exp_cap_off + + PCI_EXP_LNKCAP, 4, val); } - dw_pcie_cfg_read(pp->dbi_base + exp_cap_off + PCI_EXP_LNKCTL2, - 0, 2, &val); + dw_pcie_cfg_read(pp->dbi_base, exp_cap_off + PCI_EXP_LNKCTL2, + 2, &val); if ((val & PCI_EXP_LNKCAP_SLS) != PCI_EXP_LNKCAP_SLS_2_5GB) { val &= ~((u32)PCI_EXP_LNKCAP_SLS); val |= PCI_EXP_LNKCAP_SLS_2_5GB; - dw_pcie_cfg_write(pp->dbi_base + exp_cap_off + - PCI_EXP_LNKCTL2, 0, 2, val); + dw_pcie_cfg_write(pp->dbi_base, exp_cap_off + + PCI_EXP_LNKCTL2, 2, val); } }