* [PATCH v2 1/4] OMAPDSS: Add DSS features for AM43xx
2014-03-13 8:58 [PATCH v2 0/4] Add Display support for AM43xx Sathya Prakash M R
@ 2014-03-13 8:58 ` Sathya Prakash M R
2014-03-13 8:58 ` [PATCH v2 2/4] ARM: AM43xx: fix dpll init in bypass mode Sathya Prakash M R
` (2 subsequent siblings)
3 siblings, 0 replies; 20+ messages in thread
From: Sathya Prakash M R @ 2014-03-13 8:58 UTC (permalink / raw)
To: tony, tomi.valkeinen, devicetree, linux-omap, rob.herring,
pawel.moll, mark.rutland, paul
Cc: Sathya Prakash M R
Add DSS features for AM43xx.
Signed-off-by: Sathya Prakash M R <sathyap@ti.com>
---
arch/arm/mach-omap2/display.c | 2 +
drivers/video/omap2/dss/dispc.c | 1 +
drivers/video/omap2/dss/dpi.c | 2 +
drivers/video/omap2/dss/dsi.c | 1 +
drivers/video/omap2/dss/dss.c | 11 ++++++
drivers/video/omap2/dss/dss_features.c | 67 ++++++++++++++++++++++++++++++++
include/video/omapdss.h | 1 +
7 files changed, 85 insertions(+)
diff --git a/arch/arm/mach-omap2/display.c b/arch/arm/mach-omap2/display.c
index a4e536b..d1cac1c 100644
--- a/arch/arm/mach-omap2/display.c
+++ b/arch/arm/mach-omap2/display.c
@@ -316,6 +316,8 @@ static enum omapdss_version __init omap_display_get_version(void)
return OMAPDSS_VER_OMAP4;
else if (soc_is_omap54xx())
return OMAPDSS_VER_OMAP5;
+ else if (soc_is_am43xx())
+ return OMAPDSS_VER_AM43xx;
else
return OMAPDSS_VER_UNKNOWN;
}
diff --git a/drivers/video/omap2/dss/dispc.c b/drivers/video/omap2/dss/dispc.c
index 4ec59ca..1b4aed5 100644
--- a/drivers/video/omap2/dss/dispc.c
+++ b/drivers/video/omap2/dss/dispc.c
@@ -3622,6 +3622,7 @@ static int __init dispc_init_features(struct platform_device *pdev)
case OMAPDSS_VER_OMAP34xx_ES3:
case OMAPDSS_VER_OMAP3630:
case OMAPDSS_VER_AM35xx:
+ case OMAPDSS_VER_AM43xx:
src = &omap34xx_rev3_0_dispc_feats;
break;
diff --git a/drivers/video/omap2/dss/dpi.c b/drivers/video/omap2/dss/dpi.c
index bd48cde..7ee7f86 100644
--- a/drivers/video/omap2/dss/dpi.c
+++ b/drivers/video/omap2/dss/dpi.c
@@ -64,6 +64,7 @@ static struct platform_device *dpi_get_dsidev(enum omap_channel channel)
case OMAPDSS_VER_OMAP34xx_ES3:
case OMAPDSS_VER_OMAP3630:
case OMAPDSS_VER_AM35xx:
+ case OMAPDSS_VER_AM43xx:
return NULL;
case OMAPDSS_VER_OMAP4430_ES1:
@@ -593,6 +594,7 @@ static enum omap_channel dpi_get_channel(void)
case OMAPDSS_VER_OMAP34xx_ES3:
case OMAPDSS_VER_OMAP3630:
case OMAPDSS_VER_AM35xx:
+ case OMAPDSS_VER_AM43xx:
return OMAP_DSS_CHANNEL_LCD;
case OMAPDSS_VER_OMAP4430_ES1:
diff --git a/drivers/video/omap2/dss/dsi.c b/drivers/video/omap2/dss/dsi.c
index 6056b27..d68b49b 100644
--- a/drivers/video/omap2/dss/dsi.c
+++ b/drivers/video/omap2/dss/dsi.c
@@ -5082,6 +5082,7 @@ static enum omap_channel dsi_get_channel(int module_id)
{
switch (omapdss_get_version()) {
case OMAPDSS_VER_OMAP24xx:
+ case OMAPDSS_VER_AM43xx:
DSSWARN("DSI not supported\n");
return OMAP_DSS_CHANNEL_LCD;
diff --git a/drivers/video/omap2/dss/dss.c b/drivers/video/omap2/dss/dss.c
index bd01608..0b60746 100644
--- a/drivers/video/omap2/dss/dss.c
+++ b/drivers/video/omap2/dss/dss.c
@@ -795,6 +795,13 @@ static const struct dss_features omap54xx_dss_feats __initconst = {
.dpi_select_source = &dss_dpi_select_source_omap5,
};
+static const struct dss_features am43xx_dss_feats __initconst = {
+ .fck_div_max = 0,
+ .dss_fck_multiplier = 0,
+ .parent_clk_name = NULL,
+ .dpi_select_source = &dss_dpi_select_source_omap2_omap3,
+};
+
static int __init dss_init_features(struct platform_device *pdev)
{
const struct dss_features *src;
@@ -831,6 +838,10 @@ static int __init dss_init_features(struct platform_device *pdev)
src = &omap54xx_dss_feats;
break;
+ case OMAPDSS_VER_AM43xx:
+ src = &am43xx_dss_feats;
+ break;
+
default:
return -ENODEV;
}
diff --git a/drivers/video/omap2/dss/dss_features.c b/drivers/video/omap2/dss/dss_features.c
index f8fd6db..79df1a2 100644
--- a/drivers/video/omap2/dss/dss_features.c
+++ b/drivers/video/omap2/dss/dss_features.c
@@ -93,6 +93,17 @@ static const struct dss_reg_field omap3_dss_reg_fields[] = {
[FEAT_REG_DSIPLL_REGM_DSI] = { 26, 23 },
};
+static const struct dss_reg_field am43xx_dss_reg_fields[] = {
+ [FEAT_REG_FIRHINC] = { 12, 0 },
+ [FEAT_REG_FIRVINC] = { 28, 16 },
+ [FEAT_REG_FIFOLOWTHRESHOLD] = { 11, 0 },
+ [FEAT_REG_FIFOHIGHTHRESHOLD] = { 27, 16 },
+ [FEAT_REG_FIFOSIZE] = { 10, 0 },
+ [FEAT_REG_HORIZONTALACCU] = { 9, 0 },
+ [FEAT_REG_VERTICALACCU] = { 25, 16 },
+ [FEAT_REG_DISPC_CLK_SWITCH] = { 0, 0 },
+};
+
static const struct dss_reg_field omap4_dss_reg_fields[] = {
[FEAT_REG_FIRHINC] = { 12, 0 },
[FEAT_REG_FIRVINC] = { 28, 16 },
@@ -149,6 +160,11 @@ static const enum omap_display_type omap3630_dss_supported_displays[] = {
OMAP_DISPLAY_TYPE_VENC,
};
+static const enum omap_display_type am43xx_dss_supported_displays[] = {
+ /* OMAP_DSS_CHANNEL_LCD */
+ OMAP_DISPLAY_TYPE_DPI | OMAP_DISPLAY_TYPE_DBI,
+};
+
static const enum omap_display_type omap4_dss_supported_displays[] = {
/* OMAP_DSS_CHANNEL_LCD */
OMAP_DISPLAY_TYPE_DBI | OMAP_DISPLAY_TYPE_DSI,
@@ -200,6 +216,11 @@ static const enum omap_dss_output_id omap3630_dss_supported_outputs[] = {
OMAP_DSS_OUTPUT_VENC,
};
+static const enum omap_dss_output_id am43xx_dss_supported_outputs[] = {
+ /* OMAP_DSS_CHANNEL_LCD */
+ OMAP_DSS_OUTPUT_DPI | OMAP_DSS_OUTPUT_DBI,
+};
+
static const enum omap_dss_output_id omap4_dss_supported_outputs[] = {
/* OMAP_DSS_CHANNEL_LCD */
OMAP_DSS_OUTPUT_DBI | OMAP_DSS_OUTPUT_DSI1,
@@ -444,6 +465,13 @@ static const struct dss_param_range omap3_dss_param_range[] = {
[FEAT_PARAM_LINEWIDTH] = { 1, 1024 },
};
+static const struct dss_param_range am43xx_dss_param_range[] = {
+ [FEAT_PARAM_DSS_FCK] = { 0, 200000000 },
+ [FEAT_PARAM_DSS_PCD] = { 2, 255 },
+ [FEAT_PARAM_DOWNSCALE] = { 1, 4 },
+ [FEAT_PARAM_LINEWIDTH] = { 1, 1024 },
+};
+
static const struct dss_param_range omap4_dss_param_range[] = {
[FEAT_PARAM_DSS_FCK] = { 0, 186000000 },
[FEAT_PARAM_DSS_PCD] = { 1, 255 },
@@ -520,6 +548,21 @@ static const enum dss_feat_id am35xx_dss_feat_list[] = {
FEAT_OMAP3_DSI_FIFO_BUG,
};
+static const enum dss_feat_id am43xx_dss_feat_list[] = {
+ FEAT_LCDENABLEPOL,
+ FEAT_LCDENABLESIGNAL,
+ FEAT_PCKFREEENABLE,
+ FEAT_FUNCGATED,
+ FEAT_LINEBUFFERSPLIT,
+ FEAT_ROWREPEATENABLE,
+ FEAT_RESIZECONF,
+ FEAT_CPR,
+ FEAT_PRELOAD,
+ FEAT_FIR_COEF_V,
+ FEAT_ALPHA_FIXED_ZORDER,
+ FEAT_FIFO_MERGE,
+};
+
static const enum dss_feat_id omap3630_dss_feat_list[] = {
FEAT_LCDENABLEPOL,
FEAT_LCDENABLESIGNAL,
@@ -681,6 +724,26 @@ static const struct omap_dss_features am35xx_dss_features = {
.burst_size_unit = 8,
};
+static const struct omap_dss_features am43xx_dss_features = {
+ .reg_fields = am43xx_dss_reg_fields,
+ .num_reg_fields = ARRAY_SIZE(am43xx_dss_reg_fields),
+
+ .features = am43xx_dss_feat_list,
+ .num_features = ARRAY_SIZE(am43xx_dss_feat_list),
+
+ .num_mgrs = 1,
+ .num_ovls = 3,
+ .supported_displays = am43xx_dss_supported_displays,
+ .supported_outputs = am43xx_dss_supported_outputs,
+ .supported_color_modes = omap3_dss_supported_color_modes,
+ .overlay_caps = omap3430_dss_overlay_caps,
+ .clksrc_names = omap2_dss_clk_source_names,
+ .dss_params = am43xx_dss_param_range,
+ .supported_rotation_types = OMAP_DSS_ROT_DMA,
+ .buffer_size_unit = 1,
+ .burst_size_unit = 8,
+};
+
static const struct omap_dss_features omap3630_dss_features = {
.reg_fields = omap3_dss_reg_fields,
.num_reg_fields = ARRAY_SIZE(omap3_dss_reg_fields),
@@ -927,6 +990,10 @@ void dss_features_init(enum omapdss_version version)
omap_current_dss_features = &am35xx_dss_features;
break;
+ case OMAPDSS_VER_AM43xx:
+ omap_current_dss_features = &am43xx_dss_features;
+ break;
+
default:
DSSWARN("Unsupported OMAP version");
break;
diff --git a/include/video/omapdss.h b/include/video/omapdss.h
index 3d7c51a..7513146 100644
--- a/include/video/omapdss.h
+++ b/include/video/omapdss.h
@@ -319,6 +319,7 @@ enum omapdss_version {
OMAPDSS_VER_OMAP4430_ES2, /* OMAP4430 ES2.0, 2.1, 2.2 */
OMAPDSS_VER_OMAP4, /* All other OMAP4s */
OMAPDSS_VER_OMAP5,
+ OMAPDSS_VER_AM43xx,
};
/* Board specific data */
--
1.7.9.5
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH v2 2/4] ARM: AM43xx: fix dpll init in bypass mode
2014-03-13 8:58 [PATCH v2 0/4] Add Display support for AM43xx Sathya Prakash M R
2014-03-13 8:58 ` [PATCH v2 1/4] OMAPDSS: Add DSS features " Sathya Prakash M R
@ 2014-03-13 8:58 ` Sathya Prakash M R
2014-03-13 10:24 ` Tomi Valkeinen
2014-03-13 8:58 ` [PATCH v2 3/4] ARM: OMAP2+: AM43xx DSS Hwmod Sathya Prakash M R
2014-03-13 8:58 ` [PATCH v2 4/4] ARM: DTS: AM43x: Add DSS node Sathya Prakash M R
3 siblings, 1 reply; 20+ messages in thread
From: Sathya Prakash M R @ 2014-03-13 8:58 UTC (permalink / raw)
To: tony, tomi.valkeinen, devicetree, linux-omap, rob.herring,
pawel.moll, mark.rutland, paul
From: Tomi Valkeinen <tomi.valkeinen@ti.com>
On AM43xx, if a PLL is in bypass at kernel init, the code in
omap2_get_dpll_rate() will not realize this and will try to calculate
the clock rate using the multiplier and the divider, resulting in
errors.
omap2_init_dpll_parent() has similar issue.
Add the missing soc_is_am43xx() check to make the code work on AM43xx.
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
arch/arm/mach-omap2/clkt_dpll.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arm/mach-omap2/clkt_dpll.c b/arch/arm/mach-omap2/clkt_dpll.c
index 924c230..14e9e45 100644
--- a/arch/arm/mach-omap2/clkt_dpll.c
+++ b/arch/arm/mach-omap2/clkt_dpll.c
@@ -209,7 +209,7 @@ u8 omap2_init_dpll_parent(struct clk_hw *hw)
if (v == OMAP3XXX_EN_DPLL_LPBYPASS ||
v == OMAP3XXX_EN_DPLL_FRBYPASS)
return 1;
- } else if (soc_is_am33xx() || cpu_is_omap44xx()) {
+ } else if (soc_is_am33xx() || cpu_is_omap44xx() || soc_is_am43xx()) {
if (v == OMAP4XXX_EN_DPLL_LPBYPASS ||
v == OMAP4XXX_EN_DPLL_FRBYPASS ||
v == OMAP4XXX_EN_DPLL_MNBYPASS)
@@ -255,7 +255,7 @@ unsigned long omap2_get_dpll_rate(struct clk_hw_omap *clk)
if (v == OMAP3XXX_EN_DPLL_LPBYPASS ||
v == OMAP3XXX_EN_DPLL_FRBYPASS)
return __clk_get_rate(dd->clk_bypass);
- } else if (soc_is_am33xx() || cpu_is_omap44xx()) {
+ } else if (soc_is_am33xx() || cpu_is_omap44xx() || soc_is_am43xx()) {
if (v == OMAP4XXX_EN_DPLL_LPBYPASS ||
v == OMAP4XXX_EN_DPLL_FRBYPASS ||
v == OMAP4XXX_EN_DPLL_MNBYPASS)
--
1.7.9.5
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v2 2/4] ARM: AM43xx: fix dpll init in bypass mode
2014-03-13 8:58 ` [PATCH v2 2/4] ARM: AM43xx: fix dpll init in bypass mode Sathya Prakash M R
@ 2014-03-13 10:24 ` Tomi Valkeinen
2014-03-13 10:32 ` Sathya Prakash
0 siblings, 1 reply; 20+ messages in thread
From: Tomi Valkeinen @ 2014-03-13 10:24 UTC (permalink / raw)
To: Sathya Prakash M R
Cc: tony, devicetree, linux-omap, rob.herring, pawel.moll,
mark.rutland, paul
[-- Attachment #1: Type: text/plain, Size: 629 bytes --]
On 13/03/14 10:58, Sathya Prakash M R wrote:
> From: Tomi Valkeinen <tomi.valkeinen@ti.com>
>
> On AM43xx, if a PLL is in bypass at kernel init, the code in
> omap2_get_dpll_rate() will not realize this and will try to calculate
> the clock rate using the multiplier and the divider, resulting in
> errors.
>
> omap2_init_dpll_parent() has similar issue.
>
> Add the missing soc_is_am43xx() check to make the code work on AM43xx.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
I see you sent this already in December, and Paul asked for your
signed-off for it, but you never replied.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 2/4] ARM: AM43xx: fix dpll init in bypass mode
2014-03-13 10:24 ` Tomi Valkeinen
@ 2014-03-13 10:32 ` Sathya Prakash
0 siblings, 0 replies; 20+ messages in thread
From: Sathya Prakash @ 2014-03-13 10:32 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: Sathya Prakash M R, tony, devicetree, linux-omap, paul
On Thursday 13 March 2014 03:54 PM, Tomi Valkeinen wrote:
> On 13/03/14 10:58, Sathya Prakash M R wrote:
>> From: Tomi Valkeinen <tomi.valkeinen@ti.com>
>>
>> On AM43xx, if a PLL is in bypass at kernel init, the code in
>> omap2_get_dpll_rate() will not realize this and will try to calculate
>> the clock rate using the multiplier and the divider, resulting in
>> errors.
>>
>> omap2_init_dpll_parent() has similar issue.
>>
>> Add the missing soc_is_am43xx() check to make the code work on AM43xx.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> I see you sent this already in December, and Paul asked for your
> signed-off for it, but you never replied.
just saw. Maybe i missed it during december vacation.
Will update it in v3 alongwith other changes you suggested.
>
> Tomi
>
Sathya
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v2 3/4] ARM: OMAP2+: AM43xx DSS Hwmod
2014-03-13 8:58 [PATCH v2 0/4] Add Display support for AM43xx Sathya Prakash M R
2014-03-13 8:58 ` [PATCH v2 1/4] OMAPDSS: Add DSS features " Sathya Prakash M R
2014-03-13 8:58 ` [PATCH v2 2/4] ARM: AM43xx: fix dpll init in bypass mode Sathya Prakash M R
@ 2014-03-13 8:58 ` Sathya Prakash M R
2014-03-13 8:58 ` [PATCH v2 4/4] ARM: DTS: AM43x: Add DSS node Sathya Prakash M R
3 siblings, 0 replies; 20+ messages in thread
From: Sathya Prakash M R @ 2014-03-13 8:58 UTC (permalink / raw)
To: tony, tomi.valkeinen, devicetree, linux-omap, rob.herring,
pawel.moll, mark.rutland, paul
Cc: Sathya Prakash M R
Add DSS hwmod structs for AM43xx.
Signed-off-by: Sathya Prakash M R <sathyap@ti.com>
---
arch/arm/mach-omap2/omap_hwmod_43xx_data.c | 104 ++++++++++++++++++++++++++++
arch/arm/mach-omap2/prcm43xx.h | 1 +
2 files changed, 105 insertions(+)
diff --git a/arch/arm/mach-omap2/omap_hwmod_43xx_data.c b/arch/arm/mach-omap2/omap_hwmod_43xx_data.c
index 9002fca..6a121db 100644
--- a/arch/arm/mach-omap2/omap_hwmod_43xx_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_43xx_data.c
@@ -19,6 +19,8 @@
#include "omap_hwmod.h"
#include "omap_hwmod_33xx_43xx_common_data.h"
#include "prcm43xx.h"
+#include "omap_hwmod_common_data.h"
+
/* IP blocks */
static struct omap_hwmod am43xx_l4_hs_hwmod = {
@@ -415,6 +417,76 @@ static struct omap_hwmod am43xx_qspi_hwmod = {
},
};
+/* Display sub system - DSS */
+
+static struct omap_hwmod_dma_info am43xx_dss_sdma_chs[] = {
+ { .name = "dispc", .dma_req = 5 },
+ { .dma_req = -1 },
+};
+
+struct omap_dss_dispc_dev_attr am43xx_dss_dispc_dev_attr = {
+ .manager_count = 1,
+ .has_framedonetv_irq = 0
+};
+
+
+static struct omap_hwmod_class_sysconfig am43xx_dispc_sysc = {
+ .rev_offs = 0x0000,
+ .sysc_offs = 0x0010,
+ .syss_offs = 0x0014,
+ .sysc_flags = (SYSC_HAS_SIDLEMODE | SYSC_HAS_MIDLEMODE),
+ .idlemodes = (SIDLE_FORCE | SIDLE_NO | SIDLE_SMART),
+ .sysc_fields = &omap_hwmod_sysc_type1,
+};
+
+static struct omap_hwmod_class am43xx_dispc_hwmod_class = {
+ .name = "dispc",
+ .sysc = &am43xx_dispc_sysc,
+};
+
+static struct omap_hwmod am43xx_dss_core_hwmod = {
+ .name = "dss_core",
+ .class = &omap2_dss_hwmod_class,
+ .clkdm_name = "dss_clkdm",
+ .main_clk = "disp_clk",
+ .sdma_reqs = am43xx_dss_sdma_chs,
+ .prcm = {
+ .omap4 = {
+ .clkctrl_offs = AM43XX_CM_PER_DSS_CLKCTRL_OFFSET,
+ .modulemode = MODULEMODE_SWCTRL,
+ },
+ },
+};
+
+/* display controller -dispc*/
+
+static struct omap_hwmod am43xx_dss_dispc_hwmod = {
+ .name = "dss_dispc",
+ .class = &am43xx_dispc_hwmod_class,
+ .clkdm_name = "dss_clkdm",
+ .main_clk = "disp_clk",
+ .prcm = {
+ .omap4 = {
+ .clkctrl_offs = AM43XX_CM_PER_DSS_CLKCTRL_OFFSET,
+ },
+ },
+ .dev_attr = &am43xx_dss_dispc_dev_attr,
+};
+
+/*RFBI*/
+
+static struct omap_hwmod am43xx_dss_rfbi_hwmod = {
+ .name = "dss_rfbi",
+ .class = &omap2_rfbi_hwmod_class,
+ .clkdm_name = "dss_clkdm",
+ .main_clk = "disp_clk",
+ .prcm = {
+ .omap4 = {
+ .clkctrl_offs = AM43XX_CM_PER_DSS_CLKCTRL_OFFSET,
+ },
+ },
+};
+
/* Interfaces */
static struct omap_hwmod_ocp_if am43xx_l3_main__l4_hs = {
.master = &am33xx_l3_main_hwmod,
@@ -654,6 +726,34 @@ static struct omap_hwmod_ocp_if am43xx_l3_s__qspi = {
.user = OCP_USER_MPU | OCP_USER_SDMA,
};
+static struct omap_hwmod_ocp_if am43xx_dss__l3_main = {
+ .master = &am43xx_dss_core_hwmod,
+ .slave = &am33xx_l3_main_hwmod,
+ .clk = "disp_clk",
+ .user = OCP_USER_MPU | OCP_USER_SDMA,
+};
+
+static struct omap_hwmod_ocp_if am43xx_l4_ls__dss = {
+ .master = &am33xx_l4_ls_hwmod,
+ .slave = &am43xx_dss_core_hwmod,
+ .clk = "l4ls_gclk",
+ .user = OCP_USER_MPU | OCP_USER_SDMA,
+};
+
+static struct omap_hwmod_ocp_if am43xx_l4_ls__dss_dispc = {
+ .master = &am33xx_l4_ls_hwmod,
+ .slave = &am43xx_dss_dispc_hwmod,
+ .clk = "l4ls_gclk",
+ .user = OCP_USER_MPU | OCP_USER_SDMA,
+};
+
+static struct omap_hwmod_ocp_if am43xx_l4_ls__dss_rfbi = {
+ .master = &am33xx_l4_ls_hwmod,
+ .slave = &am43xx_dss_rfbi_hwmod,
+ .clk = "l4ls_gclk",
+ .user = OCP_USER_MPU | OCP_USER_SDMA,
+};
+
static struct omap_hwmod_ocp_if *am43xx_hwmod_ocp_ifs[] __initdata = {
&am33xx_l4_wkup__synctimer,
&am43xx_l4_ls__timer8,
@@ -747,6 +847,10 @@ static struct omap_hwmod_ocp_if *am43xx_hwmod_ocp_ifs[] __initdata = {
&am43xx_l4_ls__ocp2scp1,
&am43xx_l3_s__usbotgss0,
&am43xx_l3_s__usbotgss1,
+ &am43xx_dss__l3_main,
+ &am43xx_l4_ls__dss,
+ &am43xx_l4_ls__dss_dispc,
+ &am43xx_l4_ls__dss_rfbi,
NULL,
};
diff --git a/arch/arm/mach-omap2/prcm43xx.h b/arch/arm/mach-omap2/prcm43xx.h
index 7785be9..ad7b3e9 100644
--- a/arch/arm/mach-omap2/prcm43xx.h
+++ b/arch/arm/mach-omap2/prcm43xx.h
@@ -142,5 +142,6 @@
#define AM43XX_CM_PER_USBPHYOCP2SCP0_CLKCTRL_OFFSET 0x05B8
#define AM43XX_CM_PER_USB_OTG_SS1_CLKCTRL_OFFSET 0x0268
#define AM43XX_CM_PER_USBPHYOCP2SCP1_CLKCTRL_OFFSET 0x05C0
+#define AM43XX_CM_PER_DSS_CLKCTRL_OFFSET 0x0a20
#endif
--
1.7.9.5
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH v2 4/4] ARM: DTS: AM43x: Add DSS node
2014-03-13 8:58 [PATCH v2 0/4] Add Display support for AM43xx Sathya Prakash M R
` (2 preceding siblings ...)
2014-03-13 8:58 ` [PATCH v2 3/4] ARM: OMAP2+: AM43xx DSS Hwmod Sathya Prakash M R
@ 2014-03-13 8:58 ` Sathya Prakash M R
2014-03-13 10:21 ` Tomi Valkeinen
2014-03-13 17:46 ` Mark Rutland
3 siblings, 2 replies; 20+ messages in thread
From: Sathya Prakash M R @ 2014-03-13 8:58 UTC (permalink / raw)
To: tony, tomi.valkeinen, devicetree, linux-omap, rob.herring,
pawel.moll, mark.rutland, paul
Cc: Sathya Prakash M R
Add device node for DSS module for AM4372. Both the
AM437x-Gp evm and Am43x-Epos evm use the same LCD panel.
The lcd timings are added in respective dts files.
Adds display pinctrl and enables required gpio.
Also set the right parent clock to the DSS clock.
Signed-off-by: Sathya Prakash M R <sathyap@ti.com>
---
arch/arm/boot/dts/am4372.dtsi | 28 +++++++++++++
arch/arm/boot/dts/am437x-gp-evm.dts | 77 ++++++++++++++++++++++++++++++++++
arch/arm/boot/dts/am43x-epos-evm.dts | 73 ++++++++++++++++++++++++++++++++
arch/arm/boot/dts/am43xx-clocks.dtsi | 2 +
4 files changed, 180 insertions(+)
diff --git a/arch/arm/boot/dts/am4372.dtsi b/arch/arm/boot/dts/am4372.dtsi
index ea55a4e..b72a7df 100644
--- a/arch/arm/boot/dts/am4372.dtsi
+++ b/arch/arm/boot/dts/am4372.dtsi
@@ -684,6 +684,34 @@
num-cs = <4>;
status = "disabled";
};
+
+ dss: dss@4832A000 {
+ compatible = "ti,omap3-dss", "simple-bus";
+ reg = <0x4832A000 0x200>;
+ ti,hwmods = "dss_core";
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges;
+
+ dispc@4832A400 {
+ compatible = "ti,omap3-dispc";
+ reg = <0x4832A400 0x400>;
+ interrupts = <GIC_SPI 127 IRQ_TYPE_LEVEL_HIGH>;
+ ti,hwmods = "dss_dispc";
+ };
+
+ dpi: encoder@0 {
+ compatible = "ti,omap3-dpi";
+ };
+
+ rfbi: rfbi@4832A800 {
+ compatible = "ti,omap3-rfbi";
+ reg = <0x4832A800 0x100>;
+ ti,hwmods = "dss_rfbi";
+ };
+
+ };
+
};
clocks {
diff --git a/arch/arm/boot/dts/am437x-gp-evm.dts b/arch/arm/boot/dts/am437x-gp-evm.dts
index 2e79bda..a178e8d 100644
--- a/arch/arm/boot/dts/am437x-gp-evm.dts
+++ b/arch/arm/boot/dts/am437x-gp-evm.dts
@@ -24,6 +24,33 @@
brightness-levels = <0 51 53 56 62 75 101 152 255>;
default-brightness-level = <8>;
};
+
+ aliases {
+ display0 = &lcd0;
+ };
+
+ lcd0: display {
+ compatible = "osddisplays,osd057T0559-34ts", "panel-dpi";
+ label = "lcd";
+ panel-timing {
+ clock-frequency = <33000000>;
+ hactive = <800>;
+ vactive = <480>;
+ hfront-porch = <210>;
+ hback-porch = <16>;
+ hsync-len = <30>;
+ vback-porch = <10>;
+ vfront-porch = <22>;
+ vsync-len = <13>;
+ hsync-active = <0>;
+ vsync-active = <0>;
+ de-active = <1>;
+ pixelclk-active = <1>;
+ };
+ lcd_in: endpoint {
+ remote-endpoint = <&dpi_out>;
+ };
+ };
};
&am43xx_pinmux {
@@ -46,6 +73,40 @@
0x164 MUX_MODE0 /* eCAP0_in_PWM0_out.eCAP0_in_PWM0_out MODE0 */
>;
};
+
+ dss_pinctrl: dss_pinctrl {
+ pinctrl-single,pins = <
+ 0x020 (PIN_OUTPUT_PULLUP | MUX_MODE1) /*gpmc ad 8 -> DSS DATA 23 */
+ 0x024 (PIN_OUTPUT_PULLUP | MUX_MODE1)
+ 0x028 (PIN_OUTPUT_PULLUP | MUX_MODE1)
+ 0x02C (PIN_OUTPUT_PULLUP | MUX_MODE1)
+ 0x030 (PIN_OUTPUT_PULLUP | MUX_MODE1)
+ 0x034 (PIN_OUTPUT_PULLUP | MUX_MODE1)
+ 0x038 (PIN_OUTPUT_PULLUP | MUX_MODE1)
+ 0x03C (PIN_OUTPUT_PULLUP | MUX_MODE1) /*gpmc ad 15 -> DSS DATA 16 */
+ 0x0A0 (PIN_OUTPUT_PULLUP | MUX_MODE0) /* DSS DATA 0 */
+ 0x0A4 (PIN_OUTPUT_PULLUP | MUX_MODE0)
+ 0x0A8 (PIN_OUTPUT_PULLUP | MUX_MODE0)
+ 0x0AC (PIN_OUTPUT_PULLUP | MUX_MODE0)
+ 0x0B0 (PIN_OUTPUT_PULLUP | MUX_MODE0)
+ 0x0B4 (PIN_OUTPUT_PULLUP | MUX_MODE0)
+ 0x0B8 (PIN_OUTPUT_PULLUP | MUX_MODE0)
+ 0x0BC (PIN_OUTPUT_PULLUP | MUX_MODE0)
+ 0x0C0 (PIN_OUTPUT_PULLUP | MUX_MODE0)
+ 0x0C4 (PIN_OUTPUT_PULLUP | MUX_MODE0)
+ 0x0C8 (PIN_OUTPUT_PULLUP | MUX_MODE0)
+ 0x0CC (PIN_OUTPUT_PULLUP | MUX_MODE0)
+ 0x0D0 (PIN_OUTPUT_PULLUP | MUX_MODE0)
+ 0x0D4 (PIN_OUTPUT_PULLUP | MUX_MODE0)
+ 0x0D8 (PIN_OUTPUT_PULLUP | MUX_MODE0)
+ 0x0DC (PIN_OUTPUT_PULLUP | MUX_MODE0) /* DSS DATA 15 */
+ 0x0E0 (PIN_OUTPUT_PULLUP | MUX_MODE0) /* DSS VSYNC */
+ 0x0E4 (PIN_OUTPUT_PULLUP | MUX_MODE0) /* DSS HSYNC */
+ 0x0E8 (PIN_OUTPUT_PULLUP | MUX_MODE0) /* DSS PCLK */
+ 0x0EC (PIN_OUTPUT_PULLUP | MUX_MODE0) /* DSS AC BIAS EN */
+ 0x238 (PIN_OUTPUT_PULLUP | MUX_MODE7) /* GPIO 5_8 to select LCD / HDMI */
+ >;
+ };
};
&i2c0 {
@@ -69,3 +130,19 @@
pinctrl-names = "default";
pinctrl-0 = <&ecap0_pins>;
};
+
+&gpio5 {
+ status = "okay";
+};
+
+&dss {
+ status = "ok";
+
+ pinctrl-names = "default";
+ pinctrl-0 = <&dss_pinctrl>;
+
+ dpi_out: endpoint@0 {
+ remote-endpoint = <&lcd_in>;
+ data-lines = <24>;
+ };
+};
diff --git a/arch/arm/boot/dts/am43x-epos-evm.dts b/arch/arm/boot/dts/am43x-epos-evm.dts
index 2ebcde6..3f9643b 100644
--- a/arch/arm/boot/dts/am43x-epos-evm.dts
+++ b/arch/arm/boot/dts/am43x-epos-evm.dts
@@ -27,6 +27,33 @@
enable-active-high;
};
+ aliases {
+ display0 = &lcd0;
+ };
+
+ lcd0: display {
+ compatible = "osddisplays,osd057T0559-34ts", "panel-dpi";
+ label = "lcd";
+ panel-timing {
+ clock-frequency = <33000000>;
+ hactive = <800>;
+ vactive = <480>;
+ hfront-porch = <210>;
+ hback-porch = <16>;
+ hsync-len = <30>;
+ vback-porch = <10>;
+ vfront-porch = <22>;
+ vsync-len = <13>;
+ hsync-active = <0>;
+ vsync-active = <0>;
+ de-active = <1>;
+ pixelclk-active = <1>;
+ };
+ lcd_in: endpoint {
+ remote-endpoint = <&dpi_out>;
+ };
+ };
+
am43xx_pinmux: pinmux@44e10800 {
cpsw_default: cpsw_default {
pinctrl-single,pins = <
@@ -122,6 +149,40 @@
0x19c (PIN_OUTPUT | MUX_MODE3) /* mcasp0_ahclkr.spi1_cs0 */
>;
};
+
+ dss_pinctrl: dss_pinctrl {
+ pinctrl-single,pins = <
+ 0x020 (PIN_OUTPUT_PULLUP | MUX_MODE1) /*gpmc ad 8 -> DSS DATA 23 */
+ 0x024 (PIN_OUTPUT_PULLUP | MUX_MODE1)
+ 0x028 (PIN_OUTPUT_PULLUP | MUX_MODE1)
+ 0x02C (PIN_OUTPUT_PULLUP | MUX_MODE1)
+ 0x030 (PIN_OUTPUT_PULLUP | MUX_MODE1)
+ 0x034 (PIN_OUTPUT_PULLUP | MUX_MODE1)
+ 0x038 (PIN_OUTPUT_PULLUP | MUX_MODE1)
+ 0x03C (PIN_OUTPUT_PULLUP | MUX_MODE1) /*gpmc ad 15 -> DSS DATA 16 */
+ 0x0A0 (PIN_OUTPUT_PULLUP | MUX_MODE0) /* DSS DATA 0 */
+ 0x0A4 (PIN_OUTPUT_PULLUP | MUX_MODE0)
+ 0x0A8 (PIN_OUTPUT_PULLUP | MUX_MODE0)
+ 0x0AC (PIN_OUTPUT_PULLUP | MUX_MODE0)
+ 0x0B0 (PIN_OUTPUT_PULLUP | MUX_MODE0)
+ 0x0B4 (PIN_OUTPUT_PULLUP | MUX_MODE0)
+ 0x0B8 (PIN_OUTPUT_PULLUP | MUX_MODE0)
+ 0x0BC (PIN_OUTPUT_PULLUP | MUX_MODE0)
+ 0x0C0 (PIN_OUTPUT_PULLUP | MUX_MODE0)
+ 0x0C4 (PIN_OUTPUT_PULLUP | MUX_MODE0)
+ 0x0C8 (PIN_OUTPUT_PULLUP | MUX_MODE0)
+ 0x0CC (PIN_OUTPUT_PULLUP | MUX_MODE0)
+ 0x0D0 (PIN_OUTPUT_PULLUP | MUX_MODE0)
+ 0x0D4 (PIN_OUTPUT_PULLUP | MUX_MODE0)
+ 0x0D8 (PIN_OUTPUT_PULLUP | MUX_MODE0)
+ 0x0DC (PIN_OUTPUT_PULLUP | MUX_MODE0) /* DSS DATA 15 */
+ 0x0E0 (PIN_OUTPUT_PULLUP | MUX_MODE0) /* DSS VSYNC */
+ 0x0E4 (PIN_OUTPUT_PULLUP | MUX_MODE0) /* DSS HSYNC */
+ 0x0E8 (PIN_OUTPUT_PULLUP | MUX_MODE0) /* DSS PCLK */
+ 0x0EC (PIN_OUTPUT_PULLUP | MUX_MODE0) /* DSS AC BIAS EN */
+ 0x08C (PIN_OUTPUT_PULLUP | MUX_MODE7) /* GPMC CLK -> GPIO 2_1 to select LCD / HDMI */
+ >;
+ };
};
matrix_keypad: matrix_keypad@0 {
@@ -279,3 +340,15 @@
pinctrl-0 = <&spi1_pins>;
status = "okay";
};
+
+&dss {
+ status = "ok";
+
+ pinctrl-names = "default";
+ pinctrl-0 = <&dss_pinctrl>;
+
+ dpi_out: endpoint@0 {
+ remote-endpoint = <&lcd_in>;
+ data-lines = <24>;
+ };
+};
diff --git a/arch/arm/boot/dts/am43xx-clocks.dtsi b/arch/arm/boot/dts/am43xx-clocks.dtsi
index 85e7d4b..b20e192 100644
--- a/arch/arm/boot/dts/am43xx-clocks.dtsi
+++ b/arch/arm/boot/dts/am43xx-clocks.dtsi
@@ -512,6 +512,8 @@ disp_clk: disp_clk@44df4244 {
compatible = "ti,mux-clock";
clocks = <&dpll_disp_m2_ck>, <&dpll_core_m5_ck>, <&dpll_per_m2_ck>;
reg = <0x44df4244 0x4>;
+ bit-mask = <0x3>;
+ set-rate-parent;
};
dpll_extdev_ck: dpll_extdev_ck@44df2e60 {
--
1.7.9.5
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v2 4/4] ARM: DTS: AM43x: Add DSS node
2014-03-13 8:58 ` [PATCH v2 4/4] ARM: DTS: AM43x: Add DSS node Sathya Prakash M R
@ 2014-03-13 10:21 ` Tomi Valkeinen
2014-03-13 10:30 ` Sathya Prakash
2014-03-13 17:46 ` Mark Rutland
1 sibling, 1 reply; 20+ messages in thread
From: Tomi Valkeinen @ 2014-03-13 10:21 UTC (permalink / raw)
To: Sathya Prakash M R
Cc: tony, devicetree, linux-omap, rob.herring, pawel.moll,
mark.rutland, paul
[-- Attachment #1: Type: text/plain, Size: 1971 bytes --]
On 13/03/14 10:58, Sathya Prakash M R wrote:
> Add device node for DSS module for AM4372. Both the
> AM437x-Gp evm and Am43x-Epos evm use the same LCD panel.
> The lcd timings are added in respective dts files.
> Adds display pinctrl and enables required gpio.
> Also set the right parent clock to the DSS clock.
>
> Signed-off-by: Sathya Prakash M R <sathyap@ti.com>
> ---
> arch/arm/boot/dts/am4372.dtsi | 28 +++++++++++++
> arch/arm/boot/dts/am437x-gp-evm.dts | 77 ++++++++++++++++++++++++++++++++++
> arch/arm/boot/dts/am43x-epos-evm.dts | 73 ++++++++++++++++++++++++++++++++
> arch/arm/boot/dts/am43xx-clocks.dtsi | 2 +
> 4 files changed, 180 insertions(+)
>
> diff --git a/arch/arm/boot/dts/am4372.dtsi b/arch/arm/boot/dts/am4372.dtsi
> index ea55a4e..b72a7df 100644
> --- a/arch/arm/boot/dts/am4372.dtsi
> +++ b/arch/arm/boot/dts/am4372.dtsi
> @@ -684,6 +684,34 @@
> num-cs = <4>;
> status = "disabled";
> };
> +
> + dss: dss@4832A000 {
> + compatible = "ti,omap3-dss", "simple-bus";
> + reg = <0x4832A000 0x200>;
> + ti,hwmods = "dss_core";
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges;
> +
> + dispc@4832A400 {
> + compatible = "ti,omap3-dispc";
> + reg = <0x4832A400 0x400>;
> + interrupts = <GIC_SPI 127 IRQ_TYPE_LEVEL_HIGH>;
> + ti,hwmods = "dss_dispc";
> + };
> +
> + dpi: encoder@0 {
> + compatible = "ti,omap3-dpi";
> + };
> +
> + rfbi: rfbi@4832A800 {
> + compatible = "ti,omap3-rfbi";
> + reg = <0x4832A800 0x100>;
> + ti,hwmods = "dss_rfbi";
> + };
> +
> + };
> +
This seems to be based on old version of the DSS DT. Please rebase this
on top of the latest one:
git://git.kernel.org/pub/scm/linux/kernel/git/tomba/linux.git work/dss-dt
You also need to split this into smaller pieces: the dts clock fix, the
am4372.dtsi, and the board changes.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v2 4/4] ARM: DTS: AM43x: Add DSS node
2014-03-13 10:21 ` Tomi Valkeinen
@ 2014-03-13 10:30 ` Sathya Prakash
0 siblings, 0 replies; 20+ messages in thread
From: Sathya Prakash @ 2014-03-13 10:30 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Sathya Prakash M R, tony, devicetree, linux-omap, rob.herring,
pawel.moll, mark.rutland, paul
On Thursday 13 March 2014 03:51 PM, Tomi Valkeinen wrote:
> On 13/03/14 10:58, Sathya Prakash M R wrote:
>> Add device node for DSS module for AM4372. Both the
>> AM437x-Gp evm and Am43x-Epos evm use the same LCD panel.
>> The lcd timings are added in respective dts files.
>> Adds display pinctrl and enables required gpio.
>> Also set the right parent clock to the DSS clock.
>>
>> Signed-off-by: Sathya Prakash M R <sathyap@ti.com>
>> ---
>> arch/arm/boot/dts/am4372.dtsi | 28 +++++++++++++
>> arch/arm/boot/dts/am437x-gp-evm.dts | 77 ++++++++++++++++++++++++++++++++++
>> arch/arm/boot/dts/am43x-epos-evm.dts | 73 ++++++++++++++++++++++++++++++++
>> arch/arm/boot/dts/am43xx-clocks.dtsi | 2 +
>> 4 files changed, 180 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/am4372.dtsi b/arch/arm/boot/dts/am4372.dtsi
>> index ea55a4e..b72a7df 100644
>> --- a/arch/arm/boot/dts/am4372.dtsi
>> +++ b/arch/arm/boot/dts/am4372.dtsi
>> @@ -684,6 +684,34 @@
>> num-cs = <4>;
>> status = "disabled";
>> };
>> +
>> + dss: dss@4832A000 {
>> + compatible = "ti,omap3-dss", "simple-bus";
>> + reg = <0x4832A000 0x200>;
>> + ti,hwmods = "dss_core";
>> + #address-cells = <1>;
>> + #size-cells = <1>;
>> + ranges;
>> +
>> + dispc@4832A400 {
>> + compatible = "ti,omap3-dispc";
>> + reg = <0x4832A400 0x400>;
>> + interrupts = <GIC_SPI 127 IRQ_TYPE_LEVEL_HIGH>;
>> + ti,hwmods = "dss_dispc";
>> + };
>> +
>> + dpi: encoder@0 {
>> + compatible = "ti,omap3-dpi";
>> + };
>> +
>> + rfbi: rfbi@4832A800 {
>> + compatible = "ti,omap3-rfbi";
>> + reg = <0x4832A800 0x100>;
>> + ti,hwmods = "dss_rfbi";
>> + };
>> +
>> + };
>> +
> This seems to be based on old version of the DSS DT. Please rebase this
> on top of the latest one:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/tomba/linux.git work/dss-dt
Thanks. maybe i missed couple of changes. Will do that.
>
> You also need to split this into smaller pieces: the dts clock fix, the
> am4372.dtsi, and the board changes.
ok sure.
>
> Tomi
>
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 4/4] ARM: DTS: AM43x: Add DSS node
2014-03-13 8:58 ` [PATCH v2 4/4] ARM: DTS: AM43x: Add DSS node Sathya Prakash M R
2014-03-13 10:21 ` Tomi Valkeinen
@ 2014-03-13 17:46 ` Mark Rutland
2014-03-13 18:22 ` Tomi Valkeinen
1 sibling, 1 reply; 20+ messages in thread
From: Mark Rutland @ 2014-03-13 17:46 UTC (permalink / raw)
To: Sathya Prakash M R
Cc: tony@atomide.com, tomi.valkeinen@ti.com,
devicetree@vger.kernel.org, linux-omap@vger.kernel.org,
rob.herring@calxeda.com, Pawel Moll, paul@pwsan.com
On Thu, Mar 13, 2014 at 08:58:29AM +0000, Sathya Prakash M R wrote:
> Add device node for DSS module for AM4372. Both the
> AM437x-Gp evm and Am43x-Epos evm use the same LCD panel.
> The lcd timings are added in respective dts files.
> Adds display pinctrl and enables required gpio.
> Also set the right parent clock to the DSS clock.
>
> Signed-off-by: Sathya Prakash M R <sathyap@ti.com>
> ---
> arch/arm/boot/dts/am4372.dtsi | 28 +++++++++++++
> arch/arm/boot/dts/am437x-gp-evm.dts | 77 ++++++++++++++++++++++++++++++++++
> arch/arm/boot/dts/am43x-epos-evm.dts | 73 ++++++++++++++++++++++++++++++++
> arch/arm/boot/dts/am43xx-clocks.dtsi | 2 +
> 4 files changed, 180 insertions(+)
>
> diff --git a/arch/arm/boot/dts/am4372.dtsi b/arch/arm/boot/dts/am4372.dtsi
> index ea55a4e..b72a7df 100644
> --- a/arch/arm/boot/dts/am4372.dtsi
> +++ b/arch/arm/boot/dts/am4372.dtsi
> @@ -684,6 +684,34 @@
> num-cs = <4>;
> status = "disabled";
> };
> +
> + dss: dss@4832A000 {
> + compatible = "ti,omap3-dss", "simple-bus";
This doesn't look right to me. I'm not sure it makes sense for
"simple-bus" to be in the compatible list.
Are the child nodes usable in isolation, or are they dependent on the
"ti,omap3-dss" node? What exactly does the "ti,omap3-dss" node
represent?
Thanks,
Mark.
> + reg = <0x4832A000 0x200>;
> + ti,hwmods = "dss_core";
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges;
> +
> + dispc@4832A400 {
> + compatible = "ti,omap3-dispc";
> + reg = <0x4832A400 0x400>;
> + interrupts = <GIC_SPI 127 IRQ_TYPE_LEVEL_HIGH>;
> + ti,hwmods = "dss_dispc";
> + };
> +
> + dpi: encoder@0 {
> + compatible = "ti,omap3-dpi";
> + };
> +
> + rfbi: rfbi@4832A800 {
> + compatible = "ti,omap3-rfbi";
> + reg = <0x4832A800 0x100>;
> + ti,hwmods = "dss_rfbi";
> + };
> +
> + };
> +
> };
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v2 4/4] ARM: DTS: AM43x: Add DSS node
2014-03-13 17:46 ` Mark Rutland
@ 2014-03-13 18:22 ` Tomi Valkeinen
2014-03-14 9:10 ` Mark Rutland
0 siblings, 1 reply; 20+ messages in thread
From: Tomi Valkeinen @ 2014-03-13 18:22 UTC (permalink / raw)
To: Mark Rutland
Cc: Sathya Prakash M R, tony@atomide.com, devicetree@vger.kernel.org,
linux-omap@vger.kernel.org, rob.herring@calxeda.com, Pawel Moll,
paul@pwsan.com
[-- Attachment #1: Type: text/plain, Size: 2953 bytes --]
On 13/03/14 19:46, Mark Rutland wrote:
> On Thu, Mar 13, 2014 at 08:58:29AM +0000, Sathya Prakash M R wrote:
>> Add device node for DSS module for AM4372. Both the
>> AM437x-Gp evm and Am43x-Epos evm use the same LCD panel.
>> The lcd timings are added in respective dts files.
>> Adds display pinctrl and enables required gpio.
>> Also set the right parent clock to the DSS clock.
>>
>> Signed-off-by: Sathya Prakash M R <sathyap@ti.com>
>> ---
>> arch/arm/boot/dts/am4372.dtsi | 28 +++++++++++++
>> arch/arm/boot/dts/am437x-gp-evm.dts | 77 ++++++++++++++++++++++++++++++++++
>> arch/arm/boot/dts/am43x-epos-evm.dts | 73 ++++++++++++++++++++++++++++++++
>> arch/arm/boot/dts/am43xx-clocks.dtsi | 2 +
>> 4 files changed, 180 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/am4372.dtsi b/arch/arm/boot/dts/am4372.dtsi
>> index ea55a4e..b72a7df 100644
>> --- a/arch/arm/boot/dts/am4372.dtsi
>> +++ b/arch/arm/boot/dts/am4372.dtsi
>> @@ -684,6 +684,34 @@
>> num-cs = <4>;
>> status = "disabled";
>> };
>> +
>> + dss: dss@4832A000 {
>> + compatible = "ti,omap3-dss", "simple-bus";
>
> This doesn't look right to me. I'm not sure it makes sense for
> "simple-bus" to be in the compatible list.
>
> Are the child nodes usable in isolation, or are they dependent on the
> "ti,omap3-dss" node? What exactly does the "ti,omap3-dss" node
> represent?
The child nodes are dependent on the dss node.
The "ti,omap3-dss" represents the dss_core block of the OMAP display
subsystem. The dss_core is a small IP, a wrapper for the submodules,
handling things like clock and video path routing between the submodules
and the OMAP's other components (like the PRCM where we get clocks). It
also handles reset, so when dss_core is reset, all the submodules are reset.
The HW design is a bit odd, in my opinion, as the submodules are proper
IP blocks, and as far as I see, they could be designed to be independent
of each others. But they have not been designed so.
Having dss_core as the parent node for the submodules gives us automatic
runtime-pm handling, so when one submodule is enabled, it forces
dss_core to be enabled first. This makes the reset work right (i.e. we
don't accidentally reset dss_core when one of the submodules is in use),
and, as the dss_core is always needed to setup the clock and video path
routing, it gets properly initialized before any of the submodules will
use it.
What "simple-bus" mostly gives us here is automatic creation of the
platform devices for the submodules. We could also create the devices
for submodules in the driver of the dss_core. I did have that at some
point, but the "simple-bus" does identical job, and it seemed to make
sense to me.
Note that the same method is used for omap2/3/4 also, in the patches
that have been going around for some time in the lists.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v2 4/4] ARM: DTS: AM43x: Add DSS node
2014-03-13 18:22 ` Tomi Valkeinen
@ 2014-03-14 9:10 ` Mark Rutland
2014-03-14 9:42 ` Tomi Valkeinen
0 siblings, 1 reply; 20+ messages in thread
From: Mark Rutland @ 2014-03-14 9:10 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Sathya Prakash M R, tony@atomide.com, devicetree@vger.kernel.org,
linux-omap@vger.kernel.org, rob.herring@calxeda.com, Pawel Moll,
paul@pwsan.com
On Thu, Mar 13, 2014 at 06:22:54PM +0000, Tomi Valkeinen wrote:
> On 13/03/14 19:46, Mark Rutland wrote:
> > On Thu, Mar 13, 2014 at 08:58:29AM +0000, Sathya Prakash M R wrote:
> >> Add device node for DSS module for AM4372. Both the
> >> AM437x-Gp evm and Am43x-Epos evm use the same LCD panel.
> >> The lcd timings are added in respective dts files.
> >> Adds display pinctrl and enables required gpio.
> >> Also set the right parent clock to the DSS clock.
> >>
> >> Signed-off-by: Sathya Prakash M R <sathyap@ti.com>
> >> ---
> >> arch/arm/boot/dts/am4372.dtsi | 28 +++++++++++++
> >> arch/arm/boot/dts/am437x-gp-evm.dts | 77 ++++++++++++++++++++++++++++++++++
> >> arch/arm/boot/dts/am43x-epos-evm.dts | 73 ++++++++++++++++++++++++++++++++
> >> arch/arm/boot/dts/am43xx-clocks.dtsi | 2 +
> >> 4 files changed, 180 insertions(+)
> >>
> >> diff --git a/arch/arm/boot/dts/am4372.dtsi b/arch/arm/boot/dts/am4372.dtsi
> >> index ea55a4e..b72a7df 100644
> >> --- a/arch/arm/boot/dts/am4372.dtsi
> >> +++ b/arch/arm/boot/dts/am4372.dtsi
> >> @@ -684,6 +684,34 @@
> >> num-cs = <4>;
> >> status = "disabled";
> >> };
> >> +
> >> + dss: dss@4832A000 {
> >> + compatible = "ti,omap3-dss", "simple-bus";
> >
> > This doesn't look right to me. I'm not sure it makes sense for
> > "simple-bus" to be in the compatible list.
> >
> > Are the child nodes usable in isolation, or are they dependent on the
> > "ti,omap3-dss" node? What exactly does the "ti,omap3-dss" node
> > represent?
>
> The child nodes are dependent on the dss node.
Ok. Then simple-bus is not appropriate, as the dss node cannot be
ignored for the children to function.
>
> The "ti,omap3-dss" represents the dss_core block of the OMAP display
> subsystem. The dss_core is a small IP, a wrapper for the submodules,
> handling things like clock and video path routing between the submodules
> and the OMAP's other components (like the PRCM where we get clocks). It
> also handles reset, so when dss_core is reset, all the submodules are reset.
>
> The HW design is a bit odd, in my opinion, as the submodules are proper
> IP blocks, and as far as I see, they could be designed to be independent
> of each others. But they have not been designed so.
>
> Having dss_core as the parent node for the submodules gives us automatic
> runtime-pm handling, so when one submodule is enabled, it forces
> dss_core to be enabled first. This makes the reset work right (i.e. we
> don't accidentally reset dss_core when one of the submodules is in use),
> and, as the dss_core is always needed to setup the clock and video path
> routing, it gets properly initialized before any of the submodules will
> use it.
>
> What "simple-bus" mostly gives us here is automatic creation of the
> platform devices for the submodules. We could also create the devices
> for submodules in the driver of the dss_core. I did have that at some
> point, but the "simple-bus" does identical job, and it seemed to make
> sense to me.
The "simple-bus" compatible string is intended for busses which are
transparent (bar some address remapping expressed via ranges), and is
not intended as an annotation to get Linux to probe child nodes.
Any node with a "simple-bus" entry in the compatible list should either
be handled as a transparent bus, or optionally as the more specific bus
it claims to be (where some hardware configuration may be required
before children can be probed). Unfortunately Linux probes chidlren
regardless, which is arguable a Linux bug.
There's no reason to leak this issue into dts files. Please remove the
"simple-bus" string, and get the dss driver to probe children as
required -- as described above the dss node never makes sense as a
simple-bus.
>
> Note that the same method is used for omap2/3/4 also, in the patches
> that have been going around for some time in the lists.
And those should be fixed.
Cheers,
Mark.
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v2 4/4] ARM: DTS: AM43x: Add DSS node
2014-03-14 9:10 ` Mark Rutland
@ 2014-03-14 9:42 ` Tomi Valkeinen
2014-03-14 10:14 ` Mark Rutland
0 siblings, 1 reply; 20+ messages in thread
From: Tomi Valkeinen @ 2014-03-14 9:42 UTC (permalink / raw)
To: Mark Rutland
Cc: Sathya Prakash M R, tony@atomide.com, devicetree@vger.kernel.org,
linux-omap@vger.kernel.org, rob.herring@calxeda.com, Pawel Moll,
paul@pwsan.com
[-- Attachment #1: Type: text/plain, Size: 1173 bytes --]
On 14/03/14 11:10, Mark Rutland wrote:
> The "simple-bus" compatible string is intended for busses which are
> transparent (bar some address remapping expressed via ranges), and is
> not intended as an annotation to get Linux to probe child nodes.
>
> Any node with a "simple-bus" entry in the compatible list should either
> be handled as a transparent bus, or optionally as the more specific bus
> it claims to be (where some hardware configuration may be required
> before children can be probed). Unfortunately Linux probes chidlren
> regardless, which is arguable a Linux bug.
>
> There's no reason to leak this issue into dts files. Please remove the
> "simple-bus" string, and get the dss driver to probe children as
> required -- as described above the dss node never makes sense as a
> simple-bus.
Ok. I'll remove the simple-bus, and make the dss_core register the
devices. I presume of_platform_populate() is fine for this? Seems to
work fine for registration, but I haven't figured out yet how to
unregister the devices (I get a crash in platform_device_del() if I just
call platform_device_unregister for the submodules).
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 4/4] ARM: DTS: AM43x: Add DSS node
2014-03-14 9:42 ` Tomi Valkeinen
@ 2014-03-14 10:14 ` Mark Rutland
2014-03-14 10:19 ` Tomi Valkeinen
0 siblings, 1 reply; 20+ messages in thread
From: Mark Rutland @ 2014-03-14 10:14 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Sathya Prakash M R, tony@atomide.com, devicetree@vger.kernel.org,
linux-omap@vger.kernel.org, rob.herring@calxeda.com, Pawel Moll,
paul@pwsan.com
On Fri, Mar 14, 2014 at 09:42:26AM +0000, Tomi Valkeinen wrote:
> On 14/03/14 11:10, Mark Rutland wrote:
>
> > The "simple-bus" compatible string is intended for busses which are
> > transparent (bar some address remapping expressed via ranges), and is
> > not intended as an annotation to get Linux to probe child nodes.
> >
> > Any node with a "simple-bus" entry in the compatible list should either
> > be handled as a transparent bus, or optionally as the more specific bus
> > it claims to be (where some hardware configuration may be required
> > before children can be probed). Unfortunately Linux probes chidlren
> > regardless, which is arguable a Linux bug.
> >
> > There's no reason to leak this issue into dts files. Please remove the
> > "simple-bus" string, and get the dss driver to probe children as
> > required -- as described above the dss node never makes sense as a
> > simple-bus.
>
> Ok. I'll remove the simple-bus, and make the dss_core register the
> devices. I presume of_platform_populate() is fine for this? Seems to
> work fine for registration, but I haven't figured out yet how to
> unregister the devices (I get a crash in platform_device_del() if I just
> call platform_device_unregister for the submodules).
I think of_platform_populate should be ok. It's not fantastic -- all
child nodes will be probed, regardless of whether you expect them to
exist, but it's not as broken as using "simple-bus".
I'm unfortunately not familiar with how unregistration works.
I can't see anything obviously wrong in platform_device_del. Do you have
a backtrace?
Thanks,
Mark.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 4/4] ARM: DTS: AM43x: Add DSS node
2014-03-14 10:14 ` Mark Rutland
@ 2014-03-14 10:19 ` Tomi Valkeinen
2014-03-14 11:07 ` Tomi Valkeinen
0 siblings, 1 reply; 20+ messages in thread
From: Tomi Valkeinen @ 2014-03-14 10:19 UTC (permalink / raw)
To: Mark Rutland
Cc: Sathya Prakash M R, tony@atomide.com, devicetree@vger.kernel.org,
linux-omap@vger.kernel.org, rob.herring@calxeda.com, Pawel Moll,
paul@pwsan.com
[-- Attachment #1: Type: text/plain, Size: 5436 bytes --]
On 14/03/14 12:14, Mark Rutland wrote:
> I can't see anything obviously wrong in platform_device_del. Do you have
> a backtrace?
Yes, below.
I can see at least drivers/usb/dwc3/dwc3-exynos.c doing the exact same thing
I do, so maybe I've got something wrong with the omapdss driver.
Tomi
[ 62.987335] Unable to handle kernel NULL pointer dereference at virtual address 00000018
[ 62.995910] pgd = eb6b8000
[ 62.998779] [00000018] *pgd=aa127831, *pte=00000000, *ppte=00000000
[ 63.005462] Internal error: Oops: 17 [#1] SMP ARM
[ 63.005462] Modules linked in: omapdss(-) [last unloaded: encoder_tfp410]
[ 63.011779] CPU: 1 PID: 1021 Comm: rmmod Not tainted 3.14.0-rc2-00057-gacd3401a1fea-dirty #69
[ 63.025909] task: eb17a040 ti: ea14e000 task.ti: ea14e000
[ 63.032287] PC is at release_resource+0x1c/0x84
[ 63.032287] LR is at _raw_write_lock+0x50/0x58
[ 63.041748] pc : [<c004adc8>] lr : [<c05bbd5c>] psr: 60000113
[ 63.041748] sp : ea14fde0 ip : ea14fdb8 fp : ea14fdf4
[ 63.053833] r10: 00000000 r9 : ea14e000 r8 : c000f704
[ 63.053833] r7 : 00000081 r6 : bf0004fc r5 : eb714400 r4 : eb6a9600
[ 63.061798] r3 : 00000000 r2 : 00000001 r1 : 00000011 r0 : c08cec3c
[ 63.071746] Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user
[ 63.071746] Control: 10c53c7d Table: ab6b804a DAC: 00000015
[ 63.081787] Process rmmod (pid: 1021, stack limit = 0xea14e248)
[ 63.091735] Stack: (0xea14fde0 to 0xea150000)
[ 63.093109] fde0: 00000001 eb714400 ea14fe0c ea14fdf8 c03953dc c004adb8 eb714400 00000000
[ 63.101806] fe00: ea14fe24 ea14fe10 c0395808 c0395374 eb17a040 eb714400 ea14fe3c ea14fe28
[ 63.114715] fe20: bf000530 c0395800 eb714410 00000000 ea14fe64 ea14fe40 c038fa5c bf000508
[ 63.121795] fe40: eb214b80 eb6a9d30 ea14fe7c eb204410 bf032d98 eb204444 ea14fe7c ea14fe68
[ 63.123352] fe60: bf022574 c038fa2c bf02254c eb204410 ea14fe8c ea14fe80 c0395080 bf022558
[ 63.140136] fe80: ea14fea4 ea14fe90 c0393494 c039506c eb204410 bf032d98 ea14fec4 ea14fea8
[ 63.149200] fea0: c0393efc c0393428 00000000 bf032d98 bf034438 bf0342b8 ea14fedc ea14fec8
[ 63.154327] fec0: c0393244 c0393e4c eb6fa300 bf032d98 ea14fef4 ea14fee0 c0394570 c03931ec
[ 63.166442] fee0: bf022800 00000005 ea14ff04 ea14fef8 c03951d0 c039454c ea14ff14 ea14ff08
[ 63.175048] ff00: bf0010e8 c03951c8 ea14ff34 ea14ff18 bf022534 bf0010e0 bf0224f4 00000000
[ 63.183685] ff20: bf0342d0 00000880 ea14ffa4 ea14ff38 c00b7f50 bf022500 c000f564 00000000
[ 63.183685] ff40: bf0342d0 00000880 ea14ff3c 70616d6f 00737364 00088ec9 ea14ff84 ea14ff68
[ 63.200927] ff60: c008fff0 c008fdd8 0001cec8 70616d6f 00737364 00000081 ea14ff94 ea14ff88
[ 63.209533] ff80: c00900dc 0008ff08 00000000 0001cec8 70616d6f 00737364 00000000 ea14ffa8
[ 63.218170] ffa0: c000f540 c00b7e0c 0001cec8 70616d6f bea28b10 00000880 bea28b10 00000880
[ 63.226776] ffc0: 0001cec8 70616d6f 00737364 00000081 000acc00 00000042 00088ec9 00000000
[ 63.234619] ffe0: bea28b08 bea28af8 0001cda4 b6ed3390 60000110 bea28b10 00000000 00000000
[ 63.234619] Backtrace:
[ 63.246612] [<c004adac>] (release_resource) from [<c03953dc>] (platform_device_del+0x74/0xa4)
[ 63.246612] r5:eb714400 r4:00000001
[ 63.246612] [<c0395368>] (platform_device_del) from [<c0395808>] (platform_device_unregister+0x14/0x20)
[ 63.267150] r5:00000000 r4:eb714400
[ 63.273223] [<c03957f4>] (platform_device_unregister) from [<bf000530>] (dss_uninit_submodule_dev+0x34/0x40 [omapdss])
[ 63.281799] r4:eb714400 r3:eb17a040
[ 63.281799] [<bf0004fc>] (dss_uninit_submodule_dev [omapdss]) from [<c038fa5c>] (device_for_each_child+0x3c/0x7c)
[ 63.291839] r4:00000000 r3:eb714410
[ 63.303070] [<c038fa20>] (device_for_each_child) from [<bf022574>] (omap_dsshw_remove+0x28/0x70 [omapdss])
[ 63.312255] r6:eb204444 r5:bf032d98 r4:eb204410
[ 63.318237] [<bf02254c>] (omap_dsshw_remove [omapdss]) from [<c0395080>] (platform_drv_remove+0x20/0x24)
[ 63.321807] r4:eb204410 r3:bf02254c
[ 63.331848] [<c0395060>] (platform_drv_remove) from [<c0393494>] (__device_release_driver+0x78/0xd0)
[ 63.341644] [<c039341c>] (__device_release_driver) from [<c0393efc>] (driver_detach+0xbc/0xc0)
[ 63.350708] r5:bf032d98 r4:eb204410
[ 63.352874] [<c0393e40>] (driver_detach) from [<c0393244>] (bus_remove_driver+0x64/0xcc)
[ 63.352874] r6:bf0342b8 r5:bf034438 r4:bf032d98 r3:00000000
[ 63.369018] [<c03931e0>] (bus_remove_driver) from [<c0394570>] (driver_unregister+0x30/0x50)
[ 63.372894] r4:bf032d98 r3:eb6fa300
[ 63.381713] [<c0394540>] (driver_unregister) from [<c03951d0>] (platform_driver_unregister+0x14/0x18)
[ 63.391418] r4:00000005 r3:bf022800
[ 63.391845] [<c03951bc>] (platform_driver_unregister) from [<bf0010e8>] (dss_uninit_platform_driver+0x14/0x1c [omapdss])
[ 63.401794] [<bf0010d4>] (dss_uninit_platform_driver [omapdss]) from [<bf022534>] (omap_dss_exit+0x40/0x58 [omapdss])
[ 63.406951] [<bf0224f4>] (omap_dss_exit [omapdss]) from [<c00b7f50>] (SyS_delete_module+0x150/0x1e0)
[ 63.421844] r6:00000880 r5:bf0342d0 r4:00000000 r3:bf0224f4
[ 63.431793] [<c00b7e00>] (SyS_delete_module) from [<c000f540>] (ret_fast_syscall+0x0/0x48)
[ 63.431793] r6:00737364 r5:70616d6f r4:0001cec8
[ 63.447448] Code: e1a04000 e59f0068 eb15c3d1 e5943010 (e5932018)
[ 63.453918] ---[ end trace 9bdaba0cecbfc6c4 ]---
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 4/4] ARM: DTS: AM43x: Add DSS node
2014-03-14 10:19 ` Tomi Valkeinen
@ 2014-03-14 11:07 ` Tomi Valkeinen
2014-03-14 14:07 ` Mark Rutland
0 siblings, 1 reply; 20+ messages in thread
From: Tomi Valkeinen @ 2014-03-14 11:07 UTC (permalink / raw)
To: Mark Rutland
Cc: Sathya Prakash M R, tony@atomide.com, devicetree@vger.kernel.org,
linux-omap@vger.kernel.org, rob.herring@calxeda.com, Pawel Moll,
paul@pwsan.com
[-- Attachment #1: Type: text/plain, Size: 714 bytes --]
On 14/03/14 12:19, Tomi Valkeinen wrote:
> On 14/03/14 12:14, Mark Rutland wrote:
>
>> I can't see anything obviously wrong in platform_device_del. Do you have
>> a backtrace?
>
> Yes, below.
>
> I can see at least drivers/usb/dwc3/dwc3-exynos.c doing the exact same thing
> I do, so maybe I've got something wrong with the omapdss driver.
Looks to me that the devices created by of_platform_populate() are not
unregisterable in all cases. The address resource created via
of_platform_populate() had NULL res->parent, which causes
release_resource to crash.
I can as well call of_platform_populate() in the platform init code at
boot time, and create the devices there for now.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 4/4] ARM: DTS: AM43x: Add DSS node
2014-03-14 11:07 ` Tomi Valkeinen
@ 2014-03-14 14:07 ` Mark Rutland
2014-03-14 16:04 ` Felipe Balbi
0 siblings, 1 reply; 20+ messages in thread
From: Mark Rutland @ 2014-03-14 14:07 UTC (permalink / raw)
To: Tomi Valkeinen, balbi@ti.com, av.tikhomirov@samsung.com
Cc: Sathya Prakash M R, tony@atomide.com, devicetree@vger.kernel.org,
linux-omap@vger.kernel.org, Pawel Moll, paul@pwsan.com
On Fri, Mar 14, 2014 at 11:07:05AM +0000, Tomi Valkeinen wrote:
> On 14/03/14 12:19, Tomi Valkeinen wrote:
> > On 14/03/14 12:14, Mark Rutland wrote:
> >
> >> I can't see anything obviously wrong in platform_device_del. Do you have
> >> a backtrace?
> >
> > Yes, below.
> >
> > I can see at least drivers/usb/dwc3/dwc3-exynos.c doing the exact same thing
> > I do, so maybe I've got something wrong with the omapdss driver.
>
> Looks to me that the devices created by of_platform_populate() are not
> unregisterable in all cases. The address resource created via
> of_platform_populate() had NULL res->parent, which causes
> release_resource to crash.
Hmm. I can't see that unregistering such devices ever works as you say,
given that __release_resource expects a non-NULL parent pointer. Either
we should be setting the parent pointer when initialising devices from
dt or we should teach __release_resource to not care. I'll have a go at
fixing that.
It looks like drivers/usb/dwc3/dwc3-exynos.c only unregisters the
top-level device, not children. This top-level device has no
IORESOURCE_{IO,MEM} resources judging by
arch/arm/boot/dts/exynos5250.dtsi, which would explain why that driver
isn't exploding: __release_resource will never get called.
Anton, Felipe:
Does unregistering the parent ensure the children get cleaned up, or
does it leave them dangling in the dwc3-exynos driver?
Cheers,
Mark.
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v2 4/4] ARM: DTS: AM43x: Add DSS node
2014-03-14 14:07 ` Mark Rutland
@ 2014-03-14 16:04 ` Felipe Balbi
2014-03-14 16:34 ` Tomi Valkeinen
0 siblings, 1 reply; 20+ messages in thread
From: Felipe Balbi @ 2014-03-14 16:04 UTC (permalink / raw)
To: Mark Rutland
Cc: Tomi Valkeinen, balbi@ti.com, av.tikhomirov@samsung.com,
Sathya Prakash M R, tony@atomide.com, devicetree@vger.kernel.org,
linux-omap@vger.kernel.org, Pawel Moll, paul@pwsan.com
[-- Attachment #1: Type: text/plain, Size: 2519 bytes --]
On Fri, Mar 14, 2014 at 02:07:45PM +0000, Mark Rutland wrote:
> On Fri, Mar 14, 2014 at 11:07:05AM +0000, Tomi Valkeinen wrote:
> > On 14/03/14 12:19, Tomi Valkeinen wrote:
> > > On 14/03/14 12:14, Mark Rutland wrote:
> > >
> > >> I can't see anything obviously wrong in platform_device_del. Do you have
> > >> a backtrace?
> > >
> > > Yes, below.
> > >
> > > I can see at least drivers/usb/dwc3/dwc3-exynos.c doing the exact same thing
> > > I do, so maybe I've got something wrong with the omapdss driver.
> >
> > Looks to me that the devices created by of_platform_populate() are not
> > unregisterable in all cases. The address resource created via
> > of_platform_populate() had NULL res->parent, which causes
> > release_resource to crash.
>
> Hmm. I can't see that unregistering such devices ever works as you say,
> given that __release_resource expects a non-NULL parent pointer. Either
> we should be setting the parent pointer when initialising devices from
> dt or we should teach __release_resource to not care. I'll have a go at
> fixing that.
>
> It looks like drivers/usb/dwc3/dwc3-exynos.c only unregisters the
> top-level device, not children. This top-level device has no
> IORESOURCE_{IO,MEM} resources judging by
> arch/arm/boot/dts/exynos5250.dtsi, which would explain why that driver
> isn't exploding: __release_resource will never get called.
>
> Anton, Felipe:
>
> Does unregistering the parent ensure the children get cleaned up, or
> does it leave them dangling in the dwc3-exynos driver?
you should platform_device_unregister() for each children and
dwc3-exynos does that just fine:
167 static int dwc3_exynos_remove(struct platform_device *pdev)
168 {
169 struct dwc3_exynos *exynos = platform_get_drvdata(pdev);
170
171 device_for_each_child(&pdev->dev, NULL, dwc3_exynos_remove_child);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
172 platform_device_unregister(exynos->usb2_phy);
173 platform_device_unregister(exynos->usb3_phy);
174
175 clk_disable_unprepare(exynos->clk);
176
177 return 0;
178 }
for each child of this device, we call dwc3_exynos_remove_child(), which
will:
94 static int dwc3_exynos_remove_child(struct device *dev, void *unused)
95 {
96 struct platform_device *pdev = to_platform_device(dev);
97
98 platform_device_unregister(pdev);
99
100 return 0;
101 }
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v2 4/4] ARM: DTS: AM43x: Add DSS node
2014-03-14 16:04 ` Felipe Balbi
@ 2014-03-14 16:34 ` Tomi Valkeinen
2014-03-14 18:00 ` Mark Rutland
0 siblings, 1 reply; 20+ messages in thread
From: Tomi Valkeinen @ 2014-03-14 16:34 UTC (permalink / raw)
To: balbi, Mark Rutland
Cc: av.tikhomirov@samsung.com, Sathya Prakash M R, tony@atomide.com,
devicetree@vger.kernel.org, linux-omap@vger.kernel.org,
Pawel Moll, paul@pwsan.com
[-- Attachment #1: Type: text/plain, Size: 2209 bytes --]
On 14/03/14 18:04, Felipe Balbi wrote:
> On Fri, Mar 14, 2014 at 02:07:45PM +0000, Mark Rutland wrote:
>> On Fri, Mar 14, 2014 at 11:07:05AM +0000, Tomi Valkeinen wrote:
>>> On 14/03/14 12:19, Tomi Valkeinen wrote:
>>>> On 14/03/14 12:14, Mark Rutland wrote:
>>>>
>>>>> I can't see anything obviously wrong in platform_device_del. Do you have
>>>>> a backtrace?
>>>>
>>>> Yes, below.
>>>>
>>>> I can see at least drivers/usb/dwc3/dwc3-exynos.c doing the exact same thing
>>>> I do, so maybe I've got something wrong with the omapdss driver.
>>>
>>> Looks to me that the devices created by of_platform_populate() are not
>>> unregisterable in all cases. The address resource created via
>>> of_platform_populate() had NULL res->parent, which causes
>>> release_resource to crash.
>>
>> Hmm. I can't see that unregistering such devices ever works as you say,
>> given that __release_resource expects a non-NULL parent pointer. Either
>> we should be setting the parent pointer when initialising devices from
>> dt or we should teach __release_resource to not care. I'll have a go at
>> fixing that.
>>
>> It looks like drivers/usb/dwc3/dwc3-exynos.c only unregisters the
>> top-level device, not children. This top-level device has no
>> IORESOURCE_{IO,MEM} resources judging by
>> arch/arm/boot/dts/exynos5250.dtsi, which would explain why that driver
>> isn't exploding: __release_resource will never get called.
>>
>> Anton, Felipe:
>>
>> Does unregistering the parent ensure the children get cleaned up, or
>> does it leave them dangling in the dwc3-exynos driver?
>
> you should platform_device_unregister() for each children and
> dwc3-exynos does that just fine:
Yes, that's what I do also, and it crashes. What Mark said above about
unregistering never working for such devices is correct, but I don't
know why he said dwc3-exynos only unregisters the top level device.
"such devices" above meaning devices with a 'reg' defined in the DT
data, if I'm not mistaken.
So at the moment, I think of_platform_populate() and
platform_device_unregister() combination in a driver is a bit risky.
Work fine for certain cases, not for some other.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v2 4/4] ARM: DTS: AM43x: Add DSS node
2014-03-14 16:34 ` Tomi Valkeinen
@ 2014-03-14 18:00 ` Mark Rutland
0 siblings, 0 replies; 20+ messages in thread
From: Mark Rutland @ 2014-03-14 18:00 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: balbi@ti.com, av.tikhomirov@samsung.com, Sathya Prakash M R,
tony@atomide.com, devicetree@vger.kernel.org,
linux-omap@vger.kernel.org, Pawel Moll, paul@pwsan.com
On Fri, Mar 14, 2014 at 04:34:19PM +0000, Tomi Valkeinen wrote:
> On 14/03/14 18:04, Felipe Balbi wrote:
> > On Fri, Mar 14, 2014 at 02:07:45PM +0000, Mark Rutland wrote:
> >> On Fri, Mar 14, 2014 at 11:07:05AM +0000, Tomi Valkeinen wrote:
> >>> On 14/03/14 12:19, Tomi Valkeinen wrote:
> >>>> On 14/03/14 12:14, Mark Rutland wrote:
> >>>>
> >>>>> I can't see anything obviously wrong in platform_device_del. Do you have
> >>>>> a backtrace?
> >>>>
> >>>> Yes, below.
> >>>>
> >>>> I can see at least drivers/usb/dwc3/dwc3-exynos.c doing the exact same thing
> >>>> I do, so maybe I've got something wrong with the omapdss driver.
> >>>
> >>> Looks to me that the devices created by of_platform_populate() are not
> >>> unregisterable in all cases. The address resource created via
> >>> of_platform_populate() had NULL res->parent, which causes
> >>> release_resource to crash.
> >>
> >> Hmm. I can't see that unregistering such devices ever works as you say,
> >> given that __release_resource expects a non-NULL parent pointer. Either
> >> we should be setting the parent pointer when initialising devices from
> >> dt or we should teach __release_resource to not care. I'll have a go at
> >> fixing that.
> >>
> >> It looks like drivers/usb/dwc3/dwc3-exynos.c only unregisters the
> >> top-level device, not children. This top-level device has no
> >> IORESOURCE_{IO,MEM} resources judging by
> >> arch/arm/boot/dts/exynos5250.dtsi, which would explain why that driver
> >> isn't exploding: __release_resource will never get called.
> >>
> >> Anton, Felipe:
> >>
> >> Does unregistering the parent ensure the children get cleaned up, or
> >> does it leave them dangling in the dwc3-exynos driver?
> >
> > you should platform_device_unregister() for each children and
> > dwc3-exynos does that just fine:
>
> Yes, that's what I do also, and it crashes. What Mark said above about
> unregistering never working for such devices is correct, but I don't
> know why he said dwc3-exynos only unregisters the top level device.
Because I'd failed to read the code correctly. It does indeed unregister
each of the children via platform_device_unregister.
Apologies for the confusion there.
> "such devices" above meaning devices with a 'reg' defined in the DT
> data, if I'm not mistaken.
Yes. As far as I can see, IORESOURCE_MEM resources instantiated from reg
entries in dt do not have their parent pointer initialised, and will
cause __release_resource to blow up. A quick inspection of resources on
my TC2 shows this to be the case -- the parent, sibling, and child
pointers are all NULL.
I'm at a loss to explain how the dwc3-exynos driver can call
platform_device_unregister on devices with such resource and not blow
up. Is anyone able to test dwc3_exynos_remove?
> So at the moment, I think of_platform_populate() and
> platform_device_unregister() combination in a driver is a bit risky.
> Work fine for certain cases, not for some other.
It certainly looks to be broken at the moment, but it should be fixable.
Thanks,
Mark.
^ permalink raw reply [flat|nested] 20+ messages in thread