* Re: [v3 6/7] drm/mediatek: change the dsi phytiming calculate method
[not found] ` <20190519092537.69053-7-jitao.shi@mediatek.com>
@ 2019-05-19 19:29 ` kbuild test robot
2019-05-30 20:45 ` Ryan Case
2019-06-03 18:14 ` Guenter Roeck
2 siblings, 0 replies; 7+ messages in thread
From: kbuild test robot @ 2019-05-19 19:29 UTC (permalink / raw)
To: Jitao Shi
Cc: kbuild-all, Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell,
Kumar Gala, linux-pwm, David Airlie, Matthias Brugger, stonea168,
dri-devel, Andy Yan, Ajay Kumar, Vincent Palatin, cawa.cheng,
Russell King, Thierry Reding, devicetree, Jitao Shi,
linux-mediatek, yingjoe.chen, eddie.huang, linux-arm-kernel,
Rahul Sharma, srv_heupstream, linux-kernel, Sascha Hauer,
Sean Paul
[-- Attachment #1: Type: text/plain, Size: 1083 bytes --]
Hi Jitao,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v5.1 next-20190517]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Jitao-Shi/Support-dsi-for-mt8183/20190519-231001
config: arm-allmodconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.2.0 make.cross ARCH=arm
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
>> ERROR: "__aeabi_uldivmod" [drivers/gpu/drm/mediatek/mediatek-drm.ko] undefined!
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 69766 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [v3 1/7] drm/mediatek: move mipi_dsi_host_register to probe
[not found] ` <20190519092537.69053-2-jitao.shi@mediatek.com>
@ 2019-05-20 7:18 ` CK Hu
2019-05-29 4:19 ` Hsin-Yi Wang
1 sibling, 0 replies; 7+ messages in thread
From: CK Hu @ 2019-05-20 7:18 UTC (permalink / raw)
To: Jitao Shi
Cc: Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
linux-pwm, David Airlie, Matthias Brugger, Thierry Reding,
Ajay Kumar, Inki Dae, Rahul Sharma, Sean Paul, Vincent Palatin,
Andy Yan, Philipp Zabel, Russell King, devicetree, linux-kernel,
dri-devel, linux-arm-kernel, linux-mediatek, srv_heupstream,
Sascha Hauer, yingjoe.chen, eddie.huang, cawa.cheng, bibby.hsieh,
stonea168
Hi, Jitao:
On Sun, 2019-05-19 at 17:25 +0800, Jitao Shi wrote:
> DSI panel driver need attach function which is inculde in
> mipi_dsi_host_ops.
>
> If mipi_dsi_host_register is not in probe, dsi panel will
> probe fail or more delay.
In [1], you have agreed this patch just for delay not for probe fail
Remove 'probe fail'.in commit message.
[1]
http://lists.infradead.org/pipermail/linux-mediatek/2019-May/019836.html
>
> So move the mipi_dsi_host_register to probe from bind.
>
> Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
> ---
> drivers/gpu/drm/mediatek/mtk_dsi.c | 50 ++++++++++++++++++------------
> 1 file changed, 30 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
> index b00eb2d2e086..6c4ac37f983d 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> @@ -1045,12 +1045,6 @@ static int mtk_dsi_bind(struct device *dev, struct device *master, void *data)
> return ret;
> }
>
> - ret = mipi_dsi_host_register(&dsi->host);
> - if (ret < 0) {
> - dev_err(dev, "failed to register DSI host: %d\n", ret);
> - goto err_ddp_comp_unregister;
> - }
> -
> ret = mtk_dsi_create_conn_enc(drm, dsi);
> if (ret) {
> DRM_ERROR("Encoder create failed with %d\n", ret);
> @@ -1060,8 +1054,6 @@ static int mtk_dsi_bind(struct device *dev, struct device *master, void *data)
> return 0;
>
> err_unregister:
> - mipi_dsi_host_unregister(&dsi->host);
> -err_ddp_comp_unregister:
> mtk_ddp_comp_unregister(drm, &dsi->ddp_comp);
> return ret;
> }
> @@ -1097,31 +1089,37 @@ static int mtk_dsi_probe(struct platform_device *pdev)
>
> dsi->host.ops = &mtk_dsi_ops;
> dsi->host.dev = dev;
> + dsi->dev = dev;
This is for debug message, not for the timing of
mipi_dsi_host_register(), so move to an independent patch.
Regards,
CK
> + ret = mipi_dsi_host_register(&dsi->host);
> + if (ret < 0) {
> + dev_err(dev, "failed to register DSI host: %d\n", ret);
> + return ret;
> + }
>
> ret = drm_of_find_panel_or_bridge(dev->of_node, 0, 0,
> &dsi->panel, &dsi->bridge);
> if (ret)
> - return ret;
> + goto err_unregister_host;
>
> dsi->engine_clk = devm_clk_get(dev, "engine");
> if (IS_ERR(dsi->engine_clk)) {
> ret = PTR_ERR(dsi->engine_clk);
> dev_err(dev, "Failed to get engine clock: %d\n", ret);
> - return ret;
> + goto err_unregister_host;
> }
>
> dsi->digital_clk = devm_clk_get(dev, "digital");
> if (IS_ERR(dsi->digital_clk)) {
> ret = PTR_ERR(dsi->digital_clk);
> dev_err(dev, "Failed to get digital clock: %d\n", ret);
> - return ret;
> + goto err_unregister_host;
> }
>
> dsi->hs_clk = devm_clk_get(dev, "hs");
> if (IS_ERR(dsi->hs_clk)) {
> ret = PTR_ERR(dsi->hs_clk);
> dev_err(dev, "Failed to get hs clock: %d\n", ret);
> - return ret;
> + goto err_unregister_host;
> }
>
> regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> @@ -1129,33 +1127,35 @@ static int mtk_dsi_probe(struct platform_device *pdev)
> if (IS_ERR(dsi->regs)) {
> ret = PTR_ERR(dsi->regs);
> dev_err(dev, "Failed to ioremap memory: %d\n", ret);
> - return ret;
> + goto err_unregister_host;
> }
>
> dsi->phy = devm_phy_get(dev, "dphy");
> if (IS_ERR(dsi->phy)) {
> ret = PTR_ERR(dsi->phy);
> dev_err(dev, "Failed to get MIPI-DPHY: %d\n", ret);
> - return ret;
> + goto err_unregister_host;
> }
>
> comp_id = mtk_ddp_comp_get_id(dev->of_node, MTK_DSI);
> if (comp_id < 0) {
> dev_err(dev, "Failed to identify by alias: %d\n", comp_id);
> - return comp_id;
> + ret = comp_id;
> + goto err_unregister_host;
> }
>
> ret = mtk_ddp_comp_init(dev, dev->of_node, &dsi->ddp_comp, comp_id,
> &mtk_dsi_funcs);
> if (ret) {
> dev_err(dev, "Failed to initialize component: %d\n", ret);
> - return ret;
> + goto err_unregister_host;
> }
>
> irq_num = platform_get_irq(pdev, 0);
> if (irq_num < 0) {
> - dev_err(&pdev->dev, "failed to request dsi irq resource\n");
> - return -EPROBE_DEFER;
> + dev_err(&pdev->dev, "failed to get dsi irq_num: %d\n", irq_num);
> + ret = irq_num;
> + goto err_unregister_host;
> }
>
> irq_set_status_flags(irq_num, IRQ_TYPE_LEVEL_LOW);
> @@ -1163,14 +1163,24 @@ static int mtk_dsi_probe(struct platform_device *pdev)
> IRQF_TRIGGER_LOW, dev_name(&pdev->dev), dsi);
> if (ret) {
> dev_err(&pdev->dev, "failed to request mediatek dsi irq\n");
> - return -EPROBE_DEFER;
> + goto err_unregister_host;
> }
>
> init_waitqueue_head(&dsi->irq_wait_queue);
>
> platform_set_drvdata(pdev, dsi);
>
> - return component_add(&pdev->dev, &mtk_dsi_component_ops);
> + ret = component_add(&pdev->dev, &mtk_dsi_component_ops);
> + if (ret) {
> + dev_err(&pdev->dev, "failed to add component: %d\n", ret);
> + goto err_unregister_host;
> + }
> +
> + return 0;
> +
> +err_unregister_host:
> + mipi_dsi_host_unregister(&dsi->host);
> + return ret;
> }
>
> static int mtk_dsi_remove(struct platform_device *pdev)
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [v3 1/7] drm/mediatek: move mipi_dsi_host_register to probe
[not found] ` <20190519092537.69053-2-jitao.shi@mediatek.com>
2019-05-20 7:18 ` [v3 1/7] drm/mediatek: move mipi_dsi_host_register to probe CK Hu
@ 2019-05-29 4:19 ` Hsin-Yi Wang
1 sibling, 0 replies; 7+ messages in thread
From: Hsin-Yi Wang @ 2019-05-29 4:19 UTC (permalink / raw)
To: Jitao Shi
Cc: Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
linux-pwm, David Airlie, Matthias Brugger, stonea168, dri-devel,
Andy Yan, Ajay Kumar, Vincent Palatin, cawa.cheng, bibby.hsieh,
CK Hu, Russell King, Thierry Reding, devicetree, Philipp Zabel,
Inki Dae, linux-mediatek, yingjoe.chen, eddie.huang,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
Rahul Sharma, srv_heupstream, lkml, Sascha Hauer, Sean Paul
On Sun, May 19, 2019 at 9:25 AM Jitao Shi <jitao.shi@mediatek.com> wrote:
> @@ -1045,12 +1045,6 @@ static int mtk_dsi_bind(struct device *dev, struct device *master, void *data)
> return ret;
> }
>
> - ret = mipi_dsi_host_register(&dsi->host);
> - if (ret < 0) {
> - dev_err(dev, "failed to register DSI host: %d\n", ret);
> - goto err_ddp_comp_unregister;
> - }
> -
> ret = mtk_dsi_create_conn_enc(drm, dsi);
> if (ret) {
> DRM_ERROR("Encoder create failed with %d\n", ret);
> @@ -1060,8 +1054,6 @@ static int mtk_dsi_bind(struct device *dev, struct device *master, void *data)
> return 0;
>
> err_unregister:
> - mipi_dsi_host_unregister(&dsi->host);
> -err_ddp_comp_unregister:
> mtk_ddp_comp_unregister(drm, &dsi->ddp_comp);
> return ret;
> }
> @@ -1097,31 +1089,37 @@ static int mtk_dsi_probe(struct platform_device *pdev)
>
> dsi->host.ops = &mtk_dsi_ops;
> dsi->host.dev = dev;
> + dsi->dev = dev;
> + ret = mipi_dsi_host_register(&dsi->host);
> + if (ret < 0) {
> + dev_err(dev, "failed to register DSI host: %d\n", ret);
> + return ret;
> + }
>
Since mipi_dsi_host_register() is moved from .bind to .probe,
mipi_dsi_host_unregister() should also be moved from .unbind to
.remove?
Thanks
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [v3 6/7] drm/mediatek: change the dsi phytiming calculate method
[not found] ` <20190519092537.69053-7-jitao.shi@mediatek.com>
2019-05-19 19:29 ` [v3 6/7] drm/mediatek: change the dsi phytiming calculate method kbuild test robot
@ 2019-05-30 20:45 ` Ryan Case
2019-06-03 18:14 ` Guenter Roeck
2 siblings, 0 replies; 7+ messages in thread
From: Ryan Case @ 2019-05-30 20:45 UTC (permalink / raw)
To: Jitao Shi
Cc: Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
linux-pwm, David Airlie, Matthias Brugger, stonea168, dri-devel,
Andy Yan, Ajay Kumar, Vincent Palatin, cawa.cheng, bibby.hsieh,
ck.hu, Russell King, Thierry Reding, devicetree, Philipp Zabel,
Inki Dae, linux-mediatek, yingjoe.chen, eddie.huang,
linux-arm-kernel, Rahul Sharma, srv_heupstream, linux-kernel,
Sascha Hauer, Sean Paul
Hi Jitao,
On Sun, May 19, 2019 at 2:27 AM Jitao Shi <jitao.shi@mediatek.com> wrote:
>
> Change the method of frame rate calc which can get more accurate
> frame rate.
>
> data rate = pixel_clock * bit_per_pixel / lanes
> Adjust hfp_wc to adapt the additional phy_data
>
> if MIPI_DSI_MODE_VIDEO_BURST
> hfp_wc = hfp * bpp - data_phy_cycles * lanes - 12 - 6;
> else
> hfp_wc = hfp * bpp - data_phy_cycles * lanes - 12;
>
> Note:
> //(2: 1 for sync, 1 for phy idle)
> data_phy_cycles = T_hs_exit + T_lpx + T_hs_prepare + T_hs_zero + 2;
>
> bpp: bit per pixel
>
> Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
> ---
> drivers/gpu/drm/mediatek/mtk_dsi.c | 119 +++++++++++++++++++++--------
> 1 file changed, 86 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
> index 1165ff944889..3f51b2000c68 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> @@ -158,6 +158,25 @@
> (type == MIPI_DSI_GENERIC_READ_REQUEST_2_PARAM) || \
> (type == MIPI_DSI_DCS_READ))
>
> +struct mtk_phy_timing {
> + u32 lpx;
> + u32 da_hs_prepare;
> + u32 da_hs_zero;
> + u32 da_hs_trail;
> +
> + u32 ta_go;
> + u32 ta_sure;
> + u32 ta_get;
> + u32 da_hs_exit;
> +
> + u32 clk_hs_zero;
> + u32 clk_hs_trail;
> +
> + u32 clk_hs_prepare;
> + u32 clk_hs_post;
> + u32 clk_hs_exit;
> +};
> +
> struct phy;
>
> struct mtk_dsi_driver_data {
> @@ -182,12 +201,13 @@ struct mtk_dsi {
> struct clk *digital_clk;
> struct clk *hs_clk;
>
> - u32 data_rate;
> + u64 data_rate;
>
> unsigned long mode_flags;
> enum mipi_dsi_pixel_format format;
> unsigned int lanes;
> struct videomode vm;
> + struct mtk_phy_timing phy_timing;
> int refcount;
> bool enabled;
> u32 irq_data;
> @@ -221,17 +241,39 @@ static void mtk_dsi_phy_timconfig(struct mtk_dsi *dsi)
> {
> u32 timcon0, timcon1, timcon2, timcon3;
> u32 ui, cycle_time;
> + struct mtk_phy_timing *timing = &dsi->phy_timing;
> +
> + ui = 1000000000 / dsi->data_rate;
> + cycle_time = 8000000000 / dsi->data_rate;
> +
> + timing->lpx = NS_TO_CYCLE(60, cycle_time);
> + timing->da_hs_prepare = NS_TO_CYCLE((40 + 5 * ui), cycle_time);
> + timing->da_hs_zero = NS_TO_CYCLE((110 + 6 * ui), cycle_time);
> + timing->da_hs_trail = NS_TO_CYCLE(((0x4 * ui) + 80), cycle_time);
> +
> + if (timing->da_hs_zero > timing->da_hs_prepare)
> + timing->da_hs_zero -= timing->da_hs_prepare;
I don't follow why the above comparison and subtraction is necessary
when the values are being explicitly set immediately prior and it
seems to introduce a bug. Leftover from an early revision?
It looks like you've tuned the values such that hs_prepare+hs_zero are
just above the minimum requirements for that sum, however due to this
comparison and subtraction we wind up with a value of
hs_prepare+hs_zero-hs_prepare and fall below spec. Either boosting the
initial value set for hs_zero or removing the comparison makes display
happy again. Since I don't see any reason for the compare and subtract
I'd just drop that.
> +
> + timing->ta_go = 4 * timing->lpx;
> + timing->ta_sure = 3 * timing->lpx / 2;
> + timing->ta_get = 5 * timing->lpx;
> + timing->da_hs_exit = 2 * timing->lpx;
> +
> + timing->clk_hs_zero = NS_TO_CYCLE(0x150, cycle_time);
> + timing->clk_hs_trail = NS_TO_CYCLE(0x64, cycle_time) + 0xa;
>
> - ui = 1000 / dsi->data_rate + 0x01;
> - cycle_time = 8000 / dsi->data_rate + 0x01;
> + timing->clk_hs_prepare = NS_TO_CYCLE(0x40, cycle_time);
> + timing->clk_hs_post = NS_TO_CYCLE(80 + 52 * ui, cycle_time);
> + timing->clk_hs_exit = 2 * timing->lpx;
There is a lot of alternating between hex and decimal values in this
function which makes it a little hard to follow. Would be nice to
stick to one or the other.
>
> - timcon0 = T_LPX | T_HS_PREP << 8 | T_HS_ZERO << 16 | T_HS_TRAIL << 24;
> - timcon1 = 4 * T_LPX | (3 * T_LPX / 2) << 8 | 5 * T_LPX << 16 |
> - T_HS_EXIT << 24;
> - timcon2 = ((NS_TO_CYCLE(0x64, cycle_time) + 0xa) << 24) |
> - (NS_TO_CYCLE(0x150, cycle_time) << 16);
> - timcon3 = NS_TO_CYCLE(0x40, cycle_time) | (2 * T_LPX) << 16 |
> - NS_TO_CYCLE(80 + 52 * ui, cycle_time) << 8;
> + timcon0 = timing->lpx | timing->da_hs_prepare << 8 |
> + timing->da_hs_zero << 16 | timing->da_hs_trail << 24;
> + timcon1 = timing->ta_go | timing->ta_sure << 8 |
> + timing->ta_get << 16 | timing->da_hs_exit << 24;
> + timcon2 = 1 << 8 | timing->clk_hs_zero << 16 |
> + timing->clk_hs_trail << 24;
> + timcon3 = timing->clk_hs_prepare | timing->clk_hs_post << 8 |
> + timing->clk_hs_exit << 16;
>
> writel(timcon0, dsi->regs + DSI_PHY_TIMECON0);
> writel(timcon1, dsi->regs + DSI_PHY_TIMECON1);
> @@ -418,7 +460,8 @@ static void mtk_dsi_config_vdo_timing(struct mtk_dsi *dsi)
> u32 horizontal_sync_active_byte;
> u32 horizontal_backporch_byte;
> u32 horizontal_frontporch_byte;
> - u32 dsi_tmp_buf_bpp;
> + u32 dsi_tmp_buf_bpp, data_phy_cycles;
> + struct mtk_phy_timing *timing = &dsi->phy_timing;
>
> struct videomode *vm = &dsi->vm;
>
> @@ -433,7 +476,8 @@ static void mtk_dsi_config_vdo_timing(struct mtk_dsi *dsi)
> writel(vm->vactive, dsi->regs + DSI_VACT_NL);
>
> if (dsi->driver_data->has_size_ctl)
> - writel(vm->vactive << 16 | vm->hactive, dsi->regs + DSI_SIZE_CON);
> + writel(vm->vactive << 16 | vm->hactive,
> + dsi->regs + DSI_SIZE_CON);
>
> horizontal_sync_active_byte = (vm->hsync_len * dsi_tmp_buf_bpp - 10);
>
> @@ -444,7 +488,34 @@ static void mtk_dsi_config_vdo_timing(struct mtk_dsi *dsi)
> horizontal_backporch_byte = ((vm->hback_porch + vm->hsync_len) *
> dsi_tmp_buf_bpp - 10);
>
> - horizontal_frontporch_byte = (vm->hfront_porch * dsi_tmp_buf_bpp - 12);
> + data_phy_cycles = timing->lpx + timing->da_hs_prepare +
> + timing->da_hs_zero + timing->da_hs_exit + 2;
> +
> + if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_BURST) {
> + if (vm->hfront_porch * dsi_tmp_buf_bpp >
> + data_phy_cycles * dsi->lanes + 18) {
> + horizontal_frontporch_byte = vm->hfront_porch *
> + dsi_tmp_buf_bpp -
> + data_phy_cycles *
> + dsi->lanes - 18;
> + } else {
> + DRM_WARN("HFP less than d-phy, FPS will under 60Hz\n");
> + horizontal_frontporch_byte = vm->hfront_porch *
> + dsi_tmp_buf_bpp;
> + }
> + } else {
> + if (vm->hfront_porch * dsi_tmp_buf_bpp >
> + data_phy_cycles * dsi->lanes + 12) {
> + horizontal_frontporch_byte = vm->hfront_porch *
> + dsi_tmp_buf_bpp -
> + data_phy_cycles *
> + dsi->lanes - 12;
> + } else {
> + DRM_WARN("HFP less than d-phy, FPS will under 60Hz\n");
> + horizontal_frontporch_byte = vm->hfront_porch *
> + dsi_tmp_buf_bpp;
> + }
> + }
>
> writel(horizontal_sync_active_byte, dsi->regs + DSI_HSA_WC);
> writel(horizontal_backporch_byte, dsi->regs + DSI_HBP_WC);
> @@ -544,8 +615,7 @@ static int mtk_dsi_poweron(struct mtk_dsi *dsi)
> {
> struct device *dev = dsi->dev;
> int ret;
> - u64 pixel_clock, total_bits;
> - u32 htotal, htotal_bits, bit_per_pixel, overhead_cycles, overhead_bits;
> + u32 bit_per_pixel;
>
> if (++dsi->refcount != 1)
> return 0;
> @@ -564,24 +634,7 @@ static int mtk_dsi_poweron(struct mtk_dsi *dsi)
> break;
> }
>
> - /**
> - * htotal_time = htotal * byte_per_pixel / num_lanes
> - * overhead_time = lpx + hs_prepare + hs_zero + hs_trail + hs_exit
> - * mipi_ratio = (htotal_time + overhead_time) / htotal_time
> - * data_rate = pixel_clock * bit_per_pixel * mipi_ratio / num_lanes;
> - */
> - pixel_clock = dsi->vm.pixelclock;
> - htotal = dsi->vm.hactive + dsi->vm.hback_porch + dsi->vm.hfront_porch +
> - dsi->vm.hsync_len;
> - htotal_bits = htotal * bit_per_pixel;
> -
> - overhead_cycles = T_LPX + T_HS_PREP + T_HS_ZERO + T_HS_TRAIL +
> - T_HS_EXIT;
> - overhead_bits = overhead_cycles * dsi->lanes * 8;
> - total_bits = htotal_bits + overhead_bits;
> -
> - dsi->data_rate = DIV_ROUND_UP_ULL(pixel_clock * total_bits,
> - htotal * dsi->lanes);
> + dsi->data_rate = dsi->vm.pixelclock * bit_per_pixel / dsi->lanes;
>
> ret = clk_set_rate(dsi->hs_clk, dsi->data_rate);
> if (ret < 0) {
With the earlier fix feel free to add to the next revision
Tested-by: Ryan Case <ryandcase@chromium.org>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [v3 3/7] drm/mediatek: add dsi reg commit disable control
[not found] ` <20190519092537.69053-4-jitao.shi@mediatek.com>
@ 2019-05-31 3:38 ` CK Hu
0 siblings, 0 replies; 7+ messages in thread
From: CK Hu @ 2019-05-31 3:38 UTC (permalink / raw)
To: Jitao Shi
Cc: Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
linux-pwm, David Airlie, Matthias Brugger, Thierry Reding,
Ajay Kumar, Inki Dae, Rahul Sharma, Sean Paul, Vincent Palatin,
Andy Yan, Philipp Zabel, Russell King, devicetree, linux-kernel,
dri-devel, linux-arm-kernel, linux-mediatek, srv_heupstream,
Sascha Hauer, yingjoe.chen, eddie.huang, cawa.cheng, bibby.hsieh,
stonea168
Hi, Jitao:
On Sun, 2019-05-19 at 17:25 +0800, Jitao Shi wrote:
> New DSI IP has shadow register and working reg. The register
> values are writen to shadow register. And then trigger with
> commit reg, the register values will be moved working register.
>
> This fucntion is defualt on. But this driver doesn't use this
> function. So add the disable control.
>
> Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
> ---
> drivers/gpu/drm/mediatek/mtk_dsi.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
> index a48db056df6c..fd367985c7fd 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> @@ -131,6 +131,10 @@
> #define VM_CMD_EN BIT(0)
> #define TS_VFP_EN BIT(5)
>
> +#define DSI_SHADOW_DEBUG 0x190U
> +#define FORCE_COMMIT BIT(0)
> +#define BYPASS_SHADOW BIT(1)
One more 'tab' for bitwise definition.
Regards,
CK
> +
> #define CONFIG (0xff << 0)
> #define SHORT_PACKET 0
> #define LONG_PACKET 2
> @@ -157,6 +161,7 @@ struct phy;
>
> struct mtk_dsi_driver_data {
> const u32 reg_cmdq_off;
> + bool has_shadow_ctl;
> };
>
> struct mtk_dsi {
> @@ -594,6 +599,11 @@ static int mtk_dsi_poweron(struct mtk_dsi *dsi)
> }
>
> mtk_dsi_enable(dsi);
> +
> + if (dsi->driver_data->has_shadow_ctl)
> + writel(FORCE_COMMIT | BYPASS_SHADOW,
> + dsi->regs + DSI_SHADOW_DEBUG);
> +
> mtk_dsi_reset_engine(dsi);
> mtk_dsi_phy_timconfig(dsi);
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [v3 7/7] drm: mediatek: adjust dsi and mipi_tx probe sequence
[not found] ` <20190519092537.69053-8-jitao.shi@mediatek.com>
@ 2019-05-31 3:40 ` CK Hu
0 siblings, 0 replies; 7+ messages in thread
From: CK Hu @ 2019-05-31 3:40 UTC (permalink / raw)
To: Jitao Shi
Cc: Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
linux-pwm, David Airlie, Matthias Brugger, Thierry Reding,
Ajay Kumar, Inki Dae, Rahul Sharma, Sean Paul, Vincent Palatin,
Andy Yan, Philipp Zabel, Russell King, devicetree, linux-kernel,
dri-devel, linux-arm-kernel, linux-mediatek, srv_heupstream,
Sascha Hauer, yingjoe.chen, eddie.huang, cawa.cheng, bibby.hsieh,
stonea168
Hi, Jitao:
On Sun, 2019-05-19 at 17:25 +0800, Jitao Shi wrote:
> mtk_mipi_tx is the phy of mtk_dsi.
> mtk_dsi get the phy(mtk_mipi_tx) in probe().
>
> So, mtk_mipi_tx init should be ahead of mtk_dsi. Or mtk_dsi will
> defer to wait mtk_mipi_tx probe done.
Reviewed-by: CK Hu <ck.hu@mediatek.com>
>
> Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
> ---
> drivers/gpu/drm/mediatek/mtk_drm_drv.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> index cf59ea9bccfd..583d533d9574 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> @@ -633,8 +633,8 @@ static struct platform_driver * const mtk_drm_drivers[] = {
> &mtk_disp_rdma_driver,
> &mtk_dpi_driver,
> &mtk_drm_platform_driver,
> - &mtk_dsi_driver,
> &mtk_mipi_tx_driver,
> + &mtk_dsi_driver,
> };
>
> static int __init mtk_drm_init(void)
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [v3 6/7] drm/mediatek: change the dsi phytiming calculate method
[not found] ` <20190519092537.69053-7-jitao.shi@mediatek.com>
2019-05-19 19:29 ` [v3 6/7] drm/mediatek: change the dsi phytiming calculate method kbuild test robot
2019-05-30 20:45 ` Ryan Case
@ 2019-06-03 18:14 ` Guenter Roeck
2 siblings, 0 replies; 7+ messages in thread
From: Guenter Roeck @ 2019-06-03 18:14 UTC (permalink / raw)
To: Jitao Shi
Cc: Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
linux-pwm, David Airlie, Matthias Brugger, stonea168, dri-devel,
Andy Yan, Ajay Kumar, Vincent Palatin, cawa.cheng, bibby.hsieh,
ck.hu, Russell King, Thierry Reding, devicetree, Philipp Zabel,
Inki Dae, linux-mediatek, yingjoe.chen, eddie.huang,
linux-arm-kernel, Rahul Sharma, srv_heupstream, linux-kernel,
Sascha Hauer, Sean Paul
On Sun, May 19, 2019 at 05:25:36PM +0800, Jitao Shi wrote:
> Change the method of frame rate calc which can get more accurate
> frame rate.
>
> data rate = pixel_clock * bit_per_pixel / lanes
> Adjust hfp_wc to adapt the additional phy_data
>
> if MIPI_DSI_MODE_VIDEO_BURST
> hfp_wc = hfp * bpp - data_phy_cycles * lanes - 12 - 6;
> else
> hfp_wc = hfp * bpp - data_phy_cycles * lanes - 12;
>
> Note:
> //(2: 1 for sync, 1 for phy idle)
> data_phy_cycles = T_hs_exit + T_lpx + T_hs_prepare + T_hs_zero + 2;
>
> bpp: bit per pixel
>
> Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
> Tested-by: Ryan Case <ryandcase@chromium.org>
> ---
> drivers/gpu/drm/mediatek/mtk_dsi.c | 119 +++++++++++++++++++++--------
> 1 file changed, 86 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
> index 1165ff944889..3f51b2000c68 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> @@ -158,6 +158,25 @@
> (type == MIPI_DSI_GENERIC_READ_REQUEST_2_PARAM) || \
> (type == MIPI_DSI_DCS_READ))
>
> +struct mtk_phy_timing {
> + u32 lpx;
> + u32 da_hs_prepare;
> + u32 da_hs_zero;
> + u32 da_hs_trail;
> +
> + u32 ta_go;
> + u32 ta_sure;
> + u32 ta_get;
> + u32 da_hs_exit;
> +
> + u32 clk_hs_zero;
> + u32 clk_hs_trail;
> +
> + u32 clk_hs_prepare;
> + u32 clk_hs_post;
> + u32 clk_hs_exit;
> +};
> +
> struct phy;
>
> struct mtk_dsi_driver_data {
> @@ -182,12 +201,13 @@ struct mtk_dsi {
> struct clk *digital_clk;
> struct clk *hs_clk;
>
> - u32 data_rate;
> + u64 data_rate;
This results in 64-bit divide operations and thus build failures
with 32-bit builds. More on that below.
>
> unsigned long mode_flags;
> enum mipi_dsi_pixel_format format;
> unsigned int lanes;
> struct videomode vm;
> + struct mtk_phy_timing phy_timing;
> int refcount;
> bool enabled;
> u32 irq_data;
> @@ -221,17 +241,39 @@ static void mtk_dsi_phy_timconfig(struct mtk_dsi *dsi)
> {
> u32 timcon0, timcon1, timcon2, timcon3;
> u32 ui, cycle_time;
> + struct mtk_phy_timing *timing = &dsi->phy_timing;
> +
> + ui = 1000000000 / dsi->data_rate;
> + cycle_time = 8000000000 / dsi->data_rate;
This results in 64-bit divide operations. On top of that, 8000000000
is larger than 0xffffffff, resulting in an integer overflow on 32-bit
systems; it should be provided as 8000000000ULL.
> +
> + timing->lpx = NS_TO_CYCLE(60, cycle_time);
> + timing->da_hs_prepare = NS_TO_CYCLE((40 + 5 * ui), cycle_time);
> + timing->da_hs_zero = NS_TO_CYCLE((110 + 6 * ui), cycle_time);
> + timing->da_hs_trail = NS_TO_CYCLE(((0x4 * ui) + 80), cycle_time);
> +
> + if (timing->da_hs_zero > timing->da_hs_prepare)
> + timing->da_hs_zero -= timing->da_hs_prepare;
> +
> + timing->ta_go = 4 * timing->lpx;
> + timing->ta_sure = 3 * timing->lpx / 2;
> + timing->ta_get = 5 * timing->lpx;
> + timing->da_hs_exit = 2 * timing->lpx;
> +
> + timing->clk_hs_zero = NS_TO_CYCLE(0x150, cycle_time);
> + timing->clk_hs_trail = NS_TO_CYCLE(0x64, cycle_time) + 0xa;
>
> - ui = 1000 / dsi->data_rate + 0x01;
> - cycle_time = 8000 / dsi->data_rate + 0x01;
> + timing->clk_hs_prepare = NS_TO_CYCLE(0x40, cycle_time);
> + timing->clk_hs_post = NS_TO_CYCLE(80 + 52 * ui, cycle_time);
> + timing->clk_hs_exit = 2 * timing->lpx;
>
> - timcon0 = T_LPX | T_HS_PREP << 8 | T_HS_ZERO << 16 | T_HS_TRAIL << 24;
> - timcon1 = 4 * T_LPX | (3 * T_LPX / 2) << 8 | 5 * T_LPX << 16 |
> - T_HS_EXIT << 24;
> - timcon2 = ((NS_TO_CYCLE(0x64, cycle_time) + 0xa) << 24) |
> - (NS_TO_CYCLE(0x150, cycle_time) << 16);
> - timcon3 = NS_TO_CYCLE(0x40, cycle_time) | (2 * T_LPX) << 16 |
> - NS_TO_CYCLE(80 + 52 * ui, cycle_time) << 8;
> + timcon0 = timing->lpx | timing->da_hs_prepare << 8 |
> + timing->da_hs_zero << 16 | timing->da_hs_trail << 24;
> + timcon1 = timing->ta_go | timing->ta_sure << 8 |
> + timing->ta_get << 16 | timing->da_hs_exit << 24;
> + timcon2 = 1 << 8 | timing->clk_hs_zero << 16 |
> + timing->clk_hs_trail << 24;
> + timcon3 = timing->clk_hs_prepare | timing->clk_hs_post << 8 |
> + timing->clk_hs_exit << 16;
>
> writel(timcon0, dsi->regs + DSI_PHY_TIMECON0);
> writel(timcon1, dsi->regs + DSI_PHY_TIMECON1);
> @@ -418,7 +460,8 @@ static void mtk_dsi_config_vdo_timing(struct mtk_dsi *dsi)
> u32 horizontal_sync_active_byte;
> u32 horizontal_backporch_byte;
> u32 horizontal_frontporch_byte;
> - u32 dsi_tmp_buf_bpp;
> + u32 dsi_tmp_buf_bpp, data_phy_cycles;
> + struct mtk_phy_timing *timing = &dsi->phy_timing;
>
> struct videomode *vm = &dsi->vm;
>
> @@ -433,7 +476,8 @@ static void mtk_dsi_config_vdo_timing(struct mtk_dsi *dsi)
> writel(vm->vactive, dsi->regs + DSI_VACT_NL);
>
> if (dsi->driver_data->has_size_ctl)
> - writel(vm->vactive << 16 | vm->hactive, dsi->regs + DSI_SIZE_CON);
> + writel(vm->vactive << 16 | vm->hactive,
> + dsi->regs + DSI_SIZE_CON);
>
> horizontal_sync_active_byte = (vm->hsync_len * dsi_tmp_buf_bpp - 10);
>
> @@ -444,7 +488,34 @@ static void mtk_dsi_config_vdo_timing(struct mtk_dsi *dsi)
> horizontal_backporch_byte = ((vm->hback_porch + vm->hsync_len) *
> dsi_tmp_buf_bpp - 10);
>
> - horizontal_frontporch_byte = (vm->hfront_porch * dsi_tmp_buf_bpp - 12);
> + data_phy_cycles = timing->lpx + timing->da_hs_prepare +
> + timing->da_hs_zero + timing->da_hs_exit + 2;
> +
> + if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_BURST) {
> + if (vm->hfront_porch * dsi_tmp_buf_bpp >
> + data_phy_cycles * dsi->lanes + 18) {
> + horizontal_frontporch_byte = vm->hfront_porch *
> + dsi_tmp_buf_bpp -
> + data_phy_cycles *
> + dsi->lanes - 18;
> + } else {
> + DRM_WARN("HFP less than d-phy, FPS will under 60Hz\n");
> + horizontal_frontporch_byte = vm->hfront_porch *
> + dsi_tmp_buf_bpp;
> + }
> + } else {
> + if (vm->hfront_porch * dsi_tmp_buf_bpp >
> + data_phy_cycles * dsi->lanes + 12) {
> + horizontal_frontporch_byte = vm->hfront_porch *
> + dsi_tmp_buf_bpp -
> + data_phy_cycles *
> + dsi->lanes - 12;
> + } else {
> + DRM_WARN("HFP less than d-phy, FPS will under 60Hz\n");
> + horizontal_frontporch_byte = vm->hfront_porch *
> + dsi_tmp_buf_bpp;
> + }
> + }
>
> writel(horizontal_sync_active_byte, dsi->regs + DSI_HSA_WC);
> writel(horizontal_backporch_byte, dsi->regs + DSI_HBP_WC);
> @@ -544,8 +615,7 @@ static int mtk_dsi_poweron(struct mtk_dsi *dsi)
> {
> struct device *dev = dsi->dev;
> int ret;
> - u64 pixel_clock, total_bits;
> - u32 htotal, htotal_bits, bit_per_pixel, overhead_cycles, overhead_bits;
> + u32 bit_per_pixel;
>
> if (++dsi->refcount != 1)
> return 0;
> @@ -564,24 +634,7 @@ static int mtk_dsi_poweron(struct mtk_dsi *dsi)
> break;
> }
>
> - /**
> - * htotal_time = htotal * byte_per_pixel / num_lanes
> - * overhead_time = lpx + hs_prepare + hs_zero + hs_trail + hs_exit
> - * mipi_ratio = (htotal_time + overhead_time) / htotal_time
> - * data_rate = pixel_clock * bit_per_pixel * mipi_ratio / num_lanes;
> - */
> - pixel_clock = dsi->vm.pixelclock;
> - htotal = dsi->vm.hactive + dsi->vm.hback_porch + dsi->vm.hfront_porch +
> - dsi->vm.hsync_len;
> - htotal_bits = htotal * bit_per_pixel;
> -
> - overhead_cycles = T_LPX + T_HS_PREP + T_HS_ZERO + T_HS_TRAIL +
> - T_HS_EXIT;
> - overhead_bits = overhead_cycles * dsi->lanes * 8;
> - total_bits = htotal_bits + overhead_bits;
> -
> - dsi->data_rate = DIV_ROUND_UP_ULL(pixel_clock * total_bits,
> - htotal * dsi->lanes);
> + dsi->data_rate = dsi->vm.pixelclock * bit_per_pixel / dsi->lanes;
pixelclock and bit_per_pixel are not u64, and neither is dsi->lanes.
The above will thus be a 32-bit operation on 32-bit systems, and never
really assign a true 64-bit value to data_rate. On top of that,
clk_set_rate() expects an unsigned long argument. Declaring data_rate
as anything but unsigned long has therefore no value.
Note that the old code took possible overflow conditions due to interim
results larger than u32 / unsigned long into account. This is no longer
the case. I don't know if this is a concern, but it might be worth watching
out for. If it is not a concern because the code is not anymore expected
to run on 32-bit systems, it should be restricted to 64-bit builds.
Guenter
>
> ret = clk_set_rate(dsi->hs_clk, dsi->data_rate);
> if (ret < 0) {
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-06-03 18:14 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20190519092537.69053-1-jitao.shi@mediatek.com>
[not found] ` <20190519092537.69053-7-jitao.shi@mediatek.com>
2019-05-19 19:29 ` [v3 6/7] drm/mediatek: change the dsi phytiming calculate method kbuild test robot
2019-05-30 20:45 ` Ryan Case
2019-06-03 18:14 ` Guenter Roeck
[not found] ` <20190519092537.69053-2-jitao.shi@mediatek.com>
2019-05-20 7:18 ` [v3 1/7] drm/mediatek: move mipi_dsi_host_register to probe CK Hu
2019-05-29 4:19 ` Hsin-Yi Wang
[not found] ` <20190519092537.69053-4-jitao.shi@mediatek.com>
2019-05-31 3:38 ` [v3 3/7] drm/mediatek: add dsi reg commit disable control CK Hu
[not found] ` <20190519092537.69053-8-jitao.shi@mediatek.com>
2019-05-31 3:40 ` [v3 7/7] drm: mediatek: adjust dsi and mipi_tx probe sequence CK Hu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).