Hi, Almost there :) Just one more comment below.
* Janusz Krzysztofik <[email protected]> [091208 08:06]: > Allocate space for storing cached copies of McBSP register values. > Modify omap_msbcp_write() to update the cache with every register write > operation. > Modify omap_mcbsp_read() to support reading from cache or hardware. > Update MCBSP_READ/MCBSP_WRITE macros for modified function APIs. > Introduce a new macro that reads from the cache. > > 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 82f1d8f22f2c65e70206e40a6f17688bf64a892c. > Compile-tested with omap_generic_2420_defconfig, omap_3430sdp_defconfig. > > Signed-off-by: Janusz Krzysztofik <[email protected]> > > --- > Monday 07 December 2009 22:06:31 Tony Lindgren wrote: > > > > Well if you want to optimize it out further, how about just kzalloc it > > during init? Then you have just one copy that gets set the right size > > depending on what you boot. > > Tony, > In v4 of the patch, a single table was actually kzalloced during init as a > part of struct omap_mcbsp for each McBSP port configured. Since you objected > it, and the reason was not quite obvious to me, I submitted two alternate > versions: 5a, with ifdef...else cleanup, maybe still not good enough but > keeping that dynamic allocation, and 5b, using static storage instead, since I > could suspect you may like it better. Now it appears not quite true. > > Anyway, sorry for all that long discussion, but it's really important for me > to understand precisely what your concearns, goals and preferences are when > you request changes before I'm able to implement them correctly, ie. in an way > acceptable for you. Otherwise, it all turns into a kind of quiz. Yeah, thanks for considering all the issues, hopefully it will make things easier for us in the long run. > Tuesday 08 December 2009 08:35:21 Jarkko Nikula wrote: > > > > How about declaring the reg_cache as a void pointer in struct > > omap_mcbsp and allocate the cache and its size in omap_mcbsp_request > > according to omap type? Read and write functions would access the > > *reg_cache either as 16-bit or 32-bit table depending on omap. > > > > No ifdefs, no unused cache tables in multi-omap binary and cache tables > > are allocated only for used ports. I don't think there is need to > > preserve cache content over omap_mcbsp_free and new omap_mcbsp_request? > > Jarkko, > I think this is the best solution. I had almost already sent a modified > version last night, with two pointers, u16 *omap1_cache and u32 *omap2_cache, > inside omap_mcbsp structre, but now I think that if using casts inside > omap_mcbsp_read()/_write() is not objected by anyone, the version below, > based on your idea, seems the most clean one. > > I was not sure about deferring memory allocation until omap_mcbsp_request(), > but if you say it should be ok, I'm all in favour of that. > > Note: 2420 handling may look weird, but I just tried to follow exactly what I > was able to learn about it by reading the code: register address spacing > were 32-bit, register read/write operations - 16-bit, since u16 reg_cache > elements. > > Thanks, > Janusz > > arch/arm/plat-omap/include/plat/mcbsp.h | 9 ++++++ > arch/arm/plat-omap/mcbsp.c | 66 > +++++++++++++++++++++++++++++++++++++++++---------- > 2 files changed, 63 insertions(+), 12 deletions(-) > > diff -upr git.orig/arch/arm/plat-omap/include/plat/mcbsp.h > git/arch/arm/plat-omap/include/plat/mcbsp.h > --- git.orig/arch/arm/plat-omap/include/plat/mcbsp.h 2009-12-02 > 15:48:51.000000000 +0100 > +++ git/arch/arm/plat-omap/include/plat/mcbsp.h 2009-12-08 > 14:26:42.000000000 +0100 > @@ -157,6 +157,14 @@ > > #endif > > +#define OMAP7XX_MCBSP_REG_NUM (OMAP_MCBSP_REG_XCERH / sizeof(u16) + 1) > +#define OMAP15XX_MCBSP_REG_NUM (OMAP_MCBSP_REG_XCERH / sizeof(u16) + 1) > +#define OMAP16XX_MCBSP_REG_NUM (OMAP_MCBSP_REG_XCERH / sizeof(u16) + 1) > + > +#define OMAP24XX_MCBSP_REG_NUM (OMAP_MCBSP_REG_RCCR / sizeof(u32) + 1) > +#define OMAP34XX_MCBSP_REG_NUM (OMAP_MCBSP_REG_RCCR / sizeof(u32) + 1) > +#define OMAP44XX_MCBSP_REG_NUM (OMAP_MCBSP_REG_RCCR / sizeof(u32) + 1) > + > /************************** McBSP SPCR1 bit definitions > ***********************/ > #define RRST 0x0001 > #define RRDY 0x0002 > @@ -415,6 +423,7 @@ struct omap_mcbsp { > u16 max_tx_thres; > u16 max_rx_thres; > #endif > + void *reg_cache; > }; > extern struct omap_mcbsp **mcbsp_ptr; > extern int omap_mcbsp_count; > diff -upr git.orig/arch/arm/plat-omap/mcbsp.c git/arch/arm/plat-omap/mcbsp.c > --- git.orig/arch/arm/plat-omap/mcbsp.c 2009-12-07 22:43:56.000000000 > +0100 > +++ git/arch/arm/plat-omap/mcbsp.c 2009-12-08 14:38:38.000000000 +0100 > @@ -30,26 +30,40 @@ > struct omap_mcbsp **mcbsp_ptr; > int omap_mcbsp_count; > > -void omap_mcbsp_write(void __iomem *io_base, u16 reg, u32 val) > +void omap_mcbsp_write(struct omap_mcbsp *mcbsp, u16 reg, u32 val) > { > - if (cpu_class_is_omap1() || cpu_is_omap2420()) > - __raw_writew((u16)val, io_base + reg); > - else > - __raw_writel(val, io_base + reg); > + if (cpu_class_is_omap1()) { > + ((u16 *)mcbsp->reg_cache)[reg / sizeof(u16)] = (u16)val; > + __raw_writew((u16)val, mcbsp->io_base + reg); > + } else if (cpu_is_omap2420()) { > + ((u16 *)mcbsp->reg_cache)[reg / sizeof(u32)] = (u16)val; > + __raw_writew((u16)val, mcbsp->io_base + reg); > + } else { > + ((u32 *)mcbsp->reg_cache)[reg / sizeof(u32)] = val; > + __raw_writel(val, mcbsp->io_base + reg); > + } > } > > -int omap_mcbsp_read(void __iomem *io_base, u16 reg) > +int omap_mcbsp_read(struct omap_mcbsp *mcbsp, u16 reg, bool from_cache) > { > - if (cpu_class_is_omap1() || cpu_is_omap2420()) > - return __raw_readw(io_base + reg); > - else > - return __raw_readl(io_base + reg); > + if (cpu_class_is_omap1()) { > + return !from_cache ? __raw_readw(mcbsp->io_base + reg) : > + ((u16 *)mcbsp->reg_cache)[reg / sizeof(u16)]; > + } else if (cpu_is_omap2420()) { > + return !from_cache ? __raw_readw(mcbsp->io_base + reg) : > + ((u16 *)mcbsp->reg_cache)[reg / sizeof(u32)]; > + } else { > + return !from_cache ? __raw_readl(mcbsp->io_base + reg) : > + ((u32 *)mcbsp->reg_cache)[reg / sizeof(u32)]; > + } > } > > #define MCBSP_READ(mcbsp, reg) \ > - omap_mcbsp_read(mcbsp->io_base, OMAP_MCBSP_REG_##reg) > + omap_mcbsp_read(mcbsp, OMAP_MCBSP_REG_##reg, 0) > #define MCBSP_WRITE(mcbsp, reg, val) \ > - omap_mcbsp_write(mcbsp->io_base, OMAP_MCBSP_REG_##reg, val) > + omap_mcbsp_write(mcbsp, OMAP_MCBSP_REG_##reg, val) > +#define MCBSP_READ_CACHE(mcbsp, reg) \ > + omap_mcbsp_read(mcbsp, OMAP_MCBSP_REG_##reg, 1) > > #define omap_mcbsp_check_valid_id(id) (id < omap_mcbsp_count) > #define id_to_mcbsp_ptr(id) mcbsp_ptr[id]; > @@ -391,6 +405,31 @@ int omap_mcbsp_request(unsigned int id) > } > mcbsp = id_to_mcbsp_ptr(id); > > + if (cpu_is_omap7xx()) { > + mcbsp->reg_cache = kzalloc(sizeof(u16) * > + OMAP7XX_MCBSP_REG_NUM, GFP_KERNEL); > + } else if (cpu_is_omap15xx()) { > + mcbsp->reg_cache = kzalloc(sizeof(u16) * > + OMAP15XX_MCBSP_REG_NUM, GFP_KERNEL); > + } else if (cpu_is_omap16xx()) { > + mcbsp->reg_cache = kzalloc(sizeof(u16) * > + OMAP16XX_MCBSP_REG_NUM, GFP_KERNEL); > + } else if (cpu_is_omap2420()) { > + mcbsp->reg_cache = kzalloc(sizeof(u16) * > + OMAP24XX_MCBSP_REG_NUM, GFP_KERNEL); > + } else if (cpu_is_omap2430()) { > + mcbsp->reg_cache = kzalloc(sizeof(u32) * > + OMAP24XX_MCBSP_REG_NUM, GFP_KERNEL); > + } else if (cpu_is_omap34xx()) { > + mcbsp->reg_cache = kzalloc(sizeof(u32) * > + OMAP34XX_MCBSP_REG_NUM, GFP_KERNEL); > + } else if (cpu_is_omap44xx()) { > + mcbsp->reg_cache = kzalloc(sizeof(u32) * > + OMAP44XX_MCBSP_REG_NUM, GFP_KERNEL); > + } How about just set the cache size above based on the processor, then do kzalloc here: mcbsp->reg_cache = kzalloc(size, GFP_KERNEL); > + if (!mcbsp->reg_cache) > + return -ENOMEM; > + That way the kzalloc and error checking are in the same place. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
