* [PATCH RFC 1/8] dmaengine: Actions: get rid of bit fields from dma descriptor [not found] <1588761371-9078-1-git-send-email-amittomer25@gmail.com> @ 2020-05-06 10:36 ` Amit Singh Tomar 2020-05-10 15:51 ` Manivannan Sadhasivam 2020-05-06 10:36 ` [PATCH RFC 2/8] dmaengine: Actions: Add support for S700 DMA engine Amit Singh Tomar 1 sibling, 1 reply; 12+ messages in thread From: Amit Singh Tomar @ 2020-05-06 10:36 UTC (permalink / raw) To: andre.przywara, vkoul, afaerber, manivannan.sadhasivam Cc: dan.j.williams, cristian.ciocaltea, dmaengine, linux-arm-kernel, linux-actions 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH RFC 1/8] dmaengine: Actions: get rid of bit fields from dma descriptor 2020-05-06 10:36 ` [PATCH RFC 1/8] dmaengine: Actions: get rid of bit fields from dma descriptor Amit Singh Tomar @ 2020-05-10 15:51 ` Manivannan Sadhasivam 2020-05-11 10:45 ` Amit Tomer 0 siblings, 1 reply; 12+ messages in thread From: Manivannan Sadhasivam @ 2020-05-10 15:51 UTC (permalink / raw) To: Amit Singh Tomar Cc: andre.przywara, vkoul, afaerber, dan.j.williams, cristian.ciocaltea, dmaengine, linux-arm-kernel, linux-actions 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 > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH RFC 1/8] dmaengine: Actions: get rid of bit fields from dma descriptor 2020-05-10 15:51 ` Manivannan Sadhasivam @ 2020-05-11 10:45 ` Amit Tomer 2020-05-11 11:20 ` Manivannan Sadhasivam 0 siblings, 1 reply; 12+ messages in thread From: Amit Tomer @ 2020-05-11 10:45 UTC (permalink / raw) To: Manivannan Sadhasivam Cc: Andre Przywara, vkoul, Andreas Färber, dan.j.williams, cristian.ciocaltea, dmaengine, linux-arm-kernel, linux-actions 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH RFC 1/8] dmaengine: Actions: get rid of bit fields from dma descriptor 2020-05-11 10:45 ` Amit Tomer @ 2020-05-11 11:20 ` Manivannan Sadhasivam 2020-05-11 11:44 ` André Przywara 0 siblings, 1 reply; 12+ messages in thread From: Manivannan Sadhasivam @ 2020-05-11 11:20 UTC (permalink / raw) To: Amit Tomer Cc: Andre Przywara, vkoul, Andreas Färber, dan.j.williams, cristian.ciocaltea, dmaengine, linux-arm-kernel, linux-actions 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH RFC 1/8] dmaengine: Actions: get rid of bit fields from dma descriptor 2020-05-11 11:20 ` Manivannan Sadhasivam @ 2020-05-11 11:44 ` André Przywara 2020-05-11 12:04 ` Manivannan Sadhasivam 0 siblings, 1 reply; 12+ messages in thread From: André Przywara @ 2020-05-11 11:44 UTC (permalink / raw) To: Manivannan Sadhasivam, Amit Tomer Cc: vkoul, Andreas Färber, dan.j.williams, cristian.ciocaltea, dmaengine, linux-arm-kernel, linux-actions 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH RFC 1/8] dmaengine: Actions: get rid of bit fields from dma descriptor 2020-05-11 11:44 ` André Przywara @ 2020-05-11 12:04 ` Manivannan Sadhasivam 2020-05-11 12:48 ` André Przywara 0 siblings, 1 reply; 12+ messages in thread From: Manivannan Sadhasivam @ 2020-05-11 12:04 UTC (permalink / raw) To: André Przywara Cc: Amit Tomer, vkoul, Andreas Färber, dan.j.williams, cristian.ciocaltea, dmaengine, linux-arm-kernel, linux-actions 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 > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH RFC 1/8] dmaengine: Actions: get rid of bit fields from dma descriptor 2020-05-11 12:04 ` Manivannan Sadhasivam @ 2020-05-11 12:48 ` André Przywara 2020-05-11 15:29 ` Manivannan Sadhasivam 0 siblings, 1 reply; 12+ messages in thread From: André Przywara @ 2020-05-11 12:48 UTC (permalink / raw) To: Manivannan Sadhasivam Cc: Amit Tomer, vkoul, Andreas Färber, dan.j.williams, cristian.ciocaltea, dmaengine, linux-arm-kernel, linux-actions 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 >> ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH RFC 1/8] dmaengine: Actions: get rid of bit fields from dma descriptor 2020-05-11 12:48 ` André Przywara @ 2020-05-11 15:29 ` Manivannan Sadhasivam 0 siblings, 0 replies; 12+ messages in thread From: Manivannan Sadhasivam @ 2020-05-11 15:29 UTC (permalink / raw) To: André Przywara Cc: Amit Tomer, vkoul, Andreas Färber, dan.j.williams, cristian.ciocaltea, dmaengine, linux-arm-kernel, linux-actions 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 > >> > ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH RFC 2/8] dmaengine: Actions: Add support for S700 DMA engine [not found] <1588761371-9078-1-git-send-email-amittomer25@gmail.com> 2020-05-06 10:36 ` [PATCH RFC 1/8] dmaengine: Actions: get rid of bit fields from dma descriptor Amit Singh Tomar @ 2020-05-06 10:36 ` Amit Singh Tomar 2020-05-06 11:12 ` André Przywara 1 sibling, 1 reply; 12+ messages in thread From: Amit Singh Tomar @ 2020-05-06 10:36 UTC (permalink / raw) To: andre.przywara, vkoul, afaerber, manivannan.sadhasivam Cc: dan.j.williams, cristian.ciocaltea, dmaengine, linux-arm-kernel, linux-actions 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH RFC 2/8] dmaengine: Actions: Add support for S700 DMA engine 2020-05-06 10:36 ` [PATCH RFC 2/8] dmaengine: Actions: Add support for S700 DMA engine Amit Singh Tomar @ 2020-05-06 11:12 ` André Przywara 2020-05-06 12:54 ` Amit Tomer 0 siblings, 1 reply; 12+ messages in thread From: André Przywara @ 2020-05-06 11:12 UTC (permalink / raw) To: Amit Singh Tomar, vkoul, afaerber, manivannan.sadhasivam Cc: dan.j.williams, cristian.ciocaltea, dmaengine, linux-arm-kernel, linux-actions 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, > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH RFC 2/8] dmaengine: Actions: Add support for S700 DMA engine 2020-05-06 11:12 ` André Przywara @ 2020-05-06 12:54 ` Amit Tomer 2020-05-06 13:04 ` André Przywara 0 siblings, 1 reply; 12+ messages in thread From: Amit Tomer @ 2020-05-06 12:54 UTC (permalink / raw) To: André Przywara Cc: vkoul, Andreas Färber, Manivannan Sadhasivam, dan.j.williams, cristian.ciocaltea, dmaengine, linux-arm-kernel, linux-actions 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH RFC 2/8] dmaengine: Actions: Add support for S700 DMA engine 2020-05-06 12:54 ` Amit Tomer @ 2020-05-06 13:04 ` André Przywara 0 siblings, 0 replies; 12+ messages in thread From: André Przywara @ 2020-05-06 13:04 UTC (permalink / raw) To: Amit Tomer Cc: vkoul, Andreas Färber, Manivannan Sadhasivam, dan.j.williams, cristian.ciocaltea, dmaengine, linux-arm-kernel, linux-actions 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 > ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-05-11 15:29 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <1588761371-9078-1-git-send-email-amittomer25@gmail.com> 2020-05-06 10:36 ` [PATCH RFC 1/8] dmaengine: Actions: get rid of bit fields from dma descriptor Amit Singh Tomar 2020-05-10 15:51 ` Manivannan Sadhasivam 2020-05-11 10:45 ` Amit Tomer 2020-05-11 11:20 ` Manivannan Sadhasivam 2020-05-11 11:44 ` André Przywara 2020-05-11 12:04 ` Manivannan Sadhasivam 2020-05-11 12:48 ` André Przywara 2020-05-11 15:29 ` Manivannan Sadhasivam 2020-05-06 10:36 ` [PATCH RFC 2/8] dmaengine: Actions: Add support for S700 DMA engine Amit Singh Tomar 2020-05-06 11:12 ` André Przywara 2020-05-06 12:54 ` Amit Tomer 2020-05-06 13:04 ` André Przywara
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).