| Message ID | 1345115913-6773-1-git-send-email-cmahapatra@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 2012-08-16 at 16:48 +0530, Chandrabhanu Mahapatra wrote: > All the cpu_is checks have been moved to dss_init_features function providing a > much more generic and cleaner interface. The OMAP version and revision specific > initializations in various functions are cleaned and the necessary data are > moved to dss_features structure which is local to dss.c. > > Signed-off-by: Chandrabhanu Mahapatra <cmahapatra@ti.com> > +static int __init dss_init_features(struct device *dev) > +{ > + dss.feat = devm_kzalloc(dev, sizeof(*dss.feat), GFP_KERNEL); > + if (!dss.feat) { > + dev_err(dev, "Failed to allocate local DSS Features\n"); > + return -ENOMEM; > + } > + > + if (cpu_is_omap24xx()) > + dss.feat = &omap24xx_dss_features; > + else if (cpu_is_omap34xx()) > + dss.feat = &omap34xx_dss_features; > + else if (cpu_is_omap3630()) > + dss.feat = &omap3630_dss_features; > + else if (cpu_is_omap44xx()) > + dss.feat = &omap44xx_dss_features; > + else > + return -ENODEV; > + > + return 0; > +} This is not correct (and same problem in dispc). You allocate the feat struct and assign the pointer to dss.feat, but then overwrite dss.feat pointer with the pointer to omap24xx_dss_features (which is freed later). You need to memcpy it. I also get a crash on omap3 overo board when loading omapdss: loading nfs/work/linux/drivers/video/omap2/dss/omapdss.ko debug=y def_disp=lcd43 [ 20.411224] Unable to handle kernel NULL pointer dereference at virtual address 00000008 [ 20.419921] pgd = ce8a8000 [ 20.422790] [00000008] *pgd=8e8c5831, *pte=00000000, *ppte=00000000 [ 20.429473] Internal error: Oops: 17 [#1] SMP ARM [ 20.434448] Modules linked in: omapdss(+) [ 20.438690] CPU: 0 Tainted: G W (3.5.0-rc2-00058-g1c1e55c #93) [ 20.446350] PC is at omap_dsshw_probe+0xa4/0x290 [omapdss] [ 20.452148] LR is at 0x2e39 [ 20.455108] pc : [<bf043288>] lr : [<00002e39>] psr: 80000013 [ 20.455108] sp : ce89ddd0 ip : c0b797e0 fp : 00006133 [ 20.467224] r10: 00000028 r9 : c0c5c07c r8 : bf02eadc [ 20.472717] r7 : 00000000 r6 : c06e9644 r5 : cf0cf808 r4 : bf02f430 [ 20.479614] r3 : cf0cf808 r2 : 00000000 r1 : 00000000 r0 : 00000000 [ 20.486511] Flags: Nzcv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user [ 20.494049] Control: 10c5387d Table: 8e8a8019 DAC: 00000015 [ 20.500091] Process insmod (pid: 664, stack limit = 0xce89c2f8) [ 20.506347] Stack: (0xce89ddd0 to 0xce89e000) [ 20.510955] ddc0: cf0cf808 c0c96ed8 c0c96ee8 c02c22e4 [ 20.519592] dde0: c02c22cc c02c0f28 22222222 cf0cf808 bf02eadc cf0cf83c 00000000 00000001 [ 20.528198] de00: 00000028 c02c113c bf02eadc c02c10a8 00000000 c02bf6c8 cf0192a8 cf0cec10 [ 20.536834] de20: bf02eadc c072fab8 cf3e7440 c02c05dc bf0249e0 00000000 cf04ce40 bf02eadc [ 20.545471] de40: c0748880 ce89c000 00000000 00000001 c0c5c07c 00000028 00006133 c02c1670 [ 20.554107] de60: 00000000 bf02eac8 c0748880 ce89c000 00000000 00000001 00000028 c02c26e0 [ 20.562744] de80: 00000003 00000000 c0748880 bf043124 00000000 c0748880 ce89c000 00000000 [ 20.571380] dea0: 00000001 c0008730 bf02f29c 00000001 00000001 bf0430cc c071bcd0 00000000 [ 20.580017] dec0: bf02f29c c006823c 00000000 ce827ec0 cf0001c0 00000000 bf02f29c 00000001 [ 20.588623] dee0: ce851480 00000001 c0c5c07c 00000028 00006133 c0099d20 bf02f2a8 00007fff [ 20.597259] df00: c0098aa4 c012480c 00000000 c0098890 bf02f3f0 ce89c000 c06d32d8 d08fe09c [ 20.605895] df20: d0a19624 000a7008 d08ce000 001f2ab7 d0a18bd4 d0a18948 d0aba984 00030ed0 [ 20.614532] df40: 0003b0e0 00000000 00000000 00000042 00000043 00000026 0000002a 00000014 [ 20.623138] df60: 00000000 bf022024 00000043 00000000 00000000 00000000 00000000 c0623b14 [ 20.631774] df80: 001f2ab7 001f2ab7 00000004 beb48e7c 00000080 c0013f28 ce89c000 00000000 [ 20.640411] dfa0: 00000000 c0013d60 001f2ab7 00000004 b6c49008 001f2ab7 000a7008 beb48e7c [ 20.649047] dfc0: 001f2ab7 00000004 beb48e7c 00000080 000a47f8 00000000 b6f80000 00000000 [ 20.657684] dfe0: beb48bb8 beb48ba8 00019dfc b6f10020 60000010 b6c49008 00000000 00000000 [ 20.666442] [<bf043288>] (omap_dsshw_probe+0xa4/0x290 [omapdss]) from [<c02c22e4>] (platform_drv_ probe+0x18/0x1c) [ 20.677276] [<c02c22e4>] (platform_drv_probe+0x18/0x1c) from [<c02c0f28>] (driver_probe_device+0x 9c/0x21c) [ 20.687499] [<c02c0f28>] (driver_probe_device+0x9c/0x21c) from [<c02c113c>] (__driver_attach+0x94 /0x98) [ 20.697418] [<c02c113c>] (__driver_attach+0x94/0x98) from [<c02bf6c8>] (bus_for_each_dev+0x50/0x7 c) [ 20.706970] [<c02bf6c8>] (bus_for_each_dev+0x50/0x7c) from [<c02c05dc>] (bus_add_driver+0xa0/0x2a 8) [ 20.716522] [<c02c05dc>] (bus_add_driver+0xa0/0x2a8) from [<c02c1670>] (driver_register+0x78/0x17 4) [ 20.726074] [<c02c1670>] (driver_register+0x78/0x174) from [<c02c26e0>] (platform_driver_probe+0x 18/0x9c) [ 20.736267] [<c02c26e0>] (platform_driver_probe+0x18/0x9c) from [<bf043124>] (omap_dss_init+0x58/ 0x118 [omapdss]) [ 20.747192] [<bf043124>] (omap_dss_init+0x58/0x118 [omapdss]) from [<c0008730>] (do_one_initcall+ 0x34/0x194) [ 20.757568] [<c0008730>] (do_one_initcall+0x34/0x194) from [<c0099d20>] (sys_init_module+0xdc/0x1 cc4) [ 20.767333] [<c0099d20>] (sys_init_module+0xdc/0x1cc4) from [<c0013d60>] (ret_fast_syscall+0x0/0x 3c) [ 20.776947] Code: ea00000d e5941248 e3a00000 e584600c (e5911008) [ 20.783599] ---[ end trace bcb6e89e4ea810ae ]---
On Fri, 2012-08-17 at 16:54 +0300, Tomi Valkeinen wrote: > On Thu, 2012-08-16 at 16:48 +0530, Chandrabhanu Mahapatra wrote: > > All the cpu_is checks have been moved to dss_init_features function providing a > > much more generic and cleaner interface. The OMAP version and revision specific > > initializations in various functions are cleaned and the necessary data are > > moved to dss_features structure which is local to dss.c. > > > > Signed-off-by: Chandrabhanu Mahapatra <cmahapatra@ti.com> > > > +static int __init dss_init_features(struct device *dev) > > +{ > > + dss.feat = devm_kzalloc(dev, sizeof(*dss.feat), GFP_KERNEL); > > + if (!dss.feat) { > > + dev_err(dev, "Failed to allocate local DSS Features\n"); > > + return -ENOMEM; > > + } > > + > > + if (cpu_is_omap24xx()) > > + dss.feat = &omap24xx_dss_features; > > + else if (cpu_is_omap34xx()) > > + dss.feat = &omap34xx_dss_features; > > + else if (cpu_is_omap3630()) > > + dss.feat = &omap3630_dss_features; > > + else if (cpu_is_omap44xx()) > > + dss.feat = &omap44xx_dss_features; > > + else > > + return -ENODEV; > > + > > + return 0; > > +} > > This is not correct (and same problem in dispc). You allocate the feat > struct and assign the pointer to dss.feat, but then overwrite dss.feat > pointer with the pointer to omap24xx_dss_features (which is freed > later). You need to memcpy it. > > I also get a crash on omap3 overo board when loading omapdss: The crash happens because dss_get_clocks uses the feat stuff, but the dss_init_features is only called later. Did you test this? I can't see how that can work on any board. Also, in the current place you have dss_init_features call, in case there's an error you can't just return, you need to release the resources. The same problem is in the dispc.c. Tomi
On Mon, Aug 20, 2012 at 2:12 PM, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote: > On Fri, 2012-08-17 at 16:54 +0300, Tomi Valkeinen wrote: >> On Thu, 2012-08-16 at 16:48 +0530, Chandrabhanu Mahapatra wrote: >> > All the cpu_is checks have been moved to dss_init_features function providing a >> > much more generic and cleaner interface. The OMAP version and revision specific >> > initializations in various functions are cleaned and the necessary data are >> > moved to dss_features structure which is local to dss.c. >> > >> > Signed-off-by: Chandrabhanu Mahapatra <cmahapatra@ti.com> >> >> > +static int __init dss_init_features(struct device *dev) >> > +{ >> > + dss.feat = devm_kzalloc(dev, sizeof(*dss.feat), GFP_KERNEL); >> > + if (!dss.feat) { >> > + dev_err(dev, "Failed to allocate local DSS Features\n"); >> > + return -ENOMEM; >> > + } >> > + >> > + if (cpu_is_omap24xx()) >> > + dss.feat = &omap24xx_dss_features; >> > + else if (cpu_is_omap34xx()) >> > + dss.feat = &omap34xx_dss_features; >> > + else if (cpu_is_omap3630()) >> > + dss.feat = &omap3630_dss_features; >> > + else if (cpu_is_omap44xx()) >> > + dss.feat = &omap44xx_dss_features; >> > + else >> > + return -ENODEV; >> > + >> > + return 0; >> > +} >> >> This is not correct (and same problem in dispc). You allocate the feat >> struct and assign the pointer to dss.feat, but then overwrite dss.feat >> pointer with the pointer to omap24xx_dss_features (which is freed >> later). You need to memcpy it. >> Ok. >> I also get a crash on omap3 overo board when loading omapdss: > During recent tests I never saw the crash happen. The kernel used to freeze during booting after printing "Uncompressing Linux... done, booting the kernel." > The crash happens because dss_get_clocks uses the feat stuff, but the > dss_init_features is only called later. Did you test this? I can't see > how that can work on any board. > Thanks for pointing that out. Correcting above fixed the problem. > Also, in the current place you have dss_init_features call, in case > there's an error you can't just return, you need to release the > resources. The same problem is in the dispc.c. > > Tomi > By releasing resources did you mean the dss.feat memory? I think dss_init_features call is wrongly placed and should be placed at the very beginning of the probe function.
On Mon, 2012-08-20 at 16:06 +0530, Mahapatra, Chandrabhanu wrote: > By releasing resources did you mean the dss.feat memory? I think > dss_init_features call is wrongly placed and should be placed at the > very beginning of the probe function. I meant e.g. pm_runtime_disable() etc. But if the call is moved t the beginning, there's nothing to release. Tomi
diff --git a/drivers/video/omap2/dss/dss.c b/drivers/video/omap2/dss/dss.c index 7b1c6ac..a9cb84b 100644 --- a/drivers/video/omap2/dss/dss.c +++ b/drivers/video/omap2/dss/dss.c @@ -31,6 +31,7 @@ #include <linux/clk.h> #include <linux/platform_device.h> #include <linux/pm_runtime.h> +#include <linux/gfp.h> #include <video/omapdss.h> @@ -65,6 +66,12 @@ struct dss_reg { static int dss_runtime_get(void); static void dss_runtime_put(void); +struct dss_features { + u16 fck_div_max; + int dss_fck_multiplier; + char *clk_name; +}; + static struct { struct platform_device *pdev; void __iomem *base; @@ -83,6 +90,8 @@ static struct { bool ctx_valid; u32 ctx[DSS_SZ_REGS / sizeof(u32)]; + + const struct dss_features *feat; } dss; static const char * const dss_generic_clk_source_names[] = { @@ -91,6 +100,30 @@ static const char * const dss_generic_clk_source_names[] = { [OMAP_DSS_CLK_SRC_FCK] = "DSS_FCK", }; +static const struct dss_features omap24xx_dss_features __initconst = { + .fck_div_max = 16, + .dss_fck_multiplier = 2, + .clk_name = NULL, +}; + +static const struct dss_features omap34xx_dss_features __initconst = { + .fck_div_max = 16, + .dss_fck_multiplier = 2, + .clk_name = "dpll4_m4_ck", +}; + +static const struct dss_features omap3630_dss_features __initconst = { + .fck_div_max = 32, + .dss_fck_multiplier = 1, + .clk_name = "dpll4_m4_ck", +}; + +static const struct dss_features omap44xx_dss_features __initconst = { + .fck_div_max = 32, + .dss_fck_multiplier = 1, + .clk_name = "dpll_per_m5x2_ck", +}; + static inline void dss_write_reg(const struct dss_reg idx, u32 val) { __raw_writel(val, dss.base + idx.idx); @@ -236,7 +269,6 @@ const char *dss_get_generic_clk_source_name(enum omap_dss_clk_source clk_src) return dss_generic_clk_source_names[clk_src]; } - void dss_dump_clocks(struct seq_file *s) { unsigned long dpll4_ck_rate; @@ -259,18 +291,10 @@ void dss_dump_clocks(struct seq_file *s) seq_printf(s, "dpll4_ck %lu\n", dpll4_ck_rate); - if (cpu_is_omap3630() || cpu_is_omap44xx()) - seq_printf(s, "%s (%s) = %lu / %lu = %lu\n", - fclk_name, fclk_real_name, - dpll4_ck_rate, - dpll4_ck_rate / dpll4_m4_ck_rate, - fclk_rate); - else - seq_printf(s, "%s (%s) = %lu / %lu * 2 = %lu\n", - fclk_name, fclk_real_name, - dpll4_ck_rate, - dpll4_ck_rate / dpll4_m4_ck_rate, - fclk_rate); + seq_printf(s, "%s (%s) = %lu / %lu * %d = %lu\n", + fclk_name, fclk_real_name, dpll4_ck_rate, + dpll4_ck_rate / dpll4_m4_ck_rate, + dss.feat->dss_fck_multiplier, fclk_rate); } else { seq_printf(s, "%s (%s) = %lu\n", fclk_name, fclk_real_name, @@ -470,7 +494,7 @@ int dss_calc_clock_div(unsigned long req_pck, struct dss_clock_info *dss_cinfo, unsigned long fck, max_dss_fck; - u16 fck_div, fck_div_max = 16; + u16 fck_div; int match = 0; int min_fck_per_pck; @@ -480,9 +504,8 @@ int dss_calc_clock_div(unsigned long req_pck, struct dss_clock_info *dss_cinfo, max_dss_fck = dss_feat_get_param_max(FEAT_PARAM_DSS_FCK); fck = clk_get_rate(dss.dss_clk); - if (req_pck == dss.cache_req_pck && - ((cpu_is_omap34xx() && prate == dss.cache_prate) || - dss.cache_dss_cinfo.fck == fck)) { + if (req_pck == dss.cache_req_pck && prate == dss.cache_prate && + dss.cache_dss_cinfo.fck == fck) { DSSDBG("dispc clock info found from cache.\n"); *dss_cinfo = dss.cache_dss_cinfo; *dispc_cinfo = dss.cache_dispc_cinfo; @@ -519,16 +542,10 @@ retry: goto found; } else { - if (cpu_is_omap3630() || cpu_is_omap44xx()) - fck_div_max = 32; - - for (fck_div = fck_div_max; fck_div > 0; --fck_div) { + for (fck_div = dss.feat->fck_div_max; fck_div > 0; --fck_div) { struct dispc_clock_info cur_dispc; - if (fck_div_max == 32) - fck = prate / fck_div; - else - fck = prate / fck_div * 2; + fck = prate / fck_div * dss.feat->dss_fck_multiplier; if (fck > max_dss_fck) continue; @@ -633,22 +650,11 @@ static int dss_get_clocks(void) dss.dss_clk = clk; - if (cpu_is_omap34xx()) { - clk = clk_get(NULL, "dpll4_m4_ck"); - if (IS_ERR(clk)) { - DSSERR("Failed to get dpll4_m4_ck\n"); - r = PTR_ERR(clk); - goto err; - } - } else if (cpu_is_omap44xx()) { - clk = clk_get(NULL, "dpll_per_m5x2_ck"); - if (IS_ERR(clk)) { - DSSERR("Failed to get dpll_per_m5x2_ck\n"); - r = PTR_ERR(clk); - goto err; - } - } else { /* omap24xx */ - clk = NULL; + clk = clk_get(NULL, dss.feat->clk_name); + if (IS_ERR(clk)) { + DSSERR("Failed to get %s\n", dss.feat->clk_name); + r = PTR_ERR(clk); + goto err; } dss.dpll4_m4_ck = clk; @@ -704,6 +710,28 @@ void dss_debug_dump_clocks(struct seq_file *s) } #endif +static int __init dss_init_features(struct device *dev) +{ + dss.feat = devm_kzalloc(dev, sizeof(*dss.feat), GFP_KERNEL); + if (!dss.feat) { + dev_err(dev, "Failed to allocate local DSS Features\n"); + return -ENOMEM; + } + + if (cpu_is_omap24xx()) + dss.feat = &omap24xx_dss_features; + else if (cpu_is_omap34xx()) + dss.feat = &omap34xx_dss_features; + else if (cpu_is_omap3630()) + dss.feat = &omap3630_dss_features; + else if (cpu_is_omap44xx()) + dss.feat = &omap44xx_dss_features; + else + return -ENODEV; + + return 0; +} + /* DSS HW IP initialisation */ static int __init omap_dsshw_probe(struct platform_device *pdev) { @@ -750,6 +778,10 @@ static int __init omap_dsshw_probe(struct platform_device *pdev) dss.lcd_clk_source[0] = OMAP_DSS_CLK_SRC_FCK; dss.lcd_clk_source[1] = OMAP_DSS_CLK_SRC_FCK; + r = dss_init_features(&dss.pdev->dev); + if (r) + return r; + rev = dss_read_reg(DSS_REVISION); printk(KERN_INFO "OMAP DSS rev %d.%d\n", FLD_GET(rev, 7, 4), FLD_GET(rev, 3, 0));
All the cpu_is checks have been moved to dss_init_features function providing a much more generic and cleaner interface. The OMAP version and revision specific initializations in various functions are cleaned and the necessary data are moved to dss_features structure which is local to dss.c. Signed-off-by: Chandrabhanu Mahapatra <cmahapatra@ti.com> --- drivers/video/omap2/dss/dss.c | 114 ++++++++++++++++++++++++++--------------- 1 file changed, 73 insertions(+), 41 deletions(-)