diff mbox

video: ARM CLCD: Added dt support to set tim2 register

Message ID 1424898082-1522-1-git-send-email-arun.ramamurthy@broadcom.com
State New, archived
Headers show

Commit Message

Arun Ramamurthy Feb. 25, 2015, 9:01 p.m. UTC
Added code based on linaro tree:
http://git.linaro.org/kernel/linux-linaro-stable.git
with commit id:6846e7822c4cab5a84672baace3b768c2d0db142
at drivers/video/amba-clcd.c. This lets the driver set
certain tim2 register bits after reading them from
device tree.

Reviewed-by: Ray Jui <rjui@broadcom.com>
Reviewed-by: Scott Branden <sbranden@broadcom.com>
Signed-off-by: Arun Ramamurthy <arun.ramamurthy@broadcom.com>
---
 .../devicetree/bindings/video/arm,pl11x.txt        | 17 ++++++++-
 drivers/video/fbdev/amba-clcd.c                    | 41 ++++++++++++++++++++++
 2 files changed, 57 insertions(+), 1 deletion(-)

Comments

Pawel Moll March 2, 2015, 4:11 p.m. UTC | #1
On Wed, 2015-02-25 at 21:01 +0000, Arun Ramamurthy wrote:
> Added code based on linaro tree:
> http://git.linaro.org/kernel/linux-linaro-stable.git
> with commit id:6846e7822c4cab5a84672baace3b768c2d0db142
> at drivers/video/amba-clcd.c. This lets the driver set
> certain tim2 register bits after reading them from
> device tree.
> 
> Reviewed-by: Ray Jui <rjui@broadcom.com>
> Reviewed-by: Scott Branden <sbranden@broadcom.com>
> Signed-off-by: Arun Ramamurthy <arun.ramamurthy@broadcom.com>
> ---
>  .../devicetree/bindings/video/arm,pl11x.txt        | 17 ++++++++-
>  drivers/video/fbdev/amba-clcd.c                    | 41 ++++++++++++++++++++++
>  2 files changed, 57 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/video/arm,pl11x.txt b/Documentation/devicetree/bindings/video/arm,pl11x.txt
> index 3e3039a..14d6f87 100644
> --- a/Documentation/devicetree/bindings/video/arm,pl11x.txt
> +++ b/Documentation/devicetree/bindings/video/arm,pl11x.txt
> @@ -35,6 +35,21 @@ Optional properties:
>  	cell's memory interface can handle; if not present, the memory
>  	interface is fast enough to handle all possible video modes
>  
> +- tim2: Used to set certain bits in LCDTiming2 register.
> +	It can be TIM2_CLKSEL or TIM2_IOE or both
> +
> +	TIM2_CLKSEL: This bit drives the CLCDCLKSEL signal. It is the select
> +	signal for the external LCD clock multiplexor.
> +
> +	TIM2_IOE: Invert output enable:
> +	0 = CLAC output pin is active HIGH in TFT mode
> +	1 = CLAC output pin is active LOW in TFT mode.
> +	This bit selects the active polarity of the output enable signal in
> +	TFT mode. In this mode, the CLAC pin is an enable that indicates to
> +	the LCD panel when valid display data is available. In active
> +	display mode, data is driven onto the LCD data lines at the
> +	programmed edge of CLCP when CLAC is in its active state.
> +

The existing bindings intentionally avoided quoting internal registers -
they are supposed to describe how the hardware is wired up...

So how about something like "arm,pl11x,tft-invert-clac"? Then the driver
sets the bit or not, depending on the property existance?

Pawel

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arun Ramamurthy March 2, 2015, 7:09 p.m. UTC | #2
On 15-03-02 08:11 AM, Pawel Moll wrote:
> On Wed, 2015-02-25 at 21:01 +0000, Arun Ramamurthy wrote:
>> Added code based on linaro tree:
>> http://git.linaro.org/kernel/linux-linaro-stable.git
>> with commit id:6846e7822c4cab5a84672baace3b768c2d0db142
>> at drivers/video/amba-clcd.c. This lets the driver set
>> certain tim2 register bits after reading them from
>> device tree.
>>
>> Reviewed-by: Ray Jui <rjui@broadcom.com>
>> Reviewed-by: Scott Branden <sbranden@broadcom.com>
>> Signed-off-by: Arun Ramamurthy <arun.ramamurthy@broadcom.com>
>> ---
>>   .../devicetree/bindings/video/arm,pl11x.txt        | 17 ++++++++-
>>   drivers/video/fbdev/amba-clcd.c                    | 41 ++++++++++++++++++++++
>>   2 files changed, 57 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/video/arm,pl11x.txt b/Documentation/devicetree/bindings/video/arm,pl11x.txt
>> index 3e3039a..14d6f87 100644
>> --- a/Documentation/devicetree/bindings/video/arm,pl11x.txt
>> +++ b/Documentation/devicetree/bindings/video/arm,pl11x.txt
>> @@ -35,6 +35,21 @@ Optional properties:
>>   	cell's memory interface can handle; if not present, the memory
>>   	interface is fast enough to handle all possible video modes
>>
>> +- tim2: Used to set certain bits in LCDTiming2 register.
>> +	It can be TIM2_CLKSEL or TIM2_IOE or both
>> +
>> +	TIM2_CLKSEL: This bit drives the CLCDCLKSEL signal. It is the select
>> +	signal for the external LCD clock multiplexor.
>> +
>> +	TIM2_IOE: Invert output enable:
>> +	0 = CLAC output pin is active HIGH in TFT mode
>> +	1 = CLAC output pin is active LOW in TFT mode.
>> +	This bit selects the active polarity of the output enable signal in
>> +	TFT mode. In this mode, the CLAC pin is an enable that indicates to
>> +	the LCD panel when valid display data is available. In active
>> +	display mode, data is driven onto the LCD data lines at the
>> +	programmed edge of CLCP when CLAC is in its active state.
>> +
>
> The existing bindings intentionally avoided quoting internal registers -
> they are supposed to describe how the hardware is wired up...
>
> So how about something like "arm,pl11x,tft-invert-clac"? Then the driver
> sets the bit or not, depending on the property existance?
>
Sure, I can change it to two properties called arm,pl11x,tft-invert-clac 
and arm,pl11x,tft-clksel. Would that be acceptable?

> Pawel
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pawel Moll March 3, 2015, 10:02 a.m. UTC | #3
On Mon, 2015-03-02 at 19:09 +0000, Arun Ramamurthy wrote:
> > The existing bindings intentionally avoided quoting internal registers -
> > they are supposed to describe how the hardware is wired up...
> >
> > So how about something like "arm,pl11x,tft-invert-clac"? Then the driver
> > sets the bit or not, depending on the property existance?
> >
> Sure, I can change it to two properties called arm,pl11x,tft-invert-clac 
> and arm,pl11x,tft-clksel. Would that be acceptable?

That would be fine by me :-)

Pawel

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pawel Moll March 3, 2015, 10:22 a.m. UTC | #4
On Tue, 2015-03-03 at 10:02 +0000, Pawel Moll wrote:
> On Mon, 2015-03-02 at 19:09 +0000, Arun Ramamurthy wrote:
> > > The existing bindings intentionally avoided quoting internal registers -
> > > they are supposed to describe how the hardware is wired up...
> > >
> > > So how about something like "arm,pl11x,tft-invert-clac"? Then the driver
> > > sets the bit or not, depending on the property existance?
> > >
> > Sure, I can change it to two properties called arm,pl11x,tft-invert-clac 
> > and arm,pl11x,tft-clksel. Would that be acceptable?
> 
> That would be fine by me :-)

Or (after having a look at the TRM) I should rather say: the invert-clac
is fine by me :-) but the tft-clksel doesn't work, I afraid.

If I'm not mistaken, there are two problems with it.

Number one: it's not TFT-specific, is it? So it certainly should not
have the "tft-" bit.

Number two: setting this bit says "do not use CLCDCLK for the logic; use
HCLK instead", correct? If so, have a look at the clock properties. They
say:

- clock-names: should contain "clcdclk" and "apb_pclk"

- clocks: contains phandle and clock specifier pairs for the entries
        in the clock-names property. See

So if your hardware has the reference clock wired to HCLK, and you
defining the clocks as "clcdclk", you are (no offence meant ;-)
lying :-)

So how about solving the problem by extending the clock-names definition
like this (feel free to use own wording):

- clock-names: should contain two clocks, either "clcdclk" or "hclk"
               (depending on which input is to be used as a reference
               clock by the controller logic) and "apb_pclk"

That way you're precisely describing the way the hardware is wired up.
And the driver simply tries to get clcdclk first, if it's defined -
cool, set clksel to 1, if not - try hclk and set clksel to 0. If neither
of them is present - bail out.

Does this make any sense?

Pawel

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arun Ramamurthy March 4, 2015, 12:37 a.m. UTC | #5
On 15-03-03 02:22 AM, Pawel Moll wrote:
> On Tue, 2015-03-03 at 10:02 +0000, Pawel Moll wrote:
>> On Mon, 2015-03-02 at 19:09 +0000, Arun Ramamurthy wrote:
>>>> The existing bindings intentionally avoided quoting internal registers -
>>>> they are supposed to describe how the hardware is wired up...
>>>>
>>>> So how about something like "arm,pl11x,tft-invert-clac"? Then the driver
>>>> sets the bit or not, depending on the property existance?
>>>>
>>> Sure, I can change it to two properties called arm,pl11x,tft-invert-clac
>>> and arm,pl11x,tft-clksel. Would that be acceptable?
>>
>> That would be fine by me :-)
>
> Or (after having a look at the TRM) I should rather say: the invert-clac
> is fine by me :-) but the tft-clksel doesn't work, I afraid.
>
> If I'm not mistaken, there are two problems with it.
>
> Number one: it's not TFT-specific, is it? So it certainly should not
> have the "tft-" bit.
>
> Number two: setting this bit says "do not use CLCDCLK for the logic; use
> HCLK instead", correct? If so, have a look at the clock properties. They
> say:
>
> - clock-names: should contain "clcdclk" and "apb_pclk"
>
> - clocks: contains phandle and clock specifier pairs for the entries
>          in the clock-names property. See
>
> So if your hardware has the reference clock wired to HCLK, and you
> defining the clocks as "clcdclk", you are (no offence meant ;-)
> lying :-)
>
No offense taken :)
> So how about solving the problem by extending the clock-names definition
> like this (feel free to use own wording):
>
> - clock-names: should contain two clocks, either "clcdclk" or "hclk"
>                 (depending on which input is to be used as a reference
>                 clock by the controller logic) and "apb_pclk"
>
> That way you're precisely describing the way the hardware is wired up.
> And the driver simply tries to get clcdclk first, if it's defined -
> cool, set clksel to 1, if not - try hclk and set clksel to 0. If neither
> of them is present - bail out.
>
> Does this make any sense?
>
This makes sense to me, thank you for the suggestions. I will fix it all 
up in V2
> Pawel
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pawel Moll March 5, 2015, 10:59 a.m. UTC | #6
On Wed, 2015-03-04 at 00:37 +0000, Arun Ramamurthy wrote:
> > That way you're precisely describing the way the hardware is wired up.
> > And the driver simply tries to get clcdclk first, if it's defined -
> > cool, set clksel to 1, if not - try hclk and set clksel to 0. If neither
> > of them is present - bail out.
> >
> > Does this make any sense?
> >
> This makes sense to me, thank you for the suggestions. I will fix it all 
> up in V2

Cool. Just a word of comment to my own words ;-) The "bail out" case was
a bad idea - the non-DT use cases more likely than not will have no
clock name defined. So, to maintain backward compatibility, the driver
will still have to work when no named clock is available. I think the
simplest way to do that is to check if "hclk" is available, and use it
if it is (setting the clksel accordingly). Otherwise - proceed as it was
the case previously.

Pawel

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Russell King - ARM Linux March 9, 2015, 4:16 p.m. UTC | #7
On Tue, Mar 03, 2015 at 10:22:07AM +0000, Pawel Moll wrote:
> On Tue, 2015-03-03 at 10:02 +0000, Pawel Moll wrote:
> > On Mon, 2015-03-02 at 19:09 +0000, Arun Ramamurthy wrote:
> > > > The existing bindings intentionally avoided quoting internal registers -
> > > > they are supposed to describe how the hardware is wired up...
> > > >
> > > > So how about something like "arm,pl11x,tft-invert-clac"? Then the driver
> > > > sets the bit or not, depending on the property existance?
> > > >
> > > Sure, I can change it to two properties called arm,pl11x,tft-invert-clac 
> > > and arm,pl11x,tft-clksel. Would that be acceptable?
> > 
> > That would be fine by me :-)
> 
> Or (after having a look at the TRM) I should rather say: the invert-clac
> is fine by me :-) but the tft-clksel doesn't work, I afraid.
> 
> If I'm not mistaken, there are two problems with it.
> 
> Number one: it's not TFT-specific, is it? So it certainly should not
> have the "tft-" bit.
> 
> Number two: setting this bit says "do not use CLCDCLK for the logic; use
> HCLK instead", correct? If so, have a look at the clock properties. They
> say:
> 
> - clock-names: should contain "clcdclk" and "apb_pclk"
> 
> - clocks: contains phandle and clock specifier pairs for the entries
>         in the clock-names property. See
> 
> So if your hardware has the reference clock wired to HCLK, and you
> defining the clocks as "clcdclk", you are (no offence meant ;-)
> lying :-)

No.  The CLCD block always takes two clock signals - the AHB bus clock
(HCLK) for the slave interface, and a CLCD clock.

The CLCDCLKSEL is a bit which affects a signal sent to the world outside
of the CLCD block, which is used to drive an _external_ multiplexer to
select the CLCD clock source.  (See the description for bit 5 of the
LCDTiming2 register.)

So, the clock is still input to the CLCDCLK input, even if it is ultimately
derived from HCLK.

Remember, the clock API does not deal with names describing the source of
the clock, but the consumer of the clock.  The consumer in this case is
the PL11x CLCD block, which takes a CLCDCLK input.
Linus Walleij Feb. 10, 2016, 1:58 p.m. UTC | #8
On Wed, Feb 25, 2015 at 10:01 PM, Arun Ramamurthy
<arun.ramamurthy@broadcom.com> wrote:

> Added code based on linaro tree:
> http://git.linaro.org/kernel/linux-linaro-stable.git
> with commit id:6846e7822c4cab5a84672baace3b768c2d0db142
> at drivers/video/amba-clcd.c. This lets the driver set
> certain tim2 register bits after reading them from
> device tree.
>
> Reviewed-by: Ray Jui <rjui@broadcom.com>
> Reviewed-by: Scott Branden <sbranden@broadcom.com>
> Signed-off-by: Arun Ramamurthy <arun.ramamurthy@broadcom.com>

I have now implemented this properly in this patch:
http://marc.info/?l=linux-fbdev&m=145459469913983&w=2

Please provide your Tested-by/Reviewed-by/Ack if you're
still working on this.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ray Jui Feb. 10, 2016, 5:48 p.m. UTC | #9
Hi Linus,

On 2/10/2016 5:58 AM, Linus Walleij wrote:
> On Wed, Feb 25, 2015 at 10:01 PM, Arun Ramamurthy
> <arun.ramamurthy@broadcom.com> wrote:
>
>> Added code based on linaro tree:
>> http://git.linaro.org/kernel/linux-linaro-stable.git
>> with commit id:6846e7822c4cab5a84672baace3b768c2d0db142
>> at drivers/video/amba-clcd.c. This lets the driver set
>> certain tim2 register bits after reading them from
>> device tree.
>>
>> Reviewed-by: Ray Jui <rjui@broadcom.com>
>> Reviewed-by: Scott Branden <sbranden@broadcom.com>
>> Signed-off-by: Arun Ramamurthy <arun.ramamurthy@broadcom.com>
>
> I have now implemented this properly in this patch:
> http://marc.info/?l=linux-fbdev&m=145459469913983&w=2
>
> Please provide your Tested-by/Reviewed-by/Ack if you're
> still working on this.

Could you please add me to the email thread and I can review it there (I 
won't have time to test, but I can help to review the code and find time 
to test later)?

This may be a dumb question, is there any way for me to directly reply 
to the thread here?

http://marc.info/?l=linux-fbdev&m=145459469913983&w=2

>
> Yours,
> Linus Walleij
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij Feb. 15, 2016, 1:25 p.m. UTC | #10
On Wed, Feb 10, 2016 at 6:48 PM, Ray Jui <ray.jui@broadcom.com> wrote:

> Could you please add me to the email thread and I can review it there (I
> won't have time to test, but I can help to review the code and find time to
> test later)?

OK I will add you to subsequent postings, if any.

> This may be a dumb question, is there any way for me to directly reply to
> the thread here?

Not easily, I guess it is possible to conjure an SMTP mail
with the right in-reply-to message ID but that is so complex
hacking that I have no clue how to do it, just ever reached
the limit of "a little knowledge is dangerous".

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ray Jui Feb. 17, 2016, 5:42 p.m. UTC | #11
On 2/16/2016 11:32 AM, Dmitry Torokhov wrote:
>
>
> On Mon, Feb 15, 2016 at 5:25 AM, Linus Walleij <linus.walleij@linaro.org
> <mailto:linus.walleij@linaro.org>> wrote:
>
>     On Wed, Feb 10, 2016 at 6:48 PM, Ray Jui <ray.jui@broadcom.com
>     <mailto:ray.jui@broadcom.com>> wrote:
>
>     > Could you please add me to the email thread and I can review it there (I
>     > won't have time to test, but I can help to review the code and find time to
>     > test later)?
>
>     OK I will add you to subsequent postings, if any.
>
>     > This may be a dumb question, is there any way for me to directly reply to
>     > the thread here?
>
>     Not easily, I guess it is possible to conjure an SMTP mail
>     with the right in-reply-to message ID but that is so complex
>     hacking that I have no clue how to do it, just ever reached
>     the limit of "a little knowledge is dangerous".
>
>
> To reply to a patch find it in patchwork, download it as mbox, open
> downloaded file with mutt and reply as usual.
>
> Thanks,
> Dmitry

Got it, thanks!

Ray
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/video/arm,pl11x.txt b/Documentation/devicetree/bindings/video/arm,pl11x.txt
index 3e3039a..14d6f87 100644
--- a/Documentation/devicetree/bindings/video/arm,pl11x.txt
+++ b/Documentation/devicetree/bindings/video/arm,pl11x.txt
@@ -35,6 +35,21 @@  Optional properties:
 	cell's memory interface can handle; if not present, the memory
 	interface is fast enough to handle all possible video modes
 
+- tim2: Used to set certain bits in LCDTiming2 register.
+	It can be TIM2_CLKSEL or TIM2_IOE or both
+
+	TIM2_CLKSEL: This bit drives the CLCDCLKSEL signal. It is the select
+	signal for the external LCD clock multiplexor.
+
+	TIM2_IOE: Invert output enable:
+	0 = CLAC output pin is active HIGH in TFT mode
+	1 = CLAC output pin is active LOW in TFT mode.
+	This bit selects the active polarity of the output enable signal in
+	TFT mode. In this mode, the CLAC pin is an enable that indicates to
+	the LCD panel when valid display data is available. In active
+	display mode, data is driven onto the LCD data lines at the
+	programmed edge of CLCP when CLAC is in its active state.
+
 Required sub-nodes:
 
 - port: describes LCD panel signals, following the common binding
@@ -76,7 +91,7 @@  Example:
 		clocks = <&oscclk1>, <&oscclk2>;
 		clock-names = "clcdclk", "apb_pclk";
 		max-memory-bandwidth = <94371840>; /* Bps, 1024x768@60 16bpp */
-
+		tim2 = "TIM2_CLKSEL";
 		port {
 			clcd_pads: endpoint {
 				remote-endpoint = <&clcd_panel>;
diff --git a/drivers/video/fbdev/amba-clcd.c b/drivers/video/fbdev/amba-clcd.c
index 32c0b6b..4e4e50f 100644
--- a/drivers/video/fbdev/amba-clcd.c
+++ b/drivers/video/fbdev/amba-clcd.c
@@ -41,6 +41,44 @@ 
 /* This is limited to 16 characters when displayed by X startup */
 static const char *clcd_name = "CLCD FB";
 
+struct string_lookup {
+	const char	*string;
+	const u32	val;
+};
+
+static const struct string_lookup tim2_lookups[] = {
+	{ "TIM2_CLKSEL",	TIM2_CLKSEL},
+	{ "TIM2_IOE",		TIM2_IOE},
+	{ NULL, 0},
+};
+
+static u32 parse_setting(const struct string_lookup *lookup, const char *name)
+{
+	int i = 0;
+
+	while (lookup[i].string != NULL) {
+		if (strcmp(lookup[i].string, name) == 0)
+			return lookup[i].val;
+		++i;
+	}
+	return 0;
+}
+
+static u32 get_string_lookup(struct device_node *node, const char *name,
+		      const struct string_lookup *lookup)
+{
+	const char *string;
+	int count, i;
+	u32 ret = 0;
+
+	count = of_property_count_strings(node, name);
+	if (count >= 0)
+		for (i = 0; i < count; i++)
+			if (of_property_read_string_index(node, name, i,
+					&string) == 0)
+				ret |= parse_setting(lookup, string);
+	return ret;
+}
 /*
  * Unfortunately, the enable/disable functions may be called either from
  * process or IRQ context, and we _need_ to delay.  This is _not_ good.
@@ -626,6 +664,9 @@  static int clcdfb_of_init_tft_panel(struct clcd_fb *fb, u32 r0, u32 g0, u32 b0)
 	/* Bypass pixel clock divider, data output on the falling edge */
 	fb->panel->tim2 = TIM2_BCD | TIM2_IPC;
 
+	fb->panel->tim2 |= get_string_lookup(fb->dev->dev.of_node,
+					"tim2", tim2_lookups);
+
 	/* TFT display, vert. comp. interrupt at the start of the back porch */
 	fb->panel->cntl |= CNTL_LCDTFT | CNTL_LCDVCOMP(1);