On Sun, Jan 10, 2010 at 7:46 PM, Khasim Syed Mohammed <kha...@beagleboard.org> wrote: > On Sun, Jan 10, 2010 at 9:11 PM, Nishanth Menon > <menon.nisha...@gmail.com> wrote: >> Khasim Syed Mohammed said the following on 01/09/2010 09:16 PM: >>> >>> On Sat, Jan 9, 2010 at 8:18 PM, Nishanth Menon <menon.nisha...@gmail.com> >>> wrote: >>>> >>>> Khasim Syed Mohammed said the following on 01/08/2010 09:00 PM: >>>>> >>>>> On Sat, Jan 9, 2010 at 1:31 AM, Nishanth Menon >>>>> <menon.nisha...@gmail.com> >>>>> wrote: >>>>> >>>>>> On Fri, Jan 8, 2010 at 9:40 AM, Khasim Syed Mohammed >>>>>> <kha...@beagleboard.org> wrote: >>>>>> >>>>>>> From 239c47a4180fb4d5b5217f892955524d476916cf Mon Sep 17 00:00:00 2001 >>>>>>> From: Syed Mohammed Khasim <kha...@ti.com> >> >> [...] >> >>>> The recomendation here is to move from #defines to struct based register >>>> usage. I am ok with the rest(except for need to split). >>> >>> Split is done, posted yesterday. >>> >>> Struct based register needs more comments, not that I am lazy to >>> implement that. I need to know the reason for doing the same when no >>> multiple instances are used. >>> >>>>> You can add a new panel or a new tv standard with these structures >>>>> easily. Structures are not used for register accesses. >>>>> >>>>> >>>>>> here is what I think: >>>>>> venc_config { >>>>>> } >>>>>> >>>>>> if it is organized as the register definitions, >>>>>> >>>>>> configure_venc(struct venc_config *values) >>>>>> struct venc_config * d = BASE_ADDRESS_OF_OMAP3_VENC; >>>>>> writel(values->regx, &d->regx); >>>>>> >>>>>> refer: drivers/mtd/nand/omap_gpmc.c >>>>>> >>>>>> >>>>> GPIO, GPMC and other controllers have multiple instances in OMAP, it >>>>> makes sense to organize such register set in structure mode. I did >>>>> start with that but found no use for DSS as it is just one instance. >>>>> Structures don't give any value here. >>>>> >>>> there were other reasons mentioned when nand got split -> one of them had >>>> to >>>> do with the compiler or something. Dirk might remember - unfortunately, >>>> this >>>> was more than a year back.. if I recollect right.. >>> >>> Will try doing a google. May be some one can point me to that >>> decision. It would help developing drivers which have single instance >>> of controller being used. >> >> the reference I got: >> http://old.nabble.com/-U-Boot---PATCH-08-13-v4--ARM%3A-OMAP3%3A-Add-NAND-support-tp20039673p20039673.html >> >> V5 became: >> http://old.nabble.com/-U-Boot---PATCH-07-13-v5--ARM%3A-OMAP3%3A-Add-NAND-support-tp20292477p20292477.html >> >> similar changes happend for GPMC etc.. >> >> Quote: >>> >Is GPMC_BASE an integer or a pointer? >>> >>> Nothing. A macro: >>> >>> #define OMAP34XX_GPMC_BASE (0x6E000000) >>> #define GPMC_BASE (OMAP34XX_GPMC_BASE) >> >> So it's an integer. >> >>> It's then casted to volatile pointer by ARM's readx/writex. >> >> The cast should be done by the driver, or you'll get warnings if >> readx/writex ever become inline functions (as they are on other arches). >> >> might help explain.. >> > This is a valid comment, many thanks for digging this out. Considering > this comment, my dss_read_reg and dss_write_reg should become as shown > below > > +static inline void dss_write_reg(int reg, u32 val) > +{ > + __raw_writel(val, (uint32_t *) reg); > +} > + > +static inline u32 dss_read_reg(int reg) > +{ > + u32 l = __raw_readl((uint32_t *) reg); > + return l; > +} > > I will do the above changes and re-submit the patch. > > But Kindly NOTE: This still doesn't give me a reason to implement the > register definition as structures when we have single instance of > register set. I am still not considering the structure based > read/write currently.
IIRC the main reason was that Wolfgang would refuse to merge initial OMAP3 support unless _all_ register accesses were structure based, single instance or not. He gave his reasons but they didn't look convincing to me (personal humble opinion only), CCed him for a possible comments or reminder :) > >>> >>>>> More over I am introducing minimal DSS driver with minimal register >>>>> set. It doesn't help any to give structure based register access for >>>>> single instance drivers. >>>>> >>>> moving to struct based is easy and done once and improves your chance of >>>> your driver getting upstreamed :). >>> >>> DSS in OMAP3 has following register domains. >>> >>> DSI Protocol Engine 0x4804 FC00 512 bytes >>> DSI_PHY 0x4804 FE00 64 bytes >>> DSI PLL Controller 0x4804 FF00 32 bytes >>> Display Subsystem 0x4805 0000 512 bytes >>> Display Controller 0x4805 0400 1K byte >>> Display Controller VID1 0x4805 0400 1K byte >>> Display Controller VID2 0x4805 0400 1K byte >>> RFBI 0x4805 0800 256 bytes >>> Video Encode 0x4805 0C00 256 bytes >>> >>> I am not sure why one would ask me to give struct definitions for >>> these 500 (approx) registers when only 50 of these are required to >>> implement background and color bar. As I am saying all the way, DSS is >>> not multiple instance module like GPMC (NAND) and GPIO it is just one >>> module. >> >> Aren't you extrapolating this a bit out of scope? DSI,RFBI etc.. is not >> relevant to your patch. > For Beagle it is not, but other boards will have to use DSI, RFBI etc. > We have boards that use these modules today. > >> you may need DSS, controller and VID1(and VID2 is the same). I think your >> complaint is about having >> to define the reg structs when multiple instances dont exist - how about >> OMAP4? wont these structs >> get reused there(once we get around to it)? > > OMAP4 DSS is completely different from that of 3. So it won't be re-used. > > Thanks, > > Regards, > Khasim > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot > _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot