[U-Boot] [beagleboard] TI:OMAP: [PATCH 4/4] Minimal Display driver for OMAP3
Khasim Syed Mohammed
khasim at beagleboard.org
Mon Jan 11 14:48:20 CET 2010
On Mon, Jan 11, 2010 at 6:39 PM, Grazvydas Ignotas <notasas at gmail.com> wrote:
> On Sun, Jan 10, 2010 at 7:46 PM, Khasim Syed Mohammed
> <khasim at beagleboard.org> wrote:
>> On Sun, Jan 10, 2010 at 9:11 PM, Nishanth Menon
>> <menon.nishanth at 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 at 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 at gmail.com>
>>>>>> wrote:
>>>>>>
>>>>>>> On Fri, Jan 8, 2010 at 9:40 AM, Khasim Syed Mohammed
>>>>>>> <khasim at beagleboard.org> wrote:
>>>>>>>
>>>>>>>> From 239c47a4180fb4d5b5217f892955524d476916cf Mon Sep 17 00:00:00 2001
>>>>>>>> From: Syed Mohammed Khasim <khasim at 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 :)
>
oh ok, then I don't want to waste time waiting. I will change them to
structures and repost.
Regards,
Khasim
>
>>
>>>>
>>>>>> 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 at lists.denx.de
>> http://lists.denx.de/mailman/listinfo/u-boot
>>
>
More information about the U-Boot
mailing list