[PATCH RFC 0/8] Add MMC support for S700

[PATCH RFC 0/8] Add MMC support for S700

[Amit Singh Tomar]

The intention of this series is to enable uSD support for Cubieboard7
based on Actions S700 SoC, and on the way we found that it requires
changes in dmaengine present on S700 as its different from what is
present on S900.

Patch(1/8) does provide a new way to describe DMA descriptor, idea is
to remove the bit-fields as its less maintainable. It is only build
tested and it would be great if this can be tested on S900 based
hardware.

Patch(2/8) adds S700 DMA engine support, there is new compatible
string added for it, which means a changed bindings needed to submitted
for this. I would plan to send it later the converted "owl-dma.yaml".

Patch(4/8) disables the sps node as its memory range is conflicting
pinctrl node and results in pinctrl proble failure.

Rest of patches in the series adds DMA/MMC nodes for S700
alone with binding constants and enables the uSD for Cubieboard7.

This whole series is tested, by building/compiling Kernel on
Cubieboard7-lite which was *almost* successful (OOM kicked in,
while Linking due to less RAM present on hardware).

Following is the mmc speed :

ubuntu@ubuntu:~$ sudo hdparm -tT /dev/mmcblk0

/dev/mmcblk0:
 Timing cached reads:   1310 MB in  2.00 seconds = 655.15 MB/sec
 Timing buffered disk reads:  62 MB in  3.05 seconds =  20.30 MB/sec

Amit Singh Tomar (8):
  dmaengine: Actions: get rid of bit fields from dma descriptor
  dmaengine: Actions: Add support for S700 DMA engine
  clk: actions: Add MMC clock-register reset bits
  arm64: dts: actions: disable sps node from S700
  arm64: dts: actions: Add DMA Controller for S700
  dt-bindings: reset: s700: Add binding constants for mmc
  arm64: dts: actions: Add MMC controller support for S700
  arm64: dts: actions: Add uSD support for Cubieboard7

 arch/arm64/boot/dts/actions/s700-cubieboard7.dts |  41 ++++++
 arch/arm64/boot/dts/actions/s700.dtsi            |  48 +++++++
 drivers/clk/actions/owl-s700.c                   |   3 +
 drivers/dma/owl-dma.c                            | 166 +++++++++++++----------
 include/dt-bindings/reset/actions,s700-reset.h   |   3 +
 5 files changed, 193 insertions(+), 68 deletions(-)

-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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
>>


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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
> >>
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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,
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH RFC 3/8] clk: actions: Add MMC clock-register reset bits

[Amit Singh Tomar]

This commit adds reset bits needed for MMC clock registers present
on Actions S700 SoC.

Signed-off-by: Amit Singh Tomar <amittomer25@gmail.com>
---
 drivers/clk/actions/owl-s700.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/clk/actions/owl-s700.c b/drivers/clk/actions/owl-s700.c
index a2f34d13fb54..cd60eca7727d 100644
--- a/drivers/clk/actions/owl-s700.c
+++ b/drivers/clk/actions/owl-s700.c
@@ -577,6 +577,9 @@ static const struct owl_reset_map s700_resets[] = {
 	[RESET_DSI]	= { CMU_DEVRST0, BIT(2) },
 	[RESET_CSI]	= { CMU_DEVRST0, BIT(13) },
 	[RESET_SI]	= { CMU_DEVRST0, BIT(14) },
+	[RESET_SD0]     = { CMU_DEVRST0, BIT(22) },
+	[RESET_SD1]     = { CMU_DEVRST0, BIT(23) },
+	[RESET_SD2]     = { CMU_DEVRST0, BIT(24) },
 	[RESET_I2C0]	= { CMU_DEVRST1, BIT(0) },
 	[RESET_I2C1]	= { CMU_DEVRST1, BIT(1) },
 	[RESET_I2C2]	= { CMU_DEVRST1, BIT(2) },
-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH RFC 4/8] arm64: dts: actions: disable sps node from S700

[Amit Singh Tomar]

After commit 7cdf8446ed1d ("arm64: dts: actions: Add pinctrl node for
Actions Semi S700") following error has been observed while booting
Linux on Cubieboard7-lite(based on S700 SoC).

[    0.257415] pinctrl-s700 e01b0000.pinctrl: can't request region for
resource [mem 0xe01b0000-0xe01b0fff]
[    0.266902] pinctrl-s700: probe of e01b0000.pinctrl failed with error -16

This is due to the fact that memory range for "sps" power domain controller
clashes with pinctrl.

This commit fixes it by disabling "sps" node, it is safe as "sps" is not
being used at the moment.

Fixes: 7cdf8446ed1d ("arm64: dts: actions: Add pinctrl node for Actions
Semi S700")

Signed-off-by: Amit Singh Tomar <amittomer25@gmail.com>
---
 arch/arm64/boot/dts/actions/s700.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/actions/s700.dtsi b/arch/arm64/boot/dts/actions/s700.dtsi
index 2006ad5424fa..0397c5dd3dec 100644
--- a/arch/arm64/boot/dts/actions/s700.dtsi
+++ b/arch/arm64/boot/dts/actions/s700.dtsi
@@ -220,6 +220,7 @@
 			compatible = "actions,s700-sps";
 			reg = <0x0 0xe01b0100 0x0 0x100>;
 			#power-domain-cells = <1>;
+			status = "disabled";
 		};
 
 		timer: timer@e024c000 {
-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH RFC 4/8] arm64: dts: actions: disable sps node from S700

[André Przywara]

On 06/05/2020 11:36, Amit Singh Tomar wrote:
> After commit 7cdf8446ed1d ("arm64: dts: actions: Add pinctrl node for
> Actions Semi S700") following error has been observed while booting
> Linux on Cubieboard7-lite(based on S700 SoC).
> 
> [    0.257415] pinctrl-s700 e01b0000.pinctrl: can't request region for
> resource [mem 0xe01b0000-0xe01b0fff]
> [    0.266902] pinctrl-s700: probe of e01b0000.pinctrl failed with error -16
> 
> This is due to the fact that memory range for "sps" power domain controller
> clashes with pinctrl.
> 
> This commit fixes it by disabling "sps" node, it is safe as "sps" is not
> being used at the moment.
> 
> Fixes: 7cdf8446ed1d ("arm64: dts: actions: Add pinctrl node for Actions
> Semi S700")

But this is more of a hack than a proper solution, right? Especially
since you actually need the SPS later on (patch 8/8).
It's probably good enough to prove that the DMA and MMC parts are
working, but should not be merged.

Cheers,
Andre.

> 
> Signed-off-by: Amit Singh Tomar <amittomer25@gmail.com>
> ---
>  arch/arm64/boot/dts/actions/s700.dtsi | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm64/boot/dts/actions/s700.dtsi b/arch/arm64/boot/dts/actions/s700.dtsi
> index 2006ad5424fa..0397c5dd3dec 100644
> --- a/arch/arm64/boot/dts/actions/s700.dtsi
> +++ b/arch/arm64/boot/dts/actions/s700.dtsi
> @@ -220,6 +220,7 @@
>  			compatible = "actions,s700-sps";
>  			reg = <0x0 0xe01b0100 0x0 0x100>;
>  			#power-domain-cells = <1>;
> +			status = "disabled";
>  		};
>  
>  		timer: timer@e024c000 {
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH RFC 5/8] arm64: dts: actions: Add DMA Controller for S700

[Amit Singh Tomar]

This commit adds DAM controller present on Actions S700, it differs from
S900 in terms of number of dma channels and requests.

Signed-off-by: Amit Singh Tomar <amittomer25@gmail.com>
---
 arch/arm64/boot/dts/actions/s700.dtsi | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/arch/arm64/boot/dts/actions/s700.dtsi b/arch/arm64/boot/dts/actions/s700.dtsi
index 0397c5dd3dec..56f2f84812cb 100644
--- a/arch/arm64/boot/dts/actions/s700.dtsi
+++ b/arch/arm64/boot/dts/actions/s700.dtsi
@@ -245,5 +245,18 @@
 				     <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>,
 				     <GIC_SPI 40 IRQ_TYPE_LEVEL_HIGH>;
 		};
+
+		dma: dma-controller@e0230000 {
+			compatible = "actions,s700-dma";
+			reg = <0x0 0xe0230000 0x0 0x1000>;
+			interrupts = <GIC_SPI 57 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 58 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 59 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 60 IRQ_TYPE_LEVEL_HIGH>;
+			#dma-cells = <1>;
+			dma-channels = <10>;
+			dma-requests = <44>;
+			clocks = <&cmu CLK_DMAC>;
+		};
 	};
 };
-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH RFC 6/8] dt-bindings: reset: s700: Add binding constants for mmc

[Amit Singh Tomar]

This commit adds device tree binding reset constants for mmc controller
present on Actions S700 Soc.

Signed-off-by: Amit Singh Tomar <amittomer25@gmail.com>
---
 include/dt-bindings/reset/actions,s700-reset.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/dt-bindings/reset/actions,s700-reset.h b/include/dt-bindings/reset/actions,s700-reset.h
index 5e3b16b8ef53..a3118de6d7aa 100644
--- a/include/dt-bindings/reset/actions,s700-reset.h
+++ b/include/dt-bindings/reset/actions,s700-reset.h
@@ -30,5 +30,8 @@
 #define RESET_UART4				20
 #define RESET_UART5				21
 #define RESET_UART6				22
+#define RESET_SD0				23
+#define RESET_SD1				24
+#define RESET_SD2				25
 
 #endif /* __DT_BINDINGS_ACTIONS_S700_RESET_H */
-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH RFC 6/8] dt-bindings: reset: s700: Add binding constants for mmc

[Rob Herring]

On Wed,  6 May 2020 16:06:08 +0530, Amit Singh Tomar wrote:
> This commit adds device tree binding reset constants for mmc controller
> present on Actions S700 Soc.
> 
> Signed-off-by: Amit Singh Tomar <amittomer25@gmail.com>
> ---
>  include/dt-bindings/reset/actions,s700-reset.h | 3 +++
>  1 file changed, 3 insertions(+)
> 

Acked-by: Rob Herring <robh@kernel.org>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH RFC 7/8] arm64: dts: actions: Add MMC controller support for S700

[Amit Singh Tomar]

This commits adds support for MMC controllers present on Actions S700 SoC,
there are 3 MMC controllers in this SoC which can be used for accessing
SD/EMMC/SDIO cards.

Signed-off-by: Amit Singh Tomar <amittomer25@gmail.com>
---
 arch/arm64/boot/dts/actions/s700.dtsi | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/arch/arm64/boot/dts/actions/s700.dtsi b/arch/arm64/boot/dts/actions/s700.dtsi
index 56f2f84812cb..3f1fc3e48415 100644
--- a/arch/arm64/boot/dts/actions/s700.dtsi
+++ b/arch/arm64/boot/dts/actions/s700.dtsi
@@ -258,5 +258,38 @@
 			dma-requests = <44>;
 			clocks = <&cmu CLK_DMAC>;
 		};
+
+		mmc0: mmc@e0210000 {
+			compatible = "actions,owl-mmc";
+			reg = <0x0 0xe0210000 0x0 0x4000>;
+			interrupts = <GIC_SPI 42 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&cmu CLK_SD0>;
+			resets = <&cmu RESET_SD0>;
+			dmas = <&dma 2>;
+			dma-names = "mmc";
+			status = "disabled";
+		};
+
+		mmc1: mmc@e0214000 {
+			compatible = "actions,owl-mmc";
+			reg = <0x0 0xe0214000 0x0 0x4000>;
+			interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&cmu CLK_SD1>;
+			resets = <&cmu RESET_SD1>;
+			dmas = <&dma 3>;
+			dma-names = "mmc";
+			status = "disabled";
+		};
+
+		mmc2: mmc@e0218000 {
+			compatible = "actions,owl-mmc";
+			reg = <0x0 0xe0218000 0x0 0x4000>;
+			interrupts = <GIC_SPI 44 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&cmu CLK_SD2>;
+			resets = <&cmu RESET_SD2>;
+			dmas = <&dma 4>;
+			dma-names = "mmc";
+			status = "disabled";
+		};
 	};
 };
-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH RFC 8/8] arm64: dts: actions: Add uSD support for Cubieboard7

[Amit Singh Tomar]

This commit adds uSD support for Cubieboard7 board based on Actions Semi
S700 SoC. SD0 is connected to uSD slot. Since there is no PMIC support
added yet, fixed regulator has been used as a regulator node.

Signed-off-by: Amit Singh Tomar <amittomer25@gmail.com>
---
 arch/arm64/boot/dts/actions/s700-cubieboard7.dts | 41 ++++++++++++++++++++++++
 arch/arm64/boot/dts/actions/s700.dtsi            |  1 +
 2 files changed, 42 insertions(+)

diff --git a/arch/arm64/boot/dts/actions/s700-cubieboard7.dts b/arch/arm64/boot/dts/actions/s700-cubieboard7.dts
index 63e375cd9eb4..ec117eb12f3a 100644
--- a/arch/arm64/boot/dts/actions/s700-cubieboard7.dts
+++ b/arch/arm64/boot/dts/actions/s700-cubieboard7.dts
@@ -13,6 +13,7 @@
 
 	aliases {
 		serial3 = &uart3;
+		mmc0 = &mmc0;
 	};
 
 	chosen {
@@ -28,6 +29,23 @@
 		device_type = "memory";
 		reg = <0x1 0xe0000000 0x0 0x0>;
 	};
+
+	/* Fixed regulator used in the absence of PMIC */
+	vcc_3v1: vcc-3v1 {
+		compatible = "regulator-fixed";
+		regulator-name = "fixed-3.1V";
+		regulator-min-microvolt = <3100000>;
+		regulator-max-microvolt = <3100000>;
+	};
+
+	/* Fixed regulator used in the absence of PMIC */
+	sd_vcc: sd-vcc {
+		compatible = "regulator-fixed";
+		regulator-name = "fixed-3.1V";
+		regulator-min-microvolt = <3100000>;
+		regulator-max-microvolt = <3100000>;
+		regulator-always-on;
+	};
 };
 
 &i2c0 {
@@ -81,6 +99,14 @@
 			bias-pull-up;
 		};
 	};
+
+	mmc0_default: mmc0_default {
+		pinmux {
+			groups = "sd0_d0_mfp", "sd0_d1_mfp", "sd0_d2_d3_mfp",
+				 "sd0_cmd_mfp", "sd0_clk_mfp";
+			function = "sd0";
+		};
+	};
 };
 
 &timer {
@@ -90,3 +116,18 @@
 &uart3 {
 	status = "okay";
 };
+
+/* uSD */
+&mmc0 {
+	status = "okay";
+	pinctrl-names = "default";
+	pinctrl-0 = <&mmc0_default>;
+	cd-gpios = <&pinctrl 120 GPIO_ACTIVE_LOW>;
+	no-sdio;
+	no-mmc;
+	no-1-8-v;
+	bus-width = <4>;
+	vmmc-supply = <&sd_vcc>;
+	vqmmc-supply = <&sd_vcc>;
+};
+
diff --git a/arch/arm64/boot/dts/actions/s700.dtsi b/arch/arm64/boot/dts/actions/s700.dtsi
index 3f1fc3e48415..8a541dd48f61 100644
--- a/arch/arm64/boot/dts/actions/s700.dtsi
+++ b/arch/arm64/boot/dts/actions/s700.dtsi
@@ -4,6 +4,7 @@
  */
 
 #include <dt-bindings/clock/actions,s700-cmu.h>
+#include <dt-bindings/gpio/gpio.h>
 #include <dt-bindings/interrupt-controller/arm-gic.h>
 #include <dt-bindings/reset/actions,s700-reset.h>
 
-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel