[U-Boot] [beagleboard] TI:OMAP: [PATCH 4/4] Minimal Display driver for OMAP3

[Khasim Syed Mohammed <khasim@beagleboard.org>]

On Sun, Jan 10, 2010 at 9:11 PM, Nishanth Menon
<menon.nishanth@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.nishanth@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.nishanth@gmail.com>
>>>> wrote:
>>>>
>>>>> On Fri, Jan 8, 2010 at 9:40 AM, Khasim Syed Mohammed
>>>>> <khasim@beagleboard.org> wrote:
>>>>>
>>>>>> From 239c47a4180fb4d5b5217f892955524d476916cf Mon Sep 17 00:00:00 2001
>>>>>> From: Syed Mohammed Khasim <khasim@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.

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