Re: [PATCH 3/4 v4] OMAP: McBSP: Introduce caching in register write operations

2009-12-04 Thread Janusz Krzysztofik 

Janusz Krzysztofik napisał(a):

Thursday 03 December 2009 21:03:02 Tony Lindgren napisał(a):

Hi,

* Janusz Krzysztofik jkrzy...@tis.icnet.pl [091203 04:29]:

Reserve space inside omap_mcbsp structure for storing cached copies of
McBSP register values.
Modify the MCBSP_WRITE() macro to update the cache with every register
write operation.
Introduce a new macro that reads from the cache instead of hardware.

Applies on top of patch 2 from this series:
[PATCH v3 2/4] OMAP: McBSP: Prepare register read/write macros API for
caching

Tested on OMAP1510 based Amstrad Delta using linux-omap for-next,
commit 4421752e331cfb1d942b47ffdb26e451a8da58a0.
Compile-tested with omap_3430sdp_defconfig.

Signed-off-by: Janusz Krzysztofik jkrzy...@tis.icnet.pl

---

Thursday 03 December 2009 08:49:21 Jarkko Nikula wrote:

On Tue, 1 Dec 2009 04:15:50 +0100

Janusz Krzysztofik jkrzy...@tis.icnet.pl wrote:

 #define MCBSP_WRITE(mcbsp, reg, val) \
-   omap_mcbsp_write(mcbsp-io_base, OMAP_MCBSP_REG_##reg, val)
+   omap_mcbsp_write(mcbsp-io_base, OMAP_MCBSP_REG_##reg, \
+   mcbsp-reg_cache[OMAP_MCBSP_REG_##reg / OMAP_MCBSP_REG_DRR1] \
+   = val)
+#define MCBSP_READ_CACHE(mcbsp, reg) \
+   (mcbsp-reg_cache[OMAP_MCBSP_REG_##reg / OMAP_MCBSP_REG_DRR1])

These divisions by DDR1 are confusing. Use rather sizeof:

reg_cache[OMAP_MCBSP_REG_##reg / sizeof(*mcbsp-reg_cache)]

Done.

Jarkko, many thanks for your cooperation :).

Janusz

 arch/arm/plat-omap/include/plat/mcbsp.h |5 +
 arch/arm/plat-omap/mcbsp.c  |7 ++-
 2 files changed, 11 insertions(+), 1 deletion(-)

--- git/arch/arm/plat-omap/include/plat/mcbsp.h.orig2009-11-27
11:53:45.0 +0100 +++
git/arch/arm/plat-omap/include/plat/mcbsp.h 2009-12-01 03:37:21.0
+0100 @@ -415,6 +415,11 @@ struct omap_mcbsp {
u16 max_tx_thres;
u16 max_rx_thres;
 #endif
+#ifdef CONFIG_ARCH_OMAP1
+   u16 reg_cache[OMAP_MCBSP_REG_XCERH / sizeof(u16) + 1];
+#else
+   u32 reg_cache[OMAP_MCBSP_REG_RCCR / sizeof(u32) + 1];
+#endif
 };
 extern struct omap_mcbsp **mcbsp_ptr;
 extern int omap_mcbsp_count;

How about rather set:

static u16 omap1_reg_cache[];

and

static u32 omap2_reg_cache[];


Hi Tony,

Let me understand more precisely what you mean before I submit next version.

Did you mean something like:

static u16 omap1_reg_cache[3][OMAP_MCBSP_REG_XCERH / sizeof(u16) + 1];
static u32 omap2_reg_cache[5][OMAP_MCBSP_REG_RCCR / sizeof(u32) + 1];

both put in arch/arm/plat-omap/mcbsp.c without any ifdefs?


Tony,

I think I've already found one part of the answer myself: you probably 
mean putting each table definition in a corresponding 
arch/arm/mach-omap[12]/mcbsp.c source file, don't you?


What remains unclear for me is if you really intend to reserve static 
cache space for all McBSP ports supported by a machine, even if not 
supported by a particular cpu or not used on a particular board.



--- git/arch/arm/plat-omap/mcbsp.c.orig 2009-12-01 03:19:56.0
+0100 +++ git/arch/arm/plat-omap/mcbsp.c2009-12-03 11:37:21.0
+0100 @@ -49,7 +49,12 @@ int omap_mcbsp_read(void __iomem *io_bas
 #define MCBSP_READ(mcbsp, reg) \
omap_mcbsp_read(mcbsp-io_base, OMAP_MCBSP_REG_##reg)
 #define MCBSP_WRITE(mcbsp, reg, val) \
-   omap_mcbsp_write(mcbsp-io_base, OMAP_MCBSP_REG_##reg, val)
+   omap_mcbsp_write(mcbsp-io_base, OMAP_MCBSP_REG_##reg, \
+   mcbsp-reg_cache[OMAP_MCBSP_REG_##reg / \
+sizeof(*mcbsp-reg_cache)] = val)
+#define MCBSP_READ_CACHE(mcbsp, reg) \
+   (mcbsp-reg_cache[OMAP_MCBSP_REG_##reg / \
+ sizeof(*mcbsp-reg_cache)])

 #define omap_mcbsp_check_valid_id(id)  (id  omap_mcbsp_count)
 #define id_to_mcbsp_ptr(id)mcbsp_ptr[id];

Then rather than doing this cache manipulation in the macro, just
do it in the omap_mcbsp_read/write functions based on the
cpu_class_is_omap1() accessing omap1_reg_cache or omap2_reg_cache.


omap_mcbsp_wrtie() case seems clear to me, but:

Did you mean defining a new omap_mcbsp_read_cache() function, similiar to 
omap_mcbsp_read(), but reading from cache instead of hardware?


Or do you prefere if I modify omap_mcbsp_read() to be able to read from both 
hardware and cache, dependig on an additional argument, for example?


Thanks,
Janusz


This way you can get rid of the ifdef else.


But shouldn't declarations for both tables be placed in 
arch/arm/plat-omap/include/plat/mcbsp.h withing an ifdef else block? If 
yes, one ifdef else block would be replaced with another. Or did you 
mean placing those declarations inside existing ifdef block with defines 
for register offsets?


Thanks,
Janusz




Regards,

Tony

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at

Re: [PATCH 3/4 v4] OMAP: McBSP: Introduce caching in register write operations

2009-12-04 Thread Tony Lindgren 
* Janusz Krzysztofik jkrzy...@tis.icnet.pl [091204 04:56]:
 Janusz Krzysztofik napisał(a):

snip
 
 I think I've already found one part of the answer myself: you
 probably mean putting each table definition in a corresponding
 arch/arm/mach-omap[12]/mcbsp.c source file, don't you?

Well I did not initially, but sounds like that's the cleanest
way to go.
 
 What remains unclear for me is if you really intend to reserve
 static cache space for all McBSP ports supported by a machine, even
 if not supported by a particular cpu or not used on a particular
 board.

No idea :) But please consider what happens if we want to compile in
omap2 + omap3 into a single binary. Or what happens if some newer omaps
add more McBSP registers.

snip
 
 
 Did you mean defining a new omap_mcbsp_read_cache() function,
 similiar to omap_mcbsp_read(), but reading from cache instead of
 hardware?

Yeah something like that. That is more future proof.

 Or do you prefere if I modify omap_mcbsp_read() to be able to read
 from both hardware and cache, dependig on an additional argument,
 for example?

That actually sounds like the best way to solve it to me!

 But shouldn't declarations for both tables be placed in
 arch/arm/plat-omap/include/plat/mcbsp.h withing an ifdef else block?
 If yes, one ifdef else block would be replaced with another. Or did
 you mean placing those declarations inside existing ifdef block with
 defines for register offsets?

Ifdefs are OK if they just optimize out code when some omap is not
compiled in. But if you do ifdef else, then it means trouble sooner
or later. In this case you should probably have the cache initialized
separately in mach-omap1/mcbsp.c and mach-omap2/mcbsp.c.

Regards,

Tony
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/4 v4] OMAP: McBSP: Introduce caching in register write operations

2009-12-03 Thread Tony Lindgren 
Hi,

* Janusz Krzysztofik jkrzy...@tis.icnet.pl [091203 04:29]:
 Reserve space inside omap_mcbsp structure for storing cached copies of McBSP
 register values.
 Modify the MCBSP_WRITE() macro to update the cache with every register write
 operation.
 Introduce a new macro that reads from the cache instead of hardware.
 
 Applies on top of patch 2 from this series:
 [PATCH v3 2/4] OMAP: McBSP: Prepare register read/write macros API for caching
 
 Tested on OMAP1510 based Amstrad Delta using linux-omap for-next,
 commit 4421752e331cfb1d942b47ffdb26e451a8da58a0.
 Compile-tested with omap_3430sdp_defconfig.
 
 Signed-off-by: Janusz Krzysztofik jkrzy...@tis.icnet.pl
 
 ---
 Thursday 03 December 2009 08:49:21 Jarkko Nikula wrote:
  On Tue, 1 Dec 2009 04:15:50 +0100
  Janusz Krzysztofik jkrzy...@tis.icnet.pl wrote:
#define MCBSP_WRITE(mcbsp, reg, val) \
   - omap_mcbsp_write(mcbsp-io_base, OMAP_MCBSP_REG_##reg, val)
   + omap_mcbsp_write(mcbsp-io_base, OMAP_MCBSP_REG_##reg, \
   + mcbsp-reg_cache[OMAP_MCBSP_REG_##reg / OMAP_MCBSP_REG_DRR1] \
   + = val)
   +#define MCBSP_READ_CACHE(mcbsp, reg) \
   + (mcbsp-reg_cache[OMAP_MCBSP_REG_##reg / OMAP_MCBSP_REG_DRR1])
 
  These divisions by DDR1 are confusing. Use rather sizeof:
 
  reg_cache[OMAP_MCBSP_REG_##reg / sizeof(*mcbsp-reg_cache)]
 
 Done.
 
 Jarkko, many thanks for your cooperation :).
 
 Janusz
 
  arch/arm/plat-omap/include/plat/mcbsp.h |5 +
  arch/arm/plat-omap/mcbsp.c  |7 ++-
  2 files changed, 11 insertions(+), 1 deletion(-)
 
 --- git/arch/arm/plat-omap/include/plat/mcbsp.h.orig  2009-11-27 
 11:53:45.0 +0100
 +++ git/arch/arm/plat-omap/include/plat/mcbsp.h   2009-12-01 
 03:37:21.0 +0100
 @@ -415,6 +415,11 @@ struct omap_mcbsp {
   u16 max_tx_thres;
   u16 max_rx_thres;
  #endif
 +#ifdef CONFIG_ARCH_OMAP1
 + u16 reg_cache[OMAP_MCBSP_REG_XCERH / sizeof(u16) + 1];
 +#else
 + u32 reg_cache[OMAP_MCBSP_REG_RCCR / sizeof(u32) + 1];
 +#endif
  };
  extern struct omap_mcbsp **mcbsp_ptr;
  extern int omap_mcbsp_count;

How about rather set:

static u16 omap1_reg_cache[];

and

static u32 omap2_reg_cache[];


 --- git/arch/arm/plat-omap/mcbsp.c.orig   2009-12-01 03:19:56.0 
 +0100
 +++ git/arch/arm/plat-omap/mcbsp.c2009-12-03 11:37:21.0 +0100
 @@ -49,7 +49,12 @@ int omap_mcbsp_read(void __iomem *io_bas
  #define MCBSP_READ(mcbsp, reg) \
   omap_mcbsp_read(mcbsp-io_base, OMAP_MCBSP_REG_##reg)
  #define MCBSP_WRITE(mcbsp, reg, val) \
 - omap_mcbsp_write(mcbsp-io_base, OMAP_MCBSP_REG_##reg, val)
 + omap_mcbsp_write(mcbsp-io_base, OMAP_MCBSP_REG_##reg, \
 + mcbsp-reg_cache[OMAP_MCBSP_REG_##reg / \
 +  sizeof(*mcbsp-reg_cache)] = val)
 +#define MCBSP_READ_CACHE(mcbsp, reg) \
 + (mcbsp-reg_cache[OMAP_MCBSP_REG_##reg / \
 +   sizeof(*mcbsp-reg_cache)])
  
  #define omap_mcbsp_check_valid_id(id)(id  omap_mcbsp_count)
  #define id_to_mcbsp_ptr(id)  mcbsp_ptr[id];

Then rather than doing this cache manipulation in the macro, just
do it in the omap_mcbsp_read/write functions based on the
cpu_class_is_omap1() accessing omap1_reg_cache or omap2_reg_cache.
This way you can get rid of the ifdef else.

Regards,

Tony

--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/4 v4] OMAP: McBSP: Introduce caching in register write operations

2009-12-03 Thread Janusz Krzysztofik 
Thursday 03 December 2009 21:03:02 Tony Lindgren napisał(a):
 Hi,

 * Janusz Krzysztofik jkrzy...@tis.icnet.pl [091203 04:29]:
  Reserve space inside omap_mcbsp structure for storing cached copies of
  McBSP register values.
  Modify the MCBSP_WRITE() macro to update the cache with every register
  write operation.
  Introduce a new macro that reads from the cache instead of hardware.
 
  Applies on top of patch 2 from this series:
  [PATCH v3 2/4] OMAP: McBSP: Prepare register read/write macros API for
  caching
 
  Tested on OMAP1510 based Amstrad Delta using linux-omap for-next,
  commit 4421752e331cfb1d942b47ffdb26e451a8da58a0.
  Compile-tested with omap_3430sdp_defconfig.
 
  Signed-off-by: Janusz Krzysztofik jkrzy...@tis.icnet.pl
 
  ---
 
  Thursday 03 December 2009 08:49:21 Jarkko Nikula wrote:
   On Tue, 1 Dec 2009 04:15:50 +0100
  
   Janusz Krzysztofik jkrzy...@tis.icnet.pl wrote:
 #define MCBSP_WRITE(mcbsp, reg, val) \
-   omap_mcbsp_write(mcbsp-io_base, OMAP_MCBSP_REG_##reg, 
val)
+   omap_mcbsp_write(mcbsp-io_base, OMAP_MCBSP_REG_##reg, \
+   mcbsp-reg_cache[OMAP_MCBSP_REG_##reg / 
OMAP_MCBSP_REG_DRR1] \
+   = val)
+#define MCBSP_READ_CACHE(mcbsp, reg) \
+   (mcbsp-reg_cache[OMAP_MCBSP_REG_##reg / 
OMAP_MCBSP_REG_DRR1])
  
   These divisions by DDR1 are confusing. Use rather sizeof:
  
   reg_cache[OMAP_MCBSP_REG_##reg / sizeof(*mcbsp-reg_cache)]
 
  Done.
 
  Jarkko, many thanks for your cooperation :).
 
  Janusz
 
   arch/arm/plat-omap/include/plat/mcbsp.h |5 +
   arch/arm/plat-omap/mcbsp.c  |7 ++-
   2 files changed, 11 insertions(+), 1 deletion(-)
 
  --- git/arch/arm/plat-omap/include/plat/mcbsp.h.orig2009-11-27
  11:53:45.0 +0100 +++
  git/arch/arm/plat-omap/include/plat/mcbsp.h 2009-12-01 03:37:21.0
  +0100 @@ -415,6 +415,11 @@ struct omap_mcbsp {
  u16 max_tx_thres;
  u16 max_rx_thres;
   #endif
  +#ifdef CONFIG_ARCH_OMAP1
  +   u16 reg_cache[OMAP_MCBSP_REG_XCERH / sizeof(u16) + 1];
  +#else
  +   u32 reg_cache[OMAP_MCBSP_REG_RCCR / sizeof(u32) + 1];
  +#endif
   };
   extern struct omap_mcbsp **mcbsp_ptr;
   extern int omap_mcbsp_count;

 How about rather set:

 static u16 omap1_reg_cache[];

 and

 static u32 omap2_reg_cache[];

Hi Tony,

Let me understand more precisely what you mean before I submit next version.

Did you mean something like:

static u16 omap1_reg_cache[3][OMAP_MCBSP_REG_XCERH / sizeof(u16) + 1];
static u32 omap2_reg_cache[5][OMAP_MCBSP_REG_RCCR / sizeof(u32) + 1];

both put in arch/arm/plat-omap/mcbsp.c without any ifdefs?

  --- git/arch/arm/plat-omap/mcbsp.c.orig 2009-12-01 03:19:56.0
  +0100 +++ git/arch/arm/plat-omap/mcbsp.c2009-12-03 11:37:21.0
  +0100 @@ -49,7 +49,12 @@ int omap_mcbsp_read(void __iomem *io_bas
   #define MCBSP_READ(mcbsp, reg) \
  omap_mcbsp_read(mcbsp-io_base, OMAP_MCBSP_REG_##reg)
   #define MCBSP_WRITE(mcbsp, reg, val) \
  -   omap_mcbsp_write(mcbsp-io_base, OMAP_MCBSP_REG_##reg, val)
  +   omap_mcbsp_write(mcbsp-io_base, OMAP_MCBSP_REG_##reg, \
  +   mcbsp-reg_cache[OMAP_MCBSP_REG_##reg / \
  +sizeof(*mcbsp-reg_cache)] = val)
  +#define MCBSP_READ_CACHE(mcbsp, reg) \
  +   (mcbsp-reg_cache[OMAP_MCBSP_REG_##reg / \
  + sizeof(*mcbsp-reg_cache)])
 
   #define omap_mcbsp_check_valid_id(id)  (id  omap_mcbsp_count)
   #define id_to_mcbsp_ptr(id)mcbsp_ptr[id];

 Then rather than doing this cache manipulation in the macro, just
 do it in the omap_mcbsp_read/write functions based on the
 cpu_class_is_omap1() accessing omap1_reg_cache or omap2_reg_cache.

omap_mcbsp_wrtie() case seems clear to me, but:

Did you mean defining a new omap_mcbsp_read_cache() function, similiar to 
omap_mcbsp_read(), but reading from cache instead of hardware?

Or do you prefere if I modify omap_mcbsp_read() to be able to read from both 
hardware and cache, dependig on an additional argument, for example?

Thanks,
Janusz

 This way you can get rid of the ifdef else.

 Regards,

 Tony

 --
 To unsubscribe from this list: send the line unsubscribe linux-omap in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html


--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html