Re: [PATCHv2 03/22] drm/bridge: tc358767: fix ansi 8b10b use
On Mon, May 6, 2019 at 2:59 AM Tomi Valkeinen wrote: > > Hi Laurent, Andrey, > > On 03/05/2019 20:11, Laurent Pinchart wrote: > >> I agree that if the panel Andrey mentioned is still used, we need to > >> handle it somehow. But I think explicitly handling such a case is better > >> than guessing. > > > > The risk may not be worth it, I agree. I would explain this in details > > in the commit message though, and add a comment to the code as well, to > > ease future debugging. > > Andrey, do you still have the panel that doesn't work with 8b10b? Is it > used in real life (i.e. it was not just a temporary development phase > panel)? What's the model, and is there a public datasheet? Note that I am a different Andrey, and I can't speak about the original panel that caused the issue. However, production units are shipped with this panel: https://www.data-modul.com/productfinder/sites/default/files/products/NL192108AC13-02D_specification_12023727.pdf which seems to do pretty standard DP stuff. In all of my testing of Tomi's patches, I haven't seen any problems so far (I still have to test v3 though), so I think we can carefully proceed assuming that that weird panel was just a fluke. Thanks, Andrey Smirnov ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCHv2 21/22] drm/bridge: tc358767: add IRQ and HPD support
On Fri, Apr 12, 2019 at 1:02 AM Tomi Valkeinen wrote:
>
> Hi Andrey,
>
> On 03/04/2019 14:34, Tomi Valkeinen wrote:
> > On 02/04/2019 05:16, Andrey Smirnov wrote:
> >
> >> The early return above causes tc_get_display_props() to never be
> >> called for eDP case, which in turn result in tc_mode_valid() returning
> >> MODE_BAD for every mode it is given since it depends on tc->link.base
> >> being initialized properly. I had to change this code to:
> >>
> >> if (tc->hpd_num < 0) {
> >> if (!tc->panel)
> >> return connector_status_unknown;
> >>
> >> conn = true;
> >> } else {
> >> tc_read(GPIOI, &val);
> >>
> >> conn = val & BIT(tc->hpd_num);
> >> }
> >>
> >> to fix the problem.
> >
> > Ah, right. There's tc_get_display_props() in tc_bridge_enable() but
> > that's of course too late (and maybe even not needed at all).
> >
> > What you suggest here looks fine to me, so I'll change my patch
> > accordingly. Thanks!
>
> With the change above, does the series work with your setup? Can I add
> your tested-by? I'll send v3 series soon with the DT change suggested by
> Rob and the above fix.
>
Yeah, other than that, the series seemed to work as expected on my
setup. You can add
Tested-by: Andrey Smirnov
to v3.
Thanks,
Andrey Smirnov
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCHv2 02/22] drm/bridge: tc358767: reset voltage-swing & pre-emphasis
On Fri, Apr 26, 2019 at 7:14 AM Tomi Valkeinen wrote: > > On 20/04/2019 23:30, Laurent Pinchart wrote: > > Hi Tomi, > > > > Thank you for the patch. > > > > On Tue, Mar 26, 2019 at 12:31:26PM +0200, Tomi Valkeinen wrote: > >> We need to reset DPCD voltage-swing & pre-emphasis before starting the > >> link training, as otherwise tc358767 will use the previous values as > >> minimums. > >> > >> Signed-off-by: Tomi Valkeinen > >> --- > >> drivers/gpu/drm/bridge/tc358767.c | 6 ++ > >> 1 file changed, 6 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/bridge/tc358767.c > >> b/drivers/gpu/drm/bridge/tc358767.c > >> index 7031c4f52c57..11a50f7bb4be 100644 > >> --- a/drivers/gpu/drm/bridge/tc358767.c > >> +++ b/drivers/gpu/drm/bridge/tc358767.c > >> @@ -956,6 +956,12 @@ static int tc_main_link_setup(struct tc_data *tc) > >> if (ret < 0) > >> goto err_dpcd_write; > >> > >> +// Reset voltage-swing & pre-emphasis > > > > The driver uses C-style comments, I think it would be best to stick to > > them to avoid a style mismatch. > > Oops. Yep. I often use c++ comments when hacking/developing as they're > often easier to use. Sometimes I miss converting them to c comments... > > > > >> +tmp[0] = tmp[1] = DP_TRAIN_VOLTAGE_SWING_LEVEL_0 | > >> DP_TRAIN_PRE_EMPH_LEVEL_0; > > > > You may want to wrap the line. > > Well, I personally don't think wrapping at 80 is a good idea. Trying to read two files side by side on a 13" laptop screen might change your mind :-) +1 on wrapping the code around 80 character from me. Thanks, Andrey Smirnov ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 1/9] drm/bridge: tc358767: Simplify tc_poll_timeout()
Implementation of tc_poll_timeout() is almost a 100% copy-and-paste of
the code for regmap_read_poll_timeout(). Replace copied code with a
call to the original. No functional change intended.
Signed-off-by: Andrey Smirnov
Cc: Archit Taneja
Cc: Andrzej Hajda
Cc: Laurent Pinchart
Cc: Chris Healy
Cc: Lucas Stach
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
drivers/gpu/drm/bridge/tc358767.c | 19 +++
1 file changed, 3 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c
b/drivers/gpu/drm/bridge/tc358767.c
index e6403b9549f1..b0f8264a1285 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -252,24 +252,11 @@ static inline int tc_poll_timeout(struct regmap *map,
unsigned int addr,
unsigned int cond_value,
unsigned long sleep_us, u64 timeout_us)
{
- ktime_t timeout = ktime_add_us(ktime_get(), timeout_us);
unsigned int val;
- int ret;
- for (;;) {
- ret = regmap_read(map, addr, &val);
- if (ret)
- break;
- if ((val & cond_mask) == cond_value)
- break;
- if (timeout_us && ktime_compare(ktime_get(), timeout) > 0) {
- ret = regmap_read(map, addr, &val);
- break;
- }
- if (sleep_us)
- usleep_range((sleep_us >> 2) + 1, sleep_us);
- }
- return ret ?: (((val & cond_mask) == cond_value) ? 0 : -ETIMEDOUT);
+ return regmap_read_poll_timeout(map, addr, val,
+ (val & cond_mask) == cond_value,
+ sleep_us, timeout_us);
}
static int tc_aux_wait_busy(struct tc_data *tc, unsigned int timeout_ms)
--
2.20.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 9/9] drm/bridge: tc358767: Drop tc_read() macro
There's only one place where tc_read() is used, so it doesn't save us
much. Drop it. No functional change intended.
Signed-off-by: Andrey Smirnov
Cc: Archit Taneja
Cc: Andrzej Hajda
Cc: Laurent Pinchart
Cc: Chris Healy
Cc: Lucas Stach
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
drivers/gpu/drm/bridge/tc358767.c | 15 +++
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c
b/drivers/gpu/drm/bridge/tc358767.c
index 239b3aaa255d..3c574f1569aa 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -240,12 +240,6 @@ static inline struct tc_data *connector_to_tc(struct
drm_connector *c)
if (ret)\
goto err; \
} while (0)
-#define tc_read(reg, var) \
- do {\
- ret = regmap_read(tc->regmap, reg, var);\
- if (ret)\
- goto err; \
- } while (0)
static inline int tc_poll_timeout(struct regmap *map, unsigned int addr,
unsigned int cond_mask,
@@ -337,8 +331,13 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
if (request == DP_AUX_I2C_READ || request == DP_AUX_NATIVE_READ) {
/* Read data */
while (i < size) {
- if ((i % 4) == 0)
- tc_read(DP0_AUXRDATA(i >> 2), &tmp);
+ if ((i % 4) == 0) {
+ ret = regmap_read(tc->regmap,
+ DP0_AUXRDATA(i >> 2),
+ &tmp);
+ if (ret)
+ goto err;
+ }
buf[i] = tmp & 0xff;
tmp = tmp >> 8;
i++;
--
2.20.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 0/9] tc358767 driver improvements
Everyone: This series contains various improvements (at least in my mind) 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 Andrey Smirnov (9): drm/bridge: tc358767: Simplify tc_poll_timeout() drm/bridge: tc358767: Simplify tc_stream_clock_calc() drm/bridge: tc358767: Simplify tc_set_video_mode() drm/bridge: tc358767: Simplify polling in tc_main_link_setup() drm/bridge: tc358767: Simplify polling in tc_link_training() drm/bridge: tc358767: Simplify error check in tc_aux_linx_setup() drm/bridge: tc358767: Introduce tc_set_syspllparam() drm/bridge: tc358767: Introduce tc_pllupdate_pllen() drm/bridge: tc358767: Drop tc_read() macro drivers/gpu/drm/bridge/tc358767.c | 283 ++ 1 file changed, 129 insertions(+), 154 deletions(-) -- 2.20.1 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 2/9] drm/bridge: tc358767: Simplify tc_stream_clock_calc()
Drop the use of tc_write() as well as "magicly" used "ret" and "err:"
and replace it with a simple call to regmap_write(). No functional
change intended.
Signed-off-by: Andrey Smirnov
Cc: Archit Taneja
Cc: Andrzej Hajda
Cc: Laurent Pinchart
Cc: Chris Healy
Cc: Lucas Stach
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
drivers/gpu/drm/bridge/tc358767.c | 7 +--
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c
b/drivers/gpu/drm/bridge/tc358767.c
index b0f8264a1285..86ebd49194b7 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -504,7 +504,6 @@ static int tc_pxl_pll_dis(struct tc_data *tc)
static int tc_stream_clock_calc(struct tc_data *tc)
{
- int ret;
/*
* If the Stream clock and Link Symbol clock are
* asynchronous with each other, the value of M changes over
@@ -520,11 +519,7 @@ static int tc_stream_clock_calc(struct tc_data *tc)
* M/N = f_STRMCLK / f_LSCLK
*
*/
- tc_write(DP0_VIDMNGEN1, 32768);
-
- return 0;
-err:
- return ret;
+ return regmap_write(tc->regmap, DP0_VIDMNGEN1, 32768);
}
static int tc_aux_link_setup(struct tc_data *tc)
--
2.20.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 5/9] drm/bridge: tc358767: Simplify polling in tc_link_training()
Replace explicit polling in tc_link_training() with equivalent call to
regmap_read_poll_timeout() for simplicity. No functional change
intended (not including slightly altered debug output).
Signed-off-by: Andrey Smirnov
Cc: Archit Taneja
Cc: Andrzej Hajda
Cc: Laurent Pinchart
Cc: Chris Healy
Cc: Lucas Stach
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
drivers/gpu/drm/bridge/tc358767.c | 14 +-
1 file changed, 5 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c
b/drivers/gpu/drm/bridge/tc358767.c
index 6455e6484722..ea30cec4a0c3 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -735,7 +735,6 @@ static int tc_link_training(struct tc_data *tc, int pattern)
const char * const *errors;
u32 srcctrl = tc_srcctrl(tc) | DP0_SRCCTRL_SCRMBLDIS |
DP0_SRCCTRL_AUTOCORRECT;
- int timeout;
int retry;
u32 value;
int ret;
@@ -765,20 +764,17 @@ static int tc_link_training(struct tc_data *tc, int
pattern)
tc_write(DP0CTL, DP_EN);
/* wait */
- timeout = 1000;
- do {
- tc_read(DP0_LTSTAT, &value);
- udelay(1);
- } while ((!(value & LT_LOOPDONE)) && (--timeout));
- if (timeout == 0) {
+ ret = regmap_read_poll_timeout(tc->regmap, DP0_LTSTAT, value,
+ value & LT_LOOPDONE, 1, 1000);
+ if (ret) {
dev_err(tc->dev, "Link training timeout!\n");
} else {
int pattern = (value >> 11) & 0x3;
int error = (value >> 8) & 0x7;
dev_dbg(tc->dev,
- "Link training phase %d done after %d uS: %s\n",
- pattern, 1000 - timeout, errors[error]);
+ "Link training phase %d done: %s\n",
+ pattern, errors[error]);
if (pattern == DP_TRAINING_PATTERN_1 && error == 0)
break;
if (pattern == DP_TRAINING_PATTERN_2) {
--
2.20.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 3/9] drm/bridge: tc358767: Simplify tc_set_video_mode()
Simplify tc_set_video_mode() by replacing repreated calls to
tc_write()/regmap_write() with a single call regmap_multi_reg_write().
No functional change intended.
Signed-off-by: Andrey Smirnov
Cc: Archit Taneja
Cc: Andrzej Hajda
Cc: Laurent Pinchart
Cc: Chris Healy
Cc: Lucas Stach
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
drivers/gpu/drm/bridge/tc358767.c | 125 --
1 file changed, 65 insertions(+), 60 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c
b/drivers/gpu/drm/bridge/tc358767.c
index 86ebd49194b7..c85468fcc157 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -641,10 +641,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;
@@ -653,76 +649,85 @@ 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 */
-
+ u32 vpctrl0 = (0 << 20) /* VSDELAY */ |
+ OPXLFMT_RGB888 | FRMSYNC_DISABLED |
+ MSF_DISABLED;
+ u32 htim01 = (ALIGN(left_margin, 2) << 16) | /* H back porch */
+(ALIGN(hsync_len, 2) << 0); /* Hsync */
+ u32 htim02 = (ALIGN(right_margin, 2) << 16) | /* H front porch */
+(ALIGN(mode->hdisplay, 2) << 0); /* width */
+ u32 vtim01 = (upper_margin << 16) | /* V back porch */
+(vsync_len << 0); /* Vsync */
+ u32 vtim02 = (lower_margin << 16) | /* V front porch */
+(mode->vdisplay << 0); /* height */
/* 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));
+ u32 tstctl = (120 << 24) | /* Red Color component value */
+(20 << 16) | /* Green Color component value */
+(99 << 8) | /* Blue Color component value */
+
[PATCH 6/9] drm/bridge: tc358767: Simplify error check in tc_aux_linx_setup()
Tc_poll_timeout() can only return -ETIMEDOUT, so checking for other
errors is not necessary. Drop it. No functional change intended.
Signed-off-by: Andrey Smirnov
Cc: Archit Taneja
Cc: Andrzej Hajda
Cc: Laurent Pinchart
Cc: Chris Healy
Cc: Lucas Stach
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
drivers/gpu/drm/bridge/tc358767.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c
b/drivers/gpu/drm/bridge/tc358767.c
index ea30cec4a0c3..54ff95f66e30 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -569,11 +569,10 @@ static int tc_aux_link_setup(struct tc_data *tc)
ret = tc_poll_timeout(tc->regmap, DP_PHY_CTRL, PHY_RDY, PHY_RDY, 1,
1000);
- if (ret == -ETIMEDOUT) {
+ if (ret) {
dev_err(tc->dev, "Timeout waiting for PHY to become ready");
return ret;
- } else if (ret)
- goto err;
+ }
/* Setup AUX link */
tc_write(DP0_AUXCFG1, AUX_RX_FILTER_EN |
--
2.20.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 4/9] drm/bridge: tc358767: Simplify polling in tc_main_link_setup()
Replace explicit polling loop with equivalent call to
regmap_read_poll_timeout() for simplicity. No functional change
intended.
Signed-off-by: Andrey Smirnov
Cc: Archit Taneja
Cc: Andrzej Hajda
Cc: Laurent Pinchart
Cc: Chris Healy
Cc: Lucas Stach
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
drivers/gpu/drm/bridge/tc358767.c | 12
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c
b/drivers/gpu/drm/bridge/tc358767.c
index c85468fcc157..6455e6484722 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -882,15 +882,11 @@ static int tc_main_link_setup(struct tc_data *tc)
dp_phy_ctrl &= ~(DP_PHY_RST | PHY_M1_RST | PHY_M0_RST);
tc_write(DP_PHY_CTRL, dp_phy_ctrl);
- timeout = 1000;
- do {
- tc_read(DP_PHY_CTRL, &value);
- udelay(1);
- } while ((!(value & PHY_RDY)) && (--timeout));
-
- if (timeout == 0) {
+ ret = regmap_read_poll_timeout(tc->regmap, DP_PHY_CTRL, value,
+ value & PHY_RDY, 1, 1000);
+ if (ret) {
dev_err(dev, "timeout waiting for phy become ready");
- return -ETIMEDOUT;
+ return ret;
}
/* Set misc: 8 bits per color */
--
2.20.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 8/9] drm/bridge: tc358767: Introduce tc_pllupdate_pllen()
Tc_wait_pll_lock() is always called as a follow-up for updating
PLLUPDATE and PLLEN bit of a given PLL control register. To simplify
things, merge the two operation into a single helper function
tc_pllupdate_pllen() and convert the rest of the code to use it. No
functional change intended.
Signed-off-by: Andrey Smirnov
Cc: Archit Taneja
Cc: Andrzej Hajda
Cc: Laurent Pinchart
Cc: Chris Healy
Cc: Lucas Stach
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
drivers/gpu/drm/bridge/tc358767.c | 36 +++
1 file changed, 22 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c
b/drivers/gpu/drm/bridge/tc358767.c
index 227f14cd2d3d..239b3aaa255d 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -390,10 +390,18 @@ static u32 tc_srcctrl(struct tc_data *tc)
return reg;
}
-static void tc_wait_pll_lock(struct tc_data *tc)
+static int tc_pllupdate_pllen(struct tc_data *tc, unsigned int pllctrl)
{
+ int ret;
+
+ ret = regmap_write(tc->regmap, pllctrl, PLLUPDATE | PLLEN);
+ if (ret)
+ return ret;
+
/* Wait for PLL to lock: up to 2.09 ms, depending on refclk */
usleep_range(3000, 6000);
+
+ return 0;
}
static int tc_pxl_pll_en(struct tc_data *tc, u32 refclk, u32 pixelclock)
@@ -487,11 +495,7 @@ static int tc_pxl_pll_en(struct tc_data *tc, u32 refclk,
u32 pixelclock)
(best_mul << 0)); /* Multiplier for PLL */
/* Force PLL parameter update and disable bypass */
- tc_write(PXL_PLLCTRL, PLLUPDATE | PLLEN);
-
- tc_wait_pll_lock(tc);
-
- return 0;
+ return tc_pllupdate_pllen(tc, PXL_PLLCTRL);
err:
return ret;
}
@@ -568,11 +572,13 @@ static int tc_aux_link_setup(struct tc_data *tc)
* Initially PLLs are in bypass. Force PLL parameter update,
* disable PLL bypass, enable PLL
*/
- tc_write(DP0_PLLCTRL, PLLUPDATE | PLLEN);
- tc_wait_pll_lock(tc);
+ ret = tc_pllupdate_pllen(tc, DP0_PLLCTRL);
+ if (ret)
+ return ret;
- tc_write(DP1_PLLCTRL, PLLUPDATE | PLLEN);
- tc_wait_pll_lock(tc);
+ ret = tc_pllupdate_pllen(tc, DP1_PLLCTRL);
+ if (ret)
+ return ret;
ret = tc_poll_timeout(tc->regmap, DP_PHY_CTRL, PHY_RDY, PHY_RDY, 1,
1000);
@@ -846,11 +852,13 @@ static int tc_main_link_setup(struct tc_data *tc)
msleep(100);
/* PLL setup */
- tc_write(DP0_PLLCTRL, PLLUPDATE | PLLEN);
- tc_wait_pll_lock(tc);
+ ret = tc_pllupdate_pllen(tc, DP0_PLLCTRL);
+ if (ret)
+ return ret;
- tc_write(DP1_PLLCTRL, PLLUPDATE | PLLEN);
- tc_wait_pll_lock(tc);
+ ret = tc_pllupdate_pllen(tc, DP1_PLLCTRL);
+ if (ret)
+ return ret;
/* PXL PLL setup */
if (tc_test_pattern) {
--
2.20.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 7/9] drm/bridge: tc358767: Introduce tc_set_syspllparam()
Move common code converting clock rate to an appropriate constant and
configuring SYS_PLLPARAM register into a separate routine and convert
the rest of the code to use it. No functional change intended.
Signed-off-by: Andrey Smirnov
Cc: Archit Taneja
Cc: Andrzej Hajda
Cc: Laurent Pinchart
Cc: Chris Healy
Cc: Lucas Stach
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
drivers/gpu/drm/bridge/tc358767.c | 50 +--
1 file changed, 20 insertions(+), 30 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c
b/drivers/gpu/drm/bridge/tc358767.c
index 54ff95f66e30..227f14cd2d3d 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -522,35 +522,42 @@ static int tc_stream_clock_calc(struct tc_data *tc)
return regmap_write(tc->regmap, DP0_VIDMNGEN1, 32768);
}
-static int tc_aux_link_setup(struct tc_data *tc)
+static int tc_set_syspllparam(struct tc_data *tc)
{
unsigned long rate;
- u32 value;
- int ret;
- u32 dp_phy_ctrl;
+ u32 pllparam = SYSCLK_SEL_LSCLK | LSCLK_DIV_2;
rate = clk_get_rate(tc->refclk);
switch (rate) {
case 3840:
- value = REF_FREQ_38M4;
+ pllparam |= REF_FREQ_38M4;
break;
case 2600:
- value = REF_FREQ_26M;
+ pllparam |= REF_FREQ_26M;
break;
case 1920:
- value = REF_FREQ_19M2;
+ pllparam |= REF_FREQ_19M2;
break;
case 1300:
- value = REF_FREQ_13M;
+ pllparam |= REF_FREQ_13M;
break;
default:
dev_err(tc->dev, "Invalid refclk rate: %lu Hz\n", rate);
return -EINVAL;
}
+ return regmap_write(tc->regmap, SYS_PLLPARAM, pllparam);
+}
+
+static int tc_aux_link_setup(struct tc_data *tc)
+{
+ int ret;
+ u32 dp_phy_ctrl;
+
/* Setup DP-PHY / PLL */
- value |= SYSCLK_SEL_LSCLK | LSCLK_DIV_2;
- tc_write(SYS_PLLPARAM, value);
+ ret = tc_set_syspllparam(tc);
+ if (ret)
+ return ret;
dp_phy_ctrl = BGREN | PWR_SW_EN | PHY_A0_EN;
if (tc->link.base.num_lanes == 2)
@@ -811,7 +818,6 @@ static int tc_main_link_setup(struct tc_data *tc)
{
struct drm_dp_aux *aux = &tc->aux;
struct device *dev = tc->dev;
- unsigned int rate;
u32 dp_phy_ctrl;
int timeout;
u32 value;
@@ -828,25 +834,9 @@ static int tc_main_link_setup(struct tc_data *tc)
(tc->link.spread ? DP0_SRCCTRL_SSCG : 0) |
((tc->link.base.rate != 162000) ? DP0_SRCCTRL_BW27 : 0));
- rate = clk_get_rate(tc->refclk);
- switch (rate) {
- case 3840:
- value = REF_FREQ_38M4;
- break;
- case 2600:
- value = REF_FREQ_26M;
- break;
- case 1920:
- value = REF_FREQ_19M2;
- break;
- case 1300:
- value = REF_FREQ_13M;
- break;
- default:
- return -EINVAL;
- }
- value |= SYSCLK_SEL_LSCLK | LSCLK_DIV_2;
- tc_write(SYS_PLLPARAM, value);
+ ret = tc_set_syspllparam(tc);
+ if (ret)
+ return ret;
/* Setup Main Link */
dp_phy_ctrl = BGREN | PWR_SW_EN | PHY_A0_EN | PHY_M0_EN;
--
2.20.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 3/9] drm/bridge: tc358767: Simplify tc_set_video_mode()
On Mon, Mar 4, 2019 at 4:26 AM Laurent Pinchart
wrote:
>
> Hi Andrey,
>
> Thank you for the patch.
>
> On Tue, Feb 26, 2019 at 11:36:03AM -0800, Andrey Smirnov wrote:
> > Simplify tc_set_video_mode() by replacing repreated calls to
> > tc_write()/regmap_write() with a single call regmap_multi_reg_write().
> > No functional change intended.
> >
> > Signed-off-by: Andrey Smirnov
> > Cc: Archit Taneja
> > Cc: Andrzej Hajda
> > Cc: Laurent Pinchart
> > Cc: Chris Healy
> > Cc: Lucas Stach
> > Cc: dri-devel@lists.freedesktop.org
> > Cc: linux-ker...@vger.kernel.org
> > ---
> > drivers/gpu/drm/bridge/tc358767.c | 125 --
> > 1 file changed, 65 insertions(+), 60 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/tc358767.c
> > b/drivers/gpu/drm/bridge/tc358767.c
> > index 86ebd49194b7..c85468fcc157 100644
> > --- a/drivers/gpu/drm/bridge/tc358767.c
> > +++ b/drivers/gpu/drm/bridge/tc358767.c
> > @@ -641,10 +641,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;
> > @@ -653,76 +649,85 @@ 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 */
> > -
> > + u32 vpctrl0 = (0 << 20) /* VSDELAY */ |
> > + OPXLFMT_RGB888 | FRMSYNC_DISABLED |
> > + MSF_DISABLED;
> > + u32 htim01 = (ALIGN(left_margin, 2) << 16) | /* H back porch */
> > + (ALIGN(hsync_len, 2) << 0); /* Hsync */
> > + u32 htim02 = (ALIGN(right_margin, 2) << 16) | /* H front porch */
> > + (ALIGN(mode->hdisplay, 2) << 0); /* width */
> > + u32 vtim01 = (upper_margin << 16) | /* V back porch */
> > + (vsync_len << 0); /* Vsync */
> > + u32 vtim02 = (lower_margin << 16) | /* V front porch */
> > +
Re: [PATCH 5/9] drm/bridge: tc358767: Simplify polling in tc_link_training()
On Mon, Mar 4, 2019 at 4:30 AM Laurent Pinchart
wrote:
>
> Hi Andrey,
>
> Thank you for the patch.
>
> On Tue, Feb 26, 2019 at 11:36:05AM -0800, Andrey Smirnov wrote:
> > Replace explicit polling in tc_link_training() with equivalent call to
> > regmap_read_poll_timeout() for simplicity. No functional change
> > intended (not including slightly altered debug output).
> >
> > Signed-off-by: Andrey Smirnov
> > Cc: Archit Taneja
> > Cc: Andrzej Hajda
> > Cc: Laurent Pinchart
> > Cc: Chris Healy
> > Cc: Lucas Stach
> > Cc: dri-devel@lists.freedesktop.org
> > Cc: linux-ker...@vger.kernel.org
> > ---
> > drivers/gpu/drm/bridge/tc358767.c | 14 +-
> > 1 file changed, 5 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/tc358767.c
> > b/drivers/gpu/drm/bridge/tc358767.c
> > index 6455e6484722..ea30cec4a0c3 100644
> > --- a/drivers/gpu/drm/bridge/tc358767.c
> > +++ b/drivers/gpu/drm/bridge/tc358767.c
> > @@ -735,7 +735,6 @@ static int tc_link_training(struct tc_data *tc, int
> > pattern)
> > const char * const *errors;
> > u32 srcctrl = tc_srcctrl(tc) | DP0_SRCCTRL_SCRMBLDIS |
> > DP0_SRCCTRL_AUTOCORRECT;
> > - int timeout;
> > int retry;
> > u32 value;
> > int ret;
> > @@ -765,20 +764,17 @@ static int tc_link_training(struct tc_data *tc, int
> > pattern)
> > tc_write(DP0CTL, DP_EN);
> >
> > /* wait */
> > - timeout = 1000;
> > - do {
> > - tc_read(DP0_LTSTAT, &value);
> > - udelay(1);
> > - } while ((!(value & LT_LOOPDONE)) && (--timeout));
> > - if (timeout == 0) {
> > + ret = regmap_read_poll_timeout(tc->regmap, DP0_LTSTAT, value,
> > +value & LT_LOOPDONE, 1, 1000);
> > + if (ret) {
> > dev_err(tc->dev, "Link training timeout!\n");
> > } else {
> > int pattern = (value >> 11) & 0x3;
> > int error = (value >> 8) & 0x7;
> >
> > dev_dbg(tc->dev,
> > - "Link training phase %d done after %d uS:
> > %s\n",
> > - pattern, 1000 - timeout, errors[error]);
> > + "Link training phase %d done: %s\n",
> > + pattern, errors[error]);
>
> It's probably not a big deal in this specific case, but in general it
> can be useful to know how long the poll took.
I don't disagree, but bear in mind that the way time is measured in
original loop assumes that tc_read, an I2C transaction over 100KHz
bus, takes insignificant amount of time compared to 1 uS delay. I
think original debug statement does a bit of a false advertising when
it presents a number of polling loop iterations as if it is time it
took to establish a link in microseconds.
> Any hope to enhance regmap_read_poll_timeout() to return either the elapsed
> time or the
> remaining timeout instead of 0 on success ?
>
I'd rather not go there. That'll take convincing Mark Brown to accept
that semantics change, then fixing all of the callers across the tree
via a separate patch series.
What if instead we just add an extra debug statement before link
training starts, so that duration of the process can be discerned from
logging timestamps? This does require user doing a bit of math by
hand, but it's actually more accurate timing info compared to original
and it doesn't require any API modification.
Thanks,
Andrey Smirnov
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/9] drm/bridge: tc358767: Simplify tc_stream_clock_calc()
On Mon, Mar 4, 2019 at 4:20 AM Laurent Pinchart
wrote:
>
> Hello,
>
> On Mon, Mar 04, 2019 at 10:42:20AM +0100, Andrzej Hajda wrote:
> > On 26.02.2019 20:36, Andrey Smirnov wrote:
> > > Drop the use of tc_write() as well as "magicly" used "ret" and "err:"
> > > and replace it with a simple call to regmap_write(). No functional
> > > change intended.
> > >
> > > Signed-off-by: Andrey Smirnov
> > > Cc: Archit Taneja
> > > Cc: Andrzej Hajda
> > > Cc: Laurent Pinchart
> > > Cc: Chris Healy
> > > Cc: Lucas Stach
> > > Cc: dri-devel@lists.freedesktop.org
> > > Cc: linux-ker...@vger.kernel.org
> > > ---
> > > drivers/gpu/drm/bridge/tc358767.c | 7 +--
> > > 1 file changed, 1 insertion(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/bridge/tc358767.c
> > > b/drivers/gpu/drm/bridge/tc358767.c
> > > index b0f8264a1285..86ebd49194b7 100644
> > > --- a/drivers/gpu/drm/bridge/tc358767.c
> > > +++ b/drivers/gpu/drm/bridge/tc358767.c
> > > @@ -504,7 +504,6 @@ static int tc_pxl_pll_dis(struct tc_data *tc)
> > >
> > > static int tc_stream_clock_calc(struct tc_data *tc)
> > > {
> > > - int ret;
> > > /*
> > > * If the Stream clock and Link Symbol clock are
> > > * asynchronous with each other, the value of M changes over
> > > @@ -520,11 +519,7 @@ static int tc_stream_clock_calc(struct tc_data *tc)
> > > * M/N = f_STRMCLK / f_LSCLK
> > > *
> > > */
> > > - tc_write(DP0_VIDMNGEN1, 32768);
> > > -
> > > - return 0;
> > > -err:
> > > - return ret;
> > > + return regmap_write(tc->regmap, DP0_VIDMNGEN1, 32768);
> >
> >
> > The patch looks semantically correct, but you are dropping here custom
> > accessor (tc_write) in favor of regmap API.
> >
> > I think the best would be consistent across the whole driver: either use
> > only accessors, either drop them entirely and use regmap API.
>
> I agree with this. The tc_write macro with its goto err is pretty nasty,
> and I'd like to see it removed from the whole driver, or at least
> replaced with an accessor that wouldn't hide the goto.
>
> > And it would be good to have comment of the original authors about this
> > change.
> >
OK, I'll create a patch to remove tc_write/read in v2 and add original
authors to CC.
Thanks,
Andrey Smirnov
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 6/9] drm/bridge: tc358767: Simplify error check in tc_aux_linx_setup()
On Mon, Mar 4, 2019 at 4:33 AM Laurent Pinchart wrote: > > Hi Andrey, > > Thank you for the patch. > > On Tue, Feb 26, 2019 at 11:36:06AM -0800, Andrey Smirnov wrote: > > Tc_poll_timeout() can only return -ETIMEDOUT, so checking for other > > errors is not necessary. Drop it. No functional change intended. > > Is that true given patch 1/9 in this series ? regmap_read_poll_timeout() > can also return an error from regmap_read(). > Ugh, I misread "?:" in regmap_read_poll_timeout() code as "?", this patch is incorrect. Sorry about that, will drop in v2. Thanks, Andrey Smirnov ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v3 03/15] drm/bridge: tc358767: Simplify polling in tc_link_training()
Replace explicit polling in tc_link_training() with equivalent call to
tc_poll_timeout() for simplicity. No functional change intended (not
including slightly altered debug output).
Signed-off-by: Andrey Smirnov
Cc: Archit Taneja
Cc: Andrzej Hajda
Cc: Laurent Pinchart
Cc: Tomi Valkeinen
Cc: Andrey Gusakov
Cc: Philipp Zabel
Cc: Cory Tusar
Cc: Chris Healy
Cc: Lucas Stach
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
drivers/gpu/drm/bridge/tc358767.c | 15 ++-
1 file changed, 6 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c
b/drivers/gpu/drm/bridge/tc358767.c
index 5e1e73a91696..115cffc55a96 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -748,22 +748,19 @@ static int tc_set_video_mode(struct tc_data *tc,
static int tc_wait_link_training(struct tc_data *tc)
{
- u32 timeout = 1000;
u32 value;
int ret;
- do {
- udelay(1);
- tc_read(DP0_LTSTAT, &value);
- } while ((!(value & LT_LOOPDONE)) && (--timeout));
-
- if (timeout == 0) {
+ ret = tc_poll_timeout(tc, DP0_LTSTAT, LT_LOOPDONE,
+ LT_LOOPDONE, 1, 1000);
+ if (ret) {
dev_err(tc->dev, "Link training timeout waiting for
LT_LOOPDONE!\n");
- return -ETIMEDOUT;
+ return ret;
}
- return (value >> 8) & 0x7;
+ tc_read(DP0_LTSTAT, &value);
+ return (value >> 8) & 0x7;
err:
return ret;
}
--
2.21.0
[PATCH v3 06/15] drm/bridge: tc358767: Simplify AUX data read
Simplify AUX data read by removing index arithmetic and shifting with
a helper functions that does three things:
1. Fetch data from up to 4 32-bit registers from the chip
2. Optionally fix data endianness (not needed on LE hosts)
3. Copy read data into user provided array.
Signed-off-by: Andrey Smirnov
Cc: Archit Taneja
Cc: Andrzej Hajda
Cc: Laurent Pinchart
Cc: Tomi Valkeinen
Cc: Andrey Gusakov
Cc: Philipp Zabel
Cc: Cory Tusar
Cc: Chris Healy
Cc: Lucas Stach
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
drivers/gpu/drm/bridge/tc358767.c | 40 +--
1 file changed, 27 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c
b/drivers/gpu/drm/bridge/tc358767.c
index e197ce0fb166..da47d81e7109 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -321,6 +321,29 @@ static int tc_aux_get_status(struct tc_data *tc, u8 *reply)
return 0;
}
+static int tc_aux_read_data(struct tc_data *tc, void *data, size_t size)
+{
+ u32 auxrdata[DP_AUX_MAX_PAYLOAD_BYTES / sizeof(u32)];
+ int ret, i, count = DIV_ROUND_UP(size, sizeof(u32));
+
+ ret = regmap_bulk_read(tc->regmap, DP0_AUXRDATA(0), auxrdata, count);
+ if (ret)
+ return ret;
+
+ for (i = 0; i < count; i++) {
+ /*
+* Our regmap is configured as LE for register data,
+* so we need undo any byte swapping that might have
+* happened to preserve original byte order.
+*/
+ le32_to_cpus(&auxrdata[i]);
+ }
+
+ memcpy(data, auxrdata, size);
+
+ return size;
+}
+
static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
struct drm_dp_aux_msg *msg)
{
@@ -379,19 +402,10 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
if (ret)
return ret;
- if (request == DP_AUX_I2C_READ || request == DP_AUX_NATIVE_READ) {
- /* Read data */
- while (i < size) {
- if ((i % 4) == 0) {
- ret = regmap_read(tc->regmap,
- DP0_AUXRDATA(i >> 2), &tmp);
- if (ret)
- return ret;
- }
- buf[i] = tmp & 0xff;
- tmp = tmp >> 8;
- i++;
- }
+ switch (request) {
+ case DP_AUX_NATIVE_READ:
+ case DP_AUX_I2C_READ:
+ return tc_aux_read_data(tc, msg->buffer, size);
}
return size;
--
2.21.0
[PATCH v3 07/15] drm/bridge: tc358767: Simplify AUX data write
Simplify AUX data write by dropping index arithmetic and shifting and
replacing it with a call to a helper function that does three things:
1. Copies user-provided data into a write buffer
2. Optionally fixes the endianness of the write buffer (not needed
on LE hosts)
3. Transfers contenst of the write buffer to up to 4 32-bit
registers on the chip
Note that separate data endianness fix:
tmp = (tmp << 8) | buf[i];
that was reserved for DP_AUX_I2C_WRITE looks really strange, since it
will place data differently depending on the passed user-data
size. E.g. for a write of 1 byte, data transferred to the chip would
look like:
[byte0] [dummy1] [dummy2] [dummy3]
whereas for a write of 4 bytes we'd get:
[byte3] [byte2] [byte1] [byte0]
Since there's no indication in the datasheet that I2C write buffer
should be treated differently than AUX write buffer and no comment in
the original code explaining why it was done this way, that special
I2C write buffer transformation was dropped in this patch.
Signed-off-by: Andrey Smirnov
Cc: Archit Taneja
Cc: Andrzej Hajda
Cc: Laurent Pinchart
Cc: Tomi Valkeinen
Cc: Andrey Gusakov
Cc: Philipp Zabel
Cc: Cory Tusar
Cc: Chris Healy
Cc: Lucas Stach
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
drivers/gpu/drm/bridge/tc358767.c | 59 +++
1 file changed, 37 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c
b/drivers/gpu/drm/bridge/tc358767.c
index da47d81e7109..260fbcd0271e 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -321,6 +321,32 @@ static int tc_aux_get_status(struct tc_data *tc, u8 *reply)
return 0;
}
+static int tc_aux_write_data(struct tc_data *tc, const void *data,
+size_t size)
+{
+ u32 auxwdata[DP_AUX_MAX_PAYLOAD_BYTES / sizeof(u32)] = { 0 };
+ int ret, i, count = DIV_ROUND_UP(size, sizeof(u32));
+
+ memcpy(auxwdata, data, size);
+
+ for (i = 0; i < count; i++) {
+ /*
+* Our regmap is configured as LE
+* for register data, so we need
+* undo any byte swapping that will
+* happened to preserve original
+* byte order.
+*/
+ cpu_to_le32s(&auxwdata[i]);
+ }
+
+ ret = regmap_bulk_write(tc->regmap, DP0_AUXWDATA(0), auxwdata, count);
+ if (ret)
+ return ret;
+
+ return size;
+}
+
static int tc_aux_read_data(struct tc_data *tc, void *data, size_t size)
{
u32 auxrdata[DP_AUX_MAX_PAYLOAD_BYTES / sizeof(u32)];
@@ -350,9 +376,6 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
struct tc_data *tc = aux_to_tc(aux);
size_t size = min_t(size_t, 8, msg->size);
u8 request = msg->request & ~DP_AUX_I2C_MOT;
- u8 *buf = msg->buffer;
- u32 tmp = 0;
- int i = 0;
int ret;
if (size == 0)
@@ -362,25 +385,17 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
if (ret)
return ret;
- if (request == DP_AUX_I2C_WRITE || request == DP_AUX_NATIVE_WRITE) {
- /* Store data */
- while (i < size) {
- if (request == DP_AUX_NATIVE_WRITE)
- tmp = tmp | (buf[i] << (8 * (i & 0x3)));
- else
- tmp = (tmp << 8) | buf[i];
- i++;
- if (((i % 4) == 0) || (i == size)) {
- ret = regmap_write(tc->regmap,
- DP0_AUXWDATA((i - 1) >> 2),
- tmp);
- if (ret)
- return ret;
- tmp = 0;
- }
- }
- } else if (request != DP_AUX_I2C_READ &&
- request != DP_AUX_NATIVE_READ) {
+ switch (request) {
+ case DP_AUX_NATIVE_READ:
+ case DP_AUX_I2C_READ:
+ break;
+ case DP_AUX_NATIVE_WRITE:
+ case DP_AUX_I2C_WRITE:
+ ret = tc_aux_write_data(tc, msg->buffer, size);
+ if (ret < 0)
+ return ret;
+ break;
+ default:
return -EINVAL;
}
--
2.21.0
[PATCH v3 13/15] drm/bridge: tc358767: Simplify tc_aux_wait_busy()
We never pass anything but 100 as timeout_ms to tc_aux_wait_busy(), so
we may as well hardcode that value and simplify function's signature.
Signed-off-by: Andrey Smirnov
Cc: Archit Taneja
Cc: Andrzej Hajda
Cc: Laurent Pinchart
Cc: Tomi Valkeinen
Cc: Andrey Gusakov
Cc: Philipp Zabel
Cc: Cory Tusar
Cc: Chris Healy
Cc: Lucas Stach
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
drivers/gpu/drm/bridge/tc358767.c | 9 -
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c
b/drivers/gpu/drm/bridge/tc358767.c
index a04401cf2a92..4fe7641f84ee 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -294,10 +294,9 @@ static inline int tc_poll_timeout(struct tc_data *tc,
unsigned int addr,
sleep_us, timeout_us);
}
-static int tc_aux_wait_busy(struct tc_data *tc, unsigned int timeout_ms)
+static int tc_aux_wait_busy(struct tc_data *tc)
{
- return tc_poll_timeout(tc, DP0_AUXSTATUS, AUX_BUSY, 0,
- 1000, 1000 * timeout_ms);
+ return tc_poll_timeout(tc, DP0_AUXSTATUS, AUX_BUSY, 0, 1000, 10);
}
static int tc_aux_write_data(struct tc_data *tc, const void *data,
@@ -370,7 +369,7 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
u32 auxstatus;
int ret;
- ret = tc_aux_wait_busy(tc, 100);
+ ret = tc_aux_wait_busy(tc);
if (ret)
return ret;
@@ -397,7 +396,7 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
if (ret)
return ret;
- ret = tc_aux_wait_busy(tc, 100);
+ ret = tc_aux_wait_busy(tc);
if (ret)
return ret;
--
2.21.0
[PATCH v3 02/15] drm/bridge: tc358767: Simplify polling in tc_main_link_setup()
Replace explicit polling loop with equivalent call to
tc_poll_timeout() for brevity. No functional change intended.
Signed-off-by: Andrey Smirnov
Cc: Archit Taneja
Cc: Andrzej Hajda
Cc: Laurent Pinchart
Cc: Tomi Valkeinen
Cc: Andrey Gusakov
Cc: Philipp Zabel
Cc: Cory Tusar
Cc: Chris Healy
Cc: Lucas Stach
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
drivers/gpu/drm/bridge/tc358767.c | 15 +--
1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c
b/drivers/gpu/drm/bridge/tc358767.c
index fb8a1942ec54..5e1e73a91696 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -774,7 +774,6 @@ static int tc_main_link_enable(struct tc_data *tc)
struct device *dev = tc->dev;
unsigned int rate;
u32 dp_phy_ctrl;
- int timeout;
u32 value;
int ret;
u8 tmp[8];
@@ -831,15 +830,11 @@ static int tc_main_link_enable(struct tc_data *tc)
dp_phy_ctrl &= ~(DP_PHY_RST | PHY_M1_RST | PHY_M0_RST);
tc_write(DP_PHY_CTRL, dp_phy_ctrl);
- timeout = 1000;
- do {
- tc_read(DP_PHY_CTRL, &value);
- udelay(1);
- } while ((!(value & PHY_RDY)) && (--timeout));
-
- if (timeout == 0) {
- dev_err(dev, "timeout waiting for phy become ready");
- return -ETIMEDOUT;
+ ret = tc_poll_timeout(tc, DP_PHY_CTRL, PHY_RDY, PHY_RDY, 1, 1000);
+ if (ret) {
+ if (ret == -ETIMEDOUT)
+ dev_err(dev, "timeout waiting for phy become ready");
+ return ret;
}
/* Set misc: 8 bits per color */
--
2.21.0
[PATCH v3 00/15] tc358767 driver improvements
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 [v2]: - Patchset rebased on top of v4 of Tomi's series that recently went in (https://patchwork.freedesktop.org/series/58176/#rev5) - AUX transfer code converted to user regmap_bulk_read(), regmap_bulk_write() 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() [v2] lkml.kernel.org/r/20190322032901.12045-1-andrew.smir...@gmail.com [v1] lkml.kernel.org/r/20190226193609.9862-1-andrew.smir...@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 | 668 ++ 1 file changed, 392 insertions(+), 276 deletions(-) -- 2.21.0
[PATCH v3 10/15] drm/bridge: tc358767: Add support for address-only I2C transfers
Transfer size of zero means a request to do an address-only
transfer. Since the HW support this, we probably shouldn't be just
ignoring such requests. While at it allow DP_AUX_I2C_MOT flag to pass
through, since it is supported by the HW as well.
Signed-off-by: Andrey Smirnov
Cc: Archit Taneja
Cc: Andrzej Hajda
Cc: Laurent Pinchart
Cc: Tomi Valkeinen
Cc: Andrey Gusakov
Cc: Philipp Zabel
Cc: Cory Tusar
Cc: Chris Healy
Cc: Lucas Stach
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
drivers/gpu/drm/bridge/tc358767.c | 30 +++---
1 file changed, 23 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c
b/drivers/gpu/drm/bridge/tc358767.c
index 90ec33caacbc..7b84fbb72973 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -145,6 +145,8 @@
/* AUX channel */
#define DP0_AUXCFG00x0660
+#define DP0_AUXCFG0_BSIZE GENMASK(11, 8)
+#define DP0_AUXCFG0_ADDR_ONLY BIT(4)
#define DP0_AUXCFG10x0664
#define AUX_RX_FILTER_EN BIT(16)
@@ -347,6 +349,18 @@ static int tc_aux_read_data(struct tc_data *tc, void
*data, size_t size)
return size;
}
+static u32 tc_auxcfg0(struct drm_dp_aux_msg *msg, size_t size)
+{
+ u32 auxcfg0 = msg->request;
+
+ if (size)
+ auxcfg0 |= FIELD_PREP(DP0_AUXCFG0_BSIZE, size - 1);
+ else
+ auxcfg0 |= DP0_AUXCFG0_ADDR_ONLY;
+
+ return auxcfg0;
+}
+
static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
struct drm_dp_aux_msg *msg)
{
@@ -356,9 +370,6 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
u32 auxstatus;
int ret;
- if (size == 0)
- return 0;
-
ret = tc_aux_wait_busy(tc, 100);
if (ret)
return ret;
@@ -382,8 +393,7 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
if (ret)
return ret;
/* Start transfer */
- ret = regmap_write(tc->regmap, DP0_AUXCFG0,
- ((size - 1) << 8) | request);
+ ret = regmap_write(tc->regmap, DP0_AUXCFG0, tc_auxcfg0(msg, size));
if (ret)
return ret;
@@ -397,8 +407,14 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
if (auxstatus & AUX_TIMEOUT)
return -ETIMEDOUT;
-
- size = FIELD_GET(AUX_BYTES, auxstatus);
+ /*
+* For some reason address-only DP_AUX_I2C_WRITE (MOT), still
+* reports 1 byte transferred in its status. To deal we that
+* we ignore aux_bytes field if we know that this was an
+* address-only transfer
+*/
+ if (size)
+ size = FIELD_GET(AUX_BYTES, auxstatus);
msg->reply = FIELD_GET(AUX_STATUS, auxstatus);
switch (request) {
--
2.21.0
[PATCH v3 12/15] drm/bridge: tc358767: Introduce tc_pllupdate_pllen()
tc_wait_pll_lock() is always called as a follow-up for updating
PLLUPDATE and PLLEN bit of a given PLL control register. To simplify
things, merge the two operation into a single helper function
tc_pllupdate_pllen() and convert the rest of the code to use it. No
functional change intended.
Signed-off-by: Andrey Smirnov
Reviewed-by: Laurent Pinchart
Cc: Archit Taneja
Cc: Andrzej Hajda
Cc: Laurent Pinchart
Cc: Tomi Valkeinen
Cc: Andrey Gusakov
Cc: Philipp Zabel
Cc: Cory Tusar
Cc: Chris Healy
Cc: Lucas Stach
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
drivers/gpu/drm/bridge/tc358767.c | 30 ++
1 file changed, 14 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c
b/drivers/gpu/drm/bridge/tc358767.c
index c58714daa0a1..a04401cf2a92 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -463,10 +463,18 @@ static u32 tc_srcctrl(struct tc_data *tc)
return reg;
}
-static void tc_wait_pll_lock(struct tc_data *tc)
+static int tc_pllupdate_pllen(struct tc_data *tc, unsigned int pllctrl)
{
+ int ret;
+
+ ret = regmap_write(tc->regmap, pllctrl, PLLUPDATE | PLLEN);
+ if (ret)
+ return ret;
+
/* Wait for PLL to lock: up to 2.09 ms, depending on refclk */
usleep_range(3000, 6000);
+
+ return 0;
}
static int tc_pxl_pll_en(struct tc_data *tc, u32 refclk, u32 pixelclock)
@@ -566,13 +574,7 @@ static int tc_pxl_pll_en(struct tc_data *tc, u32 refclk,
u32 pixelclock)
return ret;
/* Force PLL parameter update and disable bypass */
- ret = regmap_write(tc->regmap, PXL_PLLCTRL, PLLUPDATE | PLLEN);
- if (ret)
- return ret;
-
- tc_wait_pll_lock(tc);
-
- return 0;
+ return tc_pllupdate_pllen(tc, PXL_PLLCTRL);
}
static int tc_pxl_pll_dis(struct tc_data *tc)
@@ -645,15 +647,13 @@ static int tc_aux_link_setup(struct tc_data *tc)
* Initially PLLs are in bypass. Force PLL parameter update,
* disable PLL bypass, enable PLL
*/
- ret = regmap_write(tc->regmap, DP0_PLLCTRL, PLLUPDATE | PLLEN);
+ ret = tc_pllupdate_pllen(tc, DP0_PLLCTRL);
if (ret)
goto err;
- tc_wait_pll_lock(tc);
- ret = regmap_write(tc->regmap, DP1_PLLCTRL, PLLUPDATE | PLLEN);
+ ret = tc_pllupdate_pllen(tc, DP1_PLLCTRL);
if (ret)
goto err;
- tc_wait_pll_lock(tc);
ret = tc_poll_timeout(tc, DP_PHY_CTRL, PHY_RDY, PHY_RDY, 1, 1000);
if (ret == -ETIMEDOUT) {
@@ -933,15 +933,13 @@ static int tc_main_link_enable(struct tc_data *tc)
return ret;
/* PLL setup */
- ret = regmap_write(tc->regmap, DP0_PLLCTRL, PLLUPDATE | PLLEN);
+ ret = tc_pllupdate_pllen(tc, DP0_PLLCTRL);
if (ret)
return ret;
- tc_wait_pll_lock(tc);
- ret = regmap_write(tc->regmap, DP1_PLLCTRL, PLLUPDATE | PLLEN);
+ ret = tc_pllupdate_pllen(tc, DP1_PLLCTRL);
if (ret)
return ret;
- tc_wait_pll_lock(tc);
/* Reset/Enable Main Links */
dp_phy_ctrl |= DP_PHY_RST | PHY_M1_RST | PHY_M0_RST;
--
2.21.0
[PATCH v3 01/15] drm/bridge: tc358767: Simplify tc_poll_timeout()
Implementation of tc_poll_timeout() is almost a 100% copy-and-paste of
the code for regmap_read_poll_timeout(). Replace copied code with a
call to the original. While at it change tc_poll_timeout to accept
"struct tc_data *" instead of "struct regmap *" for brevity. No
functional change intended.
Signed-off-by: Andrey Smirnov
Reviewed-by: Andrzej Hajda
Reviewed-by: Laurent Pinchart
Cc: Archit Taneja
Cc: Andrzej Hajda
Cc: Laurent Pinchart
Cc: Tomi Valkeinen
Cc: Andrey Gusakov
Cc: Philipp Zabel
Cc: Cory Tusar
Cc: Chris Healy
Cc: Lucas Stach
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
drivers/gpu/drm/bridge/tc358767.c | 26 ++
1 file changed, 6 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c
b/drivers/gpu/drm/bridge/tc358767.c
index 58e3ca0e25af..fb8a1942ec54 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -264,34 +264,21 @@ static inline struct tc_data *connector_to_tc(struct
drm_connector *c)
goto err; \
} while (0)
-static inline int tc_poll_timeout(struct regmap *map, unsigned int addr,
+static inline int tc_poll_timeout(struct tc_data *tc, unsigned int addr,
unsigned int cond_mask,
unsigned int cond_value,
unsigned long sleep_us, u64 timeout_us)
{
- ktime_t timeout = ktime_add_us(ktime_get(), timeout_us);
unsigned int val;
- int ret;
- for (;;) {
- ret = regmap_read(map, addr, &val);
- if (ret)
- break;
- if ((val & cond_mask) == cond_value)
- break;
- if (timeout_us && ktime_compare(ktime_get(), timeout) > 0) {
- ret = regmap_read(map, addr, &val);
- break;
- }
- if (sleep_us)
- usleep_range((sleep_us >> 2) + 1, sleep_us);
- }
- return ret ?: (((val & cond_mask) == cond_value) ? 0 : -ETIMEDOUT);
+ return regmap_read_poll_timeout(tc->regmap, addr, val,
+ (val & cond_mask) == cond_value,
+ sleep_us, timeout_us);
}
static int tc_aux_wait_busy(struct tc_data *tc, unsigned int timeout_ms)
{
- return tc_poll_timeout(tc->regmap, DP0_AUXSTATUS, AUX_BUSY, 0,
+ return tc_poll_timeout(tc, DP0_AUXSTATUS, AUX_BUSY, 0,
1000, 1000 * timeout_ms);
}
@@ -598,8 +585,7 @@ static int tc_aux_link_setup(struct tc_data *tc)
tc_write(DP1_PLLCTRL, PLLUPDATE | PLLEN);
tc_wait_pll_lock(tc);
- ret = tc_poll_timeout(tc->regmap, DP_PHY_CTRL, PHY_RDY, PHY_RDY, 1,
- 1000);
+ ret = tc_poll_timeout(tc, DP_PHY_CTRL, PHY_RDY, PHY_RDY, 1, 1000);
if (ret == -ETIMEDOUT) {
dev_err(tc->dev, "Timeout waiting for PHY to become ready");
return ret;
--
2.21.0
[PATCH v3 05/15] drm/bridge: tc358767: Drop custom tc_write()/tc_read() accessors
A very unfortunate aspect of tc_write()/tc_read() macro helpers is
that they capture quite a bit of context around them and thus require
the caller to have magic variables 'ret' and 'tc' as well as label
'err'. That makes a number of code paths rather counterintuitive and
somewhat clunky, for example tc_stream_clock_calc() ends up being like
this:
int ret;
tc_write(DP0_VIDMNGEN1, 32768);
return 0;
err:
return ret;
which is rather surprising when you read the code for the first
time. Since those helpers arguably aren't really saving that much code
and there's no way of fixing them without making them too verbose to
be worth it change the driver code to not use them at all.
Signed-off-by: Andrey Smirnov
Cc: Archit Taneja
Cc: Andrzej Hajda
Cc: Laurent Pinchart
Cc: Tomi Valkeinen
Cc: Andrey Gusakov
Cc: Philipp Zabel
Cc: Cory Tusar
Cc: Chris Healy
Cc: Lucas Stach
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
drivers/gpu/drm/bridge/tc358767.c | 381 ++
1 file changed, 229 insertions(+), 152 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c
b/drivers/gpu/drm/bridge/tc358767.c
index c0fc686ce5ec..e197ce0fb166 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -280,20 +280,6 @@ static inline struct tc_data *connector_to_tc(struct
drm_connector *c)
return container_of(c, struct tc_data, connector);
}
-/* Simple macros to avoid repeated error checks */
-#define tc_write(reg, var) \
- do {\
- ret = regmap_write(tc->regmap, reg, var); \
- if (ret)\
- goto err; \
- } while (0)
-#define tc_read(reg, var) \
- do {\
- ret = regmap_read(tc->regmap, reg, var);\
- if (ret)\
- goto err; \
- } while (0)
-
static inline int tc_poll_timeout(struct tc_data *tc, unsigned int addr,
unsigned int cond_mask,
unsigned int cond_value,
@@ -351,7 +337,7 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
ret = tc_aux_wait_busy(tc, 100);
if (ret)
- goto err;
+ return ret;
if (request == DP_AUX_I2C_WRITE || request == DP_AUX_NATIVE_WRITE) {
/* Store data */
@@ -362,7 +348,11 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
tmp = (tmp << 8) | buf[i];
i++;
if (((i % 4) == 0) || (i == size)) {
- tc_write(DP0_AUXWDATA((i - 1) >> 2), tmp);
+ ret = regmap_write(tc->regmap,
+ DP0_AUXWDATA((i - 1) >> 2),
+ tmp);
+ if (ret)
+ return ret;
tmp = 0;
}
}
@@ -372,23 +362,32 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
}
/* Store address */
- tc_write(DP0_AUXADDR, msg->address);
+ ret = regmap_write(tc->regmap, DP0_AUXADDR, msg->address);
+ if (ret)
+ return ret;
/* Start transfer */
- tc_write(DP0_AUXCFG0, ((size - 1) << 8) | request);
+ ret = regmap_write(tc->regmap, DP0_AUXCFG0,
+ ((size - 1) << 8) | request);
+ if (ret)
+ return ret;
ret = tc_aux_wait_busy(tc, 100);
if (ret)
- goto err;
+ return ret;
ret = tc_aux_get_status(tc, &msg->reply);
if (ret)
- goto err;
+ return ret;
if (request == DP_AUX_I2C_READ || request == DP_AUX_NATIVE_READ) {
/* Read data */
while (i < size) {
- if ((i % 4) == 0)
- tc_read(DP0_AUXRDATA(i >> 2), &tmp);
+ if ((i % 4) == 0) {
+ ret = regmap_read(tc->regmap,
+ DP0_AUXRDATA(i >> 2), &tmp);
+ if (ret)
+ return ret;
+ }
buf[i] = tmp & 0xff;
tmp = tmp >> 8;
i++;
@@ -396,8 +395,6 @@ static ssiz
[PATCH v3 15/15] drm/bridge: tc358767: Replace magic number in tc_main_link_enable()
We don't need 8 byte array, DP_LINK_STATUS_SIZE (6) should be enough. This also gets rid of a magic number as a bonus. Signed-off-by: Andrey Smirnov Cc: Archit Taneja Cc: Andrzej Hajda Cc: Laurent Pinchart Cc: Tomi Valkeinen Cc: Andrey Gusakov Cc: Philipp Zabel Cc: Cory Tusar Cc: Chris Healy Cc: Lucas Stach Cc: dri-devel@lists.freedesktop.org Cc: linux-ker...@vger.kernel.org --- drivers/gpu/drm/bridge/tc358767.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c index 41a976dff13b..cf38f943e656 100644 --- a/drivers/gpu/drm/bridge/tc358767.c +++ b/drivers/gpu/drm/bridge/tc358767.c @@ -893,7 +893,7 @@ static int tc_main_link_enable(struct tc_data *tc) u32 dp_phy_ctrl; u32 value; int ret; - u8 tmp[8]; + u8 tmp[DP_LINK_STATUS_SIZE]; dev_dbg(tc->dev, "link enable\n"); -- 2.21.0
[PATCH v3 08/15] drm/bridge: tc358767: Increase AUX transfer length limit
According to the datasheet tc358767 can transfer up to 16 bytes via
its AUX channel, so the artificial limit of 8 apperas to be too
low. However only up to 15-bytes seem to be actually supported and
trying to use 16-byte transfers results in transfers failing
sporadically (with bogus status in case of I2C transfers), so limit it
to 15.
Signed-off-by: Andrey Smirnov
Cc: Archit Taneja
Cc: Andrzej Hajda
Cc: Laurent Pinchart
Cc: Tomi Valkeinen
Cc: Andrey Gusakov
Cc: Philipp Zabel
Cc: Cory Tusar
Cc: Chris Healy
Cc: Lucas Stach
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
drivers/gpu/drm/bridge/tc358767.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c
b/drivers/gpu/drm/bridge/tc358767.c
index 260fbcd0271e..0125e2f7e076 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -374,7 +374,7 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
struct drm_dp_aux_msg *msg)
{
struct tc_data *tc = aux_to_tc(aux);
- size_t size = min_t(size_t, 8, msg->size);
+ size_t size = min_t(size_t, DP_AUX_MAX_PAYLOAD_BYTES - 1, msg->size);
u8 request = msg->request & ~DP_AUX_I2C_MOT;
int ret;
--
2.21.0
[PATCH v3 04/15] drm/bridge: tc358767: Simplify tc_set_video_mode()
Simplify tc_set_video_mode() by replacing explicit shifting using macros from . No functional change intended. Signed-off-by: Andrey Smirnov Cc: Archit Taneja Cc: Andrzej Hajda Cc: Laurent Pinchart Cc: Tomi Valkeinen Cc: Andrey Gusakov Cc: Philipp Zabel Cc: Cory Tusar Cc: Chris Healy Cc: Lucas Stach Cc: dri-devel@lists.freedesktop.org Cc: linux-ker...@vger.kernel.org --- drivers/gpu/drm/bridge/tc358767.c | 106 ++ 1 file changed, 78 insertions(+), 28 deletions(-) diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c index 115cffc55a96..c0fc686ce5ec 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 #include #include #include @@ -56,6 +57,7 @@ /* Video Path */ #define VPCTRL00x0450 +#define VSDELAYGENMASK(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 HPWGENMASK(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 */ @@ -108,14 +118,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_TOTALGENMASK(15, 0) +#define V_TOTALGENMASK(31, 16) #define DP0_STARTVAL 0x064c +#define H_STARTGENMASK(15, 0) +#define V_STARTGENMASK(31, 16) #define DP0_ACTIVEVAL 0x0650 +#define H_ACT GENMASK(15, 0) +#define V_ACT GENMASK(31, 16) + #define DP0_SYNCVAL0x0654 +#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_SIZEGENMASK(21, 16) #define BPC_6 (0 << 5) #define BPC_8 (1 << 5) @@ -192,6 +216,12 @@ /* Test & Debug */ #define TSTCTL 0x0a00 +#define COLOR_RGENMASK(31, 24) +#define COLOR_GGENMASK(23, 16) +#define COLOR_BGENMASK(15, 8) +#define ENI2CFILTERBIT(4) +#define COLOR_BAR_MODE GENMASK(1, 0) +#define COLOR_BAR_MODE_BARS2 #define PLL_DBG0x0a04 static bool tc_test_pattern; @@ -672,6 +702,7 @@ static int tc_set_video_mode(struct tc_data *tc, int upper_margin = mode->vtotal - mode->vsync_end; int lower_margin = mode->vsync_start - mode->vdisplay; int vsync_len = mode->vsync_end - mode->vsync_start; + u32 dp0_syncval; /* * Recommended maximum number of symbols transferred in a transfer unit: @@ -696,50 +727,69 @@ static int tc_set_video_mode(struct tc_data *tc, * assume we do not need any delay when DPI is a source of * sync signals */ - tc_write(VPCTRL0, (0 << 20) /* VSDELAY */ | + tc_write(VPCTRL0, +FIELD_PREP(VSDELAY, 0) | 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 <<
[PATCH v3 14/15] drm/bridge: tc358767: Drop unnecessary 8 byte buffer
tc_get_display_props() never reads more than a byte via AUX, so
there's no need to reserve 8 for that purpose. No function change
intended.
Signed-off-by: Andrey Smirnov
Cc: Archit Taneja
Cc: Andrzej Hajda
Cc: Laurent Pinchart
Cc: Tomi Valkeinen
Cc: Andrey Gusakov
Cc: Philipp Zabel
Cc: Cory Tusar
Cc: Chris Healy
Cc: Lucas Stach
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
drivers/gpu/drm/bridge/tc358767.c | 13 ++---
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c
b/drivers/gpu/drm/bridge/tc358767.c
index 4fe7641f84ee..41a976dff13b 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -680,8 +680,7 @@ static int tc_aux_link_setup(struct tc_data *tc)
static int tc_get_display_props(struct tc_data *tc)
{
int ret;
- /* temp buffer */
- u8 tmp[8];
+ u8 reg;
/* Read DP Rx Link Capability */
ret = drm_dp_link_probe(&tc->aux, &tc->link.base);
@@ -697,21 +696,21 @@ static int tc_get_display_props(struct tc_data *tc)
tc->link.base.num_lanes = 2;
}
- ret = drm_dp_dpcd_readb(&tc->aux, DP_MAX_DOWNSPREAD, tmp);
+ ret = drm_dp_dpcd_readb(&tc->aux, DP_MAX_DOWNSPREAD, ®);
if (ret < 0)
goto err_dpcd_read;
- tc->link.spread = tmp[0] & DP_MAX_DOWNSPREAD_0_5;
+ tc->link.spread = reg & DP_MAX_DOWNSPREAD_0_5;
- ret = drm_dp_dpcd_readb(&tc->aux, DP_MAIN_LINK_CHANNEL_CODING, tmp);
+ ret = drm_dp_dpcd_readb(&tc->aux, DP_MAIN_LINK_CHANNEL_CODING, ®);
if (ret < 0)
goto err_dpcd_read;
tc->link.scrambler_dis = false;
/* read assr */
- ret = drm_dp_dpcd_readb(&tc->aux, DP_EDP_CONFIGURATION_SET, tmp);
+ ret = drm_dp_dpcd_readb(&tc->aux, DP_EDP_CONFIGURATION_SET, ®);
if (ret < 0)
goto err_dpcd_read;
- tc->link.assr = tmp[0] & DP_ALTERNATE_SCRAMBLER_RESET_ENABLE;
+ tc->link.assr = reg & DP_ALTERNATE_SCRAMBLER_RESET_ENABLE;
dev_dbg(tc->dev, "DPCD rev: %d.%d, rate: %s, lanes: %d, framing: %s\n",
tc->link.base.revision >> 4, tc->link.base.revision & 0x0f,
--
2.21.0
[PATCH v3 11/15] drm/bridge: tc358767: Introduce tc_set_syspllparam()
Move common code converting clock rate to an appropriate constant and
configuring SYS_PLLPARAM register into a separate routine and convert
the rest of the code to use it. No functional change intended.
Signed-off-by: Andrey Smirnov
Reviewed-by: Laurent Pinchart
Cc: Archit Taneja
Cc: Andrzej Hajda
Cc: Laurent Pinchart
Cc: Tomi Valkeinen
Cc: Andrey Gusakov
Cc: Philipp Zabel
Cc: Chris Healy
Cc: Cory Tusar
Cc: Lucas Stach
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
drivers/gpu/drm/bridge/tc358767.c | 46 +++
1 file changed, 16 insertions(+), 30 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c
b/drivers/gpu/drm/bridge/tc358767.c
index 7b84fbb72973..c58714daa0a1 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -601,35 +601,40 @@ static int tc_stream_clock_calc(struct tc_data *tc)
return regmap_write(tc->regmap, DP0_VIDMNGEN1, 32768);
}
-static int tc_aux_link_setup(struct tc_data *tc)
+static int tc_set_syspllparam(struct tc_data *tc)
{
unsigned long rate;
- u32 dp0_auxcfg1;
- u32 value;
- int ret;
+ u32 pllparam = SYSCLK_SEL_LSCLK | LSCLK_DIV_2;
rate = clk_get_rate(tc->refclk);
switch (rate) {
case 3840:
- value = REF_FREQ_38M4;
+ pllparam |= REF_FREQ_38M4;
break;
case 2600:
- value = REF_FREQ_26M;
+ pllparam |= REF_FREQ_26M;
break;
case 1920:
- value = REF_FREQ_19M2;
+ pllparam |= REF_FREQ_19M2;
break;
case 1300:
- value = REF_FREQ_13M;
+ pllparam |= REF_FREQ_13M;
break;
default:
dev_err(tc->dev, "Invalid refclk rate: %lu Hz\n", rate);
return -EINVAL;
}
+ return regmap_write(tc->regmap, SYS_PLLPARAM, pllparam);
+}
+
+static int tc_aux_link_setup(struct tc_data *tc)
+{
+ int ret;
+ u32 dp0_auxcfg1;
+
/* Setup DP-PHY / PLL */
- value |= SYSCLK_SEL_LSCLK | LSCLK_DIV_2;
- ret = regmap_write(tc->regmap, SYS_PLLPARAM, value);
+ ret = tc_set_syspllparam(tc);
if (ret)
goto err;
@@ -887,7 +892,6 @@ static int tc_main_link_enable(struct tc_data *tc)
{
struct drm_dp_aux *aux = &tc->aux;
struct device *dev = tc->dev;
- unsigned int rate;
u32 dp_phy_ctrl;
u32 value;
int ret;
@@ -915,25 +919,7 @@ static int tc_main_link_enable(struct tc_data *tc)
if (ret)
return ret;
- rate = clk_get_rate(tc->refclk);
- switch (rate) {
- case 3840:
- value = REF_FREQ_38M4;
- break;
- case 2600:
- value = REF_FREQ_26M;
- break;
- case 1920:
- value = REF_FREQ_19M2;
- break;
- case 1300:
- value = REF_FREQ_13M;
- break;
- default:
- return -EINVAL;
- }
- value |= SYSCLK_SEL_LSCLK | LSCLK_DIV_2;
- ret = regmap_write(tc->regmap, SYS_PLLPARAM, value);
+ ret = tc_set_syspllparam(tc);
if (ret)
return ret;
--
2.21.0
[PATCH v3 09/15] drm/bridge: tc358767: Use reported AUX transfer size
Don't assume that requested data transfer size is the same as amount
of data that was transferred. Change the code to get that information
from DP0_AUXSTATUS instead.
Since the check for AUX_BUSY in tc_aux_get_status() is pointless (it
will always called after tc_aux_wait_busy()) and there's only one user
of it, inline its code into tc_aux_transfer() instead of trying to
accommodate the change above.
Signed-off-by: Andrey Smirnov
Cc: Archit Taneja
Cc: Andrzej Hajda
Cc: Laurent Pinchart
Cc: Tomi Valkeinen
Cc: Andrey Gusakov
Cc: Philipp Zabel
Cc: Cory Tusar
Cc: Chris Healy
Cc: Lucas Stach
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
drivers/gpu/drm/bridge/tc358767.c | 40 ++-
1 file changed, 12 insertions(+), 28 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c
b/drivers/gpu/drm/bridge/tc358767.c
index 0125e2f7e076..90ec33caacbc 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -152,10 +152,10 @@
#define DP0_AUXWDATA(i)(0x066c + (i) * 4)
#define DP0_AUXRDATA(i)(0x067c + (i) * 4)
#define DP0_AUXSTATUS 0x068c
-#define AUX_STATUS_MASK0xf0
-#define AUX_STATUS_SHIFT 4
-#define AUX_TIMEOUTBIT(1)
-#define AUX_BUSY BIT(0)
+#define AUX_BYTES GENMASK(15, 8)
+#define AUX_STATUS GENMASK(7, 4)
+#define AUX_TIMEOUTBIT(1)
+#define AUX_BUSY BIT(0)
#define DP0_AUXI2CADR 0x0698
/* Link Training */
@@ -298,29 +298,6 @@ static int tc_aux_wait_busy(struct tc_data *tc, unsigned
int timeout_ms)
1000, 1000 * timeout_ms);
}
-static int tc_aux_get_status(struct tc_data *tc, u8 *reply)
-{
- int ret;
- u32 value;
-
- ret = regmap_read(tc->regmap, DP0_AUXSTATUS, &value);
- if (ret < 0)
- return ret;
-
- if (value & AUX_BUSY) {
- dev_err(tc->dev, "aux busy!\n");
- return -EBUSY;
- }
-
- if (value & AUX_TIMEOUT) {
- dev_err(tc->dev, "aux access timeout!\n");
- return -ETIMEDOUT;
- }
-
- *reply = (value & AUX_STATUS_MASK) >> AUX_STATUS_SHIFT;
- return 0;
-}
-
static int tc_aux_write_data(struct tc_data *tc, const void *data,
size_t size)
{
@@ -376,6 +353,7 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
struct tc_data *tc = aux_to_tc(aux);
size_t size = min_t(size_t, DP_AUX_MAX_PAYLOAD_BYTES - 1, msg->size);
u8 request = msg->request & ~DP_AUX_I2C_MOT;
+ u32 auxstatus;
int ret;
if (size == 0)
@@ -413,10 +391,16 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
if (ret)
return ret;
- ret = tc_aux_get_status(tc, &msg->reply);
+ ret = regmap_read(tc->regmap, DP0_AUXSTATUS, &auxstatus);
if (ret)
return ret;
+ if (auxstatus & AUX_TIMEOUT)
+ return -ETIMEDOUT;
+
+ size = FIELD_GET(AUX_BYTES, auxstatus);
+ msg->reply = FIELD_GET(AUX_STATUS, auxstatus);
+
switch (request) {
case DP_AUX_NATIVE_READ:
case DP_AUX_I2C_READ:
--
2.21.0
Re: [PATCH v3 03/15] drm/bridge: tc358767: Simplify polling in tc_link_training()
On Thu, Jun 6, 2019 at 1:08 AM Andrzej Hajda wrote:
>
> On 05.06.2019 09:04, Andrey Smirnov wrote:
> > Replace explicit polling in tc_link_training() with equivalent call to
> > tc_poll_timeout() for simplicity. No functional change intended (not
> > including slightly altered debug output).
> >
> > Signed-off-by: Andrey Smirnov
> > Cc: Archit Taneja
> > Cc: Andrzej Hajda
> > Cc: Laurent Pinchart
> > Cc: Tomi Valkeinen
> > Cc: Andrey Gusakov
> > Cc: Philipp Zabel
> > Cc: Cory Tusar
> > Cc: Chris Healy
> > Cc: Lucas Stach
> > Cc: dri-devel@lists.freedesktop.org
> > Cc: linux-ker...@vger.kernel.org
> > ---
> > drivers/gpu/drm/bridge/tc358767.c | 15 ++-
> > 1 file changed, 6 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/tc358767.c
> > b/drivers/gpu/drm/bridge/tc358767.c
> > index 5e1e73a91696..115cffc55a96 100644
> > --- a/drivers/gpu/drm/bridge/tc358767.c
> > +++ b/drivers/gpu/drm/bridge/tc358767.c
> > @@ -748,22 +748,19 @@ static int tc_set_video_mode(struct tc_data *tc,
> >
> > static int tc_wait_link_training(struct tc_data *tc)
> > {
> > - u32 timeout = 1000;
> > u32 value;
> > int ret;
> >
> > - do {
> > - udelay(1);
> > - tc_read(DP0_LTSTAT, &value);
> > - } while ((!(value & LT_LOOPDONE)) && (--timeout));
> > -
> > - if (timeout == 0) {
> > + ret = tc_poll_timeout(tc, DP0_LTSTAT, LT_LOOPDONE,
> > + LT_LOOPDONE, 1, 1000);
> > + if (ret) {
> > dev_err(tc->dev, "Link training timeout waiting for
> > LT_LOOPDONE!\n");
> > - return -ETIMEDOUT;
> > + return ret;
> > }
>
>
> Inconsistent coding, in previous patch you check (ret == -ETIMEDOUT) but
> not here. To simplify the code you can assume that tc_poll_timeout < 0,
> means timeout, in such case please adjust previous patch.
>
Sure, will do.
Thanks,
Andrey Smirnov
Re: [PATCH v3 05/15] drm/bridge: tc358767: Drop custom tc_write()/tc_read() accessors
On Thu, Jun 6, 2019 at 3:34 AM Andrzej Hajda wrote:
>
> On 05.06.2019 09:04, Andrey Smirnov wrote:
> > A very unfortunate aspect of tc_write()/tc_read() macro helpers is
> > that they capture quite a bit of context around them and thus require
> > the caller to have magic variables 'ret' and 'tc' as well as label
> > 'err'. That makes a number of code paths rather counterintuitive and
> > somewhat clunky, for example tc_stream_clock_calc() ends up being like
> > this:
> >
> > int ret;
> >
> > tc_write(DP0_VIDMNGEN1, 32768);
> >
> > return 0;
> > err:
> > return ret;
> >
> > which is rather surprising when you read the code for the first
> > time. Since those helpers arguably aren't really saving that much code
> > and there's no way of fixing them without making them too verbose to
> > be worth it change the driver code to not use them at all.
>
>
> On the other side, error checking after every registry access is very
> annoying and significantly augments the code, makes it redundant and
> less readable. To avoid it one can cache error state, and do not perform
> real work until the error is clear. For example with following accessor:
>
> void tc_write(struct tc_data *tc, u32 reg, u32 val){
>
> int ret;
>
> if (tc->error) //This check is IMPORTANT
>
> return;
>
> ret =regmap_write(...);
>
> if (ret >= 0)
>
> return;
>
> tc->error = ret;
>
> dev_err(tc->dev, "Error writing register %#x\n", reg);
>
> }
>
> You can safely write code like:
>
> tc_write(tc, DP_PHY_CTRL, BGREN | PWR_SW_EN | PHY_A0_EN);
>
> tc_write(tc, DP0_PLLCTRL, PLLUPDATE | PLLEN);
>
> tc_write(tc, DP1_PLLCTRL, PLLUPDATE | PLLEN);
>
> if (tc->error) {
>
> tc->error = 0;
>
> goto err;
>
> }
>
> This is of course loose suggestion.
>
I am going to have to disagree with you on this one, unfortunately.
Using regmap API explicitly definitely makes code more verbose, less
readable or more annoying though? Not really from my perspective. With
regmap code I know what the code is doing the moment I look at it,
with the example above, not so much. I also find it annoying that I
now have to remember the tricks that tc_write is pulling internally as
well as be mindful of a global-ish error state object. My problem with
original code was that a) it traded explicitness for conciseness in a
an unfavorable way, which I still think is true for code above b) it
didn't provide a comprehensive abstraction completely removing regmap
API and still relied on things like regmap_update_bits() explicitly,
making the code even more confusing (true for above example as well).
I think this driver isn't big enough to have a dedicated person always
working on it and it will mostly see occasional commits from somewhat
random folks who are coming to the codebase fresh, so creating as
little "institutional knowledge", so to speak, in a form of a custom
exception-like mechanism and opting for explicit but verbose code
seems like a preferable choice.
Anyway, I get it that's it is a loose suggestion :-), just wanted to
provide a detailed explanation why I'd rather not go that way.
Thanks,
Andrey Smirnov
[PATCH v4 02/15] drm/bridge: tc358767: Simplify polling in tc_main_link_setup()
Replace explicit polling loop with equivalent call to
tc_poll_timeout() for brevity. No functional change intended.
Signed-off-by: Andrey Smirnov
Reviewed-by: Andrzej Hajda
Cc: Andrzej Hajda
Cc: Laurent Pinchart
Cc: Tomi Valkeinen
Cc: Andrey Gusakov
Cc: Philipp Zabel
Cc: Cory Tusar
Cc: Chris Healy
Cc: Lucas Stach
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
drivers/gpu/drm/bridge/tc358767.c | 12 +++-
1 file changed, 3 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c
b/drivers/gpu/drm/bridge/tc358767.c
index fb8a1942ec54..f463ef6d4271 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -774,7 +774,6 @@ static int tc_main_link_enable(struct tc_data *tc)
struct device *dev = tc->dev;
unsigned int rate;
u32 dp_phy_ctrl;
- int timeout;
u32 value;
int ret;
u8 tmp[8];
@@ -831,15 +830,10 @@ static int tc_main_link_enable(struct tc_data *tc)
dp_phy_ctrl &= ~(DP_PHY_RST | PHY_M1_RST | PHY_M0_RST);
tc_write(DP_PHY_CTRL, dp_phy_ctrl);
- timeout = 1000;
- do {
- tc_read(DP_PHY_CTRL, &value);
- udelay(1);
- } while ((!(value & PHY_RDY)) && (--timeout));
-
- if (timeout == 0) {
+ ret = tc_poll_timeout(tc, DP_PHY_CTRL, PHY_RDY, PHY_RDY, 1, 1000);
+ if (ret) {
dev_err(dev, "timeout waiting for phy become ready");
- return -ETIMEDOUT;
+ return ret;
}
/* Set misc: 8 bits per color */
--
2.21.0
[PATCH v4 03/15] drm/bridge: tc358767: Simplify polling in tc_link_training()
Replace explicit polling in tc_link_training() with equivalent call to
tc_poll_timeout() for simplicity. No functional change intended (not
including slightly altered debug output).
Signed-off-by: Andrey Smirnov
Reviewed-by: Andrzej Hajda
Cc: Andrzej Hajda
Cc: Laurent Pinchart
Cc: Tomi Valkeinen
Cc: Andrey Gusakov
Cc: Philipp Zabel
Cc: Cory Tusar
Cc: Chris Healy
Cc: Lucas Stach
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
drivers/gpu/drm/bridge/tc358767.c | 15 ++-
1 file changed, 6 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c
b/drivers/gpu/drm/bridge/tc358767.c
index f463ef6d4271..31f5045e7e42 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -748,22 +748,19 @@ static int tc_set_video_mode(struct tc_data *tc,
static int tc_wait_link_training(struct tc_data *tc)
{
- u32 timeout = 1000;
u32 value;
int ret;
- do {
- udelay(1);
- tc_read(DP0_LTSTAT, &value);
- } while ((!(value & LT_LOOPDONE)) && (--timeout));
-
- if (timeout == 0) {
+ ret = tc_poll_timeout(tc, DP0_LTSTAT, LT_LOOPDONE,
+ LT_LOOPDONE, 1, 1000);
+ if (ret) {
dev_err(tc->dev, "Link training timeout waiting for
LT_LOOPDONE!\n");
- return -ETIMEDOUT;
+ return ret;
}
- return (value >> 8) & 0x7;
+ tc_read(DP0_LTSTAT, &value);
+ return (value >> 8) & 0x7;
err:
return ret;
}
--
2.21.0
[PATCH v4 00/15] tc358767 driver improvements
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 [v3]: - Collected Reviewed-bys from Andrzej - Dropped explicit check for -ETIMEDOUT in "drm/bridge: tc358767: Simplify polling in tc_main_link_setup()" for consistency - AUX transfer code converted to user regmap_raw_read(), regmap_raw_write() Changes since [v2]: - Patchset rebased on top of v4 of Tomi's series that recently went in (https://patchwork.freedesktop.org/series/58176/#rev5) - AUX transfer code converted to user regmap_bulk_read(), regmap_bulk_write() 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() [v3] lkml.kernel.org/r/20190605070507.11417-1-andrew.smir...@gmail.com [v2] lkml.kernel.org/r/20190322032901.12045-1-andrew.smir...@gmail.com [v1] lkml.kernel.org/r/20190226193609.9862-1-andrew.smir...@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, 372 insertions(+), 276 deletions(-) -- 2.21.0
[PATCH v4 04/15] drm/bridge: tc358767: Simplify tc_set_video_mode()
Simplify tc_set_video_mode() by replacing explicit shifting using macros from . No functional change intended. Signed-off-by: Andrey Smirnov Reviewed-by: Andrzej Hajda Cc: Andrzej Hajda Cc: Laurent Pinchart Cc: Tomi Valkeinen Cc: Andrey Gusakov Cc: Philipp Zabel Cc: Cory Tusar Cc: Chris Healy Cc: Lucas Stach Cc: dri-devel@lists.freedesktop.org Cc: linux-ker...@vger.kernel.org --- drivers/gpu/drm/bridge/tc358767.c | 106 ++ 1 file changed, 78 insertions(+), 28 deletions(-) diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c index 31f5045e7e42..5b78021d6c5b 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 #include #include #include @@ -56,6 +57,7 @@ /* Video Path */ #define VPCTRL00x0450 +#define VSDELAYGENMASK(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 HPWGENMASK(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 */ @@ -108,14 +118,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_TOTALGENMASK(15, 0) +#define V_TOTALGENMASK(31, 16) #define DP0_STARTVAL 0x064c +#define H_STARTGENMASK(15, 0) +#define V_STARTGENMASK(31, 16) #define DP0_ACTIVEVAL 0x0650 +#define H_ACT GENMASK(15, 0) +#define V_ACT GENMASK(31, 16) + #define DP0_SYNCVAL0x0654 +#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_SIZEGENMASK(21, 16) #define BPC_6 (0 << 5) #define BPC_8 (1 << 5) @@ -192,6 +216,12 @@ /* Test & Debug */ #define TSTCTL 0x0a00 +#define COLOR_RGENMASK(31, 24) +#define COLOR_GGENMASK(23, 16) +#define COLOR_BGENMASK(15, 8) +#define ENI2CFILTERBIT(4) +#define COLOR_BAR_MODE GENMASK(1, 0) +#define COLOR_BAR_MODE_BARS2 #define PLL_DBG0x0a04 static bool tc_test_pattern; @@ -672,6 +702,7 @@ static int tc_set_video_mode(struct tc_data *tc, int upper_margin = mode->vtotal - mode->vsync_end; int lower_margin = mode->vsync_start - mode->vdisplay; int vsync_len = mode->vsync_end - mode->vsync_start; + u32 dp0_syncval; /* * Recommended maximum number of symbols transferred in a transfer unit: @@ -696,50 +727,69 @@ static int tc_set_video_mode(struct tc_data *tc, * assume we do not need any delay when DPI is a source of * sync signals */ - tc_write(VPCTRL0, (0 << 20) /* VSDELAY */ | + tc_write(VPCTRL0, +FIELD_PREP(VSDELAY, 0) | 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 <<
[PATCH v4 01/15] drm/bridge: tc358767: Simplify tc_poll_timeout()
Implementation of tc_poll_timeout() is almost a 100% copy-and-paste of
the code for regmap_read_poll_timeout(). Replace copied code with a
call to the original. While at it change tc_poll_timeout to accept
"struct tc_data *" instead of "struct regmap *" for brevity. No
functional change intended.
Signed-off-by: Andrey Smirnov
Reviewed-by: Andrzej Hajda
Reviewed-by: Laurent Pinchart
Cc: Andrzej Hajda
Cc: Laurent Pinchart
Cc: Tomi Valkeinen
Cc: Andrey Gusakov
Cc: Philipp Zabel
Cc: Cory Tusar
Cc: Chris Healy
Cc: Lucas Stach
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
drivers/gpu/drm/bridge/tc358767.c | 26 ++
1 file changed, 6 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c
b/drivers/gpu/drm/bridge/tc358767.c
index 58e3ca0e25af..fb8a1942ec54 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -264,34 +264,21 @@ static inline struct tc_data *connector_to_tc(struct
drm_connector *c)
goto err; \
} while (0)
-static inline int tc_poll_timeout(struct regmap *map, unsigned int addr,
+static inline int tc_poll_timeout(struct tc_data *tc, unsigned int addr,
unsigned int cond_mask,
unsigned int cond_value,
unsigned long sleep_us, u64 timeout_us)
{
- ktime_t timeout = ktime_add_us(ktime_get(), timeout_us);
unsigned int val;
- int ret;
- for (;;) {
- ret = regmap_read(map, addr, &val);
- if (ret)
- break;
- if ((val & cond_mask) == cond_value)
- break;
- if (timeout_us && ktime_compare(ktime_get(), timeout) > 0) {
- ret = regmap_read(map, addr, &val);
- break;
- }
- if (sleep_us)
- usleep_range((sleep_us >> 2) + 1, sleep_us);
- }
- return ret ?: (((val & cond_mask) == cond_value) ? 0 : -ETIMEDOUT);
+ return regmap_read_poll_timeout(tc->regmap, addr, val,
+ (val & cond_mask) == cond_value,
+ sleep_us, timeout_us);
}
static int tc_aux_wait_busy(struct tc_data *tc, unsigned int timeout_ms)
{
- return tc_poll_timeout(tc->regmap, DP0_AUXSTATUS, AUX_BUSY, 0,
+ return tc_poll_timeout(tc, DP0_AUXSTATUS, AUX_BUSY, 0,
1000, 1000 * timeout_ms);
}
@@ -598,8 +585,7 @@ static int tc_aux_link_setup(struct tc_data *tc)
tc_write(DP1_PLLCTRL, PLLUPDATE | PLLEN);
tc_wait_pll_lock(tc);
- ret = tc_poll_timeout(tc->regmap, DP_PHY_CTRL, PHY_RDY, PHY_RDY, 1,
- 1000);
+ ret = tc_poll_timeout(tc, DP_PHY_CTRL, PHY_RDY, PHY_RDY, 1, 1000);
if (ret == -ETIMEDOUT) {
dev_err(tc->dev, "Timeout waiting for PHY to become ready");
return ret;
--
2.21.0
[PATCH v4 14/15] drm/bridge: tc358767: Drop unnecessary 8 byte buffer
tc_get_display_props() never reads more than a byte via AUX, so
there's no need to reserve 8 for that purpose. No function change
intended.
Signed-off-by: Andrey Smirnov
Cc: Andrzej Hajda
Cc: Laurent Pinchart
Cc: Tomi Valkeinen
Cc: Andrey Gusakov
Cc: Philipp Zabel
Cc: Cory Tusar
Cc: Chris Healy
Cc: Lucas Stach
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
drivers/gpu/drm/bridge/tc358767.c | 13 ++---
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c
b/drivers/gpu/drm/bridge/tc358767.c
index e747f10021e3..4a245144aa83 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -661,8 +661,7 @@ static int tc_aux_link_setup(struct tc_data *tc)
static int tc_get_display_props(struct tc_data *tc)
{
int ret;
- /* temp buffer */
- u8 tmp[8];
+ u8 reg;
/* Read DP Rx Link Capability */
ret = drm_dp_link_probe(&tc->aux, &tc->link.base);
@@ -678,21 +677,21 @@ static int tc_get_display_props(struct tc_data *tc)
tc->link.base.num_lanes = 2;
}
- ret = drm_dp_dpcd_readb(&tc->aux, DP_MAX_DOWNSPREAD, tmp);
+ ret = drm_dp_dpcd_readb(&tc->aux, DP_MAX_DOWNSPREAD, ®);
if (ret < 0)
goto err_dpcd_read;
- tc->link.spread = tmp[0] & DP_MAX_DOWNSPREAD_0_5;
+ tc->link.spread = reg & DP_MAX_DOWNSPREAD_0_5;
- ret = drm_dp_dpcd_readb(&tc->aux, DP_MAIN_LINK_CHANNEL_CODING, tmp);
+ ret = drm_dp_dpcd_readb(&tc->aux, DP_MAIN_LINK_CHANNEL_CODING, ®);
if (ret < 0)
goto err_dpcd_read;
tc->link.scrambler_dis = false;
/* read assr */
- ret = drm_dp_dpcd_readb(&tc->aux, DP_EDP_CONFIGURATION_SET, tmp);
+ ret = drm_dp_dpcd_readb(&tc->aux, DP_EDP_CONFIGURATION_SET, ®);
if (ret < 0)
goto err_dpcd_read;
- tc->link.assr = tmp[0] & DP_ALTERNATE_SCRAMBLER_RESET_ENABLE;
+ tc->link.assr = reg & DP_ALTERNATE_SCRAMBLER_RESET_ENABLE;
dev_dbg(tc->dev, "DPCD rev: %d.%d, rate: %s, lanes: %d, framing: %s\n",
tc->link.base.revision >> 4, tc->link.base.revision & 0x0f,
--
2.21.0
[PATCH v4 09/15] drm/bridge: tc358767: Use reported AUX transfer size
Don't assume that requested data transfer size is the same as amount
of data that was transferred. Change the code to get that information
from DP0_AUXSTATUS instead.
Since the check for AUX_BUSY in tc_aux_get_status() is pointless (it
will always called after tc_aux_wait_busy()) and there's only one user
of it, inline its code into tc_aux_transfer() instead of trying to
accommodate the change above.
Signed-off-by: Andrey Smirnov
Cc: Andrzej Hajda
Cc: Laurent Pinchart
Cc: Tomi Valkeinen
Cc: Andrey Gusakov
Cc: Philipp Zabel
Cc: Cory Tusar
Cc: Chris Healy
Cc: Lucas Stach
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
drivers/gpu/drm/bridge/tc358767.c | 40 ++-
1 file changed, 12 insertions(+), 28 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c
b/drivers/gpu/drm/bridge/tc358767.c
index 8b53dc8908d3..7d0fbb12195b 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -152,10 +152,10 @@
#define DP0_AUXWDATA(i)(0x066c + (i) * 4)
#define DP0_AUXRDATA(i)(0x067c + (i) * 4)
#define DP0_AUXSTATUS 0x068c
-#define AUX_STATUS_MASK0xf0
-#define AUX_STATUS_SHIFT 4
-#define AUX_TIMEOUTBIT(1)
-#define AUX_BUSY BIT(0)
+#define AUX_BYTES GENMASK(15, 8)
+#define AUX_STATUS GENMASK(7, 4)
+#define AUX_TIMEOUTBIT(1)
+#define AUX_BUSY BIT(0)
#define DP0_AUXI2CADR 0x0698
/* Link Training */
@@ -298,29 +298,6 @@ static int tc_aux_wait_busy(struct tc_data *tc, unsigned
int timeout_ms)
1000, 1000 * timeout_ms);
}
-static int tc_aux_get_status(struct tc_data *tc, u8 *reply)
-{
- int ret;
- u32 value;
-
- ret = regmap_read(tc->regmap, DP0_AUXSTATUS, &value);
- if (ret < 0)
- return ret;
-
- if (value & AUX_BUSY) {
- dev_err(tc->dev, "aux busy!\n");
- return -EBUSY;
- }
-
- if (value & AUX_TIMEOUT) {
- dev_err(tc->dev, "aux access timeout!\n");
- return -ETIMEDOUT;
- }
-
- *reply = (value & AUX_STATUS_MASK) >> AUX_STATUS_SHIFT;
- return 0;
-}
-
static int tc_aux_write_data(struct tc_data *tc, const void *data,
size_t size)
{
@@ -356,6 +333,7 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
struct tc_data *tc = aux_to_tc(aux);
size_t size = min_t(size_t, DP_AUX_MAX_PAYLOAD_BYTES - 1, msg->size);
u8 request = msg->request & ~DP_AUX_I2C_MOT;
+ u32 auxstatus;
int ret;
if (size == 0)
@@ -393,10 +371,16 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
if (ret)
return ret;
- ret = tc_aux_get_status(tc, &msg->reply);
+ ret = regmap_read(tc->regmap, DP0_AUXSTATUS, &auxstatus);
if (ret)
return ret;
+ if (auxstatus & AUX_TIMEOUT)
+ return -ETIMEDOUT;
+
+ size = FIELD_GET(AUX_BYTES, auxstatus);
+ msg->reply = FIELD_GET(AUX_STATUS, auxstatus);
+
switch (request) {
case DP_AUX_NATIVE_READ:
case DP_AUX_I2C_READ:
--
2.21.0
[PATCH v4 10/15] drm/bridge: tc358767: Add support for address-only I2C transfers
Transfer size of zero means a request to do an address-only
transfer. Since the HW support this, we probably shouldn't be just
ignoring such requests. While at it allow DP_AUX_I2C_MOT flag to pass
through, since it is supported by the HW as well.
Signed-off-by: Andrey Smirnov
Cc: Andrzej Hajda
Cc: Laurent Pinchart
Cc: Tomi Valkeinen
Cc: Andrey Gusakov
Cc: Philipp Zabel
Cc: Cory Tusar
Cc: Chris Healy
Cc: Lucas Stach
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
drivers/gpu/drm/bridge/tc358767.c | 30 +++---
1 file changed, 23 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c
b/drivers/gpu/drm/bridge/tc358767.c
index 7d0fbb12195b..4bb9b15e1324 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -145,6 +145,8 @@
/* AUX channel */
#define DP0_AUXCFG00x0660
+#define DP0_AUXCFG0_BSIZE GENMASK(11, 8)
+#define DP0_AUXCFG0_ADDR_ONLY BIT(4)
#define DP0_AUXCFG10x0664
#define AUX_RX_FILTER_EN BIT(16)
@@ -327,6 +329,18 @@ static int tc_aux_read_data(struct tc_data *tc, void
*data, size_t size)
return size;
}
+static u32 tc_auxcfg0(struct drm_dp_aux_msg *msg, size_t size)
+{
+ u32 auxcfg0 = msg->request;
+
+ if (size)
+ auxcfg0 |= FIELD_PREP(DP0_AUXCFG0_BSIZE, size - 1);
+ else
+ auxcfg0 |= DP0_AUXCFG0_ADDR_ONLY;
+
+ return auxcfg0;
+}
+
static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
struct drm_dp_aux_msg *msg)
{
@@ -336,9 +350,6 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
u32 auxstatus;
int ret;
- if (size == 0)
- return 0;
-
ret = tc_aux_wait_busy(tc, 100);
if (ret)
return ret;
@@ -362,8 +373,7 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
if (ret)
return ret;
/* Start transfer */
- ret = regmap_write(tc->regmap, DP0_AUXCFG0,
- ((size - 1) << 8) | request);
+ ret = regmap_write(tc->regmap, DP0_AUXCFG0, tc_auxcfg0(msg, size));
if (ret)
return ret;
@@ -377,8 +387,14 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
if (auxstatus & AUX_TIMEOUT)
return -ETIMEDOUT;
-
- size = FIELD_GET(AUX_BYTES, auxstatus);
+ /*
+* For some reason address-only DP_AUX_I2C_WRITE (MOT), still
+* reports 1 byte transferred in its status. To deal we that
+* we ignore aux_bytes field if we know that this was an
+* address-only transfer
+*/
+ if (size)
+ size = FIELD_GET(AUX_BYTES, auxstatus);
msg->reply = FIELD_GET(AUX_STATUS, auxstatus);
switch (request) {
--
2.21.0
[PATCH v4 13/15] drm/bridge: tc358767: Simplify tc_aux_wait_busy()
We never pass anything but 100 as timeout_ms to tc_aux_wait_busy(), so
we may as well hardcode that value and simplify function's signature.
Signed-off-by: Andrey Smirnov
Cc: Andrzej Hajda
Cc: Laurent Pinchart
Cc: Tomi Valkeinen
Cc: Andrey Gusakov
Cc: Philipp Zabel
Cc: Cory Tusar
Cc: Chris Healy
Cc: Lucas Stach
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
drivers/gpu/drm/bridge/tc358767.c | 9 -
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c
b/drivers/gpu/drm/bridge/tc358767.c
index c994c72eb330..e747f10021e3 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -294,10 +294,9 @@ static inline int tc_poll_timeout(struct tc_data *tc,
unsigned int addr,
sleep_us, timeout_us);
}
-static int tc_aux_wait_busy(struct tc_data *tc, unsigned int timeout_ms)
+static int tc_aux_wait_busy(struct tc_data *tc)
{
- return tc_poll_timeout(tc, DP0_AUXSTATUS, AUX_BUSY, 0,
- 1000, 1000 * timeout_ms);
+ return tc_poll_timeout(tc, DP0_AUXSTATUS, AUX_BUSY, 0, 1000, 10);
}
static int tc_aux_write_data(struct tc_data *tc, const void *data,
@@ -350,7 +349,7 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
u32 auxstatus;
int ret;
- ret = tc_aux_wait_busy(tc, 100);
+ ret = tc_aux_wait_busy(tc);
if (ret)
return ret;
@@ -377,7 +376,7 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
if (ret)
return ret;
- ret = tc_aux_wait_busy(tc, 100);
+ ret = tc_aux_wait_busy(tc);
if (ret)
return ret;
--
2.21.0
[PATCH v4 08/15] drm/bridge: tc358767: Increase AUX transfer length limit
According to the datasheet tc358767 can transfer up to 16 bytes via
its AUX channel, so the artificial limit of 8 appears to be too
low. However only up to 15-bytes seem to be actually supported and
trying to use 16-byte transfers results in transfers failing
sporadically (with bogus status in case of I2C transfers), so limit it
to 15.
Signed-off-by: Andrey Smirnov
Reviewed-by: Andrzej Hajda
Cc: Andrzej Hajda
Cc: Laurent Pinchart
Cc: Tomi Valkeinen
Cc: Andrey Gusakov
Cc: Philipp Zabel
Cc: Cory Tusar
Cc: Chris Healy
Cc: Lucas Stach
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
drivers/gpu/drm/bridge/tc358767.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c
b/drivers/gpu/drm/bridge/tc358767.c
index e60692b8cd69..8b53dc8908d3 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -354,7 +354,7 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
struct drm_dp_aux_msg *msg)
{
struct tc_data *tc = aux_to_tc(aux);
- size_t size = min_t(size_t, 8, msg->size);
+ size_t size = min_t(size_t, DP_AUX_MAX_PAYLOAD_BYTES - 1, msg->size);
u8 request = msg->request & ~DP_AUX_I2C_MOT;
int ret;
--
2.21.0
[PATCH v4 12/15] drm/bridge: tc358767: Introduce tc_pllupdate_pllen()
tc_wait_pll_lock() is always called as a follow-up for updating
PLLUPDATE and PLLEN bit of a given PLL control register. To simplify
things, merge the two operation into a single helper function
tc_pllupdate_pllen() and convert the rest of the code to use it. No
functional change intended.
Signed-off-by: Andrey Smirnov
Reviewed-by: Laurent Pinchart
Cc: Archit Taneja
Cc: Andrzej Hajda
Cc: Laurent Pinchart
Cc: Tomi Valkeinen
Cc: Andrey Gusakov
Cc: Philipp Zabel
Cc: Cory Tusar
Cc: Chris Healy
Cc: Lucas Stach
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
drivers/gpu/drm/bridge/tc358767.c | 30 ++
1 file changed, 14 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c
b/drivers/gpu/drm/bridge/tc358767.c
index ac55b59249e3..c994c72eb330 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -443,10 +443,18 @@ static u32 tc_srcctrl(struct tc_data *tc)
return reg;
}
-static void tc_wait_pll_lock(struct tc_data *tc)
+static int tc_pllupdate_pllen(struct tc_data *tc, unsigned int pllctrl)
{
+ int ret;
+
+ ret = regmap_write(tc->regmap, pllctrl, PLLUPDATE | PLLEN);
+ if (ret)
+ return ret;
+
/* Wait for PLL to lock: up to 2.09 ms, depending on refclk */
usleep_range(3000, 6000);
+
+ return 0;
}
static int tc_pxl_pll_en(struct tc_data *tc, u32 refclk, u32 pixelclock)
@@ -546,13 +554,7 @@ static int tc_pxl_pll_en(struct tc_data *tc, u32 refclk,
u32 pixelclock)
return ret;
/* Force PLL parameter update and disable bypass */
- ret = regmap_write(tc->regmap, PXL_PLLCTRL, PLLUPDATE | PLLEN);
- if (ret)
- return ret;
-
- tc_wait_pll_lock(tc);
-
- return 0;
+ return tc_pllupdate_pllen(tc, PXL_PLLCTRL);
}
static int tc_pxl_pll_dis(struct tc_data *tc)
@@ -626,15 +628,13 @@ static int tc_aux_link_setup(struct tc_data *tc)
* Initially PLLs are in bypass. Force PLL parameter update,
* disable PLL bypass, enable PLL
*/
- ret = regmap_write(tc->regmap, DP0_PLLCTRL, PLLUPDATE | PLLEN);
+ ret = tc_pllupdate_pllen(tc, DP0_PLLCTRL);
if (ret)
goto err;
- tc_wait_pll_lock(tc);
- ret = regmap_write(tc->regmap, DP1_PLLCTRL, PLLUPDATE | PLLEN);
+ ret = tc_pllupdate_pllen(tc, DP1_PLLCTRL);
if (ret)
goto err;
- tc_wait_pll_lock(tc);
ret = tc_poll_timeout(tc, DP_PHY_CTRL, PHY_RDY, PHY_RDY, 1, 1000);
if (ret == -ETIMEDOUT) {
@@ -914,15 +914,13 @@ static int tc_main_link_enable(struct tc_data *tc)
return ret;
/* PLL setup */
- ret = regmap_write(tc->regmap, DP0_PLLCTRL, PLLUPDATE | PLLEN);
+ ret = tc_pllupdate_pllen(tc, DP0_PLLCTRL);
if (ret)
return ret;
- tc_wait_pll_lock(tc);
- ret = regmap_write(tc->regmap, DP1_PLLCTRL, PLLUPDATE | PLLEN);
+ ret = tc_pllupdate_pllen(tc, DP1_PLLCTRL);
if (ret)
return ret;
- tc_wait_pll_lock(tc);
/* Reset/Enable Main Links */
dp_phy_ctrl |= DP_PHY_RST | PHY_M1_RST | PHY_M0_RST;
--
2.21.0
[PATCH v4 11/15] drm/bridge: tc358767: Introduce tc_set_syspllparam()
Move common code converting clock rate to an appropriate constant and
configuring SYS_PLLPARAM register into a separate routine and convert
the rest of the code to use it. No functional change intended.
Signed-off-by: Andrey Smirnov
Reviewed-by: Laurent Pinchart
Cc: Archit Taneja
Cc: Andrzej Hajda
Cc: Laurent Pinchart
Cc: Tomi Valkeinen
Cc: Andrey Gusakov
Cc: Philipp Zabel
Cc: Chris Healy
Cc: Cory Tusar
Cc: Lucas Stach
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
drivers/gpu/drm/bridge/tc358767.c | 46 +++
1 file changed, 16 insertions(+), 30 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c
b/drivers/gpu/drm/bridge/tc358767.c
index 4bb9b15e1324..ac55b59249e3 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -581,35 +581,40 @@ static int tc_stream_clock_calc(struct tc_data *tc)
return regmap_write(tc->regmap, DP0_VIDMNGEN1, 32768);
}
-static int tc_aux_link_setup(struct tc_data *tc)
+static int tc_set_syspllparam(struct tc_data *tc)
{
unsigned long rate;
- u32 dp0_auxcfg1;
- u32 value;
- int ret;
+ u32 pllparam = SYSCLK_SEL_LSCLK | LSCLK_DIV_2;
rate = clk_get_rate(tc->refclk);
switch (rate) {
case 3840:
- value = REF_FREQ_38M4;
+ pllparam |= REF_FREQ_38M4;
break;
case 2600:
- value = REF_FREQ_26M;
+ pllparam |= REF_FREQ_26M;
break;
case 1920:
- value = REF_FREQ_19M2;
+ pllparam |= REF_FREQ_19M2;
break;
case 1300:
- value = REF_FREQ_13M;
+ pllparam |= REF_FREQ_13M;
break;
default:
dev_err(tc->dev, "Invalid refclk rate: %lu Hz\n", rate);
return -EINVAL;
}
+ return regmap_write(tc->regmap, SYS_PLLPARAM, pllparam);
+}
+
+static int tc_aux_link_setup(struct tc_data *tc)
+{
+ int ret;
+ u32 dp0_auxcfg1;
+
/* Setup DP-PHY / PLL */
- value |= SYSCLK_SEL_LSCLK | LSCLK_DIV_2;
- ret = regmap_write(tc->regmap, SYS_PLLPARAM, value);
+ ret = tc_set_syspllparam(tc);
if (ret)
goto err;
@@ -868,7 +873,6 @@ static int tc_main_link_enable(struct tc_data *tc)
{
struct drm_dp_aux *aux = &tc->aux;
struct device *dev = tc->dev;
- unsigned int rate;
u32 dp_phy_ctrl;
u32 value;
int ret;
@@ -896,25 +900,7 @@ static int tc_main_link_enable(struct tc_data *tc)
if (ret)
return ret;
- rate = clk_get_rate(tc->refclk);
- switch (rate) {
- case 3840:
- value = REF_FREQ_38M4;
- break;
- case 2600:
- value = REF_FREQ_26M;
- break;
- case 1920:
- value = REF_FREQ_19M2;
- break;
- case 1300:
- value = REF_FREQ_13M;
- break;
- default:
- return -EINVAL;
- }
- value |= SYSCLK_SEL_LSCLK | LSCLK_DIV_2;
- ret = regmap_write(tc->regmap, SYS_PLLPARAM, value);
+ ret = tc_set_syspllparam(tc);
if (ret)
return ret;
--
2.21.0
[PATCH v4 07/15] drm/bridge: tc358767: Simplify AUX data write
Simplify AUX data write by dropping index arithmetic and shifting and
replacing it with a call to a helper function that does two things:
1. Copies user-provided data into a write buffer
2. Transfers contents of the write buffer to up to 4 32-bit
registers on the chip
Note that separate data endianness fix:
tmp = (tmp << 8) | buf[i];
that was reserved for DP_AUX_I2C_WRITE looks really strange, since it
will place data differently depending on the passed user-data
size. E.g. for a write of 1 byte, data transferred to the chip would
look like:
[byte0] [dummy1] [dummy2] [dummy3]
whereas for a write of 4 bytes we'd get:
[byte3] [byte2] [byte1] [byte0]
Since there's no indication in the datasheet that I2C write buffer
should be treated differently than AUX write buffer and no comment in
the original code explaining why it was done this way, that special
I2C write buffer transformation was dropped in this patch.
Signed-off-by: Andrey Smirnov
Reviewed-by: Andrzej Hajda
Cc: Andrzej Hajda
Cc: Laurent Pinchart
Cc: Tomi Valkeinen
Cc: Andrey Gusakov
Cc: Philipp Zabel
Cc: Cory Tusar
Cc: Chris Healy
Cc: Lucas Stach
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
drivers/gpu/drm/bridge/tc358767.c | 48 +--
1 file changed, 26 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c
b/drivers/gpu/drm/bridge/tc358767.c
index 7152b44db8a3..e60692b8cd69 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -321,6 +321,21 @@ static int tc_aux_get_status(struct tc_data *tc, u8 *reply)
return 0;
}
+static int tc_aux_write_data(struct tc_data *tc, const void *data,
+size_t size)
+{
+ u32 auxwdata[DP_AUX_MAX_PAYLOAD_BYTES / sizeof(u32)] = { 0 };
+ int ret, count = ALIGN(size, sizeof(u32));
+
+ memcpy(auxwdata, data, size);
+
+ ret = regmap_raw_write(tc->regmap, DP0_AUXWDATA(0), auxwdata, count);
+ if (ret)
+ return ret;
+
+ return size;
+}
+
static int tc_aux_read_data(struct tc_data *tc, void *data, size_t size)
{
u32 auxrdata[DP_AUX_MAX_PAYLOAD_BYTES / sizeof(u32)];
@@ -341,9 +356,6 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
struct tc_data *tc = aux_to_tc(aux);
size_t size = min_t(size_t, 8, msg->size);
u8 request = msg->request & ~DP_AUX_I2C_MOT;
- u8 *buf = msg->buffer;
- u32 tmp = 0;
- int i = 0;
int ret;
if (size == 0)
@@ -353,25 +365,17 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
if (ret)
return ret;
- if (request == DP_AUX_I2C_WRITE || request == DP_AUX_NATIVE_WRITE) {
- /* Store data */
- while (i < size) {
- if (request == DP_AUX_NATIVE_WRITE)
- tmp = tmp | (buf[i] << (8 * (i & 0x3)));
- else
- tmp = (tmp << 8) | buf[i];
- i++;
- if (((i % 4) == 0) || (i == size)) {
- ret = regmap_write(tc->regmap,
- DP0_AUXWDATA((i - 1) >> 2),
- tmp);
- if (ret)
- return ret;
- tmp = 0;
- }
- }
- } else if (request != DP_AUX_I2C_READ &&
- request != DP_AUX_NATIVE_READ) {
+ switch (request) {
+ case DP_AUX_NATIVE_READ:
+ case DP_AUX_I2C_READ:
+ break;
+ case DP_AUX_NATIVE_WRITE:
+ case DP_AUX_I2C_WRITE:
+ ret = tc_aux_write_data(tc, msg->buffer, size);
+ if (ret < 0)
+ return ret;
+ break;
+ default:
return -EINVAL;
}
--
2.21.0
[PATCH v4 05/15] drm/bridge: tc358767: Drop custom tc_write()/tc_read() accessors
A very unfortunate aspect of tc_write()/tc_read() macro helpers is
that they capture quite a bit of context around them and thus require
the caller to have magic variables 'ret' and 'tc' as well as label
'err'. That makes a number of code paths rather counter-intuitive and
somewhat clunky, for example tc_stream_clock_calc() ends up being like
this:
int ret;
tc_write(DP0_VIDMNGEN1, 32768);
return 0;
err:
return ret;
which is rather surprising when you read the code for the first
time. Since those helpers arguably aren't really saving that much code
and there's no way of fixing them without making them too verbose to
be worth it change the driver code to not use them at all.
Signed-off-by: Andrey Smirnov
Reviewed-by: Andrzej Hajda
Cc: Andrzej Hajda
Cc: Laurent Pinchart
Cc: Tomi Valkeinen
Cc: Andrey Gusakov
Cc: Philipp Zabel
Cc: Cory Tusar
Cc: Chris Healy
Cc: Lucas Stach
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
drivers/gpu/drm/bridge/tc358767.c | 382 ++
1 file changed, 230 insertions(+), 152 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c
b/drivers/gpu/drm/bridge/tc358767.c
index 5b78021d6c5b..7b15caec2ce5 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -280,20 +280,6 @@ static inline struct tc_data *connector_to_tc(struct
drm_connector *c)
return container_of(c, struct tc_data, connector);
}
-/* Simple macros to avoid repeated error checks */
-#define tc_write(reg, var) \
- do {\
- ret = regmap_write(tc->regmap, reg, var); \
- if (ret)\
- goto err; \
- } while (0)
-#define tc_read(reg, var) \
- do {\
- ret = regmap_read(tc->regmap, reg, var);\
- if (ret)\
- goto err; \
- } while (0)
-
static inline int tc_poll_timeout(struct tc_data *tc, unsigned int addr,
unsigned int cond_mask,
unsigned int cond_value,
@@ -351,7 +337,7 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
ret = tc_aux_wait_busy(tc, 100);
if (ret)
- goto err;
+ return ret;
if (request == DP_AUX_I2C_WRITE || request == DP_AUX_NATIVE_WRITE) {
/* Store data */
@@ -362,7 +348,11 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
tmp = (tmp << 8) | buf[i];
i++;
if (((i % 4) == 0) || (i == size)) {
- tc_write(DP0_AUXWDATA((i - 1) >> 2), tmp);
+ ret = regmap_write(tc->regmap,
+ DP0_AUXWDATA((i - 1) >> 2),
+ tmp);
+ if (ret)
+ return ret;
tmp = 0;
}
}
@@ -372,23 +362,32 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
}
/* Store address */
- tc_write(DP0_AUXADDR, msg->address);
+ ret = regmap_write(tc->regmap, DP0_AUXADDR, msg->address);
+ if (ret)
+ return ret;
/* Start transfer */
- tc_write(DP0_AUXCFG0, ((size - 1) << 8) | request);
+ ret = regmap_write(tc->regmap, DP0_AUXCFG0,
+ ((size - 1) << 8) | request);
+ if (ret)
+ return ret;
ret = tc_aux_wait_busy(tc, 100);
if (ret)
- goto err;
+ return ret;
ret = tc_aux_get_status(tc, &msg->reply);
if (ret)
- goto err;
+ return ret;
if (request == DP_AUX_I2C_READ || request == DP_AUX_NATIVE_READ) {
/* Read data */
while (i < size) {
- if ((i % 4) == 0)
- tc_read(DP0_AUXRDATA(i >> 2), &tmp);
+ if ((i % 4) == 0) {
+ ret = regmap_read(tc->regmap,
+ DP0_AUXRDATA(i >> 2), &tmp);
+ if (ret)
+ return ret;
+ }
buf[i] = tmp & 0xff;
tmp = tmp >> 8;
i++;
@@ -396,8 +395,6 @@ sta
[PATCH v4 06/15] drm/bridge: tc358767: Simplify AUX data read
Simplify AUX data read by removing index arithmetic and shifting with
a helper function that does two things:
1. Fetch data from up to 4 32-bit registers from the chip
2. Copy read data into user provided array.
Signed-off-by: Andrey Smirnov
Reviewed-by: Andrzej Hajda
Cc: Andrzej Hajda
Cc: Laurent Pinchart
Cc: Tomi Valkeinen
Cc: Andrey Gusakov
Cc: Philipp Zabel
Cc: Cory Tusar
Cc: Chris Healy
Cc: Lucas Stach
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
drivers/gpu/drm/bridge/tc358767.c | 31 ++-
1 file changed, 18 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c
b/drivers/gpu/drm/bridge/tc358767.c
index 7b15caec2ce5..7152b44db8a3 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -321,6 +321,20 @@ static int tc_aux_get_status(struct tc_data *tc, u8 *reply)
return 0;
}
+static int tc_aux_read_data(struct tc_data *tc, void *data, size_t size)
+{
+ u32 auxrdata[DP_AUX_MAX_PAYLOAD_BYTES / sizeof(u32)];
+ int ret, count = ALIGN(size, sizeof(u32));
+
+ ret = regmap_raw_read(tc->regmap, DP0_AUXRDATA(0), auxrdata, count);
+ if (ret)
+ return ret;
+
+ memcpy(data, auxrdata, size);
+
+ return size;
+}
+
static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
struct drm_dp_aux_msg *msg)
{
@@ -379,19 +393,10 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
if (ret)
return ret;
- if (request == DP_AUX_I2C_READ || request == DP_AUX_NATIVE_READ) {
- /* Read data */
- while (i < size) {
- if ((i % 4) == 0) {
- ret = regmap_read(tc->regmap,
- DP0_AUXRDATA(i >> 2), &tmp);
- if (ret)
- return ret;
- }
- buf[i] = tmp & 0xff;
- tmp = tmp >> 8;
- i++;
- }
+ switch (request) {
+ case DP_AUX_NATIVE_READ:
+ case DP_AUX_I2C_READ:
+ return tc_aux_read_data(tc, msg->buffer, size);
}
return size;
--
2.21.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v4 15/15] drm/bridge: tc358767: Replace magic number in tc_main_link_enable()
We don't need 8 byte array, DP_LINK_STATUS_SIZE (6) should be enough. This also gets rid of a magic number as a bonus. Signed-off-by: Andrey Smirnov Cc: Andrzej Hajda Cc: Laurent Pinchart Cc: Tomi Valkeinen Cc: Andrey Gusakov Cc: Philipp Zabel Cc: Cory Tusar Cc: Chris Healy Cc: Lucas Stach Cc: dri-devel@lists.freedesktop.org Cc: linux-ker...@vger.kernel.org --- drivers/gpu/drm/bridge/tc358767.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c index 4a245144aa83..05c5fab011f8 100644 --- a/drivers/gpu/drm/bridge/tc358767.c +++ b/drivers/gpu/drm/bridge/tc358767.c @@ -874,7 +874,7 @@ static int tc_main_link_enable(struct tc_data *tc) u32 dp_phy_ctrl; u32 value; int ret; - u8 tmp[8]; + u8 tmp[DP_LINK_STATUS_SIZE]; dev_dbg(tc->dev, "link enable\n"); -- 2.21.0 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 06/15] drm/bridge: tc358767: Simplify AUX data read
On Thu, Jun 6, 2019 at 3:59 AM Andrzej Hajda wrote:
>
> On 05.06.2019 09:04, Andrey Smirnov wrote:
> > Simplify AUX data read by removing index arithmetic and shifting with
> > a helper functions that does three things:
> >
> > 1. Fetch data from up to 4 32-bit registers from the chip
> > 2. Optionally fix data endianness (not needed on LE hosts)
> > 3. Copy read data into user provided array.
> >
> > Signed-off-by: Andrey Smirnov
> > Cc: Archit Taneja
> > Cc: Andrzej Hajda
> > Cc: Laurent Pinchart
> > Cc: Tomi Valkeinen
> > Cc: Andrey Gusakov
> > Cc: Philipp Zabel
> > Cc: Cory Tusar
> > Cc: Chris Healy
> > Cc: Lucas Stach
> > Cc: dri-devel@lists.freedesktop.org
> > Cc: linux-ker...@vger.kernel.org
> > ---
> > drivers/gpu/drm/bridge/tc358767.c | 40 +--
> > 1 file changed, 27 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/tc358767.c
> > b/drivers/gpu/drm/bridge/tc358767.c
> > index e197ce0fb166..da47d81e7109 100644
> > --- a/drivers/gpu/drm/bridge/tc358767.c
> > +++ b/drivers/gpu/drm/bridge/tc358767.c
> > @@ -321,6 +321,29 @@ static int tc_aux_get_status(struct tc_data *tc, u8
> > *reply)
> > return 0;
> > }
> >
> > +static int tc_aux_read_data(struct tc_data *tc, void *data, size_t size)
> > +{
> > + u32 auxrdata[DP_AUX_MAX_PAYLOAD_BYTES / sizeof(u32)];
> > + int ret, i, count = DIV_ROUND_UP(size, sizeof(u32));
> > +
> > + ret = regmap_bulk_read(tc->regmap, DP0_AUXRDATA(0), auxrdata, count);
> > + if (ret)
> > + return ret;
> > +
> > + for (i = 0; i < count; i++) {
> > + /*
> > + * Our regmap is configured as LE for register data,
> > + * so we need undo any byte swapping that might have
> > + * happened to preserve original byte order.
> > + */
> > + le32_to_cpus(&auxrdata[i]);
> > + }
> > +
> > + memcpy(data, auxrdata, size);
> > +
> > + return size;
> > +}
> > +
>
>
> Hmm, cannot we just use regmap_raw_read?
I'll give it a try in v4.
Thanks,
Andrey Smirnov
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v5 00/15] tc358767 driver improvements
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 [v4]: - tc_pllupdate_pllen() renamed to tc_pllupdate() - Collected Reviewed-bys from Andrzej for the rest of the series Changes since [v3]: - Collected Reviewed-bys from Andrzej - Dropped explicit check for -ETIMEDOUT in "drm/bridge: tc358767: Simplify polling in tc_main_link_setup()" for consistency - AUX transfer code converted to user regmap_raw_read(), regmap_raw_write() Changes since [v2]: - Patchset rebased on top of v4 of Tomi's series that recently went in (https://patchwork.freedesktop.org/series/58176/#rev5) - AUX transfer code converted to user regmap_bulk_read(), regmap_bulk_write() 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() [v4] lkml.kernel.org/r/20190607044550.13361-1-andrew.smir...@gmail.com [v3] lkml.kernel.org/r/20190605070507.11417-1-andrew.smir...@gmail.com [v2] lkml.kernel.org/r/20190322032901.12045-1-andrew.smir...@gmail.com [v1] lkml.kernel.org/r/20190226193609.9862-1-andrew.smir...@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() 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, 372 insertions(+), 276 deletions(-) -- 2.21.0
[PATCH v5 02/15] drm/bridge: tc358767: Simplify polling in tc_main_link_setup()
Replace explicit polling loop with equivalent call to
tc_poll_timeout() for brevity. No functional change intended.
Signed-off-by: Andrey Smirnov
Reviewed-by: Andrzej Hajda
Cc: Andrzej Hajda
Cc: Laurent Pinchart
Cc: Tomi Valkeinen
Cc: Andrey Gusakov
Cc: Philipp Zabel
Cc: Cory Tusar
Cc: Chris Healy
Cc: Lucas Stach
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
drivers/gpu/drm/bridge/tc358767.c | 12 +++-
1 file changed, 3 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c
b/drivers/gpu/drm/bridge/tc358767.c
index fb8a1942ec54..f463ef6d4271 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -774,7 +774,6 @@ static int tc_main_link_enable(struct tc_data *tc)
struct device *dev = tc->dev;
unsigned int rate;
u32 dp_phy_ctrl;
- int timeout;
u32 value;
int ret;
u8 tmp[8];
@@ -831,15 +830,10 @@ static int tc_main_link_enable(struct tc_data *tc)
dp_phy_ctrl &= ~(DP_PHY_RST | PHY_M1_RST | PHY_M0_RST);
tc_write(DP_PHY_CTRL, dp_phy_ctrl);
- timeout = 1000;
- do {
- tc_read(DP_PHY_CTRL, &value);
- udelay(1);
- } while ((!(value & PHY_RDY)) && (--timeout));
-
- if (timeout == 0) {
+ ret = tc_poll_timeout(tc, DP_PHY_CTRL, PHY_RDY, PHY_RDY, 1, 1000);
+ if (ret) {
dev_err(dev, "timeout waiting for phy become ready");
- return -ETIMEDOUT;
+ return ret;
}
/* Set misc: 8 bits per color */
--
2.21.0
[PATCH v5 10/15] drm/bridge: tc358767: Add support for address-only I2C transfers
Transfer size of zero means a request to do an address-only
transfer. Since the HW support this, we probably shouldn't be just
ignoring such requests. While at it allow DP_AUX_I2C_MOT flag to pass
through, since it is supported by the HW as well.
Signed-off-by: Andrey Smirnov
Reviewed-by: Andrzej Hajda
Cc: Andrzej Hajda
Cc: Laurent Pinchart
Cc: Tomi Valkeinen
Cc: Andrey Gusakov
Cc: Philipp Zabel
Cc: Cory Tusar
Cc: Chris Healy
Cc: Lucas Stach
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
drivers/gpu/drm/bridge/tc358767.c | 30 +++---
1 file changed, 23 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c
b/drivers/gpu/drm/bridge/tc358767.c
index 7d0fbb12195b..4bb9b15e1324 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -145,6 +145,8 @@
/* AUX channel */
#define DP0_AUXCFG00x0660
+#define DP0_AUXCFG0_BSIZE GENMASK(11, 8)
+#define DP0_AUXCFG0_ADDR_ONLY BIT(4)
#define DP0_AUXCFG10x0664
#define AUX_RX_FILTER_EN BIT(16)
@@ -327,6 +329,18 @@ static int tc_aux_read_data(struct tc_data *tc, void
*data, size_t size)
return size;
}
+static u32 tc_auxcfg0(struct drm_dp_aux_msg *msg, size_t size)
+{
+ u32 auxcfg0 = msg->request;
+
+ if (size)
+ auxcfg0 |= FIELD_PREP(DP0_AUXCFG0_BSIZE, size - 1);
+ else
+ auxcfg0 |= DP0_AUXCFG0_ADDR_ONLY;
+
+ return auxcfg0;
+}
+
static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
struct drm_dp_aux_msg *msg)
{
@@ -336,9 +350,6 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
u32 auxstatus;
int ret;
- if (size == 0)
- return 0;
-
ret = tc_aux_wait_busy(tc, 100);
if (ret)
return ret;
@@ -362,8 +373,7 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
if (ret)
return ret;
/* Start transfer */
- ret = regmap_write(tc->regmap, DP0_AUXCFG0,
- ((size - 1) << 8) | request);
+ ret = regmap_write(tc->regmap, DP0_AUXCFG0, tc_auxcfg0(msg, size));
if (ret)
return ret;
@@ -377,8 +387,14 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
if (auxstatus & AUX_TIMEOUT)
return -ETIMEDOUT;
-
- size = FIELD_GET(AUX_BYTES, auxstatus);
+ /*
+* For some reason address-only DP_AUX_I2C_WRITE (MOT), still
+* reports 1 byte transferred in its status. To deal we that
+* we ignore aux_bytes field if we know that this was an
+* address-only transfer
+*/
+ if (size)
+ size = FIELD_GET(AUX_BYTES, auxstatus);
msg->reply = FIELD_GET(AUX_STATUS, auxstatus);
switch (request) {
--
2.21.0
[PATCH v5 03/15] drm/bridge: tc358767: Simplify polling in tc_link_training()
Replace explicit polling in tc_link_training() with equivalent call to
tc_poll_timeout() for simplicity. No functional change intended (not
including slightly altered debug output).
Signed-off-by: Andrey Smirnov
Reviewed-by: Andrzej Hajda
Cc: Andrzej Hajda
Cc: Laurent Pinchart
Cc: Tomi Valkeinen
Cc: Andrey Gusakov
Cc: Philipp Zabel
Cc: Cory Tusar
Cc: Chris Healy
Cc: Lucas Stach
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
drivers/gpu/drm/bridge/tc358767.c | 15 ++-
1 file changed, 6 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c
b/drivers/gpu/drm/bridge/tc358767.c
index f463ef6d4271..31f5045e7e42 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -748,22 +748,19 @@ static int tc_set_video_mode(struct tc_data *tc,
static int tc_wait_link_training(struct tc_data *tc)
{
- u32 timeout = 1000;
u32 value;
int ret;
- do {
- udelay(1);
- tc_read(DP0_LTSTAT, &value);
- } while ((!(value & LT_LOOPDONE)) && (--timeout));
-
- if (timeout == 0) {
+ ret = tc_poll_timeout(tc, DP0_LTSTAT, LT_LOOPDONE,
+ LT_LOOPDONE, 1, 1000);
+ if (ret) {
dev_err(tc->dev, "Link training timeout waiting for
LT_LOOPDONE!\n");
- return -ETIMEDOUT;
+ return ret;
}
- return (value >> 8) & 0x7;
+ tc_read(DP0_LTSTAT, &value);
+ return (value >> 8) & 0x7;
err:
return ret;
}
--
2.21.0
[PATCH v5 04/15] drm/bridge: tc358767: Simplify tc_set_video_mode()
Simplify tc_set_video_mode() by replacing explicit shifting using macros from . No functional change intended. Signed-off-by: Andrey Smirnov Reviewed-by: Andrzej Hajda Cc: Andrzej Hajda Cc: Laurent Pinchart Cc: Tomi Valkeinen Cc: Andrey Gusakov Cc: Philipp Zabel Cc: Cory Tusar Cc: Chris Healy Cc: Lucas Stach Cc: dri-devel@lists.freedesktop.org Cc: linux-ker...@vger.kernel.org --- drivers/gpu/drm/bridge/tc358767.c | 106 ++ 1 file changed, 78 insertions(+), 28 deletions(-) diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c index 31f5045e7e42..5b78021d6c5b 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 #include #include #include @@ -56,6 +57,7 @@ /* Video Path */ #define VPCTRL00x0450 +#define VSDELAYGENMASK(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 HPWGENMASK(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 */ @@ -108,14 +118,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_TOTALGENMASK(15, 0) +#define V_TOTALGENMASK(31, 16) #define DP0_STARTVAL 0x064c +#define H_STARTGENMASK(15, 0) +#define V_STARTGENMASK(31, 16) #define DP0_ACTIVEVAL 0x0650 +#define H_ACT GENMASK(15, 0) +#define V_ACT GENMASK(31, 16) + #define DP0_SYNCVAL0x0654 +#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_SIZEGENMASK(21, 16) #define BPC_6 (0 << 5) #define BPC_8 (1 << 5) @@ -192,6 +216,12 @@ /* Test & Debug */ #define TSTCTL 0x0a00 +#define COLOR_RGENMASK(31, 24) +#define COLOR_GGENMASK(23, 16) +#define COLOR_BGENMASK(15, 8) +#define ENI2CFILTERBIT(4) +#define COLOR_BAR_MODE GENMASK(1, 0) +#define COLOR_BAR_MODE_BARS2 #define PLL_DBG0x0a04 static bool tc_test_pattern; @@ -672,6 +702,7 @@ static int tc_set_video_mode(struct tc_data *tc, int upper_margin = mode->vtotal - mode->vsync_end; int lower_margin = mode->vsync_start - mode->vdisplay; int vsync_len = mode->vsync_end - mode->vsync_start; + u32 dp0_syncval; /* * Recommended maximum number of symbols transferred in a transfer unit: @@ -696,50 +727,69 @@ static int tc_set_video_mode(struct tc_data *tc, * assume we do not need any delay when DPI is a source of * sync signals */ - tc_write(VPCTRL0, (0 << 20) /* VSDELAY */ | + tc_write(VPCTRL0, +FIELD_PREP(VSDELAY, 0) | 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 <<
[PATCH v5 12/15] drm/bridge: tc358767: Introduce tc_pllupdate()
tc_wait_pll_lock() is always called as a follow-up for updating
PLLUPDATE and PLLEN bit of a given PLL control register. To simplify
things, merge the two operation into a single helper function
tc_pllupdate() and convert the rest of the code to use it. No
functional change intended.
Signed-off-by: Andrey Smirnov
Reviewed-by: Laurent Pinchart
Reviewed-by: Andrzej Hajda
Cc: Andrzej Hajda
Cc: Laurent Pinchart
Cc: Tomi Valkeinen
Cc: Andrey Gusakov
Cc: Philipp Zabel
Cc: Cory Tusar
Cc: Chris Healy
Cc: Lucas Stach
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
drivers/gpu/drm/bridge/tc358767.c | 30 ++
1 file changed, 14 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c
b/drivers/gpu/drm/bridge/tc358767.c
index ac55b59249e3..28df53f7c349 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -443,10 +443,18 @@ static u32 tc_srcctrl(struct tc_data *tc)
return reg;
}
-static void tc_wait_pll_lock(struct tc_data *tc)
+static int tc_pllupdate(struct tc_data *tc, unsigned int pllctrl)
{
+ int ret;
+
+ ret = regmap_write(tc->regmap, pllctrl, PLLUPDATE | PLLEN);
+ if (ret)
+ return ret;
+
/* Wait for PLL to lock: up to 2.09 ms, depending on refclk */
usleep_range(3000, 6000);
+
+ return 0;
}
static int tc_pxl_pll_en(struct tc_data *tc, u32 refclk, u32 pixelclock)
@@ -546,13 +554,7 @@ static int tc_pxl_pll_en(struct tc_data *tc, u32 refclk,
u32 pixelclock)
return ret;
/* Force PLL parameter update and disable bypass */
- ret = regmap_write(tc->regmap, PXL_PLLCTRL, PLLUPDATE | PLLEN);
- if (ret)
- return ret;
-
- tc_wait_pll_lock(tc);
-
- return 0;
+ return tc_pllupdate(tc, PXL_PLLCTRL);
}
static int tc_pxl_pll_dis(struct tc_data *tc)
@@ -626,15 +628,13 @@ static int tc_aux_link_setup(struct tc_data *tc)
* Initially PLLs are in bypass. Force PLL parameter update,
* disable PLL bypass, enable PLL
*/
- ret = regmap_write(tc->regmap, DP0_PLLCTRL, PLLUPDATE | PLLEN);
+ ret = tc_pllupdate(tc, DP0_PLLCTRL);
if (ret)
goto err;
- tc_wait_pll_lock(tc);
- ret = regmap_write(tc->regmap, DP1_PLLCTRL, PLLUPDATE | PLLEN);
+ ret = tc_pllupdate(tc, DP1_PLLCTRL);
if (ret)
goto err;
- tc_wait_pll_lock(tc);
ret = tc_poll_timeout(tc, DP_PHY_CTRL, PHY_RDY, PHY_RDY, 1, 1000);
if (ret == -ETIMEDOUT) {
@@ -914,15 +914,13 @@ static int tc_main_link_enable(struct tc_data *tc)
return ret;
/* PLL setup */
- ret = regmap_write(tc->regmap, DP0_PLLCTRL, PLLUPDATE | PLLEN);
+ ret = tc_pllupdate(tc, DP0_PLLCTRL);
if (ret)
return ret;
- tc_wait_pll_lock(tc);
- ret = regmap_write(tc->regmap, DP1_PLLCTRL, PLLUPDATE | PLLEN);
+ ret = tc_pllupdate(tc, DP1_PLLCTRL);
if (ret)
return ret;
- tc_wait_pll_lock(tc);
/* Reset/Enable Main Links */
dp_phy_ctrl |= DP_PHY_RST | PHY_M1_RST | PHY_M0_RST;
--
2.21.0
[PATCH v5 08/15] drm/bridge: tc358767: Increase AUX transfer length limit
According to the datasheet tc358767 can transfer up to 16 bytes via
its AUX channel, so the artificial limit of 8 appears to be too
low. However only up to 15-bytes seem to be actually supported and
trying to use 16-byte transfers results in transfers failing
sporadically (with bogus status in case of I2C transfers), so limit it
to 15.
Signed-off-by: Andrey Smirnov
Reviewed-by: Andrzej Hajda
Cc: Andrzej Hajda
Cc: Laurent Pinchart
Cc: Tomi Valkeinen
Cc: Andrey Gusakov
Cc: Philipp Zabel
Cc: Cory Tusar
Cc: Chris Healy
Cc: Lucas Stach
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
drivers/gpu/drm/bridge/tc358767.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c
b/drivers/gpu/drm/bridge/tc358767.c
index e60692b8cd69..8b53dc8908d3 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -354,7 +354,7 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
struct drm_dp_aux_msg *msg)
{
struct tc_data *tc = aux_to_tc(aux);
- size_t size = min_t(size_t, 8, msg->size);
+ size_t size = min_t(size_t, DP_AUX_MAX_PAYLOAD_BYTES - 1, msg->size);
u8 request = msg->request & ~DP_AUX_I2C_MOT;
int ret;
--
2.21.0
[PATCH v5 05/15] drm/bridge: tc358767: Drop custom tc_write()/tc_read() accessors
A very unfortunate aspect of tc_write()/tc_read() macro helpers is
that they capture quite a bit of context around them and thus require
the caller to have magic variables 'ret' and 'tc' as well as label
'err'. That makes a number of code paths rather counter-intuitive and
somewhat clunky, for example tc_stream_clock_calc() ends up being like
this:
int ret;
tc_write(DP0_VIDMNGEN1, 32768);
return 0;
err:
return ret;
which is rather surprising when you read the code for the first
time. Since those helpers arguably aren't really saving that much code
and there's no way of fixing them without making them too verbose to
be worth it change the driver code to not use them at all.
Signed-off-by: Andrey Smirnov
Reviewed-by: Andrzej Hajda
Cc: Andrzej Hajda
Cc: Laurent Pinchart
Cc: Tomi Valkeinen
Cc: Andrey Gusakov
Cc: Philipp Zabel
Cc: Cory Tusar
Cc: Chris Healy
Cc: Lucas Stach
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
drivers/gpu/drm/bridge/tc358767.c | 382 ++
1 file changed, 230 insertions(+), 152 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c
b/drivers/gpu/drm/bridge/tc358767.c
index 5b78021d6c5b..7b15caec2ce5 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -280,20 +280,6 @@ static inline struct tc_data *connector_to_tc(struct
drm_connector *c)
return container_of(c, struct tc_data, connector);
}
-/* Simple macros to avoid repeated error checks */
-#define tc_write(reg, var) \
- do {\
- ret = regmap_write(tc->regmap, reg, var); \
- if (ret)\
- goto err; \
- } while (0)
-#define tc_read(reg, var) \
- do {\
- ret = regmap_read(tc->regmap, reg, var);\
- if (ret)\
- goto err; \
- } while (0)
-
static inline int tc_poll_timeout(struct tc_data *tc, unsigned int addr,
unsigned int cond_mask,
unsigned int cond_value,
@@ -351,7 +337,7 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
ret = tc_aux_wait_busy(tc, 100);
if (ret)
- goto err;
+ return ret;
if (request == DP_AUX_I2C_WRITE || request == DP_AUX_NATIVE_WRITE) {
/* Store data */
@@ -362,7 +348,11 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
tmp = (tmp << 8) | buf[i];
i++;
if (((i % 4) == 0) || (i == size)) {
- tc_write(DP0_AUXWDATA((i - 1) >> 2), tmp);
+ ret = regmap_write(tc->regmap,
+ DP0_AUXWDATA((i - 1) >> 2),
+ tmp);
+ if (ret)
+ return ret;
tmp = 0;
}
}
@@ -372,23 +362,32 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
}
/* Store address */
- tc_write(DP0_AUXADDR, msg->address);
+ ret = regmap_write(tc->regmap, DP0_AUXADDR, msg->address);
+ if (ret)
+ return ret;
/* Start transfer */
- tc_write(DP0_AUXCFG0, ((size - 1) << 8) | request);
+ ret = regmap_write(tc->regmap, DP0_AUXCFG0,
+ ((size - 1) << 8) | request);
+ if (ret)
+ return ret;
ret = tc_aux_wait_busy(tc, 100);
if (ret)
- goto err;
+ return ret;
ret = tc_aux_get_status(tc, &msg->reply);
if (ret)
- goto err;
+ return ret;
if (request == DP_AUX_I2C_READ || request == DP_AUX_NATIVE_READ) {
/* Read data */
while (i < size) {
- if ((i % 4) == 0)
- tc_read(DP0_AUXRDATA(i >> 2), &tmp);
+ if ((i % 4) == 0) {
+ ret = regmap_read(tc->regmap,
+ DP0_AUXRDATA(i >> 2), &tmp);
+ if (ret)
+ return ret;
+ }
buf[i] = tmp & 0xff;
tmp = tmp >> 8;
i++;
@@ -396,8 +395,6 @@ sta
[PATCH v5 09/15] drm/bridge: tc358767: Use reported AUX transfer size
Don't assume that requested data transfer size is the same as amount
of data that was transferred. Change the code to get that information
from DP0_AUXSTATUS instead.
Since the check for AUX_BUSY in tc_aux_get_status() is pointless (it
will always called after tc_aux_wait_busy()) and there's only one user
of it, inline its code into tc_aux_transfer() instead of trying to
accommodate the change above.
Signed-off-by: Andrey Smirnov
Reviewed-by: Andrzej Hajda
Cc: Andrzej Hajda
Cc: Laurent Pinchart
Cc: Tomi Valkeinen
Cc: Andrey Gusakov
Cc: Philipp Zabel
Cc: Cory Tusar
Cc: Chris Healy
Cc: Lucas Stach
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
drivers/gpu/drm/bridge/tc358767.c | 40 ++-
1 file changed, 12 insertions(+), 28 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c
b/drivers/gpu/drm/bridge/tc358767.c
index 8b53dc8908d3..7d0fbb12195b 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -152,10 +152,10 @@
#define DP0_AUXWDATA(i)(0x066c + (i) * 4)
#define DP0_AUXRDATA(i)(0x067c + (i) * 4)
#define DP0_AUXSTATUS 0x068c
-#define AUX_STATUS_MASK0xf0
-#define AUX_STATUS_SHIFT 4
-#define AUX_TIMEOUTBIT(1)
-#define AUX_BUSY BIT(0)
+#define AUX_BYTES GENMASK(15, 8)
+#define AUX_STATUS GENMASK(7, 4)
+#define AUX_TIMEOUTBIT(1)
+#define AUX_BUSY BIT(0)
#define DP0_AUXI2CADR 0x0698
/* Link Training */
@@ -298,29 +298,6 @@ static int tc_aux_wait_busy(struct tc_data *tc, unsigned
int timeout_ms)
1000, 1000 * timeout_ms);
}
-static int tc_aux_get_status(struct tc_data *tc, u8 *reply)
-{
- int ret;
- u32 value;
-
- ret = regmap_read(tc->regmap, DP0_AUXSTATUS, &value);
- if (ret < 0)
- return ret;
-
- if (value & AUX_BUSY) {
- dev_err(tc->dev, "aux busy!\n");
- return -EBUSY;
- }
-
- if (value & AUX_TIMEOUT) {
- dev_err(tc->dev, "aux access timeout!\n");
- return -ETIMEDOUT;
- }
-
- *reply = (value & AUX_STATUS_MASK) >> AUX_STATUS_SHIFT;
- return 0;
-}
-
static int tc_aux_write_data(struct tc_data *tc, const void *data,
size_t size)
{
@@ -356,6 +333,7 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
struct tc_data *tc = aux_to_tc(aux);
size_t size = min_t(size_t, DP_AUX_MAX_PAYLOAD_BYTES - 1, msg->size);
u8 request = msg->request & ~DP_AUX_I2C_MOT;
+ u32 auxstatus;
int ret;
if (size == 0)
@@ -393,10 +371,16 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
if (ret)
return ret;
- ret = tc_aux_get_status(tc, &msg->reply);
+ ret = regmap_read(tc->regmap, DP0_AUXSTATUS, &auxstatus);
if (ret)
return ret;
+ if (auxstatus & AUX_TIMEOUT)
+ return -ETIMEDOUT;
+
+ size = FIELD_GET(AUX_BYTES, auxstatus);
+ msg->reply = FIELD_GET(AUX_STATUS, auxstatus);
+
switch (request) {
case DP_AUX_NATIVE_READ:
case DP_AUX_I2C_READ:
--
2.21.0
[PATCH v5 07/15] drm/bridge: tc358767: Simplify AUX data write
Simplify AUX data write by dropping index arithmetic and shifting and
replacing it with a call to a helper function that does two things:
1. Copies user-provided data into a write buffer
2. Transfers contents of the write buffer to up to 4 32-bit
registers on the chip
Note that separate data endianness fix:
tmp = (tmp << 8) | buf[i];
that was reserved for DP_AUX_I2C_WRITE looks really strange, since it
will place data differently depending on the passed user-data
size. E.g. for a write of 1 byte, data transferred to the chip would
look like:
[byte0] [dummy1] [dummy2] [dummy3]
whereas for a write of 4 bytes we'd get:
[byte3] [byte2] [byte1] [byte0]
Since there's no indication in the datasheet that I2C write buffer
should be treated differently than AUX write buffer and no comment in
the original code explaining why it was done this way, that special
I2C write buffer transformation was dropped in this patch.
Signed-off-by: Andrey Smirnov
Reviewed-by: Andrzej Hajda
Cc: Andrzej Hajda
Cc: Laurent Pinchart
Cc: Tomi Valkeinen
Cc: Andrey Gusakov
Cc: Philipp Zabel
Cc: Cory Tusar
Cc: Chris Healy
Cc: Lucas Stach
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
drivers/gpu/drm/bridge/tc358767.c | 48 +--
1 file changed, 26 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c
b/drivers/gpu/drm/bridge/tc358767.c
index 7152b44db8a3..e60692b8cd69 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -321,6 +321,21 @@ static int tc_aux_get_status(struct tc_data *tc, u8 *reply)
return 0;
}
+static int tc_aux_write_data(struct tc_data *tc, const void *data,
+size_t size)
+{
+ u32 auxwdata[DP_AUX_MAX_PAYLOAD_BYTES / sizeof(u32)] = { 0 };
+ int ret, count = ALIGN(size, sizeof(u32));
+
+ memcpy(auxwdata, data, size);
+
+ ret = regmap_raw_write(tc->regmap, DP0_AUXWDATA(0), auxwdata, count);
+ if (ret)
+ return ret;
+
+ return size;
+}
+
static int tc_aux_read_data(struct tc_data *tc, void *data, size_t size)
{
u32 auxrdata[DP_AUX_MAX_PAYLOAD_BYTES / sizeof(u32)];
@@ -341,9 +356,6 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
struct tc_data *tc = aux_to_tc(aux);
size_t size = min_t(size_t, 8, msg->size);
u8 request = msg->request & ~DP_AUX_I2C_MOT;
- u8 *buf = msg->buffer;
- u32 tmp = 0;
- int i = 0;
int ret;
if (size == 0)
@@ -353,25 +365,17 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
if (ret)
return ret;
- if (request == DP_AUX_I2C_WRITE || request == DP_AUX_NATIVE_WRITE) {
- /* Store data */
- while (i < size) {
- if (request == DP_AUX_NATIVE_WRITE)
- tmp = tmp | (buf[i] << (8 * (i & 0x3)));
- else
- tmp = (tmp << 8) | buf[i];
- i++;
- if (((i % 4) == 0) || (i == size)) {
- ret = regmap_write(tc->regmap,
- DP0_AUXWDATA((i - 1) >> 2),
- tmp);
- if (ret)
- return ret;
- tmp = 0;
- }
- }
- } else if (request != DP_AUX_I2C_READ &&
- request != DP_AUX_NATIVE_READ) {
+ switch (request) {
+ case DP_AUX_NATIVE_READ:
+ case DP_AUX_I2C_READ:
+ break;
+ case DP_AUX_NATIVE_WRITE:
+ case DP_AUX_I2C_WRITE:
+ ret = tc_aux_write_data(tc, msg->buffer, size);
+ if (ret < 0)
+ return ret;
+ break;
+ default:
return -EINVAL;
}
--
2.21.0
[PATCH v5 14/15] drm/bridge: tc358767: Drop unnecessary 8 byte buffer
tc_get_display_props() never reads more than a byte via AUX, so
there's no need to reserve 8 for that purpose. No function change
intended.
Signed-off-by: Andrey Smirnov
Reviewed-by: Andrzej Hajda
Cc: Andrzej Hajda
Cc: Laurent Pinchart
Cc: Tomi Valkeinen
Cc: Andrey Gusakov
Cc: Philipp Zabel
Cc: Cory Tusar
Cc: Chris Healy
Cc: Lucas Stach
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
drivers/gpu/drm/bridge/tc358767.c | 13 ++---
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c
b/drivers/gpu/drm/bridge/tc358767.c
index 01ca92a6d9c8..81ed359487c7 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -661,8 +661,7 @@ static int tc_aux_link_setup(struct tc_data *tc)
static int tc_get_display_props(struct tc_data *tc)
{
int ret;
- /* temp buffer */
- u8 tmp[8];
+ u8 reg;
/* Read DP Rx Link Capability */
ret = drm_dp_link_probe(&tc->aux, &tc->link.base);
@@ -678,21 +677,21 @@ static int tc_get_display_props(struct tc_data *tc)
tc->link.base.num_lanes = 2;
}
- ret = drm_dp_dpcd_readb(&tc->aux, DP_MAX_DOWNSPREAD, tmp);
+ ret = drm_dp_dpcd_readb(&tc->aux, DP_MAX_DOWNSPREAD, ®);
if (ret < 0)
goto err_dpcd_read;
- tc->link.spread = tmp[0] & DP_MAX_DOWNSPREAD_0_5;
+ tc->link.spread = reg & DP_MAX_DOWNSPREAD_0_5;
- ret = drm_dp_dpcd_readb(&tc->aux, DP_MAIN_LINK_CHANNEL_CODING, tmp);
+ ret = drm_dp_dpcd_readb(&tc->aux, DP_MAIN_LINK_CHANNEL_CODING, ®);
if (ret < 0)
goto err_dpcd_read;
tc->link.scrambler_dis = false;
/* read assr */
- ret = drm_dp_dpcd_readb(&tc->aux, DP_EDP_CONFIGURATION_SET, tmp);
+ ret = drm_dp_dpcd_readb(&tc->aux, DP_EDP_CONFIGURATION_SET, ®);
if (ret < 0)
goto err_dpcd_read;
- tc->link.assr = tmp[0] & DP_ALTERNATE_SCRAMBLER_RESET_ENABLE;
+ tc->link.assr = reg & DP_ALTERNATE_SCRAMBLER_RESET_ENABLE;
dev_dbg(tc->dev, "DPCD rev: %d.%d, rate: %s, lanes: %d, framing: %s\n",
tc->link.base.revision >> 4, tc->link.base.revision & 0x0f,
--
2.21.0
[PATCH v5 13/15] drm/bridge: tc358767: Simplify tc_aux_wait_busy()
We never pass anything but 100 as timeout_ms to tc_aux_wait_busy(), so
we may as well hardcode that value and simplify function's signature.
Signed-off-by: Andrey Smirnov
Reviewed-by: Andrzej Hajda
Cc: Andrzej Hajda
Cc: Laurent Pinchart
Cc: Tomi Valkeinen
Cc: Andrey Gusakov
Cc: Philipp Zabel
Cc: Cory Tusar
Cc: Chris Healy
Cc: Lucas Stach
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
drivers/gpu/drm/bridge/tc358767.c | 9 -
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c
b/drivers/gpu/drm/bridge/tc358767.c
index 28df53f7c349..01ca92a6d9c8 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -294,10 +294,9 @@ static inline int tc_poll_timeout(struct tc_data *tc,
unsigned int addr,
sleep_us, timeout_us);
}
-static int tc_aux_wait_busy(struct tc_data *tc, unsigned int timeout_ms)
+static int tc_aux_wait_busy(struct tc_data *tc)
{
- return tc_poll_timeout(tc, DP0_AUXSTATUS, AUX_BUSY, 0,
- 1000, 1000 * timeout_ms);
+ return tc_poll_timeout(tc, DP0_AUXSTATUS, AUX_BUSY, 0, 1000, 10);
}
static int tc_aux_write_data(struct tc_data *tc, const void *data,
@@ -350,7 +349,7 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
u32 auxstatus;
int ret;
- ret = tc_aux_wait_busy(tc, 100);
+ ret = tc_aux_wait_busy(tc);
if (ret)
return ret;
@@ -377,7 +376,7 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
if (ret)
return ret;
- ret = tc_aux_wait_busy(tc, 100);
+ ret = tc_aux_wait_busy(tc);
if (ret)
return ret;
--
2.21.0
[PATCH v5 06/15] drm/bridge: tc358767: Simplify AUX data read
Simplify AUX data read by removing index arithmetic and shifting with
a helper function that does two things:
1. Fetch data from up to 4 32-bit registers from the chip
2. Copy read data into user provided array.
Signed-off-by: Andrey Smirnov
Reviewed-by: Andrzej Hajda
Cc: Andrzej Hajda
Cc: Laurent Pinchart
Cc: Tomi Valkeinen
Cc: Andrey Gusakov
Cc: Philipp Zabel
Cc: Cory Tusar
Cc: Chris Healy
Cc: Lucas Stach
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
drivers/gpu/drm/bridge/tc358767.c | 31 ++-
1 file changed, 18 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c
b/drivers/gpu/drm/bridge/tc358767.c
index 7b15caec2ce5..7152b44db8a3 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -321,6 +321,20 @@ static int tc_aux_get_status(struct tc_data *tc, u8 *reply)
return 0;
}
+static int tc_aux_read_data(struct tc_data *tc, void *data, size_t size)
+{
+ u32 auxrdata[DP_AUX_MAX_PAYLOAD_BYTES / sizeof(u32)];
+ int ret, count = ALIGN(size, sizeof(u32));
+
+ ret = regmap_raw_read(tc->regmap, DP0_AUXRDATA(0), auxrdata, count);
+ if (ret)
+ return ret;
+
+ memcpy(data, auxrdata, size);
+
+ return size;
+}
+
static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
struct drm_dp_aux_msg *msg)
{
@@ -379,19 +393,10 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
if (ret)
return ret;
- if (request == DP_AUX_I2C_READ || request == DP_AUX_NATIVE_READ) {
- /* Read data */
- while (i < size) {
- if ((i % 4) == 0) {
- ret = regmap_read(tc->regmap,
- DP0_AUXRDATA(i >> 2), &tmp);
- if (ret)
- return ret;
- }
- buf[i] = tmp & 0xff;
- tmp = tmp >> 8;
- i++;
- }
+ switch (request) {
+ case DP_AUX_NATIVE_READ:
+ case DP_AUX_I2C_READ:
+ return tc_aux_read_data(tc, msg->buffer, size);
}
return size;
--
2.21.0
[PATCH v5 15/15] drm/bridge: tc358767: Replace magic number in tc_main_link_enable()
We don't need 8 byte array, DP_LINK_STATUS_SIZE (6) should be enough. This also gets rid of a magic number as a bonus. Signed-off-by: Andrey Smirnov Reviewed-by: Andrzej Hajda Cc: Andrzej Hajda Cc: Laurent Pinchart Cc: Tomi Valkeinen Cc: Andrey Gusakov Cc: Philipp Zabel Cc: Cory Tusar Cc: Chris Healy Cc: Lucas Stach Cc: dri-devel@lists.freedesktop.org Cc: linux-ker...@vger.kernel.org --- drivers/gpu/drm/bridge/tc358767.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c index 81ed359487c7..b403c7bad156 100644 --- a/drivers/gpu/drm/bridge/tc358767.c +++ b/drivers/gpu/drm/bridge/tc358767.c @@ -874,7 +874,7 @@ static int tc_main_link_enable(struct tc_data *tc) u32 dp_phy_ctrl; u32 value; int ret; - u8 tmp[8]; + u8 tmp[DP_LINK_STATUS_SIZE]; dev_dbg(tc->dev, "link enable\n"); -- 2.21.0
[PATCH v5 11/15] drm/bridge: tc358767: Introduce tc_set_syspllparam()
Move common code converting clock rate to an appropriate constant and
configuring SYS_PLLPARAM register into a separate routine and convert
the rest of the code to use it. No functional change intended.
Signed-off-by: Andrey Smirnov
Reviewed-by: Laurent Pinchart
Reviewed-by: Andrzej Hajda
Cc: Andrzej Hajda
Cc: Laurent Pinchart
Cc: Tomi Valkeinen
Cc: Andrey Gusakov
Cc: Philipp Zabel
Cc: Chris Healy
Cc: Cory Tusar
Cc: Lucas Stach
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
drivers/gpu/drm/bridge/tc358767.c | 46 +++
1 file changed, 16 insertions(+), 30 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c
b/drivers/gpu/drm/bridge/tc358767.c
index 4bb9b15e1324..ac55b59249e3 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -581,35 +581,40 @@ static int tc_stream_clock_calc(struct tc_data *tc)
return regmap_write(tc->regmap, DP0_VIDMNGEN1, 32768);
}
-static int tc_aux_link_setup(struct tc_data *tc)
+static int tc_set_syspllparam(struct tc_data *tc)
{
unsigned long rate;
- u32 dp0_auxcfg1;
- u32 value;
- int ret;
+ u32 pllparam = SYSCLK_SEL_LSCLK | LSCLK_DIV_2;
rate = clk_get_rate(tc->refclk);
switch (rate) {
case 3840:
- value = REF_FREQ_38M4;
+ pllparam |= REF_FREQ_38M4;
break;
case 2600:
- value = REF_FREQ_26M;
+ pllparam |= REF_FREQ_26M;
break;
case 1920:
- value = REF_FREQ_19M2;
+ pllparam |= REF_FREQ_19M2;
break;
case 1300:
- value = REF_FREQ_13M;
+ pllparam |= REF_FREQ_13M;
break;
default:
dev_err(tc->dev, "Invalid refclk rate: %lu Hz\n", rate);
return -EINVAL;
}
+ return regmap_write(tc->regmap, SYS_PLLPARAM, pllparam);
+}
+
+static int tc_aux_link_setup(struct tc_data *tc)
+{
+ int ret;
+ u32 dp0_auxcfg1;
+
/* Setup DP-PHY / PLL */
- value |= SYSCLK_SEL_LSCLK | LSCLK_DIV_2;
- ret = regmap_write(tc->regmap, SYS_PLLPARAM, value);
+ ret = tc_set_syspllparam(tc);
if (ret)
goto err;
@@ -868,7 +873,6 @@ static int tc_main_link_enable(struct tc_data *tc)
{
struct drm_dp_aux *aux = &tc->aux;
struct device *dev = tc->dev;
- unsigned int rate;
u32 dp_phy_ctrl;
u32 value;
int ret;
@@ -896,25 +900,7 @@ static int tc_main_link_enable(struct tc_data *tc)
if (ret)
return ret;
- rate = clk_get_rate(tc->refclk);
- switch (rate) {
- case 3840:
- value = REF_FREQ_38M4;
- break;
- case 2600:
- value = REF_FREQ_26M;
- break;
- case 1920:
- value = REF_FREQ_19M2;
- break;
- case 1300:
- value = REF_FREQ_13M;
- break;
- default:
- return -EINVAL;
- }
- value |= SYSCLK_SEL_LSCLK | LSCLK_DIV_2;
- ret = regmap_write(tc->regmap, SYS_PLLPARAM, value);
+ ret = tc_set_syspllparam(tc);
if (ret)
return ret;
--
2.21.0
Re: [PATCH v5 10/15] drm/bridge: tc358767: Add support for address-only I2C transfers
On Wed, Jun 12, 2019 at 5:48 AM Tomi Valkeinen wrote: > > Hi, > > On 12/06/2019 11:32, Andrey Smirnov wrote: > > Transfer size of zero means a request to do an address-only > > transfer. Since the HW support this, we probably shouldn't be just > > ignoring such requests. While at it allow DP_AUX_I2C_MOT flag to pass > > through, since it is supported by the HW as well. > > I bisected the EDID read issue to this patch... > I don't think I've had any problems on my end with this. I'll double check. It might be the case that yours is the only setup where the problem can be repro'd, though. We can drop this patch if you don't have time/would rather not dig into this. Thanks, Andrey Smirnov
[PATCH v5 01/15] drm/bridge: tc358767: Simplify tc_poll_timeout()
Implementation of tc_poll_timeout() is almost a 100% copy-and-paste of
the code for regmap_read_poll_timeout(). Replace copied code with a
call to the original. While at it change tc_poll_timeout to accept
"struct tc_data *" instead of "struct regmap *" for brevity. No
functional change intended.
Signed-off-by: Andrey Smirnov
Reviewed-by: Andrzej Hajda
Reviewed-by: Laurent Pinchart
Cc: Andrzej Hajda
Cc: Laurent Pinchart
Cc: Tomi Valkeinen
Cc: Andrey Gusakov
Cc: Philipp Zabel
Cc: Cory Tusar
Cc: Chris Healy
Cc: Lucas Stach
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
drivers/gpu/drm/bridge/tc358767.c | 26 ++
1 file changed, 6 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c
b/drivers/gpu/drm/bridge/tc358767.c
index 58e3ca0e25af..fb8a1942ec54 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -264,34 +264,21 @@ static inline struct tc_data *connector_to_tc(struct
drm_connector *c)
goto err; \
} while (0)
-static inline int tc_poll_timeout(struct regmap *map, unsigned int addr,
+static inline int tc_poll_timeout(struct tc_data *tc, unsigned int addr,
unsigned int cond_mask,
unsigned int cond_value,
unsigned long sleep_us, u64 timeout_us)
{
- ktime_t timeout = ktime_add_us(ktime_get(), timeout_us);
unsigned int val;
- int ret;
- for (;;) {
- ret = regmap_read(map, addr, &val);
- if (ret)
- break;
- if ((val & cond_mask) == cond_value)
- break;
- if (timeout_us && ktime_compare(ktime_get(), timeout) > 0) {
- ret = regmap_read(map, addr, &val);
- break;
- }
- if (sleep_us)
- usleep_range((sleep_us >> 2) + 1, sleep_us);
- }
- return ret ?: (((val & cond_mask) == cond_value) ? 0 : -ETIMEDOUT);
+ return regmap_read_poll_timeout(tc->regmap, addr, val,
+ (val & cond_mask) == cond_value,
+ sleep_us, timeout_us);
}
static int tc_aux_wait_busy(struct tc_data *tc, unsigned int timeout_ms)
{
- return tc_poll_timeout(tc->regmap, DP0_AUXSTATUS, AUX_BUSY, 0,
+ return tc_poll_timeout(tc, DP0_AUXSTATUS, AUX_BUSY, 0,
1000, 1000 * timeout_ms);
}
@@ -598,8 +585,7 @@ static int tc_aux_link_setup(struct tc_data *tc)
tc_write(DP1_PLLCTRL, PLLUPDATE | PLLEN);
tc_wait_pll_lock(tc);
- ret = tc_poll_timeout(tc->regmap, DP_PHY_CTRL, PHY_RDY, PHY_RDY, 1,
- 1000);
+ ret = tc_poll_timeout(tc, DP_PHY_CTRL, PHY_RDY, PHY_RDY, 1, 1000);
if (ret == -ETIMEDOUT) {
dev_err(tc->dev, "Timeout waiting for PHY to become ready");
return ret;
--
2.21.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v5 10/15] drm/bridge: tc358767: Add support for address-only I2C transfers
On Wed, Jun 12, 2019 at 9:22 AM Andrey Smirnov wrote: > > On Wed, Jun 12, 2019 at 5:48 AM Tomi Valkeinen wrote: > > > > Hi, > > > > On 12/06/2019 11:32, Andrey Smirnov wrote: > > > Transfer size of zero means a request to do an address-only > > > transfer. Since the HW support this, we probably shouldn't be just > > > ignoring such requests. While at it allow DP_AUX_I2C_MOT flag to pass > > > through, since it is supported by the HW as well. > > > > I bisected the EDID read issue to this patch... > > > > I don't think I've had any problems on my end with this. I'll double > check. It might be the case that yours is the only setup where the > problem can be repro'd, though. We can drop this patch if you don't > have time/would rather not dig into this. > Turns out I do have this problem. Just didn't pay enough attention to notice. Will fix in v6. Thanks, Andrey Smirnov
[PATCH v6 00/15] tc358767 driver improvements
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 [v5]: - Fixed regression in "drm/bridge: tc358767: Add support for address-only I2C transfers" that broke EDID reading - Moved said patch to be the last in case it is still causing problems and needs to be dropped Changes since [v4]: - tc_pllupdate_pllen() renamed to tc_pllupdate() - Collected Reviewed-bys from Andrzej for the rest of the series Changes since [v3]: - Collected Reviewed-bys from Andrzej - Dropped explicit check for -ETIMEDOUT in "drm/bridge: tc358767: Simplify polling in tc_main_link_setup()" for consistency - AUX transfer code converted to user regmap_raw_read(), regmap_raw_write() Changes since [v2]: - Patchset rebased on top of v4 of Tomi's series that recently went in (https://patchwork.freedesktop.org/series/58176/#rev5) - AUX transfer code converted to user regmap_bulk_read(), regmap_bulk_write() 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() [v5] lkml.kernel.org/r/20190612083252.15321-1-andrew.smir...@gmail.com [v4] lkml.kernel.org/r/20190607044550.13361-1-andrew.smir...@gmail.com [v3] lkml.kernel.org/r/20190605070507.11417-1-andrew.smir...@gmail.com [v2] lkml.kernel.org/r/20190322032901.12045-1-andrew.smir...@gmail.com [v1] lkml.kernel.org/r/20190226193609.9862-1-andrew.smir...@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: Introduce tc_set_syspllparam() drm/bridge: tc358767: Introduce tc_pllupdate() 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() drm/bridge: tc358767: Add support for address-only I2C transfers drivers/gpu/drm/bridge/tc358767.c | 651 +- 1 file changed, 376 insertions(+), 275 deletions(-) -- 2.21.0
[PATCH v6 01/15] drm/bridge: tc358767: Simplify tc_poll_timeout()
Implementation of tc_poll_timeout() is almost a 100% copy-and-paste of
the code for regmap_read_poll_timeout(). Replace copied code with a
call to the original. While at it change tc_poll_timeout to accept
"struct tc_data *" instead of "struct regmap *" for brevity. No
functional change intended.
Signed-off-by: Andrey Smirnov
Reviewed-by: Andrzej Hajda
Reviewed-by: Laurent Pinchart
Cc: Andrzej Hajda
Cc: Laurent Pinchart
Cc: Tomi Valkeinen
Cc: Andrey Gusakov
Cc: Philipp Zabel
Cc: Cory Tusar
Cc: Chris Healy
Cc: Lucas Stach
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
drivers/gpu/drm/bridge/tc358767.c | 26 ++
1 file changed, 6 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c
b/drivers/gpu/drm/bridge/tc358767.c
index 58e3ca0e25af..fb8a1942ec54 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -264,34 +264,21 @@ static inline struct tc_data *connector_to_tc(struct
drm_connector *c)
goto err; \
} while (0)
-static inline int tc_poll_timeout(struct regmap *map, unsigned int addr,
+static inline int tc_poll_timeout(struct tc_data *tc, unsigned int addr,
unsigned int cond_mask,
unsigned int cond_value,
unsigned long sleep_us, u64 timeout_us)
{
- ktime_t timeout = ktime_add_us(ktime_get(), timeout_us);
unsigned int val;
- int ret;
- for (;;) {
- ret = regmap_read(map, addr, &val);
- if (ret)
- break;
- if ((val & cond_mask) == cond_value)
- break;
- if (timeout_us && ktime_compare(ktime_get(), timeout) > 0) {
- ret = regmap_read(map, addr, &val);
- break;
- }
- if (sleep_us)
- usleep_range((sleep_us >> 2) + 1, sleep_us);
- }
- return ret ?: (((val & cond_mask) == cond_value) ? 0 : -ETIMEDOUT);
+ return regmap_read_poll_timeout(tc->regmap, addr, val,
+ (val & cond_mask) == cond_value,
+ sleep_us, timeout_us);
}
static int tc_aux_wait_busy(struct tc_data *tc, unsigned int timeout_ms)
{
- return tc_poll_timeout(tc->regmap, DP0_AUXSTATUS, AUX_BUSY, 0,
+ return tc_poll_timeout(tc, DP0_AUXSTATUS, AUX_BUSY, 0,
1000, 1000 * timeout_ms);
}
@@ -598,8 +585,7 @@ static int tc_aux_link_setup(struct tc_data *tc)
tc_write(DP1_PLLCTRL, PLLUPDATE | PLLEN);
tc_wait_pll_lock(tc);
- ret = tc_poll_timeout(tc->regmap, DP_PHY_CTRL, PHY_RDY, PHY_RDY, 1,
- 1000);
+ ret = tc_poll_timeout(tc, DP_PHY_CTRL, PHY_RDY, PHY_RDY, 1, 1000);
if (ret == -ETIMEDOUT) {
dev_err(tc->dev, "Timeout waiting for PHY to become ready");
return ret;
--
2.21.0
[PATCH v6 04/15] drm/bridge: tc358767: Simplify tc_set_video_mode()
Simplify tc_set_video_mode() by replacing explicit shifting using macros from . No functional change intended. Signed-off-by: Andrey Smirnov Reviewed-by: Andrzej Hajda Cc: Andrzej Hajda Cc: Laurent Pinchart Cc: Tomi Valkeinen Cc: Andrey Gusakov Cc: Philipp Zabel Cc: Cory Tusar Cc: Chris Healy Cc: Lucas Stach Cc: dri-devel@lists.freedesktop.org Cc: linux-ker...@vger.kernel.org --- drivers/gpu/drm/bridge/tc358767.c | 106 ++ 1 file changed, 78 insertions(+), 28 deletions(-) diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c index 31f5045e7e42..5b78021d6c5b 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 #include #include #include @@ -56,6 +57,7 @@ /* Video Path */ #define VPCTRL00x0450 +#define VSDELAYGENMASK(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 HPWGENMASK(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 */ @@ -108,14 +118,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_TOTALGENMASK(15, 0) +#define V_TOTALGENMASK(31, 16) #define DP0_STARTVAL 0x064c +#define H_STARTGENMASK(15, 0) +#define V_STARTGENMASK(31, 16) #define DP0_ACTIVEVAL 0x0650 +#define H_ACT GENMASK(15, 0) +#define V_ACT GENMASK(31, 16) + #define DP0_SYNCVAL0x0654 +#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_SIZEGENMASK(21, 16) #define BPC_6 (0 << 5) #define BPC_8 (1 << 5) @@ -192,6 +216,12 @@ /* Test & Debug */ #define TSTCTL 0x0a00 +#define COLOR_RGENMASK(31, 24) +#define COLOR_GGENMASK(23, 16) +#define COLOR_BGENMASK(15, 8) +#define ENI2CFILTERBIT(4) +#define COLOR_BAR_MODE GENMASK(1, 0) +#define COLOR_BAR_MODE_BARS2 #define PLL_DBG0x0a04 static bool tc_test_pattern; @@ -672,6 +702,7 @@ static int tc_set_video_mode(struct tc_data *tc, int upper_margin = mode->vtotal - mode->vsync_end; int lower_margin = mode->vsync_start - mode->vdisplay; int vsync_len = mode->vsync_end - mode->vsync_start; + u32 dp0_syncval; /* * Recommended maximum number of symbols transferred in a transfer unit: @@ -696,50 +727,69 @@ static int tc_set_video_mode(struct tc_data *tc, * assume we do not need any delay when DPI is a source of * sync signals */ - tc_write(VPCTRL0, (0 << 20) /* VSDELAY */ | + tc_write(VPCTRL0, +FIELD_PREP(VSDELAY, 0) | 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 <<
[PATCH v6 03/15] drm/bridge: tc358767: Simplify polling in tc_link_training()
Replace explicit polling in tc_link_training() with equivalent call to
tc_poll_timeout() for simplicity. No functional change intended (not
including slightly altered debug output).
Signed-off-by: Andrey Smirnov
Reviewed-by: Andrzej Hajda
Cc: Andrzej Hajda
Cc: Laurent Pinchart
Cc: Tomi Valkeinen
Cc: Andrey Gusakov
Cc: Philipp Zabel
Cc: Cory Tusar
Cc: Chris Healy
Cc: Lucas Stach
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
drivers/gpu/drm/bridge/tc358767.c | 15 ++-
1 file changed, 6 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c
b/drivers/gpu/drm/bridge/tc358767.c
index f463ef6d4271..31f5045e7e42 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -748,22 +748,19 @@ static int tc_set_video_mode(struct tc_data *tc,
static int tc_wait_link_training(struct tc_data *tc)
{
- u32 timeout = 1000;
u32 value;
int ret;
- do {
- udelay(1);
- tc_read(DP0_LTSTAT, &value);
- } while ((!(value & LT_LOOPDONE)) && (--timeout));
-
- if (timeout == 0) {
+ ret = tc_poll_timeout(tc, DP0_LTSTAT, LT_LOOPDONE,
+ LT_LOOPDONE, 1, 1000);
+ if (ret) {
dev_err(tc->dev, "Link training timeout waiting for
LT_LOOPDONE!\n");
- return -ETIMEDOUT;
+ return ret;
}
- return (value >> 8) & 0x7;
+ tc_read(DP0_LTSTAT, &value);
+ return (value >> 8) & 0x7;
err:
return ret;
}
--
2.21.0
[PATCH v6 02/15] drm/bridge: tc358767: Simplify polling in tc_main_link_setup()
Replace explicit polling loop with equivalent call to
tc_poll_timeout() for brevity. No functional change intended.
Signed-off-by: Andrey Smirnov
Reviewed-by: Andrzej Hajda
Cc: Andrzej Hajda
Cc: Laurent Pinchart
Cc: Tomi Valkeinen
Cc: Andrey Gusakov
Cc: Philipp Zabel
Cc: Cory Tusar
Cc: Chris Healy
Cc: Lucas Stach
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
drivers/gpu/drm/bridge/tc358767.c | 12 +++-
1 file changed, 3 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c
b/drivers/gpu/drm/bridge/tc358767.c
index fb8a1942ec54..f463ef6d4271 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -774,7 +774,6 @@ static int tc_main_link_enable(struct tc_data *tc)
struct device *dev = tc->dev;
unsigned int rate;
u32 dp_phy_ctrl;
- int timeout;
u32 value;
int ret;
u8 tmp[8];
@@ -831,15 +830,10 @@ static int tc_main_link_enable(struct tc_data *tc)
dp_phy_ctrl &= ~(DP_PHY_RST | PHY_M1_RST | PHY_M0_RST);
tc_write(DP_PHY_CTRL, dp_phy_ctrl);
- timeout = 1000;
- do {
- tc_read(DP_PHY_CTRL, &value);
- udelay(1);
- } while ((!(value & PHY_RDY)) && (--timeout));
-
- if (timeout == 0) {
+ ret = tc_poll_timeout(tc, DP_PHY_CTRL, PHY_RDY, PHY_RDY, 1, 1000);
+ if (ret) {
dev_err(dev, "timeout waiting for phy become ready");
- return -ETIMEDOUT;
+ return ret;
}
/* Set misc: 8 bits per color */
--
2.21.0
[PATCH v6 15/15] drm/bridge: tc358767: Add support for address-only I2C transfers
Transfer size of zero means a request to do an address-only
transfer. Since the HW support this, we probably shouldn't be just
ignoring such requests. While at it allow DP_AUX_I2C_MOT flag to pass
through, since it is supported by the HW as well.
Signed-off-by: Andrey Smirnov
Reviewed-by: Andrzej Hajda
Cc: Andrzej Hajda
Cc: Laurent Pinchart
Cc: Tomi Valkeinen
Cc: Andrey Gusakov
Cc: Philipp Zabel
Cc: Cory Tusar
Cc: Chris Healy
Cc: Lucas Stach
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
drivers/gpu/drm/bridge/tc358767.c | 42 +++
1 file changed, 31 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c
b/drivers/gpu/drm/bridge/tc358767.c
index 3f8a15390813..7b8e19d6cf29 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -145,6 +145,8 @@
/* AUX channel */
#define DP0_AUXCFG00x0660
+#define DP0_AUXCFG0_BSIZE GENMASK(11, 8)
+#define DP0_AUXCFG0_ADDR_ONLY BIT(4)
#define DP0_AUXCFG10x0664
#define AUX_RX_FILTER_EN BIT(16)
@@ -326,6 +328,18 @@ static int tc_aux_read_data(struct tc_data *tc, void
*data, size_t size)
return size;
}
+static u32 tc_auxcfg0(struct drm_dp_aux_msg *msg, size_t size)
+{
+ u32 auxcfg0 = msg->request;
+
+ if (size)
+ auxcfg0 |= FIELD_PREP(DP0_AUXCFG0_BSIZE, size - 1);
+ else
+ auxcfg0 |= DP0_AUXCFG0_ADDR_ONLY;
+
+ return auxcfg0;
+}
+
static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
struct drm_dp_aux_msg *msg)
{
@@ -335,9 +349,6 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
u32 auxstatus;
int ret;
- if (size == 0)
- return 0;
-
ret = tc_aux_wait_busy(tc);
if (ret)
return ret;
@@ -348,9 +359,11 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
break;
case DP_AUX_NATIVE_WRITE:
case DP_AUX_I2C_WRITE:
- ret = tc_aux_write_data(tc, msg->buffer, size);
- if (ret < 0)
- return ret;
+ if (size) {
+ ret = tc_aux_write_data(tc, msg->buffer, size);
+ if (ret < 0)
+ return ret;
+ }
break;
default:
return -EINVAL;
@@ -361,8 +374,7 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
if (ret)
return ret;
/* Start transfer */
- ret = regmap_write(tc->regmap, DP0_AUXCFG0,
- ((size - 1) << 8) | request);
+ ret = regmap_write(tc->regmap, DP0_AUXCFG0, tc_auxcfg0(msg, size));
if (ret)
return ret;
@@ -376,14 +388,22 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
if (auxstatus & AUX_TIMEOUT)
return -ETIMEDOUT;
-
- size = FIELD_GET(AUX_BYTES, auxstatus);
+ /*
+* For some reason address-only DP_AUX_I2C_WRITE (MOT), still
+* reports 1 byte transferred in its status. To deal we that
+* we ignore aux_bytes field if we know that this was an
+* address-only transfer
+*/
+ if (size)
+ size = FIELD_GET(AUX_BYTES, auxstatus);
msg->reply = FIELD_GET(AUX_STATUS, auxstatus);
switch (request) {
case DP_AUX_NATIVE_READ:
case DP_AUX_I2C_READ:
- return tc_aux_read_data(tc, msg->buffer, size);
+ if (size)
+ return tc_aux_read_data(tc, msg->buffer, size);
+ break;
}
return size;
--
2.21.0
[PATCH v6 06/15] drm/bridge: tc358767: Simplify AUX data read
Simplify AUX data read by removing index arithmetic and shifting with
a helper function that does two things:
1. Fetch data from up to 4 32-bit registers from the chip
2. Copy read data into user provided array.
Signed-off-by: Andrey Smirnov
Reviewed-by: Andrzej Hajda
Cc: Andrzej Hajda
Cc: Laurent Pinchart
Cc: Tomi Valkeinen
Cc: Andrey Gusakov
Cc: Philipp Zabel
Cc: Cory Tusar
Cc: Chris Healy
Cc: Lucas Stach
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
drivers/gpu/drm/bridge/tc358767.c | 31 ++-
1 file changed, 18 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c
b/drivers/gpu/drm/bridge/tc358767.c
index 6a3e7c7e1189..02f6d907f5c4 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -321,6 +321,20 @@ static int tc_aux_get_status(struct tc_data *tc, u8 *reply)
return 0;
}
+static int tc_aux_read_data(struct tc_data *tc, void *data, size_t size)
+{
+ u32 auxrdata[DP_AUX_MAX_PAYLOAD_BYTES / sizeof(u32)];
+ int ret, count = ALIGN(size, sizeof(u32));
+
+ ret = regmap_raw_read(tc->regmap, DP0_AUXRDATA(0), auxrdata, count);
+ if (ret)
+ return ret;
+
+ memcpy(data, auxrdata, size);
+
+ return size;
+}
+
static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
struct drm_dp_aux_msg *msg)
{
@@ -379,19 +393,10 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
if (ret)
return ret;
- if (request == DP_AUX_I2C_READ || request == DP_AUX_NATIVE_READ) {
- /* Read data */
- while (i < size) {
- if ((i % 4) == 0) {
- ret = regmap_read(tc->regmap,
- DP0_AUXRDATA(i >> 2), &tmp);
- if (ret)
- return ret;
- }
- buf[i] = tmp & 0xff;
- tmp = tmp >> 8;
- i++;
- }
+ switch (request) {
+ case DP_AUX_NATIVE_READ:
+ case DP_AUX_I2C_READ:
+ return tc_aux_read_data(tc, msg->buffer, size);
}
return size;
--
2.21.0
[PATCH v6 08/15] drm/bridge: tc358767: Increase AUX transfer length limit
According to the datasheet tc358767 can transfer up to 16 bytes via
its AUX channel, so the artificial limit of 8 appears to be too
low. However only up to 15-bytes seem to be actually supported and
trying to use 16-byte transfers results in transfers failing
sporadically (with bogus status in case of I2C transfers), so limit it
to 15.
Signed-off-by: Andrey Smirnov
Reviewed-by: Andrzej Hajda
Cc: Andrzej Hajda
Cc: Laurent Pinchart
Cc: Tomi Valkeinen
Cc: Andrey Gusakov
Cc: Philipp Zabel
Cc: Cory Tusar
Cc: Chris Healy
Cc: Lucas Stach
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
drivers/gpu/drm/bridge/tc358767.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c
b/drivers/gpu/drm/bridge/tc358767.c
index a441e8e66287..bdbf88057946 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -354,7 +354,7 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
struct drm_dp_aux_msg *msg)
{
struct tc_data *tc = aux_to_tc(aux);
- size_t size = min_t(size_t, 8, msg->size);
+ size_t size = min_t(size_t, DP_AUX_MAX_PAYLOAD_BYTES - 1, msg->size);
u8 request = msg->request & ~DP_AUX_I2C_MOT;
int ret;
--
2.21.0
[PATCH v6 11/15] drm/bridge: tc358767: Introduce tc_pllupdate()
tc_wait_pll_lock() is always called as a follow-up for updating
PLLUPDATE and PLLEN bit of a given PLL control register. To simplify
things, merge the two operation into a single helper function
tc_pllupdate() and convert the rest of the code to use it. No
functional change intended.
Signed-off-by: Andrey Smirnov
Reviewed-by: Laurent Pinchart
Reviewed-by: Andrzej Hajda
Cc: Andrzej Hajda
Cc: Laurent Pinchart
Cc: Tomi Valkeinen
Cc: Andrey Gusakov
Cc: Philipp Zabel
Cc: Cory Tusar
Cc: Chris Healy
Cc: Lucas Stach
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
drivers/gpu/drm/bridge/tc358767.c | 30 ++
1 file changed, 14 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c
b/drivers/gpu/drm/bridge/tc358767.c
index 7a3a1b2d5c56..fe672f6bba73 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -427,10 +427,18 @@ static u32 tc_srcctrl(struct tc_data *tc)
return reg;
}
-static void tc_wait_pll_lock(struct tc_data *tc)
+static int tc_pllupdate(struct tc_data *tc, unsigned int pllctrl)
{
+ int ret;
+
+ ret = regmap_write(tc->regmap, pllctrl, PLLUPDATE | PLLEN);
+ if (ret)
+ return ret;
+
/* Wait for PLL to lock: up to 2.09 ms, depending on refclk */
usleep_range(3000, 6000);
+
+ return 0;
}
static int tc_pxl_pll_en(struct tc_data *tc, u32 refclk, u32 pixelclock)
@@ -530,13 +538,7 @@ static int tc_pxl_pll_en(struct tc_data *tc, u32 refclk,
u32 pixelclock)
return ret;
/* Force PLL parameter update and disable bypass */
- ret = regmap_write(tc->regmap, PXL_PLLCTRL, PLLUPDATE | PLLEN);
- if (ret)
- return ret;
-
- tc_wait_pll_lock(tc);
-
- return 0;
+ return tc_pllupdate(tc, PXL_PLLCTRL);
}
static int tc_pxl_pll_dis(struct tc_data *tc)
@@ -610,15 +612,13 @@ static int tc_aux_link_setup(struct tc_data *tc)
* Initially PLLs are in bypass. Force PLL parameter update,
* disable PLL bypass, enable PLL
*/
- ret = regmap_write(tc->regmap, DP0_PLLCTRL, PLLUPDATE | PLLEN);
+ ret = tc_pllupdate(tc, DP0_PLLCTRL);
if (ret)
goto err;
- tc_wait_pll_lock(tc);
- ret = regmap_write(tc->regmap, DP1_PLLCTRL, PLLUPDATE | PLLEN);
+ ret = tc_pllupdate(tc, DP1_PLLCTRL);
if (ret)
goto err;
- tc_wait_pll_lock(tc);
ret = tc_poll_timeout(tc, DP_PHY_CTRL, PHY_RDY, PHY_RDY, 1, 1000);
if (ret == -ETIMEDOUT) {
@@ -898,15 +898,13 @@ static int tc_main_link_enable(struct tc_data *tc)
return ret;
/* PLL setup */
- ret = regmap_write(tc->regmap, DP0_PLLCTRL, PLLUPDATE | PLLEN);
+ ret = tc_pllupdate(tc, DP0_PLLCTRL);
if (ret)
return ret;
- tc_wait_pll_lock(tc);
- ret = regmap_write(tc->regmap, DP1_PLLCTRL, PLLUPDATE | PLLEN);
+ ret = tc_pllupdate(tc, DP1_PLLCTRL);
if (ret)
return ret;
- tc_wait_pll_lock(tc);
/* Reset/Enable Main Links */
dp_phy_ctrl |= DP_PHY_RST | PHY_M1_RST | PHY_M0_RST;
--
2.21.0
[PATCH v6 14/15] drm/bridge: tc358767: Replace magic number in tc_main_link_enable()
We don't need 8 byte array, DP_LINK_STATUS_SIZE (6) should be enough. This also gets rid of a magic number as a bonus. Signed-off-by: Andrey Smirnov Reviewed-by: Andrzej Hajda Cc: Andrzej Hajda Cc: Laurent Pinchart Cc: Tomi Valkeinen Cc: Andrey Gusakov Cc: Philipp Zabel Cc: Cory Tusar Cc: Chris Healy Cc: Lucas Stach Cc: dri-devel@lists.freedesktop.org Cc: linux-ker...@vger.kernel.org --- drivers/gpu/drm/bridge/tc358767.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c index f0baf6d7ca80..3f8a15390813 100644 --- a/drivers/gpu/drm/bridge/tc358767.c +++ b/drivers/gpu/drm/bridge/tc358767.c @@ -858,7 +858,7 @@ static int tc_main_link_enable(struct tc_data *tc) u32 dp_phy_ctrl; u32 value; int ret; - u8 tmp[8]; + u8 tmp[DP_LINK_STATUS_SIZE]; dev_dbg(tc->dev, "link enable\n"); -- 2.21.0
[PATCH v6 10/15] drm/bridge: tc358767: Introduce tc_set_syspllparam()
Move common code converting clock rate to an appropriate constant and
configuring SYS_PLLPARAM register into a separate routine and convert
the rest of the code to use it. No functional change intended.
Signed-off-by: Andrey Smirnov
Reviewed-by: Laurent Pinchart
Reviewed-by: Andrzej Hajda
Cc: Andrzej Hajda
Cc: Laurent Pinchart
Cc: Tomi Valkeinen
Cc: Andrey Gusakov
Cc: Philipp Zabel
Cc: Chris Healy
Cc: Cory Tusar
Cc: Lucas Stach
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
drivers/gpu/drm/bridge/tc358767.c | 46 +++
1 file changed, 16 insertions(+), 30 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c
b/drivers/gpu/drm/bridge/tc358767.c
index b01c1c8341e1..7a3a1b2d5c56 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -565,35 +565,40 @@ static int tc_stream_clock_calc(struct tc_data *tc)
return regmap_write(tc->regmap, DP0_VIDMNGEN1, 32768);
}
-static int tc_aux_link_setup(struct tc_data *tc)
+static int tc_set_syspllparam(struct tc_data *tc)
{
unsigned long rate;
- u32 dp0_auxcfg1;
- u32 value;
- int ret;
+ u32 pllparam = SYSCLK_SEL_LSCLK | LSCLK_DIV_2;
rate = clk_get_rate(tc->refclk);
switch (rate) {
case 3840:
- value = REF_FREQ_38M4;
+ pllparam |= REF_FREQ_38M4;
break;
case 2600:
- value = REF_FREQ_26M;
+ pllparam |= REF_FREQ_26M;
break;
case 1920:
- value = REF_FREQ_19M2;
+ pllparam |= REF_FREQ_19M2;
break;
case 1300:
- value = REF_FREQ_13M;
+ pllparam |= REF_FREQ_13M;
break;
default:
dev_err(tc->dev, "Invalid refclk rate: %lu Hz\n", rate);
return -EINVAL;
}
+ return regmap_write(tc->regmap, SYS_PLLPARAM, pllparam);
+}
+
+static int tc_aux_link_setup(struct tc_data *tc)
+{
+ int ret;
+ u32 dp0_auxcfg1;
+
/* Setup DP-PHY / PLL */
- value |= SYSCLK_SEL_LSCLK | LSCLK_DIV_2;
- ret = regmap_write(tc->regmap, SYS_PLLPARAM, value);
+ ret = tc_set_syspllparam(tc);
if (ret)
goto err;
@@ -852,7 +857,6 @@ static int tc_main_link_enable(struct tc_data *tc)
{
struct drm_dp_aux *aux = &tc->aux;
struct device *dev = tc->dev;
- unsigned int rate;
u32 dp_phy_ctrl;
u32 value;
int ret;
@@ -880,25 +884,7 @@ static int tc_main_link_enable(struct tc_data *tc)
if (ret)
return ret;
- rate = clk_get_rate(tc->refclk);
- switch (rate) {
- case 3840:
- value = REF_FREQ_38M4;
- break;
- case 2600:
- value = REF_FREQ_26M;
- break;
- case 1920:
- value = REF_FREQ_19M2;
- break;
- case 1300:
- value = REF_FREQ_13M;
- break;
- default:
- return -EINVAL;
- }
- value |= SYSCLK_SEL_LSCLK | LSCLK_DIV_2;
- ret = regmap_write(tc->regmap, SYS_PLLPARAM, value);
+ ret = tc_set_syspllparam(tc);
if (ret)
return ret;
--
2.21.0
[PATCH v6 05/15] drm/bridge: tc358767: Drop custom tc_write()/tc_read() accessors
A very unfortunate aspect of tc_write()/tc_read() macro helpers is
that they capture quite a bit of context around them and thus require
the caller to have magic variables 'ret' and 'tc' as well as label
'err'. That makes a number of code paths rather counter-intuitive and
somewhat clunky, for example tc_stream_clock_calc() ends up being like
this:
int ret;
tc_write(DP0_VIDMNGEN1, 32768);
return 0;
err:
return ret;
which is rather surprising when you read the code for the first
time. Since those helpers arguably aren't really saving that much code
and there's no way of fixing them without making them too verbose to
be worth it change the driver code to not use them at all.
Signed-off-by: Andrey Smirnov
Reviewed-by: Andrzej Hajda
Cc: Andrzej Hajda
Cc: Laurent Pinchart
Cc: Tomi Valkeinen
Cc: Andrey Gusakov
Cc: Philipp Zabel
Cc: Cory Tusar
Cc: Chris Healy
Cc: Lucas Stach
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
drivers/gpu/drm/bridge/tc358767.c | 383 ++
1 file changed, 231 insertions(+), 152 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c
b/drivers/gpu/drm/bridge/tc358767.c
index 5b78021d6c5b..6a3e7c7e1189 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -280,20 +280,6 @@ static inline struct tc_data *connector_to_tc(struct
drm_connector *c)
return container_of(c, struct tc_data, connector);
}
-/* Simple macros to avoid repeated error checks */
-#define tc_write(reg, var) \
- do {\
- ret = regmap_write(tc->regmap, reg, var); \
- if (ret)\
- goto err; \
- } while (0)
-#define tc_read(reg, var) \
- do {\
- ret = regmap_read(tc->regmap, reg, var);\
- if (ret)\
- goto err; \
- } while (0)
-
static inline int tc_poll_timeout(struct tc_data *tc, unsigned int addr,
unsigned int cond_mask,
unsigned int cond_value,
@@ -351,7 +337,7 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
ret = tc_aux_wait_busy(tc, 100);
if (ret)
- goto err;
+ return ret;
if (request == DP_AUX_I2C_WRITE || request == DP_AUX_NATIVE_WRITE) {
/* Store data */
@@ -362,7 +348,11 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
tmp = (tmp << 8) | buf[i];
i++;
if (((i % 4) == 0) || (i == size)) {
- tc_write(DP0_AUXWDATA((i - 1) >> 2), tmp);
+ ret = regmap_write(tc->regmap,
+ DP0_AUXWDATA((i - 1) >> 2),
+ tmp);
+ if (ret)
+ return ret;
tmp = 0;
}
}
@@ -372,23 +362,32 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
}
/* Store address */
- tc_write(DP0_AUXADDR, msg->address);
+ ret = regmap_write(tc->regmap, DP0_AUXADDR, msg->address);
+ if (ret)
+ return ret;
/* Start transfer */
- tc_write(DP0_AUXCFG0, ((size - 1) << 8) | request);
+ ret = regmap_write(tc->regmap, DP0_AUXCFG0,
+ ((size - 1) << 8) | request);
+ if (ret)
+ return ret;
ret = tc_aux_wait_busy(tc, 100);
if (ret)
- goto err;
+ return ret;
ret = tc_aux_get_status(tc, &msg->reply);
if (ret)
- goto err;
+ return ret;
if (request == DP_AUX_I2C_READ || request == DP_AUX_NATIVE_READ) {
/* Read data */
while (i < size) {
- if ((i % 4) == 0)
- tc_read(DP0_AUXRDATA(i >> 2), &tmp);
+ if ((i % 4) == 0) {
+ ret = regmap_read(tc->regmap,
+ DP0_AUXRDATA(i >> 2), &tmp);
+ if (ret)
+ return ret;
+ }
buf[i] = tmp & 0xff;
tmp = tmp >> 8;
i++;
@@ -396,8 +395,6 @@ sta
[PATCH v6 13/15] drm/bridge: tc358767: Drop unnecessary 8 byte buffer
tc_get_display_props() never reads more than a byte via AUX, so
there's no need to reserve 8 for that purpose. No function change
intended.
Signed-off-by: Andrey Smirnov
Reviewed-by: Andrzej Hajda
Cc: Andrzej Hajda
Cc: Laurent Pinchart
Cc: Tomi Valkeinen
Cc: Andrey Gusakov
Cc: Philipp Zabel
Cc: Cory Tusar
Cc: Chris Healy
Cc: Lucas Stach
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
drivers/gpu/drm/bridge/tc358767.c | 13 ++---
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c
b/drivers/gpu/drm/bridge/tc358767.c
index 7cc26e26f371..f0baf6d7ca80 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -645,8 +645,7 @@ static int tc_aux_link_setup(struct tc_data *tc)
static int tc_get_display_props(struct tc_data *tc)
{
int ret;
- /* temp buffer */
- u8 tmp[8];
+ u8 reg;
/* Read DP Rx Link Capability */
ret = drm_dp_link_probe(&tc->aux, &tc->link.base);
@@ -662,21 +661,21 @@ static int tc_get_display_props(struct tc_data *tc)
tc->link.base.num_lanes = 2;
}
- ret = drm_dp_dpcd_readb(&tc->aux, DP_MAX_DOWNSPREAD, tmp);
+ ret = drm_dp_dpcd_readb(&tc->aux, DP_MAX_DOWNSPREAD, ®);
if (ret < 0)
goto err_dpcd_read;
- tc->link.spread = tmp[0] & DP_MAX_DOWNSPREAD_0_5;
+ tc->link.spread = reg & DP_MAX_DOWNSPREAD_0_5;
- ret = drm_dp_dpcd_readb(&tc->aux, DP_MAIN_LINK_CHANNEL_CODING, tmp);
+ ret = drm_dp_dpcd_readb(&tc->aux, DP_MAIN_LINK_CHANNEL_CODING, ®);
if (ret < 0)
goto err_dpcd_read;
tc->link.scrambler_dis = false;
/* read assr */
- ret = drm_dp_dpcd_readb(&tc->aux, DP_EDP_CONFIGURATION_SET, tmp);
+ ret = drm_dp_dpcd_readb(&tc->aux, DP_EDP_CONFIGURATION_SET, ®);
if (ret < 0)
goto err_dpcd_read;
- tc->link.assr = tmp[0] & DP_ALTERNATE_SCRAMBLER_RESET_ENABLE;
+ tc->link.assr = reg & DP_ALTERNATE_SCRAMBLER_RESET_ENABLE;
dev_dbg(tc->dev, "DPCD rev: %d.%d, rate: %s, lanes: %d, framing: %s\n",
tc->link.base.revision >> 4, tc->link.base.revision & 0x0f,
--
2.21.0
[PATCH v6 12/15] drm/bridge: tc358767: Simplify tc_aux_wait_busy()
We never pass anything but 100 as timeout_ms to tc_aux_wait_busy(), so
we may as well hardcode that value and simplify function's signature.
Signed-off-by: Andrey Smirnov
Reviewed-by: Andrzej Hajda
Cc: Andrzej Hajda
Cc: Laurent Pinchart
Cc: Tomi Valkeinen
Cc: Andrey Gusakov
Cc: Philipp Zabel
Cc: Cory Tusar
Cc: Chris Healy
Cc: Lucas Stach
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
drivers/gpu/drm/bridge/tc358767.c | 9 -
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c
b/drivers/gpu/drm/bridge/tc358767.c
index fe672f6bba73..7cc26e26f371 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -292,10 +292,9 @@ static inline int tc_poll_timeout(struct tc_data *tc,
unsigned int addr,
sleep_us, timeout_us);
}
-static int tc_aux_wait_busy(struct tc_data *tc, unsigned int timeout_ms)
+static int tc_aux_wait_busy(struct tc_data *tc)
{
- return tc_poll_timeout(tc, DP0_AUXSTATUS, AUX_BUSY, 0,
- 1000, 1000 * timeout_ms);
+ return tc_poll_timeout(tc, DP0_AUXSTATUS, AUX_BUSY, 0, 1000, 10);
}
static int tc_aux_write_data(struct tc_data *tc, const void *data,
@@ -339,7 +338,7 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
if (size == 0)
return 0;
- ret = tc_aux_wait_busy(tc, 100);
+ ret = tc_aux_wait_busy(tc);
if (ret)
return ret;
@@ -367,7 +366,7 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
if (ret)
return ret;
- ret = tc_aux_wait_busy(tc, 100);
+ ret = tc_aux_wait_busy(tc);
if (ret)
return ret;
--
2.21.0
[PATCH v6 07/15] drm/bridge: tc358767: Simplify AUX data write
Simplify AUX data write by dropping index arithmetic and shifting and
replacing it with a call to a helper function that does two things:
1. Copies user-provided data into a write buffer
2. Transfers contents of the write buffer to up to 4 32-bit
registers on the chip
Note that separate data endianness fix:
tmp = (tmp << 8) | buf[i];
that was reserved for DP_AUX_I2C_WRITE looks really strange, since it
will place data differently depending on the passed user-data
size. E.g. for a write of 1 byte, data transferred to the chip would
look like:
[byte0] [dummy1] [dummy2] [dummy3]
whereas for a write of 4 bytes we'd get:
[byte3] [byte2] [byte1] [byte0]
Since there's no indication in the datasheet that I2C write buffer
should be treated differently than AUX write buffer and no comment in
the original code explaining why it was done this way, that special
I2C write buffer transformation was dropped in this patch.
Signed-off-by: Andrey Smirnov
Reviewed-by: Andrzej Hajda
Cc: Andrzej Hajda
Cc: Laurent Pinchart
Cc: Tomi Valkeinen
Cc: Andrey Gusakov
Cc: Philipp Zabel
Cc: Cory Tusar
Cc: Chris Healy
Cc: Lucas Stach
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
drivers/gpu/drm/bridge/tc358767.c | 48 +--
1 file changed, 26 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c
b/drivers/gpu/drm/bridge/tc358767.c
index 02f6d907f5c4..a441e8e66287 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -321,6 +321,21 @@ static int tc_aux_get_status(struct tc_data *tc, u8 *reply)
return 0;
}
+static int tc_aux_write_data(struct tc_data *tc, const void *data,
+size_t size)
+{
+ u32 auxwdata[DP_AUX_MAX_PAYLOAD_BYTES / sizeof(u32)] = { 0 };
+ int ret, count = ALIGN(size, sizeof(u32));
+
+ memcpy(auxwdata, data, size);
+
+ ret = regmap_raw_write(tc->regmap, DP0_AUXWDATA(0), auxwdata, count);
+ if (ret)
+ return ret;
+
+ return size;
+}
+
static int tc_aux_read_data(struct tc_data *tc, void *data, size_t size)
{
u32 auxrdata[DP_AUX_MAX_PAYLOAD_BYTES / sizeof(u32)];
@@ -341,9 +356,6 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
struct tc_data *tc = aux_to_tc(aux);
size_t size = min_t(size_t, 8, msg->size);
u8 request = msg->request & ~DP_AUX_I2C_MOT;
- u8 *buf = msg->buffer;
- u32 tmp = 0;
- int i = 0;
int ret;
if (size == 0)
@@ -353,25 +365,17 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
if (ret)
return ret;
- if (request == DP_AUX_I2C_WRITE || request == DP_AUX_NATIVE_WRITE) {
- /* Store data */
- while (i < size) {
- if (request == DP_AUX_NATIVE_WRITE)
- tmp = tmp | (buf[i] << (8 * (i & 0x3)));
- else
- tmp = (tmp << 8) | buf[i];
- i++;
- if (((i % 4) == 0) || (i == size)) {
- ret = regmap_write(tc->regmap,
- DP0_AUXWDATA((i - 1) >> 2),
- tmp);
- if (ret)
- return ret;
- tmp = 0;
- }
- }
- } else if (request != DP_AUX_I2C_READ &&
- request != DP_AUX_NATIVE_READ) {
+ switch (request) {
+ case DP_AUX_NATIVE_READ:
+ case DP_AUX_I2C_READ:
+ break;
+ case DP_AUX_NATIVE_WRITE:
+ case DP_AUX_I2C_WRITE:
+ ret = tc_aux_write_data(tc, msg->buffer, size);
+ if (ret < 0)
+ return ret;
+ break;
+ default:
return -EINVAL;
}
--
2.21.0
[PATCH v6 09/15] drm/bridge: tc358767: Use reported AUX transfer size
Don't assume that requested data transfer size is the same as amount
of data that was transferred. Change the code to get that information
from DP0_AUXSTATUS instead.
Since the check for AUX_BUSY in tc_aux_get_status() is pointless (it
will always called after tc_aux_wait_busy()) and there's only one user
of it, inline its code into tc_aux_transfer() instead of trying to
accommodate the change above.
Signed-off-by: Andrey Smirnov
Reviewed-by: Andrzej Hajda
Cc: Andrzej Hajda
Cc: Laurent Pinchart
Cc: Tomi Valkeinen
Cc: Andrey Gusakov
Cc: Philipp Zabel
Cc: Cory Tusar
Cc: Chris Healy
Cc: Lucas Stach
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
drivers/gpu/drm/bridge/tc358767.c | 40 ++-
1 file changed, 12 insertions(+), 28 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c
b/drivers/gpu/drm/bridge/tc358767.c
index bdbf88057946..b01c1c8341e1 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -152,10 +152,10 @@
#define DP0_AUXWDATA(i)(0x066c + (i) * 4)
#define DP0_AUXRDATA(i)(0x067c + (i) * 4)
#define DP0_AUXSTATUS 0x068c
-#define AUX_STATUS_MASK0xf0
-#define AUX_STATUS_SHIFT 4
-#define AUX_TIMEOUTBIT(1)
-#define AUX_BUSY BIT(0)
+#define AUX_BYTES GENMASK(15, 8)
+#define AUX_STATUS GENMASK(7, 4)
+#define AUX_TIMEOUTBIT(1)
+#define AUX_BUSY BIT(0)
#define DP0_AUXI2CADR 0x0698
/* Link Training */
@@ -298,29 +298,6 @@ static int tc_aux_wait_busy(struct tc_data *tc, unsigned
int timeout_ms)
1000, 1000 * timeout_ms);
}
-static int tc_aux_get_status(struct tc_data *tc, u8 *reply)
-{
- int ret;
- u32 value;
-
- ret = regmap_read(tc->regmap, DP0_AUXSTATUS, &value);
- if (ret < 0)
- return ret;
-
- if (value & AUX_BUSY) {
- dev_err(tc->dev, "aux busy!\n");
- return -EBUSY;
- }
-
- if (value & AUX_TIMEOUT) {
- dev_err(tc->dev, "aux access timeout!\n");
- return -ETIMEDOUT;
- }
-
- *reply = (value & AUX_STATUS_MASK) >> AUX_STATUS_SHIFT;
- return 0;
-}
-
static int tc_aux_write_data(struct tc_data *tc, const void *data,
size_t size)
{
@@ -356,6 +333,7 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
struct tc_data *tc = aux_to_tc(aux);
size_t size = min_t(size_t, DP_AUX_MAX_PAYLOAD_BYTES - 1, msg->size);
u8 request = msg->request & ~DP_AUX_I2C_MOT;
+ u32 auxstatus;
int ret;
if (size == 0)
@@ -393,10 +371,16 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
if (ret)
return ret;
- ret = tc_aux_get_status(tc, &msg->reply);
+ ret = regmap_read(tc->regmap, DP0_AUXSTATUS, &auxstatus);
if (ret)
return ret;
+ if (auxstatus & AUX_TIMEOUT)
+ return -ETIMEDOUT;
+
+ size = FIELD_GET(AUX_BYTES, auxstatus);
+ msg->reply = FIELD_GET(AUX_STATUS, auxstatus);
+
switch (request) {
case DP_AUX_NATIVE_READ:
case DP_AUX_I2C_READ:
--
2.21.0
Re: [21/21] drm/bridge: tc358767: implement naive HPD handling
> tc358767 driver doesn't have any HPD handling at the moment, as it was
> originally developed to support only eDP.
> This patch implements a naive, polling-based HPD handling, which is used
> when the driver is in DP mode.
> Signed-off-by: Tomi Valkeinen
> ---
> drivers/gpu/drm/bridge/tc358767.c | 56 +--
> 1 file changed, 38 insertions(+), 18 deletions(-)
> diff --git a/drivers/gpu/drm/bridge/tc358767.c
> b/drivers/gpu/drm/bridge/tc358767.c
> index 8606de29c9b2..2b252f7ac070 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -1095,6 +1095,12 @@ static void tc_bridge_enable(struct drm_bridge *bridge)
> struct tc_data *tc = bridge_to_tc(bridge);
> int ret;
> + ret = tc_get_display_props(tc);
> + if (ret < 0) {
> + dev_err(tc->dev, "failed to read display props: %d\n", ret);
> + return;
> + }
> +
> ret = tc_main_link_enable(tc);
> if (ret < 0) {
> dev_err(tc->dev, "main link enable error: %d\n", ret);
> @@ -1200,19 +1206,35 @@ static int tc_connector_get_modes(struct
> drm_connector *connector)
> return count;
> }
> -static void tc_connector_set_polling(struct tc_data *tc,
> - struct drm_connector *connector)
> -{
> - /* TODO: add support for HPD */
> - connector->polled = DRM_CONNECTOR_POLL_CONNECT |
> - DRM_CONNECTOR_POLL_DISCONNECT;
> -}
> -
> static const struct drm_connector_helper_funcs tc_connector_helper_funcs = {
> .get_modes = tc_connector_get_modes,
> };
> +static enum drm_connector_status tc_connector_detect(struct drm_connector
> *connector, bool force)
> +{
> + struct tc_data *tc = connector_to_tc(connector);
> + u32 val;
> + int ret;
> + bool conn;
> +
> + tc_read(GPIOI, &val);
> +
> + conn = val & BIT(0);
TC358767 has two GPIO pins that can be used for HPD signal. I think
instead of hardcoding GPIO0 here it would be more flexible to expose
boths gpios as a gpiochip and use gpiolib API to query the value of
HPD as well as use "hpd-gpios" binidng in DT to select which input to
use.
Another argument in favour of this solution is that Toshiba's FAEs (at
least some) recommend thier customers to connect HPD signal to SoC's
GPIOs and bypass TC358767 entirely. Their reasoning being that
TC358767 implements a generic GPIO contoller and there's no advantage
in going through TC358767 if you could use your "normal" GPIOs.
Using gpiolib API would allow us to handle both usecase with the same
code.
Lastly, by treating "hpd-gpios" as an optional property, we can
preserve old driver behaviour regardless if it is connected to DP or
eDP panel. Not saying that this is really worth doing, just pointing
out that this option would be on the table as well.
Thanks,
Andrey Smirnov
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [21/21] drm/bridge: tc358767: implement naive HPD handling
On Tue, Mar 19, 2019 at 11:18 AM Andrey Smirnov
wrote:
>
> > tc358767 driver doesn't have any HPD handling at the moment, as it was
> > originally developed to support only eDP.
>
> > This patch implements a naive, polling-based HPD handling, which is used
> > when the driver is in DP mode.
>
> > Signed-off-by: Tomi Valkeinen
> > ---
> > drivers/gpu/drm/bridge/tc358767.c | 56 +--
> > 1 file changed, 38 insertions(+), 18 deletions(-)
>
> > diff --git a/drivers/gpu/drm/bridge/tc358767.c
> > b/drivers/gpu/drm/bridge/tc358767.c
> > index 8606de29c9b2..2b252f7ac070 100644
> > --- a/drivers/gpu/drm/bridge/tc358767.c
> > +++ b/drivers/gpu/drm/bridge/tc358767.c
> > @@ -1095,6 +1095,12 @@ static void tc_bridge_enable(struct drm_bridge
> > *bridge)
> > struct tc_data *tc = bridge_to_tc(bridge);
> > int ret;
>
> > + ret = tc_get_display_props(tc);
> > + if (ret < 0) {
> > + dev_err(tc->dev, "failed to read display props: %d\n", ret);
> > + return;
> > + }
> > +
> > ret = tc_main_link_enable(tc);
> > if (ret < 0) {
> > dev_err(tc->dev, "main link enable error: %d\n", ret);
> > @@ -1200,19 +1206,35 @@ static int tc_connector_get_modes(struct
> > drm_connector *connector)
> > return count;
> > }
>
> > -static void tc_connector_set_polling(struct tc_data *tc,
> > - struct drm_connector *connector)
> > -{
> > - /* TODO: add support for HPD */
> > - connector->polled = DRM_CONNECTOR_POLL_CONNECT |
> > - DRM_CONNECTOR_POLL_DISCONNECT;
> > -}
> > -
> > static const struct drm_connector_helper_funcs tc_connector_helper_funcs =
> > {
> > .get_modes = tc_connector_get_modes,
> > };
>
> > +static enum drm_connector_status tc_connector_detect(struct drm_connector
> > *connector, bool force)
> > +{
> > + struct tc_data *tc = connector_to_tc(connector);
> > + u32 val;
> > + int ret;
> > + bool conn;
> > +
> > + tc_read(GPIOI, &val);
> > +
> > + conn = val & BIT(0);
Another thing I noticed when trying this patch is that
tc_connector_detect() will get called via drm_helper_probe_detect()
even if tc->panel is not NULL and tc->connector.polled is zero, which
creates a problem for eDP use-case.
Thanks,
Andrey Smirnov
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [21/21] drm/bridge: tc358767: implement naive HPD handling
On Wed, Mar 20, 2019 at 6:03 AM Tomi Valkeinen wrote: > > On 20/03/2019 08:57, Tomi Valkeinen wrote: > > On 19/03/2019 20:18, Andrey Smirnov wrote: > > > >> TC358767 has two GPIO pins that can be used for HPD signal. I think > >> instead of hardcoding GPIO0 here it would be more flexible to expose > >> boths gpios as a gpiochip and use gpiolib API to query the value of > >> HPD as well as use "hpd-gpios" binidng in DT to select which input to > >> use. > >> > >> Another argument in favour of this solution is that Toshiba's FAEs (at > >> least some) recommend thier customers to connect HPD signal to SoC's > >> GPIOs and bypass TC358767 entirely. Their reasoning being that > >> TC358767 implements a generic GPIO contoller and there's no advantage > >> in going through TC358767 if you could use your "normal" GPIOs. > >> > >> Using gpiolib API would allow us to handle both usecase with the same > >> code. > >> > >> Lastly, by treating "hpd-gpios" as an optional property, we can > >> preserve old driver behaviour regardless if it is connected to DP or > >> eDP panel. Not saying that this is really worth doing, just pointing > >> out that this option would be on the table as well. > > > > I think that's a good point. > > > > There's one thing that TC358767 offers, which may not be available on > > most generic GPIO controllers, though: it can detect short (programmable > > length) pulses, thus it's possible to easily implement the DisplayPort > > IRQ mechanism. I'm not sure if it's possible to implement reliable DP > > IRQ detection without HW support. > > > > Still, I think using standard gpios makes sense. > > After implementing the gpiochip (it works), I started to wonder... > > If TC358767 is used as a gpio expander, for whatever purpose, outside > the TC358767 driver, then obviously we need the gpiochip driver. But I > don't think anyone needs that. > Regardless of how it's going to be implemented in the end, there'd have to be a way to specify which HPD input is being used. Which means a either a new DT binding or re-using already existing to be agreed on by DT folks. It just seems to me that there exists a much stronger case to solve this using existing "language" of GPIO references as opposed to introducing some vendor specific binding serving just this single purpose. If DT is supposed to be used to describe the HW, then, IMHO, it might be the other way around, TC358767 is also a GPIO expander and has to be modeled/implemented as such, whether anyone would ever use it in such capacity fully isn't that significant. > Then we have two cases 1) HPD connected to TC358767, 2) HPD goes > directly to the SoC, or worded differently, HPD is handled by something > else than TC358767. > > 1) was implemented in this current patch, and there's no real benefit > with the gpiochip. It's somewhat confusing that the driver provides a > gpiochip which the same driver then uses, for its internal functionality. > > 2) should actually not involve TC358767 driver at all as it's totally > outside TC358767. > There's already precedent for such usage in ti-tfp410.c, analogix/ and andanalogix-anx78xx.c, so it's not unheard of. > If the HPD goes from the DP connector to the SoC, we should have the DP > connector driver handle it. Currently that connector is in the TC358767 > driver, but it should really be separated. > Sure, there's definitely more than one way to solve this. > So... Obviously what's missing from the current patch is that we need to > be able to say which of the two GPIOs are used for the HPD (if any). But > I'm debating with myself whether gpiochip here is a sane choice or not. Yeah, maybe it'd be best to submit a patch to DT mailing list and see what they have to say? Thanks, Andrey Smirnov ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 14/15] drm/bridge: tc358767: Drop unnecessary 8 byte buffer
tc_get_display_props() never reads more than a byte via AUX, so
there's no need to reserve 8 for that purpose. No function change
intended.
Signed-off-by: Andrey Smirnov
Cc: Archit Taneja
Cc: Andrzej Hajda
Cc: Laurent Pinchart
Cc: Tomi Valkeinen
Cc: Andrey Gusakov
Cc: Philipp Zabel
Cc: Chris Healy
Cc: Lucas Stach
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
drivers/gpu/drm/bridge/tc358767.c | 13 ++---
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c
b/drivers/gpu/drm/bridge/tc358767.c
index 07d568761463..b106e6181a5f 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -664,8 +664,7 @@ static int tc_aux_link_setup(struct tc_data *tc)
static int tc_get_display_props(struct tc_data *tc)
{
int ret;
- /* temp buffer */
- u8 tmp[8];
+ u8 reg;
/* Read DP Rx Link Capability */
ret = drm_dp_link_probe(&tc->aux, &tc->link.base);
@@ -681,21 +680,21 @@ static int tc_get_display_props(struct tc_data *tc)
tc->link.base.num_lanes = 2;
}
- ret = drm_dp_dpcd_readb(&tc->aux, DP_MAX_DOWNSPREAD, tmp);
+ ret = drm_dp_dpcd_readb(&tc->aux, DP_MAX_DOWNSPREAD, ®);
if (ret < 0)
goto err_dpcd_read;
- tc->link.spread = tmp[0] & DP_MAX_DOWNSPREAD_0_5;
+ tc->link.spread = reg & DP_MAX_DOWNSPREAD_0_5;
- ret = drm_dp_dpcd_readb(&tc->aux, DP_MAIN_LINK_CHANNEL_CODING, tmp);
+ ret = drm_dp_dpcd_readb(&tc->aux, DP_MAIN_LINK_CHANNEL_CODING, ®);
if (ret < 0)
goto err_dpcd_read;
tc->link.scrambler_dis = false;
/* read assr */
- ret = drm_dp_dpcd_readb(&tc->aux, DP_EDP_CONFIGURATION_SET, tmp);
+ ret = drm_dp_dpcd_readb(&tc->aux, DP_EDP_CONFIGURATION_SET, ®);
if (ret < 0)
goto err_dpcd_read;
- tc->link.assr = tmp[0] & DP_ALTERNATE_SCRAMBLER_RESET_ENABLE;
+ tc->link.assr = reg & DP_ALTERNATE_SCRAMBLER_RESET_ENABLE;
dev_dbg(tc->dev, "DPCD rev: %d.%d, rate: %s, lanes: %d, framing: %s\n",
tc->link.base.revision >> 4, tc->link.base.revision & 0x0f,
--
2.20.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 05/15] drm/bridge: tc358767: Drop custom tc_write()/tc_read() accessors
A very unfortunate aspect of tc_write()/tc_read() macro helpers is
that they capture quite a bit of context around them and thus require
the caller to have magic variables 'ret' and 'tc' as well as label
'err'. That makes a number of code paths rather counterintuitive and
somewhat clunky, for example tc_stream_clock_calc() ends up being like
this:
int ret;
tc_write(DP0_VIDMNGEN1, 32768);
return 0;
err:
return ret;
which is rather surprising when you read the code for the first
time. Since those helpers arguably aren't really saving that much code
and there's no way of fixing them without making them too verbose to
be worth it change the driver code to not use them at all.
Signed-off-by: Andrey Smirnov
Cc: Archit Taneja
Cc: Andrzej Hajda
Cc: Laurent Pinchart
Cc: Tomi Valkeinen
Cc: Andrey Gusakov
Cc: Philipp Zabel
Cc: Chris Healy
Cc: Lucas Stach
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
drivers/gpu/drm/bridge/tc358767.c | 249 +-
1 file changed, 144 insertions(+), 105 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c
b/drivers/gpu/drm/bridge/tc358767.c
index d99c9f32a133..060e4b05095a 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -266,20 +266,6 @@ static inline struct tc_data *connector_to_tc(struct
drm_connector *c)
return container_of(c, struct tc_data, connector);
}
-/* Simple macros to avoid repeated error checks */
-#define tc_write(reg, var) \
- do {\
- ret = regmap_write(tc->regmap, reg, var); \
- if (ret)\
- goto err; \
- } while (0)
-#define tc_read(reg, var) \
- do {\
- ret = regmap_read(tc->regmap, reg, var);\
- if (ret)\
- goto err; \
- } while (0)
-
static inline int tc_poll_timeout(struct tc_data *tc, unsigned int addr,
unsigned int cond_mask,
unsigned int cond_value,
@@ -337,7 +323,7 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
ret = tc_aux_wait_busy(tc, 100);
if (ret)
- goto err;
+ return ret;
if (request == DP_AUX_I2C_WRITE || request == DP_AUX_NATIVE_WRITE) {
/* Store data */
@@ -348,7 +334,11 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
tmp = (tmp << 8) | buf[i];
i++;
if (((i % 4) == 0) || (i == size)) {
- tc_write(DP0_AUXWDATA((i - 1) >> 2), tmp);
+ ret = regmap_write(tc->regmap,
+ DP0_AUXWDATA((i - 1) >> 2),
+ tmp);
+ if (ret)
+ return ret;
tmp = 0;
}
}
@@ -358,23 +348,32 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
}
/* Store address */
- tc_write(DP0_AUXADDR, msg->address);
+ ret = regmap_write(tc->regmap, DP0_AUXADDR, msg->address);
+ if (ret)
+ return ret;
/* Start transfer */
- tc_write(DP0_AUXCFG0, ((size - 1) << 8) | request);
+ ret = regmap_write(tc->regmap, DP0_AUXCFG0,
+ ((size - 1) << 8) | request);
+ if (ret)
+ return ret;
ret = tc_aux_wait_busy(tc, 100);
if (ret)
- goto err;
+ return ret;
ret = tc_aux_get_status(tc, &msg->reply);
if (ret)
- goto err;
+ return ret;
if (request == DP_AUX_I2C_READ || request == DP_AUX_NATIVE_READ) {
/* Read data */
while (i < size) {
- if ((i % 4) == 0)
- tc_read(DP0_AUXRDATA(i >> 2), &tmp);
+ if ((i % 4) == 0) {
+ ret = regmap_read(tc->regmap,
+ DP0_AUXRDATA(i >> 2), &tmp);
+ if (ret)
+ return ret;
+ }
buf[i] = tmp & 0xff;
tmp = tmp >> 8;
i++;
@@ -382,8 +381,6 @@ static ssiz
[PATCH v2 00/15] tc358767 driver improvements
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.smir...@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(-) -- 2.20.1 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 07/15] drm/bridge: tc358767: Simplify AUX data write
Simplify AUX data write by dropping index arithmetic and shifting and
replacing it with a call to a helper function that does three things:
1. Copies user-provided data into a write buffer
2. Optionally fixes the endianness of the write buffer (not needed
on LE hosts)
3. Transfers contenst of the write buffer to up to 4 32-bit
registers on the chip
Note that separate data endianness fix:
tmp = (tmp << 8) | buf[i];
that was reserved for DP_AUX_I2C_WRITE looks really strange, since it
will place data differently depending on the passed user-data
size. E.g. for a write of 1 byte, data transferred to the chip would
look like:
[byte0] [dummy1] [dummy2] [dummy3]
whereas for a write of 4 bytes we'd get:
[byte3] [byte2] [byte1] [byte0]
Since there's no indication in the datasheet that I2C write buffer
should be treated differently than AUX write buffer and no comment in
the original code explaining why it was done this way, that special
I2C write buffer transformation was dropped in this patch.
Signed-off-by: Andrey Smirnov
Cc: Archit Taneja
Cc: Andrzej Hajda
Cc: Laurent Pinchart
Cc: Tomi Valkeinen
Cc: Andrey Gusakov
Cc: Philipp Zabel
Cc: Chris Healy
Cc: Lucas Stach
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
drivers/gpu/drm/bridge/tc358767.c | 58 +++
1 file changed, 36 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c
b/drivers/gpu/drm/bridge/tc358767.c
index 81c10a5d8106..21374565585d 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -307,6 +307,31 @@ static int tc_aux_get_status(struct tc_data *tc, u8 *reply)
return 0;
}
+static int tc_aux_write_data(struct tc_data *tc, const void *data,
+size_t size)
+{
+ u32 auxwdata[DP_AUX_MAX_PAYLOAD_BYTES / sizeof(u32)] = { 0 };
+ int ret, i, count = DIV_ROUND_UP(size, 4);
+
+ memcpy(auxwdata, data, size);
+
+ for (i = 0; i < count; i++) {
+ ret = regmap_write(tc->regmap, DP0_AUXWDATA(i),
+ /*
+ * Our regmap is configured as LE
+ * for register data, so we need
+ * undo any byte swapping that will
+ * happened to preserve original
+ * byte order.
+ */
+ cpu_to_le32(auxwdata[i]));
+ if (ret)
+ return ret;
+ }
+
+ return size;
+}
+
static int tc_aux_read_data(struct tc_data *tc, void *data, size_t size)
{
u32 auxrdata[DP_AUX_MAX_PAYLOAD_BYTES / sizeof(u32)];
@@ -335,9 +360,6 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
struct tc_data *tc = aux_to_tc(aux);
size_t size = min_t(size_t, 8, msg->size);
u8 request = msg->request & ~DP_AUX_I2C_MOT;
- u8 *buf = msg->buffer;
- u32 tmp = 0;
- int i = 0;
int ret;
if (size == 0)
@@ -347,25 +369,17 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
if (ret)
return ret;
- if (request == DP_AUX_I2C_WRITE || request == DP_AUX_NATIVE_WRITE) {
- /* Store data */
- while (i < size) {
- if (request == DP_AUX_NATIVE_WRITE)
- tmp = tmp | (buf[i] << (8 * (i & 0x3)));
- else
- tmp = (tmp << 8) | buf[i];
- i++;
- if (((i % 4) == 0) || (i == size)) {
- ret = regmap_write(tc->regmap,
- DP0_AUXWDATA((i - 1) >> 2),
- tmp);
- if (ret)
- return ret;
- tmp = 0;
- }
- }
- } else if (request != DP_AUX_I2C_READ &&
- request != DP_AUX_NATIVE_READ) {
+ switch (request) {
+ case DP_AUX_NATIVE_READ:
+ case DP_AUX_I2C_READ:
+ break;
+ case DP_AUX_NATIVE_WRITE:
+ case DP_AUX_I2C_WRITE:
+ ret = tc_aux_write_data(tc, msg->buffer, size);
+ if (ret < 0)
+ return ret;
+ break;
+ default:
return -EINVAL;
}
--
2.20.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 03/15] drm/bridge: tc358767: Simplify polling in tc_link_training()
Replace explicit polling in tc_link_training() with equivalent call to
tc_poll_timeout() for simplicity. No functional change intended (not
including slightly altered debug output).
Signed-off-by: Andrey Smirnov
Cc: Archit Taneja
Cc: Andrzej Hajda
Cc: Laurent Pinchart
Cc: Tomi Valkeinen
Cc: Andrey Gusakov
Cc: Philipp Zabel
Cc: Chris Healy
Cc: Lucas Stach
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
drivers/gpu/drm/bridge/tc358767.c | 13 +
1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c
b/drivers/gpu/drm/bridge/tc358767.c
index 2531f4dadbf8..38d542f553cd 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -733,21 +733,18 @@ static int tc_set_video_mode(struct tc_data *tc, struct
drm_display_mode *mode)
static int tc_wait_link_training(struct tc_data *tc, u32 *error)
{
- u32 timeout = 1000;
u32 value;
int ret;
- do {
- udelay(1);
- tc_read(DP0_LTSTAT, &value);
- } while ((!(value & LT_LOOPDONE)) && (--timeout));
-
- if (timeout == 0) {
+ ret = tc_poll_timeout(tc, DP0_LTSTAT, LT_LOOPDONE,
+ LT_LOOPDONE, 1, 1000);
+ if (ret) {
dev_err(tc->dev, "Link training timeout waiting for
LT_LOOPDONE!\n");
- ret = -ETIMEDOUT;
goto err;
}
+ tc_read(DP0_LTSTAT, &value);
+
*error = (value >> 8) & 0x7;
return 0;
--
2.20.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 11/15] drm/bridge: tc358767: Introduce tc_set_syspllparam()
Move common code converting clock rate to an appropriate constant and
configuring SYS_PLLPARAM register into a separate routine and convert
the rest of the code to use it. No functional change intended.
Signed-off-by: Andrey Smirnov
Reviewed-by: Laurent Pinchart
Cc: Archit Taneja
Cc: Andrzej Hajda
Cc: Laurent Pinchart
Cc: Tomi Valkeinen
Cc: Andrey Gusakov
Cc: Philipp Zabel
Cc: Chris Healy
Cc: Lucas Stach
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
drivers/gpu/drm/bridge/tc358767.c | 48 +++
1 file changed, 17 insertions(+), 31 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c
b/drivers/gpu/drm/bridge/tc358767.c
index 768f01cc2a30..976a9861e537 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -585,35 +585,41 @@ static int tc_stream_clock_calc(struct tc_data *tc)
return regmap_write(tc->regmap, DP0_VIDMNGEN1, 32768);
}
-static int tc_aux_link_setup(struct tc_data *tc)
+static int tc_set_syspllparam(struct tc_data *tc)
{
unsigned long rate;
- u32 dp0_auxcfg1;
- u32 value;
- int ret;
+ u32 pllparam = SYSCLK_SEL_LSCLK | LSCLK_DIV_2;
rate = clk_get_rate(tc->refclk);
switch (rate) {
case 3840:
- value = REF_FREQ_38M4;
+ pllparam |= REF_FREQ_38M4;
break;
case 2600:
- value = REF_FREQ_26M;
+ pllparam |= REF_FREQ_26M;
break;
case 1920:
- value = REF_FREQ_19M2;
+ pllparam |= REF_FREQ_19M2;
break;
case 1300:
- value = REF_FREQ_13M;
+ pllparam |= REF_FREQ_13M;
break;
default:
dev_err(tc->dev, "Invalid refclk rate: %lu Hz\n", rate);
return -EINVAL;
}
+ return regmap_write(tc->regmap, SYS_PLLPARAM, pllparam);
+}
+
+static int tc_aux_link_setup(struct tc_data *tc)
+{
+ int ret;
+ u32 dp_phy_ctrl;
+ u32 dp0_auxcfg1;
+
/* Setup DP-PHY / PLL */
- value |= SYSCLK_SEL_LSCLK | LSCLK_DIV_2;
- ret = regmap_write(tc->regmap, SYS_PLLPARAM, value);
+ ret = tc_set_syspllparam(tc);
if (ret)
goto err;
@@ -817,8 +823,6 @@ static int tc_main_link_enable(struct tc_data *tc)
{
struct drm_dp_aux *aux = &tc->aux;
struct device *dev = tc->dev;
- unsigned int rate;
- u32 dp_phy_ctrl;
u32 value;
int ret;
u8 tmp[8];
@@ -840,25 +844,7 @@ static int tc_main_link_enable(struct tc_data *tc)
if (ret)
return ret;
- rate = clk_get_rate(tc->refclk);
- switch (rate) {
- case 3840:
- value = REF_FREQ_38M4;
- break;
- case 2600:
- value = REF_FREQ_26M;
- break;
- case 1920:
- value = REF_FREQ_19M2;
- break;
- case 1300:
- value = REF_FREQ_13M;
- break;
- default:
- return -EINVAL;
- }
- value |= SYSCLK_SEL_LSCLK | LSCLK_DIV_2;
- ret = regmap_write(tc->regmap, SYS_PLLPARAM, value);
+ ret = tc_set_syspllparam(tc);
if (ret)
return ret;
--
2.20.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 15/15] drm/bridge: tc358767: Replace magic number in tc_main_link_enable()
We don't need 8 byte array, DP_LINK_STATUS_SIZE (6) should be enough. This also gets rid of a magic number as a bonus. Signed-off-by: Andrey Smirnov Cc: Archit Taneja Cc: Andrzej Hajda Cc: Laurent Pinchart Cc: Tomi Valkeinen Cc: Andrey Gusakov Cc: Philipp Zabel Cc: Chris Healy Cc: Lucas Stach Cc: dri-devel@lists.freedesktop.org Cc: linux-ker...@vger.kernel.org --- drivers/gpu/drm/bridge/tc358767.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c index b106e6181a5f..68af144b8768 100644 --- a/drivers/gpu/drm/bridge/tc358767.c +++ b/drivers/gpu/drm/bridge/tc358767.c @@ -823,7 +823,7 @@ static int tc_main_link_enable(struct tc_data *tc) u32 dp_phy_ctrl; u32 value; int ret; - u8 tmp[8]; + u8 tmp[DP_LINK_STATUS_SIZE]; u32 error; dev_dbg(tc->dev, "link enable\n"); -- 2.20.1 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 13/15] drm/bridge: tc358767: Simplify tc_aux_wait_busy()
We never pass anything but 100 as timeout_ms to tc_aux_wait_busy(), so
we may as well hardcode that value and simplify function's signature.
Signed-off-by: Andrey Smirnov
Cc: Archit Taneja
Cc: Andrzej Hajda
Cc: Laurent Pinchart
Cc: Tomi Valkeinen
Cc: Andrey Gusakov
Cc: Philipp Zabel
Cc: Chris Healy
Cc: Lucas Stach
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
drivers/gpu/drm/bridge/tc358767.c | 9 -
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c
b/drivers/gpu/drm/bridge/tc358767.c
index f66a1c4a2047..07d568761463 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -280,10 +280,9 @@ static inline int tc_poll_timeout(struct tc_data *tc,
unsigned int addr,
sleep_us, timeout_us);
}
-static int tc_aux_wait_busy(struct tc_data *tc, unsigned int timeout_ms)
+static int tc_aux_wait_busy(struct tc_data *tc)
{
- return tc_poll_timeout(tc, DP0_AUXSTATUS, AUX_BUSY, 0,
- 1000, 1000 * timeout_ms);
+ return tc_poll_timeout(tc, DP0_AUXSTATUS, AUX_BUSY, 0, 1000, 10);
}
static int tc_aux_write_data(struct tc_data *tc, const void *data,
@@ -354,7 +353,7 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
u32 auxstatus;
int ret;
- ret = tc_aux_wait_busy(tc, 100);
+ ret = tc_aux_wait_busy(tc);
if (ret)
return ret;
@@ -381,7 +380,7 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
if (ret)
return ret;
- ret = tc_aux_wait_busy(tc, 100);
+ ret = tc_aux_wait_busy(tc);
if (ret)
return ret;
--
2.20.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 09/15] drm/bridge: tc358767: Use reported AUX transfer size
Don't assume that requested data transfer size is the same as amount
of data that was transferred. Change the code to get that information
from DP0_AUXSTATUS instead.
Since the check for AUX_BUSY in tc_aux_get_status() is pointless (it
will always called after tc_aux_wait_busy()) and there's only one user
of it, inline its code into tc_aux_transfer() instead of trying to
accommodate the change above.
Signed-off-by: Andrey Smirnov
Cc: Archit Taneja
Cc: Andrzej Hajda
Cc: Laurent Pinchart
Cc: Tomi Valkeinen
Cc: Andrey Gusakov
Cc: Philipp Zabel
Cc: Chris Healy
Cc: Lucas Stach
Cc: dri-devel@lists.freedesktop.org
Cc: linux-ker...@vger.kernel.org
---
drivers/gpu/drm/bridge/tc358767.c | 40 ++-
1 file changed, 12 insertions(+), 28 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c
b/drivers/gpu/drm/bridge/tc358767.c
index 8adaac5ca271..7e4607c6907f 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -144,10 +144,10 @@
#define DP0_AUXWDATA(i)(0x066c + (i) * 4)
#define DP0_AUXRDATA(i)(0x067c + (i) * 4)
#define DP0_AUXSTATUS 0x068c
-#define AUX_STATUS_MASK0xf0
-#define AUX_STATUS_SHIFT 4
-#define AUX_TIMEOUTBIT(1)
-#define AUX_BUSY BIT(0)
+#define AUX_BYTES GENMASK(15, 8)
+#define AUX_STATUS GENMASK(7, 4)
+#define AUX_TIMEOUTBIT(1)
+#define AUX_BUSY BIT(0)
#define DP0_AUXI2CADR 0x0698
/* Link Training */
@@ -284,29 +284,6 @@ static int tc_aux_wait_busy(struct tc_data *tc, unsigned
int timeout_ms)
1000, 1000 * timeout_ms);
}
-static int tc_aux_get_status(struct tc_data *tc, u8 *reply)
-{
- int ret;
- u32 value;
-
- ret = regmap_read(tc->regmap, DP0_AUXSTATUS, &value);
- if (ret < 0)
- return ret;
-
- if (value & AUX_BUSY) {
- dev_err(tc->dev, "aux busy!\n");
- return -EBUSY;
- }
-
- if (value & AUX_TIMEOUT) {
- dev_err(tc->dev, "aux access timeout!\n");
- return -ETIMEDOUT;
- }
-
- *reply = (value & AUX_STATUS_MASK) >> AUX_STATUS_SHIFT;
- return 0;
-}
-
static int tc_aux_write_data(struct tc_data *tc, const void *data,
size_t size)
{
@@ -360,6 +337,7 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
struct tc_data *tc = aux_to_tc(aux);
size_t size = min_t(size_t, DP_AUX_MAX_PAYLOAD_BYTES - 1, msg->size);
u8 request = msg->request & ~DP_AUX_I2C_MOT;
+ u32 auxstatus;
int ret;
if (size == 0)
@@ -397,10 +375,16 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
if (ret)
return ret;
- ret = tc_aux_get_status(tc, &msg->reply);
+ ret = regmap_read(tc->regmap, DP0_AUXSTATUS, &auxstatus);
if (ret)
return ret;
+ if (auxstatus & AUX_TIMEOUT)
+ return -ETIMEDOUT;
+
+ size = FIELD_GET(AUX_BYTES, auxstatus);
+ msg->reply = FIELD_GET(AUX_STATUS, auxstatus);
+
switch (request) {
case DP_AUX_NATIVE_READ:
case DP_AUX_I2C_READ:
--
2.20.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 04/15] drm/bridge: tc358767: Simplify tc_set_video_mode()
On Fri, Mar 22, 2019 at 3:19 AM Tomi Valkeinen 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 . No functional change intended. > > > > Signed-off-by: Andrey Smirnov > > Cc: Archit Taneja > > Cc: Andrzej Hajda > > Cc: Laurent Pinchart > > Cc: Tomi Valkeinen > > Cc: Andrey Gusakov > > Cc: Philipp Zabel > > Cc: Chris Healy > > Cc: Lucas Stach > > Cc: dri-devel@lists.freedesktop.org > > Cc: linux-ker...@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 > > #include > > #include > > #include > > @@ -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 VFUENBIT(0) /* Video Frame > > Timing Upload */ > > > > @@ -100,14 +110,28 @@ > > /* Main Channel */ > > #define DP0_SECSAMPLE0x0640 > > #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_ACTIVEVAL0x0650 > > +#define H_ACTGENMASK(15, 0) > > +#define V_ACTGENMASK(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_SYMBOLGENMASK(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; > &g
[PATCH v2 06/15] drm/bridge: tc358767: Simplify AUX data read
Simplify AUX data read by removing index arithmetic and shifting with
a helper functions that does three things:
1. Fetch data from up to 4 32-bit registers from the chip
2. Optionally fix data endianness (not needed on LE hosts)
3. Copy read data into user provided array.
Signed-off-by: Andrey Smirnov
---
drivers/gpu/drm/bridge/tc358767.c | 39 ---
1 file changed, 26 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/bridge/tc358767.c
b/drivers/gpu/drm/bridge/tc358767.c
index 060e4b05095a..81c10a5d8106 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -307,6 +307,28 @@ static int tc_aux_get_status(struct tc_data *tc, u8 *reply)
return 0;
}
+static int tc_aux_read_data(struct tc_data *tc, void *data, size_t size)
+{
+ u32 auxrdata[DP_AUX_MAX_PAYLOAD_BYTES / sizeof(u32)];
+ int ret, i, count = DIV_ROUND_UP(size, 4);
+
+ for (i = 0; i < count; i++) {
+ ret = regmap_read(tc->regmap, DP0_AUXRDATA(i), &auxrdata[i]);
+ if (ret)
+ return ret;
+ /*
+* Our regmap is configured as LE for register data,
+* so we need undo any byte swapping that might have
+* happened to preserve original byte order.
+*/
+ le32_to_cpus(&auxrdata[i]);
+ }
+
+ memcpy(data, auxrdata, size);
+
+ return size;
+}
+
static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
struct drm_dp_aux_msg *msg)
{
@@ -365,19 +387,10 @@ static ssize_t tc_aux_transfer(struct drm_dp_aux *aux,
if (ret)
return ret;
- if (request == DP_AUX_I2C_READ || request == DP_AUX_NATIVE_READ) {
- /* Read data */
- while (i < size) {
- if ((i % 4) == 0) {
- ret = regmap_read(tc->regmap,
- DP0_AUXRDATA(i >> 2), &tmp);
- if (ret)
- return ret;
- }
- buf[i] = tmp & 0xff;
- tmp = tmp >> 8;
- i++;
- }
+ switch (request) {
+ case DP_AUX_NATIVE_READ:
+ case DP_AUX_I2C_READ:
+ return tc_aux_read_data(tc, msg->buffer, size);
}
return size;
--
2.20.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
