Re: [PATCH v3 2/5] PCI: dwc: Skip finding eDMA channels count if glue drivers have passed them

[Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>]

On Tue, Feb 27, 2024 at 03:21:00PM +0300, Serge Semin wrote:
> On Tue, Feb 27, 2024 at 02:12:04PM +0530, Manivannan Sadhasivam wrote:
> > On Tue, Feb 27, 2024 at 12:32:44AM +0300, Serge Semin wrote:
> > > On Mon, Feb 26, 2024 at 09:00:14PM +0530, Manivannan Sadhasivam wrote:
> > > > On Mon, Feb 26, 2024 at 03:53:20PM +0300, Serge Semin wrote:
> > > > > On Mon, Feb 26, 2024 at 05:07:27PM +0530, Manivannan Sadhasivam wrote:
> > > > > > In the case of Hyper DMA (HDMA) present in DWC controllers, there is no way
> > > > > > the drivers can auto detect the number of read/write channels as like its
> > > > > > predecessor embedded DMA (eDMA). So the glue drivers making use of HDMA
> > > > > > have to pass the channels count during probe.
> > > > > > 
> > > > > > To accommodate that, let's skip finding the channels if the channels count
> > > > > > were already passed by glue drivers. If the channels count passed were
> > > > > > wrong in any form, then the existing sanity check will catch it.
> > > > > > 
> > > > > > Suggested-by: Serge Semin <fancer.lancer@gmail.com>
> > > > > > Reviewed-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> > > > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > > > > ---
> > > > > >  drivers/pci/controller/dwc/pcie-designware.c | 16 +++++++++-------
> > > > > >  1 file changed, 9 insertions(+), 7 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> > > > > > index 193fcd86cf93..ce273c3c5421 100644
> > > > > > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > > > > > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > > > > > @@ -927,13 +927,15 @@ static int dw_pcie_edma_find_channels(struct dw_pcie *pci)
> > > > > >  {
> > > > > >  	u32 val;
> > > > > >  
> > > > > > -	if (pci->edma.mf == EDMA_MF_EDMA_LEGACY)
> > > > > > -		val = dw_pcie_readl_dbi(pci, PCIE_DMA_VIEWPORT_BASE + PCIE_DMA_CTRL);
> > > > > > -	else
> > > > > > -		val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL);
> > > > > > -
> > > > > > -	pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val);
> > > > > > -	pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val);
> > > > > 
> > > > > > +	if (!pci->edma.ll_wr_cnt || !pci->edma.ll_rd_cnt) {
> > > > > 
> > > > > Are you sure that the partly initialized case should be considered as
> > > > > a request for the auto-detection? IMO &&-ing here and letting the
> > > > > sanity check to fail further would be more correct since thus the
> > > > > developer would know about improper initialized data.
> > > > > 
> > > > 
> > > > We already have the check below. So the partly initialized case will fail
> > > > anyway.
> > > 
> > > Not really. If the partly initialized case activates the
> > > auto-detection procedure it will override both non-initialized and
> > > _initialized_ number of channels with the values retrieved from the
> > > hardware, which the glue driver has been willing not to use. This
> > > prone to undefined behavior depending on the reasons of skipping the
> > > auto-detection procedure. For instance, assume the DMA_CTRL register
> > > reports an invalid number of read channels. A glue driver by mistake
> > > or willingly overwrites the pci->edma.ll_rd_cnt field only. This won't
> > > solve the problem since the auto-detection will be proceeded due to
> > > the pci->edma.ll_wr_cnt field being left uninitialized.
> > > 
> > > So to speak it would be better to implement a strictly determined case
> > > for activating the auto-detection procedure: both number of channels
> > > aren't initialized; if only one field is initialized then report an
> > > error.
> > > 
> > > Alternatively we can have the auto-detection executed on the
> > > per-channel basis:
> > > 
> > > +	if (pci->edma.mf != EDMA_MF_HDMA_NATIVE) {
> > > +		val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL);
> > > +
> > > +		if (!pci->edma.ll_wr_cnt)
> > > +			pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val);
> > > +
> > > +		if (!pci->edma.ll_rd_cnt)
> > > +			pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val);
> > > +	}
> > > 
> > 
> > Hmm, in this case there is no need to check for uninitialized channels count:
> > 
> > 	/*
> > 	 * Autodetect the read/write channels count only for non-HDMA platforms.
> > 	 * HDMA platforms doesn't support autodetect, so the glue drivers should've
> > 	 * passed the valid count already. If not, the below sanity check will
> > 	 * catch it.
> > 	 */
> > 	if (pci->edma.mf != EDMA_MF_HDMA_NATIVE) {
> > 		val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL);
> > 
> > 		pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val);
> > 		pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val);
> > 	}
> > 
> > 	/* Sanity check */
> 
> That is another possible implementation. Let's sum all of them up:
> 
> 1. Channel fields-base conditional statement:
> +	if (!pci->edma.ll_wr_cnt && !pci->edma.ll_rd_cnt) {
> +		val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL);
> +
> +		pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val);
> +		pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val);
> +	}
> pros: NoF channels override support for all IP-cores; simple.
> cons: incompatible with HDMA, but can be taken by mistake/bug; no
> partial NoF channels pre-initialization.
> 
> 2. Channel fields-base conditional statement with logical OR operator #1:
> +	if (!pci->edma.ll_wr_cnt || !pci->edma.ll_rd_cnt) {
> +		val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL);
> +
> +		pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val);
> +		pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val);
> +	}
> pros: NoF channels override support for all IP-cores; simple.
> cons: incompatible with HDMA, but can be taken by mistake/bug; no
> partial NoF channels pre-initialization; silently overrides the
> partial NoF channels case.
> 
> 3. Channel fields-base conditional statement with logical OR operator #2:
> +	if (!pci->edma.ll_wr_cnt || !pci->edma.ll_rd_cnt) {
> +		val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL);
> +
> +		if (!pci->edma.ll_wr_cnt)
> +			pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val);
> +
> +		if (!pci->edma.ll_rd_cnt)
> +			pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val);
> +	}
> pros: NoF channels override support for all IP-cores; partial NoF
> channels pre-initialization support.
> cons: incompatible with HDMA, but can be taken by mistake/bug; more
> complex (and actually looking a bit clumsy due to two conditional
> statements over the same fields).
> 
> 4. Unconditional auto-detection:
> +	val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL);
> +
> +	if (!pci->edma.ll_wr_cnt)
> +		pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val);
> +
> +	if (!pci->edma.ll_rd_cnt)
> +		pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val);
> pros: NoF channels override support for all IP-cores; partial NoF
> channels pre-initialization support; simple.
> cons: incompatible with HDMA, but will be executed for it anyway so
> the NoF channels fields will be overridden with the Channel#0.prefetch
> CSR data if haven't been pre-initialized;
> 
> 5. Mapping format-based conditional statement:
> +	if (pci->edma.mf != EDMA_MF_HDMA_NATIVE) {
> +		val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL);
> +
> +		pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val);
> +		pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val);
> +	}
> pros: free of being executed for HDMA IP-core, simple
> cons: no NoF channels override support for non-HDMA IP-cores.
> 

Is it possible for the non-HDMA IPs to override the channels count? Atleast any
such IPs supported in mainline now? If not, then I'd like to go with this
approach.

Because, this makes it explicit that override is only supported for HDMA IPs and
also simplifies the logic.

> 6. Mapping format-based conditional statement with partial NoF channels override:
> +	if (pci->edma.mf != EDMA_MF_HDMA_NATIVE) {
> +		val = dw_pcie_readl_dma(pci, PCIE_DMA_CTRL);
> +
> +		if (!pci->edma.ll_wr_cnt)
> +			pci->edma.ll_wr_cnt = FIELD_GET(PCIE_DMA_NUM_WR_CHAN, val);
> +
> +		if (!pci->edma.ll_rd_cnt)
> +			pci->edma.ll_rd_cnt = FIELD_GET(PCIE_DMA_NUM_RD_CHAN, val);
> +	}
> pros: free of being executed for HDMA IP-core; NoF channels override
> support for all IP-cores.
> cons: more complex.
> 
> 
> Looking at all of that I'd say that options 5 and 6 seems better to me
> now since they prohibit the auto-detection for HDMA IP-cores which
> have the Channel#0.prefetch CSR at the 0x8 offset. I don't have strong
> opinion which of those two to choose. If you think simplicity is
> preferable, then option 2 will be enough. If you wish to have the NoF
> channels override supported for all IP-cores, then option 3 will work
> for it.
> 

Thanks for such elaborative comparision :)

- Mani

-- 
மணிவண்ணன் சதாசிவம்