* [PATCH 0/5] fixes for atmel-hlcdc @ 2019-12-10 13:24 ` Claudiu Beznea 0 siblings, 0 replies; 54+ messages in thread From: Claudiu Beznea @ 2019-12-10 13:24 UTC (permalink / raw) To: sam, bbrezillon, airlied, daniel, nicolas.ferre, alexandre.belloni, ludovic.desroches, lee.jones Cc: dri-devel, linux-arm-kernel, linux-kernel, Claudiu Beznea Hi, I have few fixes for atmel-hlcdc driver in this series as well as two reverts. Revert "drm: atmel-hlcdc: enable sys_clk during initalization." is due to the fix in in patch 2/5. Thank you, Claudiu Beznea Claudiu Beznea (5): drm: atmel-hlcdc: use double rate for pixel clock only if supported drm: atmel-hlcdc: enable clock before configuring timing engine mfd: atmel-hlcdc: return in case of error Revert "drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested" Revert "drm: atmel-hlcdc: enable sys_clk during initalization." drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 26 ++++++++++---------------- drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 19 +------------------ drivers/mfd/atmel-hlcdc.c | 14 ++++++++++---- 3 files changed, 21 insertions(+), 38 deletions(-) -- 2.7.4 ^ permalink raw reply [flat|nested] 54+ messages in thread
* [PATCH 0/5] fixes for atmel-hlcdc @ 2019-12-10 13:24 ` Claudiu Beznea 0 siblings, 0 replies; 54+ messages in thread From: Claudiu Beznea @ 2019-12-10 13:24 UTC (permalink / raw) To: sam, bbrezillon, airlied, daniel, nicolas.ferre, alexandre.belloni, ludovic.desroches, lee.jones Cc: Claudiu Beznea, linux-arm-kernel, dri-devel, linux-kernel Hi, I have few fixes for atmel-hlcdc driver in this series as well as two reverts. Revert "drm: atmel-hlcdc: enable sys_clk during initalization." is due to the fix in in patch 2/5. Thank you, Claudiu Beznea Claudiu Beznea (5): drm: atmel-hlcdc: use double rate for pixel clock only if supported drm: atmel-hlcdc: enable clock before configuring timing engine mfd: atmel-hlcdc: return in case of error Revert "drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested" Revert "drm: atmel-hlcdc: enable sys_clk during initalization." drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 26 ++++++++++---------------- drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 19 +------------------ drivers/mfd/atmel-hlcdc.c | 14 ++++++++++---- 3 files changed, 21 insertions(+), 38 deletions(-) -- 2.7.4 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 54+ messages in thread
* [PATCH 0/5] fixes for atmel-hlcdc @ 2019-12-10 13:24 ` Claudiu Beznea 0 siblings, 0 replies; 54+ messages in thread From: Claudiu Beznea @ 2019-12-10 13:24 UTC (permalink / raw) To: sam, bbrezillon, airlied, daniel, nicolas.ferre, alexandre.belloni, ludovic.desroches, lee.jones Cc: Claudiu Beznea, linux-arm-kernel, dri-devel, linux-kernel Hi, I have few fixes for atmel-hlcdc driver in this series as well as two reverts. Revert "drm: atmel-hlcdc: enable sys_clk during initalization." is due to the fix in in patch 2/5. Thank you, Claudiu Beznea Claudiu Beznea (5): drm: atmel-hlcdc: use double rate for pixel clock only if supported drm: atmel-hlcdc: enable clock before configuring timing engine mfd: atmel-hlcdc: return in case of error Revert "drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested" Revert "drm: atmel-hlcdc: enable sys_clk during initalization." drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 26 ++++++++++---------------- drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 19 +------------------ drivers/mfd/atmel-hlcdc.c | 14 ++++++++++---- 3 files changed, 21 insertions(+), 38 deletions(-) -- 2.7.4 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 54+ messages in thread
* [PATCH 1/5] drm: atmel-hlcdc: use double rate for pixel clock only if supported 2019-12-10 13:24 ` Claudiu Beznea (?) @ 2019-12-10 13:24 ` Claudiu Beznea -1 siblings, 0 replies; 54+ messages in thread From: Claudiu Beznea @ 2019-12-10 13:24 UTC (permalink / raw) To: sam, bbrezillon, airlied, daniel, nicolas.ferre, alexandre.belloni, ludovic.desroches, lee.jones Cc: dri-devel, linux-arm-kernel, linux-kernel, Claudiu Beznea Doubled system clock should be used as pixel cock source only if this is supported. This is emphasized by the value of atmel_hlcdc_crtc::dc::desc::fixed_clksrc. Fixes: a6eca2abdd42 ("drm: atmel-hlcdc: add config option for clock selection") Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> --- drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c index f2e73e6d46b8..5040ed8d0871 100644 --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c @@ -95,14 +95,14 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c) (adj->crtc_hdisplay - 1) | ((adj->crtc_vdisplay - 1) << 16)); + prate = clk_get_rate(crtc->dc->hlcdc->sys_clk); + mode_rate = adj->crtc_clock * 1000; if (!crtc->dc->desc->fixed_clksrc) { + prate *= 2; cfg |= ATMEL_HLCDC_CLKSEL; mask |= ATMEL_HLCDC_CLKSEL; } - prate = 2 * clk_get_rate(crtc->dc->hlcdc->sys_clk); - mode_rate = adj->crtc_clock * 1000; - div = DIV_ROUND_UP(prate, mode_rate); if (div < 2) { div = 2; -- 2.7.4 ^ permalink raw reply related [flat|nested] 54+ messages in thread
* [PATCH 1/5] drm: atmel-hlcdc: use double rate for pixel clock only if supported @ 2019-12-10 13:24 ` Claudiu Beznea 0 siblings, 0 replies; 54+ messages in thread From: Claudiu Beznea @ 2019-12-10 13:24 UTC (permalink / raw) To: sam, bbrezillon, airlied, daniel, nicolas.ferre, alexandre.belloni, ludovic.desroches, lee.jones Cc: Claudiu Beznea, linux-arm-kernel, dri-devel, linux-kernel Doubled system clock should be used as pixel cock source only if this is supported. This is emphasized by the value of atmel_hlcdc_crtc::dc::desc::fixed_clksrc. Fixes: a6eca2abdd42 ("drm: atmel-hlcdc: add config option for clock selection") Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> --- drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c index f2e73e6d46b8..5040ed8d0871 100644 --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c @@ -95,14 +95,14 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c) (adj->crtc_hdisplay - 1) | ((adj->crtc_vdisplay - 1) << 16)); + prate = clk_get_rate(crtc->dc->hlcdc->sys_clk); + mode_rate = adj->crtc_clock * 1000; if (!crtc->dc->desc->fixed_clksrc) { + prate *= 2; cfg |= ATMEL_HLCDC_CLKSEL; mask |= ATMEL_HLCDC_CLKSEL; } - prate = 2 * clk_get_rate(crtc->dc->hlcdc->sys_clk); - mode_rate = adj->crtc_clock * 1000; - div = DIV_ROUND_UP(prate, mode_rate); if (div < 2) { div = 2; -- 2.7.4 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 54+ messages in thread
* [PATCH 1/5] drm: atmel-hlcdc: use double rate for pixel clock only if supported @ 2019-12-10 13:24 ` Claudiu Beznea 0 siblings, 0 replies; 54+ messages in thread From: Claudiu Beznea @ 2019-12-10 13:24 UTC (permalink / raw) To: sam, bbrezillon, airlied, daniel, nicolas.ferre, alexandre.belloni, ludovic.desroches, lee.jones Cc: Claudiu Beznea, linux-arm-kernel, dri-devel, linux-kernel Doubled system clock should be used as pixel cock source only if this is supported. This is emphasized by the value of atmel_hlcdc_crtc::dc::desc::fixed_clksrc. Fixes: a6eca2abdd42 ("drm: atmel-hlcdc: add config option for clock selection") Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> --- drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c index f2e73e6d46b8..5040ed8d0871 100644 --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c @@ -95,14 +95,14 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c) (adj->crtc_hdisplay - 1) | ((adj->crtc_vdisplay - 1) << 16)); + prate = clk_get_rate(crtc->dc->hlcdc->sys_clk); + mode_rate = adj->crtc_clock * 1000; if (!crtc->dc->desc->fixed_clksrc) { + prate *= 2; cfg |= ATMEL_HLCDC_CLKSEL; mask |= ATMEL_HLCDC_CLKSEL; } - prate = 2 * clk_get_rate(crtc->dc->hlcdc->sys_clk); - mode_rate = adj->crtc_clock * 1000; - div = DIV_ROUND_UP(prate, mode_rate); if (div < 2) { div = 2; -- 2.7.4 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 54+ messages in thread
* [PATCH 2/5] drm: atmel-hlcdc: enable clock before configuring timing engine 2019-12-10 13:24 ` Claudiu Beznea (?) @ 2019-12-10 13:24 ` Claudiu Beznea -1 siblings, 0 replies; 54+ messages in thread From: Claudiu Beznea @ 2019-12-10 13:24 UTC (permalink / raw) To: sam, bbrezillon, airlied, daniel, nicolas.ferre, alexandre.belloni, ludovic.desroches, lee.jones Cc: dri-devel, linux-arm-kernel, linux-kernel, Claudiu Beznea Changing pixel clock source without having this clock source enabled will block the timing engine and the next operations after (in this case setting ATMEL_HLCDC_CFG(5) settings in atmel_hlcdc_crtc_mode_set_nofb() will fail). It is recomended (although in datasheet this is not present) to actually enabled pixel clock source before doing any changes on timing enginge (only SAM9X60 datasheet specifies that the peripheral clock and pixel clock must be enabled before using LCD controller). Fixes: 1a396789f65a ("drm: add Atmel HLCDC Display Controller support") Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> --- drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c index 5040ed8d0871..721fa88bf71d 100644 --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c @@ -73,7 +73,11 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c) unsigned long prate; unsigned int mask = ATMEL_HLCDC_CLKDIV_MASK | ATMEL_HLCDC_CLKPOL; unsigned int cfg = 0; - int div; + int div, ret; + + ret = clk_prepare_enable(crtc->dc->hlcdc->sys_clk); + if (ret) + return; vm.vfront_porch = adj->crtc_vsync_start - adj->crtc_vdisplay; vm.vback_porch = adj->crtc_vtotal - adj->crtc_vsync_end; @@ -147,6 +151,8 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c) ATMEL_HLCDC_VSPSU | ATMEL_HLCDC_VSPHO | ATMEL_HLCDC_GUARDTIME_MASK | ATMEL_HLCDC_MODE_MASK, cfg); + + clk_disable_unprepare(crtc->dc->hlcdc->sys_clk); } static enum drm_mode_status -- 2.7.4 ^ permalink raw reply related [flat|nested] 54+ messages in thread
* [PATCH 2/5] drm: atmel-hlcdc: enable clock before configuring timing engine @ 2019-12-10 13:24 ` Claudiu Beznea 0 siblings, 0 replies; 54+ messages in thread From: Claudiu Beznea @ 2019-12-10 13:24 UTC (permalink / raw) To: sam, bbrezillon, airlied, daniel, nicolas.ferre, alexandre.belloni, ludovic.desroches, lee.jones Cc: Claudiu Beznea, linux-arm-kernel, dri-devel, linux-kernel Changing pixel clock source without having this clock source enabled will block the timing engine and the next operations after (in this case setting ATMEL_HLCDC_CFG(5) settings in atmel_hlcdc_crtc_mode_set_nofb() will fail). It is recomended (although in datasheet this is not present) to actually enabled pixel clock source before doing any changes on timing enginge (only SAM9X60 datasheet specifies that the peripheral clock and pixel clock must be enabled before using LCD controller). Fixes: 1a396789f65a ("drm: add Atmel HLCDC Display Controller support") Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> --- drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c index 5040ed8d0871..721fa88bf71d 100644 --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c @@ -73,7 +73,11 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c) unsigned long prate; unsigned int mask = ATMEL_HLCDC_CLKDIV_MASK | ATMEL_HLCDC_CLKPOL; unsigned int cfg = 0; - int div; + int div, ret; + + ret = clk_prepare_enable(crtc->dc->hlcdc->sys_clk); + if (ret) + return; vm.vfront_porch = adj->crtc_vsync_start - adj->crtc_vdisplay; vm.vback_porch = adj->crtc_vtotal - adj->crtc_vsync_end; @@ -147,6 +151,8 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c) ATMEL_HLCDC_VSPSU | ATMEL_HLCDC_VSPHO | ATMEL_HLCDC_GUARDTIME_MASK | ATMEL_HLCDC_MODE_MASK, cfg); + + clk_disable_unprepare(crtc->dc->hlcdc->sys_clk); } static enum drm_mode_status -- 2.7.4 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 54+ messages in thread
* [PATCH 2/5] drm: atmel-hlcdc: enable clock before configuring timing engine @ 2019-12-10 13:24 ` Claudiu Beznea 0 siblings, 0 replies; 54+ messages in thread From: Claudiu Beznea @ 2019-12-10 13:24 UTC (permalink / raw) To: sam, bbrezillon, airlied, daniel, nicolas.ferre, alexandre.belloni, ludovic.desroches, lee.jones Cc: Claudiu Beznea, linux-arm-kernel, dri-devel, linux-kernel Changing pixel clock source without having this clock source enabled will block the timing engine and the next operations after (in this case setting ATMEL_HLCDC_CFG(5) settings in atmel_hlcdc_crtc_mode_set_nofb() will fail). It is recomended (although in datasheet this is not present) to actually enabled pixel clock source before doing any changes on timing enginge (only SAM9X60 datasheet specifies that the peripheral clock and pixel clock must be enabled before using LCD controller). Fixes: 1a396789f65a ("drm: add Atmel HLCDC Display Controller support") Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> --- drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c index 5040ed8d0871..721fa88bf71d 100644 --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c @@ -73,7 +73,11 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c) unsigned long prate; unsigned int mask = ATMEL_HLCDC_CLKDIV_MASK | ATMEL_HLCDC_CLKPOL; unsigned int cfg = 0; - int div; + int div, ret; + + ret = clk_prepare_enable(crtc->dc->hlcdc->sys_clk); + if (ret) + return; vm.vfront_porch = adj->crtc_vsync_start - adj->crtc_vdisplay; vm.vback_porch = adj->crtc_vtotal - adj->crtc_vsync_end; @@ -147,6 +151,8 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c) ATMEL_HLCDC_VSPSU | ATMEL_HLCDC_VSPHO | ATMEL_HLCDC_GUARDTIME_MASK | ATMEL_HLCDC_MODE_MASK, cfg); + + clk_disable_unprepare(crtc->dc->hlcdc->sys_clk); } static enum drm_mode_status -- 2.7.4 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 54+ messages in thread
* Re: [PATCH 2/5] drm: atmel-hlcdc: enable clock before configuring timing engine 2019-12-10 13:24 ` Claudiu Beznea (?) @ 2019-12-10 20:18 ` Sam Ravnborg -1 siblings, 0 replies; 54+ messages in thread From: Sam Ravnborg @ 2019-12-10 20:18 UTC (permalink / raw) To: Claudiu Beznea Cc: bbrezillon, airlied, daniel, nicolas.ferre, alexandre.belloni, ludovic.desroches, lee.jones, dri-devel, linux-arm-kernel, linux-kernel Hi Claudiu. On Tue, Dec 10, 2019 at 03:24:44PM +0200, Claudiu Beznea wrote: > Changing pixel clock source without having this clock source enabled > will block the timing engine and the next operations after (in this case > setting ATMEL_HLCDC_CFG(5) settings in atmel_hlcdc_crtc_mode_set_nofb() > will fail). It is recomended (although in datasheet this is not present) > to actually enabled pixel clock source before doing any changes on timing > enginge (only SAM9X60 datasheet specifies that the peripheral clock and > pixel clock must be enabled before using LCD controller). > > Fixes: 1a396789f65a ("drm: add Atmel HLCDC Display Controller support") > Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> We already had a remotely similar fix. See 262d67e73f9a920a20bd75278761400404a82de0 ("drm: atmel-hlcdc: enable sys_clk during initalization.") In this patch sys_clk is only enabled if we have a fixed_clk. Maybe we should do this unconditionally in atmel_hlcdc_dc_load()? Then we do not need this enable(disable in the mode_set_nofb implementation. Have you considered this way to fix it? Sam > --- > drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c > index 5040ed8d0871..721fa88bf71d 100644 > --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c > +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c > @@ -73,7 +73,11 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c) > unsigned long prate; > unsigned int mask = ATMEL_HLCDC_CLKDIV_MASK | ATMEL_HLCDC_CLKPOL; > unsigned int cfg = 0; > - int div; > + int div, ret; > + > + ret = clk_prepare_enable(crtc->dc->hlcdc->sys_clk); > + if (ret) > + return; > > vm.vfront_porch = adj->crtc_vsync_start - adj->crtc_vdisplay; > vm.vback_porch = adj->crtc_vtotal - adj->crtc_vsync_end; > @@ -147,6 +151,8 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c) > ATMEL_HLCDC_VSPSU | ATMEL_HLCDC_VSPHO | > ATMEL_HLCDC_GUARDTIME_MASK | ATMEL_HLCDC_MODE_MASK, > cfg); > + > + clk_disable_unprepare(crtc->dc->hlcdc->sys_clk); > } > > static enum drm_mode_status > -- > 2.7.4 ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 2/5] drm: atmel-hlcdc: enable clock before configuring timing engine @ 2019-12-10 20:18 ` Sam Ravnborg 0 siblings, 0 replies; 54+ messages in thread From: Sam Ravnborg @ 2019-12-10 20:18 UTC (permalink / raw) To: Claudiu Beznea Cc: alexandre.belloni, bbrezillon, airlied, nicolas.ferre, dri-devel, linux-kernel, ludovic.desroches, lee.jones, linux-arm-kernel Hi Claudiu. On Tue, Dec 10, 2019 at 03:24:44PM +0200, Claudiu Beznea wrote: > Changing pixel clock source without having this clock source enabled > will block the timing engine and the next operations after (in this case > setting ATMEL_HLCDC_CFG(5) settings in atmel_hlcdc_crtc_mode_set_nofb() > will fail). It is recomended (although in datasheet this is not present) > to actually enabled pixel clock source before doing any changes on timing > enginge (only SAM9X60 datasheet specifies that the peripheral clock and > pixel clock must be enabled before using LCD controller). > > Fixes: 1a396789f65a ("drm: add Atmel HLCDC Display Controller support") > Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> We already had a remotely similar fix. See 262d67e73f9a920a20bd75278761400404a82de0 ("drm: atmel-hlcdc: enable sys_clk during initalization.") In this patch sys_clk is only enabled if we have a fixed_clk. Maybe we should do this unconditionally in atmel_hlcdc_dc_load()? Then we do not need this enable(disable in the mode_set_nofb implementation. Have you considered this way to fix it? Sam > --- > drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c > index 5040ed8d0871..721fa88bf71d 100644 > --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c > +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c > @@ -73,7 +73,11 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c) > unsigned long prate; > unsigned int mask = ATMEL_HLCDC_CLKDIV_MASK | ATMEL_HLCDC_CLKPOL; > unsigned int cfg = 0; > - int div; > + int div, ret; > + > + ret = clk_prepare_enable(crtc->dc->hlcdc->sys_clk); > + if (ret) > + return; > > vm.vfront_porch = adj->crtc_vsync_start - adj->crtc_vdisplay; > vm.vback_porch = adj->crtc_vtotal - adj->crtc_vsync_end; > @@ -147,6 +151,8 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c) > ATMEL_HLCDC_VSPSU | ATMEL_HLCDC_VSPHO | > ATMEL_HLCDC_GUARDTIME_MASK | ATMEL_HLCDC_MODE_MASK, > cfg); > + > + clk_disable_unprepare(crtc->dc->hlcdc->sys_clk); > } > > static enum drm_mode_status > -- > 2.7.4 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 2/5] drm: atmel-hlcdc: enable clock before configuring timing engine @ 2019-12-10 20:18 ` Sam Ravnborg 0 siblings, 0 replies; 54+ messages in thread From: Sam Ravnborg @ 2019-12-10 20:18 UTC (permalink / raw) To: Claudiu Beznea Cc: alexandre.belloni, bbrezillon, airlied, dri-devel, linux-kernel, ludovic.desroches, daniel, lee.jones, linux-arm-kernel Hi Claudiu. On Tue, Dec 10, 2019 at 03:24:44PM +0200, Claudiu Beznea wrote: > Changing pixel clock source without having this clock source enabled > will block the timing engine and the next operations after (in this case > setting ATMEL_HLCDC_CFG(5) settings in atmel_hlcdc_crtc_mode_set_nofb() > will fail). It is recomended (although in datasheet this is not present) > to actually enabled pixel clock source before doing any changes on timing > enginge (only SAM9X60 datasheet specifies that the peripheral clock and > pixel clock must be enabled before using LCD controller). > > Fixes: 1a396789f65a ("drm: add Atmel HLCDC Display Controller support") > Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> We already had a remotely similar fix. See 262d67e73f9a920a20bd75278761400404a82de0 ("drm: atmel-hlcdc: enable sys_clk during initalization.") In this patch sys_clk is only enabled if we have a fixed_clk. Maybe we should do this unconditionally in atmel_hlcdc_dc_load()? Then we do not need this enable(disable in the mode_set_nofb implementation. Have you considered this way to fix it? Sam > --- > drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c > index 5040ed8d0871..721fa88bf71d 100644 > --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c > +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c > @@ -73,7 +73,11 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c) > unsigned long prate; > unsigned int mask = ATMEL_HLCDC_CLKDIV_MASK | ATMEL_HLCDC_CLKPOL; > unsigned int cfg = 0; > - int div; > + int div, ret; > + > + ret = clk_prepare_enable(crtc->dc->hlcdc->sys_clk); > + if (ret) > + return; > > vm.vfront_porch = adj->crtc_vsync_start - adj->crtc_vdisplay; > vm.vback_porch = adj->crtc_vtotal - adj->crtc_vsync_end; > @@ -147,6 +151,8 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c) > ATMEL_HLCDC_VSPSU | ATMEL_HLCDC_VSPHO | > ATMEL_HLCDC_GUARDTIME_MASK | ATMEL_HLCDC_MODE_MASK, > cfg); > + > + clk_disable_unprepare(crtc->dc->hlcdc->sys_clk); > } > > static enum drm_mode_status > -- > 2.7.4 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 54+ messages in thread
* [PATCH 3/5] mfd: atmel-hlcdc: return in case of error 2019-12-10 13:24 ` Claudiu Beznea (?) @ 2019-12-10 13:24 ` Claudiu Beznea -1 siblings, 0 replies; 54+ messages in thread From: Claudiu Beznea @ 2019-12-10 13:24 UTC (permalink / raw) To: sam, bbrezillon, airlied, daniel, nicolas.ferre, alexandre.belloni, ludovic.desroches, lee.jones Cc: dri-devel, linux-arm-kernel, linux-kernel, Claudiu Beznea For HLCDC timing engine configurations bit ATMEL_HLCDC_SIP of ATMEL_HLCDC_SR needs to checked if it is equal with zero before applying new configuration to timing engine. In case of timeout there is no indicator about this, so, return with error in case of timeout in regmap_atmel_hlcdc_reg_write() and also print a message about this. Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> --- drivers/mfd/atmel-hlcdc.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/drivers/mfd/atmel-hlcdc.c b/drivers/mfd/atmel-hlcdc.c index 64013c57a920..19f1dbeb8bcd 100644 --- a/drivers/mfd/atmel-hlcdc.c +++ b/drivers/mfd/atmel-hlcdc.c @@ -39,10 +39,16 @@ static int regmap_atmel_hlcdc_reg_write(void *context, unsigned int reg, if (reg <= ATMEL_HLCDC_DIS) { u32 status; - - readl_poll_timeout_atomic(hregmap->regs + ATMEL_HLCDC_SR, - status, !(status & ATMEL_HLCDC_SIP), - 1, 100); + int ret; + + ret = readl_poll_timeout_atomic(hregmap->regs + ATMEL_HLCDC_SR, + status, + !(status & ATMEL_HLCDC_SIP), + 1, 100); + if (ret) { + pr_err("Timeout waiting for ATMEL_HLCDC_SIP\n"); + return ret; + } } writel(val, hregmap->regs + reg); -- 2.7.4 ^ permalink raw reply related [flat|nested] 54+ messages in thread
* [PATCH 3/5] mfd: atmel-hlcdc: return in case of error @ 2019-12-10 13:24 ` Claudiu Beznea 0 siblings, 0 replies; 54+ messages in thread From: Claudiu Beznea @ 2019-12-10 13:24 UTC (permalink / raw) To: sam, bbrezillon, airlied, daniel, nicolas.ferre, alexandre.belloni, ludovic.desroches, lee.jones Cc: Claudiu Beznea, linux-arm-kernel, dri-devel, linux-kernel For HLCDC timing engine configurations bit ATMEL_HLCDC_SIP of ATMEL_HLCDC_SR needs to checked if it is equal with zero before applying new configuration to timing engine. In case of timeout there is no indicator about this, so, return with error in case of timeout in regmap_atmel_hlcdc_reg_write() and also print a message about this. Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> --- drivers/mfd/atmel-hlcdc.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/drivers/mfd/atmel-hlcdc.c b/drivers/mfd/atmel-hlcdc.c index 64013c57a920..19f1dbeb8bcd 100644 --- a/drivers/mfd/atmel-hlcdc.c +++ b/drivers/mfd/atmel-hlcdc.c @@ -39,10 +39,16 @@ static int regmap_atmel_hlcdc_reg_write(void *context, unsigned int reg, if (reg <= ATMEL_HLCDC_DIS) { u32 status; - - readl_poll_timeout_atomic(hregmap->regs + ATMEL_HLCDC_SR, - status, !(status & ATMEL_HLCDC_SIP), - 1, 100); + int ret; + + ret = readl_poll_timeout_atomic(hregmap->regs + ATMEL_HLCDC_SR, + status, + !(status & ATMEL_HLCDC_SIP), + 1, 100); + if (ret) { + pr_err("Timeout waiting for ATMEL_HLCDC_SIP\n"); + return ret; + } } writel(val, hregmap->regs + reg); -- 2.7.4 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 54+ messages in thread
* [PATCH 3/5] mfd: atmel-hlcdc: return in case of error @ 2019-12-10 13:24 ` Claudiu Beznea 0 siblings, 0 replies; 54+ messages in thread From: Claudiu Beznea @ 2019-12-10 13:24 UTC (permalink / raw) To: sam, bbrezillon, airlied, daniel, nicolas.ferre, alexandre.belloni, ludovic.desroches, lee.jones Cc: Claudiu Beznea, linux-arm-kernel, dri-devel, linux-kernel For HLCDC timing engine configurations bit ATMEL_HLCDC_SIP of ATMEL_HLCDC_SR needs to checked if it is equal with zero before applying new configuration to timing engine. In case of timeout there is no indicator about this, so, return with error in case of timeout in regmap_atmel_hlcdc_reg_write() and also print a message about this. Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> --- drivers/mfd/atmel-hlcdc.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/drivers/mfd/atmel-hlcdc.c b/drivers/mfd/atmel-hlcdc.c index 64013c57a920..19f1dbeb8bcd 100644 --- a/drivers/mfd/atmel-hlcdc.c +++ b/drivers/mfd/atmel-hlcdc.c @@ -39,10 +39,16 @@ static int regmap_atmel_hlcdc_reg_write(void *context, unsigned int reg, if (reg <= ATMEL_HLCDC_DIS) { u32 status; - - readl_poll_timeout_atomic(hregmap->regs + ATMEL_HLCDC_SR, - status, !(status & ATMEL_HLCDC_SIP), - 1, 100); + int ret; + + ret = readl_poll_timeout_atomic(hregmap->regs + ATMEL_HLCDC_SR, + status, + !(status & ATMEL_HLCDC_SIP), + 1, 100); + if (ret) { + pr_err("Timeout waiting for ATMEL_HLCDC_SIP\n"); + return ret; + } } writel(val, hregmap->regs + reg); -- 2.7.4 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 54+ messages in thread
* Re: [PATCH 3/5] mfd: atmel-hlcdc: return in case of error 2019-12-10 13:24 ` Claudiu Beznea (?) @ 2019-12-10 20:37 ` Sam Ravnborg -1 siblings, 0 replies; 54+ messages in thread From: Sam Ravnborg @ 2019-12-10 20:37 UTC (permalink / raw) To: Claudiu Beznea Cc: bbrezillon, airlied, daniel, nicolas.ferre, alexandre.belloni, ludovic.desroches, lee.jones, dri-devel, linux-arm-kernel, linux-kernel Hi Claudiu. On Tue, Dec 10, 2019 at 03:24:45PM +0200, Claudiu Beznea wrote: > For HLCDC timing engine configurations bit ATMEL_HLCDC_SIP of > ATMEL_HLCDC_SR needs to checked if it is equal with zero before applying > new configuration to timing engine. In case of timeout there is no > indicator about this, so, return with error in case of timeout in > regmap_atmel_hlcdc_reg_write() and also print a message about this. > > Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> > --- > drivers/mfd/atmel-hlcdc.c | 14 ++++++++++---- > 1 file changed, 10 insertions(+), 4 deletions(-) > > diff --git a/drivers/mfd/atmel-hlcdc.c b/drivers/mfd/atmel-hlcdc.c > index 64013c57a920..19f1dbeb8bcd 100644 > --- a/drivers/mfd/atmel-hlcdc.c > +++ b/drivers/mfd/atmel-hlcdc.c > @@ -39,10 +39,16 @@ static int regmap_atmel_hlcdc_reg_write(void *context, unsigned int reg, > > if (reg <= ATMEL_HLCDC_DIS) { > u32 status; > - > - readl_poll_timeout_atomic(hregmap->regs + ATMEL_HLCDC_SR, > - status, !(status & ATMEL_HLCDC_SIP), > - 1, 100); > + int ret; > + > + ret = readl_poll_timeout_atomic(hregmap->regs + ATMEL_HLCDC_SR, > + status, > + !(status & ATMEL_HLCDC_SIP), > + 1, 100); > + if (ret) { > + pr_err("Timeout waiting for ATMEL_HLCDC_SIP\n"); > + return ret; Consider adding device * to atmel_hlcdc_regmap - so you can use dev_err() here. This makes it obvious what device this comes from. Sam > + } > } > > writel(val, hregmap->regs + reg); > -- > 2.7.4 ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 3/5] mfd: atmel-hlcdc: return in case of error @ 2019-12-10 20:37 ` Sam Ravnborg 0 siblings, 0 replies; 54+ messages in thread From: Sam Ravnborg @ 2019-12-10 20:37 UTC (permalink / raw) To: Claudiu Beznea Cc: alexandre.belloni, bbrezillon, airlied, nicolas.ferre, dri-devel, linux-kernel, ludovic.desroches, lee.jones, linux-arm-kernel Hi Claudiu. On Tue, Dec 10, 2019 at 03:24:45PM +0200, Claudiu Beznea wrote: > For HLCDC timing engine configurations bit ATMEL_HLCDC_SIP of > ATMEL_HLCDC_SR needs to checked if it is equal with zero before applying > new configuration to timing engine. In case of timeout there is no > indicator about this, so, return with error in case of timeout in > regmap_atmel_hlcdc_reg_write() and also print a message about this. > > Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> > --- > drivers/mfd/atmel-hlcdc.c | 14 ++++++++++---- > 1 file changed, 10 insertions(+), 4 deletions(-) > > diff --git a/drivers/mfd/atmel-hlcdc.c b/drivers/mfd/atmel-hlcdc.c > index 64013c57a920..19f1dbeb8bcd 100644 > --- a/drivers/mfd/atmel-hlcdc.c > +++ b/drivers/mfd/atmel-hlcdc.c > @@ -39,10 +39,16 @@ static int regmap_atmel_hlcdc_reg_write(void *context, unsigned int reg, > > if (reg <= ATMEL_HLCDC_DIS) { > u32 status; > - > - readl_poll_timeout_atomic(hregmap->regs + ATMEL_HLCDC_SR, > - status, !(status & ATMEL_HLCDC_SIP), > - 1, 100); > + int ret; > + > + ret = readl_poll_timeout_atomic(hregmap->regs + ATMEL_HLCDC_SR, > + status, > + !(status & ATMEL_HLCDC_SIP), > + 1, 100); > + if (ret) { > + pr_err("Timeout waiting for ATMEL_HLCDC_SIP\n"); > + return ret; Consider adding device * to atmel_hlcdc_regmap - so you can use dev_err() here. This makes it obvious what device this comes from. Sam > + } > } > > writel(val, hregmap->regs + reg); > -- > 2.7.4 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 3/5] mfd: atmel-hlcdc: return in case of error @ 2019-12-10 20:37 ` Sam Ravnborg 0 siblings, 0 replies; 54+ messages in thread From: Sam Ravnborg @ 2019-12-10 20:37 UTC (permalink / raw) To: Claudiu Beznea Cc: alexandre.belloni, bbrezillon, airlied, dri-devel, linux-kernel, ludovic.desroches, daniel, lee.jones, linux-arm-kernel Hi Claudiu. On Tue, Dec 10, 2019 at 03:24:45PM +0200, Claudiu Beznea wrote: > For HLCDC timing engine configurations bit ATMEL_HLCDC_SIP of > ATMEL_HLCDC_SR needs to checked if it is equal with zero before applying > new configuration to timing engine. In case of timeout there is no > indicator about this, so, return with error in case of timeout in > regmap_atmel_hlcdc_reg_write() and also print a message about this. > > Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> > --- > drivers/mfd/atmel-hlcdc.c | 14 ++++++++++---- > 1 file changed, 10 insertions(+), 4 deletions(-) > > diff --git a/drivers/mfd/atmel-hlcdc.c b/drivers/mfd/atmel-hlcdc.c > index 64013c57a920..19f1dbeb8bcd 100644 > --- a/drivers/mfd/atmel-hlcdc.c > +++ b/drivers/mfd/atmel-hlcdc.c > @@ -39,10 +39,16 @@ static int regmap_atmel_hlcdc_reg_write(void *context, unsigned int reg, > > if (reg <= ATMEL_HLCDC_DIS) { > u32 status; > - > - readl_poll_timeout_atomic(hregmap->regs + ATMEL_HLCDC_SR, > - status, !(status & ATMEL_HLCDC_SIP), > - 1, 100); > + int ret; > + > + ret = readl_poll_timeout_atomic(hregmap->regs + ATMEL_HLCDC_SR, > + status, > + !(status & ATMEL_HLCDC_SIP), > + 1, 100); > + if (ret) { > + pr_err("Timeout waiting for ATMEL_HLCDC_SIP\n"); > + return ret; Consider adding device * to atmel_hlcdc_regmap - so you can use dev_err() here. This makes it obvious what device this comes from. Sam > + } > } > > writel(val, hregmap->regs + reg); > -- > 2.7.4 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 3/5] mfd: atmel-hlcdc: return in case of error 2019-12-10 20:37 ` Sam Ravnborg (?) @ 2019-12-11 12:07 ` Claudiu.Beznea -1 siblings, 0 replies; 54+ messages in thread From: Claudiu.Beznea @ 2019-12-11 12:07 UTC (permalink / raw) To: sam Cc: alexandre.belloni, bbrezillon, airlied, dri-devel, linux-kernel, Ludovic.Desroches, daniel, lee.jones, linux-arm-kernel Hi Sam, On 10.12.2019 22:37, Sam Ravnborg wrote: > Hi Claudiu. > > On Tue, Dec 10, 2019 at 03:24:45PM +0200, Claudiu Beznea wrote: >> For HLCDC timing engine configurations bit ATMEL_HLCDC_SIP of >> ATMEL_HLCDC_SR needs to checked if it is equal with zero before applying >> new configuration to timing engine. In case of timeout there is no >> indicator about this, so, return with error in case of timeout in >> regmap_atmel_hlcdc_reg_write() and also print a message about this. >> >> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> >> --- >> drivers/mfd/atmel-hlcdc.c | 14 ++++++++++---- >> 1 file changed, 10 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/mfd/atmel-hlcdc.c b/drivers/mfd/atmel-hlcdc.c >> index 64013c57a920..19f1dbeb8bcd 100644 >> --- a/drivers/mfd/atmel-hlcdc.c >> +++ b/drivers/mfd/atmel-hlcdc.c >> @@ -39,10 +39,16 @@ static int regmap_atmel_hlcdc_reg_write(void *context, unsigned int reg, >> >> if (reg <= ATMEL_HLCDC_DIS) { >> u32 status; >> - >> - readl_poll_timeout_atomic(hregmap->regs + ATMEL_HLCDC_SR, >> - status, !(status & ATMEL_HLCDC_SIP), >> - 1, 100); >> + int ret; >> + >> + ret = readl_poll_timeout_atomic(hregmap->regs + ATMEL_HLCDC_SR, >> + status, >> + !(status & ATMEL_HLCDC_SIP), >> + 1, 100); >> + if (ret) { >> + pr_err("Timeout waiting for ATMEL_HLCDC_SIP\n"); >> + return ret; > Consider adding device * to atmel_hlcdc_regmap - so you can use > dev_err() here. This makes it obvious what device this comes from. OK! I'll do it in v2. > > Sam > >> + } >> } >> >> writel(val, hregmap->regs + reg); >> -- >> 2.7.4 > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 3/5] mfd: atmel-hlcdc: return in case of error @ 2019-12-11 12:07 ` Claudiu.Beznea 0 siblings, 0 replies; 54+ messages in thread From: Claudiu.Beznea @ 2019-12-11 12:07 UTC (permalink / raw) To: sam Cc: alexandre.belloni, bbrezillon, airlied, linux-kernel, dri-devel, Ludovic.Desroches, lee.jones, linux-arm-kernel Hi Sam, On 10.12.2019 22:37, Sam Ravnborg wrote: > Hi Claudiu. > > On Tue, Dec 10, 2019 at 03:24:45PM +0200, Claudiu Beznea wrote: >> For HLCDC timing engine configurations bit ATMEL_HLCDC_SIP of >> ATMEL_HLCDC_SR needs to checked if it is equal with zero before applying >> new configuration to timing engine. In case of timeout there is no >> indicator about this, so, return with error in case of timeout in >> regmap_atmel_hlcdc_reg_write() and also print a message about this. >> >> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> >> --- >> drivers/mfd/atmel-hlcdc.c | 14 ++++++++++---- >> 1 file changed, 10 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/mfd/atmel-hlcdc.c b/drivers/mfd/atmel-hlcdc.c >> index 64013c57a920..19f1dbeb8bcd 100644 >> --- a/drivers/mfd/atmel-hlcdc.c >> +++ b/drivers/mfd/atmel-hlcdc.c >> @@ -39,10 +39,16 @@ static int regmap_atmel_hlcdc_reg_write(void *context, unsigned int reg, >> >> if (reg <= ATMEL_HLCDC_DIS) { >> u32 status; >> - >> - readl_poll_timeout_atomic(hregmap->regs + ATMEL_HLCDC_SR, >> - status, !(status & ATMEL_HLCDC_SIP), >> - 1, 100); >> + int ret; >> + >> + ret = readl_poll_timeout_atomic(hregmap->regs + ATMEL_HLCDC_SR, >> + status, >> + !(status & ATMEL_HLCDC_SIP), >> + 1, 100); >> + if (ret) { >> + pr_err("Timeout waiting for ATMEL_HLCDC_SIP\n"); >> + return ret; > Consider adding device * to atmel_hlcdc_regmap - so you can use > dev_err() here. This makes it obvious what device this comes from. OK! I'll do it in v2. > > Sam > >> + } >> } >> >> writel(val, hregmap->regs + reg); >> -- >> 2.7.4 > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 3/5] mfd: atmel-hlcdc: return in case of error @ 2019-12-11 12:07 ` Claudiu.Beznea 0 siblings, 0 replies; 54+ messages in thread From: Claudiu.Beznea @ 2019-12-11 12:07 UTC (permalink / raw) To: sam Cc: alexandre.belloni, bbrezillon, airlied, linux-kernel, dri-devel, Ludovic.Desroches, daniel, lee.jones, linux-arm-kernel Hi Sam, On 10.12.2019 22:37, Sam Ravnborg wrote: > Hi Claudiu. > > On Tue, Dec 10, 2019 at 03:24:45PM +0200, Claudiu Beznea wrote: >> For HLCDC timing engine configurations bit ATMEL_HLCDC_SIP of >> ATMEL_HLCDC_SR needs to checked if it is equal with zero before applying >> new configuration to timing engine. In case of timeout there is no >> indicator about this, so, return with error in case of timeout in >> regmap_atmel_hlcdc_reg_write() and also print a message about this. >> >> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> >> --- >> drivers/mfd/atmel-hlcdc.c | 14 ++++++++++---- >> 1 file changed, 10 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/mfd/atmel-hlcdc.c b/drivers/mfd/atmel-hlcdc.c >> index 64013c57a920..19f1dbeb8bcd 100644 >> --- a/drivers/mfd/atmel-hlcdc.c >> +++ b/drivers/mfd/atmel-hlcdc.c >> @@ -39,10 +39,16 @@ static int regmap_atmel_hlcdc_reg_write(void *context, unsigned int reg, >> >> if (reg <= ATMEL_HLCDC_DIS) { >> u32 status; >> - >> - readl_poll_timeout_atomic(hregmap->regs + ATMEL_HLCDC_SR, >> - status, !(status & ATMEL_HLCDC_SIP), >> - 1, 100); >> + int ret; >> + >> + ret = readl_poll_timeout_atomic(hregmap->regs + ATMEL_HLCDC_SR, >> + status, >> + !(status & ATMEL_HLCDC_SIP), >> + 1, 100); >> + if (ret) { >> + pr_err("Timeout waiting for ATMEL_HLCDC_SIP\n"); >> + return ret; > Consider adding device * to atmel_hlcdc_regmap - so you can use > dev_err() here. This makes it obvious what device this comes from. OK! I'll do it in v2. > > Sam > >> + } >> } >> >> writel(val, hregmap->regs + reg); >> -- >> 2.7.4 > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 54+ messages in thread
* [PATCH 4/5] Revert "drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested" 2019-12-10 13:24 ` Claudiu Beznea (?) @ 2019-12-10 13:24 ` Claudiu Beznea -1 siblings, 0 replies; 54+ messages in thread From: Claudiu Beznea @ 2019-12-10 13:24 UTC (permalink / raw) To: sam, bbrezillon, airlied, daniel, nicolas.ferre, alexandre.belloni, ludovic.desroches, lee.jones Cc: dri-devel, linux-arm-kernel, linux-kernel, Claudiu Beznea, Peter Rosin This reverts commit f6f7ad3234613f6f7f27c25036aaf078de07e9b0. ("drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested") because allowing selecting a higher pixel clock may overclock LCD devices, not all of them being capable of this. Cc: Peter Rosin <peda@axentia.se> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> --- drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c index 721fa88bf71d..1a70dff1a417 100644 --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c @@ -117,18 +117,6 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c) div = DIV_ROUND_UP(prate, mode_rate); if (ATMEL_HLCDC_CLKDIV(div) & ~ATMEL_HLCDC_CLKDIV_MASK) div = ATMEL_HLCDC_CLKDIV_MASK; - } else { - int div_low = prate / mode_rate; - - if (div_low >= 2 && - ((prate / div_low - mode_rate) < - 10 * (mode_rate - prate / div))) - /* - * At least 10 times better when using a higher - * frequency than requested, instead of a lower. - * So, go with that. - */ - div = div_low; } cfg |= ATMEL_HLCDC_CLKDIV(div); -- 2.7.4 ^ permalink raw reply related [flat|nested] 54+ messages in thread
* [PATCH 4/5] Revert "drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested" @ 2019-12-10 13:24 ` Claudiu Beznea 0 siblings, 0 replies; 54+ messages in thread From: Claudiu Beznea @ 2019-12-10 13:24 UTC (permalink / raw) To: sam, bbrezillon, airlied, daniel, nicolas.ferre, alexandre.belloni, ludovic.desroches, lee.jones Cc: Peter Rosin, Claudiu Beznea, linux-arm-kernel, dri-devel, linux-kernel This reverts commit f6f7ad3234613f6f7f27c25036aaf078de07e9b0. ("drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested") because allowing selecting a higher pixel clock may overclock LCD devices, not all of them being capable of this. Cc: Peter Rosin <peda@axentia.se> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> --- drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c index 721fa88bf71d..1a70dff1a417 100644 --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c @@ -117,18 +117,6 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c) div = DIV_ROUND_UP(prate, mode_rate); if (ATMEL_HLCDC_CLKDIV(div) & ~ATMEL_HLCDC_CLKDIV_MASK) div = ATMEL_HLCDC_CLKDIV_MASK; - } else { - int div_low = prate / mode_rate; - - if (div_low >= 2 && - ((prate / div_low - mode_rate) < - 10 * (mode_rate - prate / div))) - /* - * At least 10 times better when using a higher - * frequency than requested, instead of a lower. - * So, go with that. - */ - div = div_low; } cfg |= ATMEL_HLCDC_CLKDIV(div); -- 2.7.4 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 54+ messages in thread
* [PATCH 4/5] Revert "drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested" @ 2019-12-10 13:24 ` Claudiu Beznea 0 siblings, 0 replies; 54+ messages in thread From: Claudiu Beznea @ 2019-12-10 13:24 UTC (permalink / raw) To: sam, bbrezillon, airlied, daniel, nicolas.ferre, alexandre.belloni, ludovic.desroches, lee.jones Cc: Peter Rosin, Claudiu Beznea, linux-arm-kernel, dri-devel, linux-kernel This reverts commit f6f7ad3234613f6f7f27c25036aaf078de07e9b0. ("drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested") because allowing selecting a higher pixel clock may overclock LCD devices, not all of them being capable of this. Cc: Peter Rosin <peda@axentia.se> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> --- drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c index 721fa88bf71d..1a70dff1a417 100644 --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c @@ -117,18 +117,6 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c) div = DIV_ROUND_UP(prate, mode_rate); if (ATMEL_HLCDC_CLKDIV(div) & ~ATMEL_HLCDC_CLKDIV_MASK) div = ATMEL_HLCDC_CLKDIV_MASK; - } else { - int div_low = prate / mode_rate; - - if (div_low >= 2 && - ((prate / div_low - mode_rate) < - 10 * (mode_rate - prate / div))) - /* - * At least 10 times better when using a higher - * frequency than requested, instead of a lower. - * So, go with that. - */ - div = div_low; } cfg |= ATMEL_HLCDC_CLKDIV(div); -- 2.7.4 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 54+ messages in thread
* Re: [PATCH 4/5] Revert "drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested" 2019-12-10 13:24 ` Claudiu Beznea (?) @ 2019-12-10 14:11 ` Peter Rosin -1 siblings, 0 replies; 54+ messages in thread From: Peter Rosin @ 2019-12-10 14:11 UTC (permalink / raw) To: Claudiu Beznea, sam, bbrezillon, airlied, daniel, nicolas.ferre, alexandre.belloni, ludovic.desroches, lee.jones Cc: dri-devel, linux-arm-kernel, linux-kernel On 2019-12-10 14:24, Claudiu Beznea wrote: > This reverts commit f6f7ad3234613f6f7f27c25036aaf078de07e9b0. > ("drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested") > because allowing selecting a higher pixel clock may overclock > LCD devices, not all of them being capable of this. Without this patch, there are panels that are *severly* underclocked (on the magnitude of 40MHz instead of 65MHz or something like that, I don't remember the exact figures). And they are of course not capable of that. All panels have *some* slack as to what frequencies are supported, and the patch was written under the assumption that the preferred frequency of the panel was requested, which should leave at least a *little* headroom. So, I'm curious as to what panel regressed. Or rather, what pixel-clock it needs and what it gets with/without the patch? Or is the revert based on some theory of a perceived risk of toasting a panel? In short, this revert regresses my use case and I would like at least a hook to re-enable the removed logic. Cheers, Peter > Cc: Peter Rosin <peda@axentia.se> > Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> > --- > drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 12 ------------ > 1 file changed, 12 deletions(-) > > diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c > index 721fa88bf71d..1a70dff1a417 100644 > --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c > +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c > @@ -117,18 +117,6 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c) > div = DIV_ROUND_UP(prate, mode_rate); > if (ATMEL_HLCDC_CLKDIV(div) & ~ATMEL_HLCDC_CLKDIV_MASK) > div = ATMEL_HLCDC_CLKDIV_MASK; > - } else { > - int div_low = prate / mode_rate; > - > - if (div_low >= 2 && > - ((prate / div_low - mode_rate) < > - 10 * (mode_rate - prate / div))) > - /* > - * At least 10 times better when using a higher > - * frequency than requested, instead of a lower. > - * So, go with that. > - */ > - div = div_low; > } > > cfg |= ATMEL_HLCDC_CLKDIV(div); > ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 4/5] Revert "drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested" @ 2019-12-10 14:11 ` Peter Rosin 0 siblings, 0 replies; 54+ messages in thread From: Peter Rosin @ 2019-12-10 14:11 UTC (permalink / raw) To: Claudiu Beznea, sam, bbrezillon, airlied, daniel, nicolas.ferre, alexandre.belloni, ludovic.desroches, lee.jones Cc: linux-arm-kernel, dri-devel, linux-kernel On 2019-12-10 14:24, Claudiu Beznea wrote: > This reverts commit f6f7ad3234613f6f7f27c25036aaf078de07e9b0. > ("drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested") > because allowing selecting a higher pixel clock may overclock > LCD devices, not all of them being capable of this. Without this patch, there are panels that are *severly* underclocked (on the magnitude of 40MHz instead of 65MHz or something like that, I don't remember the exact figures). And they are of course not capable of that. All panels have *some* slack as to what frequencies are supported, and the patch was written under the assumption that the preferred frequency of the panel was requested, which should leave at least a *little* headroom. So, I'm curious as to what panel regressed. Or rather, what pixel-clock it needs and what it gets with/without the patch? Or is the revert based on some theory of a perceived risk of toasting a panel? In short, this revert regresses my use case and I would like at least a hook to re-enable the removed logic. Cheers, Peter > Cc: Peter Rosin <peda@axentia.se> > Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> > --- > drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 12 ------------ > 1 file changed, 12 deletions(-) > > diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c > index 721fa88bf71d..1a70dff1a417 100644 > --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c > +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c > @@ -117,18 +117,6 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c) > div = DIV_ROUND_UP(prate, mode_rate); > if (ATMEL_HLCDC_CLKDIV(div) & ~ATMEL_HLCDC_CLKDIV_MASK) > div = ATMEL_HLCDC_CLKDIV_MASK; > - } else { > - int div_low = prate / mode_rate; > - > - if (div_low >= 2 && > - ((prate / div_low - mode_rate) < > - 10 * (mode_rate - prate / div))) > - /* > - * At least 10 times better when using a higher > - * frequency than requested, instead of a lower. > - * So, go with that. > - */ > - div = div_low; > } > > cfg |= ATMEL_HLCDC_CLKDIV(div); > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 4/5] Revert "drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested" @ 2019-12-10 14:11 ` Peter Rosin 0 siblings, 0 replies; 54+ messages in thread From: Peter Rosin @ 2019-12-10 14:11 UTC (permalink / raw) To: Claudiu Beznea, sam, bbrezillon, airlied, daniel, nicolas.ferre, alexandre.belloni, ludovic.desroches, lee.jones Cc: linux-arm-kernel, dri-devel, linux-kernel On 2019-12-10 14:24, Claudiu Beznea wrote: > This reverts commit f6f7ad3234613f6f7f27c25036aaf078de07e9b0. > ("drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested") > because allowing selecting a higher pixel clock may overclock > LCD devices, not all of them being capable of this. Without this patch, there are panels that are *severly* underclocked (on the magnitude of 40MHz instead of 65MHz or something like that, I don't remember the exact figures). And they are of course not capable of that. All panels have *some* slack as to what frequencies are supported, and the patch was written under the assumption that the preferred frequency of the panel was requested, which should leave at least a *little* headroom. So, I'm curious as to what panel regressed. Or rather, what pixel-clock it needs and what it gets with/without the patch? Or is the revert based on some theory of a perceived risk of toasting a panel? In short, this revert regresses my use case and I would like at least a hook to re-enable the removed logic. Cheers, Peter > Cc: Peter Rosin <peda@axentia.se> > Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> > --- > drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 12 ------------ > 1 file changed, 12 deletions(-) > > diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c > index 721fa88bf71d..1a70dff1a417 100644 > --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c > +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c > @@ -117,18 +117,6 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c) > div = DIV_ROUND_UP(prate, mode_rate); > if (ATMEL_HLCDC_CLKDIV(div) & ~ATMEL_HLCDC_CLKDIV_MASK) > div = ATMEL_HLCDC_CLKDIV_MASK; > - } else { > - int div_low = prate / mode_rate; > - > - if (div_low >= 2 && > - ((prate / div_low - mode_rate) < > - 10 * (mode_rate - prate / div))) > - /* > - * At least 10 times better when using a higher > - * frequency than requested, instead of a lower. > - * So, go with that. > - */ > - div = div_low; > } > > cfg |= ATMEL_HLCDC_CLKDIV(div); > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 4/5] Revert "drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested" 2019-12-10 14:11 ` Peter Rosin (?) @ 2019-12-10 14:59 ` Claudiu.Beznea -1 siblings, 0 replies; 54+ messages in thread From: Claudiu.Beznea @ 2019-12-10 14:59 UTC (permalink / raw) To: peda, sam, bbrezillon, airlied, daniel, Nicolas.Ferre, alexandre.belloni, Ludovic.Desroches, lee.jones Cc: dri-devel, linux-arm-kernel, linux-kernel On 10.12.2019 16:11, Peter Rosin wrote: > On 2019-12-10 14:24, Claudiu Beznea wrote: >> This reverts commit f6f7ad3234613f6f7f27c25036aaf078de07e9b0. >> ("drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested") >> because allowing selecting a higher pixel clock may overclock >> LCD devices, not all of them being capable of this. > > Without this patch, there are panels that are *severly* underclocked (on the > magnitude of 40MHz instead of 65MHz or something like that, I don't remember > the exact figures). With patch that switches by default to 2xsystem clock for pixel clock, if using 133MHz system clock (as you specified in the patch I proposed for revert here) that would go, without this patch at 53MHz if 65MHz is requested. Correct me if I'm wrong. > And they are of course not capable of that. All panels > have *some* slack as to what frequencies are supported, and the patch was > written under the assumption that the preferred frequency of the panel was > requested, which should leave at least a *little* headroom. I see, but from my point of view, the upper layers should decide what frequency settings should be done on the LCD controller and not let this at the driver's latitude. > > So, I'm curious as to what panel regressed. Or rather, what pixel-clock it needs > and what it gets with/without the patch? I have 2 use cases: 1/ system clock = 200MHz and requested pixel clock (mode_rate) ~71MHz. With the reverted patch the resulted computed pixel clock would be 80MHz. Previously it was at 66MHz 2/ system clock = 133MHz and the requested pixel clock (mode_rate) 60MHz. With the reverted patch the computed pixel clock would be 66MHz. I took into account the patch that uses by default 2xsystem clock as pixel clock (and this was on a system that supported it). > > Or is the revert based on some theory of a perceived risk of toasting a panel? It's based on the use cases I mentioned above. > > In short, this revert regresses my use case and I would like at least a hook to > re-enable the removed logic. I see, but, FMPOV, you have to take into account that some of the devices don't support it. Thank you, Claudiu Beznea > > Cheers, > Peter > >> Cc: Peter Rosin <peda@axentia.se> >> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> >> --- >> drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 12 ------------ >> 1 file changed, 12 deletions(-) >> >> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c >> index 721fa88bf71d..1a70dff1a417 100644 >> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c >> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c >> @@ -117,18 +117,6 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c) >> div = DIV_ROUND_UP(prate, mode_rate); >> if (ATMEL_HLCDC_CLKDIV(div) & ~ATMEL_HLCDC_CLKDIV_MASK) >> div = ATMEL_HLCDC_CLKDIV_MASK; >> - } else { >> - int div_low = prate / mode_rate; >> - >> - if (div_low >= 2 && >> - ((prate / div_low - mode_rate) < >> - 10 * (mode_rate - prate / div))) >> - /* >> - * At least 10 times better when using a higher >> - * frequency than requested, instead of a lower. >> - * So, go with that. >> - */ >> - div = div_low; >> } >> >> cfg |= ATMEL_HLCDC_CLKDIV(div); >> > ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 4/5] Revert "drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested" @ 2019-12-10 14:59 ` Claudiu.Beznea 0 siblings, 0 replies; 54+ messages in thread From: Claudiu.Beznea @ 2019-12-10 14:59 UTC (permalink / raw) To: peda, sam, bbrezillon, airlied, daniel, Nicolas.Ferre, alexandre.belloni, Ludovic.Desroches, lee.jones Cc: linux-arm-kernel, dri-devel, linux-kernel On 10.12.2019 16:11, Peter Rosin wrote: > On 2019-12-10 14:24, Claudiu Beznea wrote: >> This reverts commit f6f7ad3234613f6f7f27c25036aaf078de07e9b0. >> ("drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested") >> because allowing selecting a higher pixel clock may overclock >> LCD devices, not all of them being capable of this. > > Without this patch, there are panels that are *severly* underclocked (on the > magnitude of 40MHz instead of 65MHz or something like that, I don't remember > the exact figures). With patch that switches by default to 2xsystem clock for pixel clock, if using 133MHz system clock (as you specified in the patch I proposed for revert here) that would go, without this patch at 53MHz if 65MHz is requested. Correct me if I'm wrong. > And they are of course not capable of that. All panels > have *some* slack as to what frequencies are supported, and the patch was > written under the assumption that the preferred frequency of the panel was > requested, which should leave at least a *little* headroom. I see, but from my point of view, the upper layers should decide what frequency settings should be done on the LCD controller and not let this at the driver's latitude. > > So, I'm curious as to what panel regressed. Or rather, what pixel-clock it needs > and what it gets with/without the patch? I have 2 use cases: 1/ system clock = 200MHz and requested pixel clock (mode_rate) ~71MHz. With the reverted patch the resulted computed pixel clock would be 80MHz. Previously it was at 66MHz 2/ system clock = 133MHz and the requested pixel clock (mode_rate) 60MHz. With the reverted patch the computed pixel clock would be 66MHz. I took into account the patch that uses by default 2xsystem clock as pixel clock (and this was on a system that supported it). > > Or is the revert based on some theory of a perceived risk of toasting a panel? It's based on the use cases I mentioned above. > > In short, this revert regresses my use case and I would like at least a hook to > re-enable the removed logic. I see, but, FMPOV, you have to take into account that some of the devices don't support it. Thank you, Claudiu Beznea > > Cheers, > Peter > >> Cc: Peter Rosin <peda@axentia.se> >> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> >> --- >> drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 12 ------------ >> 1 file changed, 12 deletions(-) >> >> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c >> index 721fa88bf71d..1a70dff1a417 100644 >> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c >> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c >> @@ -117,18 +117,6 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c) >> div = DIV_ROUND_UP(prate, mode_rate); >> if (ATMEL_HLCDC_CLKDIV(div) & ~ATMEL_HLCDC_CLKDIV_MASK) >> div = ATMEL_HLCDC_CLKDIV_MASK; >> - } else { >> - int div_low = prate / mode_rate; >> - >> - if (div_low >= 2 && >> - ((prate / div_low - mode_rate) < >> - 10 * (mode_rate - prate / div))) >> - /* >> - * At least 10 times better when using a higher >> - * frequency than requested, instead of a lower. >> - * So, go with that. >> - */ >> - div = div_low; >> } >> >> cfg |= ATMEL_HLCDC_CLKDIV(div); >> > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 4/5] Revert "drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested" @ 2019-12-10 14:59 ` Claudiu.Beznea 0 siblings, 0 replies; 54+ messages in thread From: Claudiu.Beznea @ 2019-12-10 14:59 UTC (permalink / raw) To: peda, sam, bbrezillon, airlied, daniel, Nicolas.Ferre, alexandre.belloni, Ludovic.Desroches, lee.jones Cc: linux-arm-kernel, dri-devel, linux-kernel On 10.12.2019 16:11, Peter Rosin wrote: > On 2019-12-10 14:24, Claudiu Beznea wrote: >> This reverts commit f6f7ad3234613f6f7f27c25036aaf078de07e9b0. >> ("drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested") >> because allowing selecting a higher pixel clock may overclock >> LCD devices, not all of them being capable of this. > > Without this patch, there are panels that are *severly* underclocked (on the > magnitude of 40MHz instead of 65MHz or something like that, I don't remember > the exact figures). With patch that switches by default to 2xsystem clock for pixel clock, if using 133MHz system clock (as you specified in the patch I proposed for revert here) that would go, without this patch at 53MHz if 65MHz is requested. Correct me if I'm wrong. > And they are of course not capable of that. All panels > have *some* slack as to what frequencies are supported, and the patch was > written under the assumption that the preferred frequency of the panel was > requested, which should leave at least a *little* headroom. I see, but from my point of view, the upper layers should decide what frequency settings should be done on the LCD controller and not let this at the driver's latitude. > > So, I'm curious as to what panel regressed. Or rather, what pixel-clock it needs > and what it gets with/without the patch? I have 2 use cases: 1/ system clock = 200MHz and requested pixel clock (mode_rate) ~71MHz. With the reverted patch the resulted computed pixel clock would be 80MHz. Previously it was at 66MHz 2/ system clock = 133MHz and the requested pixel clock (mode_rate) 60MHz. With the reverted patch the computed pixel clock would be 66MHz. I took into account the patch that uses by default 2xsystem clock as pixel clock (and this was on a system that supported it). > > Or is the revert based on some theory of a perceived risk of toasting a panel? It's based on the use cases I mentioned above. > > In short, this revert regresses my use case and I would like at least a hook to > re-enable the removed logic. I see, but, FMPOV, you have to take into account that some of the devices don't support it. Thank you, Claudiu Beznea > > Cheers, > Peter > >> Cc: Peter Rosin <peda@axentia.se> >> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> >> --- >> drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 12 ------------ >> 1 file changed, 12 deletions(-) >> >> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c >> index 721fa88bf71d..1a70dff1a417 100644 >> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c >> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c >> @@ -117,18 +117,6 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c) >> div = DIV_ROUND_UP(prate, mode_rate); >> if (ATMEL_HLCDC_CLKDIV(div) & ~ATMEL_HLCDC_CLKDIV_MASK) >> div = ATMEL_HLCDC_CLKDIV_MASK; >> - } else { >> - int div_low = prate / mode_rate; >> - >> - if (div_low >= 2 && >> - ((prate / div_low - mode_rate) < >> - 10 * (mode_rate - prate / div))) >> - /* >> - * At least 10 times better when using a higher >> - * frequency than requested, instead of a lower. >> - * So, go with that. >> - */ >> - div = div_low; >> } >> >> cfg |= ATMEL_HLCDC_CLKDIV(div); >> > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 4/5] Revert "drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested" 2019-12-10 14:59 ` Claudiu.Beznea (?) @ 2019-12-10 17:22 ` Peter Rosin -1 siblings, 0 replies; 54+ messages in thread From: Peter Rosin @ 2019-12-10 17:22 UTC (permalink / raw) To: Claudiu.Beznea, sam, bbrezillon, airlied, daniel, Nicolas.Ferre, alexandre.belloni, Ludovic.Desroches, lee.jones Cc: dri-devel, linux-arm-kernel, linux-kernel On 2019-12-10 15:59, Claudiu.Beznea@microchip.com wrote: > > > On 10.12.2019 16:11, Peter Rosin wrote: >> On 2019-12-10 14:24, Claudiu Beznea wrote: >>> This reverts commit f6f7ad3234613f6f7f27c25036aaf078de07e9b0. >>> ("drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested") >>> because allowing selecting a higher pixel clock may overclock >>> LCD devices, not all of them being capable of this. >> >> Without this patch, there are panels that are *severly* underclocked (on the >> magnitude of 40MHz instead of 65MHz or something like that, I don't remember >> the exact figures). > > With patch that switches by default to 2xsystem clock for pixel clock, if > using 133MHz system clock (as you specified in the patch I proposed for > revert here) that would go, without this patch at 53MHz if 65MHz is > requested. Correct me if I'm wrong. It might have been 53MHz, whatever it was it was too low for things to work. >> And they are of course not capable of that. All panels >> have *some* slack as to what frequencies are supported, and the patch was >> written under the assumption that the preferred frequency of the panel was >> requested, which should leave at least a *little* headroom. > > I see, but from my point of view, the upper layers should decide what > frequency settings should be done on the LCD controller and not let this at > the driver's latitude. Right, but the upper layers do not support negotiating a frequency from ranges. At least the didn't when the patch was written, and implementing *that* seemed like a huge undertaking. >> >> So, I'm curious as to what panel regressed. Or rather, what pixel-clock it needs >> and what it gets with/without the patch? > > I have 2 use cases: > 1/ system clock = 200MHz and requested pixel clock (mode_rate) ~71MHz. With > the reverted patch the resulted computed pixel clock would be 80MHz. > Previously it was at 66MHz I don't see how that's possible. [doing some calculation by hand] Arrgh. *blush* The code does not do what I intended for it to do. Can you please try this instead of reverting? Cheers, Peter From b3e86d55b8d107a5c07e98f879c67f67120c87a6 Mon Sep 17 00:00:00 2001 From: Peter Rosin <peda@axentia.se> Date: Tue, 10 Dec 2019 18:11:28 +0100 Subject: [PATCH] drm/atmel-hlcdc: prefer a lower pixel-clock than requested The intention was to only select a higher pixel-clock rate than the requested, if a slight overclocking would result in a rate significantly closer to the requested rate than if the conservative lower pixel-clock rate is selected. The fixed patch has the logic the other way around and actually prefers the higher frequency. Fix that. Fixes: f6f7ad323461 ("drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested") Reported-by: Claudiu Beznea <claudiu.beznea@microchip.com> Signed-off-by: Peter Rosin <peda@axentia.se> --- drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c index 9e34bce089d0..03691845d37a 100644 --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c @@ -120,8 +120,8 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c) int div_low = prate / mode_rate; if (div_low >= 2 && - ((prate / div_low - mode_rate) < - 10 * (mode_rate - prate / div))) + (10 * (prate / div_low - mode_rate) < + (mode_rate - prate / div))) /* * At least 10 times better when using a higher * frequency than requested, instead of a lower. -- 2.20.1 ^ permalink raw reply related [flat|nested] 54+ messages in thread
* Re: [PATCH 4/5] Revert "drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested" @ 2019-12-10 17:22 ` Peter Rosin 0 siblings, 0 replies; 54+ messages in thread From: Peter Rosin @ 2019-12-10 17:22 UTC (permalink / raw) To: Claudiu.Beznea, sam, bbrezillon, airlied, daniel, Nicolas.Ferre, alexandre.belloni, Ludovic.Desroches, lee.jones Cc: linux-arm-kernel, dri-devel, linux-kernel On 2019-12-10 15:59, Claudiu.Beznea@microchip.com wrote: > > > On 10.12.2019 16:11, Peter Rosin wrote: >> On 2019-12-10 14:24, Claudiu Beznea wrote: >>> This reverts commit f6f7ad3234613f6f7f27c25036aaf078de07e9b0. >>> ("drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested") >>> because allowing selecting a higher pixel clock may overclock >>> LCD devices, not all of them being capable of this. >> >> Without this patch, there are panels that are *severly* underclocked (on the >> magnitude of 40MHz instead of 65MHz or something like that, I don't remember >> the exact figures). > > With patch that switches by default to 2xsystem clock for pixel clock, if > using 133MHz system clock (as you specified in the patch I proposed for > revert here) that would go, without this patch at 53MHz if 65MHz is > requested. Correct me if I'm wrong. It might have been 53MHz, whatever it was it was too low for things to work. >> And they are of course not capable of that. All panels >> have *some* slack as to what frequencies are supported, and the patch was >> written under the assumption that the preferred frequency of the panel was >> requested, which should leave at least a *little* headroom. > > I see, but from my point of view, the upper layers should decide what > frequency settings should be done on the LCD controller and not let this at > the driver's latitude. Right, but the upper layers do not support negotiating a frequency from ranges. At least the didn't when the patch was written, and implementing *that* seemed like a huge undertaking. >> >> So, I'm curious as to what panel regressed. Or rather, what pixel-clock it needs >> and what it gets with/without the patch? > > I have 2 use cases: > 1/ system clock = 200MHz and requested pixel clock (mode_rate) ~71MHz. With > the reverted patch the resulted computed pixel clock would be 80MHz. > Previously it was at 66MHz I don't see how that's possible. [doing some calculation by hand] Arrgh. *blush* The code does not do what I intended for it to do. Can you please try this instead of reverting? Cheers, Peter From b3e86d55b8d107a5c07e98f879c67f67120c87a6 Mon Sep 17 00:00:00 2001 From: Peter Rosin <peda@axentia.se> Date: Tue, 10 Dec 2019 18:11:28 +0100 Subject: [PATCH] drm/atmel-hlcdc: prefer a lower pixel-clock than requested The intention was to only select a higher pixel-clock rate than the requested, if a slight overclocking would result in a rate significantly closer to the requested rate than if the conservative lower pixel-clock rate is selected. The fixed patch has the logic the other way around and actually prefers the higher frequency. Fix that. Fixes: f6f7ad323461 ("drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested") Reported-by: Claudiu Beznea <claudiu.beznea@microchip.com> Signed-off-by: Peter Rosin <peda@axentia.se> --- drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c index 9e34bce089d0..03691845d37a 100644 --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c @@ -120,8 +120,8 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c) int div_low = prate / mode_rate; if (div_low >= 2 && - ((prate / div_low - mode_rate) < - 10 * (mode_rate - prate / div))) + (10 * (prate / div_low - mode_rate) < + (mode_rate - prate / div))) /* * At least 10 times better when using a higher * frequency than requested, instead of a lower. -- 2.20.1 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 54+ messages in thread
* Re: [PATCH 4/5] Revert "drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested" @ 2019-12-10 17:22 ` Peter Rosin 0 siblings, 0 replies; 54+ messages in thread From: Peter Rosin @ 2019-12-10 17:22 UTC (permalink / raw) To: Claudiu.Beznea, sam, bbrezillon, airlied, daniel, Nicolas.Ferre, alexandre.belloni, Ludovic.Desroches, lee.jones Cc: linux-arm-kernel, dri-devel, linux-kernel On 2019-12-10 15:59, Claudiu.Beznea@microchip.com wrote: > > > On 10.12.2019 16:11, Peter Rosin wrote: >> On 2019-12-10 14:24, Claudiu Beznea wrote: >>> This reverts commit f6f7ad3234613f6f7f27c25036aaf078de07e9b0. >>> ("drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested") >>> because allowing selecting a higher pixel clock may overclock >>> LCD devices, not all of them being capable of this. >> >> Without this patch, there are panels that are *severly* underclocked (on the >> magnitude of 40MHz instead of 65MHz or something like that, I don't remember >> the exact figures). > > With patch that switches by default to 2xsystem clock for pixel clock, if > using 133MHz system clock (as you specified in the patch I proposed for > revert here) that would go, without this patch at 53MHz if 65MHz is > requested. Correct me if I'm wrong. It might have been 53MHz, whatever it was it was too low for things to work. >> And they are of course not capable of that. All panels >> have *some* slack as to what frequencies are supported, and the patch was >> written under the assumption that the preferred frequency of the panel was >> requested, which should leave at least a *little* headroom. > > I see, but from my point of view, the upper layers should decide what > frequency settings should be done on the LCD controller and not let this at > the driver's latitude. Right, but the upper layers do not support negotiating a frequency from ranges. At least the didn't when the patch was written, and implementing *that* seemed like a huge undertaking. >> >> So, I'm curious as to what panel regressed. Or rather, what pixel-clock it needs >> and what it gets with/without the patch? > > I have 2 use cases: > 1/ system clock = 200MHz and requested pixel clock (mode_rate) ~71MHz. With > the reverted patch the resulted computed pixel clock would be 80MHz. > Previously it was at 66MHz I don't see how that's possible. [doing some calculation by hand] Arrgh. *blush* The code does not do what I intended for it to do. Can you please try this instead of reverting? Cheers, Peter From b3e86d55b8d107a5c07e98f879c67f67120c87a6 Mon Sep 17 00:00:00 2001 From: Peter Rosin <peda@axentia.se> Date: Tue, 10 Dec 2019 18:11:28 +0100 Subject: [PATCH] drm/atmel-hlcdc: prefer a lower pixel-clock than requested The intention was to only select a higher pixel-clock rate than the requested, if a slight overclocking would result in a rate significantly closer to the requested rate than if the conservative lower pixel-clock rate is selected. The fixed patch has the logic the other way around and actually prefers the higher frequency. Fix that. Fixes: f6f7ad323461 ("drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested") Reported-by: Claudiu Beznea <claudiu.beznea@microchip.com> Signed-off-by: Peter Rosin <peda@axentia.se> --- drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c index 9e34bce089d0..03691845d37a 100644 --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c @@ -120,8 +120,8 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c) int div_low = prate / mode_rate; if (div_low >= 2 && - ((prate / div_low - mode_rate) < - 10 * (mode_rate - prate / div))) + (10 * (prate / div_low - mode_rate) < + (mode_rate - prate / div))) /* * At least 10 times better when using a higher * frequency than requested, instead of a lower. -- 2.20.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 54+ messages in thread
* Re: [PATCH 4/5] Revert "drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested" 2019-12-10 17:22 ` Peter Rosin (?) @ 2019-12-11 11:45 ` Claudiu.Beznea -1 siblings, 0 replies; 54+ messages in thread From: Claudiu.Beznea @ 2019-12-11 11:45 UTC (permalink / raw) To: peda, sam, bbrezillon, airlied, daniel, Nicolas.Ferre, alexandre.belloni, Ludovic.Desroches, lee.jones Cc: dri-devel, linux-arm-kernel, linux-kernel On 10.12.2019 19:22, Peter Rosin wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On 2019-12-10 15:59, Claudiu.Beznea@microchip.com wrote: >> >> >> On 10.12.2019 16:11, Peter Rosin wrote: >>> On 2019-12-10 14:24, Claudiu Beznea wrote: >>>> This reverts commit f6f7ad3234613f6f7f27c25036aaf078de07e9b0. >>>> ("drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested") >>>> because allowing selecting a higher pixel clock may overclock >>>> LCD devices, not all of them being capable of this. >>> >>> Without this patch, there are panels that are *severly* underclocked (on the >>> magnitude of 40MHz instead of 65MHz or something like that, I don't remember >>> the exact figures). >> >> With patch that switches by default to 2xsystem clock for pixel clock, if >> using 133MHz system clock (as you specified in the patch I proposed for >> revert here) that would go, without this patch at 53MHz if 65MHz is >> requested. Correct me if I'm wrong. > > It might have been 53MHz, whatever it was it was too low for things to work. > >>> And they are of course not capable of that. All panels >>> have *some* slack as to what frequencies are supported, and the patch was >>> written under the assumption that the preferred frequency of the panel was >>> requested, which should leave at least a *little* headroom. >> >> I see, but from my point of view, the upper layers should decide what >> frequency settings should be done on the LCD controller and not let this at >> the driver's latitude. > > Right, but the upper layers do not support negotiating a frequency from > ranges. At least the didn't when the patch was written, and implementing > *that* seemed like a huge undertaking. > >>> >>> So, I'm curious as to what panel regressed. Or rather, what pixel-clock it needs >>> and what it gets with/without the patch? >> >> I have 2 use cases: >> 1/ system clock = 200MHz and requested pixel clock (mode_rate) ~71MHz. With >> the reverted patch the resulted computed pixel clock would be 80MHz. >> Previously it was at 66MHz > > I don't see how that's possible. > > [doing some calculation by hand] > > Arrgh. *blush* > > The code does not do what I intended for it to do. > Can you please try this instead of reverting? > > Cheers, > Peter > > From b3e86d55b8d107a5c07e98f879c67f67120c87a6 Mon Sep 17 00:00:00 2001 > From: Peter Rosin <peda@axentia.se> > Date: Tue, 10 Dec 2019 18:11:28 +0100 > Subject: [PATCH] drm/atmel-hlcdc: prefer a lower pixel-clock than requested > > The intention was to only select a higher pixel-clock rate than the > requested, if a slight overclocking would result in a rate significantly > closer to the requested rate than if the conservative lower pixel-clock > rate is selected. The fixed patch has the logic the other way around and > actually prefers the higher frequency. Fix that. > > Fixes: f6f7ad323461 ("drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested") > Reported-by: Claudiu Beznea <claudiu.beznea@microchip.com> > Signed-off-by: Peter Rosin <peda@axentia.se> > --- > drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c > index 9e34bce089d0..03691845d37a 100644 > --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c > +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c > @@ -120,8 +120,8 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c) > int div_low = prate / mode_rate; > > if (div_low >= 2 && > - ((prate / div_low - mode_rate) < > - 10 * (mode_rate - prate / div))) > + (10 * (prate / div_low - mode_rate) < > + (mode_rate - prate / div))) I tested it on my setup (I have only one of those specified above) and it is OK. Doing some math for the other setup it should also be OK. As a whole, I'm OK with this at the moment (let's hope it will work for all use-cases) but still I am not OK with selecting here, in the driver, something that might work. Although I am not familiar with how other DRM drivers are handling this kind of scenarios. Maybe you and/or other DRM guys knows more about it. Just as a notice, it may worth adding a print message saying what was frequency was requested and what frequency has been setup by driver. > /* > * At least 10 times better when using a higher > * frequency than requested, instead of a lower. > -- > 2.20.1 > ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 4/5] Revert "drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested" @ 2019-12-11 11:45 ` Claudiu.Beznea 0 siblings, 0 replies; 54+ messages in thread From: Claudiu.Beznea @ 2019-12-11 11:45 UTC (permalink / raw) To: peda, sam, bbrezillon, airlied, daniel, Nicolas.Ferre, alexandre.belloni, Ludovic.Desroches, lee.jones Cc: linux-arm-kernel, dri-devel, linux-kernel On 10.12.2019 19:22, Peter Rosin wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On 2019-12-10 15:59, Claudiu.Beznea@microchip.com wrote: >> >> >> On 10.12.2019 16:11, Peter Rosin wrote: >>> On 2019-12-10 14:24, Claudiu Beznea wrote: >>>> This reverts commit f6f7ad3234613f6f7f27c25036aaf078de07e9b0. >>>> ("drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested") >>>> because allowing selecting a higher pixel clock may overclock >>>> LCD devices, not all of them being capable of this. >>> >>> Without this patch, there are panels that are *severly* underclocked (on the >>> magnitude of 40MHz instead of 65MHz or something like that, I don't remember >>> the exact figures). >> >> With patch that switches by default to 2xsystem clock for pixel clock, if >> using 133MHz system clock (as you specified in the patch I proposed for >> revert here) that would go, without this patch at 53MHz if 65MHz is >> requested. Correct me if I'm wrong. > > It might have been 53MHz, whatever it was it was too low for things to work. > >>> And they are of course not capable of that. All panels >>> have *some* slack as to what frequencies are supported, and the patch was >>> written under the assumption that the preferred frequency of the panel was >>> requested, which should leave at least a *little* headroom. >> >> I see, but from my point of view, the upper layers should decide what >> frequency settings should be done on the LCD controller and not let this at >> the driver's latitude. > > Right, but the upper layers do not support negotiating a frequency from > ranges. At least the didn't when the patch was written, and implementing > *that* seemed like a huge undertaking. > >>> >>> So, I'm curious as to what panel regressed. Or rather, what pixel-clock it needs >>> and what it gets with/without the patch? >> >> I have 2 use cases: >> 1/ system clock = 200MHz and requested pixel clock (mode_rate) ~71MHz. With >> the reverted patch the resulted computed pixel clock would be 80MHz. >> Previously it was at 66MHz > > I don't see how that's possible. > > [doing some calculation by hand] > > Arrgh. *blush* > > The code does not do what I intended for it to do. > Can you please try this instead of reverting? > > Cheers, > Peter > > From b3e86d55b8d107a5c07e98f879c67f67120c87a6 Mon Sep 17 00:00:00 2001 > From: Peter Rosin <peda@axentia.se> > Date: Tue, 10 Dec 2019 18:11:28 +0100 > Subject: [PATCH] drm/atmel-hlcdc: prefer a lower pixel-clock than requested > > The intention was to only select a higher pixel-clock rate than the > requested, if a slight overclocking would result in a rate significantly > closer to the requested rate than if the conservative lower pixel-clock > rate is selected. The fixed patch has the logic the other way around and > actually prefers the higher frequency. Fix that. > > Fixes: f6f7ad323461 ("drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested") > Reported-by: Claudiu Beznea <claudiu.beznea@microchip.com> > Signed-off-by: Peter Rosin <peda@axentia.se> > --- > drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c > index 9e34bce089d0..03691845d37a 100644 > --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c > +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c > @@ -120,8 +120,8 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c) > int div_low = prate / mode_rate; > > if (div_low >= 2 && > - ((prate / div_low - mode_rate) < > - 10 * (mode_rate - prate / div))) > + (10 * (prate / div_low - mode_rate) < > + (mode_rate - prate / div))) I tested it on my setup (I have only one of those specified above) and it is OK. Doing some math for the other setup it should also be OK. As a whole, I'm OK with this at the moment (let's hope it will work for all use-cases) but still I am not OK with selecting here, in the driver, something that might work. Although I am not familiar with how other DRM drivers are handling this kind of scenarios. Maybe you and/or other DRM guys knows more about it. Just as a notice, it may worth adding a print message saying what was frequency was requested and what frequency has been setup by driver. > /* > * At least 10 times better when using a higher > * frequency than requested, instead of a lower. > -- > 2.20.1 > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 4/5] Revert "drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested" @ 2019-12-11 11:45 ` Claudiu.Beznea 0 siblings, 0 replies; 54+ messages in thread From: Claudiu.Beznea @ 2019-12-11 11:45 UTC (permalink / raw) To: peda, sam, bbrezillon, airlied, daniel, Nicolas.Ferre, alexandre.belloni, Ludovic.Desroches, lee.jones Cc: linux-arm-kernel, dri-devel, linux-kernel On 10.12.2019 19:22, Peter Rosin wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On 2019-12-10 15:59, Claudiu.Beznea@microchip.com wrote: >> >> >> On 10.12.2019 16:11, Peter Rosin wrote: >>> On 2019-12-10 14:24, Claudiu Beznea wrote: >>>> This reverts commit f6f7ad3234613f6f7f27c25036aaf078de07e9b0. >>>> ("drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested") >>>> because allowing selecting a higher pixel clock may overclock >>>> LCD devices, not all of them being capable of this. >>> >>> Without this patch, there are panels that are *severly* underclocked (on the >>> magnitude of 40MHz instead of 65MHz or something like that, I don't remember >>> the exact figures). >> >> With patch that switches by default to 2xsystem clock for pixel clock, if >> using 133MHz system clock (as you specified in the patch I proposed for >> revert here) that would go, without this patch at 53MHz if 65MHz is >> requested. Correct me if I'm wrong. > > It might have been 53MHz, whatever it was it was too low for things to work. > >>> And they are of course not capable of that. All panels >>> have *some* slack as to what frequencies are supported, and the patch was >>> written under the assumption that the preferred frequency of the panel was >>> requested, which should leave at least a *little* headroom. >> >> I see, but from my point of view, the upper layers should decide what >> frequency settings should be done on the LCD controller and not let this at >> the driver's latitude. > > Right, but the upper layers do not support negotiating a frequency from > ranges. At least the didn't when the patch was written, and implementing > *that* seemed like a huge undertaking. > >>> >>> So, I'm curious as to what panel regressed. Or rather, what pixel-clock it needs >>> and what it gets with/without the patch? >> >> I have 2 use cases: >> 1/ system clock = 200MHz and requested pixel clock (mode_rate) ~71MHz. With >> the reverted patch the resulted computed pixel clock would be 80MHz. >> Previously it was at 66MHz > > I don't see how that's possible. > > [doing some calculation by hand] > > Arrgh. *blush* > > The code does not do what I intended for it to do. > Can you please try this instead of reverting? > > Cheers, > Peter > > From b3e86d55b8d107a5c07e98f879c67f67120c87a6 Mon Sep 17 00:00:00 2001 > From: Peter Rosin <peda@axentia.se> > Date: Tue, 10 Dec 2019 18:11:28 +0100 > Subject: [PATCH] drm/atmel-hlcdc: prefer a lower pixel-clock than requested > > The intention was to only select a higher pixel-clock rate than the > requested, if a slight overclocking would result in a rate significantly > closer to the requested rate than if the conservative lower pixel-clock > rate is selected. The fixed patch has the logic the other way around and > actually prefers the higher frequency. Fix that. > > Fixes: f6f7ad323461 ("drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested") > Reported-by: Claudiu Beznea <claudiu.beznea@microchip.com> > Signed-off-by: Peter Rosin <peda@axentia.se> > --- > drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c > index 9e34bce089d0..03691845d37a 100644 > --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c > +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c > @@ -120,8 +120,8 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c) > int div_low = prate / mode_rate; > > if (div_low >= 2 && > - ((prate / div_low - mode_rate) < > - 10 * (mode_rate - prate / div))) > + (10 * (prate / div_low - mode_rate) < > + (mode_rate - prate / div))) I tested it on my setup (I have only one of those specified above) and it is OK. Doing some math for the other setup it should also be OK. As a whole, I'm OK with this at the moment (let's hope it will work for all use-cases) but still I am not OK with selecting here, in the driver, something that might work. Although I am not familiar with how other DRM drivers are handling this kind of scenarios. Maybe you and/or other DRM guys knows more about it. Just as a notice, it may worth adding a print message saying what was frequency was requested and what frequency has been setup by driver. > /* > * At least 10 times better when using a higher > * frequency than requested, instead of a lower. > -- > 2.20.1 > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 4/5] Revert "drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested" 2019-12-11 11:45 ` Claudiu.Beznea (?) @ 2019-12-11 13:28 ` Peter Rosin -1 siblings, 0 replies; 54+ messages in thread From: Peter Rosin @ 2019-12-11 13:28 UTC (permalink / raw) To: Claudiu.Beznea, sam, bbrezillon, airlied, daniel, Nicolas.Ferre, alexandre.belloni, Ludovic.Desroches, lee.jones Cc: dri-devel, linux-arm-kernel, linux-kernel On 2019-12-11 12:45, Claudiu.Beznea@microchip.com wrote: > > > On 10.12.2019 19:22, Peter Rosin wrote: >> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >> >> On 2019-12-10 15:59, Claudiu.Beznea@microchip.com wrote: >>> >>> >>> On 10.12.2019 16:11, Peter Rosin wrote: >>>> On 2019-12-10 14:24, Claudiu Beznea wrote: >>>>> This reverts commit f6f7ad3234613f6f7f27c25036aaf078de07e9b0. >>>>> ("drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested") >>>>> because allowing selecting a higher pixel clock may overclock >>>>> LCD devices, not all of them being capable of this. >>>> >>>> Without this patch, there are panels that are *severly* underclocked (on the >>>> magnitude of 40MHz instead of 65MHz or something like that, I don't remember >>>> the exact figures). >>> >>> With patch that switches by default to 2xsystem clock for pixel clock, if >>> using 133MHz system clock (as you specified in the patch I proposed for >>> revert here) that would go, without this patch at 53MHz if 65MHz is >>> requested. Correct me if I'm wrong. >> >> It might have been 53MHz, whatever it was it was too low for things to work. >> >>>> And they are of course not capable of that. All panels >>>> have *some* slack as to what frequencies are supported, and the patch was >>>> written under the assumption that the preferred frequency of the panel was >>>> requested, which should leave at least a *little* headroom. >>> >>> I see, but from my point of view, the upper layers should decide what >>> frequency settings should be done on the LCD controller and not let this at >>> the driver's latitude. >> >> Right, but the upper layers do not support negotiating a frequency from >> ranges. At least the didn't when the patch was written, and implementing >> *that* seemed like a huge undertaking. >> >>>> >>>> So, I'm curious as to what panel regressed. Or rather, what pixel-clock it needs >>>> and what it gets with/without the patch? >>> >>> I have 2 use cases: >>> 1/ system clock = 200MHz and requested pixel clock (mode_rate) ~71MHz. With >>> the reverted patch the resulted computed pixel clock would be 80MHz. >>> Previously it was at 66MHz >> >> I don't see how that's possible. >> >> [doing some calculation by hand] >> >> Arrgh. *blush* >> >> The code does not do what I intended for it to do. >> Can you please try this instead of reverting? >> >> Cheers, >> Peter >> >> From b3e86d55b8d107a5c07e98f879c67f67120c87a6 Mon Sep 17 00:00:00 2001 >> From: Peter Rosin <peda@axentia.se> >> Date: Tue, 10 Dec 2019 18:11:28 +0100 >> Subject: [PATCH] drm/atmel-hlcdc: prefer a lower pixel-clock than requested >> >> The intention was to only select a higher pixel-clock rate than the >> requested, if a slight overclocking would result in a rate significantly >> closer to the requested rate than if the conservative lower pixel-clock >> rate is selected. The fixed patch has the logic the other way around and >> actually prefers the higher frequency. Fix that. >> >> Fixes: f6f7ad323461 ("drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested") >> Reported-by: Claudiu Beznea <claudiu.beznea@microchip.com> >> Signed-off-by: Peter Rosin <peda@axentia.se> >> --- >> drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c >> index 9e34bce089d0..03691845d37a 100644 >> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c >> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c >> @@ -120,8 +120,8 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c) >> int div_low = prate / mode_rate; >> >> if (div_low >= 2 && >> - ((prate / div_low - mode_rate) < >> - 10 * (mode_rate - prate / div))) >> + (10 * (prate / div_low - mode_rate) < >> + (mode_rate - prate / div))) > > I tested it on my setup (I have only one of those specified above) and it > is OK. Doing some math for the other setup it should also be OK. Glad to hear it, and thanks for testing/verifying! > As a whole, I'm OK with this at the moment (let's hope it will work for all > use-cases) but still I am not OK with selecting here, in the driver, > something that might work. The driver has to select *something*. If it can deliver the exact requested frequency, fine. Otherwise? What should it do? Bail out? Why is 53MHz better and more likely to produce a picture than 66MHz, when 65MHz is requested? That's of course an impossible question for the driver to answer. So, if you are not ok with that, you need to implement something that uses the min/max fields from the various fields inside struct display_timing instead of only looking at the typ field. E.g. the panel_lvds driver calls videomode_from_timings() and the result is a single possible mode with only the typical timings, with no negotiation of the best option within the given ranges with the other drivers involved with the pipe. I think the panel-simple driver also makes this one-sided decision of only making use of the typ field for each given timing range. Having dabbled a bit in what the sound stack does to negotiate the sample rate, sample format and channel count etc, I can only predict that retrofitting something like that for video modes will be ... interesting. Which is probably why it's not done at all, at least not in the general case. And yes, I agree, the current mechanics are less than ideal. But I have no time to do anything about it. > Although I am not familiar with how other DRM > drivers are handling this kind of scenarios. Maybe you and/or other DRM > guys knows more about it. I don't know (and I mean it literally), but maybe these chips are special as they typically end up with very small dividers and thus large frequency steps? BTW, I do not consider myself a DRM guy, I have only tried to fix that which did not work out for our needs... > Just as a notice, it may worth adding a print message saying what was > frequency was requested and what frequency has been setup by driver. I have no problem with that. Cheers, Peter > >> /* >> * At least 10 times better when using a higher >> * frequency than requested, instead of a lower. >> -- >> 2.20.1 >> ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 4/5] Revert "drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested" @ 2019-12-11 13:28 ` Peter Rosin 0 siblings, 0 replies; 54+ messages in thread From: Peter Rosin @ 2019-12-11 13:28 UTC (permalink / raw) To: Claudiu.Beznea, sam, bbrezillon, airlied, daniel, Nicolas.Ferre, alexandre.belloni, Ludovic.Desroches, lee.jones Cc: linux-arm-kernel, dri-devel, linux-kernel On 2019-12-11 12:45, Claudiu.Beznea@microchip.com wrote: > > > On 10.12.2019 19:22, Peter Rosin wrote: >> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >> >> On 2019-12-10 15:59, Claudiu.Beznea@microchip.com wrote: >>> >>> >>> On 10.12.2019 16:11, Peter Rosin wrote: >>>> On 2019-12-10 14:24, Claudiu Beznea wrote: >>>>> This reverts commit f6f7ad3234613f6f7f27c25036aaf078de07e9b0. >>>>> ("drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested") >>>>> because allowing selecting a higher pixel clock may overclock >>>>> LCD devices, not all of them being capable of this. >>>> >>>> Without this patch, there are panels that are *severly* underclocked (on the >>>> magnitude of 40MHz instead of 65MHz or something like that, I don't remember >>>> the exact figures). >>> >>> With patch that switches by default to 2xsystem clock for pixel clock, if >>> using 133MHz system clock (as you specified in the patch I proposed for >>> revert here) that would go, without this patch at 53MHz if 65MHz is >>> requested. Correct me if I'm wrong. >> >> It might have been 53MHz, whatever it was it was too low for things to work. >> >>>> And they are of course not capable of that. All panels >>>> have *some* slack as to what frequencies are supported, and the patch was >>>> written under the assumption that the preferred frequency of the panel was >>>> requested, which should leave at least a *little* headroom. >>> >>> I see, but from my point of view, the upper layers should decide what >>> frequency settings should be done on the LCD controller and not let this at >>> the driver's latitude. >> >> Right, but the upper layers do not support negotiating a frequency from >> ranges. At least the didn't when the patch was written, and implementing >> *that* seemed like a huge undertaking. >> >>>> >>>> So, I'm curious as to what panel regressed. Or rather, what pixel-clock it needs >>>> and what it gets with/without the patch? >>> >>> I have 2 use cases: >>> 1/ system clock = 200MHz and requested pixel clock (mode_rate) ~71MHz. With >>> the reverted patch the resulted computed pixel clock would be 80MHz. >>> Previously it was at 66MHz >> >> I don't see how that's possible. >> >> [doing some calculation by hand] >> >> Arrgh. *blush* >> >> The code does not do what I intended for it to do. >> Can you please try this instead of reverting? >> >> Cheers, >> Peter >> >> From b3e86d55b8d107a5c07e98f879c67f67120c87a6 Mon Sep 17 00:00:00 2001 >> From: Peter Rosin <peda@axentia.se> >> Date: Tue, 10 Dec 2019 18:11:28 +0100 >> Subject: [PATCH] drm/atmel-hlcdc: prefer a lower pixel-clock than requested >> >> The intention was to only select a higher pixel-clock rate than the >> requested, if a slight overclocking would result in a rate significantly >> closer to the requested rate than if the conservative lower pixel-clock >> rate is selected. The fixed patch has the logic the other way around and >> actually prefers the higher frequency. Fix that. >> >> Fixes: f6f7ad323461 ("drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested") >> Reported-by: Claudiu Beznea <claudiu.beznea@microchip.com> >> Signed-off-by: Peter Rosin <peda@axentia.se> >> --- >> drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c >> index 9e34bce089d0..03691845d37a 100644 >> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c >> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c >> @@ -120,8 +120,8 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c) >> int div_low = prate / mode_rate; >> >> if (div_low >= 2 && >> - ((prate / div_low - mode_rate) < >> - 10 * (mode_rate - prate / div))) >> + (10 * (prate / div_low - mode_rate) < >> + (mode_rate - prate / div))) > > I tested it on my setup (I have only one of those specified above) and it > is OK. Doing some math for the other setup it should also be OK. Glad to hear it, and thanks for testing/verifying! > As a whole, I'm OK with this at the moment (let's hope it will work for all > use-cases) but still I am not OK with selecting here, in the driver, > something that might work. The driver has to select *something*. If it can deliver the exact requested frequency, fine. Otherwise? What should it do? Bail out? Why is 53MHz better and more likely to produce a picture than 66MHz, when 65MHz is requested? That's of course an impossible question for the driver to answer. So, if you are not ok with that, you need to implement something that uses the min/max fields from the various fields inside struct display_timing instead of only looking at the typ field. E.g. the panel_lvds driver calls videomode_from_timings() and the result is a single possible mode with only the typical timings, with no negotiation of the best option within the given ranges with the other drivers involved with the pipe. I think the panel-simple driver also makes this one-sided decision of only making use of the typ field for each given timing range. Having dabbled a bit in what the sound stack does to negotiate the sample rate, sample format and channel count etc, I can only predict that retrofitting something like that for video modes will be ... interesting. Which is probably why it's not done at all, at least not in the general case. And yes, I agree, the current mechanics are less than ideal. But I have no time to do anything about it. > Although I am not familiar with how other DRM > drivers are handling this kind of scenarios. Maybe you and/or other DRM > guys knows more about it. I don't know (and I mean it literally), but maybe these chips are special as they typically end up with very small dividers and thus large frequency steps? BTW, I do not consider myself a DRM guy, I have only tried to fix that which did not work out for our needs... > Just as a notice, it may worth adding a print message saying what was > frequency was requested and what frequency has been setup by driver. I have no problem with that. Cheers, Peter > >> /* >> * At least 10 times better when using a higher >> * frequency than requested, instead of a lower. >> -- >> 2.20.1 >> _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 4/5] Revert "drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested" @ 2019-12-11 13:28 ` Peter Rosin 0 siblings, 0 replies; 54+ messages in thread From: Peter Rosin @ 2019-12-11 13:28 UTC (permalink / raw) To: Claudiu.Beznea, sam, bbrezillon, airlied, daniel, Nicolas.Ferre, alexandre.belloni, Ludovic.Desroches, lee.jones Cc: linux-arm-kernel, dri-devel, linux-kernel On 2019-12-11 12:45, Claudiu.Beznea@microchip.com wrote: > > > On 10.12.2019 19:22, Peter Rosin wrote: >> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >> >> On 2019-12-10 15:59, Claudiu.Beznea@microchip.com wrote: >>> >>> >>> On 10.12.2019 16:11, Peter Rosin wrote: >>>> On 2019-12-10 14:24, Claudiu Beznea wrote: >>>>> This reverts commit f6f7ad3234613f6f7f27c25036aaf078de07e9b0. >>>>> ("drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested") >>>>> because allowing selecting a higher pixel clock may overclock >>>>> LCD devices, not all of them being capable of this. >>>> >>>> Without this patch, there are panels that are *severly* underclocked (on the >>>> magnitude of 40MHz instead of 65MHz or something like that, I don't remember >>>> the exact figures). >>> >>> With patch that switches by default to 2xsystem clock for pixel clock, if >>> using 133MHz system clock (as you specified in the patch I proposed for >>> revert here) that would go, without this patch at 53MHz if 65MHz is >>> requested. Correct me if I'm wrong. >> >> It might have been 53MHz, whatever it was it was too low for things to work. >> >>>> And they are of course not capable of that. All panels >>>> have *some* slack as to what frequencies are supported, and the patch was >>>> written under the assumption that the preferred frequency of the panel was >>>> requested, which should leave at least a *little* headroom. >>> >>> I see, but from my point of view, the upper layers should decide what >>> frequency settings should be done on the LCD controller and not let this at >>> the driver's latitude. >> >> Right, but the upper layers do not support negotiating a frequency from >> ranges. At least the didn't when the patch was written, and implementing >> *that* seemed like a huge undertaking. >> >>>> >>>> So, I'm curious as to what panel regressed. Or rather, what pixel-clock it needs >>>> and what it gets with/without the patch? >>> >>> I have 2 use cases: >>> 1/ system clock = 200MHz and requested pixel clock (mode_rate) ~71MHz. With >>> the reverted patch the resulted computed pixel clock would be 80MHz. >>> Previously it was at 66MHz >> >> I don't see how that's possible. >> >> [doing some calculation by hand] >> >> Arrgh. *blush* >> >> The code does not do what I intended for it to do. >> Can you please try this instead of reverting? >> >> Cheers, >> Peter >> >> From b3e86d55b8d107a5c07e98f879c67f67120c87a6 Mon Sep 17 00:00:00 2001 >> From: Peter Rosin <peda@axentia.se> >> Date: Tue, 10 Dec 2019 18:11:28 +0100 >> Subject: [PATCH] drm/atmel-hlcdc: prefer a lower pixel-clock than requested >> >> The intention was to only select a higher pixel-clock rate than the >> requested, if a slight overclocking would result in a rate significantly >> closer to the requested rate than if the conservative lower pixel-clock >> rate is selected. The fixed patch has the logic the other way around and >> actually prefers the higher frequency. Fix that. >> >> Fixes: f6f7ad323461 ("drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested") >> Reported-by: Claudiu Beznea <claudiu.beznea@microchip.com> >> Signed-off-by: Peter Rosin <peda@axentia.se> >> --- >> drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c >> index 9e34bce089d0..03691845d37a 100644 >> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c >> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c >> @@ -120,8 +120,8 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c) >> int div_low = prate / mode_rate; >> >> if (div_low >= 2 && >> - ((prate / div_low - mode_rate) < >> - 10 * (mode_rate - prate / div))) >> + (10 * (prate / div_low - mode_rate) < >> + (mode_rate - prate / div))) > > I tested it on my setup (I have only one of those specified above) and it > is OK. Doing some math for the other setup it should also be OK. Glad to hear it, and thanks for testing/verifying! > As a whole, I'm OK with this at the moment (let's hope it will work for all > use-cases) but still I am not OK with selecting here, in the driver, > something that might work. The driver has to select *something*. If it can deliver the exact requested frequency, fine. Otherwise? What should it do? Bail out? Why is 53MHz better and more likely to produce a picture than 66MHz, when 65MHz is requested? That's of course an impossible question for the driver to answer. So, if you are not ok with that, you need to implement something that uses the min/max fields from the various fields inside struct display_timing instead of only looking at the typ field. E.g. the panel_lvds driver calls videomode_from_timings() and the result is a single possible mode with only the typical timings, with no negotiation of the best option within the given ranges with the other drivers involved with the pipe. I think the panel-simple driver also makes this one-sided decision of only making use of the typ field for each given timing range. Having dabbled a bit in what the sound stack does to negotiate the sample rate, sample format and channel count etc, I can only predict that retrofitting something like that for video modes will be ... interesting. Which is probably why it's not done at all, at least not in the general case. And yes, I agree, the current mechanics are less than ideal. But I have no time to do anything about it. > Although I am not familiar with how other DRM > drivers are handling this kind of scenarios. Maybe you and/or other DRM > guys knows more about it. I don't know (and I mean it literally), but maybe these chips are special as they typically end up with very small dividers and thus large frequency steps? BTW, I do not consider myself a DRM guy, I have only tried to fix that which did not work out for our needs... > Just as a notice, it may worth adding a print message saying what was > frequency was requested and what frequency has been setup by driver. I have no problem with that. Cheers, Peter > >> /* >> * At least 10 times better when using a higher >> * frequency than requested, instead of a lower. >> -- >> 2.20.1 >> _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 4/5] Revert "drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested" 2019-12-11 13:28 ` Peter Rosin (?) @ 2019-12-13 9:28 ` Claudiu.Beznea -1 siblings, 0 replies; 54+ messages in thread From: Claudiu.Beznea @ 2019-12-13 9:28 UTC (permalink / raw) To: peda, sam, bbrezillon, airlied, daniel, Nicolas.Ferre, alexandre.belloni, Ludovic.Desroches, lee.jones Cc: dri-devel, linux-arm-kernel, linux-kernel On 11.12.2019 15:28, Peter Rosin wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On 2019-12-11 12:45, Claudiu.Beznea@microchip.com wrote: >> >> >> On 10.12.2019 19:22, Peter Rosin wrote: >>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >>> >>> On 2019-12-10 15:59, Claudiu.Beznea@microchip.com wrote: >>>> >>>> >>>> On 10.12.2019 16:11, Peter Rosin wrote: >>>>> On 2019-12-10 14:24, Claudiu Beznea wrote: >>>>>> This reverts commit f6f7ad3234613f6f7f27c25036aaf078de07e9b0. >>>>>> ("drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested") >>>>>> because allowing selecting a higher pixel clock may overclock >>>>>> LCD devices, not all of them being capable of this. >>>>> >>>>> Without this patch, there are panels that are *severly* underclocked (on the >>>>> magnitude of 40MHz instead of 65MHz or something like that, I don't remember >>>>> the exact figures). >>>> >>>> With patch that switches by default to 2xsystem clock for pixel clock, if >>>> using 133MHz system clock (as you specified in the patch I proposed for >>>> revert here) that would go, without this patch at 53MHz if 65MHz is >>>> requested. Correct me if I'm wrong. >>> >>> It might have been 53MHz, whatever it was it was too low for things to work. >>> >>>>> And they are of course not capable of that. All panels >>>>> have *some* slack as to what frequencies are supported, and the patch was >>>>> written under the assumption that the preferred frequency of the panel was >>>>> requested, which should leave at least a *little* headroom. >>>> >>>> I see, but from my point of view, the upper layers should decide what >>>> frequency settings should be done on the LCD controller and not let this at >>>> the driver's latitude. >>> >>> Right, but the upper layers do not support negotiating a frequency from >>> ranges. At least the didn't when the patch was written, and implementing >>> *that* seemed like a huge undertaking. >>> >>>>> >>>>> So, I'm curious as to what panel regressed. Or rather, what pixel-clock it needs >>>>> and what it gets with/without the patch? >>>> >>>> I have 2 use cases: >>>> 1/ system clock = 200MHz and requested pixel clock (mode_rate) ~71MHz. With >>>> the reverted patch the resulted computed pixel clock would be 80MHz. >>>> Previously it was at 66MHz >>> >>> I don't see how that's possible. >>> >>> [doing some calculation by hand] >>> >>> Arrgh. *blush* >>> >>> The code does not do what I intended for it to do. >>> Can you please try this instead of reverting? >>> >>> Cheers, >>> Peter >>> >>> From b3e86d55b8d107a5c07e98f879c67f67120c87a6 Mon Sep 17 00:00:00 2001 >>> From: Peter Rosin <peda@axentia.se> >>> Date: Tue, 10 Dec 2019 18:11:28 +0100 >>> Subject: [PATCH] drm/atmel-hlcdc: prefer a lower pixel-clock than requested >>> >>> The intention was to only select a higher pixel-clock rate than the >>> requested, if a slight overclocking would result in a rate significantly >>> closer to the requested rate than if the conservative lower pixel-clock >>> rate is selected. The fixed patch has the logic the other way around and >>> actually prefers the higher frequency. Fix that. >>> >>> Fixes: f6f7ad323461 ("drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested") >>> Reported-by: Claudiu Beznea <claudiu.beznea@microchip.com> >>> Signed-off-by: Peter Rosin <peda@axentia.se> >>> --- >>> drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c >>> index 9e34bce089d0..03691845d37a 100644 >>> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c >>> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c >>> @@ -120,8 +120,8 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c) >>> int div_low = prate / mode_rate; >>> >>> if (div_low >= 2 && >>> - ((prate / div_low - mode_rate) < >>> - 10 * (mode_rate - prate / div))) >>> + (10 * (prate / div_low - mode_rate) < >>> + (mode_rate - prate / div))) >> >> I tested it on my setup (I have only one of those specified above) and it >> is OK. Doing some math for the other setup it should also be OK. > > Glad to hear it, and thanks for testing/verifying! > >> As a whole, I'm OK with this at the moment (let's hope it will work for all >> use-cases) but still I am not OK with selecting here, in the driver, >> something that might work. > > The driver has to select *something*. If it can deliver the exact requested > frequency, fine. Otherwise? What should it do? Bail out? Why is 53MHz better > and more likely to produce a picture than 66MHz, when 65MHz is requested? > That's of course an impossible question for the driver to answer. > > So, if you are not ok with that, you need to implement something that uses > the min/max fields from the various fields inside struct display_timing > instead of only looking at the typ field. E.g. the panel_lvds driver calls > videomode_from_timings() and the result is a single possible mode with only > the typical timings, with no negotiation of the best option within the > given ranges with the other drivers involved with the pipe. I think the > panel-simple driver also makes this one-sided decision of only making use > of the typ field for each given timing range. Having dabbled a bit in what > the sound stack does to negotiate the sample rate, sample format and > channel count etc, I can only predict that retrofitting something like that > for video modes will be ... interesting. Which is probably why it's not > done at all, at least not in the general case. > > And yes, I agree, the current mechanics are less than ideal. But I have no > time to do anything about it. > >> Although I am not familiar with how other DRM >> drivers are handling this kind of scenarios. Maybe you and/or other DRM >> guys knows more about it. > > I don't know (and I mean it literally), but maybe these chips are special > as they typically end up with very small dividers and thus large frequency > steps? BTW, I do not consider myself a DRM guy, I have only tried to > fix that which did not work out for our needs... > >> Just as a notice, it may worth adding a print message saying what was >> frequency was requested and what frequency has been setup by driver. > > I have no problem with that. Hi Peter, I intend to prepare my v2 of this series. How would you like to proceed with the patch you provided? Are you OK if I add it to my v2 of this series or would you prefer to send it on your own? Thank you, Claudiu Beznea > > Cheers, > Peter > >> >>> /* >>> * At least 10 times better when using a higher >>> * frequency than requested, instead of a lower. >>> -- >>> 2.20.1 >>> > ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 4/5] Revert "drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested" @ 2019-12-13 9:28 ` Claudiu.Beznea 0 siblings, 0 replies; 54+ messages in thread From: Claudiu.Beznea @ 2019-12-13 9:28 UTC (permalink / raw) To: peda, sam, bbrezillon, airlied, daniel, Nicolas.Ferre, alexandre.belloni, Ludovic.Desroches, lee.jones Cc: linux-arm-kernel, dri-devel, linux-kernel On 11.12.2019 15:28, Peter Rosin wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On 2019-12-11 12:45, Claudiu.Beznea@microchip.com wrote: >> >> >> On 10.12.2019 19:22, Peter Rosin wrote: >>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >>> >>> On 2019-12-10 15:59, Claudiu.Beznea@microchip.com wrote: >>>> >>>> >>>> On 10.12.2019 16:11, Peter Rosin wrote: >>>>> On 2019-12-10 14:24, Claudiu Beznea wrote: >>>>>> This reverts commit f6f7ad3234613f6f7f27c25036aaf078de07e9b0. >>>>>> ("drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested") >>>>>> because allowing selecting a higher pixel clock may overclock >>>>>> LCD devices, not all of them being capable of this. >>>>> >>>>> Without this patch, there are panels that are *severly* underclocked (on the >>>>> magnitude of 40MHz instead of 65MHz or something like that, I don't remember >>>>> the exact figures). >>>> >>>> With patch that switches by default to 2xsystem clock for pixel clock, if >>>> using 133MHz system clock (as you specified in the patch I proposed for >>>> revert here) that would go, without this patch at 53MHz if 65MHz is >>>> requested. Correct me if I'm wrong. >>> >>> It might have been 53MHz, whatever it was it was too low for things to work. >>> >>>>> And they are of course not capable of that. All panels >>>>> have *some* slack as to what frequencies are supported, and the patch was >>>>> written under the assumption that the preferred frequency of the panel was >>>>> requested, which should leave at least a *little* headroom. >>>> >>>> I see, but from my point of view, the upper layers should decide what >>>> frequency settings should be done on the LCD controller and not let this at >>>> the driver's latitude. >>> >>> Right, but the upper layers do not support negotiating a frequency from >>> ranges. At least the didn't when the patch was written, and implementing >>> *that* seemed like a huge undertaking. >>> >>>>> >>>>> So, I'm curious as to what panel regressed. Or rather, what pixel-clock it needs >>>>> and what it gets with/without the patch? >>>> >>>> I have 2 use cases: >>>> 1/ system clock = 200MHz and requested pixel clock (mode_rate) ~71MHz. With >>>> the reverted patch the resulted computed pixel clock would be 80MHz. >>>> Previously it was at 66MHz >>> >>> I don't see how that's possible. >>> >>> [doing some calculation by hand] >>> >>> Arrgh. *blush* >>> >>> The code does not do what I intended for it to do. >>> Can you please try this instead of reverting? >>> >>> Cheers, >>> Peter >>> >>> From b3e86d55b8d107a5c07e98f879c67f67120c87a6 Mon Sep 17 00:00:00 2001 >>> From: Peter Rosin <peda@axentia.se> >>> Date: Tue, 10 Dec 2019 18:11:28 +0100 >>> Subject: [PATCH] drm/atmel-hlcdc: prefer a lower pixel-clock than requested >>> >>> The intention was to only select a higher pixel-clock rate than the >>> requested, if a slight overclocking would result in a rate significantly >>> closer to the requested rate than if the conservative lower pixel-clock >>> rate is selected. The fixed patch has the logic the other way around and >>> actually prefers the higher frequency. Fix that. >>> >>> Fixes: f6f7ad323461 ("drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested") >>> Reported-by: Claudiu Beznea <claudiu.beznea@microchip.com> >>> Signed-off-by: Peter Rosin <peda@axentia.se> >>> --- >>> drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c >>> index 9e34bce089d0..03691845d37a 100644 >>> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c >>> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c >>> @@ -120,8 +120,8 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c) >>> int div_low = prate / mode_rate; >>> >>> if (div_low >= 2 && >>> - ((prate / div_low - mode_rate) < >>> - 10 * (mode_rate - prate / div))) >>> + (10 * (prate / div_low - mode_rate) < >>> + (mode_rate - prate / div))) >> >> I tested it on my setup (I have only one of those specified above) and it >> is OK. Doing some math for the other setup it should also be OK. > > Glad to hear it, and thanks for testing/verifying! > >> As a whole, I'm OK with this at the moment (let's hope it will work for all >> use-cases) but still I am not OK with selecting here, in the driver, >> something that might work. > > The driver has to select *something*. If it can deliver the exact requested > frequency, fine. Otherwise? What should it do? Bail out? Why is 53MHz better > and more likely to produce a picture than 66MHz, when 65MHz is requested? > That's of course an impossible question for the driver to answer. > > So, if you are not ok with that, you need to implement something that uses > the min/max fields from the various fields inside struct display_timing > instead of only looking at the typ field. E.g. the panel_lvds driver calls > videomode_from_timings() and the result is a single possible mode with only > the typical timings, with no negotiation of the best option within the > given ranges with the other drivers involved with the pipe. I think the > panel-simple driver also makes this one-sided decision of only making use > of the typ field for each given timing range. Having dabbled a bit in what > the sound stack does to negotiate the sample rate, sample format and > channel count etc, I can only predict that retrofitting something like that > for video modes will be ... interesting. Which is probably why it's not > done at all, at least not in the general case. > > And yes, I agree, the current mechanics are less than ideal. But I have no > time to do anything about it. > >> Although I am not familiar with how other DRM >> drivers are handling this kind of scenarios. Maybe you and/or other DRM >> guys knows more about it. > > I don't know (and I mean it literally), but maybe these chips are special > as they typically end up with very small dividers and thus large frequency > steps? BTW, I do not consider myself a DRM guy, I have only tried to > fix that which did not work out for our needs... > >> Just as a notice, it may worth adding a print message saying what was >> frequency was requested and what frequency has been setup by driver. > > I have no problem with that. Hi Peter, I intend to prepare my v2 of this series. How would you like to proceed with the patch you provided? Are you OK if I add it to my v2 of this series or would you prefer to send it on your own? Thank you, Claudiu Beznea > > Cheers, > Peter > >> >>> /* >>> * At least 10 times better when using a higher >>> * frequency than requested, instead of a lower. >>> -- >>> 2.20.1 >>> > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 4/5] Revert "drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested" @ 2019-12-13 9:28 ` Claudiu.Beznea 0 siblings, 0 replies; 54+ messages in thread From: Claudiu.Beznea @ 2019-12-13 9:28 UTC (permalink / raw) To: peda, sam, bbrezillon, airlied, daniel, Nicolas.Ferre, alexandre.belloni, Ludovic.Desroches, lee.jones Cc: linux-arm-kernel, dri-devel, linux-kernel On 11.12.2019 15:28, Peter Rosin wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On 2019-12-11 12:45, Claudiu.Beznea@microchip.com wrote: >> >> >> On 10.12.2019 19:22, Peter Rosin wrote: >>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >>> >>> On 2019-12-10 15:59, Claudiu.Beznea@microchip.com wrote: >>>> >>>> >>>> On 10.12.2019 16:11, Peter Rosin wrote: >>>>> On 2019-12-10 14:24, Claudiu Beznea wrote: >>>>>> This reverts commit f6f7ad3234613f6f7f27c25036aaf078de07e9b0. >>>>>> ("drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested") >>>>>> because allowing selecting a higher pixel clock may overclock >>>>>> LCD devices, not all of them being capable of this. >>>>> >>>>> Without this patch, there are panels that are *severly* underclocked (on the >>>>> magnitude of 40MHz instead of 65MHz or something like that, I don't remember >>>>> the exact figures). >>>> >>>> With patch that switches by default to 2xsystem clock for pixel clock, if >>>> using 133MHz system clock (as you specified in the patch I proposed for >>>> revert here) that would go, without this patch at 53MHz if 65MHz is >>>> requested. Correct me if I'm wrong. >>> >>> It might have been 53MHz, whatever it was it was too low for things to work. >>> >>>>> And they are of course not capable of that. All panels >>>>> have *some* slack as to what frequencies are supported, and the patch was >>>>> written under the assumption that the preferred frequency of the panel was >>>>> requested, which should leave at least a *little* headroom. >>>> >>>> I see, but from my point of view, the upper layers should decide what >>>> frequency settings should be done on the LCD controller and not let this at >>>> the driver's latitude. >>> >>> Right, but the upper layers do not support negotiating a frequency from >>> ranges. At least the didn't when the patch was written, and implementing >>> *that* seemed like a huge undertaking. >>> >>>>> >>>>> So, I'm curious as to what panel regressed. Or rather, what pixel-clock it needs >>>>> and what it gets with/without the patch? >>>> >>>> I have 2 use cases: >>>> 1/ system clock = 200MHz and requested pixel clock (mode_rate) ~71MHz. With >>>> the reverted patch the resulted computed pixel clock would be 80MHz. >>>> Previously it was at 66MHz >>> >>> I don't see how that's possible. >>> >>> [doing some calculation by hand] >>> >>> Arrgh. *blush* >>> >>> The code does not do what I intended for it to do. >>> Can you please try this instead of reverting? >>> >>> Cheers, >>> Peter >>> >>> From b3e86d55b8d107a5c07e98f879c67f67120c87a6 Mon Sep 17 00:00:00 2001 >>> From: Peter Rosin <peda@axentia.se> >>> Date: Tue, 10 Dec 2019 18:11:28 +0100 >>> Subject: [PATCH] drm/atmel-hlcdc: prefer a lower pixel-clock than requested >>> >>> The intention was to only select a higher pixel-clock rate than the >>> requested, if a slight overclocking would result in a rate significantly >>> closer to the requested rate than if the conservative lower pixel-clock >>> rate is selected. The fixed patch has the logic the other way around and >>> actually prefers the higher frequency. Fix that. >>> >>> Fixes: f6f7ad323461 ("drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested") >>> Reported-by: Claudiu Beznea <claudiu.beznea@microchip.com> >>> Signed-off-by: Peter Rosin <peda@axentia.se> >>> --- >>> drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c >>> index 9e34bce089d0..03691845d37a 100644 >>> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c >>> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c >>> @@ -120,8 +120,8 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c) >>> int div_low = prate / mode_rate; >>> >>> if (div_low >= 2 && >>> - ((prate / div_low - mode_rate) < >>> - 10 * (mode_rate - prate / div))) >>> + (10 * (prate / div_low - mode_rate) < >>> + (mode_rate - prate / div))) >> >> I tested it on my setup (I have only one of those specified above) and it >> is OK. Doing some math for the other setup it should also be OK. > > Glad to hear it, and thanks for testing/verifying! > >> As a whole, I'm OK with this at the moment (let's hope it will work for all >> use-cases) but still I am not OK with selecting here, in the driver, >> something that might work. > > The driver has to select *something*. If it can deliver the exact requested > frequency, fine. Otherwise? What should it do? Bail out? Why is 53MHz better > and more likely to produce a picture than 66MHz, when 65MHz is requested? > That's of course an impossible question for the driver to answer. > > So, if you are not ok with that, you need to implement something that uses > the min/max fields from the various fields inside struct display_timing > instead of only looking at the typ field. E.g. the panel_lvds driver calls > videomode_from_timings() and the result is a single possible mode with only > the typical timings, with no negotiation of the best option within the > given ranges with the other drivers involved with the pipe. I think the > panel-simple driver also makes this one-sided decision of only making use > of the typ field for each given timing range. Having dabbled a bit in what > the sound stack does to negotiate the sample rate, sample format and > channel count etc, I can only predict that retrofitting something like that > for video modes will be ... interesting. Which is probably why it's not > done at all, at least not in the general case. > > And yes, I agree, the current mechanics are less than ideal. But I have no > time to do anything about it. > >> Although I am not familiar with how other DRM >> drivers are handling this kind of scenarios. Maybe you and/or other DRM >> guys knows more about it. > > I don't know (and I mean it literally), but maybe these chips are special > as they typically end up with very small dividers and thus large frequency > steps? BTW, I do not consider myself a DRM guy, I have only tried to > fix that which did not work out for our needs... > >> Just as a notice, it may worth adding a print message saying what was >> frequency was requested and what frequency has been setup by driver. > > I have no problem with that. Hi Peter, I intend to prepare my v2 of this series. How would you like to proceed with the patch you provided? Are you OK if I add it to my v2 of this series or would you prefer to send it on your own? Thank you, Claudiu Beznea > > Cheers, > Peter > >> >>> /* >>> * At least 10 times better when using a higher >>> * frequency than requested, instead of a lower. >>> -- >>> 2.20.1 >>> > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 4/5] Revert "drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested" 2019-12-13 9:28 ` Claudiu.Beznea (?) @ 2019-12-13 9:30 ` Peter Rosin -1 siblings, 0 replies; 54+ messages in thread From: Peter Rosin @ 2019-12-13 9:30 UTC (permalink / raw) To: Claudiu.Beznea, sam, bbrezillon, airlied, daniel, Nicolas.Ferre, alexandre.belloni, Ludovic.Desroches, lee.jones Cc: dri-devel, linux-arm-kernel, linux-kernel On 2019-12-13 10:28, Claudiu.Beznea@microchip.com wrote: > > > On 11.12.2019 15:28, Peter Rosin wrote: >> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >> >> On 2019-12-11 12:45, Claudiu.Beznea@microchip.com wrote: >>> >>> >>> On 10.12.2019 19:22, Peter Rosin wrote: >>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >>>> >>>> On 2019-12-10 15:59, Claudiu.Beznea@microchip.com wrote: >>>>> >>>>> >>>>> On 10.12.2019 16:11, Peter Rosin wrote: >>>>>> On 2019-12-10 14:24, Claudiu Beznea wrote: >>>>>>> This reverts commit f6f7ad3234613f6f7f27c25036aaf078de07e9b0. >>>>>>> ("drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested") >>>>>>> because allowing selecting a higher pixel clock may overclock >>>>>>> LCD devices, not all of them being capable of this. >>>>>> >>>>>> Without this patch, there are panels that are *severly* underclocked (on the >>>>>> magnitude of 40MHz instead of 65MHz or something like that, I don't remember >>>>>> the exact figures). >>>>> >>>>> With patch that switches by default to 2xsystem clock for pixel clock, if >>>>> using 133MHz system clock (as you specified in the patch I proposed for >>>>> revert here) that would go, without this patch at 53MHz if 65MHz is >>>>> requested. Correct me if I'm wrong. >>>> >>>> It might have been 53MHz, whatever it was it was too low for things to work. >>>> >>>>>> And they are of course not capable of that. All panels >>>>>> have *some* slack as to what frequencies are supported, and the patch was >>>>>> written under the assumption that the preferred frequency of the panel was >>>>>> requested, which should leave at least a *little* headroom. >>>>> >>>>> I see, but from my point of view, the upper layers should decide what >>>>> frequency settings should be done on the LCD controller and not let this at >>>>> the driver's latitude. >>>> >>>> Right, but the upper layers do not support negotiating a frequency from >>>> ranges. At least the didn't when the patch was written, and implementing >>>> *that* seemed like a huge undertaking. >>>> >>>>>> >>>>>> So, I'm curious as to what panel regressed. Or rather, what pixel-clock it needs >>>>>> and what it gets with/without the patch? >>>>> >>>>> I have 2 use cases: >>>>> 1/ system clock = 200MHz and requested pixel clock (mode_rate) ~71MHz. With >>>>> the reverted patch the resulted computed pixel clock would be 80MHz. >>>>> Previously it was at 66MHz >>>> >>>> I don't see how that's possible. >>>> >>>> [doing some calculation by hand] >>>> >>>> Arrgh. *blush* >>>> >>>> The code does not do what I intended for it to do. >>>> Can you please try this instead of reverting? >>>> >>>> Cheers, >>>> Peter >>>> >>>> From b3e86d55b8d107a5c07e98f879c67f67120c87a6 Mon Sep 17 00:00:00 2001 >>>> From: Peter Rosin <peda@axentia.se> >>>> Date: Tue, 10 Dec 2019 18:11:28 +0100 >>>> Subject: [PATCH] drm/atmel-hlcdc: prefer a lower pixel-clock than requested >>>> >>>> The intention was to only select a higher pixel-clock rate than the >>>> requested, if a slight overclocking would result in a rate significantly >>>> closer to the requested rate than if the conservative lower pixel-clock >>>> rate is selected. The fixed patch has the logic the other way around and >>>> actually prefers the higher frequency. Fix that. >>>> >>>> Fixes: f6f7ad323461 ("drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested") >>>> Reported-by: Claudiu Beznea <claudiu.beznea@microchip.com> >>>> Signed-off-by: Peter Rosin <peda@axentia.se> >>>> --- >>>> drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 4 ++-- >>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c >>>> index 9e34bce089d0..03691845d37a 100644 >>>> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c >>>> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c >>>> @@ -120,8 +120,8 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c) >>>> int div_low = prate / mode_rate; >>>> >>>> if (div_low >= 2 && >>>> - ((prate / div_low - mode_rate) < >>>> - 10 * (mode_rate - prate / div))) >>>> + (10 * (prate / div_low - mode_rate) < >>>> + (mode_rate - prate / div))) >>> >>> I tested it on my setup (I have only one of those specified above) and it >>> is OK. Doing some math for the other setup it should also be OK. >> >> Glad to hear it, and thanks for testing/verifying! >> >>> As a whole, I'm OK with this at the moment (let's hope it will work for all >>> use-cases) but still I am not OK with selecting here, in the driver, >>> something that might work. >> >> The driver has to select *something*. If it can deliver the exact requested >> frequency, fine. Otherwise? What should it do? Bail out? Why is 53MHz better >> and more likely to produce a picture than 66MHz, when 65MHz is requested? >> That's of course an impossible question for the driver to answer. >> >> So, if you are not ok with that, you need to implement something that uses >> the min/max fields from the various fields inside struct display_timing >> instead of only looking at the typ field. E.g. the panel_lvds driver calls >> videomode_from_timings() and the result is a single possible mode with only >> the typical timings, with no negotiation of the best option within the >> given ranges with the other drivers involved with the pipe. I think the >> panel-simple driver also makes this one-sided decision of only making use >> of the typ field for each given timing range. Having dabbled a bit in what >> the sound stack does to negotiate the sample rate, sample format and >> channel count etc, I can only predict that retrofitting something like that >> for video modes will be ... interesting. Which is probably why it's not >> done at all, at least not in the general case. >> >> And yes, I agree, the current mechanics are less than ideal. But I have no >> time to do anything about it. >> >>> Although I am not familiar with how other DRM >>> drivers are handling this kind of scenarios. Maybe you and/or other DRM >>> guys knows more about it. >> >> I don't know (and I mean it literally), but maybe these chips are special >> as they typically end up with very small dividers and thus large frequency >> steps? BTW, I do not consider myself a DRM guy, I have only tried to >> fix that which did not work out for our needs... >> >>> Just as a notice, it may worth adding a print message saying what was >>> frequency was requested and what frequency has been setup by driver. >> >> I have no problem with that. > > Hi Peter, > > I intend to prepare my v2 of this series. How would you like to proceed > with the patch you provided? Are you OK if I add it to my v2 of this series > or would you prefer to send it on your own? It would be awesome if you shepherd it for me, thanks! Cheers, Peter ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 4/5] Revert "drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested" @ 2019-12-13 9:30 ` Peter Rosin 0 siblings, 0 replies; 54+ messages in thread From: Peter Rosin @ 2019-12-13 9:30 UTC (permalink / raw) To: Claudiu.Beznea, sam, bbrezillon, airlied, daniel, Nicolas.Ferre, alexandre.belloni, Ludovic.Desroches, lee.jones Cc: linux-arm-kernel, dri-devel, linux-kernel On 2019-12-13 10:28, Claudiu.Beznea@microchip.com wrote: > > > On 11.12.2019 15:28, Peter Rosin wrote: >> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >> >> On 2019-12-11 12:45, Claudiu.Beznea@microchip.com wrote: >>> >>> >>> On 10.12.2019 19:22, Peter Rosin wrote: >>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >>>> >>>> On 2019-12-10 15:59, Claudiu.Beznea@microchip.com wrote: >>>>> >>>>> >>>>> On 10.12.2019 16:11, Peter Rosin wrote: >>>>>> On 2019-12-10 14:24, Claudiu Beznea wrote: >>>>>>> This reverts commit f6f7ad3234613f6f7f27c25036aaf078de07e9b0. >>>>>>> ("drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested") >>>>>>> because allowing selecting a higher pixel clock may overclock >>>>>>> LCD devices, not all of them being capable of this. >>>>>> >>>>>> Without this patch, there are panels that are *severly* underclocked (on the >>>>>> magnitude of 40MHz instead of 65MHz or something like that, I don't remember >>>>>> the exact figures). >>>>> >>>>> With patch that switches by default to 2xsystem clock for pixel clock, if >>>>> using 133MHz system clock (as you specified in the patch I proposed for >>>>> revert here) that would go, without this patch at 53MHz if 65MHz is >>>>> requested. Correct me if I'm wrong. >>>> >>>> It might have been 53MHz, whatever it was it was too low for things to work. >>>> >>>>>> And they are of course not capable of that. All panels >>>>>> have *some* slack as to what frequencies are supported, and the patch was >>>>>> written under the assumption that the preferred frequency of the panel was >>>>>> requested, which should leave at least a *little* headroom. >>>>> >>>>> I see, but from my point of view, the upper layers should decide what >>>>> frequency settings should be done on the LCD controller and not let this at >>>>> the driver's latitude. >>>> >>>> Right, but the upper layers do not support negotiating a frequency from >>>> ranges. At least the didn't when the patch was written, and implementing >>>> *that* seemed like a huge undertaking. >>>> >>>>>> >>>>>> So, I'm curious as to what panel regressed. Or rather, what pixel-clock it needs >>>>>> and what it gets with/without the patch? >>>>> >>>>> I have 2 use cases: >>>>> 1/ system clock = 200MHz and requested pixel clock (mode_rate) ~71MHz. With >>>>> the reverted patch the resulted computed pixel clock would be 80MHz. >>>>> Previously it was at 66MHz >>>> >>>> I don't see how that's possible. >>>> >>>> [doing some calculation by hand] >>>> >>>> Arrgh. *blush* >>>> >>>> The code does not do what I intended for it to do. >>>> Can you please try this instead of reverting? >>>> >>>> Cheers, >>>> Peter >>>> >>>> From b3e86d55b8d107a5c07e98f879c67f67120c87a6 Mon Sep 17 00:00:00 2001 >>>> From: Peter Rosin <peda@axentia.se> >>>> Date: Tue, 10 Dec 2019 18:11:28 +0100 >>>> Subject: [PATCH] drm/atmel-hlcdc: prefer a lower pixel-clock than requested >>>> >>>> The intention was to only select a higher pixel-clock rate than the >>>> requested, if a slight overclocking would result in a rate significantly >>>> closer to the requested rate than if the conservative lower pixel-clock >>>> rate is selected. The fixed patch has the logic the other way around and >>>> actually prefers the higher frequency. Fix that. >>>> >>>> Fixes: f6f7ad323461 ("drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested") >>>> Reported-by: Claudiu Beznea <claudiu.beznea@microchip.com> >>>> Signed-off-by: Peter Rosin <peda@axentia.se> >>>> --- >>>> drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 4 ++-- >>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c >>>> index 9e34bce089d0..03691845d37a 100644 >>>> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c >>>> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c >>>> @@ -120,8 +120,8 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c) >>>> int div_low = prate / mode_rate; >>>> >>>> if (div_low >= 2 && >>>> - ((prate / div_low - mode_rate) < >>>> - 10 * (mode_rate - prate / div))) >>>> + (10 * (prate / div_low - mode_rate) < >>>> + (mode_rate - prate / div))) >>> >>> I tested it on my setup (I have only one of those specified above) and it >>> is OK. Doing some math for the other setup it should also be OK. >> >> Glad to hear it, and thanks for testing/verifying! >> >>> As a whole, I'm OK with this at the moment (let's hope it will work for all >>> use-cases) but still I am not OK with selecting here, in the driver, >>> something that might work. >> >> The driver has to select *something*. If it can deliver the exact requested >> frequency, fine. Otherwise? What should it do? Bail out? Why is 53MHz better >> and more likely to produce a picture than 66MHz, when 65MHz is requested? >> That's of course an impossible question for the driver to answer. >> >> So, if you are not ok with that, you need to implement something that uses >> the min/max fields from the various fields inside struct display_timing >> instead of only looking at the typ field. E.g. the panel_lvds driver calls >> videomode_from_timings() and the result is a single possible mode with only >> the typical timings, with no negotiation of the best option within the >> given ranges with the other drivers involved with the pipe. I think the >> panel-simple driver also makes this one-sided decision of only making use >> of the typ field for each given timing range. Having dabbled a bit in what >> the sound stack does to negotiate the sample rate, sample format and >> channel count etc, I can only predict that retrofitting something like that >> for video modes will be ... interesting. Which is probably why it's not >> done at all, at least not in the general case. >> >> And yes, I agree, the current mechanics are less than ideal. But I have no >> time to do anything about it. >> >>> Although I am not familiar with how other DRM >>> drivers are handling this kind of scenarios. Maybe you and/or other DRM >>> guys knows more about it. >> >> I don't know (and I mean it literally), but maybe these chips are special >> as they typically end up with very small dividers and thus large frequency >> steps? BTW, I do not consider myself a DRM guy, I have only tried to >> fix that which did not work out for our needs... >> >>> Just as a notice, it may worth adding a print message saying what was >>> frequency was requested and what frequency has been setup by driver. >> >> I have no problem with that. > > Hi Peter, > > I intend to prepare my v2 of this series. How would you like to proceed > with the patch you provided? Are you OK if I add it to my v2 of this series > or would you prefer to send it on your own? It would be awesome if you shepherd it for me, thanks! Cheers, Peter _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 4/5] Revert "drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested" @ 2019-12-13 9:30 ` Peter Rosin 0 siblings, 0 replies; 54+ messages in thread From: Peter Rosin @ 2019-12-13 9:30 UTC (permalink / raw) To: Claudiu.Beznea, sam, bbrezillon, airlied, daniel, Nicolas.Ferre, alexandre.belloni, Ludovic.Desroches, lee.jones Cc: linux-arm-kernel, dri-devel, linux-kernel On 2019-12-13 10:28, Claudiu.Beznea@microchip.com wrote: > > > On 11.12.2019 15:28, Peter Rosin wrote: >> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >> >> On 2019-12-11 12:45, Claudiu.Beznea@microchip.com wrote: >>> >>> >>> On 10.12.2019 19:22, Peter Rosin wrote: >>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >>>> >>>> On 2019-12-10 15:59, Claudiu.Beznea@microchip.com wrote: >>>>> >>>>> >>>>> On 10.12.2019 16:11, Peter Rosin wrote: >>>>>> On 2019-12-10 14:24, Claudiu Beznea wrote: >>>>>>> This reverts commit f6f7ad3234613f6f7f27c25036aaf078de07e9b0. >>>>>>> ("drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested") >>>>>>> because allowing selecting a higher pixel clock may overclock >>>>>>> LCD devices, not all of them being capable of this. >>>>>> >>>>>> Without this patch, there are panels that are *severly* underclocked (on the >>>>>> magnitude of 40MHz instead of 65MHz or something like that, I don't remember >>>>>> the exact figures). >>>>> >>>>> With patch that switches by default to 2xsystem clock for pixel clock, if >>>>> using 133MHz system clock (as you specified in the patch I proposed for >>>>> revert here) that would go, without this patch at 53MHz if 65MHz is >>>>> requested. Correct me if I'm wrong. >>>> >>>> It might have been 53MHz, whatever it was it was too low for things to work. >>>> >>>>>> And they are of course not capable of that. All panels >>>>>> have *some* slack as to what frequencies are supported, and the patch was >>>>>> written under the assumption that the preferred frequency of the panel was >>>>>> requested, which should leave at least a *little* headroom. >>>>> >>>>> I see, but from my point of view, the upper layers should decide what >>>>> frequency settings should be done on the LCD controller and not let this at >>>>> the driver's latitude. >>>> >>>> Right, but the upper layers do not support negotiating a frequency from >>>> ranges. At least the didn't when the patch was written, and implementing >>>> *that* seemed like a huge undertaking. >>>> >>>>>> >>>>>> So, I'm curious as to what panel regressed. Or rather, what pixel-clock it needs >>>>>> and what it gets with/without the patch? >>>>> >>>>> I have 2 use cases: >>>>> 1/ system clock = 200MHz and requested pixel clock (mode_rate) ~71MHz. With >>>>> the reverted patch the resulted computed pixel clock would be 80MHz. >>>>> Previously it was at 66MHz >>>> >>>> I don't see how that's possible. >>>> >>>> [doing some calculation by hand] >>>> >>>> Arrgh. *blush* >>>> >>>> The code does not do what I intended for it to do. >>>> Can you please try this instead of reverting? >>>> >>>> Cheers, >>>> Peter >>>> >>>> From b3e86d55b8d107a5c07e98f879c67f67120c87a6 Mon Sep 17 00:00:00 2001 >>>> From: Peter Rosin <peda@axentia.se> >>>> Date: Tue, 10 Dec 2019 18:11:28 +0100 >>>> Subject: [PATCH] drm/atmel-hlcdc: prefer a lower pixel-clock than requested >>>> >>>> The intention was to only select a higher pixel-clock rate than the >>>> requested, if a slight overclocking would result in a rate significantly >>>> closer to the requested rate than if the conservative lower pixel-clock >>>> rate is selected. The fixed patch has the logic the other way around and >>>> actually prefers the higher frequency. Fix that. >>>> >>>> Fixes: f6f7ad323461 ("drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested") >>>> Reported-by: Claudiu Beznea <claudiu.beznea@microchip.com> >>>> Signed-off-by: Peter Rosin <peda@axentia.se> >>>> --- >>>> drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 4 ++-- >>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c >>>> index 9e34bce089d0..03691845d37a 100644 >>>> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c >>>> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c >>>> @@ -120,8 +120,8 @@ static void atmel_hlcdc_crtc_mode_set_nofb(struct drm_crtc *c) >>>> int div_low = prate / mode_rate; >>>> >>>> if (div_low >= 2 && >>>> - ((prate / div_low - mode_rate) < >>>> - 10 * (mode_rate - prate / div))) >>>> + (10 * (prate / div_low - mode_rate) < >>>> + (mode_rate - prate / div))) >>> >>> I tested it on my setup (I have only one of those specified above) and it >>> is OK. Doing some math for the other setup it should also be OK. >> >> Glad to hear it, and thanks for testing/verifying! >> >>> As a whole, I'm OK with this at the moment (let's hope it will work for all >>> use-cases) but still I am not OK with selecting here, in the driver, >>> something that might work. >> >> The driver has to select *something*. If it can deliver the exact requested >> frequency, fine. Otherwise? What should it do? Bail out? Why is 53MHz better >> and more likely to produce a picture than 66MHz, when 65MHz is requested? >> That's of course an impossible question for the driver to answer. >> >> So, if you are not ok with that, you need to implement something that uses >> the min/max fields from the various fields inside struct display_timing >> instead of only looking at the typ field. E.g. the panel_lvds driver calls >> videomode_from_timings() and the result is a single possible mode with only >> the typical timings, with no negotiation of the best option within the >> given ranges with the other drivers involved with the pipe. I think the >> panel-simple driver also makes this one-sided decision of only making use >> of the typ field for each given timing range. Having dabbled a bit in what >> the sound stack does to negotiate the sample rate, sample format and >> channel count etc, I can only predict that retrofitting something like that >> for video modes will be ... interesting. Which is probably why it's not >> done at all, at least not in the general case. >> >> And yes, I agree, the current mechanics are less than ideal. But I have no >> time to do anything about it. >> >>> Although I am not familiar with how other DRM >>> drivers are handling this kind of scenarios. Maybe you and/or other DRM >>> guys knows more about it. >> >> I don't know (and I mean it literally), but maybe these chips are special >> as they typically end up with very small dividers and thus large frequency >> steps? BTW, I do not consider myself a DRM guy, I have only tried to >> fix that which did not work out for our needs... >> >>> Just as a notice, it may worth adding a print message saying what was >>> frequency was requested and what frequency has been setup by driver. >> >> I have no problem with that. > > Hi Peter, > > I intend to prepare my v2 of this series. How would you like to proceed > with the patch you provided? Are you OK if I add it to my v2 of this series > or would you prefer to send it on your own? It would be awesome if you shepherd it for me, thanks! Cheers, Peter _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 54+ messages in thread
* [PATCH 5/5] Revert "drm: atmel-hlcdc: enable sys_clk during initalization." 2019-12-10 13:24 ` Claudiu Beznea (?) @ 2019-12-10 13:24 ` Claudiu Beznea -1 siblings, 0 replies; 54+ messages in thread From: Claudiu Beznea @ 2019-12-10 13:24 UTC (permalink / raw) To: sam, bbrezillon, airlied, daniel, nicolas.ferre, alexandre.belloni, ludovic.desroches, lee.jones Cc: dri-devel, linux-arm-kernel, linux-kernel, Claudiu Beznea, Sandeep Sheriker Mallikarjun This reverts commit d2c755e66617620b729041c625a6396c81d1231c. ("drm: atmel-hlcdc: enable sys_clk during initalization."). With commit "drm: atmel-hlcdc: enable clock before configuring timing engine" there is no need for this patch. Code is also simpler. Cc: Sandeep Sheriker Mallikarjun <sandeepsheriker.mallikarjun@microchip.com> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> --- drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 19 +------------------ 1 file changed, 1 insertion(+), 18 deletions(-) diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c index 8dc917a1270b..112aa5066cee 100644 --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c @@ -721,18 +721,10 @@ static int atmel_hlcdc_dc_load(struct drm_device *dev) dc->hlcdc = dev_get_drvdata(dev->dev->parent); dev->dev_private = dc; - if (dc->desc->fixed_clksrc) { - ret = clk_prepare_enable(dc->hlcdc->sys_clk); - if (ret) { - dev_err(dev->dev, "failed to enable sys_clk\n"); - goto err_destroy_wq; - } - } - ret = clk_prepare_enable(dc->hlcdc->periph_clk); if (ret) { dev_err(dev->dev, "failed to enable periph_clk\n"); - goto err_sys_clk_disable; + goto err_destroy_wq; } pm_runtime_enable(dev->dev); @@ -768,9 +760,6 @@ static int atmel_hlcdc_dc_load(struct drm_device *dev) err_periph_clk_disable: pm_runtime_disable(dev->dev); clk_disable_unprepare(dc->hlcdc->periph_clk); -err_sys_clk_disable: - if (dc->desc->fixed_clksrc) - clk_disable_unprepare(dc->hlcdc->sys_clk); err_destroy_wq: destroy_workqueue(dc->wq); @@ -795,8 +784,6 @@ static void atmel_hlcdc_dc_unload(struct drm_device *dev) pm_runtime_disable(dev->dev); clk_disable_unprepare(dc->hlcdc->periph_clk); - if (dc->desc->fixed_clksrc) - clk_disable_unprepare(dc->hlcdc->sys_clk); destroy_workqueue(dc->wq); } @@ -910,8 +897,6 @@ static int atmel_hlcdc_dc_drm_suspend(struct device *dev) regmap_read(regmap, ATMEL_HLCDC_IMR, &dc->suspend.imr); regmap_write(regmap, ATMEL_HLCDC_IDR, dc->suspend.imr); clk_disable_unprepare(dc->hlcdc->periph_clk); - if (dc->desc->fixed_clksrc) - clk_disable_unprepare(dc->hlcdc->sys_clk); return 0; } @@ -921,8 +906,6 @@ static int atmel_hlcdc_dc_drm_resume(struct device *dev) struct drm_device *drm_dev = dev_get_drvdata(dev); struct atmel_hlcdc_dc *dc = drm_dev->dev_private; - if (dc->desc->fixed_clksrc) - clk_prepare_enable(dc->hlcdc->sys_clk); clk_prepare_enable(dc->hlcdc->periph_clk); regmap_write(dc->hlcdc->regmap, ATMEL_HLCDC_IER, dc->suspend.imr); -- 2.7.4 ^ permalink raw reply related [flat|nested] 54+ messages in thread
* [PATCH 5/5] Revert "drm: atmel-hlcdc: enable sys_clk during initalization." @ 2019-12-10 13:24 ` Claudiu Beznea 0 siblings, 0 replies; 54+ messages in thread From: Claudiu Beznea @ 2019-12-10 13:24 UTC (permalink / raw) To: sam, bbrezillon, airlied, daniel, nicolas.ferre, alexandre.belloni, ludovic.desroches, lee.jones Cc: Sandeep Sheriker Mallikarjun, Claudiu Beznea, linux-arm-kernel, dri-devel, linux-kernel This reverts commit d2c755e66617620b729041c625a6396c81d1231c. ("drm: atmel-hlcdc: enable sys_clk during initalization."). With commit "drm: atmel-hlcdc: enable clock before configuring timing engine" there is no need for this patch. Code is also simpler. Cc: Sandeep Sheriker Mallikarjun <sandeepsheriker.mallikarjun@microchip.com> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> --- drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 19 +------------------ 1 file changed, 1 insertion(+), 18 deletions(-) diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c index 8dc917a1270b..112aa5066cee 100644 --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c @@ -721,18 +721,10 @@ static int atmel_hlcdc_dc_load(struct drm_device *dev) dc->hlcdc = dev_get_drvdata(dev->dev->parent); dev->dev_private = dc; - if (dc->desc->fixed_clksrc) { - ret = clk_prepare_enable(dc->hlcdc->sys_clk); - if (ret) { - dev_err(dev->dev, "failed to enable sys_clk\n"); - goto err_destroy_wq; - } - } - ret = clk_prepare_enable(dc->hlcdc->periph_clk); if (ret) { dev_err(dev->dev, "failed to enable periph_clk\n"); - goto err_sys_clk_disable; + goto err_destroy_wq; } pm_runtime_enable(dev->dev); @@ -768,9 +760,6 @@ static int atmel_hlcdc_dc_load(struct drm_device *dev) err_periph_clk_disable: pm_runtime_disable(dev->dev); clk_disable_unprepare(dc->hlcdc->periph_clk); -err_sys_clk_disable: - if (dc->desc->fixed_clksrc) - clk_disable_unprepare(dc->hlcdc->sys_clk); err_destroy_wq: destroy_workqueue(dc->wq); @@ -795,8 +784,6 @@ static void atmel_hlcdc_dc_unload(struct drm_device *dev) pm_runtime_disable(dev->dev); clk_disable_unprepare(dc->hlcdc->periph_clk); - if (dc->desc->fixed_clksrc) - clk_disable_unprepare(dc->hlcdc->sys_clk); destroy_workqueue(dc->wq); } @@ -910,8 +897,6 @@ static int atmel_hlcdc_dc_drm_suspend(struct device *dev) regmap_read(regmap, ATMEL_HLCDC_IMR, &dc->suspend.imr); regmap_write(regmap, ATMEL_HLCDC_IDR, dc->suspend.imr); clk_disable_unprepare(dc->hlcdc->periph_clk); - if (dc->desc->fixed_clksrc) - clk_disable_unprepare(dc->hlcdc->sys_clk); return 0; } @@ -921,8 +906,6 @@ static int atmel_hlcdc_dc_drm_resume(struct device *dev) struct drm_device *drm_dev = dev_get_drvdata(dev); struct atmel_hlcdc_dc *dc = drm_dev->dev_private; - if (dc->desc->fixed_clksrc) - clk_prepare_enable(dc->hlcdc->sys_clk); clk_prepare_enable(dc->hlcdc->periph_clk); regmap_write(dc->hlcdc->regmap, ATMEL_HLCDC_IER, dc->suspend.imr); -- 2.7.4 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 54+ messages in thread
* [PATCH 5/5] Revert "drm: atmel-hlcdc: enable sys_clk during initalization." @ 2019-12-10 13:24 ` Claudiu Beznea 0 siblings, 0 replies; 54+ messages in thread From: Claudiu Beznea @ 2019-12-10 13:24 UTC (permalink / raw) To: sam, bbrezillon, airlied, daniel, nicolas.ferre, alexandre.belloni, ludovic.desroches, lee.jones Cc: Sandeep Sheriker Mallikarjun, Claudiu Beznea, linux-arm-kernel, dri-devel, linux-kernel This reverts commit d2c755e66617620b729041c625a6396c81d1231c. ("drm: atmel-hlcdc: enable sys_clk during initalization."). With commit "drm: atmel-hlcdc: enable clock before configuring timing engine" there is no need for this patch. Code is also simpler. Cc: Sandeep Sheriker Mallikarjun <sandeepsheriker.mallikarjun@microchip.com> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> --- drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 19 +------------------ 1 file changed, 1 insertion(+), 18 deletions(-) diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c index 8dc917a1270b..112aa5066cee 100644 --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c @@ -721,18 +721,10 @@ static int atmel_hlcdc_dc_load(struct drm_device *dev) dc->hlcdc = dev_get_drvdata(dev->dev->parent); dev->dev_private = dc; - if (dc->desc->fixed_clksrc) { - ret = clk_prepare_enable(dc->hlcdc->sys_clk); - if (ret) { - dev_err(dev->dev, "failed to enable sys_clk\n"); - goto err_destroy_wq; - } - } - ret = clk_prepare_enable(dc->hlcdc->periph_clk); if (ret) { dev_err(dev->dev, "failed to enable periph_clk\n"); - goto err_sys_clk_disable; + goto err_destroy_wq; } pm_runtime_enable(dev->dev); @@ -768,9 +760,6 @@ static int atmel_hlcdc_dc_load(struct drm_device *dev) err_periph_clk_disable: pm_runtime_disable(dev->dev); clk_disable_unprepare(dc->hlcdc->periph_clk); -err_sys_clk_disable: - if (dc->desc->fixed_clksrc) - clk_disable_unprepare(dc->hlcdc->sys_clk); err_destroy_wq: destroy_workqueue(dc->wq); @@ -795,8 +784,6 @@ static void atmel_hlcdc_dc_unload(struct drm_device *dev) pm_runtime_disable(dev->dev); clk_disable_unprepare(dc->hlcdc->periph_clk); - if (dc->desc->fixed_clksrc) - clk_disable_unprepare(dc->hlcdc->sys_clk); destroy_workqueue(dc->wq); } @@ -910,8 +897,6 @@ static int atmel_hlcdc_dc_drm_suspend(struct device *dev) regmap_read(regmap, ATMEL_HLCDC_IMR, &dc->suspend.imr); regmap_write(regmap, ATMEL_HLCDC_IDR, dc->suspend.imr); clk_disable_unprepare(dc->hlcdc->periph_clk); - if (dc->desc->fixed_clksrc) - clk_disable_unprepare(dc->hlcdc->sys_clk); return 0; } @@ -921,8 +906,6 @@ static int atmel_hlcdc_dc_drm_resume(struct device *dev) struct drm_device *drm_dev = dev_get_drvdata(dev); struct atmel_hlcdc_dc *dc = drm_dev->dev_private; - if (dc->desc->fixed_clksrc) - clk_prepare_enable(dc->hlcdc->sys_clk); clk_prepare_enable(dc->hlcdc->periph_clk); regmap_write(dc->hlcdc->regmap, ATMEL_HLCDC_IER, dc->suspend.imr); -- 2.7.4 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 54+ messages in thread
* Re: [PATCH 5/5] Revert "drm: atmel-hlcdc: enable sys_clk during initalization." 2019-12-10 13:24 ` Claudiu Beznea (?) @ 2019-12-10 20:34 ` Sam Ravnborg -1 siblings, 0 replies; 54+ messages in thread From: Sam Ravnborg @ 2019-12-10 20:34 UTC (permalink / raw) To: Claudiu Beznea Cc: bbrezillon, airlied, daniel, nicolas.ferre, alexandre.belloni, ludovic.desroches, lee.jones, dri-devel, linux-arm-kernel, linux-kernel, Sandeep Sheriker Mallikarjun Hi Cladiu On Tue, Dec 10, 2019 at 03:24:47PM +0200, Claudiu Beznea wrote: > This reverts commit d2c755e66617620b729041c625a6396c81d1231c. > ("drm: atmel-hlcdc: enable sys_clk during initalization."). With > commit "drm: atmel-hlcdc: enable clock before configuring timing engine" > there is no need for this patch. Code is also simpler. > > Cc: Sandeep Sheriker Mallikarjun <sandeepsheriker.mallikarjun@microchip.com> > Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> Getting further in the patches tells me you looked at the patch I referenced in previous mail. Please squash the two patches together - that would make it easier to follow what is done. With the two patches applied sysclk is enabled only in mode_set_nofb() and atomic_enable(). And disabled in atomic_disable(). This is simpler and we drop the conditionals. Also good. So the end result looks OK. Sam > --- > drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 19 +------------------ > 1 file changed, 1 insertion(+), 18 deletions(-) > > diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c > index 8dc917a1270b..112aa5066cee 100644 > --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c > +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c > @@ -721,18 +721,10 @@ static int atmel_hlcdc_dc_load(struct drm_device *dev) > dc->hlcdc = dev_get_drvdata(dev->dev->parent); > dev->dev_private = dc; > > - if (dc->desc->fixed_clksrc) { > - ret = clk_prepare_enable(dc->hlcdc->sys_clk); > - if (ret) { > - dev_err(dev->dev, "failed to enable sys_clk\n"); > - goto err_destroy_wq; > - } > - } > - > ret = clk_prepare_enable(dc->hlcdc->periph_clk); > if (ret) { > dev_err(dev->dev, "failed to enable periph_clk\n"); > - goto err_sys_clk_disable; > + goto err_destroy_wq; > } > > pm_runtime_enable(dev->dev); > @@ -768,9 +760,6 @@ static int atmel_hlcdc_dc_load(struct drm_device *dev) > err_periph_clk_disable: > pm_runtime_disable(dev->dev); > clk_disable_unprepare(dc->hlcdc->periph_clk); > -err_sys_clk_disable: > - if (dc->desc->fixed_clksrc) > - clk_disable_unprepare(dc->hlcdc->sys_clk); > > err_destroy_wq: > destroy_workqueue(dc->wq); > @@ -795,8 +784,6 @@ static void atmel_hlcdc_dc_unload(struct drm_device *dev) > > pm_runtime_disable(dev->dev); > clk_disable_unprepare(dc->hlcdc->periph_clk); > - if (dc->desc->fixed_clksrc) > - clk_disable_unprepare(dc->hlcdc->sys_clk); > destroy_workqueue(dc->wq); > } > > @@ -910,8 +897,6 @@ static int atmel_hlcdc_dc_drm_suspend(struct device *dev) > regmap_read(regmap, ATMEL_HLCDC_IMR, &dc->suspend.imr); > regmap_write(regmap, ATMEL_HLCDC_IDR, dc->suspend.imr); > clk_disable_unprepare(dc->hlcdc->periph_clk); > - if (dc->desc->fixed_clksrc) > - clk_disable_unprepare(dc->hlcdc->sys_clk); > > return 0; > } > @@ -921,8 +906,6 @@ static int atmel_hlcdc_dc_drm_resume(struct device *dev) > struct drm_device *drm_dev = dev_get_drvdata(dev); > struct atmel_hlcdc_dc *dc = drm_dev->dev_private; > > - if (dc->desc->fixed_clksrc) > - clk_prepare_enable(dc->hlcdc->sys_clk); > clk_prepare_enable(dc->hlcdc->periph_clk); > regmap_write(dc->hlcdc->regmap, ATMEL_HLCDC_IER, dc->suspend.imr); > > -- > 2.7.4 ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 5/5] Revert "drm: atmel-hlcdc: enable sys_clk during initalization." @ 2019-12-10 20:34 ` Sam Ravnborg 0 siblings, 0 replies; 54+ messages in thread From: Sam Ravnborg @ 2019-12-10 20:34 UTC (permalink / raw) To: Claudiu Beznea Cc: alexandre.belloni, bbrezillon, airlied, Sandeep Sheriker Mallikarjun, nicolas.ferre, dri-devel, linux-kernel, ludovic.desroches, lee.jones, linux-arm-kernel Hi Cladiu On Tue, Dec 10, 2019 at 03:24:47PM +0200, Claudiu Beznea wrote: > This reverts commit d2c755e66617620b729041c625a6396c81d1231c. > ("drm: atmel-hlcdc: enable sys_clk during initalization."). With > commit "drm: atmel-hlcdc: enable clock before configuring timing engine" > there is no need for this patch. Code is also simpler. > > Cc: Sandeep Sheriker Mallikarjun <sandeepsheriker.mallikarjun@microchip.com> > Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> Getting further in the patches tells me you looked at the patch I referenced in previous mail. Please squash the two patches together - that would make it easier to follow what is done. With the two patches applied sysclk is enabled only in mode_set_nofb() and atomic_enable(). And disabled in atomic_disable(). This is simpler and we drop the conditionals. Also good. So the end result looks OK. Sam > --- > drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 19 +------------------ > 1 file changed, 1 insertion(+), 18 deletions(-) > > diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c > index 8dc917a1270b..112aa5066cee 100644 > --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c > +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c > @@ -721,18 +721,10 @@ static int atmel_hlcdc_dc_load(struct drm_device *dev) > dc->hlcdc = dev_get_drvdata(dev->dev->parent); > dev->dev_private = dc; > > - if (dc->desc->fixed_clksrc) { > - ret = clk_prepare_enable(dc->hlcdc->sys_clk); > - if (ret) { > - dev_err(dev->dev, "failed to enable sys_clk\n"); > - goto err_destroy_wq; > - } > - } > - > ret = clk_prepare_enable(dc->hlcdc->periph_clk); > if (ret) { > dev_err(dev->dev, "failed to enable periph_clk\n"); > - goto err_sys_clk_disable; > + goto err_destroy_wq; > } > > pm_runtime_enable(dev->dev); > @@ -768,9 +760,6 @@ static int atmel_hlcdc_dc_load(struct drm_device *dev) > err_periph_clk_disable: > pm_runtime_disable(dev->dev); > clk_disable_unprepare(dc->hlcdc->periph_clk); > -err_sys_clk_disable: > - if (dc->desc->fixed_clksrc) > - clk_disable_unprepare(dc->hlcdc->sys_clk); > > err_destroy_wq: > destroy_workqueue(dc->wq); > @@ -795,8 +784,6 @@ static void atmel_hlcdc_dc_unload(struct drm_device *dev) > > pm_runtime_disable(dev->dev); > clk_disable_unprepare(dc->hlcdc->periph_clk); > - if (dc->desc->fixed_clksrc) > - clk_disable_unprepare(dc->hlcdc->sys_clk); > destroy_workqueue(dc->wq); > } > > @@ -910,8 +897,6 @@ static int atmel_hlcdc_dc_drm_suspend(struct device *dev) > regmap_read(regmap, ATMEL_HLCDC_IMR, &dc->suspend.imr); > regmap_write(regmap, ATMEL_HLCDC_IDR, dc->suspend.imr); > clk_disable_unprepare(dc->hlcdc->periph_clk); > - if (dc->desc->fixed_clksrc) > - clk_disable_unprepare(dc->hlcdc->sys_clk); > > return 0; > } > @@ -921,8 +906,6 @@ static int atmel_hlcdc_dc_drm_resume(struct device *dev) > struct drm_device *drm_dev = dev_get_drvdata(dev); > struct atmel_hlcdc_dc *dc = drm_dev->dev_private; > > - if (dc->desc->fixed_clksrc) > - clk_prepare_enable(dc->hlcdc->sys_clk); > clk_prepare_enable(dc->hlcdc->periph_clk); > regmap_write(dc->hlcdc->regmap, ATMEL_HLCDC_IER, dc->suspend.imr); > > -- > 2.7.4 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 5/5] Revert "drm: atmel-hlcdc: enable sys_clk during initalization." @ 2019-12-10 20:34 ` Sam Ravnborg 0 siblings, 0 replies; 54+ messages in thread From: Sam Ravnborg @ 2019-12-10 20:34 UTC (permalink / raw) To: Claudiu Beznea Cc: alexandre.belloni, bbrezillon, airlied, Sandeep Sheriker Mallikarjun, dri-devel, linux-kernel, ludovic.desroches, daniel, lee.jones, linux-arm-kernel Hi Cladiu On Tue, Dec 10, 2019 at 03:24:47PM +0200, Claudiu Beznea wrote: > This reverts commit d2c755e66617620b729041c625a6396c81d1231c. > ("drm: atmel-hlcdc: enable sys_clk during initalization."). With > commit "drm: atmel-hlcdc: enable clock before configuring timing engine" > there is no need for this patch. Code is also simpler. > > Cc: Sandeep Sheriker Mallikarjun <sandeepsheriker.mallikarjun@microchip.com> > Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> Getting further in the patches tells me you looked at the patch I referenced in previous mail. Please squash the two patches together - that would make it easier to follow what is done. With the two patches applied sysclk is enabled only in mode_set_nofb() and atomic_enable(). And disabled in atomic_disable(). This is simpler and we drop the conditionals. Also good. So the end result looks OK. Sam > --- > drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 19 +------------------ > 1 file changed, 1 insertion(+), 18 deletions(-) > > diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c > index 8dc917a1270b..112aa5066cee 100644 > --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c > +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c > @@ -721,18 +721,10 @@ static int atmel_hlcdc_dc_load(struct drm_device *dev) > dc->hlcdc = dev_get_drvdata(dev->dev->parent); > dev->dev_private = dc; > > - if (dc->desc->fixed_clksrc) { > - ret = clk_prepare_enable(dc->hlcdc->sys_clk); > - if (ret) { > - dev_err(dev->dev, "failed to enable sys_clk\n"); > - goto err_destroy_wq; > - } > - } > - > ret = clk_prepare_enable(dc->hlcdc->periph_clk); > if (ret) { > dev_err(dev->dev, "failed to enable periph_clk\n"); > - goto err_sys_clk_disable; > + goto err_destroy_wq; > } > > pm_runtime_enable(dev->dev); > @@ -768,9 +760,6 @@ static int atmel_hlcdc_dc_load(struct drm_device *dev) > err_periph_clk_disable: > pm_runtime_disable(dev->dev); > clk_disable_unprepare(dc->hlcdc->periph_clk); > -err_sys_clk_disable: > - if (dc->desc->fixed_clksrc) > - clk_disable_unprepare(dc->hlcdc->sys_clk); > > err_destroy_wq: > destroy_workqueue(dc->wq); > @@ -795,8 +784,6 @@ static void atmel_hlcdc_dc_unload(struct drm_device *dev) > > pm_runtime_disable(dev->dev); > clk_disable_unprepare(dc->hlcdc->periph_clk); > - if (dc->desc->fixed_clksrc) > - clk_disable_unprepare(dc->hlcdc->sys_clk); > destroy_workqueue(dc->wq); > } > > @@ -910,8 +897,6 @@ static int atmel_hlcdc_dc_drm_suspend(struct device *dev) > regmap_read(regmap, ATMEL_HLCDC_IMR, &dc->suspend.imr); > regmap_write(regmap, ATMEL_HLCDC_IDR, dc->suspend.imr); > clk_disable_unprepare(dc->hlcdc->periph_clk); > - if (dc->desc->fixed_clksrc) > - clk_disable_unprepare(dc->hlcdc->sys_clk); > > return 0; > } > @@ -921,8 +906,6 @@ static int atmel_hlcdc_dc_drm_resume(struct device *dev) > struct drm_device *drm_dev = dev_get_drvdata(dev); > struct atmel_hlcdc_dc *dc = drm_dev->dev_private; > > - if (dc->desc->fixed_clksrc) > - clk_prepare_enable(dc->hlcdc->sys_clk); > clk_prepare_enable(dc->hlcdc->periph_clk); > regmap_write(dc->hlcdc->regmap, ATMEL_HLCDC_IER, dc->suspend.imr); > > -- > 2.7.4 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 5/5] Revert "drm: atmel-hlcdc: enable sys_clk during initalization." 2019-12-10 20:34 ` Sam Ravnborg (?) @ 2019-12-11 11:55 ` Claudiu.Beznea -1 siblings, 0 replies; 54+ messages in thread From: Claudiu.Beznea @ 2019-12-11 11:55 UTC (permalink / raw) To: sam Cc: bbrezillon, airlied, daniel, Nicolas.Ferre, alexandre.belloni, Ludovic.Desroches, lee.jones, dri-devel, linux-arm-kernel, linux-kernel, Sandeep.Sheriker On 10.12.2019 22:34, Sam Ravnborg wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > Hi Cladiu > > On Tue, Dec 10, 2019 at 03:24:47PM +0200, Claudiu Beznea wrote: >> This reverts commit d2c755e66617620b729041c625a6396c81d1231c. >> ("drm: atmel-hlcdc: enable sys_clk during initalization."). With >> commit "drm: atmel-hlcdc: enable clock before configuring timing engine" >> there is no need for this patch. Code is also simpler. >> >> Cc: Sandeep Sheriker Mallikarjun <sandeepsheriker.mallikarjun@microchip.com> >> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> > > Getting further in the patches tells me you looked at the > patch I referenced in previous mail. > Please squash the two patches together - that would make it > easier to follow what is done. Wouldn't this lead to a patch doing 2 things? 1/ fix the timeout of the timing engine after setting pixel clock which is from the beginning of the driver and has nothing to do with patch reverted here (but, actually we wouldn't had reach the point of introducing the patch reverted here with that fix) 2/ revert a previous functionality as a result of fixing the timeout. With this in mind would you still want to squash them? Thank you, Claudiu Beznea > > With the two patches applied sysclk is enabled only in mode_set_nofb() > and atomic_enable(). And disabled in atomic_disable(). > This is simpler and we drop the conditionals. Also good. > So the end result looks OK. > > Sam > >> --- >> drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 19 +------------------ >> 1 file changed, 1 insertion(+), 18 deletions(-) >> >> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c >> index 8dc917a1270b..112aa5066cee 100644 >> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c >> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c >> @@ -721,18 +721,10 @@ static int atmel_hlcdc_dc_load(struct drm_device *dev) >> dc->hlcdc = dev_get_drvdata(dev->dev->parent); >> dev->dev_private = dc; >> >> - if (dc->desc->fixed_clksrc) { >> - ret = clk_prepare_enable(dc->hlcdc->sys_clk); >> - if (ret) { >> - dev_err(dev->dev, "failed to enable sys_clk\n"); >> - goto err_destroy_wq; >> - } >> - } >> - >> ret = clk_prepare_enable(dc->hlcdc->periph_clk); >> if (ret) { >> dev_err(dev->dev, "failed to enable periph_clk\n"); >> - goto err_sys_clk_disable; >> + goto err_destroy_wq; >> } >> >> pm_runtime_enable(dev->dev); >> @@ -768,9 +760,6 @@ static int atmel_hlcdc_dc_load(struct drm_device *dev) >> err_periph_clk_disable: >> pm_runtime_disable(dev->dev); >> clk_disable_unprepare(dc->hlcdc->periph_clk); >> -err_sys_clk_disable: >> - if (dc->desc->fixed_clksrc) >> - clk_disable_unprepare(dc->hlcdc->sys_clk); >> >> err_destroy_wq: >> destroy_workqueue(dc->wq); >> @@ -795,8 +784,6 @@ static void atmel_hlcdc_dc_unload(struct drm_device *dev) >> >> pm_runtime_disable(dev->dev); >> clk_disable_unprepare(dc->hlcdc->periph_clk); >> - if (dc->desc->fixed_clksrc) >> - clk_disable_unprepare(dc->hlcdc->sys_clk); >> destroy_workqueue(dc->wq); >> } >> >> @@ -910,8 +897,6 @@ static int atmel_hlcdc_dc_drm_suspend(struct device *dev) >> regmap_read(regmap, ATMEL_HLCDC_IMR, &dc->suspend.imr); >> regmap_write(regmap, ATMEL_HLCDC_IDR, dc->suspend.imr); >> clk_disable_unprepare(dc->hlcdc->periph_clk); >> - if (dc->desc->fixed_clksrc) >> - clk_disable_unprepare(dc->hlcdc->sys_clk); >> >> return 0; >> } >> @@ -921,8 +906,6 @@ static int atmel_hlcdc_dc_drm_resume(struct device *dev) >> struct drm_device *drm_dev = dev_get_drvdata(dev); >> struct atmel_hlcdc_dc *dc = drm_dev->dev_private; >> >> - if (dc->desc->fixed_clksrc) >> - clk_prepare_enable(dc->hlcdc->sys_clk); >> clk_prepare_enable(dc->hlcdc->periph_clk); >> regmap_write(dc->hlcdc->regmap, ATMEL_HLCDC_IER, dc->suspend.imr); >> >> -- >> 2.7.4 > ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 5/5] Revert "drm: atmel-hlcdc: enable sys_clk during initalization." @ 2019-12-11 11:55 ` Claudiu.Beznea 0 siblings, 0 replies; 54+ messages in thread From: Claudiu.Beznea @ 2019-12-11 11:55 UTC (permalink / raw) To: sam Cc: alexandre.belloni, bbrezillon, airlied, Nicolas.Ferre, dri-devel, linux-kernel, Ludovic.Desroches, Sandeep.Sheriker, lee.jones, linux-arm-kernel On 10.12.2019 22:34, Sam Ravnborg wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > Hi Cladiu > > On Tue, Dec 10, 2019 at 03:24:47PM +0200, Claudiu Beznea wrote: >> This reverts commit d2c755e66617620b729041c625a6396c81d1231c. >> ("drm: atmel-hlcdc: enable sys_clk during initalization."). With >> commit "drm: atmel-hlcdc: enable clock before configuring timing engine" >> there is no need for this patch. Code is also simpler. >> >> Cc: Sandeep Sheriker Mallikarjun <sandeepsheriker.mallikarjun@microchip.com> >> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> > > Getting further in the patches tells me you looked at the > patch I referenced in previous mail. > Please squash the two patches together - that would make it > easier to follow what is done. Wouldn't this lead to a patch doing 2 things? 1/ fix the timeout of the timing engine after setting pixel clock which is from the beginning of the driver and has nothing to do with patch reverted here (but, actually we wouldn't had reach the point of introducing the patch reverted here with that fix) 2/ revert a previous functionality as a result of fixing the timeout. With this in mind would you still want to squash them? Thank you, Claudiu Beznea > > With the two patches applied sysclk is enabled only in mode_set_nofb() > and atomic_enable(). And disabled in atomic_disable(). > This is simpler and we drop the conditionals. Also good. > So the end result looks OK. > > Sam > >> --- >> drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 19 +------------------ >> 1 file changed, 1 insertion(+), 18 deletions(-) >> >> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c >> index 8dc917a1270b..112aa5066cee 100644 >> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c >> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c >> @@ -721,18 +721,10 @@ static int atmel_hlcdc_dc_load(struct drm_device *dev) >> dc->hlcdc = dev_get_drvdata(dev->dev->parent); >> dev->dev_private = dc; >> >> - if (dc->desc->fixed_clksrc) { >> - ret = clk_prepare_enable(dc->hlcdc->sys_clk); >> - if (ret) { >> - dev_err(dev->dev, "failed to enable sys_clk\n"); >> - goto err_destroy_wq; >> - } >> - } >> - >> ret = clk_prepare_enable(dc->hlcdc->periph_clk); >> if (ret) { >> dev_err(dev->dev, "failed to enable periph_clk\n"); >> - goto err_sys_clk_disable; >> + goto err_destroy_wq; >> } >> >> pm_runtime_enable(dev->dev); >> @@ -768,9 +760,6 @@ static int atmel_hlcdc_dc_load(struct drm_device *dev) >> err_periph_clk_disable: >> pm_runtime_disable(dev->dev); >> clk_disable_unprepare(dc->hlcdc->periph_clk); >> -err_sys_clk_disable: >> - if (dc->desc->fixed_clksrc) >> - clk_disable_unprepare(dc->hlcdc->sys_clk); >> >> err_destroy_wq: >> destroy_workqueue(dc->wq); >> @@ -795,8 +784,6 @@ static void atmel_hlcdc_dc_unload(struct drm_device *dev) >> >> pm_runtime_disable(dev->dev); >> clk_disable_unprepare(dc->hlcdc->periph_clk); >> - if (dc->desc->fixed_clksrc) >> - clk_disable_unprepare(dc->hlcdc->sys_clk); >> destroy_workqueue(dc->wq); >> } >> >> @@ -910,8 +897,6 @@ static int atmel_hlcdc_dc_drm_suspend(struct device *dev) >> regmap_read(regmap, ATMEL_HLCDC_IMR, &dc->suspend.imr); >> regmap_write(regmap, ATMEL_HLCDC_IDR, dc->suspend.imr); >> clk_disable_unprepare(dc->hlcdc->periph_clk); >> - if (dc->desc->fixed_clksrc) >> - clk_disable_unprepare(dc->hlcdc->sys_clk); >> >> return 0; >> } >> @@ -921,8 +906,6 @@ static int atmel_hlcdc_dc_drm_resume(struct device *dev) >> struct drm_device *drm_dev = dev_get_drvdata(dev); >> struct atmel_hlcdc_dc *dc = drm_dev->dev_private; >> >> - if (dc->desc->fixed_clksrc) >> - clk_prepare_enable(dc->hlcdc->sys_clk); >> clk_prepare_enable(dc->hlcdc->periph_clk); >> regmap_write(dc->hlcdc->regmap, ATMEL_HLCDC_IER, dc->suspend.imr); >> >> -- >> 2.7.4 > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: [PATCH 5/5] Revert "drm: atmel-hlcdc: enable sys_clk during initalization." @ 2019-12-11 11:55 ` Claudiu.Beznea 0 siblings, 0 replies; 54+ messages in thread From: Claudiu.Beznea @ 2019-12-11 11:55 UTC (permalink / raw) To: sam Cc: alexandre.belloni, bbrezillon, airlied, dri-devel, linux-kernel, Ludovic.Desroches, daniel, Sandeep.Sheriker, lee.jones, linux-arm-kernel On 10.12.2019 22:34, Sam Ravnborg wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > Hi Cladiu > > On Tue, Dec 10, 2019 at 03:24:47PM +0200, Claudiu Beznea wrote: >> This reverts commit d2c755e66617620b729041c625a6396c81d1231c. >> ("drm: atmel-hlcdc: enable sys_clk during initalization."). With >> commit "drm: atmel-hlcdc: enable clock before configuring timing engine" >> there is no need for this patch. Code is also simpler. >> >> Cc: Sandeep Sheriker Mallikarjun <sandeepsheriker.mallikarjun@microchip.com> >> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> > > Getting further in the patches tells me you looked at the > patch I referenced in previous mail. > Please squash the two patches together - that would make it > easier to follow what is done. Wouldn't this lead to a patch doing 2 things? 1/ fix the timeout of the timing engine after setting pixel clock which is from the beginning of the driver and has nothing to do with patch reverted here (but, actually we wouldn't had reach the point of introducing the patch reverted here with that fix) 2/ revert a previous functionality as a result of fixing the timeout. With this in mind would you still want to squash them? Thank you, Claudiu Beznea > > With the two patches applied sysclk is enabled only in mode_set_nofb() > and atomic_enable(). And disabled in atomic_disable(). > This is simpler and we drop the conditionals. Also good. > So the end result looks OK. > > Sam > >> --- >> drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 19 +------------------ >> 1 file changed, 1 insertion(+), 18 deletions(-) >> >> diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c >> index 8dc917a1270b..112aa5066cee 100644 >> --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c >> +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c >> @@ -721,18 +721,10 @@ static int atmel_hlcdc_dc_load(struct drm_device *dev) >> dc->hlcdc = dev_get_drvdata(dev->dev->parent); >> dev->dev_private = dc; >> >> - if (dc->desc->fixed_clksrc) { >> - ret = clk_prepare_enable(dc->hlcdc->sys_clk); >> - if (ret) { >> - dev_err(dev->dev, "failed to enable sys_clk\n"); >> - goto err_destroy_wq; >> - } >> - } >> - >> ret = clk_prepare_enable(dc->hlcdc->periph_clk); >> if (ret) { >> dev_err(dev->dev, "failed to enable periph_clk\n"); >> - goto err_sys_clk_disable; >> + goto err_destroy_wq; >> } >> >> pm_runtime_enable(dev->dev); >> @@ -768,9 +760,6 @@ static int atmel_hlcdc_dc_load(struct drm_device *dev) >> err_periph_clk_disable: >> pm_runtime_disable(dev->dev); >> clk_disable_unprepare(dc->hlcdc->periph_clk); >> -err_sys_clk_disable: >> - if (dc->desc->fixed_clksrc) >> - clk_disable_unprepare(dc->hlcdc->sys_clk); >> >> err_destroy_wq: >> destroy_workqueue(dc->wq); >> @@ -795,8 +784,6 @@ static void atmel_hlcdc_dc_unload(struct drm_device *dev) >> >> pm_runtime_disable(dev->dev); >> clk_disable_unprepare(dc->hlcdc->periph_clk); >> - if (dc->desc->fixed_clksrc) >> - clk_disable_unprepare(dc->hlcdc->sys_clk); >> destroy_workqueue(dc->wq); >> } >> >> @@ -910,8 +897,6 @@ static int atmel_hlcdc_dc_drm_suspend(struct device *dev) >> regmap_read(regmap, ATMEL_HLCDC_IMR, &dc->suspend.imr); >> regmap_write(regmap, ATMEL_HLCDC_IDR, dc->suspend.imr); >> clk_disable_unprepare(dc->hlcdc->periph_clk); >> - if (dc->desc->fixed_clksrc) >> - clk_disable_unprepare(dc->hlcdc->sys_clk); >> >> return 0; >> } >> @@ -921,8 +906,6 @@ static int atmel_hlcdc_dc_drm_resume(struct device *dev) >> struct drm_device *drm_dev = dev_get_drvdata(dev); >> struct atmel_hlcdc_dc *dc = drm_dev->dev_private; >> >> - if (dc->desc->fixed_clksrc) >> - clk_prepare_enable(dc->hlcdc->sys_clk); >> clk_prepare_enable(dc->hlcdc->periph_clk); >> regmap_write(dc->hlcdc->regmap, ATMEL_HLCDC_IER, dc->suspend.imr); >> >> -- >> 2.7.4 > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 54+ messages in thread
end of thread, other threads:[~2019-12-14 14:44 UTC | newest] Thread overview: 54+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-12-10 13:24 [PATCH 0/5] fixes for atmel-hlcdc Claudiu Beznea 2019-12-10 13:24 ` Claudiu Beznea 2019-12-10 13:24 ` Claudiu Beznea 2019-12-10 13:24 ` [PATCH 1/5] drm: atmel-hlcdc: use double rate for pixel clock only if supported Claudiu Beznea 2019-12-10 13:24 ` Claudiu Beznea 2019-12-10 13:24 ` Claudiu Beznea 2019-12-10 13:24 ` [PATCH 2/5] drm: atmel-hlcdc: enable clock before configuring timing engine Claudiu Beznea 2019-12-10 13:24 ` Claudiu Beznea 2019-12-10 13:24 ` Claudiu Beznea 2019-12-10 20:18 ` Sam Ravnborg 2019-12-10 20:18 ` Sam Ravnborg 2019-12-10 20:18 ` Sam Ravnborg 2019-12-10 13:24 ` [PATCH 3/5] mfd: atmel-hlcdc: return in case of error Claudiu Beznea 2019-12-10 13:24 ` Claudiu Beznea 2019-12-10 13:24 ` Claudiu Beznea 2019-12-10 20:37 ` Sam Ravnborg 2019-12-10 20:37 ` Sam Ravnborg 2019-12-10 20:37 ` Sam Ravnborg 2019-12-11 12:07 ` Claudiu.Beznea 2019-12-11 12:07 ` Claudiu.Beznea 2019-12-11 12:07 ` Claudiu.Beznea 2019-12-10 13:24 ` [PATCH 4/5] Revert "drm/atmel-hlcdc: allow selecting a higher pixel-clock than requested" Claudiu Beznea 2019-12-10 13:24 ` Claudiu Beznea 2019-12-10 13:24 ` Claudiu Beznea 2019-12-10 14:11 ` Peter Rosin 2019-12-10 14:11 ` Peter Rosin 2019-12-10 14:11 ` Peter Rosin 2019-12-10 14:59 ` Claudiu.Beznea 2019-12-10 14:59 ` Claudiu.Beznea 2019-12-10 14:59 ` Claudiu.Beznea 2019-12-10 17:22 ` Peter Rosin 2019-12-10 17:22 ` Peter Rosin 2019-12-10 17:22 ` Peter Rosin 2019-12-11 11:45 ` Claudiu.Beznea 2019-12-11 11:45 ` Claudiu.Beznea 2019-12-11 11:45 ` Claudiu.Beznea 2019-12-11 13:28 ` Peter Rosin 2019-12-11 13:28 ` Peter Rosin 2019-12-11 13:28 ` Peter Rosin 2019-12-13 9:28 ` Claudiu.Beznea 2019-12-13 9:28 ` Claudiu.Beznea 2019-12-13 9:28 ` Claudiu.Beznea 2019-12-13 9:30 ` Peter Rosin 2019-12-13 9:30 ` Peter Rosin 2019-12-13 9:30 ` Peter Rosin 2019-12-10 13:24 ` [PATCH 5/5] Revert "drm: atmel-hlcdc: enable sys_clk during initalization." Claudiu Beznea 2019-12-10 13:24 ` Claudiu Beznea 2019-12-10 13:24 ` Claudiu Beznea 2019-12-10 20:34 ` Sam Ravnborg 2019-12-10 20:34 ` Sam Ravnborg 2019-12-10 20:34 ` Sam Ravnborg 2019-12-11 11:55 ` Claudiu.Beznea 2019-12-11 11:55 ` Claudiu.Beznea 2019-12-11 11:55 ` Claudiu.Beznea
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.