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