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. Bjorn -- 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
