| Message ID | 1588761371-9078-2-git-send-email-amittomer25@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add MMC support for S700 | expand |
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 >
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
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
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
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 >
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 >>
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 > >> >
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); }
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(-)