Hi Bjorn, thanks for your comments > -----Original Message----- > From: Bjorn Helgaas [mailto:bhelgaas@xxxxxxxxxx] > Sent: Wednesday, September 09, 2015 8:45 PM > To: Gabriele Paoloni > Cc: Jingoo Han; Pratyush Anand Thakur; linux-pci@xxxxxxxxxxxxxxx; > Wangzhou (B); Yuanzhichang; Zhudacai; zhangjukuo; qiuzhenfa; Liguozhu > (Kenneth) > Subject: Re: [PATCH] PCI: designware: change dw_pcie_cfg_write() and > dw_pcie_cfg_read() > > Hi Gabriele, > > On Wed, Sep 9, 2015 at 11:51 AM, Gabriele Paoloni > <gabriele.paoloni@xxxxxxxxxx> wrote: > > From: gabriele paoloni <gabriele.paoloni@xxxxxxxxxx> > > > > This patch changes the implementation of dw_pcie_cfg_read() and > > dw_pcie_cfg_write() to: > > - improve the function usage from the callers perspective; > > - make sure that the callers specify proper address offsets > > according to the size of the field being read/written > > - fix a bug in pcie-spear13xx.c where the callers where passing > > the wrong address as they missed to add the appropriate offset > > Please split this into separate patches, so each patch does one thing, > i.e., separate the "where" usage change from the "return > PCIBIOS_BAD_REGISTER_NUMBER for bad offsets" change. Ok I will send out a patchset with separate patches > > I think there was a separate patch for the pcie-spear13xx.c bug, > wasn't there? At least, I don't see that fix in *this* patch. > >From your previous comments: ******** *2) Pratyush, this one's for you :) spear13xx_pcie_establish_link() * calls dw_pcie_cfg_read() and dw_pcie_cfg_write() several times like * this: * * dw_pcie_cfg_read(pp->dbi_base, exp_cap_off + PCI_EXP_DEVCTL, 4, ...) * dw_pcie_cfg_write(pp->dbi_base, exp_cap_off + PCI_EXP_DEVCTL, 4, ...) ******** I think that the implementation of dw_pcie_cfg_read() and dw_pcie_cfg_write() that I proposed in this patch is compatible with the previous calls in pcie-spear13xx.c. So the calls in pcie-spear13xx.c were buggy for the previous implementations of the functions; with the reworked functions there is no need to modify the function calls in pcie-spear13xx.c...do you agree? > > This patch is a follow up from the discussion in: > > http://www.spinics.net/lists/linux-pci/msg44351.html > > > > Signed-off-by: Gabriele Paoloni <gabriele.paoloni@xxxxxxxxxx> > > --- > > drivers/pci/host/pci-exynos.c | 5 ++-- > > drivers/pci/host/pci-keystone-dw.c | 4 +-- > > drivers/pci/host/pcie-designware.c | 51 ++++++++++++++++++++++------ > ---------- > > 3 files changed, 34 insertions(+), 26 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; > > } > > @@ -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 f34892e..2b391f4 100644 > > --- a/drivers/pci/host/pci-keystone-dw.c > > +++ b/drivers/pci/host/pci-keystone-dw.c > > @@ -403,7 +403,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, > > @@ -415,7 +415,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 69486be..b53aa38 100644 > > --- a/drivers/pci/host/pcie-designware.c > > +++ b/drivers/pci/host/pcie-designware.c > > @@ -82,13 +82,20 @@ 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); > > - > > - if (size == 1) > > - *val = (*val >> (8 * (where & 3))) & 0xff; > > - else if (size == 2) > > - *val = (*val >> (8 * (where & 3))) & 0xffff; > > - else if (size != 4) > > + where &= 3; > > + > > + if (size == 1) { > > + *val = (*val >> (8 * where)) & 0xff; > > + } else if (size == 2) { > > + if (where & 1) > > + return PCIBIOS_BAD_REGISTER_NUMBER; > > + *val = (*val >> (8 * where)) & 0xffff; > > + } else if (size == 4) { > > + if (where & 3) > > + return PCIBIOS_BAD_REGISTER_NUMBER; > > + } else > > return PCIBIOS_BAD_REGISTER_NUMBER; > > > > return PCIBIOS_SUCCESSFUL; > > @@ -96,12 +103,18 @@ 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) > > { > > - if (size == 4) > > + addr += where; > > + > > + if (size == 4) { > > + if ((uintptr_t)addr & 3) > > + return PCIBIOS_BAD_REGISTER_NUMBER; > > writel(val, addr); > > - else if (size == 2) > > - writew(val, addr + (where & 2)); > > - else if (size == 1) > > - writeb(val, addr + (where & 3)); > > + } else if (size == 2) { > > + if ((uintptr_t)addr & 1) > > + return PCIBIOS_BAD_REGISTER_NUMBER; > > + writew(val, addr); > > + } else if (size == 1) > > + writeb(val, addr); > > else > > return PCIBIOS_BAD_REGISTER_NUMBER; > > > > @@ -132,8 +145,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 +158,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; > > } > > @@ -541,13 +552,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; > > @@ -564,7 +574,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); > > @@ -576,13 +586,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; > > @@ -599,7 +608,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); > > -- > > 1.9.1 > > > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-pci" > in > > the body of a message to majordomo@xxxxxxxxxxxxxxx > > More majordomo info at http://vger.kernel.org/majordomo-info.html ��.n��������+%������w��{.n�����{���"�)�� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
