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 14:49:50 +0300
From:   Serge Semin <fancer.lancer@gmail.com>
To:     =?utf-8?B?S8O2cnk=?= Maincent <kory.maincent@bootlin.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 =?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: <20230607114950.zml4l2rs77cbeesy@mobilestation>
References: <20230520050854.73160-1-cai.huoqing@linux.dev>
 <20230520050854.73160-4-cai.huoqing@linux.dev>
 <20230607095832.6d6b1a73@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: <20230607095832.6d6b1a73@kmaincent-XPS-13-7390>
Precedence: bulk

Hi K?ry

On Wed, Jun 07, 2023 at 09:58:32AM +0200, K?ry Maincent wrote:
> On Sat, 20 May 2023 13:08:51 +0800
> Cai Huoqing <cai.huoqing@linux.dev> wrote:
> 
> > Add support for HDMA NATIVE, as long the IP design has set
> > the compatible register map parameter-HDMA_NATIVE,
> > which allows compatibility for native HDMA register configuration.
> 
> I know the patch has been merged in dmaengine tree but something seems weird on
> my side.
> 

> The akida_dw_edma_probe function is selecting the minimum read and write

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

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

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.

-Serge(y)

> 
> See the functions bellow:
> 
> > int akida_dw_edma_probe(struct dw_edma_chip *chip)                         
> > {                                                                          
>  ...                               
> >         dw->wr_ch_cnt = min_t(u16, chip->ll_wr_cnt,                        
> >                               dw_edma_core_ch_count(dw, EDMA_DIR_WRITE));  
> >         dw->wr_ch_cnt = min_t(u16, dw->wr_ch_cnt, EDMA_MAX_WR_CH);         
> >                                                                            
> >         dw->rd_ch_cnt = min_t(u16, chip->ll_rd_cnt,                        
> >                               dw_edma_core_ch_count(dw, EDMA_DIR_READ));   
> >         dw->rd_ch_cnt = min_t(u16, dw->rd_ch_cnt, EDMA_MAX_RD_CH);         
> >                                                                            
> >         if (!dw->wr_ch_cnt && !dw->rd_ch_cnt)                              
> >                 return -EINVAL;            
> ...
> }
> 
> 
> > +static u16 dw_hdma_v0_core_ch_count(struct dw_edma *dw, enum dw_edma_dir dir)
> > +{
> > +	u32 num_ch = 0;
> > +	int id;
> > +
> > +	for (id = 0; id < HDMA_V0_MAX_NR_CH; id++) {
> > +		if (GET_CH_32(dw, id, dir, ch_en) & BIT(0))
> > +			num_ch++;
> > +	}
> > +
> > +	if (num_ch > HDMA_V0_MAX_NR_CH)
> > +		num_ch = HDMA_V0_MAX_NR_CH;
> > +
> > +	return (u16)num_ch;
> > +}
> 
> 
> 
> > +static void dw_hdma_v0_core_start(struct dw_edma_chunk *chunk, bool first)
> > +{
> > +	struct dw_edma_chan *chan = chunk->chan;
> > +	struct dw_edma *dw = chan->dw;
> > +	u32 tmp;
> > +
> > +	dw_hdma_v0_core_write_chunk(chunk);
> > +
> > +	if (first) {
> > +		/* Enable engine */
> > +		SET_CH_32(dw, chan->dir, chan->id, ch_en, BIT(0));
> ...
> > +}
>