dmaengine: dw-edma: Add support for native HDMA

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.

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;            

drivers/dma/dw-edma/dw-hdma-v0-core.c new file +26 -0

+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));

+}

Quoted 3 lines · On Sat, 20 May 2023 13:08:51 +0800 Cai Huoqing <[email protected]> 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

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?

See the functions bellow:

Quoted 2 lines

int akida_dw_edma_probe(struct dw_edma_chip *chip) {

...

Quoted 10 lines

        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;            

...
}

Quoted 29 lines

drivers/dma/dw-edma/dw-hdma-v0-core.c new file +26 -0

+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));

...

Quoted 1 line

+}

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.

Quoted 7 lines

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.

Quoted 3 lines · On Wed, 7 Jun 2023 14:49:50 +0300 Serge Semin <[email protected]> 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.

Quoted 8 lines

Quoted 7 lines

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?

Quoted 7 lines

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.

Quoted 11 lines

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?

Quoted 3 lines · On Sat, 20 May 2023 13:08:51 +0800 Cai Huoqing <[email protected]> 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 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?

See the functions bellow:

Quoted 2 lines

int akida_dw_edma_probe(struct dw_edma_chip *chip) {

...

Quoted 10 lines

        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;            

...
}

Quoted 29 lines

drivers/dma/dw-edma/dw-hdma-v0-core.c new file +26 -0

+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));

...

Quoted 1 line

+}

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.

The HDMA Hyper-DMA IP is an enhancement of the eDMA embedded-DMA IP. And the native HDMA registers are different from eDMA,
so this patch add support for HDMA NATIVE mode.

HDMA write and read channels operate independently to maximize the performance of the HDMA read and write data transfer over the link When you configure the HDMA with multiple read channels, then it uses a round robin (RR) arbitration scheme to select the next read channel to be serviced.The same applies when youhave multiple write channels.

The native HDMA driver also supports a maximum of 16 independent channels (8 write + 8 read), which can run simultaneously. Both SAR (Source Address Register) and DAR (Destination Address Register) are aligned to byte.