Hi Bjorn,
On Fri, Sep 4, 2015 at 2:29 AM, Bjorn Helgaas <bhelgaas@xxxxxxxxxx> wrote:
> On Thu, Aug 20, 2015 at 09:58:33PM +0800, Gabriele Paoloni wrote:
>> From: gabriele paoloni <gabriele.paoloni@xxxxxxxxxx>
>>
>> Currently in dw_pcie_cfg_write() if the input size is 2 bytes
>> the address offset is wrongly calculated as we mask also bit0
>> of "where" input parameter. Instead we should mask all bits of
>> "where" except bit0 and bit1.
>
> I think there are two problems in this area:
>
> 1) Most calls of dw_pcie_cfg_write() are of the form:
>
> dw_pcie_cfg_read(pp->dbi_base + (where & ~0x3), where, ...)
> dw_pcie_cfg_write(pp->dbi_base + (where & ~0x3), where, ...)
>
> This is needlessly repetitive because the caller has to mention
> "where" twice and do the masking itself. We could simply pass
> "pp->dbi_base" and "where" and let dw_pcie_cfg_write() do all the
> required masking internally.
I think just by passing "pp->dbi_base" and "where" we can not take
care of all cases. For example, if I have to read PCI_CACHE_LINE_SIZE
(8 bit value at offset 0x0C), I will have to pass "pp->dbi_base" +
PCI_CACHE_LINE_SIZE, 1
So, I think User of this function will need to pass "addr","size" and
"val", where
"addr" as "pp->dbi_base" + "where"
"size" and "val" as it is.
one more thing IIRC, there was some discussion (however I am unable to
find any reference), and not sure if it is still true with any
platform: issues with non word aligned PCI register read.
If a platform is not able to read non word aligned register correctly,
then in that case we will have to pass where and size both :(
>
> 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 these are incorrect because dw_pcie_cfg_read() and
> dw_pcie_cfg_write() don't add anything to the "addr" argument
> ("pp->dbi_base" in this case) except the 0-3 byte select offset, so
> they use the correct alignment, but access the wrong registers.
hummm..so for sure the version which was up-streamed has never been
tested with buggy gen1 card and with any endpoint which would have
generated a read request of more than 128 bytes :( .. Will correct.
Thanks a lot for pointing this out :-)
~Pratyush
--
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
