| Message ID | 20190322032901.12045-1-andrew.smirnov@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | tc358767 driver improvements | expand |
On Fri, Mar 22, 2019 at 3:19 AM Tomi Valkeinen <tomi.valkeinen@ti.com> wrote: > > On 22/03/2019 05:28, Andrey Smirnov wrote: > > Simplify tc_set_video_mode() by replacing repreated calls to > > tc_write()/regmap_write() with a single call to > > regmap_multi_reg_write(). While at it, simplify explicit shifting by > > using macros from <linux/bitfield.h>. No functional change intended. > > > > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> > > Cc: Archit Taneja <architt@codeaurora.org> > > Cc: Andrzej Hajda <a.hajda@samsung.com> > > Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com> > > Cc: Tomi Valkeinen <tomi.valkeinen@ti.com> > > Cc: Andrey Gusakov <andrey.gusakov@cogentembedded.com> > > Cc: Philipp Zabel <p.zabel@pengutronix.de> > > Cc: Chris Healy <cphealy@gmail.com> > > Cc: Lucas Stach <l.stach@pengutronix.de> > > Cc: dri-devel@lists.freedesktop.org > > Cc: linux-kernel@vger.kernel.org > > --- > > drivers/gpu/drm/bridge/tc358767.c | 146 +++++++++++++++++------------- > > 1 file changed, 85 insertions(+), 61 deletions(-) > > > > diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c > > index 38d542f553cd..d99c9f32a133 100644 > > --- a/drivers/gpu/drm/bridge/tc358767.c > > +++ b/drivers/gpu/drm/bridge/tc358767.c > > @@ -24,6 +24,7 @@ > > * GNU General Public License for more details. > > */ > > > > +#include <linux/bitfield.h> > > #include <linux/clk.h> > > #include <linux/device.h> > > #include <linux/gpio/consumer.h> > > @@ -56,6 +57,7 @@ > > > > /* Video Path */ > > #define VPCTRL0 0x0450 > > +#define VSDELAY GENMASK(31, 20) > > #define OPXLFMT_RGB666 (0 << 8) > > #define OPXLFMT_RGB888 (1 << 8) > > #define FRMSYNC_DISABLED (0 << 4) /* Video Timing Gen Disabled */ > > @@ -63,9 +65,17 @@ > > #define MSF_DISABLED (0 << 0) /* Magic Square FRC disabled */ > > #define MSF_ENABLED (1 << 0) /* Magic Square FRC enabled */ > > #define HTIM01 0x0454 > > +#define HPW GENMASK(8, 0) > > +#define HBPR GENMASK(24, 16) > > #define HTIM02 0x0458 > > +#define HDISPR GENMASK(10, 0) > > +#define HFPR GENMASK(24, 16) > > #define VTIM01 0x045c > > +#define VSPR GENMASK(7, 0) > > +#define VBPR GENMASK(23, 16) > > #define VTIM02 0x0460 > > +#define VFPR GENMASK(23, 16) > > +#define VDISPR GENMASK(10, 0) > > #define VFUEN0 0x0464 > > #define VFUEN BIT(0) /* Video Frame Timing Upload */ > > > > @@ -100,14 +110,28 @@ > > /* Main Channel */ > > #define DP0_SECSAMPLE 0x0640 > > #define DP0_VIDSYNCDELAY 0x0644 > > +#define VID_SYNC_DLY GENMASK(15, 0) > > +#define THRESH_DLY GENMASK(31, 16) > > + > > #define DP0_TOTALVAL 0x0648 > > +#define H_TOTAL GENMASK(15, 0) > > +#define V_TOTAL GENMASK(31, 16) > > #define DP0_STARTVAL 0x064c > > +#define H_START GENMASK(15, 0) > > +#define V_START GENMASK(31, 16) > > #define DP0_ACTIVEVAL 0x0650 > > +#define H_ACT GENMASK(15, 0) > > +#define V_ACT GENMASK(31, 16) > > + > > #define DP0_SYNCVAL 0x0654 > > +#define VS_WIDTH GENMASK(30, 16) > > +#define HS_WIDTH GENMASK(14, 0) > > #define SYNCVAL_HS_POL_ACTIVE_LOW (1 << 15) > > #define SYNCVAL_VS_POL_ACTIVE_LOW (1 << 31) > > #define DP0_MISC 0x0658 > > #define TU_SIZE_RECOMMENDED (63) /* LSCLK cycles per TU */ > > +#define MAX_TU_SYMBOL GENMASK(28, 23) > > +#define TU_SIZE GENMASK(21, 16) > > #define BPC_6 (0 << 5) > > #define BPC_8 (1 << 5) > > > > @@ -184,6 +208,12 @@ > > > > /* Test & Debug */ > > #define TSTCTL 0x0a00 > > +#define COLOR_R GENMASK(31, 24) > > +#define COLOR_G GENMASK(23, 16) > > +#define COLOR_B GENMASK(15, 8) > > +#define ENI2CFILTER BIT(4) > > +#define COLOR_BAR_MODE GENMASK(1, 0) > > +#define COLOR_BAR_MODE_BARS 2 > > #define PLL_DBG 0x0a04 > > > > static bool tc_test_pattern; > > @@ -647,10 +677,6 @@ static int tc_get_display_props(struct tc_data *tc) > > > > static int tc_set_video_mode(struct tc_data *tc, struct drm_display_mode *mode) > > { > > - int ret; > > - int vid_sync_dly; > > - int max_tu_symbol; > > - > > int left_margin = mode->htotal - mode->hsync_end; > > int right_margin = mode->hsync_start - mode->hdisplay; > > int hsync_len = mode->hsync_end - mode->hsync_start; > > @@ -659,76 +685,74 @@ static int tc_set_video_mode(struct tc_data *tc, struct drm_display_mode *mode) > > int vsync_len = mode->vsync_end - mode->vsync_start; > > > > /* > > - * Recommended maximum number of symbols transferred in a transfer unit: > > + * Recommended maximum number of symbols transferred in a > > + * transfer unit: > > * DIV_ROUND_UP((input active video bandwidth in bytes) * tu_size, > > * (output active video bandwidth in bytes)) > > * Must be less than tu_size. > > */ > > - max_tu_symbol = TU_SIZE_RECOMMENDED - 1; > > - > > - dev_dbg(tc->dev, "set mode %dx%d\n", > > - mode->hdisplay, mode->vdisplay); > > - dev_dbg(tc->dev, "H margin %d,%d sync %d\n", > > - left_margin, right_margin, hsync_len); > > - dev_dbg(tc->dev, "V margin %d,%d sync %d\n", > > - upper_margin, lower_margin, vsync_len); > > - dev_dbg(tc->dev, "total: %dx%d\n", mode->htotal, mode->vtotal); > > - > > + int max_tu_symbol = TU_SIZE_RECOMMENDED - 1; > > > > + /* DP Main Stream Attributes */ > > + int vid_sync_dly = hsync_len + left_margin + mode->hdisplay; > > /* > > * LCD Ctl Frame Size > > * datasheet is not clear of vsdelay in case of DPI > > * assume we do not need any delay when DPI is a source of > > * sync signals > > */ > > - tc_write(VPCTRL0, (0 << 20) /* VSDELAY */ | > > - OPXLFMT_RGB888 | FRMSYNC_DISABLED | MSF_DISABLED); > > - tc_write(HTIM01, (ALIGN(left_margin, 2) << 16) | /* H back porch */ > > - (ALIGN(hsync_len, 2) << 0)); /* Hsync */ > > - tc_write(HTIM02, (ALIGN(right_margin, 2) << 16) | /* H front porch */ > > - (ALIGN(mode->hdisplay, 2) << 0)); /* width */ > > - tc_write(VTIM01, (upper_margin << 16) | /* V back porch */ > > - (vsync_len << 0)); /* Vsync */ > > - tc_write(VTIM02, (lower_margin << 16) | /* V front porch */ > > - (mode->vdisplay << 0)); /* height */ > > - tc_write(VFUEN0, VFUEN); /* update settings */ > > - > > - /* Test pattern settings */ > > - tc_write(TSTCTL, > > - (120 << 24) | /* Red Color component value */ > > - (20 << 16) | /* Green Color component value */ > > - (99 << 8) | /* Blue Color component value */ > > - (1 << 4) | /* Enable I2C Filter */ > > - (2 << 0) | /* Color bar Mode */ > > - 0); > > - > > - /* DP Main Stream Attributes */ > > - vid_sync_dly = hsync_len + left_margin + mode->hdisplay; > > - tc_write(DP0_VIDSYNCDELAY, > > - (max_tu_symbol << 16) | /* thresh_dly */ > > - (vid_sync_dly << 0)); > > + const u32 vs_pol = mode->flags & DRM_MODE_FLAG_NVSYNC ? > > + SYNCVAL_VS_POL_ACTIVE_LOW : 0; > > + const u32 hs_pol = mode->flags & DRM_MODE_FLAG_NHSYNC ? > > + SYNCVAL_HS_POL_ACTIVE_LOW : 0; > > + const struct reg_sequence video_mode_settings[] = { > > + { VPCTRL0, FIELD_PREP(VSDELAY, 0) | > > + OPXLFMT_RGB888 | FRMSYNC_DISABLED | MSF_DISABLED }, > > + { HTIM01, FIELD_PREP(HBPR, ALIGN(left_margin, 2)) | > > + FIELD_PREP(HPW, ALIGN(hsync_len, 2)) }, > > + { HTIM02, FIELD_PREP(HDISPR, ALIGN(mode->hdisplay, 2)) | > > + FIELD_PREP(HFPR, ALIGN(right_margin, 2)) }, > > + { VTIM01, FIELD_PREP(VBPR, upper_margin) | > > + FIELD_PREP(VSPR, vsync_len) }, > > + { VTIM02, FIELD_PREP(VFPR, lower_margin) | > > + FIELD_PREP(VDISPR, mode->vdisplay) }, > > + { VFUEN0, VFUEN }, > > + { TSTCTL, FIELD_PREP(COLOR_R, 120) | > > + FIELD_PREP(COLOR_G, 20) | > > + FIELD_PREP(COLOR_B, 99) | > > + ENI2CFILTER | > > + FIELD_PREP(COLOR_BAR_MODE, COLOR_BAR_MODE_BARS) }, > > + { DP0_VIDSYNCDELAY, FIELD_PREP(THRESH_DLY, max_tu_symbol) | > > + FIELD_PREP(VID_SYNC_DLY, vid_sync_dly) }, > > + { DP0_TOTALVAL, FIELD_PREP(H_TOTAL, mode->htotal) | > > + FIELD_PREP(V_TOTAL, mode->vtotal) }, > > + { DP0_STARTVAL, FIELD_PREP(H_START, left_margin + hsync_len) | > > + FIELD_PREP(V_START, > > + upper_margin + vsync_len) }, > > + { DP0_ACTIVEVAL, FIELD_PREP(V_ACT, mode->vdisplay) | > > + FIELD_PREP(H_ACT, mode->hdisplay) }, > > + { DP0_SYNCVAL, FIELD_PREP(VS_WIDTH, vsync_len) | > > + FIELD_PREP(HS_WIDTH, hsync_len) | > > + hs_pol | vs_pol }, > > + { DPIPXLFMT, VS_POL_ACTIVE_LOW | HS_POL_ACTIVE_LOW | > > + DE_POL_ACTIVE_HIGH | SUB_CFG_TYPE_CONFIG1 | > > + DPI_BPP_RGB888 }, > > + { DP0_MISC, FIELD_PREP(MAX_TU_SYMBOL, max_tu_symbol) | > > + FIELD_PREP(TU_SIZE, TU_SIZE_RECOMMENDED) | > > + BPC_8 }, > > + }; > > > > - tc_write(DP0_TOTALVAL, (mode->vtotal << 16) | (mode->htotal)); > > - > > - tc_write(DP0_STARTVAL, > > - ((upper_margin + vsync_len) << 16) | > > - ((left_margin + hsync_len) << 0)); > > - > > - tc_write(DP0_ACTIVEVAL, (mode->vdisplay << 16) | (mode->hdisplay)); > > - > > - tc_write(DP0_SYNCVAL, (vsync_len << 16) | (hsync_len << 0) | > > - ((mode->flags & DRM_MODE_FLAG_NHSYNC) ? SYNCVAL_HS_POL_ACTIVE_LOW : 0) | > > - ((mode->flags & DRM_MODE_FLAG_NVSYNC) ? SYNCVAL_VS_POL_ACTIVE_LOW : 0)); > > - > > - tc_write(DPIPXLFMT, VS_POL_ACTIVE_LOW | HS_POL_ACTIVE_LOW | > > - DE_POL_ACTIVE_HIGH | SUB_CFG_TYPE_CONFIG1 | DPI_BPP_RGB888); > > - > > - tc_write(DP0_MISC, (max_tu_symbol << 23) | (TU_SIZE_RECOMMENDED << 16) | > > - BPC_8); > > + dev_dbg(tc->dev, "set mode %dx%d\n", > > + mode->hdisplay, mode->vdisplay); > > + dev_dbg(tc->dev, "H margin %d,%d sync %d\n", > > + left_margin, right_margin, hsync_len); > > + dev_dbg(tc->dev, "V margin %d,%d sync %d\n", > > + upper_margin, lower_margin, vsync_len); > > + dev_dbg(tc->dev, "total: %dx%d\n", mode->htotal, mode->vtotal); > > > > - return 0; > > -err: > > - return ret; > > + return regmap_multi_reg_write(tc->regmap, > > + video_mode_settings, > > + ARRAY_SIZE(video_mode_settings)); > > } > > > > static int tc_wait_link_training(struct tc_data *tc, u32 *error) > > I don't like this change. I think multi_reg_write is good for writing > things like, say, color conversion table. But the data for video mode is > more complex and needs more logic (e.g. here ?: operator is used). The logic was stuffed into function arguments before, so this patch didn't really change anything. > You need to stuff all that logic into variable initializers. You can't use > e.g. if() anymore, Where you planning on doing so, or is it just hypothetical? > and you can't even insert a debug print between the > writes if you need to. > Same here. > So, in my opinion, this doesn't look (much) cleaner, and makes life more > difficult. > Sure, I'll drop this patch since it generates so much resistance. Thanks, Andrey Smirnov
On Fri, Mar 22, 2019 at 1:06 AM Tomi Valkeinen <tomi.valkeinen@ti.com> wrote: > > Hi, > > On 22/03/2019 05:28, Andrey Smirnov wrote: > > Everyone: > > > > This series contains various improvements (at least in my mind) and > > fixes that I made to tc358767 while working with the code of the > > driver. Hopefuly each patch is self explanatory. > > > > Feedback is welcome! Ugh, I just realized that I messed up CC list of "drm/bridge: tc358767: Simplify AUX data read" and it only went to dri-devel. Will fix in next version. > > Ah, I hadn't realized there was another series for tc358767 going > around. I should've checked dri-devel list. Sorry about that! > No worries, lucky for me Lucas Stach noticed your series and pointed me to it, so it all worked out in the end. > I'm not able to apply your patches, it fails at "Simplify > tc_set_video_mode()", even if I think I've got the exact same base as > you have. > > Can you push your branch somewhere? Sure, it should be available here: https://github.com/ndreys/linux/commits/rdu1-rdu2 Please ignore "drm/bridge: tc358767: Add code to control VDCC rail", sitting on top. I need it on my board, but it's currently purely experimental and not meant to be upstreamed yet (or maybe ever) Thanks, Andrey Smirnov
Everyone: This series contains various improvements (at least in my mind) and fixes that I made to tc358767 while working with the code of the driver. Hopefuly each patch is self explanatory. Feedback is welcome! Thanks, Andrey Smirnov Changes since [v1]: - Patchset rebased on top of https://patchwork.freedesktop.org/series/58176/ - Patches to remove both tc_write() and tc_read() helpers added - Patches to rework AUX transfer code added - Both "drm/bridge: tc358767: Simplify polling in tc_main_link_setup()" and "drm/bridge: tc358767: Simplify polling in tc_link_training()" changed to use tc_poll_timeout() instead of regmap_read_poll_timeout() [v1] lkml.kernel.org/r/20190226193609.9862-1-andrew.smirnov@gmail.com Andrey Smirnov (15): drm/bridge: tc358767: Simplify tc_poll_timeout() drm/bridge: tc358767: Simplify polling in tc_main_link_setup() drm/bridge: tc358767: Simplify polling in tc_link_training() drm/bridge: tc358767: Simplify tc_set_video_mode() drm/bridge: tc358767: Drop custom tc_write()/tc_read() accessors drm/bridge: tc358767: Simplify AUX data read drm/bridge: tc358767: Simplify AUX data write drm/bridge: tc358767: Increase AUX transfer length limit drm/bridge: tc358767: Use reported AUX transfer size drm/bridge: tc358767: Add support for address-only I2C transfers drm/bridge: tc358767: Introduce tc_set_syspllparam() drm/bridge: tc358767: Introduce tc_pllupdate_pllen() drm/bridge: tc358767: Simplify tc_aux_wait_busy() drm/bridge: tc358767: Drop unnecessary 8 byte buffer drm/bridge: tc358767: Replace magic number in tc_main_link_enable() drivers/gpu/drm/bridge/tc358767.c | 648 ++++++++++++++++-------------- 1 file changed, 349 insertions(+), 299 deletions(-)