Hi Rob,

On 5/26/2020 8:42 PM, Rob Herring wrote:

On Sun, May 24, 2020 at 9:30 PM Kishon Vijay Abraham I [off-list ref] wrote:

quoted

Hi Rob,

On 5/22/2020 9:24 PM, Rob Herring wrote:

quoted

On Thu, May 21, 2020 at 9:37 PM Kishon Vijay Abraham I [off-list ref] wrote:

quoted

Certain platforms like TI's J721E using Cadence PCIe IP can perform only
32-bit accesses for reading or writing to Cadence registers. Convert all
read and write accesses to 32-bit in Cadence PCIe driver in preparation
for adding PCIe support in TI's J721E SoC.

Looking more closely I don't think cdns_pcie_ep_assert_intx is okay
with this and never can be given the PCI_COMMAND and PCI_STATUS
registers are in the same word (IIRC, that's the main reason 32-bit
config space accesses are broken). So this isn't going to work at

right, PCI_STATUS has write '1' to clear bits and there's a chance that it
could be reset while raising legacy interrupt. While this cannot be avoided for
TI's J721E, other platforms doesn't have to have this limitation.

quoted

least for EP accesses. And maybe you need a custom .raise_irq() hook
to minimize any problems (such as making the RMW atomic at least from
the endpoint's perspective).

This is to make sure EP doesn't update in-consistent state when RC is updating
the PCI_STATUS register? Since this involves two different systems, how do we
make this atomic?

You can't make it atomic WRT both systems, but is there locking around
each RMW? Specifically, are preemption and interrupts disabled to
ensure time between a read and write are minimized? You wouldn't want
interrupts disabled during the delay too though (i.e. around
.raise_irq()).

Okay, I'll add spin spin_lock_irqsave() in cdns_pcie_write_sz(). As you also
pointed below that delay for legacy interrupt is wrong and it has to be fixed
(with a later series).

How do you want to handle cdns_pcie_ep_fn_writew() now? Because now we are
changing the default implementation to perform only 32-bit access (used for
legacy interrupt, msi-x interrupt and while writing standard headers) and it's
not okay only for legacy interrupts for platforms other than TI.

So just for legacy interrupt, you want me to add a different accessor which
does not perform 32-bit writes (while we add a different .raise_irq for TI
platform?

BTW, I've asked this question before, but aren't PCI legacy interrupts
level triggered? If so, isn't generating a pulse wrong?

You are right. This is wrong and it has to be fixed. I'll work on this later.

Thanks
Kishon

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel