Re: [Intel-gfx] [PATCH v5 02/13] drm/i915/icl: DSI vswing programming sequence
On Wed, 12 Sep 2018, Madhav Chauhan wrote:
> On 9/12/2018 12:20 AM, Jani Nikula wrote:
>> On Tue, 10 Jul 2018, Madhav Chauhan wrote:
>>> This patch setup voltage swing before enabling
>>> combo PHY DDI (shared with DSI).
>>> Note that DSI voltage swing programming is for
>>> high speed data buffers. HW automatically handles
>>> the voltage swing for the low power data buffers.
>>>
>>> v2: Rebase
>>>
>>> Signed-off-by: Madhav Chauhan
>>> ---
>>> drivers/gpu/drm/i915/icl_dsi.c | 114
>>> +
>>> 1 file changed, 114 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/icl_dsi.c b/drivers/gpu/drm/i915/icl_dsi.c
>>> index a571339..dc16c1f 100644
>>> --- a/drivers/gpu/drm/i915/icl_dsi.c
>>> +++ b/drivers/gpu/drm/i915/icl_dsi.c
>>> @@ -27,6 +27,65 @@
>>>
>>> #include "intel_dsi.h"
>>>
>>> +static void dsi_program_swing_and_deemphasis(struct intel_encoder *encoder)
>>> +{
>>> + struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>>> + struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
>>> + enum port port;
>>> + u32 tmp;
>>> + int lane;
>>> +
>>> + for_each_dsi_port(port, intel_dsi->ports) {
>>> +
>>> + /* Bspec: set scaling mode to 0x6 */
>> Today bspec says 2. Also, please don't duplicate the value in the
>> comment.
>
> Right..thanks for catching :)
>
>>
>>> + tmp = I915_READ(ICL_PORT_TX_DW5_LN0(port));
>>> + tmp |= SCALING_MODE_SEL(6);
>>> + I915_WRITE(ICL_PORT_TX_DW5_GRP(port), tmp);
>> Like Ville said, adding a blank line between each read-modify-write
>> group helps readability. Perhaps add /* DW5 */ etc. comments to group
>> the, eh, groups.
>
> Ok.
>
>>
>>> + tmp = I915_READ(ICL_PORT_TX_DW5_AUX(port));
>>> + tmp |= SCALING_MODE_SEL(6);
>>> + I915_WRITE(ICL_PORT_TX_DW5_AUX(port), tmp);
>>> + tmp = I915_READ(ICL_PORT_TX_DW5_LN0(port));
>>> + tmp |= TAP2_DISABLE | TAP3_DISABLE;
>>> + I915_WRITE(ICL_PORT_TX_DW5_GRP(port), tmp);
>>> + tmp = I915_READ(ICL_PORT_TX_DW5_AUX(port));
>>> + tmp |= TAP2_DISABLE | TAP3_DISABLE;
>>> + I915_WRITE(ICL_PORT_TX_DW5_AUX(port), tmp);
>> Are you missing RTERM_SELECT?
>
> Looks this was not earlier and added recently. Will program in next version.
>
>>
>> Why do you do two read-modify-writes (RMW) on both GRP and AUX, instead
>> of doing all the changes at once?
>
> Do you mean for tmp |= TAP2_DISABLE | TAP3_DISABLE ?? If yes, because
> GRP and AUX
> might contain different values and need to read them explicitly.
No, I mean first you RMW scaling mode for GRP and AUX, then you do
TAP2/3 disable for GRP and AUX. Why not scaling mode *and* TAP2/3
disable in one go, for GRP and AUX separately of course.
>
>>
>> The RMW doesn't actually clear the fields before changing them, just ORs
>> more stuff on top of them, and cursor program or coeff polarity might
>> contain garbage (at least in theory). The same below.
>
> Yeah, we need to reset those bits using MASK and then do 'OR'.
Yes.
> Or are you suggesting something else??
No, that's just it.
BR,
Jani.
>
>>
>>> +
>>> + /*
>>> +* swing and scaling values are taken from DSI
>>> +* table under vswing programming sequence for
>>> +* combo phy ddi in BSPEC.
>>> +* program swing values
>>> +*/
>> Please reflow the comment.
>
> Ok.
>
>>
>>> + tmp = I915_READ(ICL_PORT_TX_DW2_LN0(port));
>>> + tmp |= SWING_SEL_UPPER(0x2);
>>> + tmp |= SWING_SEL_LOWER(0x2);
>> This would benefit from
>>
>> +#define SWING_SEL_MASK(SWING_SEL_UPPER_MASK |
>> SWING_SEL_LOWER_MASK)
>> +#define SWING_SEL(x) (SWING_SEL_UPPER(x) |
>> SWING_SEL_LOWER(x))
>>
>> in i915_reg.h. But I can look the other way and fix it myself later...
>>
>>> + tmp |= RCOMP_SCALAR(0x98);
>>> + I915_WRITE(ICL_PORT_TX_DW2_GRP(port), tmp);
>>> + tmp = I915_READ(ICL_PORT_TX_DW2_AUX(port));
>>> + tmp |= SWING_SEL_UPPER(0x2);
>>> + tmp |= SWING_SEL_LOWER(0x2);
>>> + tmp |= RCOMP_SCALAR(0x98);
>>> + I915_WRITE(ICL_PORT_TX_DW2_AUX(port), tmp);
>>> +
>>> + /* program scaling values */
>>> + tmp = I915_READ(ICL_PORT_TX_DW4_AUX(port));
>>> + tmp |= POST_CURSOR_1(0x0);
>>> + tmp |= POST_CURSOR_2(0x0);
>>> + tmp |= CURSOR_COEFF(0x18);
>> 0x3f?
>
> Yes, now its changed to 0x3f.
>
>>
>> Again, you need to zero the fields before ORin the new values into them.
>
> Agree.
>
>>
>>> + I915_WRITE(ICL_PORT_TX_DW4_AUX(port), tmp);
>>> +
>>> + for (lane = 0; lane <= 3; lane++) {
>>> + /* Bspec: must not use GRP register for write */
>> I'll take your word for it, although I've missed such a requirement.
>
> :-)
>
>>
>>> + tmp = I915_READ(ICL_PORT_TX_DW4_LN(port, lane));
>>> +
Re: [Intel-gfx] [PATCH v5 02/13] drm/i915/icl: DSI vswing programming sequence
On 9/12/2018 12:20 AM, Jani Nikula wrote:
On Tue, 10 Jul 2018, Madhav Chauhan wrote:
This patch setup voltage swing before enabling
combo PHY DDI (shared with DSI).
Note that DSI voltage swing programming is for
high speed data buffers. HW automatically handles
the voltage swing for the low power data buffers.
v2: Rebase
Signed-off-by: Madhav Chauhan
---
drivers/gpu/drm/i915/icl_dsi.c | 114 +
1 file changed, 114 insertions(+)
diff --git a/drivers/gpu/drm/i915/icl_dsi.c b/drivers/gpu/drm/i915/icl_dsi.c
index a571339..dc16c1f 100644
--- a/drivers/gpu/drm/i915/icl_dsi.c
+++ b/drivers/gpu/drm/i915/icl_dsi.c
@@ -27,6 +27,65 @@
#include "intel_dsi.h"
+static void dsi_program_swing_and_deemphasis(struct intel_encoder *encoder)
+{
+ struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
+ struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
+ enum port port;
+ u32 tmp;
+ int lane;
+
+ for_each_dsi_port(port, intel_dsi->ports) {
+
+ /* Bspec: set scaling mode to 0x6 */
Today bspec says 2. Also, please don't duplicate the value in the
comment.
Right..thanks for catching :)
+ tmp = I915_READ(ICL_PORT_TX_DW5_LN0(port));
+ tmp |= SCALING_MODE_SEL(6);
+ I915_WRITE(ICL_PORT_TX_DW5_GRP(port), tmp);
Like Ville said, adding a blank line between each read-modify-write
group helps readability. Perhaps add /* DW5 */ etc. comments to group
the, eh, groups.
Ok.
+ tmp = I915_READ(ICL_PORT_TX_DW5_AUX(port));
+ tmp |= SCALING_MODE_SEL(6);
+ I915_WRITE(ICL_PORT_TX_DW5_AUX(port), tmp);
+ tmp = I915_READ(ICL_PORT_TX_DW5_LN0(port));
+ tmp |= TAP2_DISABLE | TAP3_DISABLE;
+ I915_WRITE(ICL_PORT_TX_DW5_GRP(port), tmp);
+ tmp = I915_READ(ICL_PORT_TX_DW5_AUX(port));
+ tmp |= TAP2_DISABLE | TAP3_DISABLE;
+ I915_WRITE(ICL_PORT_TX_DW5_AUX(port), tmp);
Are you missing RTERM_SELECT?
Looks this was not earlier and added recently. Will program in next version.
Why do you do two read-modify-writes (RMW) on both GRP and AUX, instead
of doing all the changes at once?
Do you mean for tmp |= TAP2_DISABLE | TAP3_DISABLE ?? If yes, because
GRP and AUX
might contain different values and need to read them explicitly.
The RMW doesn't actually clear the fields before changing them, just ORs
more stuff on top of them, and cursor program or coeff polarity might
contain garbage (at least in theory). The same below.
Yeah, we need to reset those bits using MASK and then do 'OR'.
Or are you suggesting something else??
+
+ /*
+* swing and scaling values are taken from DSI
+* table under vswing programming sequence for
+* combo phy ddi in BSPEC.
+* program swing values
+*/
Please reflow the comment.
Ok.
+ tmp = I915_READ(ICL_PORT_TX_DW2_LN0(port));
+ tmp |= SWING_SEL_UPPER(0x2);
+ tmp |= SWING_SEL_LOWER(0x2);
This would benefit from
+#define SWING_SEL_MASK (SWING_SEL_UPPER_MASK |
SWING_SEL_LOWER_MASK)
+#define SWING_SEL(x) (SWING_SEL_UPPER(x) |
SWING_SEL_LOWER(x))
in i915_reg.h. But I can look the other way and fix it myself later...
+ tmp |= RCOMP_SCALAR(0x98);
+ I915_WRITE(ICL_PORT_TX_DW2_GRP(port), tmp);
+ tmp = I915_READ(ICL_PORT_TX_DW2_AUX(port));
+ tmp |= SWING_SEL_UPPER(0x2);
+ tmp |= SWING_SEL_LOWER(0x2);
+ tmp |= RCOMP_SCALAR(0x98);
+ I915_WRITE(ICL_PORT_TX_DW2_AUX(port), tmp);
+
+ /* program scaling values */
+ tmp = I915_READ(ICL_PORT_TX_DW4_AUX(port));
+ tmp |= POST_CURSOR_1(0x0);
+ tmp |= POST_CURSOR_2(0x0);
+ tmp |= CURSOR_COEFF(0x18);
0x3f?
Yes, now its changed to 0x3f.
Again, you need to zero the fields before ORin the new values into them.
Agree.
+ I915_WRITE(ICL_PORT_TX_DW4_AUX(port), tmp);
+
+ for (lane = 0; lane <= 3; lane++) {
+ /* Bspec: must not use GRP register for write */
I'll take your word for it, although I've missed such a requirement.
:-)
+ tmp = I915_READ(ICL_PORT_TX_DW4_LN(port, lane));
+ tmp |= POST_CURSOR_1(0x0);
+ tmp |= POST_CURSOR_2(0x0);
+ tmp |= CURSOR_COEFF(0x18);
0x3f?
Yes.
+ I915_WRITE(ICL_PORT_TX_DW4_LN(port, lane), tmp);
+ }
+ }
+}
+
static void gen11_dsi_program_esc_clk_div(struct intel_encoder *encoder)
{
struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
@@ -140,6 +199,58 @@ static void gen11_dsi_config_phy_lanes_sequenc
Re: [Intel-gfx] [PATCH v5 02/13] drm/i915/icl: DSI vswing programming sequence
On Tue, 10 Jul 2018, Madhav Chauhan wrote:
> This patch setup voltage swing before enabling
> combo PHY DDI (shared with DSI).
> Note that DSI voltage swing programming is for
> high speed data buffers. HW automatically handles
> the voltage swing for the low power data buffers.
>
> v2: Rebase
>
> Signed-off-by: Madhav Chauhan
> ---
> drivers/gpu/drm/i915/icl_dsi.c | 114
> +
> 1 file changed, 114 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/icl_dsi.c b/drivers/gpu/drm/i915/icl_dsi.c
> index a571339..dc16c1f 100644
> --- a/drivers/gpu/drm/i915/icl_dsi.c
> +++ b/drivers/gpu/drm/i915/icl_dsi.c
> @@ -27,6 +27,65 @@
>
> #include "intel_dsi.h"
>
> +static void dsi_program_swing_and_deemphasis(struct intel_encoder *encoder)
> +{
> + struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> + struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
> + enum port port;
> + u32 tmp;
> + int lane;
> +
> + for_each_dsi_port(port, intel_dsi->ports) {
> +
> + /* Bspec: set scaling mode to 0x6 */
Today bspec says 2. Also, please don't duplicate the value in the
comment.
> + tmp = I915_READ(ICL_PORT_TX_DW5_LN0(port));
> + tmp |= SCALING_MODE_SEL(6);
> + I915_WRITE(ICL_PORT_TX_DW5_GRP(port), tmp);
Like Ville said, adding a blank line between each read-modify-write
group helps readability. Perhaps add /* DW5 */ etc. comments to group
the, eh, groups.
> + tmp = I915_READ(ICL_PORT_TX_DW5_AUX(port));
> + tmp |= SCALING_MODE_SEL(6);
> + I915_WRITE(ICL_PORT_TX_DW5_AUX(port), tmp);
> + tmp = I915_READ(ICL_PORT_TX_DW5_LN0(port));
> + tmp |= TAP2_DISABLE | TAP3_DISABLE;
> + I915_WRITE(ICL_PORT_TX_DW5_GRP(port), tmp);
> + tmp = I915_READ(ICL_PORT_TX_DW5_AUX(port));
> + tmp |= TAP2_DISABLE | TAP3_DISABLE;
> + I915_WRITE(ICL_PORT_TX_DW5_AUX(port), tmp);
Are you missing RTERM_SELECT?
Why do you do two read-modify-writes (RMW) on both GRP and AUX, instead
of doing all the changes at once?
The RMW doesn't actually clear the fields before changing them, just ORs
more stuff on top of them, and cursor program or coeff polarity might
contain garbage (at least in theory). The same below.
> +
> + /*
> + * swing and scaling values are taken from DSI
> + * table under vswing programming sequence for
> + * combo phy ddi in BSPEC.
> + * program swing values
> + */
Please reflow the comment.
> + tmp = I915_READ(ICL_PORT_TX_DW2_LN0(port));
> + tmp |= SWING_SEL_UPPER(0x2);
> + tmp |= SWING_SEL_LOWER(0x2);
This would benefit from
+#define SWING_SEL_MASK (SWING_SEL_UPPER_MASK |
SWING_SEL_LOWER_MASK)
+#define SWING_SEL(x) (SWING_SEL_UPPER(x) |
SWING_SEL_LOWER(x))
in i915_reg.h. But I can look the other way and fix it myself later...
> + tmp |= RCOMP_SCALAR(0x98);
> + I915_WRITE(ICL_PORT_TX_DW2_GRP(port), tmp);
> + tmp = I915_READ(ICL_PORT_TX_DW2_AUX(port));
> + tmp |= SWING_SEL_UPPER(0x2);
> + tmp |= SWING_SEL_LOWER(0x2);
> + tmp |= RCOMP_SCALAR(0x98);
> + I915_WRITE(ICL_PORT_TX_DW2_AUX(port), tmp);
> +
> + /* program scaling values */
> + tmp = I915_READ(ICL_PORT_TX_DW4_AUX(port));
> + tmp |= POST_CURSOR_1(0x0);
> + tmp |= POST_CURSOR_2(0x0);
> + tmp |= CURSOR_COEFF(0x18);
0x3f?
Again, you need to zero the fields before ORin the new values into them.
> + I915_WRITE(ICL_PORT_TX_DW4_AUX(port), tmp);
> +
> + for (lane = 0; lane <= 3; lane++) {
> + /* Bspec: must not use GRP register for write */
I'll take your word for it, although I've missed such a requirement.
> + tmp = I915_READ(ICL_PORT_TX_DW4_LN(port, lane));
> + tmp |= POST_CURSOR_1(0x0);
> + tmp |= POST_CURSOR_2(0x0);
> + tmp |= CURSOR_COEFF(0x18);
0x3f?
> + I915_WRITE(ICL_PORT_TX_DW4_LN(port, lane), tmp);
> + }
> + }
> +}
> +
> static void gen11_dsi_program_esc_clk_div(struct intel_encoder *encoder)
> {
> struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> @@ -140,6 +199,58 @@ static void gen11_dsi_config_phy_lanes_sequence(struct
> intel_encoder *encoder)
> }
> }
>
> +static void gen11_dsi_voltage_swing_program_seq(struct intel_encoder
> *encoder)
> +{
> + struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> + struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
> + u32 tmp;
> + enum port port;
> +
The step numbering below has changed in bspec. Please update. Maybe drop
the numbering, and use just the headings.
[Intel-gfx] [PATCH v5 02/13] drm/i915/icl: DSI vswing programming sequence
This patch setup voltage swing before enabling
combo PHY DDI (shared with DSI).
Note that DSI voltage swing programming is for
high speed data buffers. HW automatically handles
the voltage swing for the low power data buffers.
v2: Rebase
Signed-off-by: Madhav Chauhan
---
drivers/gpu/drm/i915/icl_dsi.c | 114 +
1 file changed, 114 insertions(+)
diff --git a/drivers/gpu/drm/i915/icl_dsi.c b/drivers/gpu/drm/i915/icl_dsi.c
index a571339..dc16c1f 100644
--- a/drivers/gpu/drm/i915/icl_dsi.c
+++ b/drivers/gpu/drm/i915/icl_dsi.c
@@ -27,6 +27,65 @@
#include "intel_dsi.h"
+static void dsi_program_swing_and_deemphasis(struct intel_encoder *encoder)
+{
+ struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
+ struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
+ enum port port;
+ u32 tmp;
+ int lane;
+
+ for_each_dsi_port(port, intel_dsi->ports) {
+
+ /* Bspec: set scaling mode to 0x6 */
+ tmp = I915_READ(ICL_PORT_TX_DW5_LN0(port));
+ tmp |= SCALING_MODE_SEL(6);
+ I915_WRITE(ICL_PORT_TX_DW5_GRP(port), tmp);
+ tmp = I915_READ(ICL_PORT_TX_DW5_AUX(port));
+ tmp |= SCALING_MODE_SEL(6);
+ I915_WRITE(ICL_PORT_TX_DW5_AUX(port), tmp);
+ tmp = I915_READ(ICL_PORT_TX_DW5_LN0(port));
+ tmp |= TAP2_DISABLE | TAP3_DISABLE;
+ I915_WRITE(ICL_PORT_TX_DW5_GRP(port), tmp);
+ tmp = I915_READ(ICL_PORT_TX_DW5_AUX(port));
+ tmp |= TAP2_DISABLE | TAP3_DISABLE;
+ I915_WRITE(ICL_PORT_TX_DW5_AUX(port), tmp);
+
+ /*
+* swing and scaling values are taken from DSI
+* table under vswing programming sequence for
+* combo phy ddi in BSPEC.
+* program swing values
+*/
+ tmp = I915_READ(ICL_PORT_TX_DW2_LN0(port));
+ tmp |= SWING_SEL_UPPER(0x2);
+ tmp |= SWING_SEL_LOWER(0x2);
+ tmp |= RCOMP_SCALAR(0x98);
+ I915_WRITE(ICL_PORT_TX_DW2_GRP(port), tmp);
+ tmp = I915_READ(ICL_PORT_TX_DW2_AUX(port));
+ tmp |= SWING_SEL_UPPER(0x2);
+ tmp |= SWING_SEL_LOWER(0x2);
+ tmp |= RCOMP_SCALAR(0x98);
+ I915_WRITE(ICL_PORT_TX_DW2_AUX(port), tmp);
+
+ /* program scaling values */
+ tmp = I915_READ(ICL_PORT_TX_DW4_AUX(port));
+ tmp |= POST_CURSOR_1(0x0);
+ tmp |= POST_CURSOR_2(0x0);
+ tmp |= CURSOR_COEFF(0x18);
+ I915_WRITE(ICL_PORT_TX_DW4_AUX(port), tmp);
+
+ for (lane = 0; lane <= 3; lane++) {
+ /* Bspec: must not use GRP register for write */
+ tmp = I915_READ(ICL_PORT_TX_DW4_LN(port, lane));
+ tmp |= POST_CURSOR_1(0x0);
+ tmp |= POST_CURSOR_2(0x0);
+ tmp |= CURSOR_COEFF(0x18);
+ I915_WRITE(ICL_PORT_TX_DW4_LN(port, lane), tmp);
+ }
+ }
+}
+
static void gen11_dsi_program_esc_clk_div(struct intel_encoder *encoder)
{
struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
@@ -140,6 +199,58 @@ static void gen11_dsi_config_phy_lanes_sequence(struct
intel_encoder *encoder)
}
}
+static void gen11_dsi_voltage_swing_program_seq(struct intel_encoder *encoder)
+{
+ struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
+ struct intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
+ u32 tmp;
+ enum port port;
+
+ /* Step C.1:clear common keeper enable bit */
+ for_each_dsi_port(port, intel_dsi->ports) {
+ tmp = I915_READ(ICL_PORT_PCS_DW1_LN0(port));
+ tmp &= ~COMMON_KEEPER_EN;
+ I915_WRITE(ICL_PORT_PCS_DW1_GRP(port), tmp);
+ tmp = I915_READ(ICL_PORT_PCS_DW1_AUX(port));
+ tmp &= ~COMMON_KEEPER_EN;
+ I915_WRITE(ICL_PORT_PCS_DW1_AUX(port), tmp);
+ }
+
+ /*
+* Step C.3: Set SUS Clock Config bitfield to 11b
+* Note: Step C.2 (loadgen select program) is done
+* as part of lane phy sequence configuration
+*/
+ for_each_dsi_port(port, intel_dsi->ports) {
+ tmp = I915_READ(ICL_PORT_CL_DW5(port));
+ tmp |= SUS_CLOCK_CONFIG;
+ I915_WRITE(ICL_PORT_CL_DW5(port), tmp);
+ }
+
+ /* Step C.4: Clear training enable to change swing values */
+ for_each_dsi_port(port, intel_dsi->ports) {
+ tmp = I915_READ(ICL_PORT_TX_DW5_LN0(port));
+ tmp &= ~TX_TRAINING_EN;
+ I915_WRITE(ICL_PORT_TX_DW5_GRP(port), tmp);
+ tmp = I915_READ(ICL_PORT_TX_DW5_AUX(port));
+ tmp &= ~TX_TRAINING_EN;
+ I915_WRITE(ICL_P
