On Thu, 13 Feb 2014, Shobhit Kumar <shobhit.kumar@xxxxxxxxx> wrote:
> MIPI Block #52 which provides configuration details for the MIPI panel
> including dphy settings as per panel and tcon specs
>
> Block #53 gives information on panel enable sequences
>
> Signed-off-by: Shobhit Kumar <shobhit.kumar@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/intel_bios.c | 5 +-
> drivers/gpu/drm/i915/intel_bios.h | 152 +++++++++++++++++++++++++++++++-------
> drivers/gpu/drm/i915/intel_dsi.h | 2 +
> 3 files changed, 130 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
> index 86b95ca..cf0b25d 100644
> --- a/drivers/gpu/drm/i915/intel_bios.c
> +++ b/drivers/gpu/drm/i915/intel_bios.c
> @@ -30,6 +30,7 @@
> #include <drm/i915_drm.h>
> #include "i915_drv.h"
> #include "intel_bios.h"
> +#include "intel_dsi.h"
Hmm, I think I'd like it better if the interface from bios to elsewhere
was defined in intel_bios.h, i.e. move the required #defines from
intel_dsi.h to intel_bios.h.
>
> #define SLAVE_ADDR1 0x70
> #define SLAVE_ADDR2 0x72
> @@ -599,14 +600,14 @@ parse_mipi(struct drm_i915_private *dev_priv, struct bdb_header *bdb)
> {
> struct bdb_mipi *mipi;
>
> - mipi = find_section(bdb, BDB_MIPI);
> + mipi = find_section(bdb, BDB_MIPI_CONFIG);
> if (!mipi) {
> DRM_DEBUG_KMS("No MIPI BDB found");
> return;
> }
>
> /* XXX: add more info */
> - dev_priv->vbt.dsi.panel_id = mipi->panel_id;
> + dev_priv->vbt.dsi.panel_id = MIPI_DSI_GENERIC_PANEL_ID;
> }
>
> static void parse_ddi_port(struct drm_i915_private *dev_priv, enum port port,
> diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
> index 282de5e..8345e0e 100644
> --- a/drivers/gpu/drm/i915/intel_bios.h
> +++ b/drivers/gpu/drm/i915/intel_bios.h
> @@ -104,7 +104,8 @@ struct vbios_data {
> #define BDB_LVDS_LFP_DATA 42
> #define BDB_LVDS_BACKLIGHT 43
> #define BDB_LVDS_POWER 44
> -#define BDB_MIPI 50
> +#define BDB_MIPI_CONFIG 52
> +#define BDB_MIPI_SEQUENCE 53
> #define BDB_SKIP 254 /* VBIOS private block, ignore */
>
> struct bdb_general_features {
> @@ -711,44 +712,141 @@ int intel_parse_bios(struct drm_device *dev);
> #define DVO_PORT_DPD 9
> #define DVO_PORT_DPA 10
>
> -/* MIPI DSI panel info */
> -struct bdb_mipi {
> +/* Block 52 contains MIPI Panel info
> + * 6 such enteries will there. Index into correct
> + * entery is based on the panel_index in #40 LFP
> + */
> +#define MAX_MIPI_CONFIGURATIONS 6
> +struct mipi_config {
> u16 panel_id;
> - u16 bridge_revision;
>
> - /* General params */
> + /* General Params */
> u32 dithering:1;
> - u32 bpp_pixel_format:1;
> u32 rsvd1:1;
> - u32 dphy_valid:1;
> - u32 resvd2:28;
> -
> - u16 port_info;
> - u16 rsvd3:2;
> - u16 num_lanes:2;
> - u16 rsvd4:12;
> -
> - /* DSI config */
> - u16 virt_ch_num:2;
> - u16 vtm:2;
> - u16 rsvd5:12;
> + u32 panel_type:1;
> + u32 panel_arch_type:2;
> + u32 cmd_mode:1;
> + u32 vtm:2;
> + u32 cabc:1;
> + u32 pwm_blc:1;
> +
> + /* Bit 13:10
> + * 000 - Reserved, 001 - RGB565, 002 - RGB666,
> + * 011 - RGB666Loosely packed, 100 - RGB888,
> + * others - rsvd
> + */
> + u32 videomode_color_format:4;
>
> - u32 dsi_clock;
> + /* Bit 15:14
> + * 0 - No rotation, 1 - 90 degree
> + * 2 - 180 degree, 3 - 270 degree
> + */
> + u32 rotation:2;
> + u32 bta:1;
> + u32 rsvd2:15;
> +
> + /* 2 byte Port Description */
> + u16 dual_link:2;
> + u16 lane_cnt:2;
> + u16 rsvd3:12;
> +
> + /* 2 byte DSI COntroller params */
> + /* 0 - Using DSI PHY, 1 - TE usage */
> + u16 dsi_usage:1;
> + u16 rsvd4:15;
> +
> + u8 rsvd5[5];
> + u32 dsi_ddr_clk;
> u32 bridge_ref_clk;
> - u16 rsvd_pwr;
>
> - /* Dphy Params */
> - u32 prepare_cnt:5;
> - u32 rsvd6:3;
> + u8 byte_clk_sel:2;
> + u8 rsvd6:6;
Per the spec you sent me, there's one more reserved byte here.
> +
> + /* DPHY Flags */
> + u16 dphy_param_valid:1;
> + u16 eot_disabled:1;
> + u16 clk_stop:1;
> + u16 rsvd7:13;
> +
> + u32 hs_tx_timeout;
> + u32 lp_rx_timeout;
> + u32 turn_around_timeout;
> + u32 device_reset_timer;
> + u32 master_init_timer;
> + u32 dbi_bw_timer;
> + u32 lp_byte_clk_val;
> +
> + /* 4 byte Dphy Params */
> + u32 prepare_cnt:6;
> + u32 rsvd8:2;
Per the spec you sent me, prepare_cnt is 5 bits, reserved 3 bits.
> u32 clk_zero_cnt:8;
> u32 trail_cnt:5;
> - u32 rsvd7:3;
> + u32 rsvd9:3;
> u32 exit_zero_cnt:6;
> - u32 rsvd8:2;
> + u32 rsvd10:2;
For this reserved field the spec is clearly bogus...
>
> - u32 hl_switch_cnt;
> - u32 lp_byte_clk;
> u32 clk_lane_switch_cnt;
> + u32 hl_switch_cnt;
> +
> + u32 rsvd11[6];
> +
> + /* timings based on dphy spec */
> + u8 tclk_miss;
> + u8 tclk_post;
> + u8 rsvd12;
> + u8 tclk_pre;
> + u8 tclk_prepare;
> + u8 tclk_settle;
> + u8 tclk_term_enable;
> + u8 tclk_trail;
> + u16 tclk_prepare_clkzero;
> + u8 rsvd13;
> + u8 td_term_enable;
> + u8 teot;
> + u8 ths_exit;
> + u8 ths_prepare;
> + u16 ths_prepare_hszero;
> + u8 rsvd14;
> + u8 ths_settle;
> + u8 ths_skip;
> + u8 ths_trail;
> + u8 tinit;
> + u8 tlpx;
> + u8 rsvd15[3];
Per the spec you sent me, there's 1 byte reserved, and 5 bytes of GPIO
indexes below.
All in all, the size of the struct is different from the spec, shifting
everything for panel_type > 0. Which one is wrong?
> +
> + /* GPIOs */
> + u8 panel_enable;
> + u8 bl_enable;
> + u8 pwm_enable;
> + u8 reset_r_n;
> + u8 pwr_down_r;
> + u8 stdby_r_n;
> +
> } __packed;
All around I would like it if the field names were slightly more
descriptive by themselves, particularly for the most important ones,
with #defines for the values in some cases. For example panel_type above
could clearly be "is_bridge" or similar (now it's confusing with the
panel_type in intel_bios.c). Same for any booleans that could be
expressed as "is_something" or "something_enabled". Color formats and
rotations could have defines, so you wouldn't need to add comments for
them at all. Same for byte_clk_sel.
I definitely do *not* mean you should rewrite all of them. If you want,
I can reply with detailed suggestions on a per field basis.
>
> +struct bdb_mipi_config {
> + struct mipi_config config[0];
Why isn't this struct:
struct mipi_config config[MAX_MIPI_CONFIGURATIONS];
struct mipi_pps_data pps[MAX_MIPI_CONFIGURATIONS];
and you could replace this ugly bit in patch 2/2:
+ pps = (struct mipi_pps_data *) &start->config[MAX_MIPI_CONFIGURATIONS];
with:
+ pps = &start->pps[panel_type];
> +};
> +
> +/* Block 52 contains MIPI configuration block
> + * 6 * bdb_mipi_config, followed by 6 pps data
> + * block below
> + */
> +struct mipi_pps_data {
> + u16 panel_on_delay;
> + u16 bl_enable_delay;
> + u16 bl_disable_delay;
> + u16 panel_off_delay;
> + u16 panel_power_cycle_delay;
> +};
I'd like the unconventional units mentioned in a comment.
> +
> +/* Block 53 contains MIPI sequences as needed by the panel
> + * for enabling it. This block can be variable in size and
> + * can be maximum of 6 blocks
> + */
> +struct bdb_mipi_sequence {
> + u8 version;
> + void *data;
> +};
> +
> #endif /* _I830_BIOS_H_ */
> diff --git a/drivers/gpu/drm/i915/intel_dsi.h b/drivers/gpu/drm/i915/intel_dsi.h
> index b4a27ce..a0e42db 100644
> --- a/drivers/gpu/drm/i915/intel_dsi.h
> +++ b/drivers/gpu/drm/i915/intel_dsi.h
> @@ -120,4 +120,6 @@ static inline struct intel_dsi *enc_to_intel_dsi(struct drm_encoder *encoder)
> extern void vlv_enable_dsi_pll(struct intel_encoder *encoder);
> extern void vlv_disable_dsi_pll(struct intel_encoder *encoder);
>
> +#define MIPI_DSI_GENERIC_PANEL_ID 1
> +
> #endif /* _INTEL_DSI_H */
> --
> 1.8.3.2
>
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
