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

["André Przywara" <andre.przywara@arm.com>]

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