Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 7 Jun 2023 16:06:53 +0300
From:   Serge Semin <fancer.lancer@gmail.com>
To:     =?utf-8?B?S8O2cnk=?= Maincent <kory.maincent@bootlin.com>,
        Cai Huoqing <cai.huoqing@linux.dev>
Cc:     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 =?utf-8?Q?Wilczy=C5=84ski?= <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
Message-ID: <20230607130653.5hueky3rfih6cnze@mobilestation>
References: <20230520050854.73160-1-cai.huoqing@linux.dev>
 <20230520050854.73160-4-cai.huoqing@linux.dev>
 <20230607095832.6d6b1a73@kmaincent-XPS-13-7390>
 <20230607114950.zml4l2rs77cbeesy@mobilestation>
 <20230607144014.6356a197@kmaincent-XPS-13-7390>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20230607144014.6356a197@kmaincent-XPS-13-7390>
Precedence: bulk

On Wed, Jun 07, 2023 at 02:40:14PM +0200, K?ry Maincent wrote:
> 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.

What about some other CSRs (llp, etc)? Are they all writable even if
the respective channel doesn't exist?

If it isn't possible to auto-detect a number of channels then indeed
we need to come up with a strategy to rely on the platform settings
instead. Your suggestion seems to be the best choice seeing there is a
commit 16b90dd94d3f ("dmaengine: dw-edma: Improve number of channels
check") which converts the rely-on-platform-settings pattern to the
current rely-on-min-between-plat-and-hw-capability.

Cai?

-Serge(y)

> Cai how does it works on your side? does the ch_en register already enabled by
> the implementation?