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

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

[Khasim Syed Mohammed]

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

[Nishanth Menon]

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>
> Date: Fri, 8 Jan 2010 21:01:44 +0530
> Subject: [PATCH] Minimal Display driver for OMAP3
>
> Supports dynamic configuration of Panel and Video Encoder
> Supports Background color on DVID
> Supports Color bar on S-Video

We are getting there.. thanks a bunch. if you can split this series
into two sets:
a) introducing DSS layer
b) introduce beagle support for the same
it will be better.

but NAK to this patch.

>
> Signed-off-by: Syed Mohammed Khasim <khasim@ti.com>
> ---
> ?board/ti/beagle/beagle.c ? ? ? ? | ? 13 +++
> ?board/ti/beagle/beagle.h ? ? ? ? | ? 73 ++++++++++++++
> ?drivers/video/Makefile ? ? ? ? ? | ? ?1 +
> ?drivers/video/omap3_dss.c ? ? ? ?| ?128 +++++++++++++++++++++++++
> ?include/asm-arm/arch-omap3/dss.h | ?193 ++++++++++++++++++++++++++++++++++++++
> ?include/configs/omap3_beagle.h ? | ? ?1 +
> ?6 files changed, 409 insertions(+), 0 deletions(-)
> ?create mode 100644 drivers/video/omap3_dss.c
> ?create mode 100644 include/asm-arm/arch-omap3/dss.h
>
[...]
> diff --git a/include/asm-arm/arch-omap3/dss.h b/include/asm-arm/arch-omap3/dss.h
> new file mode 100644
> index 0000000..08c7d8d
> --- /dev/null
> +++ b/include/asm-arm/arch-omap3/dss.h
> @@ -0,0 +1,193 @@
> +/*
> + * (C) Copyright 2010
> + * Texas Instruments, <www.ti.com>
> + * Syed Mohammed Khasim <khasim@ti.com>
> + *
> + * Referred to Linux DSS driver files for OMAP3
> + *
> + * See file CREDITS for list of people who contributed to this
> + * project.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation's version 2 of
> + * the License.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. ?See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> + * MA 02111-1307 USA
> + */
> +
> +#ifndef DSS_H
> +#define DSS_H
> +
> +/* VENC Register address */
> +#define VENC_REV_ID ? ? ? ? ? ? ? ? ? ? ? ? ? ?0x48050C00

NAK. why do you need this if you have a struct?


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

> +#define VENC_STATUS ? ? ? ? ? ? ? ? ? ? ? ? ? ?0x48050C04
> +#define VENC_F_CONTROL ? ? ? ? ? ? ? ? ? ? ? ? 0x48050C08
[...]
Regards,
Nishanth Menon

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

[Khasim Syed Mohammed]

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>
>> Date: Fri, 8 Jan 2010 21:01:44 +0530
>> Subject: [PATCH] Minimal Display driver for OMAP3
>>
>> Supports dynamic configuration of Panel and Video Encoder
>> Supports Background color on DVID
>> Supports Color bar on S-Video
>
> We are getting there.. thanks a bunch. if you can split this series
> into two sets:
> a) introducing DSS layer
> b) introduce beagle support for the same
> it will be better.

Can you complete your review review comments here, I will take up this
when in-corporate all review comments together.

Kindly remember these patches will be tested on B4, C2, C3, C4, EVM
before releasing.

>
> but NAK to this patch.
>
>>
>> Signed-off-by: Syed Mohammed Khasim <khasim@ti.com>
>> ---
>> ?board/ti/beagle/beagle.c ? ? ? ? | ? 13 +++
>> ?board/ti/beagle/beagle.h ? ? ? ? | ? 73 ++++++++++++++
>> ?drivers/video/Makefile ? ? ? ? ? | ? ?1 +
>> ?drivers/video/omap3_dss.c ? ? ? ?| ?128 +++++++++++++++++++++++++
>> ?include/asm-arm/arch-omap3/dss.h | ?193 ++++++++++++++++++++++++++++++++++++++
>> ?include/configs/omap3_beagle.h ? | ? ?1 +
>> ?6 files changed, 409 insertions(+), 0 deletions(-)
>> ?create mode 100644 drivers/video/omap3_dss.c
>> ?create mode 100644 include/asm-arm/arch-omap3/dss.h
>>
> [...]
>> diff --git a/include/asm-arm/arch-omap3/dss.h b/include/asm-arm/arch-omap3/dss.h
>> new file mode 100644
>> index 0000000..08c7d8d
>> --- /dev/null
>> +++ b/include/asm-arm/arch-omap3/dss.h
>> @@ -0,0 +1,193 @@
>> +/*
>> + * (C) Copyright 2010
>> + * Texas Instruments, <www.ti.com>
>> + * Syed Mohammed Khasim <khasim@ti.com>
>> + *
>> + * Referred to Linux DSS driver files for OMAP3
>> + *
>> + * See file CREDITS for list of people who contributed to this
>> + * project.
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License as
>> + * published by the Free Software Foundation's version 2 of
>> + * the License.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. ?See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, write to the Free Software
>> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
>> + * MA 02111-1307 USA
>> + */
>> +
>> +#ifndef DSS_H
>> +#define DSS_H
>> +
>> +/* VENC Register address */
>> +#define VENC_REV_ID ? ? ? ? ? ? ? ? ? ? ? ? ? ?0x48050C00
>
> NAK. why do you need this if you have a struct?

You need to read patch more carefully before giving NAK.

Structure is used to give multiple instance of Panels or different
VENC settings like NTSC or PAL

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.

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.

Give more reasons for NAK.

Regards,
Khasim

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

[Nishanth Menon]

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>
>>> Date: Fri, 8 Jan 2010 21:01:44 +0530
>>> Subject: [PATCH] Minimal Display driver for OMAP3
>>>
>>> Supports dynamic configuration of Panel and Video Encoder
>>> Supports Background color on DVID
>>> Supports Color bar on S-Video
>>>       
>> We are getting there.. thanks a bunch. if you can split this series
>> into two sets:
>> a) introducing DSS layer
>> b) introduce beagle support for the same
>> it will be better.
>>     
>
> Can you complete your review review comments here, I will take up this
> when in-corporate all review comments together.
>
>   
if you can give it a week before posting again, you could probably 
collate comments from various quarters. I just am a part time reviewer ;)..


> Kindly remember these patches will be tested on B4, C2, C3, C4, EVM
> before releasing.
>   
great. good to know.
>   
>> but NAK to this patch.
>>
>>     
>>> Signed-off-by: Syed Mohammed Khasim <khasim@ti.com>
>>> ---
>>>  board/ti/beagle/beagle.c         |   13 +++
>>>  board/ti/beagle/beagle.h         |   73 ++++++++++++++
>>>  drivers/video/Makefile           |    1 +
>>>  drivers/video/omap3_dss.c        |  128 +++++++++++++++++++++++++
>>>  include/asm-arm/arch-omap3/dss.h |  193 ++++++++++++++++++++++++++++++++++++++
>>>  include/configs/omap3_beagle.h   |    1 +
>>>  6 files changed, 409 insertions(+), 0 deletions(-)
>>>  create mode 100644 drivers/video/omap3_dss.c
>>>  create mode 100644 include/asm-arm/arch-omap3/dss.h
>>>
>>>       
>> [...]
>>     
>>> diff --git a/include/asm-arm/arch-omap3/dss.h b/include/asm-arm/arch-omap3/dss.h
>>> new file mode 100644
>>> index 0000000..08c7d8d
>>> --- /dev/null
>>> +++ b/include/asm-arm/arch-omap3/dss.h
>>> @@ -0,0 +1,193 @@
>>> +/*
>>> + * (C) Copyright 2010
>>> + * Texas Instruments, <www.ti.com>
>>> + * Syed Mohammed Khasim <khasim@ti.com>
>>> + *
>>> + * Referred to Linux DSS driver files for OMAP3
>>> + *
>>> + * See file CREDITS for list of people who contributed to this
>>> + * project.
>>> + *
>>> + * This program is free software; you can redistribute it and/or
>>> + * modify it under the terms of the GNU General Public License as
>>> + * published by the Free Software Foundation's version 2 of
>>> + * the License.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> + * GNU General Public License for more details.
>>> + *
>>> + * You should have received a copy of the GNU General Public License
>>> + * along with this program; if not, write to the Free Software
>>> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
>>> + * MA 02111-1307 USA
>>> + */
>>> +
>>> +#ifndef DSS_H
>>> +#define DSS_H
>>> +
>>> +/* VENC Register address */
>>> +#define VENC_REV_ID                            0x48050C00
>>>       
>> NAK. why do you need this if you have a struct?
>>     
>
> You need to read patch more carefully before giving NAK.
>
> Structure is used to give multiple instance of Panels or different
> VENC settings like NTSC or PAL
>   
Thanks for clarifying
That is the crux of the problem. if you use struct to describe the venc 
-> then you'd be staying with how the rest of u-boot accesses are:
using struct dereference. if you look at nand.c as I pointed out, Dirk 
had done a great job of converting register offsets
to struct and referencing off it. This was required to get the driver 
mainlined.
The recomendation here is to move from #defines to struct based register 
usage. I am ok with the rest(except for need to split).


> 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..
> 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 :).

> Give more reasons for NAK.
>   
This is the only reason I have other than the need to split this patch 
up into logical chunks. my NAK still stands unfortunately for the 
following generic reasons:
a) split the patch as mentioned above.
b) move away from #defines to struct based reg access.
> Regards,
> Khasim
>
>   

Regards,
Nishanth Menon

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

[Khasim Syed Mohammed]

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>
>>>> Date: Fri, 8 Jan 2010 21:01:44 +0530
>>>> Subject: [PATCH] Minimal Display driver for OMAP3
>>>>
>>>> Supports dynamic configuration of Panel and Video Encoder
>>>> Supports Background color on DVID
>>>> Supports Color bar on S-Video
>>>>
>>>
>>> We are getting there.. thanks a bunch. if you can split this series
>>> into two sets:
>>> a) introducing DSS layer
>>> b) introduce beagle support for the same
>>> it will be better.
>>>
>>
>> Can you complete your review review comments here, I will take up this
>> when in-corporate all review comments together.
>>
>>
>
> if you can give it a week before posting again, you could probably collate
> comments from various quarters. I just am a part time reviewer ;)..
>
>
>> Kindly remember these patches will be tested on B4, C2, C3, C4, EVM
>> before releasing.
>>
>
> great. good to know.
>>
>>
>>>
>>> but NAK to this patch.
>>>
>>>
>>>>
>>>> Signed-off-by: Syed Mohammed Khasim <khasim@ti.com>
>>>> ---
>>>> ?board/ti/beagle/beagle.c ? ? ? ? | ? 13 +++
>>>> ?board/ti/beagle/beagle.h ? ? ? ? | ? 73 ++++++++++++++
>>>> ?drivers/video/Makefile ? ? ? ? ? | ? ?1 +
>>>> ?drivers/video/omap3_dss.c ? ? ? ?| ?128 +++++++++++++++++++++++++
>>>> ?include/asm-arm/arch-omap3/dss.h | ?193
>>>> ++++++++++++++++++++++++++++++++++++++
>>>> ?include/configs/omap3_beagle.h ? | ? ?1 +
>>>> ?6 files changed, 409 insertions(+), 0 deletions(-)
>>>> ?create mode 100644 drivers/video/omap3_dss.c
>>>> ?create mode 100644 include/asm-arm/arch-omap3/dss.h
>>>>
>>>>
>>>
>>> [...]
>>>
>>>>
>>>> diff --git a/include/asm-arm/arch-omap3/dss.h
>>>> b/include/asm-arm/arch-omap3/dss.h
>>>> new file mode 100644
>>>> index 0000000..08c7d8d
>>>> --- /dev/null
>>>> +++ b/include/asm-arm/arch-omap3/dss.h
>>>> @@ -0,0 +1,193 @@
>>>> +/*
>>>> + * (C) Copyright 2010
>>>> + * Texas Instruments, <www.ti.com>
>>>> + * Syed Mohammed Khasim <khasim@ti.com>
>>>> + *
>>>> + * Referred to Linux DSS driver files for OMAP3
>>>> + *
>>>> + * See file CREDITS for list of people who contributed to this
>>>> + * project.
>>>> + *
>>>> + * This program is free software; you can redistribute it and/or
>>>> + * modify it under the terms of the GNU General Public License as
>>>> + * published by the Free Software Foundation's version 2 of
>>>> + * the License.
>>>> + *
>>>> + * This program is distributed in the hope that it will be useful,
>>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. ?See the
>>>> + * GNU General Public License for more details.
>>>> + *
>>>> + * You should have received a copy of the GNU General Public License
>>>> + * along with this program; if not, write to the Free Software
>>>> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
>>>> + * MA 02111-1307 USA
>>>> + */
>>>> +
>>>> +#ifndef DSS_H
>>>> +#define DSS_H
>>>> +
>>>> +/* VENC Register address */
>>>> +#define VENC_REV_ID ? ? ? ? ? ? ? ? ? ? ? ? ? ?0x48050C00
>>>>
>>>
>>> NAK. why do you need this if you have a struct?
>>>
>>
>> You need to read patch more carefully before giving NAK.
>>
>> Structure is used to give multiple instance of Panels or different
>> VENC settings like NTSC or PAL
>>
>
> Thanks for clarifying
> That is the crux of the problem. if you use struct to describe the venc ->
> then you'd be staying with how the rest of u-boot accesses are:
> using struct dereference. if you look at nand.c as I pointed out, Dirk had
> done a great job of converting register offsets
> to struct and referencing off it. This was required to get the driver
> mainlined.
As said before, I agree. NAND and GPIO needs struct based development
as it has multiple instances. It will look odd to implement in
#defines.

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

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

Will wait for others in list to comment on these.

>> Give more reasons for NAK.
>>
>
> This is the only reason I have other than the need to split this patch up
> into logical chunks. my NAK still stands unfortunately for the following
> generic reasons:
> a) split the patch as mentioned above.
Done.
> b) move away from #defines to struct based reg access.
Reasons given above. Will wait for others in community to comment on
the above implementation.

Many thanks for the review.

Regards,
Khasim

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

[Nishanth Menon]

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

> 
>>> 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.
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)?


Regards,
Nishanth Menon

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

[Khasim Syed Mohammed]

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

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

[Grazvydas Ignotas]

On Sun, Jan 10, 2010 at 7:46 PM, Khasim Syed Mohammed
<khasim@beagleboard.org> wrote:
> 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.

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
>

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

[Khasim Syed Mohammed]

On Mon, Jan 11, 2010 at 6:39 PM, Grazvydas Ignotas <notasas@gmail.com> wrote:
> On Sun, Jan 10, 2010 at 7:46 PM, Khasim Syed Mohammed
> <khasim@beagleboard.org> wrote:
>> 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.
>
> 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
>>
>