This patch correct Feedback divider setting: 1、Set Feedback divider [8:5] when HIGH_PROGRAM_EN 2、Due to the use of a "by 2 pre-scaler," the range of the feedback multiplication Feedback divider is limited to even division numbers, and Feedback divider must be greater than 12, less than 1000. 3、Make the previously configured Feedback divider(LSB) factors effective
Signed-off-by: Nickey Yang nickey.yang@rock-chips.com --- drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 83 ++++++++++++++++++++++------------ 1 file changed, 54 insertions(+), 29 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c index 9a20b9d..52698b7 100644 --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c @@ -228,7 +228,7 @@ #define LOW_PROGRAM_EN 0 #define HIGH_PROGRAM_EN BIT(7) #define LOOP_DIV_LOW_SEL(val) (((val) - 1) & 0x1f) -#define LOOP_DIV_HIGH_SEL(val) ((((val) - 1) >> 5) & 0x1f) +#define LOOP_DIV_HIGH_SEL(val) ((((val) - 1) >> 5) & 0xf) #define PLL_LOOP_DIV_EN BIT(5) #define PLL_INPUT_DIV_EN BIT(4)
@@ -461,6 +461,7 @@ static int dw_mipi_dsi_phy_init(struct dw_mipi_dsi *dsi) dw_mipi_dsi_phy_write(dsi, 0x17, INPUT_DIVIDER(dsi->input_div)); dw_mipi_dsi_phy_write(dsi, 0x18, LOOP_DIV_LOW_SEL(dsi->feedback_div) | LOW_PROGRAM_EN); + dw_mipi_dsi_phy_write(dsi, 0x19, PLL_LOOP_DIV_EN | PLL_INPUT_DIV_EN); dw_mipi_dsi_phy_write(dsi, 0x18, LOOP_DIV_HIGH_SEL(dsi->feedback_div) | HIGH_PROGRAM_EN); dw_mipi_dsi_phy_write(dsi, 0x19, PLL_LOOP_DIV_EN | PLL_INPUT_DIV_EN); @@ -521,11 +522,16 @@ static int dw_mipi_dsi_phy_init(struct dw_mipi_dsi *dsi) static int dw_mipi_dsi_get_lane_bps(struct dw_mipi_dsi *dsi, struct drm_display_mode *mode) { - unsigned int i, pre; - unsigned long mpclk, pllref, tmp; - unsigned int m = 1, n = 1, target_mbps = 1000; + unsigned long mpclk, tmp; + unsigned int target_mbps = 1000; unsigned int max_mbps = dptdin_map[ARRAY_SIZE(dptdin_map) - 1].max_mbps; int bpp; + unsigned long best_freq = 0; + unsigned long fvco_min, fvco_max, fin ,fout; + unsigned int min_prediv, max_prediv; + unsigned int _prediv, uninitialized_var(best_prediv); + unsigned long _fbdiv, uninitialized_var(best_fbdiv); + unsigned long min_delta = UINT_MAX;
bpp = mipi_dsi_pixel_format_to_bpp(dsi->format); if (bpp < 0) { @@ -544,34 +550,53 @@ static int dw_mipi_dsi_get_lane_bps(struct dw_mipi_dsi *dsi, dev_err(dsi->dev, "DPHY clock frequency is out of range\n"); }
- pllref = DIV_ROUND_UP(clk_get_rate(dsi->pllref_clk), USEC_PER_SEC); - tmp = pllref; - - /* - * The limits on the PLL divisor are: - * - * 5MHz <= (pllref / n) <= 40MHz - * - * we walk over these values in descreasing order so that if we hit - * an exact match for target_mbps it is more likely that "m" will be - * even. - * - * TODO: ensure that "m" is even after this loop. - */ - for (i = pllref / 5; i > (pllref / 40); i--) { - pre = pllref / i; - if ((tmp > (target_mbps % pre)) && (target_mbps / pre < 512)) { - tmp = target_mbps % pre; - n = i; - m = target_mbps / pre; + fin = clk_get_rate(dsi->pllref_clk); + fout = target_mbps * USEC_PER_SEC; + + /* constraint: 5Mhz < Fref / N < 40MHz */ + min_prediv = DIV_ROUND_UP(fin, 40 * USEC_PER_SEC); + max_prediv = fin / (5 * USEC_PER_SEC); + + /* constraint: 80MHz < Fvco < 1500Mhz */ + fvco_min = 80 * USEC_PER_SEC; + fvco_max = 1500 * USEC_PER_SEC; + + for (_prediv = min_prediv; _prediv <= max_prediv; _prediv++) { + u32 delta; + /* Fvco = Fref * M / N */ + tmp = fout * _prediv; + do_div(tmp, fin); + _fbdiv = tmp; + /* + * Due to the use of a "by 2 pre-scaler," the range of the + * feedback multiplication value M is limited to even division + * numbers, and m must be greater than 12, less than 1000. + */ + if (_fbdiv < 12 || _fbdiv > 1000) + continue; + + if (_fbdiv % 2) + ++_fbdiv; + + tmp = (u64)_fbdiv * fin; + do_div(tmp, _prediv); + if (tmp < fvco_min || tmp > fvco_max) + continue; + + delta = abs(fout - tmp); + if (delta < min_delta) { + best_prediv = _prediv; + best_fbdiv = _fbdiv; + min_delta = delta; + best_freq = tmp; } - if (tmp == 0) - break; }
- dsi->lane_mbps = pllref / n * m; - dsi->input_div = n; - dsi->feedback_div = m; + if (best_freq) { + dsi->lane_mbps = DIV_ROUND_UP(best_freq, USEC_PER_SEC); + dsi->input_div = best_prediv; + dsi->feedback_div = best_fbdiv; + }
return 0; }
Signed-off-by: Nickey Yang nickey.yang@rock-chips.com --- drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 353 ++++++++++++++++++++-------- drivers/gpu/drm/rockchip/rockchip_drm_drv.h | 1 + drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 2 + drivers/gpu/drm/rockchip/rockchip_drm_vop.h | 3 + 4 files changed, 257 insertions(+), 102 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c index 52698b7..9265b7f 100644 --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c @@ -42,6 +42,17 @@ #define RK3399_GRF_SOC_CON22 0x6258 #define RK3399_GRF_DSI_MODE 0xffff0000
+/* disable turndisable, forcetxstopmode, forcerxmode, enable */ +#define RK3399_GRF_SOC_CON23 0x625c +#define RK3399_GRF_DSI1_MODE1 0xffff0000 +#define RK3399_GRF_DSI1_ENABLE 0x000f000f +/* disable basedir and enable clk*/ +#define RK3399_GRF_SOC_CON24 0x6260 +#define RK3399_TXRX_MASTERSLAVEZ BIT(7) +#define RK3399_TXRX_ENABLECLK BIT(6) +#define RK3399_TXRX_BASEDIR BIT(5) +#define RK3399_GRF_DSI1_MODE2 0x00e00080 + #define DSI_VERSION 0x00 #define DSI_PWR_UP 0x04 #define RESET 0 @@ -282,6 +293,12 @@ struct dw_mipi_dsi_plat_data { u32 grf_switch_reg; u32 grf_dsi0_mode; u32 grf_dsi0_mode_reg; + u32 grf_dsi1_mode; + u32 grf_dsi1_mode_reg1; + u32 dsi1_basedir; + u32 dsi1_masterslavez; + u32 dsi1_enableclk; + u32 grf_dsi1_mode_reg2; unsigned int flags; unsigned int max_data_lanes; }; @@ -300,6 +317,12 @@ struct dw_mipi_dsi { struct clk *pclk; struct clk *phy_cfg_clk;
+ /* dual-channel */ + struct dw_mipi_dsi *master; + struct dw_mipi_dsi *slave; + struct device_node *panel_node; + int id; + int dpms_mode; unsigned int lane_mbps; /* per lane */ u32 channel; @@ -526,6 +549,7 @@ static int dw_mipi_dsi_get_lane_bps(struct dw_mipi_dsi *dsi, unsigned int target_mbps = 1000; unsigned int max_mbps = dptdin_map[ARRAY_SIZE(dptdin_map) - 1].max_mbps; int bpp; + int lanes = dsi->lanes; unsigned long best_freq = 0; unsigned long fvco_min, fvco_max, fin ,fout; unsigned int min_prediv, max_prediv; @@ -540,10 +564,13 @@ static int dw_mipi_dsi_get_lane_bps(struct dw_mipi_dsi *dsi, return bpp; }
+ if (dsi->slave || dsi->master) + lanes = dsi->lanes * 2; + mpclk = DIV_ROUND_UP(mode->clock, MSEC_PER_SEC); if (mpclk) { /* take 1 / 0.8, since mbps must big than bandwidth of RGB */ - tmp = mpclk * (bpp / dsi->lanes) * 10 / 8; + tmp = mpclk * (bpp / lanes) * 10 / 8; if (tmp < max_mbps) target_mbps = tmp; else @@ -605,17 +632,26 @@ static int dw_mipi_dsi_host_attach(struct mipi_dsi_host *host, struct mipi_dsi_device *device) { struct dw_mipi_dsi *dsi = host_to_dsi(host); + int lanes = dsi->slave ? device->lanes / 2 : device->lanes;
- if (device->lanes > dsi->pdata->max_data_lanes) { + if (lanes > dsi->pdata->max_data_lanes) { dev_err(dsi->dev, "the number of data lanes(%u) is too many\n", - device->lanes); + lanes); return -EINVAL; }
- dsi->lanes = device->lanes; + dsi->lanes = lanes; dsi->channel = device->channel; dsi->format = device->format; dsi->mode_flags = device->mode_flags; + + if (dsi->slave) { + dsi->slave->lanes = lanes; + dsi->slave->channel = device->channel; + dsi->slave->format = device->format; + dsi->slave->mode_flags = device->mode_flags; + } + dsi->panel = of_drm_find_panel(device->dev.of_node); if (dsi->panel) return drm_panel_attach(dsi->panel, &dsi->connector); @@ -745,15 +781,22 @@ static ssize_t dw_mipi_dsi_host_transfer(struct mipi_dsi_host *host, int ret;
dw_mipi_message_config(dsi, msg); + if (dsi->slave) + dw_mipi_message_config(dsi->slave, msg);
switch (msg->type) { case MIPI_DSI_DCS_SHORT_WRITE: case MIPI_DSI_DCS_SHORT_WRITE_PARAM: case MIPI_DSI_SET_MAXIMUM_RETURN_PACKET_SIZE: + case MIPI_DSI_GENERIC_SHORT_WRITE_2_PARAM: ret = dw_mipi_dsi_dcs_short_write(dsi, msg); + if (dsi->slave) + ret = dw_mipi_dsi_dcs_short_write(dsi->slave, msg); break; case MIPI_DSI_DCS_LONG_WRITE: ret = dw_mipi_dsi_dcs_long_write(dsi, msg); + if (dsi->slave) + ret = dw_mipi_dsi_dcs_long_write(dsi->slave, msg); break; default: dev_err(dsi->dev, "unsupported message type 0x%02x\n", @@ -827,6 +870,64 @@ static void dw_mipi_dsi_init(struct dw_mipi_dsi *dsi) TX_ESC_CLK_DIVIDSION(esc_clk_division)); }
+static void rockchip_dsi_grf_config(struct dw_mipi_dsi *dsi, int vop_id) +{ + const struct dw_mipi_dsi_plat_data *pdata = dsi->pdata; + int val = 0; + int ret; + + /* + * For the RK3399, the clk of grf must be enabled before writing grf + * register. And for RK3288 or other soc, this grf_clk must be NULL, + * the clk_prepare_enable return true directly. + */ + ret = clk_prepare_enable(dsi->grf_clk); + if (ret) { + dev_err(dsi->dev, "Failed to enable grf_clk: %d\n", ret); + return; + } + + if (dsi->slave) { + if (vop_id) + val = pdata->dsi0_en_bit | + (pdata->dsi0_en_bit << 16) | + pdata->dsi1_en_bit | + (pdata->dsi1_en_bit << 16); + else + val = (pdata->dsi0_en_bit << 16) | + (pdata->dsi1_en_bit << 16); + + regmap_write(dsi->grf_regmap, pdata->grf_switch_reg, val); + + if (pdata->grf_dsi0_mode_reg) + regmap_write(dsi->grf_regmap, pdata->grf_dsi0_mode_reg, + pdata->grf_dsi0_mode); + if (pdata->grf_dsi1_mode_reg1) + regmap_write(dsi->grf_regmap, pdata->grf_dsi1_mode_reg1, + pdata->grf_dsi1_mode); + if (pdata->grf_dsi1_mode_reg2) + regmap_write(dsi->grf_regmap, pdata->grf_dsi1_mode_reg2, + RK3399_GRF_DSI1_MODE2); + if (pdata->grf_dsi1_mode_reg1) + regmap_write(dsi->grf_regmap, pdata->grf_dsi1_mode_reg1, + RK3399_GRF_DSI1_ENABLE); + } else { + if (vop_id) + val = pdata->dsi0_en_bit | + (pdata->dsi0_en_bit << 16); + else + val = pdata->dsi0_en_bit << 16; + + regmap_write(dsi->grf_regmap, pdata->grf_switch_reg, val); + + if (pdata->grf_dsi0_mode_reg) + regmap_write(dsi->grf_regmap, pdata->grf_dsi0_mode_reg, + pdata->grf_dsi0_mode); + } + + clk_disable_unprepare(dsi->grf_clk); +} + static void dw_mipi_dsi_dpi_config(struct dw_mipi_dsi *dsi, struct drm_display_mode *mode) { @@ -867,7 +968,14 @@ static void dw_mipi_dsi_packet_handler_config(struct dw_mipi_dsi *dsi) static void dw_mipi_dsi_video_packet_config(struct dw_mipi_dsi *dsi, struct drm_display_mode *mode) { - dsi_write(dsi, DSI_VID_PKT_SIZE, VID_PKT_SIZE(mode->hdisplay)); + int pkt_size; + + if (dsi->slave || dsi->master) + pkt_size = VID_PKT_SIZE(mode->hdisplay / 2 + 4); + else + pkt_size = VID_PKT_SIZE(mode->hdisplay); + + dsi_write(dsi, DSI_VID_PKT_SIZE, pkt_size); }
static void dw_mipi_dsi_command_mode_config(struct dw_mipi_dsi *dsi) @@ -971,24 +1079,24 @@ static void dw_mipi_dsi_encoder_disable(struct drm_encoder *encoder)
dw_mipi_dsi_disable(dsi); pm_runtime_put(dsi->dev); + + if (dsi->slave) { + if (clk_prepare_enable(dsi->slave->pclk)) { + dev_err(dsi->slave->dev, "%s: Failed to enable pclk\n", __func__); + return; + } + dw_mipi_dsi_disable(dsi->slave); + pm_runtime_put(dsi->slave->dev); + clk_disable_unprepare(dsi->slave->pclk); + } + clk_disable_unprepare(dsi->pclk); dsi->dpms_mode = DRM_MODE_DPMS_OFF; }
-static void dw_mipi_dsi_encoder_enable(struct drm_encoder *encoder) +static void dw_mipi_dsi_enable(struct dw_mipi_dsi *dsi, struct drm_display_mode *mode) { - struct dw_mipi_dsi *dsi = encoder_to_dsi(encoder); - struct drm_display_mode *mode = &encoder->crtc->state->adjusted_mode; - const struct dw_mipi_dsi_plat_data *pdata = dsi->pdata; - int mux = drm_of_encoder_active_endpoint_id(dsi->dev->of_node, encoder); - u32 val; - int ret; - - ret = dw_mipi_dsi_get_lane_bps(dsi, mode); - if (ret < 0) - return; - - if (dsi->dpms_mode == DRM_MODE_DPMS_ON) + if (dw_mipi_dsi_get_lane_bps(dsi, mode) < 0) return;
if (clk_prepare_enable(dsi->pclk)) { @@ -1009,43 +1117,42 @@ static void dw_mipi_dsi_encoder_enable(struct drm_encoder *encoder) dw_mipi_dsi_dphy_interface_config(dsi); dw_mipi_dsi_clear_err(dsi);
- /* - * For the RK3399, the clk of grf must be enabled before writing grf - * register. And for RK3288 or other soc, this grf_clk must be NULL, - * the clk_prepare_enable return true directly. - */ - ret = clk_prepare_enable(dsi->grf_clk); - if (ret) { - dev_err(dsi->dev, "Failed to enable grf_clk: %d\n", ret); - return; - } - - if (pdata->grf_dsi0_mode_reg) - regmap_write(dsi->grf_regmap, pdata->grf_dsi0_mode_reg, - pdata->grf_dsi0_mode); - dw_mipi_dsi_phy_init(dsi); dw_mipi_dsi_wait_for_two_frames(mode);
dw_mipi_dsi_set_mode(dsi, DW_MIPI_DSI_CMD_MODE); +} + +static void dw_mipi_dsi_encoder_enable(struct drm_encoder *encoder) +{ + struct dw_mipi_dsi *dsi = encoder_to_dsi(encoder); + struct drm_display_mode *mode = &encoder->crtc->state->adjusted_mode; + int mux = drm_of_encoder_active_endpoint_id(dsi->dev->of_node, encoder); + + if (dsi->dpms_mode == DRM_MODE_DPMS_ON) + return; + + rockchip_dsi_grf_config(dsi, mux); + dev_dbg(dsi->dev, "vop %s output to dsi0\n", (mux) ? "LIT" : "BIG"); + + dw_mipi_dsi_enable(dsi, mode); + if (dsi->slave) + dw_mipi_dsi_enable(dsi->slave, mode); + if (drm_panel_prepare(dsi->panel)) dev_err(dsi->dev, "failed to prepare panel\n");
dw_mipi_dsi_set_mode(dsi, DW_MIPI_DSI_VID_MODE); + if (dsi->slave) + dw_mipi_dsi_set_mode(dsi->slave, DW_MIPI_DSI_VID_MODE); + drm_panel_enable(dsi->panel);
clk_disable_unprepare(dsi->pclk); + if (dsi->slave) + clk_disable_unprepare(dsi->slave->pclk);
- if (mux) - val = pdata->dsi0_en_bit | (pdata->dsi0_en_bit << 16); - else - val = pdata->dsi0_en_bit << 16; - - regmap_write(dsi->grf_regmap, pdata->grf_switch_reg, val); - dev_dbg(dsi->dev, "vop %s output to dsi0\n", (mux) ? "LIT" : "BIG"); dsi->dpms_mode = DRM_MODE_DPMS_ON; - - clk_disable_unprepare(dsi->grf_clk); }
static int @@ -1073,6 +1180,9 @@ static void dw_mipi_dsi_encoder_enable(struct drm_encoder *encoder)
s->output_type = DRM_MODE_CONNECTOR_DSI;
+ if (dsi->slave) + s->output_flags = ROCKCHIP_OUTPUT_DSI_DUAL_CHANNEL; + return 0; }
@@ -1178,6 +1288,12 @@ static int rockchip_mipi_parse_dt(struct dw_mipi_dsi *dsi) .grf_switch_reg = RK3399_GRF_SOC_CON20, .grf_dsi0_mode = RK3399_GRF_DSI_MODE, .grf_dsi0_mode_reg = RK3399_GRF_SOC_CON22, + .grf_dsi1_mode = RK3399_GRF_DSI1_MODE1, + .grf_dsi1_mode_reg1 = RK3399_GRF_SOC_CON23, + .dsi1_basedir = RK3399_TXRX_BASEDIR, + .dsi1_masterslavez = RK3399_TXRX_MASTERSLAVEZ, + .dsi1_enableclk = RK3399_TXRX_ENABLECLK, + .grf_dsi1_mode_reg2 = RK3399_GRF_SOC_CON24, .flags = DW_MIPI_NEEDS_PHY_CFG_CLK | DW_MIPI_NEEDS_GRF_CLK, .max_data_lanes = 4, }; @@ -1194,17 +1310,106 @@ static int rockchip_mipi_parse_dt(struct dw_mipi_dsi *dsi) }; MODULE_DEVICE_TABLE(of, dw_mipi_dsi_dt_ids);
+ +static int rockchip_dsi_dual_channel_probe(struct dw_mipi_dsi *dsi) +{ + struct device_node *np; + struct platform_device *secondary; + + np = of_parse_phandle(dsi->dev->of_node, "rockchip,dual-channel", 0); + if (np) { + secondary = of_find_device_by_node(np); + dsi->slave = platform_get_drvdata(secondary); + of_node_put(np); + + if (!dsi->slave) + return -EPROBE_DEFER; + + dsi->slave->master = dsi; + } + + return 0; +} + static int dw_mipi_dsi_bind(struct device *dev, struct device *master, void *data) { + struct drm_device *drm = data; + struct dw_mipi_dsi *dsi = dev_get_drvdata(dev); + int ret; + + ret = rockchip_dsi_dual_channel_probe(dsi); + if (ret) + return ret; + + ret = clk_prepare_enable(dsi->pllref_clk); + if (ret) { + dev_err(dev, "%s: Failed to enable pllref_clk\n", __func__); + return ret; + } + + pm_runtime_enable(dev); + + if (dsi->master) + return 0; + + ret = dw_mipi_dsi_register(drm, dsi); + if (ret) { + dev_err(dev, "Failed to register mipi_dsi: %d\n", ret); + goto err_pllref; + } + + dsi->dsi_host.ops = &dw_mipi_dsi_host_ops; + dsi->dsi_host.dev = dev; + ret = mipi_dsi_host_register(&dsi->dsi_host); + if (ret) { + dev_err(dev, "Failed to register MIPI host: %d\n", ret); + goto err_cleanup; + } + + if (!dsi->panel) { + ret = -EPROBE_DEFER; + goto err_mipi_dsi_host; + } + + return 0; + +err_mipi_dsi_host: + mipi_dsi_host_unregister(&dsi->dsi_host); +err_cleanup: + drm_encoder_cleanup(&dsi->encoder); + drm_connector_cleanup(&dsi->connector); +err_pllref: + clk_disable_unprepare(dsi->pllref_clk); + return ret; +} + +static void dw_mipi_dsi_unbind(struct device *dev, struct device *master, + void *data) +{ + struct dw_mipi_dsi *dsi = dev_get_drvdata(dev); + + mipi_dsi_host_unregister(&dsi->dsi_host); + pm_runtime_disable(dev); + if (dsi->slave) + pm_runtime_disable(dsi->slave->dev); + clk_disable_unprepare(dsi->pllref_clk); +} + +static const struct component_ops dw_mipi_dsi_ops = { + .bind = dw_mipi_dsi_bind, + .unbind = dw_mipi_dsi_unbind, +}; + +static int dw_mipi_dsi_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; const struct of_device_id *of_id = of_match_device(dw_mipi_dsi_dt_ids, dev); const struct dw_mipi_dsi_plat_data *pdata = of_id->data; - struct platform_device *pdev = to_platform_device(dev); - struct reset_control *apb_rst; - struct drm_device *drm = data; struct dw_mipi_dsi *dsi; struct resource *res; + struct reset_control *apb_rst; int ret;
dsi = devm_kzalloc(dev, sizeof(*dsi), GFP_KERNEL); @@ -1214,6 +1419,7 @@ static int dw_mipi_dsi_bind(struct device *dev, struct device *master, dsi->dev = dev; dsi->pdata = pdata; dsi->dpms_mode = DRM_MODE_DPMS_OFF; + dev_set_drvdata(dev, dsi);
ret = rockchip_mipi_parse_dt(dsi); if (ret) @@ -1288,63 +1494,6 @@ static int dw_mipi_dsi_bind(struct device *dev, struct device *master, } }
- ret = clk_prepare_enable(dsi->pllref_clk); - if (ret) { - dev_err(dev, "%s: Failed to enable pllref_clk\n", __func__); - return ret; - } - - ret = dw_mipi_dsi_register(drm, dsi); - if (ret) { - dev_err(dev, "Failed to register mipi_dsi: %d\n", ret); - goto err_pllref; - } - - pm_runtime_enable(dev); - - dsi->dsi_host.ops = &dw_mipi_dsi_host_ops; - dsi->dsi_host.dev = dev; - ret = mipi_dsi_host_register(&dsi->dsi_host); - if (ret) { - dev_err(dev, "Failed to register MIPI host: %d\n", ret); - goto err_cleanup; - } - - if (!dsi->panel) { - ret = -EPROBE_DEFER; - goto err_mipi_dsi_host; - } - - dev_set_drvdata(dev, dsi); - return 0; - -err_mipi_dsi_host: - mipi_dsi_host_unregister(&dsi->dsi_host); -err_cleanup: - drm_encoder_cleanup(&dsi->encoder); - drm_connector_cleanup(&dsi->connector); -err_pllref: - clk_disable_unprepare(dsi->pllref_clk); - return ret; -} - -static void dw_mipi_dsi_unbind(struct device *dev, struct device *master, - void *data) -{ - struct dw_mipi_dsi *dsi = dev_get_drvdata(dev); - - mipi_dsi_host_unregister(&dsi->dsi_host); - pm_runtime_disable(dev); - clk_disable_unprepare(dsi->pllref_clk); -} - -static const struct component_ops dw_mipi_dsi_ops = { - .bind = dw_mipi_dsi_bind, - .unbind = dw_mipi_dsi_unbind, -}; - -static int dw_mipi_dsi_probe(struct platform_device *pdev) -{ return component_add(&pdev->dev, &dw_mipi_dsi_ops); }
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h index c7e96b8..51ad1c2 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h @@ -36,6 +36,7 @@ struct rockchip_crtc_state { struct drm_crtc_state base; int output_type; int output_mode; + int output_flags; }; #define to_rockchip_crtc_state(s) \ container_of(s, struct rockchip_crtc_state, base) diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c index bf9ed0e..cb40cdd 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c @@ -917,6 +917,8 @@ static void vop_crtc_atomic_enable(struct drm_crtc *crtc, case DRM_MODE_CONNECTOR_DSI: VOP_REG_SET(vop, output, mipi_pin_pol, pin_pol); VOP_REG_SET(vop, output, mipi_en, 1); + VOP_REG_SET(vop, output, mipi_dual_channel_en, + !!(s->output_flags & ROCKCHIP_OUTPUT_DSI_DUAL_CHANNEL)); break; case DRM_MODE_CONNECTOR_DisplayPort: pin_pol &= ~BIT(DCLK_INVERT); diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h index 56bbd2e..d184531 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h @@ -60,6 +60,7 @@ struct vop_output { struct vop_reg edp_en; struct vop_reg hdmi_en; struct vop_reg mipi_en; + struct vop_reg mipi_dual_channel_en; struct vop_reg rgb_en; };
@@ -212,6 +213,8 @@ struct vop_data { /* for use special outface */ #define ROCKCHIP_OUT_MODE_AAAA 15
+#define ROCKCHIP_OUTPUT_DSI_DUAL_CHANNEL BIT(0) + enum alpha_mode { ALPHA_STRAIGHT, ALPHA_INVERSE,
On Mon, Sep 18, 2017 at 05:05:34PM +0800, Nickey Yang wrote:
Hi Nickey, You're adding a substantial new feature to the driver. You should add a commit message that describes what you're adding, why you're adding it, and how it is done.
Signed-off-by: Nickey Yang nickey.yang@rock-chips.com
drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 353 ++++++++++++++++++++-------- drivers/gpu/drm/rockchip/rockchip_drm_drv.h | 1 + drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 2 + drivers/gpu/drm/rockchip/rockchip_drm_vop.h | 3 + 4 files changed, 257 insertions(+), 102 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c index 52698b7..9265b7f 100644 --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c @@ -42,6 +42,17 @@ #define RK3399_GRF_SOC_CON22 0x6258 #define RK3399_GRF_DSI_MODE 0xffff0000
+/* disable turndisable, forcetxstopmode, forcerxmode, enable */ +#define RK3399_GRF_SOC_CON23 0x625c +#define RK3399_GRF_DSI1_MODE1 0xffff0000 +#define RK3399_GRF_DSI1_ENABLE 0x000f000f
Please break these out into separate defines so we know which bit is responsible for turning on/off each feature.
+/* disable basedir and enable clk*/ +#define RK3399_GRF_SOC_CON24 0x6260 +#define RK3399_TXRX_MASTERSLAVEZ BIT(7) +#define RK3399_TXRX_ENABLECLK BIT(6) +#define RK3399_TXRX_BASEDIR BIT(5) +#define RK3399_GRF_DSI1_MODE2 0x00e00080
Same comment here.
#define DSI_VERSION 0x00 #define DSI_PWR_UP 0x04 #define RESET 0 @@ -282,6 +293,12 @@ struct dw_mipi_dsi_plat_data { u32 grf_switch_reg; u32 grf_dsi0_mode; u32 grf_dsi0_mode_reg;
- u32 grf_dsi1_mode;
- u32 grf_dsi1_mode_reg1;
- u32 dsi1_basedir;
- u32 dsi1_masterslavez;
- u32 dsi1_enableclk;
- u32 grf_dsi1_mode_reg2; unsigned int flags; unsigned int max_data_lanes;
}; @@ -300,6 +317,12 @@ struct dw_mipi_dsi { struct clk *pclk; struct clk *phy_cfg_clk;
- /* dual-channel */
- struct dw_mipi_dsi *master;
- struct dw_mipi_dsi *slave;
- struct device_node *panel_node;
- int id;
- int dpms_mode; unsigned int lane_mbps; /* per lane */ u32 channel;
@@ -526,6 +549,7 @@ static int dw_mipi_dsi_get_lane_bps(struct dw_mipi_dsi *dsi, unsigned int target_mbps = 1000; unsigned int max_mbps = dptdin_map[ARRAY_SIZE(dptdin_map) - 1].max_mbps; int bpp;
- int lanes = dsi->lanes; unsigned long best_freq = 0; unsigned long fvco_min, fvco_max, fin ,fout; unsigned int min_prediv, max_prediv;
@@ -540,10 +564,13 @@ static int dw_mipi_dsi_get_lane_bps(struct dw_mipi_dsi *dsi, return bpp; }
- if (dsi->slave || dsi->master)
lanes = dsi->lanes * 2;- mpclk = DIV_ROUND_UP(mode->clock, MSEC_PER_SEC); if (mpclk) { /* take 1 / 0.8, since mbps must big than bandwidth of RGB */
tmp = mpclk * (bpp / dsi->lanes) * 10 / 8;
tmp = mpclk * (bpp / lanes) * 10 / 8;@@ -605,17 +632,26 @@ static int dw_mipi_dsi_host_attach(struct mipi_dsi_host *host, struct mipi_dsi_device *device) { struct dw_mipi_dsi *dsi = host_to_dsi(host);
- int lanes = dsi->slave ? device->lanes / 2 : device->lanes;
- if (device->lanes > dsi->pdata->max_data_lanes) {
- if (lanes > dsi->pdata->max_data_lanes) { dev_err(dsi->dev, "the number of data lanes(%u) is too many\n",
device->lanes);
lanes);
- dsi->lanes = device->lanes;
- dsi->lanes = lanes; dsi->channel = device->channel; dsi->format = device->format; dsi->mode_flags = device->mode_flags;
- if (dsi->slave) {
dsi->slave->lanes = lanes;dsi->slave->channel = device->channel;dsi->slave->format = device->format;dsi->slave->mode_flags = device->mode_flags;- }
- dsi->panel = of_drm_find_panel(device->dev.of_node); if (dsi->panel) return drm_panel_attach(dsi->panel, &dsi->connector);
@@ -745,15 +781,22 @@ static ssize_t dw_mipi_dsi_host_transfer(struct mipi_dsi_host *host, int ret;
dw_mipi_message_config(dsi, msg);
if (dsi->slave)
dw_mipi_message_config(dsi->slave, msg);switch (msg->type) { case MIPI_DSI_DCS_SHORT_WRITE: case MIPI_DSI_DCS_SHORT_WRITE_PARAM: case MIPI_DSI_SET_MAXIMUM_RETURN_PACKET_SIZE:
case MIPI_DSI_GENERIC_SHORT_WRITE_2_PARAM: ret = dw_mipi_dsi_dcs_short_write(dsi, msg);
if (dsi->slave)ret = dw_mipi_dsi_dcs_short_write(dsi->slave, msg);break; case MIPI_DSI_DCS_LONG_WRITE: ret = dw_mipi_dsi_dcs_long_write(dsi, msg);
if (dsi->slave)ret = dw_mipi_dsi_dcs_long_write(dsi->slave, msg);break; default: dev_err(dsi->dev, "unsupported message type 0x%02x\n",
@@ -827,6 +870,64 @@ static void dw_mipi_dsi_init(struct dw_mipi_dsi *dsi) TX_ESC_CLK_DIVIDSION(esc_clk_division)); }
+static void rockchip_dsi_grf_config(struct dw_mipi_dsi *dsi, int vop_id) +{
- const struct dw_mipi_dsi_plat_data *pdata = dsi->pdata;
- int val = 0;
- int ret;
- /*
* For the RK3399, the clk of grf must be enabled before writing grf* register. And for RK3288 or other soc, this grf_clk must be NULL,* the clk_prepare_enable return true directly.*/- ret = clk_prepare_enable(dsi->grf_clk);
- if (ret) {
dev_err(dsi->dev, "Failed to enable grf_clk: %d\n", ret);return;- }
- if (dsi->slave) {
if (vop_id)val = pdata->dsi0_en_bit |(pdata->dsi0_en_bit << 16) |pdata->dsi1_en_bit |(pdata->dsi1_en_bit << 16);elseval = (pdata->dsi0_en_bit << 16) |(pdata->dsi1_en_bit << 16);regmap_write(dsi->grf_regmap, pdata->grf_switch_reg, val);if (pdata->grf_dsi0_mode_reg)regmap_write(dsi->grf_regmap, pdata->grf_dsi0_mode_reg,pdata->grf_dsi0_mode);if (pdata->grf_dsi1_mode_reg1)regmap_write(dsi->grf_regmap, pdata->grf_dsi1_mode_reg1,pdata->grf_dsi1_mode);if (pdata->grf_dsi1_mode_reg2)regmap_write(dsi->grf_regmap, pdata->grf_dsi1_mode_reg2,RK3399_GRF_DSI1_MODE2);if (pdata->grf_dsi1_mode_reg1)regmap_write(dsi->grf_regmap, pdata->grf_dsi1_mode_reg1,RK3399_GRF_DSI1_ENABLE);- } else {
if (vop_id)val = pdata->dsi0_en_bit |(pdata->dsi0_en_bit << 16);elseval = pdata->dsi0_en_bit << 16;regmap_write(dsi->grf_regmap, pdata->grf_switch_reg, val);if (pdata->grf_dsi0_mode_reg)regmap_write(dsi->grf_regmap, pdata->grf_dsi0_mode_reg,pdata->grf_dsi0_mode);- }
You have some duplication here, how about:
val = pdata->dsi0_en_bit << 16; if (dsi->slave) val |= pdata->dsi1_en_bit << 16 if (vop_id) { val |= pdata->dsi0_en_bit; if (dsi->slave) val |= pdata->dsi1_en_bit; } regmap_write(dsi->grf_regmap, pdata->grf_switch_reg, val);
if (pdata->grf_dsi0_mode_reg) regmap_write(dsi->grf_regmap, pdata->grf_dsi0_mode_reg, pdata->grf_dsi0_mode);
if (dsi->slave) { if (pdata->grf_dsi1_mode_reg1) regmap_write(dsi->grf_regmap, pdata->grf_dsi1_mode_reg1, pdata->grf_dsi1_mode); if (pdata->grf_dsi1_mode_reg2) regmap_write(dsi->grf_regmap, pdata->grf_dsi1_mode_reg2, RK3399_GRF_DSI1_MODE2); if (pdata->grf_dsi1_mode_reg1) regmap_write(dsi->grf_regmap, pdata->grf_dsi1_mode_reg1, RK3399_GRF_DSI1_ENABLE); }
- clk_disable_unprepare(dsi->grf_clk);
+}
static void dw_mipi_dsi_dpi_config(struct dw_mipi_dsi *dsi, struct drm_display_mode *mode) { @@ -867,7 +968,14 @@ static void dw_mipi_dsi_packet_handler_config(struct dw_mipi_dsi *dsi) static void dw_mipi_dsi_video_packet_config(struct dw_mipi_dsi *dsi, struct drm_display_mode *mode) {
- dsi_write(dsi, DSI_VID_PKT_SIZE, VID_PKT_SIZE(mode->hdisplay));
- int pkt_size;
- if (dsi->slave || dsi->master)
pkt_size = VID_PKT_SIZE(mode->hdisplay / 2 + 4);
Why add 4? Please either add a comment, pull out into a well-named define, or both :)
- else
pkt_size = VID_PKT_SIZE(mode->hdisplay);- dsi_write(dsi, DSI_VID_PKT_SIZE, pkt_size);
}
static void dw_mipi_dsi_command_mode_config(struct dw_mipi_dsi *dsi) @@ -971,24 +1079,24 @@ static void dw_mipi_dsi_encoder_disable(struct drm_encoder *encoder)
dw_mipi_dsi_disable(dsi); pm_runtime_put(dsi->dev);
- if (dsi->slave) {
if (clk_prepare_enable(dsi->slave->pclk)) {dev_err(dsi->slave->dev, "%s: Failed to enable pclk\n", __func__);
Rockchip driver has been moved to using the DRM_DEV_* log messages, please change all instances of dev_* in the patch.
return;
You still need to disable_unprepare dsi->pclk as well. Consider adding a label 'out' above the disable_unprepare below and jump to it from here.
}dw_mipi_dsi_disable(dsi->slave);pm_runtime_put(dsi->slave->dev);clk_disable_unprepare(dsi->slave->pclk);- }
- clk_disable_unprepare(dsi->pclk); dsi->dpms_mode = DRM_MODE_DPMS_OFF;
}
-static void dw_mipi_dsi_encoder_enable(struct drm_encoder *encoder) +static void dw_mipi_dsi_enable(struct dw_mipi_dsi *dsi, struct drm_display_mode *mode) {
- struct dw_mipi_dsi *dsi = encoder_to_dsi(encoder);
- struct drm_display_mode *mode = &encoder->crtc->state->adjusted_mode;
- const struct dw_mipi_dsi_plat_data *pdata = dsi->pdata;
- int mux = drm_of_encoder_active_endpoint_id(dsi->dev->of_node, encoder);
- u32 val;
- int ret;
- ret = dw_mipi_dsi_get_lane_bps(dsi, mode);
- if (ret < 0)
return;- if (dsi->dpms_mode == DRM_MODE_DPMS_ON)
- if (dw_mipi_dsi_get_lane_bps(dsi, mode) < 0)
Can you please log an error with the return code here? Silently failing is bound to cause frustration.
return;if (clk_prepare_enable(dsi->pclk)) { @@ -1009,43 +1117,42 @@ static void dw_mipi_dsi_encoder_enable(struct drm_encoder *encoder) dw_mipi_dsi_dphy_interface_config(dsi); dw_mipi_dsi_clear_err(dsi);
/*
* For the RK3399, the clk of grf must be enabled before writing grf* register. And for RK3288 or other soc, this grf_clk must be NULL,* the clk_prepare_enable return true directly.*/ret = clk_prepare_enable(dsi->grf_clk);
if (ret) {
dev_err(dsi->dev, "Failed to enable grf_clk: %d\n", ret);return;}
if (pdata->grf_dsi0_mode_reg)
regmap_write(dsi->grf_regmap, pdata->grf_dsi0_mode_reg,pdata->grf_dsi0_mode);dw_mipi_dsi_phy_init(dsi); dw_mipi_dsi_wait_for_two_frames(mode);
dw_mipi_dsi_set_mode(dsi, DW_MIPI_DSI_CMD_MODE);
+}
+static void dw_mipi_dsi_encoder_enable(struct drm_encoder *encoder) +{
struct dw_mipi_dsi *dsi = encoder_to_dsi(encoder);
struct drm_display_mode *mode = &encoder->crtc->state->adjusted_mode;
int mux = drm_of_encoder_active_endpoint_id(dsi->dev->of_node, encoder);
if (dsi->dpms_mode == DRM_MODE_DPMS_ON)
return;rockchip_dsi_grf_config(dsi, mux);
dev_dbg(dsi->dev, "vop %s output to dsi0\n", (mux) ? "LIT" : "BIG");
dw_mipi_dsi_enable(dsi, mode);
if (dsi->slave)
dw_mipi_dsi_enable(dsi->slave, mode);if (drm_panel_prepare(dsi->panel)) dev_err(dsi->dev, "failed to prepare panel\n");
dw_mipi_dsi_set_mode(dsi, DW_MIPI_DSI_VID_MODE);
if (dsi->slave)
dw_mipi_dsi_set_mode(dsi->slave, DW_MIPI_DSI_VID_MODE);drm_panel_enable(dsi->panel);
clk_disable_unprepare(dsi->pclk);
if (dsi->slave)
clk_disable_unprepare(dsi->slave->pclk);
- if (mux)
val = pdata->dsi0_en_bit | (pdata->dsi0_en_bit << 16);- else
val = pdata->dsi0_en_bit << 16;- regmap_write(dsi->grf_regmap, pdata->grf_switch_reg, val);
- dev_dbg(dsi->dev, "vop %s output to dsi0\n", (mux) ? "LIT" : "BIG"); dsi->dpms_mode = DRM_MODE_DPMS_ON;
- clk_disable_unprepare(dsi->grf_clk);
}
static int @@ -1073,6 +1180,9 @@ static void dw_mipi_dsi_encoder_enable(struct drm_encoder *encoder)
s->output_type = DRM_MODE_CONNECTOR_DSI;
- if (dsi->slave)
s->output_flags = ROCKCHIP_OUTPUT_DSI_DUAL_CHANNEL;- return 0;
}
@@ -1178,6 +1288,12 @@ static int rockchip_mipi_parse_dt(struct dw_mipi_dsi *dsi) .grf_switch_reg = RK3399_GRF_SOC_CON20, .grf_dsi0_mode = RK3399_GRF_DSI_MODE, .grf_dsi0_mode_reg = RK3399_GRF_SOC_CON22,
- .grf_dsi1_mode = RK3399_GRF_DSI1_MODE1,
- .grf_dsi1_mode_reg1 = RK3399_GRF_SOC_CON23,
- .dsi1_basedir = RK3399_TXRX_BASEDIR,
- .dsi1_masterslavez = RK3399_TXRX_MASTERSLAVEZ,
- .dsi1_enableclk = RK3399_TXRX_ENABLECLK,
- .grf_dsi1_mode_reg2 = RK3399_GRF_SOC_CON24, .flags = DW_MIPI_NEEDS_PHY_CFG_CLK | DW_MIPI_NEEDS_GRF_CLK, .max_data_lanes = 4,
}; @@ -1194,17 +1310,106 @@ static int rockchip_mipi_parse_dt(struct dw_mipi_dsi *dsi) }; MODULE_DEVICE_TABLE(of, dw_mipi_dsi_dt_ids);
+static int rockchip_dsi_dual_channel_probe(struct dw_mipi_dsi *dsi) +{
- struct device_node *np;
- struct platform_device *secondary;
- np = of_parse_phandle(dsi->dev->of_node, "rockchip,dual-channel", 0);
- if (np) {
secondary = of_find_device_by_node(np);dsi->slave = platform_get_drvdata(secondary);of_node_put(np);if (!dsi->slave)return -EPROBE_DEFER;dsi->slave->master = dsi;- }
- return 0;
+}
static int dw_mipi_dsi_bind(struct device *dev, struct device *master, void *data) {
- struct drm_device *drm = data;
- struct dw_mipi_dsi *dsi = dev_get_drvdata(dev);
- int ret;
- ret = rockchip_dsi_dual_channel_probe(dsi);
- if (ret)
return ret;- ret = clk_prepare_enable(dsi->pllref_clk);
- if (ret) {
dev_err(dev, "%s: Failed to enable pllref_clk\n", __func__);return ret;- }
- pm_runtime_enable(dev);
- if (dsi->master)
return 0;- ret = dw_mipi_dsi_register(drm, dsi);
- if (ret) {
dev_err(dev, "Failed to register mipi_dsi: %d\n", ret);goto err_pllref;- }
- dsi->dsi_host.ops = &dw_mipi_dsi_host_ops;
- dsi->dsi_host.dev = dev;
- ret = mipi_dsi_host_register(&dsi->dsi_host);
- if (ret) {
dev_err(dev, "Failed to register MIPI host: %d\n", ret);goto err_cleanup;- }
- if (!dsi->panel) {
ret = -EPROBE_DEFER;goto err_mipi_dsi_host;- }
- return 0;
+err_mipi_dsi_host:
- mipi_dsi_host_unregister(&dsi->dsi_host);
+err_cleanup:
- drm_encoder_cleanup(&dsi->encoder);
- drm_connector_cleanup(&dsi->connector);
+err_pllref:
Do you need to disable pm runtime on dev?
- clk_disable_unprepare(dsi->pllref_clk);
- return ret;
+}
+static void dw_mipi_dsi_unbind(struct device *dev, struct device *master,
void *data)+{
- struct dw_mipi_dsi *dsi = dev_get_drvdata(dev);
- mipi_dsi_host_unregister(&dsi->dsi_host);
- pm_runtime_disable(dev);
- if (dsi->slave)
pm_runtime_disable(dsi->slave->dev);- clk_disable_unprepare(dsi->pllref_clk);
+}
+static const struct component_ops dw_mipi_dsi_ops = {
- .bind = dw_mipi_dsi_bind,
- .unbind = dw_mipi_dsi_unbind,
+};
+static int dw_mipi_dsi_probe(struct platform_device *pdev) +{
- struct device *dev = &pdev->dev; const struct of_device_id *of_id = of_match_device(dw_mipi_dsi_dt_ids, dev); const struct dw_mipi_dsi_plat_data *pdata = of_id->data;
- struct platform_device *pdev = to_platform_device(dev);
- struct reset_control *apb_rst;
- struct drm_device *drm = data; struct dw_mipi_dsi *dsi; struct resource *res;
struct reset_control *apb_rst; int ret;
dsi = devm_kzalloc(dev, sizeof(*dsi), GFP_KERNEL);
@@ -1214,6 +1419,7 @@ static int dw_mipi_dsi_bind(struct device *dev, struct device *master, dsi->dev = dev; dsi->pdata = pdata; dsi->dpms_mode = DRM_MODE_DPMS_OFF;
dev_set_drvdata(dev, dsi);
ret = rockchip_mipi_parse_dt(dsi); if (ret)
@@ -1288,63 +1494,6 @@ static int dw_mipi_dsi_bind(struct device *dev, struct device *master, } }
- ret = clk_prepare_enable(dsi->pllref_clk);
- if (ret) {
dev_err(dev, "%s: Failed to enable pllref_clk\n", __func__);return ret;- }
- ret = dw_mipi_dsi_register(drm, dsi);
- if (ret) {
dev_err(dev, "Failed to register mipi_dsi: %d\n", ret);goto err_pllref;- }
- pm_runtime_enable(dev);
- dsi->dsi_host.ops = &dw_mipi_dsi_host_ops;
- dsi->dsi_host.dev = dev;
- ret = mipi_dsi_host_register(&dsi->dsi_host);
- if (ret) {
dev_err(dev, "Failed to register MIPI host: %d\n", ret);goto err_cleanup;- }
- if (!dsi->panel) {
ret = -EPROBE_DEFER;goto err_mipi_dsi_host;- }
- dev_set_drvdata(dev, dsi);
- return 0;
-err_mipi_dsi_host:
- mipi_dsi_host_unregister(&dsi->dsi_host);
-err_cleanup:
- drm_encoder_cleanup(&dsi->encoder);
- drm_connector_cleanup(&dsi->connector);
-err_pllref:
- clk_disable_unprepare(dsi->pllref_clk);
- return ret;
-}
-static void dw_mipi_dsi_unbind(struct device *dev, struct device *master,
void *data)-{
- struct dw_mipi_dsi *dsi = dev_get_drvdata(dev);
- mipi_dsi_host_unregister(&dsi->dsi_host);
- pm_runtime_disable(dev);
- clk_disable_unprepare(dsi->pllref_clk);
-}
-static const struct component_ops dw_mipi_dsi_ops = {
- .bind = dw_mipi_dsi_bind,
- .unbind = dw_mipi_dsi_unbind,
-};
-static int dw_mipi_dsi_probe(struct platform_device *pdev) -{ return component_add(&pdev->dev, &dw_mipi_dsi_ops); }
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h index c7e96b8..51ad1c2 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h +++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h @@ -36,6 +36,7 @@ struct rockchip_crtc_state { struct drm_crtc_state base; int output_type; int output_mode;
- int output_flags;
}; #define to_rockchip_crtc_state(s) \ container_of(s, struct rockchip_crtc_state, base) diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c index bf9ed0e..cb40cdd 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c @@ -917,6 +917,8 @@ static void vop_crtc_atomic_enable(struct drm_crtc *crtc, case DRM_MODE_CONNECTOR_DSI: VOP_REG_SET(vop, output, mipi_pin_pol, pin_pol); VOP_REG_SET(vop, output, mipi_en, 1);
VOP_REG_SET(vop, output, mipi_dual_channel_en,!!(s->output_flags & ROCKCHIP_OUTPUT_DSI_DUAL_CHANNEL));diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h index 56bbd2e..d184531 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.h +++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.h @@ -60,6 +60,7 @@ struct vop_output { struct vop_reg edp_en; struct vop_reg hdmi_en; struct vop_reg mipi_en;
- struct vop_reg mipi_dual_channel_en; struct vop_reg rgb_en;
};
@@ -212,6 +213,8 @@ struct vop_data { /* for use special outface */ #define ROCKCHIP_OUT_MODE_AAAA 15
+#define ROCKCHIP_OUTPUT_DSI_DUAL_CHANNEL BIT(0)
enum alpha_mode { ALPHA_STRAIGHT, ALPHA_INVERSE, -- 1.9.1
Configure dsi slave channel when driving a panel which needs 2 DSI links.
Signed-off-by: Nickey Yang nickey.yang@rock-chips.com --- .../devicetree/bindings/display/rockchip/dw_mipi_dsi_rockchip.txt | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/display/rockchip/dw_mipi_dsi_rockchip.txt b/Documentation/devicetree/bindings/display/rockchip/dw_mipi_dsi_rockchip.txt index 6bb59ab..e13e1a3 100644 --- a/Documentation/devicetree/bindings/display/rockchip/dw_mipi_dsi_rockchip.txt +++ b/Documentation/devicetree/bindings/display/rockchip/dw_mipi_dsi_rockchip.txt @@ -19,6 +19,8 @@ Optional properties: - power-domains: a phandle to mipi dsi power domain node. - resets: list of phandle + reset specifier pairs, as described in [3]. - reset-names: string reset name, must be "apb". +- rockchip,dual-channel: configure dsi slave channel when driving a panel + which needs 2 DSI links.
[1] Documentation/devicetree/bindings/clock/clock-bindings.txt [2] Documentation/devicetree/bindings/media/video-interfaces.txt
Hi,
On Mon, Sep 18, 2017 at 05:05:35PM +0800, Nickey Yang wrote:
Configure dsi slave channel when driving a panel which needs 2 DSI links.
Signed-off-by: Nickey Yang nickey.yang@rock-chips.com
.../devicetree/bindings/display/rockchip/dw_mipi_dsi_rockchip.txt | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/Documentation/devicetree/bindings/display/rockchip/dw_mipi_dsi_rockchip.txt b/Documentation/devicetree/bindings/display/rockchip/dw_mipi_dsi_rockchip.txt index 6bb59ab..e13e1a3 100644 --- a/Documentation/devicetree/bindings/display/rockchip/dw_mipi_dsi_rockchip.txt +++ b/Documentation/devicetree/bindings/display/rockchip/dw_mipi_dsi_rockchip.txt @@ -19,6 +19,8 @@ Optional properties:
- power-domains: a phandle to mipi dsi power domain node.
- resets: list of phandle + reset specifier pairs, as described in [3].
- reset-names: string reset name, must be "apb".
+- rockchip,dual-channel: configure dsi slave channel when driving a panel
The wording "configure dsi slave channel" doesn't really tell you exactly what this *is*; in fact, it's more like a description of the SW than the HW (DT is supposed to describe HW).
You probably want something like "phandle to a 2nd DSI channel; useful as a slave channel when driving a panel which needs 2 DSI links".
Brian
- which needs 2 DSI links.
[1] Documentation/devicetree/bindings/clock/clock-bindings.txt [2] Documentation/devicetree/bindings/media/video-interfaces.txt -- 1.9.1
As MIPI PHY document show, icpctrl<3..0> and lpfctrl<5..0> should depend on frequency,so fix it.
Signed-off-by: Nickey Yang nickey.yang@rock-chips.com --- drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 70 +++++++++++++++++++--------------- 1 file changed, 40 insertions(+), 30 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c index 9265b7f..d5250e8 100644 --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c @@ -228,10 +228,10 @@ #define VCO_IN_CAP_CON_HIGH (0x2 << 1) #define REF_BIAS_CUR_SEL BIT(0)
-#define CP_CURRENT_3MA BIT(3) +#define CP_CURRENT_SEL(val) ((val) & 0xf) #define CP_PROGRAM_EN BIT(7) #define LPF_PROGRAM_EN BIT(6) -#define LPF_RESISTORS_20_KOHM 0 +#define LPF_RESISTORS_SEL(val) ((val) & 0x3f)
#define HSFREQRANGE_SEL(val) (((val) & 0x3f) << 1)
@@ -340,32 +340,44 @@ enum dw_mipi_dsi_mode { DW_MIPI_DSI_VID_MODE, };
-struct dphy_pll_testdin_map { +struct dphy_pll_parameter_map { unsigned int max_mbps; - u8 testdin; + u8 hsfreqrange; + u8 icpctrl; + u8 lpfctrl; };
/* The table is based on 27MHz DPHY pll reference clock. */ -static const struct dphy_pll_testdin_map dptdin_map[] = { - { 90, 0x00}, { 100, 0x10}, { 110, 0x20}, { 130, 0x01}, - { 140, 0x11}, { 150, 0x21}, { 170, 0x02}, { 180, 0x12}, - { 200, 0x22}, { 220, 0x03}, { 240, 0x13}, { 250, 0x23}, - { 270, 0x04}, { 300, 0x14}, { 330, 0x05}, { 360, 0x15}, - { 400, 0x25}, { 450, 0x06}, { 500, 0x16}, { 550, 0x07}, - { 600, 0x17}, { 650, 0x08}, { 700, 0x18}, { 750, 0x09}, - { 800, 0x19}, { 850, 0x29}, { 900, 0x39}, { 950, 0x0a}, - {1000, 0x1a}, {1050, 0x2a}, {1100, 0x3a}, {1150, 0x0b}, - {1200, 0x1b}, {1250, 0x2b}, {1300, 0x3b}, {1350, 0x0c}, - {1400, 0x1c}, {1450, 0x2c}, {1500, 0x3c} +static const struct dphy_pll_parameter_map dppa_map[] = { + { 90, 0x00, 0x1 ,0x02}, { 100, 0x10 ,0x1, 0x02}, + { 110, 0x20, 0x1, 0x02}, { 130, 0x01, 0x1, 0x01}, + { 140, 0x11, 0x1, 0x01}, { 150, 0x21, 0x1, 0x01}, + { 170, 0x02, 0x9, 0x02}, { 180, 0x12, 0x9, 0x02}, + { 200, 0x22, 0x9, 0x02}, { 220, 0x03, 0x2, 0x02}, + { 240, 0x13, 0x2, 0x02}, { 250, 0x23, 0x2, 0x02}, + { 270, 0x04, 0x9, 0x04}, { 300, 0x14, 0x9, 0x04}, + { 330, 0x05, 0x1, 0x01}, { 360, 0x15, 0x1, 0x01}, + { 400, 0x25, 0x1, 0x01}, { 450, 0x06, 0x6, 0x04}, + { 500, 0x16, 0x6, 0x04}, { 550, 0x07, 0x6, 0x08}, + { 600, 0x17, 0x6, 0x08}, { 650, 0x08, 0x6, 0x04}, + { 700, 0x18, 0x6, 0x04}, { 750, 0x09, 0x6, 0x04}, + { 800, 0x19, 0x6, 0x04}, { 850, 0x29, 0x6, 0x04}, + { 900, 0x39, 0x6, 0x04}, { 950, 0x0a, 0xb, 0x10}, + {1000, 0x1a, 0xb, 0x10}, {1050, 0x2a, 0xb, 0x10}, + {1100, 0x3a, 0xb, 0x10}, {1150, 0x0b, 0xb, 0x08}, + {1200, 0x1b, 0xb, 0x08}, {1250, 0x2b, 0xb, 0x08}, + {1300, 0x3b, 0xb, 0x08}, {1350, 0x0c, 0xb, 0x08}, + {1400, 0x1c, 0xb, 0x08}, {1450, 0x2c, 0xb, 0x08}, + {1500, 0x3c, 0xb, 0x08} };
-static int max_mbps_to_testdin(unsigned int max_mbps) +static int max_mbps_to_parameter(unsigned int max_mbps) { int i;
- for (i = 0; i < ARRAY_SIZE(dptdin_map); i++) - if (dptdin_map[i].max_mbps > max_mbps) - return dptdin_map[i].testdin; + for (i = 0; i < ARRAY_SIZE(dppa_map); i++) + if (dppa_map[i].max_mbps > max_mbps) + return i;
return -EINVAL; } @@ -447,16 +459,16 @@ static inline unsigned int ns2ui(struct dw_mipi_dsi *dsi, int ns)
static int dw_mipi_dsi_phy_init(struct dw_mipi_dsi *dsi) { - int ret, testdin, vco, val; + int ret, i, vco, val;
vco = (dsi->lane_mbps < 200) ? 0 : (dsi->lane_mbps + 100) / 200;
- testdin = max_mbps_to_testdin(dsi->lane_mbps); - if (testdin < 0) { + i = max_mbps_to_parameter(dsi->lane_mbps); + if (i < 0) { dev_err(dsi->dev, - "failed to get testdin for %dmbps lane clock\n", + "failed to get parameter for %dmbps lane clock\n", dsi->lane_mbps); - return testdin; + return i; }
/* Start by clearing PHY state */ @@ -475,12 +487,10 @@ static int dw_mipi_dsi_phy_init(struct dw_mipi_dsi *dsi) VCO_IN_CAP_CON_LOW | REF_BIAS_CUR_SEL);
- dw_mipi_dsi_phy_write(dsi, 0x11, CP_CURRENT_3MA); + dw_mipi_dsi_phy_write(dsi, 0x11, CP_CURRENT_SEL(dppa_map[i].icpctrl)); dw_mipi_dsi_phy_write(dsi, 0x12, CP_PROGRAM_EN | LPF_PROGRAM_EN | - LPF_RESISTORS_20_KOHM); - - dw_mipi_dsi_phy_write(dsi, 0x44, HSFREQRANGE_SEL(testdin)); - + LPF_RESISTORS_SEL(dppa_map[i].lpfctrl)); + dw_mipi_dsi_phy_write(dsi, 0x44, HSFREQRANGE_SEL(dppa_map[i].hsfreqrange)); dw_mipi_dsi_phy_write(dsi, 0x17, INPUT_DIVIDER(dsi->input_div)); dw_mipi_dsi_phy_write(dsi, 0x18, LOOP_DIV_LOW_SEL(dsi->feedback_div) | LOW_PROGRAM_EN); @@ -547,7 +557,7 @@ static int dw_mipi_dsi_get_lane_bps(struct dw_mipi_dsi *dsi, { unsigned long mpclk, tmp; unsigned int target_mbps = 1000; - unsigned int max_mbps = dptdin_map[ARRAY_SIZE(dptdin_map) - 1].max_mbps; + unsigned int max_mbps = dppa_map[ARRAY_SIZE(dppa_map) - 1].max_mbps; int bpp; int lanes = dsi->lanes; unsigned long best_freq = 0;
On Mon, Sep 18, 2017 at 05:05:36PM +0800, Nickey Yang wrote:
As MIPI PHY document show, icpctrl<3..0> and lpfctrl<5..0> should depend on frequency,so fix it.
Signed-off-by: Nickey Yang nickey.yang@rock-chips.com
drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 70 +++++++++++++++++++--------------- 1 file changed, 40 insertions(+), 30 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c index 9265b7f..d5250e8 100644 --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c @@ -228,10 +228,10 @@ #define VCO_IN_CAP_CON_HIGH (0x2 << 1) #define REF_BIAS_CUR_SEL BIT(0)
-#define CP_CURRENT_3MA BIT(3) +#define CP_CURRENT_SEL(val) ((val) & 0xf) #define CP_PROGRAM_EN BIT(7) #define LPF_PROGRAM_EN BIT(6) -#define LPF_RESISTORS_20_KOHM 0 +#define LPF_RESISTORS_SEL(val) ((val) & 0x3f)
#define HSFREQRANGE_SEL(val) (((val) & 0x3f) << 1)
@@ -340,32 +340,44 @@ enum dw_mipi_dsi_mode { DW_MIPI_DSI_VID_MODE, };
-struct dphy_pll_testdin_map { +struct dphy_pll_parameter_map { unsigned int max_mbps;
- u8 testdin;
- u8 hsfreqrange;
- u8 icpctrl;
- u8 lpfctrl;
};
/* The table is based on 27MHz DPHY pll reference clock. */ -static const struct dphy_pll_testdin_map dptdin_map[] = {
- { 90, 0x00}, { 100, 0x10}, { 110, 0x20}, { 130, 0x01},
- { 140, 0x11}, { 150, 0x21}, { 170, 0x02}, { 180, 0x12},
- { 200, 0x22}, { 220, 0x03}, { 240, 0x13}, { 250, 0x23},
- { 270, 0x04}, { 300, 0x14}, { 330, 0x05}, { 360, 0x15},
- { 400, 0x25}, { 450, 0x06}, { 500, 0x16}, { 550, 0x07},
- { 600, 0x17}, { 650, 0x08}, { 700, 0x18}, { 750, 0x09},
- { 800, 0x19}, { 850, 0x29}, { 900, 0x39}, { 950, 0x0a},
- {1000, 0x1a}, {1050, 0x2a}, {1100, 0x3a}, {1150, 0x0b},
- {1200, 0x1b}, {1250, 0x2b}, {1300, 0x3b}, {1350, 0x0c},
- {1400, 0x1c}, {1450, 0x2c}, {1500, 0x3c}
+static const struct dphy_pll_parameter_map dppa_map[] = {
- { 90, 0x00, 0x1 ,0x02}, { 100, 0x10 ,0x1, 0x02},
s/ ,/, /g
Switching icpctrl and lpfctrl values to hardcoded numbers obfuscates the value they're being set to (ie: 3mA and 20kOhm). Can you please replace the hardcoded values with #defines describing what the values mean?
- { 110, 0x20, 0x1, 0x02}, { 130, 0x01, 0x1, 0x01},
- { 140, 0x11, 0x1, 0x01}, { 150, 0x21, 0x1, 0x01},
- { 170, 0x02, 0x9, 0x02}, { 180, 0x12, 0x9, 0x02},
- { 200, 0x22, 0x9, 0x02}, { 220, 0x03, 0x2, 0x02},
- { 240, 0x13, 0x2, 0x02}, { 250, 0x23, 0x2, 0x02},
- { 270, 0x04, 0x9, 0x04}, { 300, 0x14, 0x9, 0x04},
- { 330, 0x05, 0x1, 0x01}, { 360, 0x15, 0x1, 0x01},
- { 400, 0x25, 0x1, 0x01}, { 450, 0x06, 0x6, 0x04},
- { 500, 0x16, 0x6, 0x04}, { 550, 0x07, 0x6, 0x08},
- { 600, 0x17, 0x6, 0x08}, { 650, 0x08, 0x6, 0x04},
- { 700, 0x18, 0x6, 0x04}, { 750, 0x09, 0x6, 0x04},
- { 800, 0x19, 0x6, 0x04}, { 850, 0x29, 0x6, 0x04},
- { 900, 0x39, 0x6, 0x04}, { 950, 0x0a, 0xb, 0x10},
- {1000, 0x1a, 0xb, 0x10}, {1050, 0x2a, 0xb, 0x10},
- {1100, 0x3a, 0xb, 0x10}, {1150, 0x0b, 0xb, 0x08},
- {1200, 0x1b, 0xb, 0x08}, {1250, 0x2b, 0xb, 0x08},
- {1300, 0x3b, 0xb, 0x08}, {1350, 0x0c, 0xb, 0x08},
- {1400, 0x1c, 0xb, 0x08}, {1450, 0x2c, 0xb, 0x08},
- {1500, 0x3c, 0xb, 0x08}
};
-static int max_mbps_to_testdin(unsigned int max_mbps) +static int max_mbps_to_parameter(unsigned int max_mbps) { int i;
- for (i = 0; i < ARRAY_SIZE(dptdin_map); i++)
if (dptdin_map[i].max_mbps > max_mbps)return dptdin_map[i].testdin;
for (i = 0; i < ARRAY_SIZE(dppa_map); i++)
if (dppa_map[i].max_mbps > max_mbps)return i;return -EINVAL;
} @@ -447,16 +459,16 @@ static inline unsigned int ns2ui(struct dw_mipi_dsi *dsi, int ns)
static int dw_mipi_dsi_phy_init(struct dw_mipi_dsi *dsi) {
- int ret, testdin, vco, val;
int ret, i, vco, val;
vco = (dsi->lane_mbps < 200) ? 0 : (dsi->lane_mbps + 100) / 200;
- testdin = max_mbps_to_testdin(dsi->lane_mbps);
- if (testdin < 0) {
- i = max_mbps_to_parameter(dsi->lane_mbps);
- if (i < 0) { dev_err(dsi->dev,
"failed to get testdin for %dmbps lane clock\n",
"failed to get parameter for %dmbps lane clock\n", dsi->lane_mbps);
return testdin;
return i;}
/* Start by clearing PHY state */
@@ -475,12 +487,10 @@ static int dw_mipi_dsi_phy_init(struct dw_mipi_dsi *dsi) VCO_IN_CAP_CON_LOW | REF_BIAS_CUR_SEL);
- dw_mipi_dsi_phy_write(dsi, 0x11, CP_CURRENT_3MA);
- dw_mipi_dsi_phy_write(dsi, 0x11, CP_CURRENT_SEL(dppa_map[i].icpctrl)); dw_mipi_dsi_phy_write(dsi, 0x12, CP_PROGRAM_EN | LPF_PROGRAM_EN |
LPF_RESISTORS_20_KOHM);- dw_mipi_dsi_phy_write(dsi, 0x44, HSFREQRANGE_SEL(testdin));
LPF_RESISTORS_SEL(dppa_map[i].lpfctrl));- dw_mipi_dsi_phy_write(dsi, 0x44, HSFREQRANGE_SEL(dppa_map[i].hsfreqrange)); dw_mipi_dsi_phy_write(dsi, 0x17, INPUT_DIVIDER(dsi->input_div)); dw_mipi_dsi_phy_write(dsi, 0x18, LOOP_DIV_LOW_SEL(dsi->feedback_div) | LOW_PROGRAM_EN);
@@ -547,7 +557,7 @@ static int dw_mipi_dsi_get_lane_bps(struct dw_mipi_dsi *dsi, { unsigned long mpclk, tmp; unsigned int target_mbps = 1000;
- unsigned int max_mbps = dptdin_map[ARRAY_SIZE(dptdin_map) - 1].max_mbps;
- unsigned int max_mbps = dppa_map[ARRAY_SIZE(dppa_map) - 1].max_mbps; int bpp; int lanes = dsi->lanes; unsigned long best_freq = 0;
-- 1.9.1
clk_24m --> Gate11[14] --> clk_mipidphy_ref --> Gate21[0] --> clk_dphy_pll
Signed-off-by: Nickey Yang nickey.yang@rock-chips.com --- arch/arm64/boot/dts/rockchip/rk3399.dtsi | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi b/arch/arm64/boot/dts/rockchip/rk3399.dtsi index d79e9b3..6aa43fd 100644 --- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi +++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi @@ -1629,7 +1629,7 @@ compatible = "rockchip,rk3399-mipi-dsi", "snps,dw-mipi-dsi"; reg = <0x0 0xff960000 0x0 0x8000>; interrupts = <GIC_SPI 45 IRQ_TYPE_LEVEL_HIGH 0>; - clocks = <&cru SCLK_MIPIDPHY_REF>, <&cru PCLK_MIPI_DSI0>, + clocks = <&cru SCLK_DPHY_PLL>, <&cru PCLK_MIPI_DSI0>, <&cru SCLK_DPHY_TX0_CFG>; clock-names = "ref", "pclk", "phy_cfg"; power-domains = <&power RK3399_PD_VIO>;
Hi Nickey,
Am Montag, 18. September 2017, 17:05:37 CEST schrieb Nickey Yang:
clk_24m --> Gate11[14] --> clk_mipidphy_ref --> Gate21[0] --> clk_dphy_pll
please try to be a bit more verbose in your commit messages :-) .
It looks to me, like this patch does not depend on the other ones and I can just pick it directly. Correct?
Heiko
Signed-off-by: Nickey Yang nickey.yang@rock-chips.com
arch/arm64/boot/dts/rockchip/rk3399.dtsi | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi b/arch/arm64/boot/dts/rockchip/rk3399.dtsi index d79e9b3..6aa43fd 100644 --- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi +++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi @@ -1629,7 +1629,7 @@ compatible = "rockchip,rk3399-mipi-dsi", "snps,dw-mipi-dsi"; reg = <0x0 0xff960000 0x0 0x8000>; interrupts = <GIC_SPI 45 IRQ_TYPE_LEVEL_HIGH 0>;
clocks = <&cru SCLK_MIPIDPHY_REF>, <&cru PCLK_MIPI_DSI0>,
clocks = <&cru SCLK_DPHY_PLL>, <&cru PCLK_MIPI_DSI0>, <&cru SCLK_DPHY_TX0_CFG>;
Hi Heiko,
On 2017年09月18日 19:31, Heiko Stübner wrote:
Hi Nickey,
Am Montag, 18. September 2017, 17:05:37 CEST schrieb Nickey Yang:
clk_24m --> Gate11[14] --> clk_mipidphy_ref --> Gate21[0] --> clk_dphy_pll
please try to be a bit more verbose in your commit messages :-) .
It looks to me, like this patch does not depend on the other ones and I can just pick it directly. Correct?
Yes, this patch is independent,you can pick it directly.
Heiko
Signed-off-by: Nickey Yang nickey.yang@rock-chips.com
arch/arm64/boot/dts/rockchip/rk3399.dtsi | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi b/arch/arm64/boot/dts/rockchip/rk3399.dtsi index d79e9b3..6aa43fd 100644 --- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi +++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi @@ -1629,7 +1629,7 @@ compatible = "rockchip,rk3399-mipi-dsi", "snps,dw-mipi-dsi"; reg = <0x0 0xff960000 0x0 0x8000>; interrupts = <GIC_SPI 45 IRQ_TYPE_LEVEL_HIGH 0>;
clocks = <&cru SCLK_MIPIDPHY_REF>, <&cru PCLK_MIPI_DSI0>,
clocks = <&cru SCLK_DPHY_PLL>, <&cru PCLK_MIPI_DSI0>, <&cru SCLK_DPHY_TX0_CFG>;
Am Montag, 18. September 2017, 17:05:37 CEST schrieb Nickey Yang:
clk_24m --> Gate11[14] --> clk_mipidphy_ref --> Gate21[0] --> clk_dphy_pll
Signed-off-by: Nickey Yang nickey.yang@rock-chips.com
applied as fix for 4.14 after polishing the commit message a bit
Thanks Heiko
Hi Nickey,
On Mon, Sep 18, 2017 at 05:05:33PM +0800, Nickey Yang wrote:
This patch correct Feedback divider setting: 1、Set Feedback divider [8:5] when HIGH_PROGRAM_EN 2、Due to the use of a "by 2 pre-scaler," the range of the feedback multiplication Feedback divider is limited to even division numbers, and Feedback divider must be greater than 12, less than 1000. 3、Make the previously configured Feedback divider(LSB) factors effective
Signed-off-by: Nickey Yang nickey.yang@rock-chips.com
drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 83 ++++++++++++++++++++++------------ 1 file changed, 54 insertions(+), 29 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c index 9a20b9d..52698b7 100644 --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c @@ -228,7 +228,7 @@ #define LOW_PROGRAM_EN 0 #define HIGH_PROGRAM_EN BIT(7) #define LOOP_DIV_LOW_SEL(val) (((val) - 1) & 0x1f) -#define LOOP_DIV_HIGH_SEL(val) ((((val) - 1) >> 5) & 0x1f) +#define LOOP_DIV_HIGH_SEL(val) ((((val) - 1) >> 5) & 0xf) #define PLL_LOOP_DIV_EN BIT(5) #define PLL_INPUT_DIV_EN BIT(4)
@@ -461,6 +461,7 @@ static int dw_mipi_dsi_phy_init(struct dw_mipi_dsi *dsi) dw_mipi_dsi_phy_write(dsi, 0x17, INPUT_DIVIDER(dsi->input_div)); dw_mipi_dsi_phy_write(dsi, 0x18, LOOP_DIV_LOW_SEL(dsi->feedback_div) | LOW_PROGRAM_EN);
- dw_mipi_dsi_phy_write(dsi, 0x19, PLL_LOOP_DIV_EN | PLL_INPUT_DIV_EN); dw_mipi_dsi_phy_write(dsi, 0x18, LOOP_DIV_HIGH_SEL(dsi->feedback_div) | HIGH_PROGRAM_EN); dw_mipi_dsi_phy_write(dsi, 0x19, PLL_LOOP_DIV_EN | PLL_INPUT_DIV_EN);
@@ -521,11 +522,16 @@ static int dw_mipi_dsi_phy_init(struct dw_mipi_dsi *dsi) static int dw_mipi_dsi_get_lane_bps(struct dw_mipi_dsi *dsi, struct drm_display_mode *mode) {
- unsigned int i, pre;
- unsigned long mpclk, pllref, tmp;
- unsigned int m = 1, n = 1, target_mbps = 1000;
unsigned long mpclk, tmp;
unsigned int target_mbps = 1000; unsigned int max_mbps = dptdin_map[ARRAY_SIZE(dptdin_map) - 1].max_mbps; int bpp;
unsigned long best_freq = 0;
unsigned long fvco_min, fvco_max, fin ,fout;
unsigned int min_prediv, max_prediv;
unsigned int _prediv, uninitialized_var(best_prediv);
unsigned long _fbdiv, uninitialized_var(best_fbdiv);
unsigned long min_delta = UINT_MAX;
bpp = mipi_dsi_pixel_format_to_bpp(dsi->format); if (bpp < 0) {
@@ -544,34 +550,53 @@ static int dw_mipi_dsi_get_lane_bps(struct dw_mipi_dsi *dsi, dev_err(dsi->dev, "DPHY clock frequency is out of range\n"); }
- pllref = DIV_ROUND_UP(clk_get_rate(dsi->pllref_clk), USEC_PER_SEC);
- tmp = pllref;
- /*
* The limits on the PLL divisor are:** 5MHz <= (pllref / n) <= 40MHz** we walk over these values in descreasing order so that if we hit* an exact match for target_mbps it is more likely that "m" will be* even.** TODO: ensure that "m" is even after this loop.*/- for (i = pllref / 5; i > (pllref / 40); i--) {
pre = pllref / i;if ((tmp > (target_mbps % pre)) && (target_mbps / pre < 512)) {tmp = target_mbps % pre;n = i;m = target_mbps / pre;
- fin = clk_get_rate(dsi->pllref_clk);
- fout = target_mbps * USEC_PER_SEC;
- /* constraint: 5Mhz < Fref / N < 40MHz */
- min_prediv = DIV_ROUND_UP(fin, 40 * USEC_PER_SEC);
- max_prediv = fin / (5 * USEC_PER_SEC);
- /* constraint: 80MHz < Fvco < 1500Mhz */
- fvco_min = 80 * USEC_PER_SEC;
- fvco_max = 1500 * USEC_PER_SEC;
- for (_prediv = min_prediv; _prediv <= max_prediv; _prediv++) {
u32 delta;/* Fvco = Fref * M / N */tmp = fout * _prediv;do_div(tmp, fin);_fbdiv = tmp;/** Due to the use of a "by 2 pre-scaler," the range of the* feedback multiplication value M is limited to even division* numbers, and m must be greater than 12, less than 1000.*/if (_fbdiv < 12 || _fbdiv > 1000)continue;if (_fbdiv % 2)++_fbdiv;tmp = (u64)_fbdiv * fin;
Are you relying on this being able to handle >32-bit integers? They you should declare 'tmp' as a 64-bit type; 'unsigned long' isn't guaranteed on 32-bit architectures. Try 'u64'?
Brian
do_div(tmp, _prediv);if (tmp < fvco_min || tmp > fvco_max)continue;delta = abs(fout - tmp);if (delta < min_delta) {best_prediv = _prediv;best_fbdiv = _fbdiv;min_delta = delta;best_freq = tmp;
if (tmp == 0)break;}
dsi->lane_mbps = pllref / n * m;
dsi->input_div = n;
dsi->feedback_div = m;
if (best_freq) {
dsi->lane_mbps = DIV_ROUND_UP(best_freq, USEC_PER_SEC);dsi->input_div = best_prediv;dsi->feedback_div = best_fbdiv;}
return 0;
}
1.9.1
On Mon, Sep 18, 2017 at 05:05:33PM +0800, Nickey Yang wrote:
This patch correct Feedback divider setting: 1、Set Feedback divider [8:5] when HIGH_PROGRAM_EN 2、Due to the use of a "by 2 pre-scaler," the range of the feedback multiplication Feedback divider is limited to even division numbers, and Feedback divider must be greater than 12, less than 1000. 3、Make the previously configured Feedback divider(LSB) factors effective
Signed-off-by: Nickey Yang nickey.yang@rock-chips.com
drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 83 ++++++++++++++++++++++------------ 1 file changed, 54 insertions(+), 29 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c index 9a20b9d..52698b7 100644 --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c @@ -228,7 +228,7 @@ #define LOW_PROGRAM_EN 0 #define HIGH_PROGRAM_EN BIT(7) #define LOOP_DIV_LOW_SEL(val) (((val) - 1) & 0x1f) -#define LOOP_DIV_HIGH_SEL(val) ((((val) - 1) >> 5) & 0x1f) +#define LOOP_DIV_HIGH_SEL(val) ((((val) - 1) >> 5) & 0xf) #define PLL_LOOP_DIV_EN BIT(5) #define PLL_INPUT_DIV_EN BIT(4)
@@ -461,6 +461,7 @@ static int dw_mipi_dsi_phy_init(struct dw_mipi_dsi *dsi) dw_mipi_dsi_phy_write(dsi, 0x17, INPUT_DIVIDER(dsi->input_div)); dw_mipi_dsi_phy_write(dsi, 0x18, LOOP_DIV_LOW_SEL(dsi->feedback_div) | LOW_PROGRAM_EN);
- dw_mipi_dsi_phy_write(dsi, 0x19, PLL_LOOP_DIV_EN | PLL_INPUT_DIV_EN);
You do the same write 2 lines down. Are both needed? It would be nice if the register names were also defined, so this is easier to read.
dw_mipi_dsi_phy_write(dsi, 0x18, LOOP_DIV_HIGH_SEL(dsi->feedback_div) | HIGH_PROGRAM_EN); dw_mipi_dsi_phy_write(dsi, 0x19, PLL_LOOP_DIV_EN | PLL_INPUT_DIV_EN); @@ -521,11 +522,16 @@ static int dw_mipi_dsi_phy_init(struct dw_mipi_dsi *dsi) static int dw_mipi_dsi_get_lane_bps(struct dw_mipi_dsi *dsi, struct drm_display_mode *mode) {
- unsigned int i, pre;
- unsigned long mpclk, pllref, tmp;
- unsigned int m = 1, n = 1, target_mbps = 1000;
- unsigned long mpclk, tmp;
- unsigned int target_mbps = 1000; unsigned int max_mbps = dptdin_map[ARRAY_SIZE(dptdin_map) - 1].max_mbps; int bpp;
- unsigned long best_freq = 0;
- unsigned long fvco_min, fvco_max, fin ,fout;
- unsigned int min_prediv, max_prediv;
- unsigned int _prediv, uninitialized_var(best_prediv);
- unsigned long _fbdiv, uninitialized_var(best_fbdiv);
- unsigned long min_delta = UINT_MAX;
Once you explicitly type these, make sure you use the appropriate _MAX value (ie: U64_MAX)
bpp = mipi_dsi_pixel_format_to_bpp(dsi->format); if (bpp < 0) { @@ -544,34 +550,53 @@ static int dw_mipi_dsi_get_lane_bps(struct dw_mipi_dsi *dsi, dev_err(dsi->dev, "DPHY clock frequency is out of range\n"); }
- pllref = DIV_ROUND_UP(clk_get_rate(dsi->pllref_clk), USEC_PER_SEC);
- tmp = pllref;
- /*
* The limits on the PLL divisor are:** 5MHz <= (pllref / n) <= 40MHz** we walk over these values in descreasing order so that if we hit* an exact match for target_mbps it is more likely that "m" will be* even.** TODO: ensure that "m" is even after this loop.*/- for (i = pllref / 5; i > (pllref / 40); i--) {
pre = pllref / i;if ((tmp > (target_mbps % pre)) && (target_mbps / pre < 512)) {tmp = target_mbps % pre;n = i;m = target_mbps / pre;
- fin = clk_get_rate(dsi->pllref_clk);
- fout = target_mbps * USEC_PER_SEC;
- /* constraint: 5Mhz < Fref / N < 40MHz */
- min_prediv = DIV_ROUND_UP(fin, 40 * USEC_PER_SEC);
- max_prediv = fin / (5 * USEC_PER_SEC);
- /* constraint: 80MHz < Fvco < 1500Mhz */
- fvco_min = 80 * USEC_PER_SEC;
- fvco_max = 1500 * USEC_PER_SEC;
- for (_prediv = min_prediv; _prediv <= max_prediv; _prediv++) {
u32 delta;/* Fvco = Fref * M / N */tmp = fout * _prediv;do_div(tmp, fin);_fbdiv = tmp;
Why use tmp at all? Can't you just use _fbdiv directly in do_div?
/** Due to the use of a "by 2 pre-scaler," the range of the* feedback multiplication value M is limited to even division* numbers, and m must be greater than 12, less than 1000.*/if (_fbdiv < 12 || _fbdiv > 1000)continue;if (_fbdiv % 2)++_fbdiv;
You can reduce this down to: _fbdiv += _fbdiv % 2;
tmp = (u64)_fbdiv * fin;
I'll echo Brian's concerns on type mixing here. Please be explicit with size when you declare your variables.
do_div(tmp, _prediv);if (tmp < fvco_min || tmp > fvco_max)continue;delta = abs(fout - tmp);if (delta < min_delta) {best_prediv = _prediv;best_fbdiv = _fbdiv;min_delta = delta;best_freq = tmp;
if (tmp == 0)break;}
dsi->lane_mbps = pllref / n * m;
dsi->input_div = n;
dsi->feedback_div = m;
- if (best_freq) {
Should you return an error or log something if best_freq is not found?
dsi->lane_mbps = DIV_ROUND_UP(best_freq, USEC_PER_SEC);dsi->input_div = best_prediv;dsi->feedback_div = best_fbdiv;}
return 0;
}
1.9.1
Hi Sean,
On Tue, Sep 19, 2017 at 11:00:25AM -0700, Sean Paul wrote:
On Mon, Sep 18, 2017 at 05:05:33PM +0800, Nickey Yang wrote:
This patch correct Feedback divider setting: 1、Set Feedback divider [8:5] when HIGH_PROGRAM_EN 2、Due to the use of a "by 2 pre-scaler," the range of the feedback multiplication Feedback divider is limited to even division numbers, and Feedback divider must be greater than 12, less than 1000. 3、Make the previously configured Feedback divider(LSB) factors effective
Signed-off-by: Nickey Yang nickey.yang@rock-chips.com
drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 83 ++++++++++++++++++++++------------ 1 file changed, 54 insertions(+), 29 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c index 9a20b9d..52698b7 100644 --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c @@ -228,7 +228,7 @@ #define LOW_PROGRAM_EN 0 #define HIGH_PROGRAM_EN BIT(7) #define LOOP_DIV_LOW_SEL(val) (((val) - 1) & 0x1f) -#define LOOP_DIV_HIGH_SEL(val) ((((val) - 1) >> 5) & 0x1f) +#define LOOP_DIV_HIGH_SEL(val) ((((val) - 1) >> 5) & 0xf) #define PLL_LOOP_DIV_EN BIT(5) #define PLL_INPUT_DIV_EN BIT(4)
@@ -461,6 +461,7 @@ static int dw_mipi_dsi_phy_init(struct dw_mipi_dsi *dsi) dw_mipi_dsi_phy_write(dsi, 0x17, INPUT_DIVIDER(dsi->input_div)); dw_mipi_dsi_phy_write(dsi, 0x18, LOOP_DIV_LOW_SEL(dsi->feedback_div) | LOW_PROGRAM_EN);
- dw_mipi_dsi_phy_write(dsi, 0x19, PLL_LOOP_DIV_EN | PLL_INPUT_DIV_EN);
You do the same write 2 lines down. Are both needed? It would be nice if the register names were also defined, so this is easier to read.
If I'm reading correctly, I think this is what Nickey meant by:
"3、Make the previously configured Feedback divider(LSB) factors effective"
. My reading of the databook is that this step finalizes the previous two writes (to test code 0x17 and 0x18).
Given this was buggy (?) previously, it does seem like having some extra language to document this could help. Register names (or "test codes", per the docs?) could help, but additionally, maybe a few more comments.
dw_mipi_dsi_phy_write(dsi, 0x18, LOOP_DIV_HIGH_SEL(dsi->feedback_div) | HIGH_PROGRAM_EN); dw_mipi_dsi_phy_write(dsi, 0x19, PLL_LOOP_DIV_EN | PLL_INPUT_DIV_EN);
[...]
Brian
On Tue, Sep 19, 2017 at 11:19:01AM -0700, Brian Norris wrote:
Hi Sean,
On Tue, Sep 19, 2017 at 11:00:25AM -0700, Sean Paul wrote:
On Mon, Sep 18, 2017 at 05:05:33PM +0800, Nickey Yang wrote:
This patch correct Feedback divider setting: 1、Set Feedback divider [8:5] when HIGH_PROGRAM_EN 2、Due to the use of a "by 2 pre-scaler," the range of the feedback multiplication Feedback divider is limited to even division numbers, and Feedback divider must be greater than 12, less than 1000. 3、Make the previously configured Feedback divider(LSB) factors effective
Signed-off-by: Nickey Yang nickey.yang@rock-chips.com
drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 83 ++++++++++++++++++++++------------ 1 file changed, 54 insertions(+), 29 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c index 9a20b9d..52698b7 100644 --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c @@ -228,7 +228,7 @@ #define LOW_PROGRAM_EN 0 #define HIGH_PROGRAM_EN BIT(7) #define LOOP_DIV_LOW_SEL(val) (((val) - 1) & 0x1f) -#define LOOP_DIV_HIGH_SEL(val) ((((val) - 1) >> 5) & 0x1f) +#define LOOP_DIV_HIGH_SEL(val) ((((val) - 1) >> 5) & 0xf) #define PLL_LOOP_DIV_EN BIT(5) #define PLL_INPUT_DIV_EN BIT(4)
@@ -461,6 +461,7 @@ static int dw_mipi_dsi_phy_init(struct dw_mipi_dsi *dsi) dw_mipi_dsi_phy_write(dsi, 0x17, INPUT_DIVIDER(dsi->input_div)); dw_mipi_dsi_phy_write(dsi, 0x18, LOOP_DIV_LOW_SEL(dsi->feedback_div) | LOW_PROGRAM_EN);
- dw_mipi_dsi_phy_write(dsi, 0x19, PLL_LOOP_DIV_EN | PLL_INPUT_DIV_EN);
You do the same write 2 lines down. Are both needed? It would be nice if the register names were also defined, so this is easier to read.
If I'm reading correctly, I think this is what Nickey meant by:
"3、Make the previously configured Feedback divider(LSB) factors effective"
. My reading of the databook is that this step finalizes the previous two writes (to test code 0x17 and 0x18).
Given this was buggy (?) previously, it does seem like having some extra language to document this could help. Register names (or "test codes", per the docs?) could help, but additionally, maybe a few more comments.
Ah, yeah, thanks for the explanation. It's not clear that this latches the values above. I think register names and comments would be immensely helpful.
Sean
dw_mipi_dsi_phy_write(dsi, 0x18, LOOP_DIV_HIGH_SEL(dsi->feedback_div) | HIGH_PROGRAM_EN); dw_mipi_dsi_phy_write(dsi, 0x19, PLL_LOOP_DIV_EN | PLL_INPUT_DIV_EN);
[...]
Brian
On Tue, Sep 19, 2017 at 01:27:40PM -0700, Sean Paul wrote:
On Tue, Sep 19, 2017 at 11:19:01AM -0700, Brian Norris wrote:
Hi Sean,
On Tue, Sep 19, 2017 at 11:00:25AM -0700, Sean Paul wrote:
On Mon, Sep 18, 2017 at 05:05:33PM +0800, Nickey Yang wrote:
This patch correct Feedback divider setting: 1、Set Feedback divider [8:5] when HIGH_PROGRAM_EN 2、Due to the use of a "by 2 pre-scaler," the range of the feedback multiplication Feedback divider is limited to even division numbers, and Feedback divider must be greater than 12, less than 1000. 3、Make the previously configured Feedback divider(LSB) factors effective
Signed-off-by: Nickey Yang nickey.yang@rock-chips.com
drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 83 ++++++++++++++++++++++------------ 1 file changed, 54 insertions(+), 29 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c index 9a20b9d..52698b7 100644 --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c @@ -228,7 +228,7 @@ #define LOW_PROGRAM_EN 0 #define HIGH_PROGRAM_EN BIT(7) #define LOOP_DIV_LOW_SEL(val) (((val) - 1) & 0x1f) -#define LOOP_DIV_HIGH_SEL(val) ((((val) - 1) >> 5) & 0x1f) +#define LOOP_DIV_HIGH_SEL(val) ((((val) - 1) >> 5) & 0xf) #define PLL_LOOP_DIV_EN BIT(5) #define PLL_INPUT_DIV_EN BIT(4)
@@ -461,6 +461,7 @@ static int dw_mipi_dsi_phy_init(struct dw_mipi_dsi *dsi) dw_mipi_dsi_phy_write(dsi, 0x17, INPUT_DIVIDER(dsi->input_div)); dw_mipi_dsi_phy_write(dsi, 0x18, LOOP_DIV_LOW_SEL(dsi->feedback_div) | LOW_PROGRAM_EN);
- dw_mipi_dsi_phy_write(dsi, 0x19, PLL_LOOP_DIV_EN | PLL_INPUT_DIV_EN);
You do the same write 2 lines down. Are both needed? It would be nice if the register names were also defined, so this is easier to read.
If I'm reading correctly, I think this is what Nickey meant by:
"3、Make the previously configured Feedback divider(LSB) factors effective"
. My reading of the databook is that this step finalizes the previous two writes (to test code 0x17 and 0x18).
Given this was buggy (?) previously, it does seem like having some extra language to document this could help. Register names (or "test codes", per the docs?) could help, but additionally, maybe a few more comments.
Ah, yeah, thanks for the explanation. It's not clear that this latches the values above. I think register names and comments would be immensely helpful.
According to the databook, 0x19 controls whether the loop/input dividers are derived from the hsfreqrange configuration or use the values in 0x17 and 0x18. I can't see why writing the same value to this register multiple times is necessary.
dw_mipi_dsi_phy_write(dsi, 0x18, LOOP_DIV_HIGH_SEL(dsi->feedback_div) | HIGH_PROGRAM_EN); dw_mipi_dsi_phy_write(dsi, 0x19, PLL_LOOP_DIV_EN | PLL_INPUT_DIV_EN);
[...]
On Wednesday, September 20, 2017 06:08 PM, John Keeping wrote:
On Tue, Sep 19, 2017 at 01:27:40PM -0700, Sean Paul wrote:
On Tue, Sep 19, 2017 at 11:19:01AM -0700, Brian Norris wrote:
Hi Sean,
On Tue, Sep 19, 2017 at 11:00:25AM -0700, Sean Paul wrote:
On Mon, Sep 18, 2017 at 05:05:33PM +0800, Nickey Yang wrote:
This patch correct Feedback divider setting: 1、Set Feedback divider [8:5] when HIGH_PROGRAM_EN 2、Due to the use of a "by 2 pre-scaler," the range of the feedback multiplication Feedback divider is limited to even division numbers, and Feedback divider must be greater than 12, less than 1000. 3、Make the previously configured Feedback divider(LSB) factors effective
Signed-off-by: Nickey Yang nickey.yang@rock-chips.com
drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 83 ++++++++++++++++++++++------------ 1 file changed, 54 insertions(+), 29 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c index 9a20b9d..52698b7 100644 --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c @@ -228,7 +228,7 @@ #define LOW_PROGRAM_EN 0 #define HIGH_PROGRAM_EN BIT(7) #define LOOP_DIV_LOW_SEL(val) (((val) - 1) & 0x1f) -#define LOOP_DIV_HIGH_SEL(val) ((((val) - 1) >> 5) & 0x1f) +#define LOOP_DIV_HIGH_SEL(val) ((((val) - 1) >> 5) & 0xf) #define PLL_LOOP_DIV_EN BIT(5) #define PLL_INPUT_DIV_EN BIT(4)
@@ -461,6 +461,7 @@ static int dw_mipi_dsi_phy_init(struct dw_mipi_dsi *dsi) dw_mipi_dsi_phy_write(dsi, 0x17, INPUT_DIVIDER(dsi->input_div)); dw_mipi_dsi_phy_write(dsi, 0x18, LOOP_DIV_LOW_SEL(dsi->feedback_div) | LOW_PROGRAM_EN);
- dw_mipi_dsi_phy_write(dsi, 0x19, PLL_LOOP_DIV_EN | PLL_INPUT_DIV_EN);
You do the same write 2 lines down. Are both needed? It would be nice if the register names were also defined, so this is easier to read.
If I'm reading correctly, I think this is what Nickey meant by:
"3、Make the previously configured Feedback divider(LSB) factors effective"
. My reading of the databook is that this step finalizes the previous two writes (to test code 0x17 and 0x18).
Given this was buggy (?) previously, it does seem like having some extra language to document this could help. Register names (or "test codes", per the docs?) could help, but additionally, maybe a few more comments.
Ah, yeah, thanks for the explanation. It's not clear that this latches the values above. I think register names and comments would be immensely helpful.
According to the databook, 0x19 controls whether the loop/input dividers are derived from the hsfreqrange configuration or use the values in 0x17 and 0x18. I can't see why writing the same value to this register multiple times is necessary.
According to databook, set 0x19 to 0x30 make the previously configured N(0x17) and M(0x18) factors effective, 0x18 need to be setted by twice, since we need to set 0x18 LSB and MSB separately, As we test, after set 0x18 LSB, if we do not set 0x19 immediately to make the configrued effective, when we set 0x18 MSB, the 0x18 LSB value will gone, and we will get wrong pll frequency. Anyway, I think should add some comment here to clear that.
dw_mipi_dsi_phy_write(dsi, 0x18, LOOP_DIV_HIGH_SEL(dsi->feedback_div) | HIGH_PROGRAM_EN); dw_mipi_dsi_phy_write(dsi, 0x19, PLL_LOOP_DIV_EN | PLL_INPUT_DIV_EN);
[...]
Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip
On Wed, Sep 20, 2017 at 07:08:11PM +0800, hl wrote:
On Wednesday, September 20, 2017 06:08 PM, John Keeping wrote:
On Tue, Sep 19, 2017 at 01:27:40PM -0700, Sean Paul wrote:
On Tue, Sep 19, 2017 at 11:19:01AM -0700, Brian Norris wrote:
Hi Sean,
On Tue, Sep 19, 2017 at 11:00:25AM -0700, Sean Paul wrote:
On Mon, Sep 18, 2017 at 05:05:33PM +0800, Nickey Yang wrote:
This patch correct Feedback divider setting: 1、Set Feedback divider [8:5] when HIGH_PROGRAM_EN 2、Due to the use of a "by 2 pre-scaler," the range of the feedback multiplication Feedback divider is limited to even division numbers, and Feedback divider must be greater than 12, less than 1000. 3、Make the previously configured Feedback divider(LSB) factors effective
Signed-off-by: Nickey Yang nickey.yang@rock-chips.com
drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 83 ++++++++++++++++++++++------------ 1 file changed, 54 insertions(+), 29 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c index 9a20b9d..52698b7 100644 --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c @@ -228,7 +228,7 @@ #define LOW_PROGRAM_EN 0 #define HIGH_PROGRAM_EN BIT(7) #define LOOP_DIV_LOW_SEL(val) (((val) - 1) & 0x1f) -#define LOOP_DIV_HIGH_SEL(val) ((((val) - 1) >> 5) & 0x1f) +#define LOOP_DIV_HIGH_SEL(val) ((((val) - 1) >> 5) & 0xf) #define PLL_LOOP_DIV_EN BIT(5) #define PLL_INPUT_DIV_EN BIT(4)
@@ -461,6 +461,7 @@ static int dw_mipi_dsi_phy_init(struct dw_mipi_dsi *dsi) dw_mipi_dsi_phy_write(dsi, 0x17, INPUT_DIVIDER(dsi->input_div)); dw_mipi_dsi_phy_write(dsi, 0x18, LOOP_DIV_LOW_SEL(dsi->feedback_div) | LOW_PROGRAM_EN);
- dw_mipi_dsi_phy_write(dsi, 0x19, PLL_LOOP_DIV_EN | PLL_INPUT_DIV_EN);
You do the same write 2 lines down. Are both needed? It would be nice if the register names were also defined, so this is easier to read.
If I'm reading correctly, I think this is what Nickey meant by:
"3、Make the previously configured Feedback divider(LSB) factors effective"
. My reading of the databook is that this step finalizes the previous two writes (to test code 0x17 and 0x18).
Given this was buggy (?) previously, it does seem like having some extra language to document this could help. Register names (or "test codes", per the docs?) could help, but additionally, maybe a few more comments.
Ah, yeah, thanks for the explanation. It's not clear that this latches the values above. I think register names and comments would be immensely helpful.
According to the databook, 0x19 controls whether the loop/input dividers are derived from the hsfreqrange configuration or use the values in 0x17 and 0x18. I can't see why writing the same value to this register multiple times is necessary.
According to databook, set 0x19 to 0x30 make the previously configured N(0x17) and M(0x18) factors effective, 0x18 need to be setted by twice, since we need to set 0x18 LSB and MSB separately, As we test, after set 0x18 LSB, if we do not set 0x19 immediately to make the configrued effective, when we set 0x18 MSB, the 0x18 LSB value will gone, and we will get wrong pll frequency. Anyway, I think should add some comment here to clear that.
That's surprising, the examples in sections 6.2.1 and 6.2.2 of the databook I have both show the high and low parts of 0x18 being written before 0x19 is set.
When reading 0x18 are you setting bit 7 in TESTDIN correctly in order to select the correct bits of the feedback divider?
dw_mipi_dsi_phy_write(dsi, 0x18, LOOP_DIV_HIGH_SEL(dsi->feedback_div) | HIGH_PROGRAM_EN); dw_mipi_dsi_phy_write(dsi, 0x19, PLL_LOOP_DIV_EN | PLL_INPUT_DIV_EN);
[...]
Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip
Hi Nickey,
El Mon, Sep 18, 2017 at 05:05:33PM +0800 Nickey Yang ha dit:
This patch correct Feedback divider setting: 1、Set Feedback divider [8:5] when HIGH_PROGRAM_EN 2、Due to the use of a "by 2 pre-scaler," the range of the feedback multiplication Feedback divider is limited to even division numbers, and Feedback divider must be greater than 12, less than 1000. 3、Make the previously configured Feedback divider(LSB) factors effective
Signed-off-by: Nickey Yang nickey.yang@rock-chips.com
drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 83 ++++++++++++++++++++++------------ 1 file changed, 54 insertions(+), 29 deletions(-)
diff --git a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c index 9a20b9d..52698b7 100644 --- a/drivers/gpu/drm/rockchip/dw-mipi-dsi.c +++ b/drivers/gpu/drm/rockchip/dw-mipi-dsi.c @@ -228,7 +228,7 @@ #define LOW_PROGRAM_EN 0 #define HIGH_PROGRAM_EN BIT(7) #define LOOP_DIV_LOW_SEL(val) (((val) - 1) & 0x1f) -#define LOOP_DIV_HIGH_SEL(val) ((((val) - 1) >> 5) & 0x1f) +#define LOOP_DIV_HIGH_SEL(val) ((((val) - 1) >> 5) & 0xf) #define PLL_LOOP_DIV_EN BIT(5) #define PLL_INPUT_DIV_EN BIT(4)
@@ -461,6 +461,7 @@ static int dw_mipi_dsi_phy_init(struct dw_mipi_dsi *dsi) dw_mipi_dsi_phy_write(dsi, 0x17, INPUT_DIVIDER(dsi->input_div)); dw_mipi_dsi_phy_write(dsi, 0x18, LOOP_DIV_LOW_SEL(dsi->feedback_div) | LOW_PROGRAM_EN);
- dw_mipi_dsi_phy_write(dsi, 0x19, PLL_LOOP_DIV_EN | PLL_INPUT_DIV_EN); dw_mipi_dsi_phy_write(dsi, 0x18, LOOP_DIV_HIGH_SEL(dsi->feedback_div) | HIGH_PROGRAM_EN); dw_mipi_dsi_phy_write(dsi, 0x19, PLL_LOOP_DIV_EN | PLL_INPUT_DIV_EN);
@@ -521,11 +522,16 @@ static int dw_mipi_dsi_phy_init(struct dw_mipi_dsi *dsi) static int dw_mipi_dsi_get_lane_bps(struct dw_mipi_dsi *dsi, struct drm_display_mode *mode) {
- unsigned int i, pre;
- unsigned long mpclk, pllref, tmp;
- unsigned int m = 1, n = 1, target_mbps = 1000;
- unsigned long mpclk, tmp;
- unsigned int target_mbps = 1000; unsigned int max_mbps = dptdin_map[ARRAY_SIZE(dptdin_map) - 1].max_mbps; int bpp;
- unsigned long best_freq = 0;
- unsigned long fvco_min, fvco_max, fin ,fout;
nit: should be 'fin, fout'.
unsigned int min_prediv, max_prediv;
unsigned int _prediv, uninitialized_var(best_prediv);
unsigned long _fbdiv, uninitialized_var(best_fbdiv);
unsigned long min_delta = UINT_MAX;
bpp = mipi_dsi_pixel_format_to_bpp(dsi->format); if (bpp < 0) {
@@ -544,34 +550,53 @@ static int dw_mipi_dsi_get_lane_bps(struct dw_mipi_dsi *dsi, dev_err(dsi->dev, "DPHY clock frequency is out of range\n"); }
- pllref = DIV_ROUND_UP(clk_get_rate(dsi->pllref_clk), USEC_PER_SEC);
- tmp = pllref;
- /*
* The limits on the PLL divisor are:** 5MHz <= (pllref / n) <= 40MHz** we walk over these values in descreasing order so that if we hit* an exact match for target_mbps it is more likely that "m" will be* even.** TODO: ensure that "m" is even after this loop.*/- for (i = pllref / 5; i > (pllref / 40); i--) {
pre = pllref / i;if ((tmp > (target_mbps % pre)) && (target_mbps / pre < 512)) {tmp = target_mbps % pre;n = i;m = target_mbps / pre;
- fin = clk_get_rate(dsi->pllref_clk);
- fout = target_mbps * USEC_PER_SEC;
- /* constraint: 5Mhz < Fref / N < 40MHz */
According to the previous comment above it should be '<=' instead of '<'.
- min_prediv = DIV_ROUND_UP(fin, 40 * USEC_PER_SEC);
- max_prediv = fin / (5 * USEC_PER_SEC);
- /* constraint: 80MHz < Fvco < 1500Mhz */
Same here.
- fvco_min = 80 * USEC_PER_SEC;
- fvco_max = 1500 * USEC_PER_SEC;
- for (_prediv = min_prediv; _prediv <= max_prediv; _prediv++) {
u32 delta;/* Fvco = Fref * M / N */tmp = fout * _prediv;do_div(tmp, fin);_fbdiv = tmp;/** Due to the use of a "by 2 pre-scaler," the range of the* feedback multiplication value M is limited to even division* numbers, and m must be greater than 12, less than 1000.*/
I didn't encounter the second part of the constraint in my version of the databook, judging from the code below it seems the comment should be "12 <= m <= 1000".
if (_fbdiv < 12 || _fbdiv > 1000)continue;if (_fbdiv % 2)++_fbdiv;tmp = (u64)_fbdiv * fin;do_div(tmp, _prediv);if (tmp < fvco_min || tmp > fvco_max)continue;delta = abs(fout - tmp);if (delta < min_delta) {best_prediv = _prediv;best_fbdiv = _fbdiv;min_delta = delta;best_freq = tmp;
if (tmp == 0)break;}
dsi->lane_mbps = pllref / n * m;
dsi->input_div = n;
dsi->feedback_div = m;
if (best_freq) {
dsi->lane_mbps = DIV_ROUND_UP(best_freq, USEC_PER_SEC);dsi->input_div = best_prediv;dsi->feedback_div = best_fbdiv;}
return 0;
}
dri-devel@lists.freedesktop.org
-
Brian Norris -
Heiko Stübner -
hl -
John Keeping -
Matthias Kaehlcke -
Nickey Yang -
Sean Paul