Re: [PATCH RESEND v5 24/24] PCI: dwc: Add DW eDMA engine support

[Bjorn Helgaas]

On Thu, Aug 25, 2022 at 08:16:14AM +0300, Serge Semin wrote:
> On Wed, Aug 24, 2022 at 01:17:54PM -0500, Bjorn Helgaas wrote:
> > On Wed, Aug 24, 2022 at 09:13:19PM +0300, Serge Semin wrote:
> > > On Wed, Aug 24, 2022 at 11:51:18AM -0500, Bjorn Helgaas wrote:
> > > > On Mon, Aug 22, 2022 at 09:53:32PM +0300, Serge Semin wrote:
> > 
> > > > > +	val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL);
> > > > > +	if (val == 0xFFFFFFFF && pci->edma.reg_base) {
> > > > > +		pci->edma.mf = EDMA_MF_EDMA_UNROLL;
> > > > > +
> > > > > +		val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL);
> > > > > +	} else if (val != 0xFFFFFFFF) {
> > > > 
> > > 
> > > > Consider PCI_POSSIBLE_ERROR() as an annotation about the meaning of
> > > > 0xFFFFFFFF and something to grep for.
> > > 
> > > In this case FFs don't mean an error but a special value, which
> > > indicates that the eDMA is mapped via the unrolled CSRs space. The
> > > similar approach has been implemented for the iATU legacy/unroll setup
> > > auto-detection. So I don't see much reasons to have it grepped, so as
> > > to have a macro-based parametrization since the special value will
> > > unluckily change while having the explicit literal utilized gives a
> > > better understanding of the way the algorithm works.
> 
> > If 0xFFFFFFFF is the result of a successful PCIe Memory Read,
> 
> Right. It is.
> 
> > and not
> > something synthesized by the host bridge when it handles an
> > Unsupported Request completion,
> 
> No it isn't. To be clear 0xFFs don't indicate some PCIe bus/controller
> malfunction, but they are a result of reading the
> DMA_CTRL_VIEWPORT_OFF register which doesn't exist. The manual
> explicitly says: "Note - When register does not exist, value is fixed
> to 32'hFFFF_FFFF". The register doesn't exist if either eDMA is
> unavailable or the eDMA CSRs are mapped via the unrolled state.

OK.  I don't think that's worded very well in the manual.  A register
that does not exist does not have a value, and attempts to read it
should fail.  If they want to say the register always exists and
contains 0xFFFFFFFF for versions earlier than X, that would make
sense.  Wouldn't be the first time a manual is ambiguous ;)

If the device itself, i.e., not the Root Complex, is fabricating this
0xFFFFFFFF value, reading it should not cause any AER or other error
status bits to be set.

If the Root Complex fabricates 0xFFFFFFFF upon receipt of a Completion
with Unsupported Request status, I would expect bits like Received
Master Abort to be set in the Root Port's Secondary Status register.