Re: [PATCH v4 2/8] MIPS: Octeon: Enable LMTDMA/LMTST operations.

[James Hogan]

On Thu, Nov 30, 2017 at 03:09:33PM -0800, David Daney wrote:
> On 11/30/2017 02:56 PM, James Hogan wrote:
> > On Thu, Nov 30, 2017 at 01:49:43PM -0800, David Daney wrote:
> >> On 11/30/2017 01:36 PM, James Hogan wrote:
> >>> On Tue, Nov 28, 2017 at 04:55:34PM -0800, David Daney wrote:
>  Signed-off-by: Carlos Munoz 
>  Signed-off-by: Steven J. Hill 
>  Signed-off-by: David Daney 
>  ---
> arch/mips/cavium-octeon/setup.c   |  6 ++
> arch/mips/include/asm/octeon/octeon.h | 12 ++--
> 2 files changed, 16 insertions(+), 2 deletions(-)
> 
>  diff --git a/arch/mips/cavium-octeon/setup.c 
>  b/arch/mips/cavium-octeon/setup.c
>  index a8034d0dcade..99e6a68bc652 100644
>  --- a/arch/mips/cavium-octeon/setup.c
>  +++ b/arch/mips/cavium-octeon/setup.c
>  @@ -609,6 +609,12 @@ void octeon_user_io_init(void)
> #else
>   cvmmemctl.s.cvmsegenak = 0;
> #endif
>  +if (OCTEON_IS_OCTEON3()) {
>  +/* Enable LMTDMA */
>  +cvmmemctl.s.lmtena = 1;
>  +/* Scratch line to use for LMT operation */
>  +cvmmemctl.s.lmtline = 2;
> >>>
> >>> Out of curiosity, is there significance to the value 2 and associated
> >>> virtual address 0x8100, or is it pretty arbitrary?
> >>
> >> Yes, there is significance.
> >>
> >> CPU local memory starts at 0x8000, each line is 0x80 bytes.
> >> so the 2nd line starts at 0x8100
> > 
> > What I mean is, why is 2 chosen instead of any other value?
> 
> That is explained in the change log of patch 5/8:
> 
> 
>  1st 128-bytes: Use by IOBDMA
>  2nd 128-bytes: Reserved by kernel for scratch/TLS emulation.
>  3rd 128-bytes: OCTEON-III LMTLINE

Ah yes. Perhaps it deserves a brief comment in the code, or even an
enum.

Cheers
James


signature.asc
Description: Digital signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 2/8] MIPS: Octeon: Enable LMTDMA/LMTST operations.

[David Daney]

On 11/30/2017 02:56 PM, James Hogan wrote:

On Thu, Nov 30, 2017 at 01:49:43PM -0800, David Daney wrote:

On 11/30/2017 01:36 PM, James Hogan wrote:

On Tue, Nov 28, 2017 at 04:55:34PM -0800, David Daney wrote:

Signed-off-by: Carlos Munoz 
Signed-off-by: Steven J. Hill 
Signed-off-by: David Daney 
---
   arch/mips/cavium-octeon/setup.c   |  6 ++
   arch/mips/include/asm/octeon/octeon.h | 12 ++--
   2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/arch/mips/cavium-octeon/setup.c b/arch/mips/cavium-octeon/setup.c
index a8034d0dcade..99e6a68bc652 100644
--- a/arch/mips/cavium-octeon/setup.c
+++ b/arch/mips/cavium-octeon/setup.c
@@ -609,6 +609,12 @@ void octeon_user_io_init(void)
   #else
cvmmemctl.s.cvmsegenak = 0;
   #endif
+   if (OCTEON_IS_OCTEON3()) {
+   /* Enable LMTDMA */
+   cvmmemctl.s.lmtena = 1;
+   /* Scratch line to use for LMT operation */
+   cvmmemctl.s.lmtline = 2;


Out of curiosity, is there significance to the value 2 and associated
virtual address 0x8100, or is it pretty arbitrary?


Yes, there is significance.

CPU local memory starts at 0x8000, each line is 0x80 bytes.
so the 2nd line starts at 0x8100


What I mean is, why is 2 chosen instead of any other value?


That is explained in the change log of patch 5/8:


1st 128-bytes: Use by IOBDMA
2nd 128-bytes: Reserved by kernel for scratch/TLS emulation.
3rd 128-bytes: OCTEON-III LMTLINE



Cheers
James



___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 2/8] MIPS: Octeon: Enable LMTDMA/LMTST operations.

[James Hogan]

On Thu, Nov 30, 2017 at 01:49:43PM -0800, David Daney wrote:
> On 11/30/2017 01:36 PM, James Hogan wrote:
> > On Tue, Nov 28, 2017 at 04:55:34PM -0800, David Daney wrote:
> >> Signed-off-by: Carlos Munoz 
> >> Signed-off-by: Steven J. Hill 
> >> Signed-off-by: David Daney 
> >> ---
> >>   arch/mips/cavium-octeon/setup.c   |  6 ++
> >>   arch/mips/include/asm/octeon/octeon.h | 12 ++--
> >>   2 files changed, 16 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/arch/mips/cavium-octeon/setup.c 
> >> b/arch/mips/cavium-octeon/setup.c
> >> index a8034d0dcade..99e6a68bc652 100644
> >> --- a/arch/mips/cavium-octeon/setup.c
> >> +++ b/arch/mips/cavium-octeon/setup.c
> >> @@ -609,6 +609,12 @@ void octeon_user_io_init(void)
> >>   #else
> >>cvmmemctl.s.cvmsegenak = 0;
> >>   #endif
> >> +  if (OCTEON_IS_OCTEON3()) {
> >> +  /* Enable LMTDMA */
> >> +  cvmmemctl.s.lmtena = 1;
> >> +  /* Scratch line to use for LMT operation */
> >> +  cvmmemctl.s.lmtline = 2;
> > 
> > Out of curiosity, is there significance to the value 2 and associated
> > virtual address 0x8100, or is it pretty arbitrary?
> 
> Yes, there is significance.
> 
> CPU local memory starts at 0x8000, each line is 0x80 bytes. 
> so the 2nd line starts at 0x8100

What I mean is, why is 2 chosen instead of any other value?

Cheers
James


signature.asc
Description: Digital signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 2/8] MIPS: Octeon: Enable LMTDMA/LMTST operations.

[David Daney]

On 11/30/2017 01:36 PM, James Hogan wrote:

On Tue, Nov 28, 2017 at 04:55:34PM -0800, David Daney wrote:

From: Carlos Munoz 

LMTDMA/LMTST operations move data between cores and I/O devices:

* LMTST operations can send an address and a variable length
   (up to 128 bytes) of data to an I/O device.
* LMTDMA operations can send an address and a variable length
   (up to 128) of data to the I/O device and then return a
   variable length (up to 128 bytes) response from the IOI device.


Should that be "I/O"?


Yes, I will fix the changelog.






Signed-off-by: Carlos Munoz 
Signed-off-by: Steven J. Hill 
Signed-off-by: David Daney 
---
  arch/mips/cavium-octeon/setup.c   |  6 ++
  arch/mips/include/asm/octeon/octeon.h | 12 ++--
  2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/arch/mips/cavium-octeon/setup.c b/arch/mips/cavium-octeon/setup.c
index a8034d0dcade..99e6a68bc652 100644
--- a/arch/mips/cavium-octeon/setup.c
+++ b/arch/mips/cavium-octeon/setup.c
@@ -609,6 +609,12 @@ void octeon_user_io_init(void)
  #else
cvmmemctl.s.cvmsegenak = 0;
  #endif
+   if (OCTEON_IS_OCTEON3()) {
+   /* Enable LMTDMA */
+   cvmmemctl.s.lmtena = 1;
+   /* Scratch line to use for LMT operation */
+   cvmmemctl.s.lmtline = 2;


Out of curiosity, is there significance to the value 2 and associated
virtual address 0x8100, or is it pretty arbitrary?


Yes, there is significance.

CPU local memory starts at 0x8000, each line is 0x80 bytes. 
so the 2nd line starts at 0x8100






+   }
/* R/W If set, CVMSEG is available for loads/stores in
 * supervisor mode. */
cvmmemctl.s.cvmsegenas = 0;
diff --git a/arch/mips/include/asm/octeon/octeon.h 
b/arch/mips/include/asm/octeon/octeon.h
index c99c4b6a79f4..92a17d67c1fa 100644
--- a/arch/mips/include/asm/octeon/octeon.h
+++ b/arch/mips/include/asm/octeon/octeon.h
@@ -179,7 +179,15 @@ union octeon_cvmemctl {
/* RO 1 = BIST fail, 0 = BIST pass */
__BITFIELD_FIELD(uint64_t wbfbist:1,
/* Reserved */
-   __BITFIELD_FIELD(uint64_t reserved:17,
+   __BITFIELD_FIELD(uint64_t reserved_52_57:6,
+   /* When set, LMTDMA/LMTST operations are permitted */
+   __BITFIELD_FIELD(uint64_t lmtena:1,
+   /* Selects the CVMSEG LM cacheline used by LMTDMA
+* LMTST and wide atomic store operations.
+*/
+   __BITFIELD_FIELD(uint64_t lmtline:6,
+   /* Reserved */
+   __BITFIELD_FIELD(uint64_t reserved_41_44:4,
/* OCTEON II - TLB replacement policy: 0 = bitmask LRU; 1 = NLU.
 * This field selects between the TLB replacement policies:
 * bitmask LRU or NLU. Bitmask LRU maintains a mask of
@@ -275,7 +283,7 @@ union octeon_cvmemctl {
/* R/W Size of local memory in cache blocks, 54 (6912
 * bytes) is max legal value. */
__BITFIELD_FIELD(uint64_t lmemsz:6,
-   ;)
+   ;
} s;
  };


Regardless, the patch looks good to me.

Reviewed-by: James Hogan 

Cheers
James



___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 2/8] MIPS: Octeon: Enable LMTDMA/LMTST operations.

[James Hogan]

On Tue, Nov 28, 2017 at 04:55:34PM -0800, David Daney wrote:
> From: Carlos Munoz 
> 
> LMTDMA/LMTST operations move data between cores and I/O devices:
> 
> * LMTST operations can send an address and a variable length
>   (up to 128 bytes) of data to an I/O device.
> * LMTDMA operations can send an address and a variable length
>   (up to 128) of data to the I/O device and then return a
>   variable length (up to 128 bytes) response from the IOI device.

Should that be "I/O"?

> 
> Signed-off-by: Carlos Munoz 
> Signed-off-by: Steven J. Hill 
> Signed-off-by: David Daney 
> ---
>  arch/mips/cavium-octeon/setup.c   |  6 ++
>  arch/mips/include/asm/octeon/octeon.h | 12 ++--
>  2 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/mips/cavium-octeon/setup.c b/arch/mips/cavium-octeon/setup.c
> index a8034d0dcade..99e6a68bc652 100644
> --- a/arch/mips/cavium-octeon/setup.c
> +++ b/arch/mips/cavium-octeon/setup.c
> @@ -609,6 +609,12 @@ void octeon_user_io_init(void)
>  #else
>   cvmmemctl.s.cvmsegenak = 0;
>  #endif
> + if (OCTEON_IS_OCTEON3()) {
> + /* Enable LMTDMA */
> + cvmmemctl.s.lmtena = 1;
> + /* Scratch line to use for LMT operation */
> + cvmmemctl.s.lmtline = 2;

Out of curiosity, is there significance to the value 2 and associated
virtual address 0x8100, or is it pretty arbitrary?

> + }
>   /* R/W If set, CVMSEG is available for loads/stores in
>* supervisor mode. */
>   cvmmemctl.s.cvmsegenas = 0;
> diff --git a/arch/mips/include/asm/octeon/octeon.h 
> b/arch/mips/include/asm/octeon/octeon.h
> index c99c4b6a79f4..92a17d67c1fa 100644
> --- a/arch/mips/include/asm/octeon/octeon.h
> +++ b/arch/mips/include/asm/octeon/octeon.h
> @@ -179,7 +179,15 @@ union octeon_cvmemctl {
>   /* RO 1 = BIST fail, 0 = BIST pass */
>   __BITFIELD_FIELD(uint64_t wbfbist:1,
>   /* Reserved */
> - __BITFIELD_FIELD(uint64_t reserved:17,
> + __BITFIELD_FIELD(uint64_t reserved_52_57:6,
> + /* When set, LMTDMA/LMTST operations are permitted */
> + __BITFIELD_FIELD(uint64_t lmtena:1,
> + /* Selects the CVMSEG LM cacheline used by LMTDMA
> +  * LMTST and wide atomic store operations.
> +  */
> + __BITFIELD_FIELD(uint64_t lmtline:6,
> + /* Reserved */
> + __BITFIELD_FIELD(uint64_t reserved_41_44:4,
>   /* OCTEON II - TLB replacement policy: 0 = bitmask LRU; 1 = NLU.
>* This field selects between the TLB replacement policies:
>* bitmask LRU or NLU. Bitmask LRU maintains a mask of
> @@ -275,7 +283,7 @@ union octeon_cvmemctl {
>   /* R/W Size of local memory in cache blocks, 54 (6912
>* bytes) is max legal value. */
>   __BITFIELD_FIELD(uint64_t lmemsz:6,
> - ;)
> + ;
>   } s;
>  };

Regardless, the patch looks good to me.

Reviewed-by: James Hogan 

Cheers
James


signature.asc
Description: Digital signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel