[PATCH RFC 1/8] dmaengine: Actions: get rid of bit fields from dma descriptor

[PATCH RFC 1/8] dmaengine: Actions: get rid of bit fields from dma descriptor

[Amit Singh Tomar]

At the moment, Driver uses bit fields to describe registers of the DMA
descriptor structure that makes it less portable and maintainable, and
Andre suugested(and even sketched important bits for it) to make use of
array to describe this DMA descriptors instead. It gives the flexibility
while extending support for other platform such as Actions S700.

This commit removes the "owl_dma_lli_hw" (that includes bit-fields) and
uses array to describe DMA descriptor.

Suggested-by: Andre Przywara <andre.przywara@arm.com>
Signed-off-by: Amit Singh Tomar <amittomer25@gmail.com>
---
 drivers/dma/owl-dma.c | 77 ++++++++++++++++++++++-----------------------------
 1 file changed, 33 insertions(+), 44 deletions(-)

diff --git a/drivers/dma/owl-dma.c b/drivers/dma/owl-dma.c
index c683051257fd..b0d80a2fa383 100644
--- a/drivers/dma/owl-dma.c
+++ b/drivers/dma/owl-dma.c
@@ -120,30 +120,18 @@
 #define BIT_FIELD(val, width, shift, newshift)	\
 		((((val) >> (shift)) & ((BIT(width)) - 1)) << (newshift))
 
-/**
- * struct owl_dma_lli_hw - Hardware link list for dma transfer
- * @next_lli: physical address of the next link list
- * @saddr: source physical address
- * @daddr: destination physical address
- * @flen: frame length
- * @fcnt: frame count
- * @src_stride: source stride
- * @dst_stride: destination stride
- * @ctrla: dma_mode and linklist ctrl config
- * @ctrlb: interrupt config
- * @const_num: data for constant fill
- */
-struct owl_dma_lli_hw {
-	u32	next_lli;
-	u32	saddr;
-	u32	daddr;
-	u32	flen:20;
-	u32	fcnt:12;
-	u32	src_stride;
-	u32	dst_stride;
-	u32	ctrla;
-	u32	ctrlb;
-	u32	const_num;
+/* Describe DMA descriptor, hardware link list for dma transfer */
+enum owl_dmadesc_offsets {
+	OWL_DMADESC_NEXT_LLI = 0,
+	OWL_DMADESC_SADDR,
+	OWL_DMADESC_DADDR,
+	OWL_DMADESC_FLEN,
+	OWL_DMADESC_SRC_STRIDE,
+	OWL_DMADESC_DST_STRIDE,
+	OWL_DMADESC_CTRLA,
+	OWL_DMADESC_CTRLB,
+	OWL_DMADESC_CONST_NUM,
+	OWL_DMADESC_SIZE
 };
 
 /**
@@ -153,7 +141,7 @@ struct owl_dma_lli_hw {
  * @node: node for txd's lli_list
  */
 struct owl_dma_lli {
-	struct  owl_dma_lli_hw	hw;
+	u32			hw[OWL_DMADESC_SIZE];
 	dma_addr_t		phys;
 	struct list_head	node;
 };
@@ -351,8 +339,9 @@ static struct owl_dma_lli *owl_dma_add_lli(struct owl_dma_txd *txd,
 		list_add_tail(&next->node, &txd->lli_list);
 
 	if (prev) {
-		prev->hw.next_lli = next->phys;
-		prev->hw.ctrla |= llc_hw_ctrla(OWL_DMA_MODE_LME, 0);
+		prev->hw[OWL_DMADESC_NEXT_LLI] = next->phys;
+		prev->hw[OWL_DMADESC_CTRLA] |=
+					llc_hw_ctrla(OWL_DMA_MODE_LME, 0);
 	}
 
 	return next;
@@ -365,8 +354,7 @@ static inline int owl_dma_cfg_lli(struct owl_dma_vchan *vchan,
 				  struct dma_slave_config *sconfig,
 				  bool is_cyclic)
 {
-	struct owl_dma_lli_hw *hw = &lli->hw;
-	u32 mode;
+	u32 mode, ctrlb;
 
 	mode = OWL_DMA_MODE_PW(0);
 
@@ -407,22 +395,22 @@ static inline int owl_dma_cfg_lli(struct owl_dma_vchan *vchan,
 		return -EINVAL;
 	}
 
-	hw->next_lli = 0; /* One link list by default */
-	hw->saddr = src;
-	hw->daddr = dst;
-
-	hw->fcnt = 1; /* Frame count fixed as 1 */
-	hw->flen = len; /* Max frame length is 1MB */
-	hw->src_stride = 0;
-	hw->dst_stride = 0;
-	hw->ctrla = llc_hw_ctrla(mode,
-				 OWL_DMA_LLC_SAV_LOAD_NEXT |
-				 OWL_DMA_LLC_DAV_LOAD_NEXT);
+	lli->hw[OWL_DMADESC_CTRLA] = llc_hw_ctrla(mode,
+						  OWL_DMA_LLC_SAV_LOAD_NEXT |
+						  OWL_DMA_LLC_DAV_LOAD_NEXT);
 
 	if (is_cyclic)
-		hw->ctrlb = llc_hw_ctrlb(OWL_DMA_INTCTL_BLOCK);
+		ctrlb = llc_hw_ctrlb(OWL_DMA_INTCTL_BLOCK);
 	else
-		hw->ctrlb = llc_hw_ctrlb(OWL_DMA_INTCTL_SUPER_BLOCK);
+		ctrlb = llc_hw_ctrlb(OWL_DMA_INTCTL_SUPER_BLOCK);
+
+	lli->hw[OWL_DMADESC_NEXT_LLI] = 0;
+	lli->hw[OWL_DMADESC_SADDR] = src;
+	lli->hw[OWL_DMADESC_DADDR] = dst;
+	lli->hw[OWL_DMADESC_SRC_STRIDE] = 0;
+	lli->hw[OWL_DMADESC_DST_STRIDE] = 0;
+	lli->hw[OWL_DMADESC_FLEN] = len | 1 << 20;
+	lli->hw[OWL_DMADESC_CTRLB] = ctrlb;
 
 	return 0;
 }
@@ -754,7 +742,8 @@ static u32 owl_dma_getbytes_chan(struct owl_dma_vchan *vchan)
 			/* Start from the next active node */
 			if (lli->phys == next_lli_phy) {
 				list_for_each_entry(lli, &txd->lli_list, node)
-					bytes += lli->hw.flen;
+					bytes += lli->hw[OWL_DMADESC_FLEN] &
+						 GENMASK(19, 0);
 				break;
 			}
 		}
@@ -785,7 +774,7 @@ static enum dma_status owl_dma_tx_status(struct dma_chan *chan,
 	if (vd) {
 		txd = to_owl_txd(&vd->tx);
 		list_for_each_entry(lli, &txd->lli_list, node)
-			bytes += lli->hw.flen;
+			bytes += lli->hw[OWL_DMADESC_FLEN] & GENMASK(19, 0);
 	} else {
 		bytes = owl_dma_getbytes_chan(vchan);
 	}
-- 
2.7.4

Re: [PATCH RFC 1/8] dmaengine: Actions: get rid of bit fields from dma descriptor

[Manivannan Sadhasivam]

Hi,

On Wed, May 06, 2020 at 04:06:03PM +0530, Amit Singh Tomar wrote:
> At the moment, Driver uses bit fields to describe registers of the DMA
> descriptor structure that makes it less portable and maintainable, and
> Andre suugested(and even sketched important bits for it) to make use of
> array to describe this DMA descriptors instead. It gives the flexibility
> while extending support for other platform such as Actions S700.
> 
> This commit removes the "owl_dma_lli_hw" (that includes bit-fields) and
> uses array to describe DMA descriptor.
> 

I'm in favor of getting rid of bitfields due to its not so defined way of
working (and forgive me for using it in first place) but I don't quite like
the current approach.

Rather I'd like to have custom bitmasks (S900/S700/S500?) for writing to those
fields.

Thanks,
Mani

> Suggested-by: Andre Przywara <andre.przywara@arm.com>
> Signed-off-by: Amit Singh Tomar <amittomer25@gmail.com>
> ---
>  drivers/dma/owl-dma.c | 77 ++++++++++++++++++++++-----------------------------
>  1 file changed, 33 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/dma/owl-dma.c b/drivers/dma/owl-dma.c
> index c683051257fd..b0d80a2fa383 100644
> --- a/drivers/dma/owl-dma.c
> +++ b/drivers/dma/owl-dma.c
> @@ -120,30 +120,18 @@
>  #define BIT_FIELD(val, width, shift, newshift)	\
>  		((((val) >> (shift)) & ((BIT(width)) - 1)) << (newshift))
>  
> -/**
> - * struct owl_dma_lli_hw - Hardware link list for dma transfer
> - * @next_lli: physical address of the next link list
> - * @saddr: source physical address
> - * @daddr: destination physical address
> - * @flen: frame length
> - * @fcnt: frame count
> - * @src_stride: source stride
> - * @dst_stride: destination stride
> - * @ctrla: dma_mode and linklist ctrl config
> - * @ctrlb: interrupt config
> - * @const_num: data for constant fill
> - */
> -struct owl_dma_lli_hw {
> -	u32	next_lli;
> -	u32	saddr;
> -	u32	daddr;
> -	u32	flen:20;
> -	u32	fcnt:12;
> -	u32	src_stride;
> -	u32	dst_stride;
> -	u32	ctrla;
> -	u32	ctrlb;
> -	u32	const_num;
> +/* Describe DMA descriptor, hardware link list for dma transfer */
> +enum owl_dmadesc_offsets {
> +	OWL_DMADESC_NEXT_LLI = 0,
> +	OWL_DMADESC_SADDR,
> +	OWL_DMADESC_DADDR,
> +	OWL_DMADESC_FLEN,
> +	OWL_DMADESC_SRC_STRIDE,
> +	OWL_DMADESC_DST_STRIDE,
> +	OWL_DMADESC_CTRLA,
> +	OWL_DMADESC_CTRLB,
> +	OWL_DMADESC_CONST_NUM,
> +	OWL_DMADESC_SIZE
>  };
>  
>  /**
> @@ -153,7 +141,7 @@ struct owl_dma_lli_hw {
>   * @node: node for txd's lli_list
>   */
>  struct owl_dma_lli {
> -	struct  owl_dma_lli_hw	hw;
> +	u32			hw[OWL_DMADESC_SIZE];
>  	dma_addr_t		phys;
>  	struct list_head	node;
>  };
> @@ -351,8 +339,9 @@ static struct owl_dma_lli *owl_dma_add_lli(struct owl_dma_txd *txd,
>  		list_add_tail(&next->node, &txd->lli_list);
>  
>  	if (prev) {
> -		prev->hw.next_lli = next->phys;
> -		prev->hw.ctrla |= llc_hw_ctrla(OWL_DMA_MODE_LME, 0);
> +		prev->hw[OWL_DMADESC_NEXT_LLI] = next->phys;
> +		prev->hw[OWL_DMADESC_CTRLA] |=
> +					llc_hw_ctrla(OWL_DMA_MODE_LME, 0);
>  	}
>  
>  	return next;
> @@ -365,8 +354,7 @@ static inline int owl_dma_cfg_lli(struct owl_dma_vchan *vchan,
>  				  struct dma_slave_config *sconfig,
>  				  bool is_cyclic)
>  {
> -	struct owl_dma_lli_hw *hw = &lli->hw;
> -	u32 mode;
> +	u32 mode, ctrlb;
>  
>  	mode = OWL_DMA_MODE_PW(0);
>  
> @@ -407,22 +395,22 @@ static inline int owl_dma_cfg_lli(struct owl_dma_vchan *vchan,
>  		return -EINVAL;
>  	}
>  
> -	hw->next_lli = 0; /* One link list by default */
> -	hw->saddr = src;
> -	hw->daddr = dst;
> -
> -	hw->fcnt = 1; /* Frame count fixed as 1 */
> -	hw->flen = len; /* Max frame length is 1MB */
> -	hw->src_stride = 0;
> -	hw->dst_stride = 0;
> -	hw->ctrla = llc_hw_ctrla(mode,
> -				 OWL_DMA_LLC_SAV_LOAD_NEXT |
> -				 OWL_DMA_LLC_DAV_LOAD_NEXT);
> +	lli->hw[OWL_DMADESC_CTRLA] = llc_hw_ctrla(mode,
> +						  OWL_DMA_LLC_SAV_LOAD_NEXT |
> +						  OWL_DMA_LLC_DAV_LOAD_NEXT);
>  
>  	if (is_cyclic)
> -		hw->ctrlb = llc_hw_ctrlb(OWL_DMA_INTCTL_BLOCK);
> +		ctrlb = llc_hw_ctrlb(OWL_DMA_INTCTL_BLOCK);
>  	else
> -		hw->ctrlb = llc_hw_ctrlb(OWL_DMA_INTCTL_SUPER_BLOCK);
> +		ctrlb = llc_hw_ctrlb(OWL_DMA_INTCTL_SUPER_BLOCK);
> +
> +	lli->hw[OWL_DMADESC_NEXT_LLI] = 0;
> +	lli->hw[OWL_DMADESC_SADDR] = src;
> +	lli->hw[OWL_DMADESC_DADDR] = dst;
> +	lli->hw[OWL_DMADESC_SRC_STRIDE] = 0;
> +	lli->hw[OWL_DMADESC_DST_STRIDE] = 0;
> +	lli->hw[OWL_DMADESC_FLEN] = len | 1 << 20;
> +	lli->hw[OWL_DMADESC_CTRLB] = ctrlb;
>  
>  	return 0;
>  }
> @@ -754,7 +742,8 @@ static u32 owl_dma_getbytes_chan(struct owl_dma_vchan *vchan)
>  			/* Start from the next active node */
>  			if (lli->phys == next_lli_phy) {
>  				list_for_each_entry(lli, &txd->lli_list, node)
> -					bytes += lli->hw.flen;
> +					bytes += lli->hw[OWL_DMADESC_FLEN] &
> +						 GENMASK(19, 0);
>  				break;
>  			}
>  		}
> @@ -785,7 +774,7 @@ static enum dma_status owl_dma_tx_status(struct dma_chan *chan,
>  	if (vd) {
>  		txd = to_owl_txd(&vd->tx);
>  		list_for_each_entry(lli, &txd->lli_list, node)
> -			bytes += lli->hw.flen;
> +			bytes += lli->hw[OWL_DMADESC_FLEN] & GENMASK(19, 0);
>  	} else {
>  		bytes = owl_dma_getbytes_chan(vchan);
>  	}
> -- 
> 2.7.4
> 

Re: [PATCH RFC 1/8] dmaengine: Actions: get rid of bit fields from dma descriptor

[Amit Tomer]

Hi

Thanks for the reply.

> I'm in favor of getting rid of bitfields due to its not so defined way of
> working (and forgive me for using it in first place) but I don't quite like
> the current approach.

Because , its less readable the way we are writing to those different fields ?
But this can be made more verbose by adding some comments around .

> Rather I'd like to have custom bitmasks (S900/S700/S500?) for writing to those
> fields.
>
I think S900 and S500 are same as pointed out by Cristian. and I didn't get by
creating custom bitmasks for it ?

Did you mean function like:

lli->hw[OWL_DMADESC_FLEN]= llc_hw_FLEN(len, FCNT_VALUE, FCNT_SHIFT);

Thanks
-Amit

Re: [PATCH RFC 1/8] dmaengine: Actions: get rid of bit fields from dma descriptor

[Manivannan Sadhasivam]

On Mon, May 11, 2020 at 04:15:57PM +0530, Amit Tomer wrote:
> Hi
> 
> Thanks for the reply.
> 
> > I'm in favor of getting rid of bitfields due to its not so defined way of
> > working (and forgive me for using it in first place) but I don't quite like
> > the current approach.
> 
> Because , its less readable the way we are writing to those different fields ?
> But this can be made more verbose by adding some comments around .
> 

I don't like the way the hw linked lists are accessed (using an array with
enums).

> > Rather I'd like to have custom bitmasks (S900/S700/S500?) for writing to those
> > fields.
> >
> I think S900 and S500 are same as pointed out by Cristian. and I didn't get by
> creating custom bitmasks for it ?
> 
> Did you mean function like:
> 
> lli->hw[OWL_DMADESC_FLEN]= llc_hw_FLEN(len, FCNT_VALUE, FCNT_SHIFT);
> 

I meant to keep using old struct for accessing the linked list and replacing
bitfields with masks as below:

struct owl_dma_lli_hw {
	...
        u32     flen;
        u32     fcnt;
	...
};

hw->flen = len & OWL_S900_DMA_FLEN_MASK;
hw->fcnt = 1 & OWL_S900_DMA_FCNT_MASK;

Then you can use different masks for S700/S900 based on the compatible.

Thanks,
Mani

> Thanks
> -Amit

Re: [PATCH RFC 1/8] dmaengine: Actions: get rid of bit fields from dma descriptor

[André Przywara]

On 11/05/2020 12:20, Manivannan Sadhasivam wrote:

Hi,

> On Mon, May 11, 2020 at 04:15:57PM +0530, Amit Tomer wrote:
>> Hi
>>
>> Thanks for the reply.
>>
>>> I'm in favor of getting rid of bitfields due to its not so defined way of
>>> working (and forgive me for using it in first place) but I don't quite like
>>> the current approach.
>>
>> Because , its less readable the way we are writing to those different fields ?
>> But this can be made more verbose by adding some comments around .
>>
> 
> I don't like the way the hw linked lists are accessed (using an array with
> enums).

But honestly this is the most sane way of doing this, see below.

>>> Rather I'd like to have custom bitmasks (S900/S700/S500?) for writing to those
>>> fields.
>>>
>> I think S900 and S500 are same as pointed out by Cristian. and I didn't get by
>> creating custom bitmasks for it ?
>>
>> Did you mean function like:
>>
>> lli->hw[OWL_DMADESC_FLEN]= llc_hw_FLEN(len, FCNT_VALUE, FCNT_SHIFT);
>>
> 
> I meant to keep using old struct for accessing the linked list and replacing
> bitfields with masks as below:
> 
> struct owl_dma_lli_hw {
> 	...
>         u32     flen;
>         u32     fcnt;
> 	...
> };

And is think this is the wrong way of modelling hardware defined
register fields. C structs have no guarantee of not introducing padding
in between fields, the only guarantee you get is that the first member
has no padding *before* it:
C standard, section 6.7.2.1, end of paragraph 15:
"There may be unnamed padding within a structure object, but not at its
beginning."

Arrays in C on the contrary have very much this guarantee: The members
are next to each other, no padding.

I see that structs are sometimes used in this function, but it's much
less common in the kernel than in other projects (U-Boot comes to mind).
It typically works, because common compiler *implementations* provide
this guarantee, but we should not rely on this.

So:
Using enums for the keys provides a natural way of increasing indices,
without gaps. Also you get this nice and automatic size value by making
this the last member of the enum.
Arrays provide the guarantee of consecutive allocation.

We can surely have a look at the masking problem, but this would need to
be runtime determined masks, which tend to become "wordy". There can be
simplifications, for instance I couldn't find where the frame length is
really limited for the S900 (it must be less than 1MB). Since the S700
supports *more* than that, there is no need to limit this differently.

Cheers,
Andre.


> 
> hw->flen = len & OWL_S900_DMA_FLEN_MASK;
> hw->fcnt = 1 & OWL_S900_DMA_FCNT_MASK;
> 
> Then you can use different masks for S700/S900 based on the compatible.
> 
> Thanks,
> Mani
> 
>> Thanks
>> -Amit

Re: [PATCH RFC 1/8] dmaengine: Actions: get rid of bit fields from dma descriptor

[Manivannan Sadhasivam]

On Mon, May 11, 2020 at 12:44:26PM +0100, André Przywara wrote:
> On 11/05/2020 12:20, Manivannan Sadhasivam wrote:
> 
> Hi,
> 
> > On Mon, May 11, 2020 at 04:15:57PM +0530, Amit Tomer wrote:
> >> Hi
> >>
> >> Thanks for the reply.
> >>
> >>> I'm in favor of getting rid of bitfields due to its not so defined way of
> >>> working (and forgive me for using it in first place) but I don't quite like
> >>> the current approach.
> >>
> >> Because , its less readable the way we are writing to those different fields ?
> >> But this can be made more verbose by adding some comments around .
> >>
> > 
> > I don't like the way the hw linked lists are accessed (using an array with
> > enums).
> 
> But honestly this is the most sane way of doing this, see below.
> 
> >>> Rather I'd like to have custom bitmasks (S900/S700/S500?) for writing to those
> >>> fields.
> >>>
> >> I think S900 and S500 are same as pointed out by Cristian. and I didn't get by
> >> creating custom bitmasks for it ?
> >>
> >> Did you mean function like:
> >>
> >> lli->hw[OWL_DMADESC_FLEN]= llc_hw_FLEN(len, FCNT_VALUE, FCNT_SHIFT);
> >>
> > 
> > I meant to keep using old struct for accessing the linked list and replacing
> > bitfields with masks as below:
> > 
> > struct owl_dma_lli_hw {
> > 	...
> >         u32     flen;
> >         u32     fcnt;
> > 	...
> > };
> 
> And is think this is the wrong way of modelling hardware defined
> register fields. C structs have no guarantee of not introducing padding
> in between fields, the only guarantee you get is that the first member
> has no padding *before* it:
> C standard, section 6.7.2.1, end of paragraph 15:
> "There may be unnamed padding within a structure object, but not at its
> beginning."
> 
> Arrays in C on the contrary have very much this guarantee: The members
> are next to each other, no padding.
> 
> I see that structs are sometimes used in this function, but it's much
> less common in the kernel than in other projects (U-Boot comes to mind).
> It typically works, because common compiler *implementations* provide
> this guarantee, but we should not rely on this.
> 
> So:
> Using enums for the keys provides a natural way of increasing indices,
> without gaps. Also you get this nice and automatic size value by making
> this the last member of the enum.
> Arrays provide the guarantee of consecutive allocation.
> 

I agree with your concerns of using struct for defining registers. But we can
safely live with the existing implementation since all fields are u32 and if
needed we can also add '__packed' flag to it to avoid padding for any cases.

The reason why I prefer to stick to this is, this is a hardware linked list and
by defining it as an array and accessing the fields using enums looks awful to
me. Other than that there is no real justification to shy away.

When you are modelling a plain register bank (which we are also doing in this
driver), I'd prefer to use the defines directly.

> We can surely have a look at the masking problem, but this would need to
> be runtime determined masks, which tend to become "wordy". There can be
> simplifications, for instance I couldn't find where the frame length is
> really limited for the S900 (it must be less than 1MB). Since the S700
> supports *more* than that, there is no need to limit this differently.

I was just giving an example of how to handle the bitmasks for different
SoCs if needed. So yeah if it can be avoided, feel free to drop it.

Thanks,
Mani

> 
> Cheers,
> Andre.
> 
> 
> > 
> > hw->flen = len & OWL_S900_DMA_FLEN_MASK;
> > hw->fcnt = 1 & OWL_S900_DMA_FCNT_MASK;
> > 
> > Then you can use different masks for S700/S900 based on the compatible.
> > 
> > Thanks,
> > Mani
> > 
> >> Thanks
> >> -Amit
> 

Re: [PATCH RFC 1/8] dmaengine: Actions: get rid of bit fields from dma descriptor

[André Przywara]

On 11/05/2020 13:04, Manivannan Sadhasivam wrote:

Hi,

> On Mon, May 11, 2020 at 12:44:26PM +0100, André Przywara wrote:
>> On 11/05/2020 12:20, Manivannan Sadhasivam wrote:
>>
>> Hi,
>>
>>> On Mon, May 11, 2020 at 04:15:57PM +0530, Amit Tomer wrote:
>>>> Hi
>>>>
>>>> Thanks for the reply.
>>>>
>>>>> I'm in favor of getting rid of bitfields due to its not so defined way of
>>>>> working (and forgive me for using it in first place) but I don't quite like
>>>>> the current approach.
>>>>
>>>> Because , its less readable the way we are writing to those different fields ?
>>>> But this can be made more verbose by adding some comments around .
>>>>
>>>
>>> I don't like the way the hw linked lists are accessed (using an array with
>>> enums).
>>
>> But honestly this is the most sane way of doing this, see below.
>>
>>>>> Rather I'd like to have custom bitmasks (S900/S700/S500?) for writing to those
>>>>> fields.
>>>>>
>>>> I think S900 and S500 are same as pointed out by Cristian. and I didn't get by
>>>> creating custom bitmasks for it ?
>>>>
>>>> Did you mean function like:
>>>>
>>>> lli->hw[OWL_DMADESC_FLEN]= llc_hw_FLEN(len, FCNT_VALUE, FCNT_SHIFT);
>>>>
>>>
>>> I meant to keep using old struct for accessing the linked list and replacing
>>> bitfields with masks as below:
>>>
>>> struct owl_dma_lli_hw {
>>> 	...
>>>         u32     flen;
>>>         u32     fcnt;
>>> 	...
>>> };
>>
>> And is think this is the wrong way of modelling hardware defined
>> register fields. C structs have no guarantee of not introducing padding
>> in between fields, the only guarantee you get is that the first member
>> has no padding *before* it:
>> C standard, section 6.7.2.1, end of paragraph 15:
>> "There may be unnamed padding within a structure object, but not at its
>> beginning."
>>
>> Arrays in C on the contrary have very much this guarantee: The members
>> are next to each other, no padding.
>>
>> I see that structs are sometimes used in this function, but it's much
>> less common in the kernel than in other projects (U-Boot comes to mind).
>> It typically works, because common compiler *implementations* provide
>> this guarantee, but we should not rely on this.
>>
>> So:
>> Using enums for the keys provides a natural way of increasing indices,
>> without gaps. Also you get this nice and automatic size value by making
>> this the last member of the enum.
>> Arrays provide the guarantee of consecutive allocation.
>>
> 
> I agree with your concerns of using struct for defining registers. But we can
> safely live with the existing implementation since all fields are u32 and if

But why, actually? I can understand that this is done in existing code,
because this was done in the past and apparently never challenged. And
since it seems to work, at least, there is probably not much reason to
change it, just for the sake of it.
But if we need to rework this anyway, we should do the right thing. This
is especially true in the Linux kernel, which is highly critical and
privileged code and also aims to be very portable. We should take no
chances here.

Honestly I don't understand the advantage of using a struct here,
especially if you need to play some tricks (__packed__) to make it work.
So why is:
	hw->flen
so much better than
	hw[DMA_FLEN]
that it justifies to introduce dodgy code?

In think in general we should be much more careful when using C language
constructs to access hardware or hardware defined data structures, and
be it to not give people the wrong idea about this.
I think with the advance of more optimising compilers (and, somewhat
related, more out-of-order CPUs) the chance of breakage becomes much
higher here.

Cheers,
Andre.

> needed we can also add '__packed' flag to it to avoid padding for any cases.
> 
> The reason why I prefer to stick to this is, this is a hardware linked list and
> by defining it as an array and accessing the fields using enums looks awful to
> me. Other than that there is no real justification to shy away.
> 
> When you are modelling a plain register bank (which we are also doing in this
> driver), I'd prefer to use the defines directly.
> 
>> We can surely have a look at the masking problem, but this would need to
>> be runtime determined masks, which tend to become "wordy". There can be
>> simplifications, for instance I couldn't find where the frame length is
>> really limited for the S900 (it must be less than 1MB). Since the S700
>> supports *more* than that, there is no need to limit this differently.
> 
> I was just giving an example of how to handle the bitmasks for different
> SoCs if needed. So yeah if it can be avoided, feel free to drop it.
> 
> Thanks,
> Mani
> 
>>
>> Cheers,
>> Andre.
>>
>>
>>>
>>> hw->flen = len & OWL_S900_DMA_FLEN_MASK;
>>> hw->fcnt = 1 & OWL_S900_DMA_FCNT_MASK;
>>>
>>> Then you can use different masks for S700/S900 based on the compatible.
>>>
>>> Thanks,
>>> Mani
>>>
>>>> Thanks
>>>> -Amit
>>

Re: [PATCH RFC 1/8] dmaengine: Actions: get rid of bit fields from dma descriptor

[Manivannan Sadhasivam]

On Mon, May 11, 2020 at 01:48:41PM +0100, André Przywara wrote:
> On 11/05/2020 13:04, Manivannan Sadhasivam wrote:
> 
> Hi,
> 
> > On Mon, May 11, 2020 at 12:44:26PM +0100, André Przywara wrote:
> >> On 11/05/2020 12:20, Manivannan Sadhasivam wrote:
> >>
> >> Hi,
> >>
> >>> On Mon, May 11, 2020 at 04:15:57PM +0530, Amit Tomer wrote:
> >>>> Hi
> >>>>
> >>>> Thanks for the reply.
> >>>>
> >>>>> I'm in favor of getting rid of bitfields due to its not so defined way of
> >>>>> working (and forgive me for using it in first place) but I don't quite like
> >>>>> the current approach.
> >>>>
> >>>> Because , its less readable the way we are writing to those different fields ?
> >>>> But this can be made more verbose by adding some comments around .
> >>>>
> >>>
> >>> I don't like the way the hw linked lists are accessed (using an array with
> >>> enums).
> >>
> >> But honestly this is the most sane way of doing this, see below.
> >>
> >>>>> Rather I'd like to have custom bitmasks (S900/S700/S500?) for writing to those
> >>>>> fields.
> >>>>>
> >>>> I think S900 and S500 are same as pointed out by Cristian. and I didn't get by
> >>>> creating custom bitmasks for it ?
> >>>>
> >>>> Did you mean function like:
> >>>>
> >>>> lli->hw[OWL_DMADESC_FLEN]= llc_hw_FLEN(len, FCNT_VALUE, FCNT_SHIFT);
> >>>>
> >>>
> >>> I meant to keep using old struct for accessing the linked list and replacing
> >>> bitfields with masks as below:
> >>>
> >>> struct owl_dma_lli_hw {
> >>> 	...
> >>>         u32     flen;
> >>>         u32     fcnt;
> >>> 	...
> >>> };
> >>
> >> And is think this is the wrong way of modelling hardware defined
> >> register fields. C structs have no guarantee of not introducing padding
> >> in between fields, the only guarantee you get is that the first member
> >> has no padding *before* it:
> >> C standard, section 6.7.2.1, end of paragraph 15:
> >> "There may be unnamed padding within a structure object, but not at its
> >> beginning."
> >>
> >> Arrays in C on the contrary have very much this guarantee: The members
> >> are next to each other, no padding.
> >>
> >> I see that structs are sometimes used in this function, but it's much
> >> less common in the kernel than in other projects (U-Boot comes to mind).
> >> It typically works, because common compiler *implementations* provide
> >> this guarantee, but we should not rely on this.
> >>
> >> So:
> >> Using enums for the keys provides a natural way of increasing indices,
> >> without gaps. Also you get this nice and automatic size value by making
> >> this the last member of the enum.
> >> Arrays provide the guarantee of consecutive allocation.
> >>
> > 
> > I agree with your concerns of using struct for defining registers. But we can
> > safely live with the existing implementation since all fields are u32 and if
> 
> But why, actually? I can understand that this is done in existing code,
> because this was done in the past and apparently never challenged. And
> since it seems to work, at least, there is probably not much reason to
> change it, just for the sake of it.
> But if we need to rework this anyway, we should do the right thing. This
> is especially true in the Linux kernel, which is highly critical and
> privileged code and also aims to be very portable. We should take no
> chances here.
> 

I gave it a spin and I think it makes sense to stick to arrays. I do talk to
a compiler guy internally and he recommended to not trust compilers to do the
right thing for non standard behaviour like this.

> Honestly I don't understand the advantage of using a struct here,
> especially if you need to play some tricks (__packed__) to make it work.
> So why is:
> 	hw->flen
> so much better than
> 	hw[DMA_FLEN]

To be honest this looks ugly to me and that's why I was reluctant. But lets not
worry about it :)

> that it justifies to introduce dodgy code?
> 
> In think in general we should be much more careful when using C language
> constructs to access hardware or hardware defined data structures, and
> be it to not give people the wrong idea about this.
> I think with the advance of more optimising compilers (and, somewhat
> related, more out-of-order CPUs) the chance of breakage becomes much
> higher here.
> 

Only way it can go wrong is, if a nasty compiler adds padding eventhough the
struct is homogeneous. And yeah, let's be on the safe side.

Sorry for stretching this so long!

Thanks,
Mani

> Cheers,
> Andre.
> 
> > needed we can also add '__packed' flag to it to avoid padding for any cases.
> > 
> > The reason why I prefer to stick to this is, this is a hardware linked list and
> > by defining it as an array and accessing the fields using enums looks awful to
> > me. Other than that there is no real justification to shy away.
> > 
> > When you are modelling a plain register bank (which we are also doing in this
> > driver), I'd prefer to use the defines directly.
> > 
> >> We can surely have a look at the masking problem, but this would need to
> >> be runtime determined masks, which tend to become "wordy". There can be
> >> simplifications, for instance I couldn't find where the frame length is
> >> really limited for the S900 (it must be less than 1MB). Since the S700
> >> supports *more* than that, there is no need to limit this differently.
> > 
> > I was just giving an example of how to handle the bitmasks for different
> > SoCs if needed. So yeah if it can be avoided, feel free to drop it.
> > 
> > Thanks,
> > Mani
> > 
> >>
> >> Cheers,
> >> Andre.
> >>
> >>
> >>>
> >>> hw->flen = len & OWL_S900_DMA_FLEN_MASK;
> >>> hw->fcnt = 1 & OWL_S900_DMA_FCNT_MASK;
> >>>
> >>> Then you can use different masks for S700/S900 based on the compatible.
> >>>
> >>> Thanks,
> >>> Mani
> >>>
> >>>> Thanks
> >>>> -Amit
> >>
> 

[PATCH RFC 2/8] dmaengine: Actions: Add support for S700 DMA engine

[Amit Singh Tomar]

DMA controller present on S700 SoC is compatible with the one on S900
(as most of registers are same), but it has different DMA descriptor
structure where registers "fcnt" and "ctrlb" uses different encoding.

For instance, on S900 "fcnt" starts at offset 0x0c and uses upper 12
bits whereas on S700, it starts at offset 0x1c and uses lower 12 bits.

This commit adds support for DMA controller present on S700.

Signed-off-by: Amit Singh Tomar <amittomer25@gmail.com>
---
 drivers/dma/owl-dma.c | 99 ++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 70 insertions(+), 29 deletions(-)

diff --git a/drivers/dma/owl-dma.c b/drivers/dma/owl-dma.c
index b0d80a2fa383..6c2f0d0aad4c 100644
--- a/drivers/dma/owl-dma.c
+++ b/drivers/dma/owl-dma.c
@@ -134,6 +134,11 @@ enum owl_dmadesc_offsets {
 	OWL_DMADESC_SIZE
 };
 
+enum owl_dma_id {
+	S900_DMA,
+	S700_DMA,
+};
+
 /**
  * struct owl_dma_lli - Link list for dma transfer
  * @hw: hardware link list
@@ -200,6 +205,7 @@ struct owl_dma_vchan {
  * @pchans: array of data for the physical channels
  * @nr_vchans: the number of physical channels
  * @vchans: array of data for the physical channels
+ * @devid: device id based on OWL SoC
  */
 struct owl_dma {
 	struct dma_device	dma;
@@ -214,6 +220,7 @@ struct owl_dma {
 
 	unsigned int		nr_vchans;
 	struct owl_dma_vchan	*vchans;
+	enum owl_dma_id		devid;
 };
 
 static void pchan_update(struct owl_dma_pchan *pchan, u32 reg,
@@ -354,6 +361,7 @@ static inline int owl_dma_cfg_lli(struct owl_dma_vchan *vchan,
 				  struct dma_slave_config *sconfig,
 				  bool is_cyclic)
 {
+	struct owl_dma *od = to_owl_dma(vchan->vc.chan.device);
 	u32 mode, ctrlb;
 
 	mode = OWL_DMA_MODE_PW(0);
@@ -409,8 +417,14 @@ static inline int owl_dma_cfg_lli(struct owl_dma_vchan *vchan,
 	lli->hw[OWL_DMADESC_DADDR] = dst;
 	lli->hw[OWL_DMADESC_SRC_STRIDE] = 0;
 	lli->hw[OWL_DMADESC_DST_STRIDE] = 0;
-	lli->hw[OWL_DMADESC_FLEN] = len | 1 << 20;
-	lli->hw[OWL_DMADESC_CTRLB] = ctrlb;
+
+	if (od->devid == S700_DMA) {
+		lli->hw[OWL_DMADESC_FLEN] = len;
+		lli->hw[OWL_DMADESC_CTRLB] = 1 | ctrlb;
+	} else {
+		lli->hw[OWL_DMADESC_FLEN] = len | 1 << 20;
+		lli->hw[OWL_DMADESC_CTRLB] = ctrlb;
+	}
 
 	return 0;
 }
@@ -562,26 +576,35 @@ static irqreturn_t owl_dma_interrupt(int irq, void *dev_id)
 	dma_writel(od, OWL_DMA_IRQ_PD0, pending);
 
 	/* Check missed pending IRQ */
-	for (i = 0; i < od->nr_pchans; i++) {
-		pchan = &od->pchans[i];
-		chan_irq_pending = pchan_readl(pchan, OWL_DMAX_INT_CTL) &
-				   pchan_readl(pchan, OWL_DMAX_INT_STATUS);
-
-		/* Dummy read to ensure OWL_DMA_IRQ_PD0 value is updated */
-		dma_readl(od, OWL_DMA_IRQ_PD0);
+	if (od->devid == S900_DMA) {
+		for (i = 0; i < od->nr_pchans; i++) {
+			pchan = &od->pchans[i];
+			chan_irq_pending = pchan_readl(pchan,
+						       OWL_DMAX_INT_CTL) &
+					   pchan_readl(pchan,
+						       OWL_DMAX_INT_STATUS)
+							;
+
+			/* Dummy read to ensure OWL_DMA_IRQ_PD0 value is
+			 * updated
+			 */
+			dma_readl(od, OWL_DMA_IRQ_PD0);
 
-		global_irq_pending = dma_readl(od, OWL_DMA_IRQ_PD0);
+			global_irq_pending = dma_readl(od,
+						       OWL_DMA_IRQ_PD0);
 
-		if (chan_irq_pending && !(global_irq_pending & BIT(i)))	{
-			dev_dbg(od->dma.dev,
-				"global and channel IRQ pending match err\n");
+			if (chan_irq_pending && !(global_irq_pending &
+						  BIT(i))) {
+				dev_dbg(od->dma.dev,
+			"global and channel IRQ pending match err\n");
 
-			/* Clear IRQ status for this pchan */
-			pchan_update(pchan, OWL_DMAX_INT_STATUS,
-				     0xff, false);
+				/* Clear IRQ status for this pchan */
+				pchan_update(pchan, OWL_DMAX_INT_STATUS,
+					     0xff, false);
 
-			/* Update global IRQ pending */
-			pending |= BIT(i);
+				/* Update global IRQ pending */
+				pending |= BIT(i);
+			}
 		}
 	}
 
@@ -720,6 +743,7 @@ static int owl_dma_resume(struct dma_chan *chan)
 
 static u32 owl_dma_getbytes_chan(struct owl_dma_vchan *vchan)
 {
+	struct owl_dma *od = to_owl_dma(vchan->vc.chan.device);
 	struct owl_dma_pchan *pchan;
 	struct owl_dma_txd *txd;
 	struct owl_dma_lli *lli;
@@ -741,9 +765,15 @@ static u32 owl_dma_getbytes_chan(struct owl_dma_vchan *vchan)
 		list_for_each_entry(lli, &txd->lli_list, node) {
 			/* Start from the next active node */
 			if (lli->phys == next_lli_phy) {
-				list_for_each_entry(lli, &txd->lli_list, node)
-					bytes += lli->hw[OWL_DMADESC_FLEN] &
-						 GENMASK(19, 0);
+				list_for_each_entry(lli, &txd->lli_list, node) {
+					if (od->devid == S700_DMA)
+						bytes +=
+						lli->hw[OWL_DMADESC_FLEN];
+					else
+						bytes +=
+						lli->hw[OWL_DMADESC_FLEN] &
+						GENMASK(19, 0);
+				}
 				break;
 			}
 		}
@@ -756,6 +786,7 @@ static enum dma_status owl_dma_tx_status(struct dma_chan *chan,
 					 dma_cookie_t cookie,
 					 struct dma_tx_state *state)
 {
+	struct owl_dma *od = to_owl_dma(chan->device);
 	struct owl_dma_vchan *vchan = to_owl_vchan(chan);
 	struct owl_dma_lli *lli;
 	struct virt_dma_desc *vd;
@@ -773,8 +804,13 @@ static enum dma_status owl_dma_tx_status(struct dma_chan *chan,
 	vd = vchan_find_desc(&vchan->vc, cookie);
 	if (vd) {
 		txd = to_owl_txd(&vd->tx);
-		list_for_each_entry(lli, &txd->lli_list, node)
-			bytes += lli->hw[OWL_DMADESC_FLEN] & GENMASK(19, 0);
+		list_for_each_entry(lli, &txd->lli_list, node) {
+			if (od->devid == S700_DMA)
+				bytes += lli->hw[OWL_DMADESC_FLEN];
+			else
+				bytes += lli->hw[OWL_DMADESC_FLEN] &
+					 GENMASK(19, 0);
+		}
 	} else {
 		bytes = owl_dma_getbytes_chan(vchan);
 	}
@@ -1031,11 +1067,20 @@ static struct dma_chan *owl_dma_of_xlate(struct of_phandle_args *dma_spec,
 	return chan;
 }
 
+static const struct of_device_id owl_dma_match[] = {
+	{ .compatible = "actions,s900-dma", .data = (void *)S900_DMA,},
+	{ .compatible = "actions,s700-dma", .data = (void *)S700_DMA,},
+	{ /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, owl_dma_match);
+
 static int owl_dma_probe(struct platform_device *pdev)
 {
 	struct device_node *np = pdev->dev.of_node;
 	struct owl_dma *od;
 	int ret, i, nr_channels, nr_requests;
+	const struct of_device_id *of_id =
+				of_match_device(owl_dma_match, &pdev->dev);
 
 	od = devm_kzalloc(&pdev->dev, sizeof(*od), GFP_KERNEL);
 	if (!od)
@@ -1060,6 +1105,8 @@ static int owl_dma_probe(struct platform_device *pdev)
 	dev_info(&pdev->dev, "dma-channels %d, dma-requests %d\n",
 		 nr_channels, nr_requests);
 
+	od->devid = (enum owl_dma_id)of_id->data;
+
 	od->nr_pchans = nr_channels;
 	od->nr_vchans = nr_requests;
 
@@ -1192,12 +1239,6 @@ static int owl_dma_remove(struct platform_device *pdev)
 	return 0;
 }
 
-static const struct of_device_id owl_dma_match[] = {
-	{ .compatible = "actions,s900-dma", },
-	{ /* sentinel */ }
-};
-MODULE_DEVICE_TABLE(of, owl_dma_match);
-
 static struct platform_driver owl_dma_driver = {
 	.probe	= owl_dma_probe,
 	.remove	= owl_dma_remove,
-- 
2.7.4

Re: [PATCH RFC 2/8] dmaengine: Actions: Add support for S700 DMA engine

[André Przywara]

On 06/05/2020 11:36, Amit Singh Tomar wrote:

Hi,

> DMA controller present on S700 SoC is compatible with the one on S900
> (as most of registers are same), but it has different DMA descriptor
> structure where registers "fcnt" and "ctrlb" uses different encoding.
> 
> For instance, on S900 "fcnt" starts at offset 0x0c and uses upper 12
> bits whereas on S700, it starts at offset 0x1c and uses lower 12 bits.
> 
> This commit adds support for DMA controller present on S700.
> 
> Signed-off-by: Amit Singh Tomar <amittomer25@gmail.com>
> ---
>  drivers/dma/owl-dma.c | 99 ++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 70 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/dma/owl-dma.c b/drivers/dma/owl-dma.c
> index b0d80a2fa383..6c2f0d0aad4c 100644
> --- a/drivers/dma/owl-dma.c
> +++ b/drivers/dma/owl-dma.c
> @@ -134,6 +134,11 @@ enum owl_dmadesc_offsets {
>  	OWL_DMADESC_SIZE
>  };
>  
> +enum owl_dma_id {
> +	S900_DMA,
> +	S700_DMA,
> +};
> +
>  /**
>   * struct owl_dma_lli - Link list for dma transfer
>   * @hw: hardware link list
> @@ -200,6 +205,7 @@ struct owl_dma_vchan {
>   * @pchans: array of data for the physical channels
>   * @nr_vchans: the number of physical channels
>   * @vchans: array of data for the physical channels
> + * @devid: device id based on OWL SoC
>   */
>  struct owl_dma {
>  	struct dma_device	dma;
> @@ -214,6 +220,7 @@ struct owl_dma {
>  
>  	unsigned int		nr_vchans;
>  	struct owl_dma_vchan	*vchans;
> +	enum owl_dma_id		devid;
>  };
>  
>  static void pchan_update(struct owl_dma_pchan *pchan, u32 reg,
> @@ -354,6 +361,7 @@ static inline int owl_dma_cfg_lli(struct owl_dma_vchan *vchan,
>  				  struct dma_slave_config *sconfig,
>  				  bool is_cyclic)
>  {
> +	struct owl_dma *od = to_owl_dma(vchan->vc.chan.device);
>  	u32 mode, ctrlb;
>  
>  	mode = OWL_DMA_MODE_PW(0);
> @@ -409,8 +417,14 @@ static inline int owl_dma_cfg_lli(struct owl_dma_vchan *vchan,
>  	lli->hw[OWL_DMADESC_DADDR] = dst;
>  	lli->hw[OWL_DMADESC_SRC_STRIDE] = 0;
>  	lli->hw[OWL_DMADESC_DST_STRIDE] = 0;
> -	lli->hw[OWL_DMADESC_FLEN] = len | 1 << 20;
> -	lli->hw[OWL_DMADESC_CTRLB] = ctrlb;
> +
> +	if (od->devid == S700_DMA) {
> +		lli->hw[OWL_DMADESC_FLEN] = len;
> +		lli->hw[OWL_DMADESC_CTRLB] = 1 | ctrlb;
> +	} else {
> +		lli->hw[OWL_DMADESC_FLEN] = len | 1 << 20;
> +		lli->hw[OWL_DMADESC_CTRLB] = ctrlb;
> +	}
>  
>  	return 0;
>  }
> @@ -562,26 +576,35 @@ static irqreturn_t owl_dma_interrupt(int irq, void *dev_id)
>  	dma_writel(od, OWL_DMA_IRQ_PD0, pending);
>  
>  	/* Check missed pending IRQ */
> -	for (i = 0; i < od->nr_pchans; i++) {
> -		pchan = &od->pchans[i];
> -		chan_irq_pending = pchan_readl(pchan, OWL_DMAX_INT_CTL) &
> -				   pchan_readl(pchan, OWL_DMAX_INT_STATUS);
> -
> -		/* Dummy read to ensure OWL_DMA_IRQ_PD0 value is updated */
> -		dma_readl(od, OWL_DMA_IRQ_PD0);
> +	if (od->devid == S900_DMA) {

You should mention (at least in the commit message) why this is needed.
And please move this into a separate function, this indentation is
becoming mad here.

> +		for (i = 0; i < od->nr_pchans; i++) {
> +			pchan = &od->pchans[i];
> +			chan_irq_pending = pchan_readl(pchan,
> +						       OWL_DMAX_INT_CTL) &
> +					   pchan_readl(pchan,
> +						       OWL_DMAX_INT_STATUS)
> +							;
> +
> +			/* Dummy read to ensure OWL_DMA_IRQ_PD0 value is
> +			 * updated
> +			 */
> +			dma_readl(od, OWL_DMA_IRQ_PD0);
>  
> -		global_irq_pending = dma_readl(od, OWL_DMA_IRQ_PD0);
> +			global_irq_pending = dma_readl(od,
> +						       OWL_DMA_IRQ_PD0);
>  
> -		if (chan_irq_pending && !(global_irq_pending & BIT(i)))	{
> -			dev_dbg(od->dma.dev,
> -				"global and channel IRQ pending match err\n");
> +			if (chan_irq_pending && !(global_irq_pending &
> +						  BIT(i))) {
> +				dev_dbg(od->dma.dev,
> +			"global and channel IRQ pending match err\n");
>  
> -			/* Clear IRQ status for this pchan */
> -			pchan_update(pchan, OWL_DMAX_INT_STATUS,
> -				     0xff, false);
> +				/* Clear IRQ status for this pchan */
> +				pchan_update(pchan, OWL_DMAX_INT_STATUS,
> +					     0xff, false);
>  
> -			/* Update global IRQ pending */
> -			pending |= BIT(i);
> +				/* Update global IRQ pending */
> +				pending |= BIT(i);
> +			}
>  		}
>  	}
>  
> @@ -720,6 +743,7 @@ static int owl_dma_resume(struct dma_chan *chan)
>  
>  static u32 owl_dma_getbytes_chan(struct owl_dma_vchan *vchan)
>  {
> +	struct owl_dma *od = to_owl_dma(vchan->vc.chan.device);
>  	struct owl_dma_pchan *pchan;
>  	struct owl_dma_txd *txd;
>  	struct owl_dma_lli *lli;
> @@ -741,9 +765,15 @@ static u32 owl_dma_getbytes_chan(struct owl_dma_vchan *vchan)
>  		list_for_each_entry(lli, &txd->lli_list, node) {
>  			/* Start from the next active node */
>  			if (lli->phys == next_lli_phy) {
> -				list_for_each_entry(lli, &txd->lli_list, node)
> -					bytes += lli->hw[OWL_DMADESC_FLEN] &
> -						 GENMASK(19, 0);
> +				list_for_each_entry(lli, &txd->lli_list, node) {
> +					if (od->devid == S700_DMA)
> +						bytes +=
> +						lli->hw[OWL_DMADESC_FLEN];
> +					else
> +						bytes +=
> +						lli->hw[OWL_DMADESC_FLEN] &
> +						GENMASK(19, 0);

You should have an accessor for getting the frame len, that should avoid
the insane wrapping here. Or factor this out into a helper function.
Alternatively revert the if statement and continue, that saves you one
level of indentation.

I guess flen is limited to 20 bits anyway, so you might want to apply
the 20-bit mask unconditionally.

Cheers,
Andre

> +				}
>  				break;
>  			}
>  		}
> @@ -756,6 +786,7 @@ static enum dma_status owl_dma_tx_status(struct dma_chan *chan,
>  					 dma_cookie_t cookie,
>  					 struct dma_tx_state *state)
>  {
> +	struct owl_dma *od = to_owl_dma(chan->device);
>  	struct owl_dma_vchan *vchan = to_owl_vchan(chan);
>  	struct owl_dma_lli *lli;
>  	struct virt_dma_desc *vd;
> @@ -773,8 +804,13 @@ static enum dma_status owl_dma_tx_status(struct dma_chan *chan,
>  	vd = vchan_find_desc(&vchan->vc, cookie);
>  	if (vd) {
>  		txd = to_owl_txd(&vd->tx);
> -		list_for_each_entry(lli, &txd->lli_list, node)
> -			bytes += lli->hw[OWL_DMADESC_FLEN] & GENMASK(19, 0);
> +		list_for_each_entry(lli, &txd->lli_list, node) {
> +			if (od->devid == S700_DMA)
> +				bytes += lli->hw[OWL_DMADESC_FLEN];
> +			else
> +				bytes += lli->hw[OWL_DMADESC_FLEN] &
> +					 GENMASK(19, 0);
> +		}
>  	} else {
>  		bytes = owl_dma_getbytes_chan(vchan);
>  	}
> @@ -1031,11 +1067,20 @@ static struct dma_chan *owl_dma_of_xlate(struct of_phandle_args *dma_spec,
>  	return chan;
>  }
>  
> +static const struct of_device_id owl_dma_match[] = {
> +	{ .compatible = "actions,s900-dma", .data = (void *)S900_DMA,},
> +	{ .compatible = "actions,s700-dma", .data = (void *)S700_DMA,},
> +	{ /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, owl_dma_match);
> +
>  static int owl_dma_probe(struct platform_device *pdev)
>  {
>  	struct device_node *np = pdev->dev.of_node;
>  	struct owl_dma *od;
>  	int ret, i, nr_channels, nr_requests;
> +	const struct of_device_id *of_id =
> +				of_match_device(owl_dma_match, &pdev->dev);
>  
>  	od = devm_kzalloc(&pdev->dev, sizeof(*od), GFP_KERNEL);
>  	if (!od)
> @@ -1060,6 +1105,8 @@ static int owl_dma_probe(struct platform_device *pdev)
>  	dev_info(&pdev->dev, "dma-channels %d, dma-requests %d\n",
>  		 nr_channels, nr_requests);
>  
> +	od->devid = (enum owl_dma_id)of_id->data;
> +
>  	od->nr_pchans = nr_channels;
>  	od->nr_vchans = nr_requests;
>  
> @@ -1192,12 +1239,6 @@ static int owl_dma_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> -static const struct of_device_id owl_dma_match[] = {
> -	{ .compatible = "actions,s900-dma", },
> -	{ /* sentinel */ }
> -};
> -MODULE_DEVICE_TABLE(of, owl_dma_match);
> -
>  static struct platform_driver owl_dma_driver = {
>  	.probe	= owl_dma_probe,
>  	.remove	= owl_dma_remove,
> 

Re: [PATCH RFC 2/8] dmaengine: Actions: Add support for S700 DMA engine

[Amit Tomer]

Hi,

Thanks for quick review

> You should mention (at least in the commit message) why this is needed.
> And please move this into a separate function, this indentation is
> becoming mad here

There is not much documented about it, and all I see is GIC crash
if I keep it open for S700. Would figure out more details about it and
update in next version.
.
>
> > +             for (i = 0; i < od->nr_pchans; i++) {
> > +                     pchan = &od->pchans[i];
> > +                     chan_irq_pending = pchan_readl(pchan,
> > +                                                    OWL_DMAX_INT_CTL) &
> > +                                        pchan_readl(pchan,
> > +                                                    OWL_DMAX_INT_STATUS)
> > +                                                     ;
> > +
> > +                     /* Dummy read to ensure OWL_DMA_IRQ_PD0 value is
> > +                      * updated
> > +                      */
> > +                     dma_readl(od, OWL_DMA_IRQ_PD0);
> >
> > -             global_irq_pending = dma_readl(od, OWL_DMA_IRQ_PD0);
> > +                     global_irq_pending = dma_readl(od,
> > +                                                    OWL_DMA_IRQ_PD0);
> >
> > -             if (chan_irq_pending && !(global_irq_pending & BIT(i))) {
> > -                     dev_dbg(od->dma.dev,
> > -                             "global and channel IRQ pending match err\n");
> > +                     if (chan_irq_pending && !(global_irq_pending &
> > +                                               BIT(i))) {
> > +                             dev_dbg(od->dma.dev,
> > +                     "global and channel IRQ pending match err\n");
> >
> > -                     /* Clear IRQ status for this pchan */
> > -                     pchan_update(pchan, OWL_DMAX_INT_STATUS,
> > -                                  0xff, false);
> > +                             /* Clear IRQ status for this pchan */
> > +                             pchan_update(pchan, OWL_DMAX_INT_STATUS,
> > +                                          0xff, false);
> >
> > -                     /* Update global IRQ pending */
> > -                     pending |= BIT(i);
> > +                             /* Update global IRQ pending */
> > +                             pending |= BIT(i);
> > +                     }
> >               }
> >       }
> >
> > @@ -720,6 +743,7 @@ static int owl_dma_resume(struct dma_chan *chan)
> >
> >  static u32 owl_dma_getbytes_chan(struct owl_dma_vchan *vchan)
> >  {
> > +     struct owl_dma *od = to_owl_dma(vchan->vc.chan.device);
> >       struct owl_dma_pchan *pchan;
> >       struct owl_dma_txd *txd;
> >       struct owl_dma_lli *lli;
> > @@ -741,9 +765,15 @@ static u32 owl_dma_getbytes_chan(struct owl_dma_vchan *vchan)
> >               list_for_each_entry(lli, &txd->lli_list, node) {
> >                       /* Start from the next active node */
> >                       if (lli->phys == next_lli_phy) {
> > -                             list_for_each_entry(lli, &txd->lli_list, node)
> > -                                     bytes += lli->hw[OWL_DMADESC_FLEN] &
> > -                                              GENMASK(19, 0);
> > +                             list_for_each_entry(lli, &txd->lli_list, node) {
> > +                                     if (od->devid == S700_DMA)
> > +                                             bytes +=
> > +                                             lli->hw[OWL_DMADESC_FLEN];
> > +                                     else
> > +                                             bytes +=
> > +                                             lli->hw[OWL_DMADESC_FLEN] &
> > +                                             GENMASK(19, 0);
>
> You should have an accessor for getting the frame len, that should avoid
> the insane wrapping here. Or factor this out into a helper function.
> Alternatively revert the if statement and continue, that saves you one
> level of indentation.
>
> I guess flen is limited to 20 bits anyway, so you might want to apply
> the 20-bit mask unconditionally.

Actually, on S700 flen uses 24 bits , so we should not use 20-bit mask.

For accessor function, shall this be okay ?

+static u32 llc_hw_flen(struct owl_dma *od,
+                       struct owl_dma_lli *lli)
+{
+       u32 bit_mask;
+
+       if (od->devid == S700_DMA)
+               bit_mask = 23;
+       else
+               bit_mask = 19;
+
+       return lli->hw[OWL_DMADESC_FLEN] & GENMASK(bit_mask, 0);
+
+}

Thanks
Amit

Re: [PATCH RFC 2/8] dmaengine: Actions: Add support for S700 DMA engine

[André Przywara]

On 06/05/2020 13:54, Amit Tomer wrote:
> Hi,
> 
> Thanks for quick review
> 
>> You should mention (at least in the commit message) why this is needed.
>> And please move this into a separate function, this indentation is
>> becoming mad here
> 
> There is not much documented about it, and all I see is GIC crash
> if I keep it open for S700. Would figure out more details about it and
> update in next version.

What I meant is that you should mention this and the fact that you got
this bit from the BSP source.

> .
>>
>>> +             for (i = 0; i < od->nr_pchans; i++) {
>>> +                     pchan = &od->pchans[i];
>>> +                     chan_irq_pending = pchan_readl(pchan,
>>> +                                                    OWL_DMAX_INT_CTL) &
>>> +                                        pchan_readl(pchan,
>>> +                                                    OWL_DMAX_INT_STATUS)
>>> +                                                     ;
>>> +
>>> +                     /* Dummy read to ensure OWL_DMA_IRQ_PD0 value is
>>> +                      * updated
>>> +                      */
>>> +                     dma_readl(od, OWL_DMA_IRQ_PD0);
>>>
>>> -             global_irq_pending = dma_readl(od, OWL_DMA_IRQ_PD0);
>>> +                     global_irq_pending = dma_readl(od,
>>> +                                                    OWL_DMA_IRQ_PD0);
>>>
>>> -             if (chan_irq_pending && !(global_irq_pending & BIT(i))) {
>>> -                     dev_dbg(od->dma.dev,
>>> -                             "global and channel IRQ pending match err\n");
>>> +                     if (chan_irq_pending && !(global_irq_pending &
>>> +                                               BIT(i))) {
>>> +                             dev_dbg(od->dma.dev,
>>> +                     "global and channel IRQ pending match err\n");
>>>
>>> -                     /* Clear IRQ status for this pchan */
>>> -                     pchan_update(pchan, OWL_DMAX_INT_STATUS,
>>> -                                  0xff, false);
>>> +                             /* Clear IRQ status for this pchan */
>>> +                             pchan_update(pchan, OWL_DMAX_INT_STATUS,
>>> +                                          0xff, false);
>>>
>>> -                     /* Update global IRQ pending */
>>> -                     pending |= BIT(i);
>>> +                             /* Update global IRQ pending */
>>> +                             pending |= BIT(i);
>>> +                     }
>>>               }
>>>       }
>>>
>>> @@ -720,6 +743,7 @@ static int owl_dma_resume(struct dma_chan *chan)
>>>
>>>  static u32 owl_dma_getbytes_chan(struct owl_dma_vchan *vchan)
>>>  {
>>> +     struct owl_dma *od = to_owl_dma(vchan->vc.chan.device);
>>>       struct owl_dma_pchan *pchan;
>>>       struct owl_dma_txd *txd;
>>>       struct owl_dma_lli *lli;
>>> @@ -741,9 +765,15 @@ static u32 owl_dma_getbytes_chan(struct owl_dma_vchan *vchan)
>>>               list_for_each_entry(lli, &txd->lli_list, node) {
>>>                       /* Start from the next active node */
>>>                       if (lli->phys == next_lli_phy) {
>>> -                             list_for_each_entry(lli, &txd->lli_list, node)
>>> -                                     bytes += lli->hw[OWL_DMADESC_FLEN] &
>>> -                                              GENMASK(19, 0);
>>> +                             list_for_each_entry(lli, &txd->lli_list, node) {
>>> +                                     if (od->devid == S700_DMA)
>>> +                                             bytes +=
>>> +                                             lli->hw[OWL_DMADESC_FLEN];
>>> +                                     else
>>> +                                             bytes +=
>>> +                                             lli->hw[OWL_DMADESC_FLEN] &
>>> +                                             GENMASK(19, 0);
>>
>> You should have an accessor for getting the frame len, that should avoid
>> the insane wrapping here. Or factor this out into a helper function.
>> Alternatively revert the if statement and continue, that saves you one
>> level of indentation.
>>
>> I guess flen is limited to 20 bits anyway, so you might want to apply
>> the 20-bit mask unconditionally.
> 
> Actually, on S700 flen uses 24 bits , so we should not use 20-bit mask.

I think it *can* use 24 bits. Where does flen come from? I guess it is
less than 1 MB  anyway already? It better should be, at least, since the
S900 seems to have this limit.

> For accessor function, shall this be okay ?

Something like that. My point was that this can be much more simplified
if you go with 20 bits *always*. Then you can save the first parameter
and this becomes a one-liner.

Cheers,
Andre

> +static u32 llc_hw_flen(struct owl_dma *od,
> +                       struct owl_dma_lli *lli)
> +{
> +       u32 bit_mask;
> +
> +       if (od->devid == S700_DMA)
> +               bit_mask = 23;
> +       else
> +               bit_mask = 19;
> +
> +       return lli->hw[OWL_DMADESC_FLEN] & GENMASK(bit_mask, 0);
> +
> +}
> 
> Thanks
> Amit
>