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...? ��.n��������+%������w��{.n�����{���"�)�� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
