Re: [PATCH v11 3/4] dmaengine: dw-edma: Add support for native HDMA

From: "Köry Maincent" <kory.maincent@bootlin.com>
To: Serge Semin <fancer.lancer@gmail.com>
Cc: "Cai Huoqing" <cai.huoqing@linux.dev>,
	vkoul@kernel.org,
	"Manivannan Sadhasivam" <manivannan.sadhasivam@linaro.org>,
	"Manivannan Sadhasivam" <mani@kernel.org>,
	"Gustavo Pimentel" <gustavo.pimentel@synopsys.com>,
	"Jingoo Han" <jingoohan1@gmail.com>,
	"Lorenzo Pieralisi" <lpieralisi@kernel.org>,
	"Krzysztof Wilczyński" <kw@linux.com>,
	"Rob Herring" <robh@kernel.org>,
	"Bjorn Helgaas" <bhelgaas@google.com>,
	linux-kernel@vger.kernel.org, dmaengine@vger.kernel.org,
	linux-pci@vger.kernel.org,
	"Thomas Petazzoni" <thomas.petazzoni@bootlin.com>
Subject: Re: [PATCH v11 3/4] dmaengine: dw-edma: Add support for native HDMA
Date: Wed, 7 Jun 2023 14:40:14 +0200	[thread overview]
Message-ID: <20230607144014.6356a197@kmaincent-XPS-13-7390> (raw)
In-Reply-To: <20230607114950.zml4l2rs77cbeesy@mobilestation>

On Wed, 7 Jun 2023 14:49:50 +0300
Serge Semin <fancer.lancer@gmail.com> wrote:

> Hi Köry
> First of all. What is akida platform you are referring to? I failed to
> find any mention in the mainline kernel repo.

Yes, sorry akida is the project prefix I am currently working on.
It is simply a prefix for the symbol export to be different than mainline,
don't take it into account.

> > channels by doing the minimum between ll_wr_cnt and the ch_count callback.
> > The hdma ch_count callback is counting the number of channels enabled by
> > reading the number of ch_en registers set. At probe time there is no
> > channels registers that has been set as it is done later in the
> > dw_hdma_v0_core_start function. Then the dw_hdma_v0_core_ch_count will
> > always return 0 at probe time and the number of channels will be set to 0
> > which is not what we want. Could I miss something?  
> 
> Based on the HDMA patches content you are right. The channels must be
> pre-enabled in order to have the dw_hdma_v0_core_ch_count() procedure
> to work correctly otherwise you'll indeed end up with an empty list of
> channels. I don't have any device with the HDMA engine embedded so I
> couldn't have possibly tracked that peculiarity on review. Anyway
> AFAICS Cai just implemented a method which seemed to work for his
> hardware setup.

Alright, on my side I have a board using this FPGA implementation and it
indeed doesn't work as is.

> If you think it doesn't work correctly or it isn't portable enough
> then you are free to provide your own implementation of the method and
> submit a patch. I hope Cai will be willing to test it out to make sure
> that it works correctly for you and his platforms.
> 
> As for me if I were on your place I would have implemented a loop
> which would walk over all possible HDMA channels (HDMA_V0_MAX_NR_CH)
> and counted all channels which could be enabled. If the ch_en CSR is
> writable (that is the channel could be enabled) then it shall be
> considered as existent. Of course before that I would have made sure
> that the non-existent channels had non-writable ch_en CSR.

This could be a nice idea but it doesn't work, non-existent channels seems to
be writable. The datasheet of the HDMA IP doesn't have any register to find out
the maximum existent channel. 
As there is no way to know this, the dw_hdma_v0_core_ch_count will simply
return HDMA_V0_MAX_NR_CH.
Cai how does it works on your side? does the ch_en register already enabled by
the implementation?