> -----Original Message----- > From: Bjorn Helgaas [mailto:bhelgaas@xxxxxxxxxx] > Sent: Wednesday, September 23, 2015 5:24 PM > To: Gabriele Paoloni > Cc: Pratyush Anand; Jingoo Han; linux-pci@xxxxxxxxxxxxxxx; 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@xxxxxxxxxx> 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@xxxxxxxxx] > >> Sent: Saturday, September 19, 2015 4:04 AM > >> To: Bjorn Helgaas > >> Cc: Gabriele Paoloni; Jingoo Han; linux-pci@xxxxxxxxxxxxxxx; > 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@xxxxxxxxxx> > >> wrote: > >> > On Fri, Sep 11, 2015 at 05:38:26PM +0800, Gabriele Paoloni 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. > >> >> 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@xxxxxxxxxx> > >> > > >> > > >> >> 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 ��.n��������+%������w��{.n�����{���"�)�� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
