Hi Rob,

On 5/28/2020 3:36 AM, Kishon Vijay Abraham I wrote:

Hi Rob,

On 5/27/2020 10:07 PM, Rob Herring wrote:

quoted

On Wed, May 27, 2020 at 4:49 AM Kishon Vijay Abraham I [off-list ref] wrote:

quoted

Hi Rob,

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

quoted

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

But you don't need a lock everywhere. You need locks in the callers
(and only sometimes).

Okay, the locks should be added only for registers where HOST can also write to
the same register? Maybe only raise_irq then..

quoted

quoted

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.

Now I'm wondering how set_msi is not racy in the current code with the
host setting/clearing PCI_MSI_FLAGS_ENABLE? Maybe that bit is RO from
the EP side?

set_msi/set_msix is a one time configuration that is invoked before the host
establishes the link with the endpoint. I don't think we have to consider this
as racy.

Can we try to close on this discussion please?

Thanks
Kishon

Thanks
Kishon

quoted

Ultimately I think you're going to have to provide your own endpoint
functions or you need accessors for specific registers like
PCI_MSI_FLAGS. Then for example, you just rely on the 2 bytes before
PCI_MSI_FLAGS being reserved and do a 32-bit access without a RMW.
Trying to abstract this at the register read/write level is going to
be fragile

Rob

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