* [PATCH 4/4] OMAPDSS: Add dump and debug support for LCD3
From: Chandrabhanu Mahapatra @ 2012-06-28 9:53 UTC (permalink / raw)
To: tomi.valkeinen; +Cc: linux-omap, linux-fbdev, Chandrabhanu Mahapatra
In-Reply-To: <cover.1340874806.git.cmahapatra@ti.com>
DISPC functions have been modified to provide clock and register dumps and debug
support for the LCD3 manager.
Signed-off-by: Chandrabhanu Mahapatra <cmahapatra@ti.com>
---
drivers/video/omap2/dss/dispc.c | 70 ++++++++++++++++++++++----------------
1 files changed, 40 insertions(+), 30 deletions(-)
diff --git a/drivers/video/omap2/dss/dispc.c b/drivers/video/omap2/dss/dispc.c
index 644ff53..29b92ea 100644
--- a/drivers/video/omap2/dss/dispc.c
+++ b/drivers/video/omap2/dss/dispc.c
@@ -2854,12 +2854,32 @@ unsigned long dispc_core_clk_rate(void)
return fclk / lcd;
}
-void dispc_dump_clocks(struct seq_file *s)
+void dispc_dump_clocks_channel(struct seq_file *s, enum omap_channel channel)
{
int lcd, pcd;
+ enum omap_dss_clk_source lcd_clk_src;
+
+ seq_printf(s, "- %s -\n", mgr_desc[channel].name);
+
+ lcd_clk_src = dss_get_lcd_clk_source(channel);
+
+ seq_printf(s, "%s clk source = %s (%s)\n", mgr_desc[channel].name,
+ dss_get_generic_clk_source_name(lcd_clk_src),
+ dss_feat_get_clk_source_name(lcd_clk_src));
+
+ dispc_mgr_get_lcd_divisor(channel, &lcd, &pcd);
+
+ seq_printf(s, "lck\t\t%-16lulck div\t%u\n",
+ dispc_mgr_lclk_rate(channel), lcd);
+ seq_printf(s, "pck\t\t%-16lupck div\t%u\n",
+ dispc_mgr_pclk_rate(channel), pcd);
+}
+
+void dispc_dump_clocks(struct seq_file *s)
+{
+ int lcd;
u32 l;
enum omap_dss_clk_source dispc_clk_src = dss_get_dispc_clk_source();
- enum omap_dss_clk_source lcd_clk_src;
if (dispc_runtime_get())
return;
@@ -2880,36 +2900,13 @@ void dispc_dump_clocks(struct seq_file *s)
seq_printf(s, "lck\t\t%-16lulck div\t%u\n",
(dispc_fclk_rate()/lcd), lcd);
}
- seq_printf(s, "- LCD1 -\n");
- lcd_clk_src = dss_get_lcd_clk_source(OMAP_DSS_CHANNEL_LCD);
+ dispc_dump_clocks_channel(s, OMAP_DSS_CHANNEL_LCD);
- seq_printf(s, "lcd1_clk source = %s (%s)\n",
- dss_get_generic_clk_source_name(lcd_clk_src),
- dss_feat_get_clk_source_name(lcd_clk_src));
-
- dispc_mgr_get_lcd_divisor(OMAP_DSS_CHANNEL_LCD, &lcd, &pcd);
-
- seq_printf(s, "lck\t\t%-16lulck div\t%u\n",
- dispc_mgr_lclk_rate(OMAP_DSS_CHANNEL_LCD), lcd);
- seq_printf(s, "pck\t\t%-16lupck div\t%u\n",
- dispc_mgr_pclk_rate(OMAP_DSS_CHANNEL_LCD), pcd);
- if (dss_has_feature(FEAT_MGR_LCD2)) {
- seq_printf(s, "- LCD2 -\n");
-
- lcd_clk_src = dss_get_lcd_clk_source(OMAP_DSS_CHANNEL_LCD2);
-
- seq_printf(s, "lcd2_clk source = %s (%s)\n",
- dss_get_generic_clk_source_name(lcd_clk_src),
- dss_feat_get_clk_source_name(lcd_clk_src));
-
- dispc_mgr_get_lcd_divisor(OMAP_DSS_CHANNEL_LCD2, &lcd, &pcd);
-
- seq_printf(s, "lck\t\t%-16lulck div\t%u\n",
- dispc_mgr_lclk_rate(OMAP_DSS_CHANNEL_LCD2), lcd);
- seq_printf(s, "pck\t\t%-16lupck div\t%u\n",
- dispc_mgr_pclk_rate(OMAP_DSS_CHANNEL_LCD2), pcd);
- }
+ if (dss_has_feature(FEAT_MGR_LCD2))
+ dispc_dump_clocks_channel(s, OMAP_DSS_CHANNEL_LCD2);
+ if (dss_has_feature(FEAT_MGR_LCD3))
+ dispc_dump_clocks_channel(s, OMAP_DSS_CHANNEL_LCD3);
dispc_runtime_put();
}
@@ -2962,6 +2959,12 @@ void dispc_dump_irqs(struct seq_file *s)
PIS(ACBIAS_COUNT_STAT2);
PIS(SYNC_LOST2);
}
+ if (dss_has_feature(FEAT_MGR_LCD3)) {
+ PIS(FRAMEDONE3);
+ PIS(VSYNC3);
+ PIS(ACBIAS_COUNT_STAT3);
+ PIS(SYNC_LOST3);
+ }
#undef PIS
}
#endif
@@ -2973,6 +2976,7 @@ static void dispc_dump_regs(struct seq_file *s)
[OMAP_DSS_CHANNEL_LCD] = "LCD",
[OMAP_DSS_CHANNEL_DIGIT] = "TV",
[OMAP_DSS_CHANNEL_LCD2] = "LCD2",
+ [OMAP_DSS_CHANNEL_LCD3] = "LCD3",
};
const char *ovl_names[] = {
[OMAP_DSS_GFX] = "GFX",
@@ -3005,6 +3009,10 @@ static void dispc_dump_regs(struct seq_file *s)
DUMPREG(DISPC_CONTROL2);
DUMPREG(DISPC_CONFIG2);
}
+ if (dss_has_feature(FEAT_MGR_LCD3)) {
+ DUMPREG(DISPC_CONTROL3);
+ DUMPREG(DISPC_CONFIG3);
+ }
#undef DUMPREG
@@ -3387,6 +3395,8 @@ static void print_irq_status(u32 status)
PIS(SYNC_LOST_DIGIT);
if (dss_has_feature(FEAT_MGR_LCD2))
PIS(SYNC_LOST2);
+ if (dss_has_feature(FEAT_MGR_LCD3))
+ PIS(SYNC_LOST3);
#undef PIS
printk("\n");
--
1.7.1
^ permalink raw reply related
* Re: [PATCH 3/3] OMAPDSS: HDMI: Cache EDID
From: Tomi Valkeinen @ 2012-06-28 10:14 UTC (permalink / raw)
To: Jassi Brar
Cc: mythripk, linux-omap, linux-fbdev, andy.green, n-dechesne,
patches
In-Reply-To: <CAJe_ZhcWSYhRWSW0v9OqsHRXASsQ8i2P8F8OS+jXkyadoRM9iA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 3356 bytes --]
On Thu, 2012-06-28 at 15:18 +0530, Jassi Brar wrote:
> On 28 June 2012 13:18, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> > On Wed, 2012-06-27 at 19:35 +0530, jaswinder.singh@linaro.org wrote:
> >> From: Jassi Brar <jaswinder.singh@linaro.org>
> >>
> >> We can easily keep track of latest EDID from the display and hence avoid
> >> expensive EDID re-reads over I2C.
> >> This could also help some cheapo displays that provide EDID reliably only
> >> immediately after asserting HPD and not later.
> >> Even with good displays, there is something in OMAPDSS that apparantly
> >> messes up DDC occasionally while EDID is being read, giving the
> >> "operation stopped when reading edid" error.
> >
> > Btw, this is in nitpicking area, but what editor do you use? I find it
> > difficult to read text that is not formatted properly =). At least vim
> > formats text nicely with its formating commands.
> >
> Indeed a nitpick :)
> I use vim and, iirc, checkpatch.pl gave 0 warning. Perhaps my poor
> cmoposition. Please do tell how I could I make it more appealing to
> you ?
Like:
---
We can easily keep track of latest EDID from the display and hence avoid
expensive EDID re-reads over I2C. This could also help some cheapo displays
that provide EDID reliably only immediately after asserting HPD and not later.
Even with good displays, there is something in OMAPDSS that apparantly messes
up DDC occasionally while EDID is being read, giving the "operation stopped
when reading edid" error.
---
So basically two things: properly formatted paragraphs and an empty line
between paragraphs. With "properly formatted" I mean that that the text
goes from the beginning of the line to the end, so that the text fills
the whole line. This can be done easily in vim with first painting the
paragraph (or multiple paragraphs), and then then press g and w.
I think those simple rules make the text much easier to read.
> >> --- a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
> >> +++ b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
> >> @@ -243,10 +243,13 @@ static int hdmi_check_hpd_state(struct hdmi_ip_data *ip_data)
> >>
> >> hpd = gpio_get_value(ip_data->hpd_gpio);
> >>
> >> - if (hpd)
> >> + if (hpd) {
> >> r = hdmi_set_phy_pwr(ip_data, HDMI_PHYPWRCMD_TXON);
> >> - else
> >> + } else {
> >> + /* Invalidate EDID Cache */
> >> + ip_data->edid_len = 0;
> >> r = hdmi_set_phy_pwr(ip_data, HDMI_PHYPWRCMD_LDOON);
> >> + }
> >
> > There's a problem with this patch, which leaves a wrong EDID in the
> > cache: if you first have the cable connected and hdmi is enabled, you
> > then turn off the HDMI display device via sysfs, we do not go to
> > hdmi_check_hpd_state at all. The next time hdmi is enabled, we only get
> > the plug-in event, and thus EDID cache is never invalidated.
> >
> If the hdmi cable is not replugged during that period, I don't see why
> would you want the EDID invalidated ?
I wasn't very clear with my comment.
When the display device is disabled, we're not listening to the hpd
interrupt anymore. So when it's disabled, the cable can be replugged and
the monitor changed, and the driver won't know about it. And so it'll
return the old EDID for the new monitor.
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 1/4] OMAPDSS: Cleanup implementation of LCD channels
From: Tomi Valkeinen @ 2012-06-28 10:50 UTC (permalink / raw)
To: Chandrabhanu Mahapatra; +Cc: linux-omap, linux-fbdev
In-Reply-To: <013a8d0a8698b3f971c5963115b03701141a4688.1340874806.git.cmahapatra@ti.com>
[-- Attachment #1: Type: text/plain, Size: 6222 bytes --]
On Thu, 2012-06-28 at 15:10 +0530, Chandrabhanu Mahapatra wrote:
> The current implementation of LCD channels and managers consists of a number of
> if-else construct which has been replaced by a simpler interface. A constant
> structure mgr_desc has been created in Display Controller (DISPC) module. The
> mgr_desc contains for each channel its name, irqs and is initialized one time
> with all registers and their corresponding fields to be written to enable
> various features of Display Subsystem. This structure is later used by various
> functions of DISPC which simplifies the further implementation of LCD channels
> and its corresponding managers.
>
> Signed-off-by: Chandrabhanu Mahapatra <cmahapatra@ti.com>
> ---
> drivers/video/omap2/dss/dispc.c | 233 +++++++++++++++++--------------------
> drivers/video/omap2/dss/dsi.c | 6 +-
> drivers/video/omap2/dss/dss.h | 6 +
> drivers/video/omap2/dss/manager.c | 12 +--
> 4 files changed, 121 insertions(+), 136 deletions(-)
>
> diff --git a/drivers/video/omap2/dss/dispc.c b/drivers/video/omap2/dss/dispc.c
> index 4749ac3..6c16b81 100644
> --- a/drivers/video/omap2/dss/dispc.c
> +++ b/drivers/video/omap2/dss/dispc.c
> @@ -57,6 +57,8 @@
>
> #define DISPC_MAX_NR_ISRS 8
>
> +#define DISPC_MGR_FLD_MAX 9
You could have this in the enum mgr_ref_fields, as a last entry. Then
it'll automatically have the value 9, and will adjust automatically when
we add new fields. And actually "MAX" is not quite right. MAX would be
8, as that's the maximum value for the vields. "NUM" is perhaps more
correct.
> +
> struct omap_dispc_isr_data {
> omap_dispc_isr_t isr;
> void *arg;
> @@ -119,6 +121,78 @@ enum omap_color_component {
> DISPC_COLOR_COMPONENT_UV = 1 << 1,
> };
>
> +enum mgr_reg_fields {
> + DISPC_MGR_FLD_ENABLE,
> + DISPC_MGR_FLD_STNTFT,
> + DISPC_MGR_FLD_GO,
> + DISPC_MGR_FLD_TFTDATALINES,
> + DISPC_MGR_FLD_STALLMODE,
> + DISPC_MGR_FLD_TCKENABLE,
> + DISPC_MGR_FLD_TCKSELECTION,
> + DISPC_MGR_FLD_CPR,
> + DISPC_MGR_FLD_FIFOHANDCHECK,
> +};
> +
> +static const struct {
> + const char *name;
> + u32 vsync_irq;
> + u32 framedone_irq;
> + u32 sync_lost_irq;
> + struct reg_field reg_desc[DISPC_MGR_FLD_MAX];
> +} mgr_desc[] = {
> + [OMAP_DSS_CHANNEL_LCD] = {
> + .name = "LCD",
> + .vsync_irq = DISPC_IRQ_VSYNC,
> + .framedone_irq = DISPC_IRQ_FRAMEDONE,
> + .sync_lost_irq = DISPC_IRQ_SYNC_LOST,
> + .reg_desc = {
> + [DISPC_MGR_FLD_ENABLE] = { DISPC_CONTROL, 0, 0 },
> + [DISPC_MGR_FLD_STNTFT] = { DISPC_CONTROL, 3, 3 },
> + [DISPC_MGR_FLD_GO] = { DISPC_CONTROL, 5, 5 },
> + [DISPC_MGR_FLD_TFTDATALINES] = { DISPC_CONTROL, 9, 8 },
> + [DISPC_MGR_FLD_STALLMODE] = { DISPC_CONTROL, 11, 11 },
> + [DISPC_MGR_FLD_TCKENABLE] = { DISPC_CONFIG, 10, 10 },
> + [DISPC_MGR_FLD_TCKSELECTION] = { DISPC_CONFIG, 11, 11 },
> + [DISPC_MGR_FLD_CPR] = { DISPC_CONFIG, 15, 15 },
> + [DISPC_MGR_FLD_FIFOHANDCHECK] = { DISPC_CONFIG, 16, 16 },
> + },
> + },
> + [OMAP_DSS_CHANNEL_DIGIT] = {
> + .name = "DIGIT",
> + .vsync_irq = DISPC_IRQ_EVSYNC_ODD | DISPC_IRQ_EVSYNC_EVEN,
> + .framedone_irq = DISPC_IRQ_FRAMEDONETV,
There's a problem with this one. FRAMEDONETV is a new thing, appeared in
omap4. So for this we need a system to select the data depending on the
DSS version.
I suggest you remove the framedone_irq entry for now, and keep the old
code to handle the framedone irq. Let's add it later when we can handle
the differences between omap versions.
Or actually, looking at the code, perhaps you can keep the framedone_irq
field, but set it to 0 for DIGIT mgr. This would keep the functionality
the same as it is now, because there's only one place in dispc.c where
we use FRAMEDONETV, and your patch doesn't touch it. In other places we
presume that TV out does not have FRAMEDONE interrupt (i.e. the irq
number is 0).
> + .sync_lost_irq = DISPC_IRQ_SYNC_LOST_DIGIT,
> + .reg_desc = {
> + [DISPC_MGR_FLD_ENABLE] = { DISPC_CONTROL, 1, 1 },
> + [DISPC_MGR_FLD_STNTFT] = { },
> + [DISPC_MGR_FLD_GO] = { DISPC_CONTROL, 6, 6 },
> + [DISPC_MGR_FLD_TFTDATALINES] = { },
> + [DISPC_MGR_FLD_STALLMODE] = { },
> + [DISPC_MGR_FLD_TCKENABLE] = { DISPC_CONFIG, 12, 12 },
> + [DISPC_MGR_FLD_TCKSELECTION] = { DISPC_CONFIG, 13, 13 },
> + [DISPC_MGR_FLD_CPR] = { },
> + [DISPC_MGR_FLD_FIFOHANDCHECK] = { DISPC_CONFIG, 16, 16 },
> + },
> + },
> + [OMAP_DSS_CHANNEL_LCD2] = {
> + .name = "LCD2",
> + .vsync_irq = DISPC_IRQ_VSYNC2,
> + .framedone_irq = DISPC_IRQ_FRAMEDONE2,
> + .sync_lost_irq = DISPC_IRQ_SYNC_LOST2,
> + .reg_desc = {
> + [DISPC_MGR_FLD_ENABLE] = { DISPC_CONTROL2, 0, 0 },
> + [DISPC_MGR_FLD_STNTFT] = { DISPC_CONTROL2, 3, 3 },
> + [DISPC_MGR_FLD_GO] = { DISPC_CONTROL2, 5, 5 },
> + [DISPC_MGR_FLD_TFTDATALINES] = { DISPC_CONTROL2, 9, 8 },
> + [DISPC_MGR_FLD_STALLMODE] = { DISPC_CONTROL2, 11, 11 },
> + [DISPC_MGR_FLD_TCKENABLE] = { DISPC_CONFIG2, 10, 10 },
> + [DISPC_MGR_FLD_TCKSELECTION] = { DISPC_CONFIG2, 11, 11 },
> + [DISPC_MGR_FLD_CPR] = { DISPC_CONFIG2, 15, 15 },
> + [DISPC_MGR_FLD_FIFOHANDCHECK] = { DISPC_CONFIG2, 16, 16 },
> + },
> + },
> +};
> +
> static void _omap_dispc_set_irqs(void);
>
> static inline void dispc_write_reg(const u16 idx, u32 val)
> @@ -131,6 +205,19 @@ static inline u32 dispc_read_reg(const u16 idx)
> return __raw_readl(dispc.base + idx);
> }
>
> +static u32 mgr_fld_read(enum omap_channel channel, enum mgr_reg_fields regfld)
> +{
> + const struct reg_field rfld = mgr_desc[channel].reg_desc[regfld];
> + return FLD_GET(dispc_read_reg(rfld.reg), rfld.high, rfld.low);
This could use REG_GET(), instead of FLD_GET(dispc_read_reg(...))
> +}
> +
> +static void mgr_fld_write(enum omap_channel channel,
> + enum mgr_reg_fields regfld, int val) {
> + const struct reg_field rfld = mgr_desc[channel].reg_desc[regfld];
> + dispc_write_reg(rfld.reg, FLD_MOD(dispc_read_reg(rfld.reg), val,
> + rfld.high, rfld.low));
> +}
And this one could use REG_FLD_MOD().
Otherwise, looks good.
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 3/3] OMAPDSS: HDMI: Cache EDID
From: Jassi Brar @ 2012-06-28 10:59 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: mythripk, linux-omap, linux-fbdev, andy.green, n-dechesne,
patches
In-Reply-To: <1340878461.5037.30.camel@deskari>
On 28 June 2012 15:44, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On Thu, 2012-06-28 at 15:18 +0530, Jassi Brar wrote:
>> >> --- a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
>> >> +++ b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
>> >> @@ -243,10 +243,13 @@ static int hdmi_check_hpd_state(struct hdmi_ip_data *ip_data)
>> >>
>> >> hpd = gpio_get_value(ip_data->hpd_gpio);
>> >>
>> >> - if (hpd)
>> >> + if (hpd) {
>> >> r = hdmi_set_phy_pwr(ip_data, HDMI_PHYPWRCMD_TXON);
>> >> - else
>> >> + } else {
>> >> + /* Invalidate EDID Cache */
>> >> + ip_data->edid_len = 0;
>> >> r = hdmi_set_phy_pwr(ip_data, HDMI_PHYPWRCMD_LDOON);
>> >> + }
>> >
>> > There's a problem with this patch, which leaves a wrong EDID in the
>> > cache: if you first have the cable connected and hdmi is enabled, you
>> > then turn off the HDMI display device via sysfs, we do not go to
>> > hdmi_check_hpd_state at all. The next time hdmi is enabled, we only get
>> > the plug-in event, and thus EDID cache is never invalidated.
>> >
>> If the hdmi cable is not replugged during that period, I don't see why
>> would you want the EDID invalidated ?
>
> I wasn't very clear with my comment.
>
> When the display device is disabled, we're not listening to the hpd
> interrupt anymore. So when it's disabled, the cable can be replugged and
> the monitor changed, and the driver won't know about it. And so it'll
> return the old EDID for the new monitor.
>
If that could be a problem, then we already have some problem with
current omapdss.
I think among the first things, while enabling HDMI, should be to see
if there is really some display connected on the port i.e, HPD
asserted. Only if ti_hdmi_4xxx_detect() returned true, should we
proceed otherwise wait for HPQ irq.
Unconditionally invalidating edid really seems like a regression - we
impose atleast 50ms (edid read) as extra cost on
hdmi_check_hpd_state(), which kills half the purpose of this patch.
^ permalink raw reply
* Re: [PATCH 3/3] OMAPDSS: HDMI: Cache EDID
From: Tomi Valkeinen @ 2012-06-28 11:04 UTC (permalink / raw)
To: Jassi Brar
Cc: mythripk, linux-omap, linux-fbdev, andy.green, n-dechesne,
patches
In-Reply-To: <CAJe_Zhc_ye1f=hOVd=AJ7rmCQ_vwDtjQOKsufPYmV1o-58jyAA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1732 bytes --]
On Thu, 2012-06-28 at 16:17 +0530, Jassi Brar wrote:
> On 28 June 2012 15:44, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> > When the display device is disabled, we're not listening to the hpd
> > interrupt anymore. So when it's disabled, the cable can be replugged and
> > the monitor changed, and the driver won't know about it. And so it'll
> > return the old EDID for the new monitor.
> >
> If that could be a problem, then we already have some problem with
> current omapdss.
>
> I think among the first things, while enabling HDMI, should be to see
> if there is really some display connected on the port i.e, HPD
> asserted. Only if ti_hdmi_4xxx_detect() returned true, should we
> proceed otherwise wait for HPQ irq.
I'm not sure I understand. The HDMI driver does just that. It calls
hdmi_check_hpd_state when it's being enabled to see if there's a display
connected.
> Unconditionally invalidating edid really seems like a regression - we
> impose atleast 50ms (edid read) as extra cost on
> hdmi_check_hpd_state(), which kills half the purpose of this patch.
If the HDMI hardware is turned off, we should unconditionally invalidate
the EDID. We have no way to keep track if the same monitor is kept
plugged in or not.
So what exactly is the purpose of this patch, what kind of scenario? I
thought it's to cache the EDID, so that if it will be read multiple
times while the same monitor is connected, we'll just return the cached
value.
Of course, I don't know why the upper layers would read the EDID
multiple times, because I think they should read it only once. So
perhaps I'm either not understanding something, or it's the omapdrm
layer that should be fixed?
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 3/3] OMAPDSS: HDMI: Cache EDID
From: Tomi Valkeinen @ 2012-06-28 11:10 UTC (permalink / raw)
To: Jassi Brar
Cc: mythripk, linux-omap, linux-fbdev, andy.green, n-dechesne,
patches
In-Reply-To: <CAJe_ZhfehDquDcyefb7Z9odH+hfQTFiRAux_iyvAycs9Otovtw@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 3118 bytes --]
On Thu, 2012-06-28 at 16:28 +0530, Jassi Brar wrote:
> On 28 June 2012 16:17, Jassi Brar <jaswinder.singh@linaro.org> wrote:
> > On 28 June 2012 15:44, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> >> On Thu, 2012-06-28 at 15:18 +0530, Jassi Brar wrote:
> >
> >>> >> --- a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
> >>> >> +++ b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
> >>> >> @@ -243,10 +243,13 @@ static int hdmi_check_hpd_state(struct hdmi_ip_data *ip_data)
> >>> >>
> >>> >> hpd = gpio_get_value(ip_data->hpd_gpio);
> >>> >>
> >>> >> - if (hpd)
> >>> >> + if (hpd) {
> >>> >> r = hdmi_set_phy_pwr(ip_data, HDMI_PHYPWRCMD_TXON);
> >>> >> - else
> >>> >> + } else {
> >>> >> + /* Invalidate EDID Cache */
> >>> >> + ip_data->edid_len = 0;
> >>> >> r = hdmi_set_phy_pwr(ip_data, HDMI_PHYPWRCMD_LDOON);
> >>> >> + }
> >>> >
> >>> > There's a problem with this patch, which leaves a wrong EDID in the
> >>> > cache: if you first have the cable connected and hdmi is enabled, you
> >>> > then turn off the HDMI display device via sysfs, we do not go to
> >>> > hdmi_check_hpd_state at all. The next time hdmi is enabled, we only get
> >>> > the plug-in event, and thus EDID cache is never invalidated.
> >>> >
> >>> If the hdmi cable is not replugged during that period, I don't see why
> >>> would you want the EDID invalidated ?
> >>
> >> I wasn't very clear with my comment.
> >>
> >> When the display device is disabled, we're not listening to the hpd
> >> interrupt anymore. So when it's disabled, the cable can be replugged and
> >> the monitor changed, and the driver won't know about it. And so it'll
> >> return the old EDID for the new monitor.
> >>
> > If that could be a problem, then we already have some problem with
> > current omapdss.
> >
> > I think among the first things, while enabling HDMI, should be to see
> > if there is really some display connected on the port i.e, HPD
> > asserted. Only if ti_hdmi_4_detect() returned true, should we
> > proceed otherwise wait for HPQ irq.
> >
> > Unconditionally invalidating edid really seems like a regression - we
> > impose atleast 50ms (edid read) as extra cost on
> > hdmi_check_hpd_state(), which kills half the purpose of this patch.
> >
> Sorry a correction. Reading detect() won't work. I suggest we keep HPD
> IRQ enabled for the lifetime of the driver.
Ok, I see. But that's not acceptable. It would require us to keep the
TPD12S015 always powered and enabled. Even if you're not interested in
using HDMI at all.
For this to work like you want we need a bigger restructuring of HDMI
and partly omapdss also. Currently we have just one big "enabled" or
"disabled" state. We need multiple states. Probably something like:
- disabled, everything totally off
- low power, hotplug detection enabled
- powered on, but no video
- video enabled
Been long in my mind, but I'm not very familiar with HDMI so I could get
my simple prototypes to work when I tried something like this once.
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 3/3] OMAPDSS: HDMI: Cache EDID
From: Jassi Brar @ 2012-06-28 11:10 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: mythripk, linux-omap, linux-fbdev, andy.green, n-dechesne,
patches
In-Reply-To: <CAJe_Zhc_ye1f=hOVd=AJ7rmCQ_vwDtjQOKsufPYmV1o-58jyAA@mail.gmail.com>
On 28 June 2012 16:17, Jassi Brar <jaswinder.singh@linaro.org> wrote:
> On 28 June 2012 15:44, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>> On Thu, 2012-06-28 at 15:18 +0530, Jassi Brar wrote:
>
>>> >> --- a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
>>> >> +++ b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
>>> >> @@ -243,10 +243,13 @@ static int hdmi_check_hpd_state(struct hdmi_ip_data *ip_data)
>>> >>
>>> >> hpd = gpio_get_value(ip_data->hpd_gpio);
>>> >>
>>> >> - if (hpd)
>>> >> + if (hpd) {
>>> >> r = hdmi_set_phy_pwr(ip_data, HDMI_PHYPWRCMD_TXON);
>>> >> - else
>>> >> + } else {
>>> >> + /* Invalidate EDID Cache */
>>> >> + ip_data->edid_len = 0;
>>> >> r = hdmi_set_phy_pwr(ip_data, HDMI_PHYPWRCMD_LDOON);
>>> >> + }
>>> >
>>> > There's a problem with this patch, which leaves a wrong EDID in the
>>> > cache: if you first have the cable connected and hdmi is enabled, you
>>> > then turn off the HDMI display device via sysfs, we do not go to
>>> > hdmi_check_hpd_state at all. The next time hdmi is enabled, we only get
>>> > the plug-in event, and thus EDID cache is never invalidated.
>>> >
>>> If the hdmi cable is not replugged during that period, I don't see why
>>> would you want the EDID invalidated ?
>>
>> I wasn't very clear with my comment.
>>
>> When the display device is disabled, we're not listening to the hpd
>> interrupt anymore. So when it's disabled, the cable can be replugged and
>> the monitor changed, and the driver won't know about it. And so it'll
>> return the old EDID for the new monitor.
>>
> If that could be a problem, then we already have some problem with
> current omapdss.
>
> I think among the first things, while enabling HDMI, should be to see
> if there is really some display connected on the port i.e, HPD
> asserted. Only if ti_hdmi_4_detect() returned true, should we
> proceed otherwise wait for HPQ irq.
>
> Unconditionally invalidating edid really seems like a regression - we
> impose atleast 50ms (edid read) as extra cost on
> hdmi_check_hpd_state(), which kills half the purpose of this patch.
>
Sorry a correction. Reading detect() won't work. I suggest we keep HPD
IRQ enabled for the lifetime of the driver.
^ permalink raw reply
* Re: [PATCH 1/4] OMAPDSS: Cleanup implementation of LCD channels
From: Tomi Valkeinen @ 2012-06-28 11:27 UTC (permalink / raw)
To: Chandrabhanu Mahapatra; +Cc: linux-omap, linux-fbdev
In-Reply-To: <4FEC3DA9.9010405@ti.com>
[-- Attachment #1: Type: text/plain, Size: 1912 bytes --]
On Thu, 2012-06-28 at 16:49 +0530, Chandrabhanu Mahapatra wrote:
> On Thursday 28 June 2012 04:20 PM, Tomi Valkeinen wrote:
> > On Thu, 2012-06-28 at 15:10 +0530, Chandrabhanu Mahapatra wrote:
> >> + [OMAP_DSS_CHANNEL_DIGIT] = {
> >> + .name = "DIGIT",
> >> + .vsync_irq = DISPC_IRQ_EVSYNC_ODD | DISPC_IRQ_EVSYNC_EVEN,
> >> + .framedone_irq = DISPC_IRQ_FRAMEDONETV,
> > There's a problem with this one. FRAMEDONETV is a new thing, appeared in
> > omap4. So for this we need a system to select the data depending on the
> > DSS version.
> >
> > I suggest you remove the framedone_irq entry for now, and keep the old
> > code to handle the framedone irq. Let's add it later when we can handle
> > the differences between omap versions.
> >
> > Or actually, looking at the code, perhaps you can keep the framedone_irq
> > field, but set it to 0 for DIGIT mgr. This would keep the functionality
> > the same as it is now, because there's only one place in dispc.c where
> > we use FRAMEDONETV, and your patch doesn't touch it. In other places we
> > presume that TV out does not have FRAMEDONE interrupt (i.e. the irq
> > number is 0).
> The purpose of FRAMEDONETV IRQ as seen from dispc_mgr_enable_digit_out()
> looks as to interrupt when active frame related to HDMI is done and so
> DISPC is disabled. I think I misinterpreted and used it here. Can
> please explain the exact purpose of DISPC_IRQ_FRAMEDONETV?
Yes, FRAMEDONE, for both LCD and TV outputs tells us that the DISPC
output has finished sending the last frame, when the output is being
disabled. So your use here is correct, except that the interrupt is not
present on omap2/3. Thus we cannot have it in this common table.
If you set the field to 0 in this table, everything should work as
before. If you look at the dispc_mgr_get_framedone_irq() code, you see
that it returns 0 for DIGIT channel.
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 1/4] OMAPDSS: Cleanup implementation of LCD channels
From: Chandrabhanu Mahapatra @ 2012-06-28 11:31 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: linux-omap, linux-fbdev
In-Reply-To: <1340880649.5037.39.camel@deskari>
On Thursday 28 June 2012 04:20 PM, Tomi Valkeinen wrote:
> On Thu, 2012-06-28 at 15:10 +0530, Chandrabhanu Mahapatra wrote:
>> The current implementation of LCD channels and managers consists of a number of
>> if-else construct which has been replaced by a simpler interface. A constant
>> structure mgr_desc has been created in Display Controller (DISPC) module. The
>> mgr_desc contains for each channel its name, irqs and is initialized one time
>> with all registers and their corresponding fields to be written to enable
>> various features of Display Subsystem. This structure is later used by various
>> functions of DISPC which simplifies the further implementation of LCD channels
>> and its corresponding managers.
>>
>> Signed-off-by: Chandrabhanu Mahapatra <cmahapatra@ti.com>
>> ---
>> drivers/video/omap2/dss/dispc.c | 233 +++++++++++++++++--------------------
>> drivers/video/omap2/dss/dsi.c | 6 +-
>> drivers/video/omap2/dss/dss.h | 6 +
>> drivers/video/omap2/dss/manager.c | 12 +--
>> 4 files changed, 121 insertions(+), 136 deletions(-)
>>
>> diff --git a/drivers/video/omap2/dss/dispc.c b/drivers/video/omap2/dss/dispc.c
>> index 4749ac3..6c16b81 100644
>> --- a/drivers/video/omap2/dss/dispc.c
>> +++ b/drivers/video/omap2/dss/dispc.c
>> @@ -57,6 +57,8 @@
>>
>> #define DISPC_MAX_NR_ISRS 8
>>
>> +#define DISPC_MGR_FLD_MAX 9
> You could have this in the enum mgr_ref_fields, as a last entry. Then
> it'll automatically have the value 9, and will adjust automatically when
> we add new fields. And actually "MAX" is not quite right. MAX would be
> 8, as that's the maximum value for the vields. "NUM" is perhaps more
> correct.
Its really a clever idea to have it as the last field of enum
mgr_ref_fields. To make it distinct from other fields I can add a
comment on top of it saying its for count of above fields or I am fine
with names as DISPC_MGR_FLD_COUNT / NUM.
>
>> +
>> struct omap_dispc_isr_data {
>> omap_dispc_isr_t isr;
>> void *arg;
>> @@ -119,6 +121,78 @@ enum omap_color_component {
>> DISPC_COLOR_COMPONENT_UV = 1 << 1,
>> };
>>
>> +enum mgr_reg_fields {
>> + DISPC_MGR_FLD_ENABLE,
>> + DISPC_MGR_FLD_STNTFT,
>> + DISPC_MGR_FLD_GO,
>> + DISPC_MGR_FLD_TFTDATALINES,
>> + DISPC_MGR_FLD_STALLMODE,
>> + DISPC_MGR_FLD_TCKENABLE,
>> + DISPC_MGR_FLD_TCKSELECTION,
>> + DISPC_MGR_FLD_CPR,
>> + DISPC_MGR_FLD_FIFOHANDCHECK,
>> +};
>> +
>> +static const struct {
>> + const char *name;
>> + u32 vsync_irq;
>> + u32 framedone_irq;
>> + u32 sync_lost_irq;
>> + struct reg_field reg_desc[DISPC_MGR_FLD_MAX];
>> +} mgr_desc[] = {
>> + [OMAP_DSS_CHANNEL_LCD] = {
>> + .name = "LCD",
>> + .vsync_irq = DISPC_IRQ_VSYNC,
>> + .framedone_irq = DISPC_IRQ_FRAMEDONE,
>> + .sync_lost_irq = DISPC_IRQ_SYNC_LOST,
>> + .reg_desc = {
>> + [DISPC_MGR_FLD_ENABLE] = { DISPC_CONTROL, 0, 0 },
>> + [DISPC_MGR_FLD_STNTFT] = { DISPC_CONTROL, 3, 3 },
>> + [DISPC_MGR_FLD_GO] = { DISPC_CONTROL, 5, 5 },
>> + [DISPC_MGR_FLD_TFTDATALINES] = { DISPC_CONTROL, 9, 8 },
>> + [DISPC_MGR_FLD_STALLMODE] = { DISPC_CONTROL, 11, 11 },
>> + [DISPC_MGR_FLD_TCKENABLE] = { DISPC_CONFIG, 10, 10 },
>> + [DISPC_MGR_FLD_TCKSELECTION] = { DISPC_CONFIG, 11, 11 },
>> + [DISPC_MGR_FLD_CPR] = { DISPC_CONFIG, 15, 15 },
>> + [DISPC_MGR_FLD_FIFOHANDCHECK] = { DISPC_CONFIG, 16, 16 },
>> + },
>> + },
>> + [OMAP_DSS_CHANNEL_DIGIT] = {
>> + .name = "DIGIT",
>> + .vsync_irq = DISPC_IRQ_EVSYNC_ODD | DISPC_IRQ_EVSYNC_EVEN,
>> + .framedone_irq = DISPC_IRQ_FRAMEDONETV,
> There's a problem with this one. FRAMEDONETV is a new thing, appeared in
> omap4. So for this we need a system to select the data depending on the
> DSS version.
>
> I suggest you remove the framedone_irq entry for now, and keep the old
> code to handle the framedone irq. Let's add it later when we can handle
> the differences between omap versions.
>
> Or actually, looking at the code, perhaps you can keep the framedone_irq
> field, but set it to 0 for DIGIT mgr. This would keep the functionality
> the same as it is now, because there's only one place in dispc.c where
> we use FRAMEDONETV, and your patch doesn't touch it. In other places we
> presume that TV out does not have FRAMEDONE interrupt (i.e. the irq
> number is 0).
The purpose of FRAMEDONETV IRQ as seen from dispc_mgr_enable_digit_out()
looks as to interrupt when active frame related to HDMI is done and so
DISPC is disabled. I think I misinterpreted and used it here. Can
please explain the exact purpose of DISPC_IRQ_FRAMEDONETV?
>> + .sync_lost_irq = DISPC_IRQ_SYNC_LOST_DIGIT,
>> + .reg_desc = {
>> + [DISPC_MGR_FLD_ENABLE] = { DISPC_CONTROL, 1, 1 },
>> + [DISPC_MGR_FLD_STNTFT] = { },
>> + [DISPC_MGR_FLD_GO] = { DISPC_CONTROL, 6, 6 },
>> + [DISPC_MGR_FLD_TFTDATALINES] = { },
>> + [DISPC_MGR_FLD_STALLMODE] = { },
>> + [DISPC_MGR_FLD_TCKENABLE] = { DISPC_CONFIG, 12, 12 },
>> + [DISPC_MGR_FLD_TCKSELECTION] = { DISPC_CONFIG, 13, 13 },
>> + [DISPC_MGR_FLD_CPR] = { },
>> + [DISPC_MGR_FLD_FIFOHANDCHECK] = { DISPC_CONFIG, 16, 16 },
>> + },
>> + },
>> + [OMAP_DSS_CHANNEL_LCD2] = {
>> + .name = "LCD2",
>> + .vsync_irq = DISPC_IRQ_VSYNC2,
>> + .framedone_irq = DISPC_IRQ_FRAMEDONE2,
>> + .sync_lost_irq = DISPC_IRQ_SYNC_LOST2,
>> + .reg_desc = {
>> + [DISPC_MGR_FLD_ENABLE] = { DISPC_CONTROL2, 0, 0 },
>> + [DISPC_MGR_FLD_STNTFT] = { DISPC_CONTROL2, 3, 3 },
>> + [DISPC_MGR_FLD_GO] = { DISPC_CONTROL2, 5, 5 },
>> + [DISPC_MGR_FLD_TFTDATALINES] = { DISPC_CONTROL2, 9, 8 },
>> + [DISPC_MGR_FLD_STALLMODE] = { DISPC_CONTROL2, 11, 11 },
>> + [DISPC_MGR_FLD_TCKENABLE] = { DISPC_CONFIG2, 10, 10 },
>> + [DISPC_MGR_FLD_TCKSELECTION] = { DISPC_CONFIG2, 11, 11 },
>> + [DISPC_MGR_FLD_CPR] = { DISPC_CONFIG2, 15, 15 },
>> + [DISPC_MGR_FLD_FIFOHANDCHECK] = { DISPC_CONFIG2, 16, 16 },
>> + },
>> + },
>> +};
>> +
>> static void _omap_dispc_set_irqs(void);
>>
>> static inline void dispc_write_reg(const u16 idx, u32 val)
>> @@ -131,6 +205,19 @@ static inline u32 dispc_read_reg(const u16 idx)
>> return __raw_readl(dispc.base + idx);
>> }
>>
>> +static u32 mgr_fld_read(enum omap_channel channel, enum mgr_reg_fields regfld)
>> +{
>> + const struct reg_field rfld = mgr_desc[channel].reg_desc[regfld];
>> + return FLD_GET(dispc_read_reg(rfld.reg), rfld.high, rfld.low);
> This could use REG_GET(), instead of FLD_GET(dispc_read_reg(...))
ok.
>
>> +}
>> +
>> +static void mgr_fld_write(enum omap_channel channel,
>> + enum mgr_reg_fields regfld, int val) {
>> + const struct reg_field rfld = mgr_desc[channel].reg_desc[regfld];
>> + dispc_write_reg(rfld.reg, FLD_MOD(dispc_read_reg(rfld.reg), val,
>> + rfld.high, rfld.low));
>> +}
> And this one could use REG_FLD_MOD().
>
> Otherwise, looks good.
>
> Tomi
>
ok.
--
Chandrabhanu Mahapatra
Texas Instruments India Pvt. Ltd.
^ permalink raw reply
* Re: [PATCH 3/3] OMAPDSS: HDMI: Cache EDID
From: Tomi Valkeinen @ 2012-06-28 11:38 UTC (permalink / raw)
To: Jassi Brar
Cc: mythripk, linux-omap, linux-fbdev, andy.green, n-dechesne,
patches
In-Reply-To: <1340881815.5037.53.camel@deskari>
[-- Attachment #1: Type: text/plain, Size: 921 bytes --]
On Thu, 2012-06-28 at 14:10 +0300, Tomi Valkeinen wrote:
> On Thu, 2012-06-28 at 16:28 +0530, Jassi Brar wrote:
> > Sorry a correction. Reading detect() won't work. I suggest we keep HPD
> > IRQ enabled for the lifetime of the driver.
>
> Ok, I see. But that's not acceptable. It would require us to keep the
> TPD12S015 always powered and enabled. Even if you're not interested in
> using HDMI at all.
Btw, a bigger problem that I see is how we have to do read_edid() (and
detect(), if I recall correctly): we enable the whole video pipeline and
output. We should only enable enough of the HW to be able to read the
EDID or read the HPD GPIO.
I've noticed that this leads to sync losts quite easily, as we switch
the hdmi output on and off quickly multiple times. I couldn't figure out
why the sync losts happen though, and I did try quite many different
combinations how to handle it.
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 3/3] OMAPDSS: HDMI: Cache EDID
From: Andy Green @ 2012-06-28 12:03 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Jassi Brar, mythripk, linux-omap, linux-fbdev, n-dechesne,
patches
In-Reply-To: <1340881815.5037.53.camel@deskari>
On 06/28/12 19:10, the mail apparently from Tomi Valkeinen included:
> On Thu, 2012-06-28 at 16:28 +0530, Jassi Brar wrote:
>> On 28 June 2012 16:17, Jassi Brar <jaswinder.singh@linaro.org> wrote:
>>> On 28 June 2012 15:44, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>>>> On Thu, 2012-06-28 at 15:18 +0530, Jassi Brar wrote:
>>>
>>>>>>> --- a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
>>>>>>> +++ b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
>>>>>>> @@ -243,10 +243,13 @@ static int hdmi_check_hpd_state(struct hdmi_ip_data *ip_data)
>>>>>>>
>>>>>>> hpd = gpio_get_value(ip_data->hpd_gpio);
>>>>>>>
>>>>>>> - if (hpd)
>>>>>>> + if (hpd) {
>>>>>>> r = hdmi_set_phy_pwr(ip_data, HDMI_PHYPWRCMD_TXON);
>>>>>>> - else
>>>>>>> + } else {
>>>>>>> + /* Invalidate EDID Cache */
>>>>>>> + ip_data->edid_len = 0;
>>>>>>> r = hdmi_set_phy_pwr(ip_data, HDMI_PHYPWRCMD_LDOON);
>>>>>>> + }
>>>>>>
>>>>>> There's a problem with this patch, which leaves a wrong EDID in the
>>>>>> cache: if you first have the cable connected and hdmi is enabled, you
>>>>>> then turn off the HDMI display device via sysfs, we do not go to
>>>>>> hdmi_check_hpd_state at all. The next time hdmi is enabled, we only get
>>>>>> the plug-in event, and thus EDID cache is never invalidated.
>>>>>>
>>>>> If the hdmi cable is not replugged during that period, I don't see why
>>>>> would you want the EDID invalidated ?
>>>>
>>>> I wasn't very clear with my comment.
>>>>
>>>> When the display device is disabled, we're not listening to the hpd
>>>> interrupt anymore. So when it's disabled, the cable can be replugged and
>>>> the monitor changed, and the driver won't know about it. And so it'll
>>>> return the old EDID for the new monitor.
>>>>
>>> If that could be a problem, then we already have some problem with
>>> current omapdss.
>>>
>>> I think among the first things, while enabling HDMI, should be to see
>>> if there is really some display connected on the port i.e, HPD
>>> asserted. Only if ti_hdmi_4_detect() returned true, should we
>>> proceed otherwise wait for HPQ irq.
>>>
>>> Unconditionally invalidating edid really seems like a regression - we
>>> impose atleast 50ms (edid read) as extra cost on
>>> hdmi_check_hpd_state(), which kills half the purpose of this patch.
>>>
>> Sorry a correction. Reading detect() won't work. I suggest we keep HPD
>> IRQ enabled for the lifetime of the driver.
>
> Ok, I see. But that's not acceptable. It would require us to keep the
> TPD12S015 always powered and enabled. Even if you're not interested in
> using HDMI at all.
>
> For this to work like you want we need a bigger restructuring of HDMI
> and partly omapdss also. Currently we have just one big "enabled" or
> "disabled" state. We need multiple states. Probably something like:
>
> - disabled, everything totally off
> - low power, hotplug detection enabled
> - powered on, but no video
> - video enabled
>
> Been long in my mind, but I'm not very familiar with HDMI so I could get
> my simple prototypes to work when I tried something like this once.
That doesn't sound too hard since the difference between the first three
states at the HDMI chip is just whether the two gpio controlling it are
00, 10 or 11.
If Jassi's alright with it we might have a go at implementing this, but
can you define a bit more about how we logically tell DSS that we want
to, eg, disable HDMI totally?
-Andy
--
Andy Green | TI Landing Team Leader
Linaro.org │ Open source software for ARM SoCs | Follow Linaro
http://facebook.com/pages/Linaro/155974581091106 -
http://twitter.com/#!/linaroorg - http://linaro.org/linaro-blog
^ permalink raw reply
* Re: [PATCH 3/3] OMAPDSS: HDMI: Cache EDID
From: Andy Green @ 2012-06-28 12:15 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Jassi Brar, mythripk, linux-omap, linux-fbdev, n-dechesne,
patches
In-Reply-To: <1340883482.5037.56.camel@deskari>
On 06/28/12 19:38, the mail apparently from Tomi Valkeinen included:
> On Thu, 2012-06-28 at 14:10 +0300, Tomi Valkeinen wrote:
>> On Thu, 2012-06-28 at 16:28 +0530, Jassi Brar wrote:
>
>>> Sorry a correction. Reading detect() won't work. I suggest we keep HPD
>>> IRQ enabled for the lifetime of the driver.
>>
>> Ok, I see. But that's not acceptable. It would require us to keep the
>> TPD12S015 always powered and enabled. Even if you're not interested in
>> using HDMI at all.
>
> Btw, a bigger problem that I see is how we have to do read_edid() (and
> detect(), if I recall correctly): we enable the whole video pipeline and
> output. We should only enable enough of the HW to be able to read the
> EDID or read the HPD GPIO.
>
> I've noticed that this leads to sync losts quite easily, as we switch
> the hdmi output on and off quickly multiple times. I couldn't figure out
> why the sync losts happen though, and I did try quite many different
> combinations how to handle it.
SYNC LOST is one evil lurking in there, the other evil is EDID fetch
"operation stopped" error. We were unable to figure out what was
trampling on what there without the SoC HDMI PHY IP data which we don't
have.
Also at the moment I think we depower / repower the internal SoC and
external PHY chip more than we need. Each time we remove the HDMI link
power, after a short time where the big capacitor at the charge pump
output decays enough (a time dependent on exact details of load
presented by the TV or monitor...), hpd from the monitor goes low and
remains there until we power the charge pump again. In turn the new hpd
recognition provokes a new edid fetch. Something about that sequence
provokes the "operation stopped" on EDID fetch, with Jassi's patches we
manage to avoid it.
Removing that syndrome, and just not enabling and disabling stuff like
SoC HDMI PHY willy nilly can maybe be something else targeted by this
proposed 4-state power scheme.
-Andy
--
Andy Green | TI Landing Team Leader
Linaro.org │ Open source software for ARM SoCs | Follow Linaro
http://facebook.com/pages/Linaro/155974581091106 -
http://twitter.com/#!/linaroorg - http://linaro.org/linaro-blog
^ permalink raw reply
* Re: [PATCH 3/3] OMAPDSS: HDMI: Cache EDID
From: Jassi Brar @ 2012-06-28 12:43 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: mythripk, linux-omap, linux-fbdev, andy.green, n-dechesne,
patches
In-Reply-To: <1340881815.5037.53.camel@deskari>
On 28 June 2012 16:40, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On Thu, 2012-06-28 at 16:28 +0530, Jassi Brar wrote:
> >
>> Sorry a correction. Reading detect() won't work. I suggest we keep HPD
>> IRQ enabled for the lifetime of the driver.
>
> Ok, I see. But that's not acceptable. It would require us to keep the
> TPD12S015 always powered and enabled. Even if you're not interested in
> using HDMI at all.
>
I think we need to differentiate between HDMI PHY enable and HDMI
5V+,HPD enable [1]... currently they are clubbed together in
omap_dss_device.platform_enable. AFAIK, at least with TPD12S015, they
can be controlled independently and PHY enabling is actually the main
source of power consumption if no display is connected.
By 'lifetime' I mean when the end-user selects some option to the
effect of "Automatically detect and configure display over HDMI" ....
and then we simply enable the HDMI 5V+/HPD, HDMI-PHY would be
enabled only when we actually detect HPD asserted. If a device doesn't
have a port or the user doesn't have a display, neither would be ever
enabled. I mean we should provide a way to make it platform dependent.
[1] Thanks to Andy and his crappy TV, he found clubbing enabling PHY
with 5V+ application comes in the way of detecting cheapo displays
that take ~700ms before asserting HPD i.e, making EDID available. See
how we don't leave it to a HDMI display to take it's own time before
asserting HPD - omapdss_hdmi_display_enable/disable pairs don't care
for that.
^ permalink raw reply
* Re: [PATCH 3/3] OMAPDSS: HDMI: Cache EDID
From: Tomi Valkeinen @ 2012-06-28 13:08 UTC (permalink / raw)
To: Andy Green
Cc: Jassi Brar, mythripk, linux-omap, linux-fbdev, n-dechesne,
patches
In-Reply-To: <4FEC47FA.1040801@linaro.org>
[-- Attachment #1: Type: text/plain, Size: 2000 bytes --]
On Thu, 2012-06-28 at 20:03 +0800, Andy Green wrote:
> On 06/28/12 19:10, the mail apparently from Tomi Valkeinen included:
> > For this to work like you want we need a bigger restructuring of HDMI
> > and partly omapdss also. Currently we have just one big "enabled" or
> > "disabled" state. We need multiple states. Probably something like:
> >
> > - disabled, everything totally off
> > - low power, hotplug detection enabled
> > - powered on, but no video
> > - video enabled
> >
> > Been long in my mind, but I'm not very familiar with HDMI so I could get
> > my simple prototypes to work when I tried something like this once.
>
> That doesn't sound too hard since the difference between the first three
> states at the HDMI chip is just whether the two gpio controlling it are
> 00, 10 or 11.
I don't think it's that simple. We should be able to do EDID read on one
of the states, perhaps second or third. For that we need parts of the
HDMI IP to be enabled.
Also, the third state should be something where the IP is fully
functional. For DSI this would mean that you can communicate over DSI
bus etc. So I guess EDID read should be possible at this level.
> If Jassi's alright with it we might have a go at implementing this, but
> can you define a bit more about how we logically tell DSS that we want
> to, eg, disable HDMI totally?
Disabling HDMI is easy, it's already done by the disable call. And the
same for the video enabled mode. The middle ones are the ones that need
implementation.
And for HDMI, there's currently no way to enable it partially. This is
what I tried with my quick hack, but I couldn't figure out what parts
need to be enabled (but I didn't spend much time on it).
This is something that should be implemented for all outputs, obviously,
but we could approach it bit by bit. For example, implement it first for
HDMI, and all other outputs just consider anything else than "disabled"
as full enable.
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* [PATCH 1/4] OMAPDSS: Cleanup implementation of LCD channels
From: Chandrabhanu Mahapatra @ 2012-06-28 13:14 UTC (permalink / raw)
To: tomi.valkeinen; +Cc: linux-omap, linux-fbdev, Chandrabhanu Mahapatra
In-Reply-To: <013a8d0a8698b3f971c5963115b03701141a4688.1340874806.git.cmahapatra@ti.com>
The current implementation of LCD channels and managers consists of a number of
if-else construct which has been replaced by a simpler interface. A constant
structure mgr_desc has been created in Display Controller (DISPC) module. The
mgr_desc contains for each channel its name, irqs and is initialized one time
with all registers and their corresponding fields to be written to enable
various features of Display Subsystem. This structure is later used by various
functions of DISPC which simplifies the further implementation of LCD channels
and its corresponding managers.
Signed-off-by: Chandrabhanu Mahapatra <cmahapatra@ti.com>
---
drivers/video/omap2/dss/dispc.c | 232 +++++++++++++++++--------------------
drivers/video/omap2/dss/dsi.c | 6 +-
drivers/video/omap2/dss/dss.h | 6 +
drivers/video/omap2/dss/manager.c | 12 +--
4 files changed, 120 insertions(+), 136 deletions(-)
diff --git a/drivers/video/omap2/dss/dispc.c b/drivers/video/omap2/dss/dispc.c
index 4749ac3..8c39371 100644
--- a/drivers/video/omap2/dss/dispc.c
+++ b/drivers/video/omap2/dss/dispc.c
@@ -119,6 +119,80 @@ enum omap_color_component {
DISPC_COLOR_COMPONENT_UV = 1 << 1,
};
+enum mgr_reg_fields {
+ DISPC_MGR_FLD_ENABLE,
+ DISPC_MGR_FLD_STNTFT,
+ DISPC_MGR_FLD_GO,
+ DISPC_MGR_FLD_TFTDATALINES,
+ DISPC_MGR_FLD_STALLMODE,
+ DISPC_MGR_FLD_TCKENABLE,
+ DISPC_MGR_FLD_TCKSELECTION,
+ DISPC_MGR_FLD_CPR,
+ DISPC_MGR_FLD_FIFOHANDCHECK,
+ /* used to maintain a count of the above fields */
+ DISPC_MGR_FLD_NUM,
+};
+
+static const struct {
+ const char *name;
+ u32 vsync_irq;
+ u32 framedone_irq;
+ u32 sync_lost_irq;
+ struct reg_field reg_desc[DISPC_MGR_FLD_NUM];
+} mgr_desc[] = {
+ [OMAP_DSS_CHANNEL_LCD] = {
+ .name = "LCD",
+ .vsync_irq = DISPC_IRQ_VSYNC,
+ .framedone_irq = DISPC_IRQ_FRAMEDONE,
+ .sync_lost_irq = DISPC_IRQ_SYNC_LOST,
+ .reg_desc = {
+ [DISPC_MGR_FLD_ENABLE] = { DISPC_CONTROL, 0, 0 },
+ [DISPC_MGR_FLD_STNTFT] = { DISPC_CONTROL, 3, 3 },
+ [DISPC_MGR_FLD_GO] = { DISPC_CONTROL, 5, 5 },
+ [DISPC_MGR_FLD_TFTDATALINES] = { DISPC_CONTROL, 9, 8 },
+ [DISPC_MGR_FLD_STALLMODE] = { DISPC_CONTROL, 11, 11 },
+ [DISPC_MGR_FLD_TCKENABLE] = { DISPC_CONFIG, 10, 10 },
+ [DISPC_MGR_FLD_TCKSELECTION] = { DISPC_CONFIG, 11, 11 },
+ [DISPC_MGR_FLD_CPR] = { DISPC_CONFIG, 15, 15 },
+ [DISPC_MGR_FLD_FIFOHANDCHECK] = { DISPC_CONFIG, 16, 16 },
+ },
+ },
+ [OMAP_DSS_CHANNEL_DIGIT] = {
+ .name = "DIGIT",
+ .vsync_irq = DISPC_IRQ_EVSYNC_ODD | DISPC_IRQ_EVSYNC_EVEN,
+ .framedone_irq = 0,
+ .sync_lost_irq = DISPC_IRQ_SYNC_LOST_DIGIT,
+ .reg_desc = {
+ [DISPC_MGR_FLD_ENABLE] = { DISPC_CONTROL, 1, 1 },
+ [DISPC_MGR_FLD_STNTFT] = { },
+ [DISPC_MGR_FLD_GO] = { DISPC_CONTROL, 6, 6 },
+ [DISPC_MGR_FLD_TFTDATALINES] = { },
+ [DISPC_MGR_FLD_STALLMODE] = { },
+ [DISPC_MGR_FLD_TCKENABLE] = { DISPC_CONFIG, 12, 12 },
+ [DISPC_MGR_FLD_TCKSELECTION] = { DISPC_CONFIG, 13, 13 },
+ [DISPC_MGR_FLD_CPR] = { },
+ [DISPC_MGR_FLD_FIFOHANDCHECK] = { DISPC_CONFIG, 16, 16 },
+ },
+ },
+ [OMAP_DSS_CHANNEL_LCD2] = {
+ .name = "LCD2",
+ .vsync_irq = DISPC_IRQ_VSYNC2,
+ .framedone_irq = DISPC_IRQ_FRAMEDONE2,
+ .sync_lost_irq = DISPC_IRQ_SYNC_LOST2,
+ .reg_desc = {
+ [DISPC_MGR_FLD_ENABLE] = { DISPC_CONTROL2, 0, 0 },
+ [DISPC_MGR_FLD_STNTFT] = { DISPC_CONTROL2, 3, 3 },
+ [DISPC_MGR_FLD_GO] = { DISPC_CONTROL2, 5, 5 },
+ [DISPC_MGR_FLD_TFTDATALINES] = { DISPC_CONTROL2, 9, 8 },
+ [DISPC_MGR_FLD_STALLMODE] = { DISPC_CONTROL2, 11, 11 },
+ [DISPC_MGR_FLD_TCKENABLE] = { DISPC_CONFIG2, 10, 10 },
+ [DISPC_MGR_FLD_TCKSELECTION] = { DISPC_CONFIG2, 11, 11 },
+ [DISPC_MGR_FLD_CPR] = { DISPC_CONFIG2, 15, 15 },
+ [DISPC_MGR_FLD_FIFOHANDCHECK] = { DISPC_CONFIG2, 16, 16 },
+ },
+ },
+};
+
static void _omap_dispc_set_irqs(void);
static inline void dispc_write_reg(const u16 idx, u32 val)
@@ -131,6 +205,18 @@ static inline u32 dispc_read_reg(const u16 idx)
return __raw_readl(dispc.base + idx);
}
+static u32 mgr_fld_read(enum omap_channel channel, enum mgr_reg_fields regfld)
+{
+ const struct reg_field rfld = mgr_desc[channel].reg_desc[regfld];
+ return REG_GET(rfld.reg, rfld.high, rfld.low);
+}
+
+static void mgr_fld_write(enum omap_channel channel,
+ enum mgr_reg_fields regfld, int val) {
+ const struct reg_field rfld = mgr_desc[channel].reg_desc[regfld];
+ REG_FLD_MOD(rfld.reg, val, rfld.high, rfld.low);
+}
+
#define SR(reg) \
dispc.ctx[DISPC_##reg / sizeof(u32)] = dispc_read_reg(DISPC_##reg)
#define RR(reg) \
@@ -398,90 +484,39 @@ static inline bool dispc_mgr_is_lcd(enum omap_channel channel)
u32 dispc_mgr_get_vsync_irq(enum omap_channel channel)
{
- switch (channel) {
- case OMAP_DSS_CHANNEL_LCD:
- return DISPC_IRQ_VSYNC;
- case OMAP_DSS_CHANNEL_LCD2:
- return DISPC_IRQ_VSYNC2;
- case OMAP_DSS_CHANNEL_DIGIT:
- return DISPC_IRQ_EVSYNC_ODD | DISPC_IRQ_EVSYNC_EVEN;
- default:
- BUG();
- return 0;
- }
+ return mgr_desc[channel].vsync_irq;
}
u32 dispc_mgr_get_framedone_irq(enum omap_channel channel)
{
- switch (channel) {
- case OMAP_DSS_CHANNEL_LCD:
- return DISPC_IRQ_FRAMEDONE;
- case OMAP_DSS_CHANNEL_LCD2:
- return DISPC_IRQ_FRAMEDONE2;
- case OMAP_DSS_CHANNEL_DIGIT:
- return 0;
- default:
- BUG();
- return 0;
- }
+ return mgr_desc[channel].framedone_irq;
}
bool dispc_mgr_go_busy(enum omap_channel channel)
{
- int bit;
-
- if (dispc_mgr_is_lcd(channel))
- bit = 5; /* GOLCD */
- else
- bit = 6; /* GODIGIT */
-
- if (channel = OMAP_DSS_CHANNEL_LCD2)
- return REG_GET(DISPC_CONTROL2, bit, bit) = 1;
- else
- return REG_GET(DISPC_CONTROL, bit, bit) = 1;
+ return mgr_fld_read(channel, DISPC_MGR_FLD_GO) = 1;
}
void dispc_mgr_go(enum omap_channel channel)
{
- int bit;
bool enable_bit, go_bit;
- if (dispc_mgr_is_lcd(channel))
- bit = 0; /* LCDENABLE */
- else
- bit = 1; /* DIGITALENABLE */
-
/* if the channel is not enabled, we don't need GO */
- if (channel = OMAP_DSS_CHANNEL_LCD2)
- enable_bit = REG_GET(DISPC_CONTROL2, bit, bit) = 1;
- else
- enable_bit = REG_GET(DISPC_CONTROL, bit, bit) = 1;
+ enable_bit = mgr_fld_read(channel, DISPC_MGR_FLD_ENABLE) = 1;
if (!enable_bit)
return;
- if (dispc_mgr_is_lcd(channel))
- bit = 5; /* GOLCD */
- else
- bit = 6; /* GODIGIT */
-
- if (channel = OMAP_DSS_CHANNEL_LCD2)
- go_bit = REG_GET(DISPC_CONTROL2, bit, bit) = 1;
- else
- go_bit = REG_GET(DISPC_CONTROL, bit, bit) = 1;
+ go_bit = mgr_fld_read(channel, DISPC_MGR_FLD_GO) = 1;
if (go_bit) {
DSSERR("GO bit not down for channel %d\n", channel);
return;
}
- DSSDBG("GO %s\n", channel = OMAP_DSS_CHANNEL_LCD ? "LCD" :
- (channel = OMAP_DSS_CHANNEL_LCD2 ? "LCD2" : "DIGIT"));
+ DSSDBG("GO %s\n", mgr_desc[channel].name);
- if (channel = OMAP_DSS_CHANNEL_LCD2)
- REG_FLD_MOD(DISPC_CONTROL2, 1, bit, bit);
- else
- REG_FLD_MOD(DISPC_CONTROL, 1, bit, bit);
+ mgr_fld_write(channel, DISPC_MGR_FLD_GO, 1);
}
static void dispc_ovl_write_firh_reg(enum omap_plane plane, int reg, u32 value)
@@ -922,16 +957,10 @@ void dispc_enable_gamma_table(bool enable)
static void dispc_mgr_enable_cpr(enum omap_channel channel, bool enable)
{
- u16 reg;
-
- if (channel = OMAP_DSS_CHANNEL_LCD)
- reg = DISPC_CONFIG;
- else if (channel = OMAP_DSS_CHANNEL_LCD2)
- reg = DISPC_CONFIG2;
- else
+ if (channel = OMAP_DSS_CHANNEL_DIGIT)
return;
- REG_FLD_MOD(reg, enable, 15, 15);
+ mgr_fld_write(channel, DISPC_MGR_FLD_CPR, enable);
}
static void dispc_mgr_set_cpr_coef(enum omap_channel channel,
@@ -2254,14 +2283,9 @@ static void dispc_disable_isr(void *data, u32 mask)
static void _enable_lcd_out(enum omap_channel channel, bool enable)
{
- if (channel = OMAP_DSS_CHANNEL_LCD2) {
- REG_FLD_MOD(DISPC_CONTROL2, enable ? 1 : 0, 0, 0);
- /* flush posted write */
- dispc_read_reg(DISPC_CONTROL2);
- } else {
- REG_FLD_MOD(DISPC_CONTROL, enable ? 1 : 0, 0, 0);
- dispc_read_reg(DISPC_CONTROL);
- }
+ mgr_fld_write(channel, DISPC_MGR_FLD_ENABLE, enable);
+ /* flush posted write */
+ mgr_fld_read(channel, DISPC_MGR_FLD_ENABLE);
}
static void dispc_mgr_enable_lcd_out(enum omap_channel channel, bool enable)
@@ -2274,12 +2298,9 @@ static void dispc_mgr_enable_lcd_out(enum omap_channel channel, bool enable)
/* When we disable LCD output, we need to wait until frame is done.
* Otherwise the DSS is still working, and turning off the clocks
* prevents DSS from going to OFF mode */
- is_on = channel = OMAP_DSS_CHANNEL_LCD2 ?
- REG_GET(DISPC_CONTROL2, 0, 0) :
- REG_GET(DISPC_CONTROL, 0, 0);
+ is_on = mgr_fld_read(channel, DISPC_MGR_FLD_ENABLE);
- irq = channel = OMAP_DSS_CHANNEL_LCD2 ? DISPC_IRQ_FRAMEDONE2 :
- DISPC_IRQ_FRAMEDONE;
+ irq = mgr_desc[channel].framedone_irq;
if (!enable && is_on) {
init_completion(&frame_done_completion);
@@ -2384,16 +2405,7 @@ static void dispc_mgr_enable_digit_out(bool enable)
bool dispc_mgr_is_enabled(enum omap_channel channel)
{
- if (channel = OMAP_DSS_CHANNEL_LCD)
- return !!REG_GET(DISPC_CONTROL, 0, 0);
- else if (channel = OMAP_DSS_CHANNEL_DIGIT)
- return !!REG_GET(DISPC_CONTROL, 1, 1);
- else if (channel = OMAP_DSS_CHANNEL_LCD2)
- return !!REG_GET(DISPC_CONTROL2, 0, 0);
- else {
- BUG();
- return false;
- }
+ return !!mgr_fld_read(channel, DISPC_MGR_FLD_ENABLE);
}
void dispc_mgr_enable(enum omap_channel channel, bool enable)
@@ -2432,10 +2444,7 @@ void dispc_pck_free_enable(bool enable)
void dispc_mgr_enable_fifohandcheck(enum omap_channel channel, bool enable)
{
- if (channel = OMAP_DSS_CHANNEL_LCD2)
- REG_FLD_MOD(DISPC_CONFIG2, enable ? 1 : 0, 16, 16);
- else
- REG_FLD_MOD(DISPC_CONFIG, enable ? 1 : 0, 16, 16);
+ mgr_fld_write(channel, DISPC_MGR_FLD_FIFOHANDCHECK, enable);
}
@@ -2458,10 +2467,7 @@ void dispc_mgr_set_lcd_display_type(enum omap_channel channel,
return;
}
- if (channel = OMAP_DSS_CHANNEL_LCD2)
- REG_FLD_MOD(DISPC_CONTROL2, mode, 3, 3);
- else
- REG_FLD_MOD(DISPC_CONTROL, mode, 3, 3);
+ mgr_fld_write(channel, DISPC_MGR_FLD_STNTFT, mode);
}
void dispc_set_loadmode(enum omap_dss_load_mode mode)
@@ -2479,24 +2485,14 @@ static void dispc_mgr_set_trans_key(enum omap_channel ch,
enum omap_dss_trans_key_type type,
u32 trans_key)
{
- if (ch = OMAP_DSS_CHANNEL_LCD)
- REG_FLD_MOD(DISPC_CONFIG, type, 11, 11);
- else if (ch = OMAP_DSS_CHANNEL_DIGIT)
- REG_FLD_MOD(DISPC_CONFIG, type, 13, 13);
- else /* OMAP_DSS_CHANNEL_LCD2 */
- REG_FLD_MOD(DISPC_CONFIG2, type, 11, 11);
+ mgr_fld_write(ch, DISPC_MGR_FLD_TCKSELECTION, type);
dispc_write_reg(DISPC_TRANS_COLOR(ch), trans_key);
}
static void dispc_mgr_enable_trans_key(enum omap_channel ch, bool enable)
{
- if (ch = OMAP_DSS_CHANNEL_LCD)
- REG_FLD_MOD(DISPC_CONFIG, enable, 10, 10);
- else if (ch = OMAP_DSS_CHANNEL_DIGIT)
- REG_FLD_MOD(DISPC_CONFIG, enable, 12, 12);
- else /* OMAP_DSS_CHANNEL_LCD2 */
- REG_FLD_MOD(DISPC_CONFIG2, enable, 10, 10);
+ mgr_fld_write(ch, DISPC_MGR_FLD_TCKENABLE, enable);
}
static void dispc_mgr_enable_alpha_fixed_zorder(enum omap_channel ch,
@@ -2547,10 +2543,7 @@ void dispc_mgr_set_tft_data_lines(enum omap_channel channel, u8 data_lines)
return;
}
- if (channel = OMAP_DSS_CHANNEL_LCD2)
- REG_FLD_MOD(DISPC_CONTROL2, code, 9, 8);
- else
- REG_FLD_MOD(DISPC_CONTROL, code, 9, 8);
+ mgr_fld_write(channel, DISPC_MGR_FLD_TFTDATALINES, code);
}
void dispc_mgr_set_io_pad_mode(enum dss_io_pad_mode mode)
@@ -2584,10 +2577,7 @@ void dispc_mgr_set_io_pad_mode(enum dss_io_pad_mode mode)
void dispc_mgr_enable_stallmode(enum omap_channel channel, bool enable)
{
- if (channel = OMAP_DSS_CHANNEL_LCD2)
- REG_FLD_MOD(DISPC_CONTROL2, enable, 11, 11);
- else
- REG_FLD_MOD(DISPC_CONTROL, enable, 11, 11);
+ mgr_fld_write(channel, DISPC_MGR_FLD_STALLMODE, enable);
}
static bool _dispc_mgr_size_ok(u16 width, u16 height)
@@ -3450,12 +3440,6 @@ static void dispc_error_worker(struct work_struct *work)
DISPC_IRQ_VID3_FIFO_UNDERFLOW,
};
- static const unsigned sync_lost_bits[] = {
- DISPC_IRQ_SYNC_LOST,
- DISPC_IRQ_SYNC_LOST_DIGIT,
- DISPC_IRQ_SYNC_LOST2,
- };
-
spin_lock_irqsave(&dispc.irq_lock, flags);
errors = dispc.error_irqs;
dispc.error_irqs = 0;
@@ -3484,7 +3468,7 @@ static void dispc_error_worker(struct work_struct *work)
unsigned bit;
mgr = omap_dss_get_overlay_manager(i);
- bit = sync_lost_bits[i];
+ bit = mgr_desc[i].sync_lost_irq;
if (bit & errors) {
struct omap_dss_device *dssdev = mgr->device;
diff --git a/drivers/video/omap2/dss/dsi.c b/drivers/video/omap2/dss/dsi.c
index ca8382d..2faf913 100644
--- a/drivers/video/omap2/dss/dsi.c
+++ b/drivers/video/omap2/dss/dsi.c
@@ -4360,8 +4360,7 @@ static int dsi_display_init_dispc(struct omap_dss_device *dssdev)
timings.x_res = dw;
timings.y_res = dh;
- irq = dssdev->manager->id = OMAP_DSS_CHANNEL_LCD ?
- DISPC_IRQ_FRAMEDONE : DISPC_IRQ_FRAMEDONE2;
+ irq = dispc_mgr_get_framedone_irq(dssdev->manager->id);
r = omap_dispc_register_isr(dsi_framedone_irq_callback,
(void *) dssdev, irq);
@@ -4393,8 +4392,7 @@ static void dsi_display_uninit_dispc(struct omap_dss_device *dssdev)
if (dssdev->panel.dsi_mode = OMAP_DSS_DSI_CMD_MODE) {
u32 irq;
- irq = dssdev->manager->id = OMAP_DSS_CHANNEL_LCD ?
- DISPC_IRQ_FRAMEDONE : DISPC_IRQ_FRAMEDONE2;
+ irq = dispc_mgr_get_framedone_irq(dssdev->manager->id);
omap_dispc_unregister_isr(dsi_framedone_irq_callback,
(void *) dssdev, irq);
diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h
index dd1092c..df131fc 100644
--- a/drivers/video/omap2/dss/dss.h
+++ b/drivers/video/omap2/dss/dss.h
@@ -152,6 +152,12 @@ struct dsi_clock_info {
u16 lp_clk_div;
};
+struct reg_field {
+ u16 reg;
+ u8 high;
+ u8 low;
+};
+
struct seq_file;
struct platform_device;
diff --git a/drivers/video/omap2/dss/manager.c b/drivers/video/omap2/dss/manager.c
index 0cbcde4..bb602a2 100644
--- a/drivers/video/omap2/dss/manager.c
+++ b/drivers/video/omap2/dss/manager.c
@@ -500,16 +500,12 @@ static int dss_mgr_wait_for_vsync(struct omap_overlay_manager *mgr)
if (r)
return r;
- if (mgr->device->type = OMAP_DISPLAY_TYPE_VENC) {
+ if (mgr->device->type = OMAP_DISPLAY_TYPE_VENC)
irq = DISPC_IRQ_EVSYNC_ODD;
- } else if (mgr->device->type = OMAP_DISPLAY_TYPE_HDMI) {
+ else if (mgr->device->type = OMAP_DISPLAY_TYPE_HDMI)
irq = DISPC_IRQ_EVSYNC_EVEN;
- } else {
- if (mgr->id = OMAP_DSS_CHANNEL_LCD)
- irq = DISPC_IRQ_VSYNC;
- else
- irq = DISPC_IRQ_VSYNC2;
- }
+ else
+ irq = dispc_mgr_get_vsync_irq(mgr->id);
r = omap_dispc_wait_for_irq_interruptible_timeout(irq, timeout);
--
1.7.1
^ permalink raw reply related
* Re: [PATCH 3/3] OMAPDSS: HDMI: Cache EDID
From: Jassi Brar @ 2012-06-28 13:25 UTC (permalink / raw)
To: Andy Green
Cc: Tomi Valkeinen, mythripk, linux-omap, linux-fbdev, n-dechesne,
patches
In-Reply-To: <4FEC47FA.1040801@linaro.org>
On 28 June 2012 17:33, Andy Green <andy.green@linaro.org> wrote:
> On 06/28/12 19:10, the mail apparently from Tomi Valkeinen included:
>
>> On Thu, 2012-06-28 at 16:28 +0530, Jassi Brar wrote:
>>>
>>> On 28 June 2012 16:17, Jassi Brar <jaswinder.singh@linaro.org> wrote:
>>>>
>>>> On 28 June 2012 15:44, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>>>>>
>>>>> On Thu, 2012-06-28 at 15:18 +0530, Jassi Brar wrote:
>>>>
>>>>
>>>>>>>> --- a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
>>>>>>>> +++ b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
>>>>>>>> @@ -243,10 +243,13 @@ static int hdmi_check_hpd_state(struct
>>>>>>>> hdmi_ip_data *ip_data)
>>>>>>>>
>>>>>>>> hpd = gpio_get_value(ip_data->hpd_gpio);
>>>>>>>>
>>>>>>>> - if (hpd)
>>>>>>>> + if (hpd) {
>>>>>>>> r = hdmi_set_phy_pwr(ip_data, HDMI_PHYPWRCMD_TXON);
>>>>>>>> - else
>>>>>>>> + } else {
>>>>>>>> + /* Invalidate EDID Cache */
>>>>>>>> + ip_data->edid_len = 0;
>>>>>>>> r = hdmi_set_phy_pwr(ip_data, HDMI_PHYPWRCMD_LDOON);
>>>>>>>> + }
>>>>>>>
>>>>>>>
>>>>>>> There's a problem with this patch, which leaves a wrong EDID in the
>>>>>>> cache: if you first have the cable connected and hdmi is enabled, you
>>>>>>> then turn off the HDMI display device via sysfs, we do not go to
>>>>>>> hdmi_check_hpd_state at all. The next time hdmi is enabled, we only
>>>>>>> get
>>>>>>> the plug-in event, and thus EDID cache is never invalidated.
>>>>>>>
>>>>>> If the hdmi cable is not replugged during that period, I don't see why
>>>>>> would you want the EDID invalidated ?
>>>>>
>>>>>
>>>>> I wasn't very clear with my comment.
>>>>>
>>>>> When the display device is disabled, we're not listening to the hpd
>>>>> interrupt anymore. So when it's disabled, the cable can be replugged
>>>>> and
>>>>> the monitor changed, and the driver won't know about it. And so it'll
>>>>> return the old EDID for the new monitor.
>>>>>
>>>> If that could be a problem, then we already have some problem with
>>>> current omapdss.
>>>>
>>>> I think among the first things, while enabling HDMI, should be to see
>>>> if there is really some display connected on the port i.e, HPD
>>>> asserted. Only if ti_hdmi_4_detect() returned true, should we
>>>> proceed otherwise wait for HPQ irq.
>>>>
>>>> Unconditionally invalidating edid really seems like a regression - we
>>>> impose atleast 50ms (edid read) as extra cost on
>>>> hdmi_check_hpd_state(), which kills half the purpose of this patch.
>>>>
>>> Sorry a correction. Reading detect() won't work. I suggest we keep HPD
>>> IRQ enabled for the lifetime of the driver.
>>
>>
>> Ok, I see. But that's not acceptable. It would require us to keep the
>> TPD12S015 always powered and enabled. Even if you're not interested in
>> using HDMI at all.
>>
>> For this to work like you want we need a bigger restructuring of HDMI
>> and partly omapdss also. Currently we have just one big "enabled" or
>> "disabled" state. We need multiple states. Probably something like:
>>
>> - disabled, everything totally off
>> - low power, hotplug detection enabled
>> - powered on, but no video
>> - video enabled
>>
>> Been long in my mind, but I'm not very familiar with HDMI so I could get
>> my simple prototypes to work when I tried something like this once.
>
>
> That doesn't sound too hard since the difference between the first three
> states at the HDMI chip is just whether the two gpio controlling it are 00,
> 10 or 11.
>
> If Jassi's alright with it we might have a go at implementing this, but can
> you define a bit more about how we logically tell DSS that we want to, eg,
> disable HDMI totally?
>
A quick reaction of my guts say, we simply enable 5V/HPD_IRQ during
probe and disable during remove.
HDMI enable/disable via /sysfs/ and HPD (de)assertion, switch only
HDMI_PHY on/off.
The user selecting "Autodetect and Configure" option would then equate
to "(un)loading" of the HDMI driver.
Not to mean a trivial job.
^ permalink raw reply
* Re: [PATCH 3/3] OMAPDSS: HDMI: Cache EDID
From: Tomi Valkeinen @ 2012-06-28 13:31 UTC (permalink / raw)
To: Jassi Brar
Cc: Andy Green, mythripk, linux-omap, linux-fbdev, n-dechesne,
patches
In-Reply-To: <CAJe_Zhdkhu4TciiwGJB2Kz8oZp2RQNZfESSr5Cfv0R1MNn=r9A@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1803 bytes --]
On Thu, 2012-06-28 at 18:43 +0530, Jassi Brar wrote:
> On 28 June 2012 17:33, Andy Green <andy.green@linaro.org> wrote:
> > If Jassi's alright with it we might have a go at implementing this, but can
> > you define a bit more about how we logically tell DSS that we want to, eg,
> > disable HDMI totally?
> >
> A quick reaction of my guts say, we simply enable 5V/HPD_IRQ during
> probe and disable during remove.
The problem with this is a feature of omapdss: we can have multiple
displays for the same output, of which only one can be enabled at the
same time. What this means is that you shouldn't (and in some cases
can't) allocate or enable resources in probe that may be shared, because
then the driver for both displays would try to allocate the same
resource.
Sure, this is not a problem for the HDMI configuration we are using now,
but it's still against the panel model we have. Thus we should allocate
resources only when the panel device is turned on, and release them when
it's disabled.
I do think the model is slightly broken, but that's what we have now.
And I'm also not even sure how it should be fixed...
And also, as I said earlier, if you keep it enabled all the time, it'll
eat power even if the user is never going to use HDMI.
On a desktop I guess the power consumption wouldn't be an issue, but I
do feel a bit uneasy about it on an embedded device.
> HDMI enable/disable via /sysfs/ and HPD (de)assertion, switch only
> HDMI_PHY on/off.
> The user selecting "Autodetect and Configure" option would then equate
> to "(un)loading" of the HDMI driver.
HDMI cannot be currently compiled as a separate module. Although I think
you can detach a device and a driver, achieving the same. Is that what
you meant with unloading?
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 3/3] OMAPDSS: HDMI: Cache EDID
From: Tomi Valkeinen @ 2012-06-28 13:35 UTC (permalink / raw)
To: Jassi Brar
Cc: mythripk, linux-omap, linux-fbdev, andy.green, n-dechesne,
patches
In-Reply-To: <CAJe_ZhfehDquDcyefb7Z9odH+hfQTFiRAux_iyvAycs9Otovtw@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 860 bytes --]
On Thu, 2012-06-28 at 16:28 +0530, Jassi Brar wrote:
> > I think among the first things, while enabling HDMI, should be to see
> > if there is really some display connected on the port i.e, HPD
> > asserted. Only if ti_hdmi_4_detect() returned true, should we
> > proceed otherwise wait for HPQ irq.
> >
> > Unconditionally invalidating edid really seems like a regression - we
> > impose atleast 50ms (edid read) as extra cost on
> > hdmi_check_hpd_state(), which kills half the purpose of this patch.
> >
> Sorry a correction. Reading detect() won't work. I suggest we keep HPD
> IRQ enabled for the lifetime of the driver.
By the way, when the device is in system suspend, we surely won't detect
the HPD even if we kept the HPD always enabled. So there we'll miss the
HPD interrupt anyway, and the EDID cache would be invalid.
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* [PATCH 00/12] OMAPDSS: Apply LCD manager related parameters
From: Archit Taneja @ 2012-06-28 14:42 UTC (permalink / raw)
To: tomi.valkeinen; +Cc: linux-fbdev, linux-omap, Archit Taneja
The LCD interface drivers(DPI, DSI, RFBI, SDI) do some direct DISPC register
writes to configure LCD manager related fields. This series groups these fields
into a single struct, and let's the interface driver apply these parameters.
This allows us to:
- Check the LCD manager related parameters before applying them.
- Remove some omap_dss_device references as APPLY holds the applied parameters.
Pushed onto the branch:
git://gitorious.org/~boddob/linux-omap-dss2/archit-dss2-clone.git apply_lcd_mgr_params
Tested on 3430SDP, 4430SDP ans Pandaboard ES.
Archit Taneja (12):
OMAPDSS: DISPC: Change return type of dispc_mgr_set_clock_div()
OMAPDSS: Add struct to hold LCD overlay manager configuration
OMAPDSS: DPI: Configure dss_lcd_mgr_config struct with lcd manager
parameters
OMAPDSS: RFBI: Configure dss_lcd_mgr_config struct with lcd manager
parameters
OMAPDSS: DSI: Configure dss_lcd_mgr_config struct with lcd manager
parameters
OMAPDSS: SDI: Configure dss_lcd_mgr_config struct with lcd manager
parameters
OMAPDSS: APPLY: Remove DISPC writes to manager's lcd parameters in
interface drivers
OMAPDSS: MANAGER: Check LCD related overlay manager parameters
OMAPDSS: APPLY: Remove usage of omap_dss_device from manual/auto
update checks
OMAPDSS: DISPC: Remove a redundant function
OMAPDSS: RFBI: Use dss_mgr_enable to enable the overlay manager
OMAPDSS: OVERLAY: Clean up replication checking
drivers/video/omap2/dss/apply.c | 82 ++++++++++++++++++++++++++--
drivers/video/omap2/dss/dispc.c | 31 ++++-------
drivers/video/omap2/dss/display.c | 34 ------------
drivers/video/omap2/dss/dpi.c | 31 ++++++-----
drivers/video/omap2/dss/dsi.c | 108 ++++++++++++++++++++++---------------
drivers/video/omap2/dss/dss.h | 30 +++++++++--
drivers/video/omap2/dss/manager.c | 35 ++++++++++++
drivers/video/omap2/dss/overlay.c | 13 +++++
drivers/video/omap2/dss/rfbi.c | 39 ++++++++++----
drivers/video/omap2/dss/sdi.c | 32 +++++------
10 files changed, 286 insertions(+), 149 deletions(-)
--
1.7.9.5
^ permalink raw reply
* [PATCH 01/12] MAPDSS: DISPC: Change return type of dispc_mgr_set_clock_div()
From: Archit Taneja @ 2012-06-28 14:42 UTC (permalink / raw)
To: tomi.valkeinen; +Cc: linux-fbdev, linux-omap, Archit Taneja
In-Reply-To: <1340893842-10626-1-git-send-email-archit@ti.com>
dipsc_mgr_set_clock div has an int return type to report errors or success.
The function doesn't really check for errors and always returns 0. Change
the return type to void.
Checking for the correct DISPC clock divider ranges will be done when a DSS2
user does a manager apply. This support will be added later.
Signed-off-by: Archit Taneja <archit@ti.com>
---
drivers/video/omap2/dss/dispc.c | 4 +---
drivers/video/omap2/dss/dpi.c | 10 ++--------
drivers/video/omap2/dss/dsi.c | 6 +-----
drivers/video/omap2/dss/dss.h | 2 +-
drivers/video/omap2/dss/sdi.c | 5 +----
5 files changed, 6 insertions(+), 21 deletions(-)
diff --git a/drivers/video/omap2/dss/dispc.c b/drivers/video/omap2/dss/dispc.c
index 864adcc..b047fe6 100644
--- a/drivers/video/omap2/dss/dispc.c
+++ b/drivers/video/omap2/dss/dispc.c
@@ -3184,15 +3184,13 @@ int dispc_calc_clock_rates(unsigned long dispc_fclk_rate,
return 0;
}
-int dispc_mgr_set_clock_div(enum omap_channel channel,
+void dispc_mgr_set_clock_div(enum omap_channel channel,
struct dispc_clock_info *cinfo)
{
DSSDBG("lck = %lu (%u)\n", cinfo->lck, cinfo->lck_div);
DSSDBG("pck = %lu (%u)\n", cinfo->pck, cinfo->pck_div);
dispc_mgr_set_lcd_divisor(channel, cinfo->lck_div, cinfo->pck_div);
-
- return 0;
}
int dispc_mgr_get_clock_div(enum omap_channel channel,
diff --git a/drivers/video/omap2/dss/dpi.c b/drivers/video/omap2/dss/dpi.c
index 046a6fb..af01430 100644
--- a/drivers/video/omap2/dss/dpi.c
+++ b/drivers/video/omap2/dss/dpi.c
@@ -100,11 +100,7 @@ static int dpi_set_dsi_clk(struct omap_dss_device *dssdev,
dss_select_dispc_clk_source(clks->dispc.fclk_src);
- r = dispc_mgr_set_clock_div(dssdev->manager->id, &dispc_cinfo);
- if (r) {
- dss_select_dispc_clk_source(OMAP_DSS_CLK_SRC_FCK);
- return r;
- }
+ dispc_mgr_set_clock_div(dssdev->manager->id, &dispc_cinfo);
*fck = dsi_cinfo.dsi_pll_hsdiv_dispc_clk;
*lck_div = dispc_cinfo.lck_div;
@@ -129,9 +125,7 @@ static int dpi_set_dispc_clk(struct omap_dss_device *dssdev,
if (r)
return r;
- r = dispc_mgr_set_clock_div(dssdev->manager->id, &dispc_cinfo);
- if (r)
- return r;
+ dispc_mgr_set_clock_div(dssdev->manager->id, &dispc_cinfo);
*fck = dss_cinfo.fck;
*lck_div = dispc_cinfo.lck_div;
diff --git a/drivers/video/omap2/dss/dsi.c b/drivers/video/omap2/dss/dsi.c
index 9f08787..94ff3aa 100644
--- a/drivers/video/omap2/dss/dsi.c
+++ b/drivers/video/omap2/dss/dsi.c
@@ -4456,11 +4456,7 @@ static int dsi_configure_dispc_clocks(struct omap_dss_device *dssdev,
return r;
}
- r = dispc_mgr_set_clock_div(dssdev->manager->id, &dispc_cinfo);
- if (r) {
- DSSERR("Failed to set dispc clocks\n");
- return r;
- }
+ dispc_mgr_set_clock_div(dssdev->manager->id, &dispc_cinfo);
return 0;
}
diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h
index 20151d5..f8289e9 100644
--- a/drivers/video/omap2/dss/dss.h
+++ b/drivers/video/omap2/dss/dss.h
@@ -416,7 +416,7 @@ void dispc_mgr_set_timings(enum omap_channel channel,
unsigned long dispc_mgr_lclk_rate(enum omap_channel channel);
unsigned long dispc_mgr_pclk_rate(enum omap_channel channel);
unsigned long dispc_core_clk_rate(void);
-int dispc_mgr_set_clock_div(enum omap_channel channel,
+void dispc_mgr_set_clock_div(enum omap_channel channel,
struct dispc_clock_info *cinfo);
int dispc_mgr_get_clock_div(enum omap_channel channel,
struct dispc_clock_info *cinfo);
diff --git a/drivers/video/omap2/dss/sdi.c b/drivers/video/omap2/dss/sdi.c
index b44ab72..0b2659f 100644
--- a/drivers/video/omap2/dss/sdi.c
+++ b/drivers/video/omap2/dss/sdi.c
@@ -106,9 +106,7 @@ int omapdss_sdi_display_enable(struct omap_dss_device *dssdev)
if (r)
goto err_set_dss_clock_div;
- r = dispc_mgr_set_clock_div(dssdev->manager->id, &dispc_cinfo);
- if (r)
- goto err_set_dispc_clock_div;
+ dispc_mgr_set_clock_div(dssdev->manager->id, &dispc_cinfo);
dss_sdi_init(dssdev->phy.sdi.datapairs);
r = dss_sdi_enable();
@@ -125,7 +123,6 @@ int omapdss_sdi_display_enable(struct omap_dss_device *dssdev)
err_mgr_enable:
dss_sdi_disable();
err_sdi_enable:
-err_set_dispc_clock_div:
err_set_dss_clock_div:
err_calc_clock_div:
dispc_runtime_put();
--
1.7.9.5
^ permalink raw reply related
* [PATCH 01/12] OMAPDSS: DISPC: Change return type of dispc_mgr_set_clock_div()
From: Archit Taneja @ 2012-06-28 14:42 UTC (permalink / raw)
To: tomi.valkeinen; +Cc: linux-fbdev, linux-omap, Archit Taneja
In-Reply-To: <1340893842-10626-1-git-send-email-archit@ti.com>
dipsc_mgr_set_clock div has an int return type to report errors or success.
The function doesn't really check for errors and always returns 0. Change
the return type to void.
Checking for the correct DISPC clock divider ranges will be done when a DSS2
user does a manager apply. This support will be added later.
Signed-off-by: Archit Taneja <archit@ti.com>
---
drivers/video/omap2/dss/dispc.c | 4 +---
drivers/video/omap2/dss/dpi.c | 10 ++--------
drivers/video/omap2/dss/dsi.c | 6 +-----
drivers/video/omap2/dss/dss.h | 2 +-
drivers/video/omap2/dss/sdi.c | 5 +----
5 files changed, 6 insertions(+), 21 deletions(-)
diff --git a/drivers/video/omap2/dss/dispc.c b/drivers/video/omap2/dss/dispc.c
index 864adcc..b047fe6 100644
--- a/drivers/video/omap2/dss/dispc.c
+++ b/drivers/video/omap2/dss/dispc.c
@@ -3184,15 +3184,13 @@ int dispc_calc_clock_rates(unsigned long dispc_fclk_rate,
return 0;
}
-int dispc_mgr_set_clock_div(enum omap_channel channel,
+void dispc_mgr_set_clock_div(enum omap_channel channel,
struct dispc_clock_info *cinfo)
{
DSSDBG("lck = %lu (%u)\n", cinfo->lck, cinfo->lck_div);
DSSDBG("pck = %lu (%u)\n", cinfo->pck, cinfo->pck_div);
dispc_mgr_set_lcd_divisor(channel, cinfo->lck_div, cinfo->pck_div);
-
- return 0;
}
int dispc_mgr_get_clock_div(enum omap_channel channel,
diff --git a/drivers/video/omap2/dss/dpi.c b/drivers/video/omap2/dss/dpi.c
index 046a6fb..af01430 100644
--- a/drivers/video/omap2/dss/dpi.c
+++ b/drivers/video/omap2/dss/dpi.c
@@ -100,11 +100,7 @@ static int dpi_set_dsi_clk(struct omap_dss_device *dssdev,
dss_select_dispc_clk_source(clks->dispc.fclk_src);
- r = dispc_mgr_set_clock_div(dssdev->manager->id, &dispc_cinfo);
- if (r) {
- dss_select_dispc_clk_source(OMAP_DSS_CLK_SRC_FCK);
- return r;
- }
+ dispc_mgr_set_clock_div(dssdev->manager->id, &dispc_cinfo);
*fck = dsi_cinfo.dsi_pll_hsdiv_dispc_clk;
*lck_div = dispc_cinfo.lck_div;
@@ -129,9 +125,7 @@ static int dpi_set_dispc_clk(struct omap_dss_device *dssdev,
if (r)
return r;
- r = dispc_mgr_set_clock_div(dssdev->manager->id, &dispc_cinfo);
- if (r)
- return r;
+ dispc_mgr_set_clock_div(dssdev->manager->id, &dispc_cinfo);
*fck = dss_cinfo.fck;
*lck_div = dispc_cinfo.lck_div;
diff --git a/drivers/video/omap2/dss/dsi.c b/drivers/video/omap2/dss/dsi.c
index 9f08787..94ff3aa 100644
--- a/drivers/video/omap2/dss/dsi.c
+++ b/drivers/video/omap2/dss/dsi.c
@@ -4456,11 +4456,7 @@ static int dsi_configure_dispc_clocks(struct omap_dss_device *dssdev,
return r;
}
- r = dispc_mgr_set_clock_div(dssdev->manager->id, &dispc_cinfo);
- if (r) {
- DSSERR("Failed to set dispc clocks\n");
- return r;
- }
+ dispc_mgr_set_clock_div(dssdev->manager->id, &dispc_cinfo);
return 0;
}
diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h
index 20151d5..f8289e9 100644
--- a/drivers/video/omap2/dss/dss.h
+++ b/drivers/video/omap2/dss/dss.h
@@ -416,7 +416,7 @@ void dispc_mgr_set_timings(enum omap_channel channel,
unsigned long dispc_mgr_lclk_rate(enum omap_channel channel);
unsigned long dispc_mgr_pclk_rate(enum omap_channel channel);
unsigned long dispc_core_clk_rate(void);
-int dispc_mgr_set_clock_div(enum omap_channel channel,
+void dispc_mgr_set_clock_div(enum omap_channel channel,
struct dispc_clock_info *cinfo);
int dispc_mgr_get_clock_div(enum omap_channel channel,
struct dispc_clock_info *cinfo);
diff --git a/drivers/video/omap2/dss/sdi.c b/drivers/video/omap2/dss/sdi.c
index b44ab72..0b2659f 100644
--- a/drivers/video/omap2/dss/sdi.c
+++ b/drivers/video/omap2/dss/sdi.c
@@ -106,9 +106,7 @@ int omapdss_sdi_display_enable(struct omap_dss_device *dssdev)
if (r)
goto err_set_dss_clock_div;
- r = dispc_mgr_set_clock_div(dssdev->manager->id, &dispc_cinfo);
- if (r)
- goto err_set_dispc_clock_div;
+ dispc_mgr_set_clock_div(dssdev->manager->id, &dispc_cinfo);
dss_sdi_init(dssdev->phy.sdi.datapairs);
r = dss_sdi_enable();
@@ -125,7 +123,6 @@ int omapdss_sdi_display_enable(struct omap_dss_device *dssdev)
err_mgr_enable:
dss_sdi_disable();
err_sdi_enable:
-err_set_dispc_clock_div:
err_set_dss_clock_div:
err_calc_clock_div:
dispc_runtime_put();
--
1.7.9.5
^ permalink raw reply related
* [PATCH 02/12] OMAPDSS: Add struct to hold LCD overlay manager configuration
From: Archit Taneja @ 2012-06-28 14:42 UTC (permalink / raw)
To: tomi.valkeinen; +Cc: linux-fbdev, linux-omap, Archit Taneja
In-Reply-To: <1340893842-10626-1-git-send-email-archit@ti.com>
Create a struct dss_lcd_mgr_config which holds LCD overlay manager related
parameters. These are currently partially contained in the omap_dss_device
connected to the manager, and the rest are in the interface driver.
The parameters are directly written to the DISPC registers in the interface
drivers. These should eventually be applied at the correct time using the
shadow register programming model. This struct would help in grouping these
parameters so that they can be applied together.
Signed-off-by: Archit Taneja <archit@ti.com>
---
drivers/video/omap2/dss/dss.h | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h
index f8289e9..6688eaf 100644
--- a/drivers/video/omap2/dss/dss.h
+++ b/drivers/video/omap2/dss/dss.h
@@ -152,6 +152,19 @@ struct dsi_clock_info {
u16 lp_clk_div;
};
+struct dss_lcd_mgr_config {
+ enum dss_io_pad_mode io_pad_mode;
+
+ bool stallmode;
+ bool fifohandcheck;
+
+ struct dispc_clock_info clock_info;
+
+ int video_port_width;
+
+ int lcden_sig_polarity;
+};
+
struct seq_file;
struct platform_device;
--
1.7.9.5
^ permalink raw reply related
* [PATCH 03/12] OMAPDSS: DPI: Configure dss_lcd_mgr_config struct with lcd manager parameters
From: Archit Taneja @ 2012-06-28 14:42 UTC (permalink / raw)
To: tomi.valkeinen; +Cc: linux-fbdev, linux-omap, Archit Taneja
In-Reply-To: <1340893842-10626-1-git-send-email-archit@ti.com>
Create a dss_lcd_mgr_config struct instance in DPI. Fill up all the parameters
of the struct with configurations held by the panel, and the configurations
required by DPI.
Use these to write to the DISPC registers. These direct register writes would be
later replaced by a function which applies the configuration using the shadow
register programming model.
The DISPC_DIVISORo registers were written in the functions dpi_set_dispc_clk()
and dpi_set_dsi_clk(), now they just fill up the dispc_clock_info parameter in
mgr_config. They are written later in dpi_config_lcd_manager.
Signed-off-by: Archit Taneja <archit@ti.com>
---
drivers/video/omap2/dss/dpi.c | 37 ++++++++++++++++++++++++++++---------
1 file changed, 28 insertions(+), 9 deletions(-)
diff --git a/drivers/video/omap2/dss/dpi.c b/drivers/video/omap2/dss/dpi.c
index af01430..a3a012b 100644
--- a/drivers/video/omap2/dss/dpi.c
+++ b/drivers/video/omap2/dss/dpi.c
@@ -38,6 +38,8 @@
static struct {
struct regulator *vdds_dsi_reg;
struct platform_device *dsidev;
+
+ struct dss_lcd_mgr_config mgr_config;
} dpi;
static struct platform_device *dpi_get_dsidev(enum omap_dss_clk_source clk)
@@ -100,7 +102,7 @@ static int dpi_set_dsi_clk(struct omap_dss_device *dssdev,
dss_select_dispc_clk_source(clks->dispc.fclk_src);
- dispc_mgr_set_clock_div(dssdev->manager->id, &dispc_cinfo);
+ dpi.mgr_config.clock_info = dispc_cinfo;
*fck = dsi_cinfo.dsi_pll_hsdiv_dispc_clk;
*lck_div = dispc_cinfo.lck_div;
@@ -125,7 +127,7 @@ static int dpi_set_dispc_clk(struct omap_dss_device *dssdev,
if (r)
return r;
- dispc_mgr_set_clock_div(dssdev->manager->id, &dispc_cinfo);
+ dpi.mgr_config.clock_info = dispc_cinfo;
*fck = dss_cinfo.fck;
*lck_div = dispc_cinfo.lck_div;
@@ -166,15 +168,32 @@ static int dpi_set_mode(struct omap_dss_device *dssdev)
return 0;
}
-static void dpi_basic_init(struct omap_dss_device *dssdev)
+static void dpi_config_lcd_manager(struct omap_dss_device *dssdev)
{
- dispc_mgr_set_io_pad_mode(DSS_IO_PAD_MODE_BYPASS);
- dispc_mgr_enable_stallmode(dssdev->manager->id, false);
+ dpi.mgr_config.io_pad_mode = DSS_IO_PAD_MODE_BYPASS;
- dispc_mgr_set_lcd_type_tft(dssdev->manager->id);
+ dpi.mgr_config.stallmode = false;
+ dpi.mgr_config.fifohandcheck = false;
+
+ dpi.mgr_config.video_port_width = dssdev->phy.dpi.data_lines;
+
+ dpi.mgr_config.lcden_sig_polarity = false;
+
+ dispc_mgr_set_io_pad_mode(dpi.mgr_config.io_pad_mode);
+ dispc_mgr_enable_stallmode(dssdev->manager->id,
+ dpi.mgr_config.stallmode);
+ dispc_mgr_enable_fifohandcheck(dssdev->manager->id,
+ dpi.mgr_config.fifohandcheck);
dispc_mgr_set_tft_data_lines(dssdev->manager->id,
- dssdev->phy.dpi.data_lines);
+ dpi.mgr_config.video_port_width);
+
+ dispc_mgr_set_clock_div(dssdev->manager->id,
+ &dpi.mgr_config.clock_info);
+
+ dispc_lcd_enable_signal_polarity(dpi.mgr_config.lcden_sig_polarity);
+
+ dispc_mgr_set_lcd_type_tft(dssdev->manager->id);
}
int omapdss_dpi_display_enable(struct omap_dss_device *dssdev)
@@ -213,8 +232,6 @@ int omapdss_dpi_display_enable(struct omap_dss_device *dssdev)
if (r)
goto err_get_dispc;
- dpi_basic_init(dssdev);
-
if (dpi_use_dsi_pll(dssdev)) {
r = dsi_runtime_get(dpi.dsidev);
if (r)
@@ -229,6 +246,8 @@ int omapdss_dpi_display_enable(struct omap_dss_device *dssdev)
if (r)
goto err_set_mode;
+ dpi_config_lcd_manager(dssdev);
+
mdelay(2);
r = dss_mgr_enable(dssdev->manager);
--
1.7.9.5
^ permalink raw reply related
* [PATCH 04/12] OMAPDSS: RFBI: Configure dss_lcd_mgr_config struct with lcd manager parameters
From: Archit Taneja @ 2012-06-28 14:42 UTC (permalink / raw)
To: tomi.valkeinen; +Cc: linux-fbdev, linux-omap, Archit Taneja
In-Reply-To: <1340893842-10626-1-git-send-email-archit@ti.com>
Create a dss_lcd_mgr_config struct instance in RFBI. Fill up all the parameters
of the struct with configurations held by the panel, and the configurations
required by RFBI.
Use these to write to the DISPC registers. These direct register writes would be
later replaced by a function which applies the configuration using the shadow
register programming model.
Create function rfbi_config_lcd_manager() which fills up the mgr_config
parameters and writes to the DISPC regs.
Signed-off-by: Archit Taneja <archit@ti.com>
---
drivers/video/omap2/dss/rfbi.c | 34 ++++++++++++++++++++++++++++------
1 file changed, 28 insertions(+), 6 deletions(-)
diff --git a/drivers/video/omap2/dss/rfbi.c b/drivers/video/omap2/dss/rfbi.c
index 45084d8..847f694 100644
--- a/drivers/video/omap2/dss/rfbi.c
+++ b/drivers/video/omap2/dss/rfbi.c
@@ -859,6 +859,33 @@ static void rfbi_dump_regs(struct seq_file *s)
#undef DUMPREG
}
+static void rfbi_config_lcd_manager(struct omap_dss_device *dssdev)
+{
+ struct dss_lcd_mgr_config mgr_config;
+
+ mgr_config.io_pad_mode = DSS_IO_PAD_MODE_RFBI;
+
+ mgr_config.stallmode = true;
+ /* Do we need fifohandcheck for RFBI? */
+ mgr_config.fifohandcheck = false;
+
+ mgr_config.video_port_width = dssdev->ctrl.pixel_size;
+ mgr_config.lcden_sig_polarity = 0;
+
+ dispc_mgr_set_io_pad_mode(mgr_config.io_pad_mode);
+
+ dispc_mgr_enable_stallmode(dssdev->manager->id, mgr_config.stallmode);
+ dispc_mgr_enable_fifohandcheck(dssdev->manager->id,
+ mgr_config.fifohandcheck);
+
+ dispc_mgr_set_tft_data_lines(dssdev->manager->id,
+ mgr_config.video_port_width);
+
+ dispc_lcd_enable_signal_polarity(mgr_config.lcden_sig_polarity);
+
+ dispc_mgr_set_lcd_type_tft(dssdev->manager->id);
+}
+
int omapdss_rfbi_display_enable(struct omap_dss_device *dssdev)
{
int r;
@@ -885,12 +912,7 @@ int omapdss_rfbi_display_enable(struct omap_dss_device *dssdev)
goto err1;
}
- dispc_mgr_set_lcd_type_tft(dssdev->manager->id);
-
- dispc_mgr_set_io_pad_mode(DSS_IO_PAD_MODE_RFBI);
- dispc_mgr_enable_stallmode(dssdev->manager->id, true);
-
- dispc_mgr_set_tft_data_lines(dssdev->manager->id, dssdev->ctrl.pixel_size);
+ rfbi_config_lcd_manager(dssdev);
rfbi_configure(dssdev->phy.rfbi.channel,
dssdev->ctrl.pixel_size,
--
1.7.9.5
^ permalink raw reply related
* [PATCH 05/12] OMAPDSS: DSI: Configure dss_lcd_mgr_config struct with lcd manager parameters
From: Archit Taneja @ 2012-06-28 14:42 UTC (permalink / raw)
To: tomi.valkeinen; +Cc: linux-fbdev, linux-omap, Archit Taneja
In-Reply-To: <1340893842-10626-1-git-send-email-archit@ti.com>
Create a dss_lcd_mgr_config struct instance in DSI. Fill up all the parameters
of the struct with configurations held by the panel, and the configurations
required by DSI.
Use these to write to the DISPC registers. These direct register writes would be
later replaced by a function which applies the configuration using the shadow
register programming model.
The function dsi_configure_dispc_clocks() is now called in
dsi_display_init_dispc(), this lets all the lcd manager related configurations
happen in the same place. The DISPC_DIVISORo register was written in
dsi_configure_dispc_clock(), now it just fills up the dispc_clock_info parameter
in mgr_config. The clock_info is written later in dsi_display_init_dispc().
Signed-off-by: Archit Taneja <archit@ti.com>
---
drivers/video/omap2/dss/dsi.c | 117 ++++++++++++++++++++++++++++-------------
1 file changed, 80 insertions(+), 37 deletions(-)
diff --git a/drivers/video/omap2/dss/dsi.c b/drivers/video/omap2/dss/dsi.c
index 94ff3aa..df92e24 100644
--- a/drivers/video/omap2/dss/dsi.c
+++ b/drivers/video/omap2/dss/dsi.c
@@ -331,6 +331,8 @@ struct dsi_data {
unsigned num_lanes_used;
unsigned scp_clk_refcount;
+
+ struct dss_lcd_mgr_config mgr_config;
};
struct dsi_packet_sent_handler_data {
@@ -4339,14 +4341,42 @@ EXPORT_SYMBOL(omap_dsi_update);
/* Display funcs */
-static int dsi_display_init_dispc(struct omap_dss_device *dssdev)
+static int dsi_configure_dispc_clocks(struct omap_dss_device *dssdev,
+ const struct omapdss_lcd_clock_config *lcd_clks)
{
+ struct platform_device *dsidev = dsi_get_dsidev_from_dssdev(dssdev);
+ struct dsi_data *dsi = dsi_get_dsidrv_data(dsidev);
+ struct dispc_clock_info dispc_cinfo;
int r;
+ unsigned long long fck;
+
+ fck = dsi_get_pll_hsdiv_dispc_rate(dsidev);
+
+ dispc_cinfo.lck_div = lcd_clks->lck_div;
+ dispc_cinfo.pck_div = lcd_clks->pck_div;
+
+ r = dispc_calc_clock_rates(fck, &dispc_cinfo);
+ if (r) {
+ DSSERR("Failed to calc dispc clocks\n");
+ return r;
+ }
+
+ dsi->mgr_config.clock_info = dispc_cinfo;
+
+ return 0;
+}
+
+static int dsi_display_init_dispc(struct omap_dss_device *dssdev)
+{
+ struct platform_device *dsidev = dsi_get_dsidev_from_dssdev(dssdev);
+ struct dsi_data *dsi = dsi_get_dsidrv_data(dsidev);
struct omap_video_timings timings;
+ const struct omapdss_clock_config *clks;
+ int r, lcd_id;
+ u32 irq;
if (dssdev->panel.dsi_mode = OMAP_DSS_DSI_CMD_MODE) {
u16 dw, dh;
- u32 irq;
dssdev->driver->get_resolution(dssdev, &dw, &dh);
@@ -4366,16 +4396,16 @@ static int dsi_display_init_dispc(struct omap_dss_device *dssdev)
(void *) dssdev, irq);
if (r) {
DSSERR("can't get FRAMEDONE irq\n");
- return r;
+ goto err;
}
- dispc_mgr_enable_stallmode(dssdev->manager->id, true);
- dispc_mgr_enable_fifohandcheck(dssdev->manager->id, 1);
+ dsi->mgr_config.stallmode = true;
+ dsi->mgr_config.fifohandcheck = true;
} else {
timings = dssdev->panel.timings;
- dispc_mgr_enable_stallmode(dssdev->manager->id, false);
- dispc_mgr_enable_fifohandcheck(dssdev->manager->id, 0);
+ dsi->mgr_config.stallmode = false;
+ dsi->mgr_config.fifohandcheck = false;
}
/*
@@ -4391,12 +4421,53 @@ static int dsi_display_init_dispc(struct omap_dss_device *dssdev)
dss_mgr_set_timings(dssdev->manager, &timings);
- dispc_mgr_set_lcd_type_tft(dssdev->manager->id);
+ clks = dss_get_platform_clock_config();
+
+ switch (dssdev->manager->id) {
+ case OMAP_DSS_CHANNEL_LCD:
+ lcd_id = 0;
+ break;
+ case OMAP_DSS_CHANNEL_LCD2:
+ lcd_id = 1;
+ break;
+ default:
+ r = -EINVAL;
+ goto err1;
+ }
+
+ r = dsi_configure_dispc_clocks(dssdev, &clks->lcd[lcd_id]);
+ if (r)
+ goto err1;
+
+ dsi->mgr_config.io_pad_mode = DSS_IO_PAD_MODE_BYPASS;
+ dsi->mgr_config.video_port_width + dsi_get_pixel_size(dssdev->panel.dsi_pix_fmt);
+ dsi->mgr_config.lcden_sig_polarity = 0;
+
+ dispc_mgr_set_io_pad_mode(dsi->mgr_config.io_pad_mode);
+
+ dispc_mgr_enable_stallmode(dssdev->manager->id,
+ dsi->mgr_config.stallmode);
+ dispc_mgr_enable_fifohandcheck(dssdev->manager->id,
+ dsi->mgr_config.fifohandcheck);
+
+ dispc_mgr_set_clock_div(dssdev->manager->id,
+ &dsi->mgr_config.clock_info);
dispc_mgr_set_tft_data_lines(dssdev->manager->id,
- dsi_get_pixel_size(dssdev->panel.dsi_pix_fmt));
+ dsi->mgr_config.video_port_width);
+
+ dispc_lcd_enable_signal_polarity(dsi->mgr_config.lcden_sig_polarity);
+
+ dispc_mgr_set_lcd_type_tft(dssdev->manager->id);
return 0;
+err1:
+ if (dssdev->panel.dsi_mode = OMAP_DSS_DSI_CMD_MODE)
+ omap_dispc_unregister_isr(dsi_framedone_irq_callback,
+ (void *) dssdev, irq);
+err:
+ return r;
}
static void dsi_display_uninit_dispc(struct omap_dss_device *dssdev)
@@ -4437,30 +4508,6 @@ static int dsi_configure_dsi_clocks(struct platform_device *dsidev,
return 0;
}
-static int dsi_configure_dispc_clocks(struct omap_dss_device *dssdev,
- const struct omapdss_lcd_clock_config *lcd_clks)
-{
- struct platform_device *dsidev = dsi_get_dsidev_from_dssdev(dssdev);
- struct dispc_clock_info dispc_cinfo;
- int r;
- unsigned long long fck;
-
- fck = dsi_get_pll_hsdiv_dispc_rate(dsidev);
-
- dispc_cinfo.lck_div = lcd_clks->lck_div;
- dispc_cinfo.pck_div = lcd_clks->pck_div;
-
- r = dispc_calc_clock_rates(fck, &dispc_cinfo);
- if (r) {
- DSSERR("Failed to calc dispc clocks\n");
- return r;
- }
-
- dispc_mgr_set_clock_div(dssdev->manager->id, &dispc_cinfo);
-
- return 0;
-}
-
static int dsi_display_init_dsi(struct omap_dss_device *dssdev)
{
struct platform_device *dsidev = dsi_get_dsidev_from_dssdev(dssdev);
@@ -4499,10 +4546,6 @@ static int dsi_display_init_dsi(struct omap_dss_device *dssdev)
DSSDBG("PLL OK\n");
- r = dsi_configure_dispc_clocks(dssdev, &clks->lcd[lcd_id]);
- if (r)
- goto err2;
-
r = dsi_cio_init(dssdev);
if (r)
goto err2;
--
1.7.9.5
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox