[U-Boot] [beagleboard] TI:OMAP: [PATCH 4/4] Minimal Display driver for OMAP3
Grazvydas Ignotas
notasas at gmail.com
Mon Jan 11 14:09:17 CET 2010
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 :)
>
>>>
>>>>> 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