[PATCH v2] drm/i915/gvt: Make correct handling to vreg BXT_PHY_CTL_FAMILY

[Colin Xu]

On 8/20/18 11:54 AM, He, Min wrote:
>
>
> On 8/15/2018 1:34 PM, Colin Xu wrote:
>> Guest kernel will write to BXT_PHY_CTL_FAMILY to reset DDI PHY
>> and pull BXT_PHY_CTL to check PHY status. Previous handling will
>> set/reset BXT_PHY_CTL of all PHYs at same time on receiving vreg
>> write to some BXT_PHY_CTL_FAMILY. If some BXT_PHY_CTL is already
>> enabled, following reset to another BXT_PHY_CTL_FAMILY will clear
>> the enabled BXT_PHY_CTL, which result in guest kernel print:
>>
>> -----------------------------------
>> [drm:intel_ddi_get_hw_state [i915]]
>> *ERROR* Port B enabled but PHY powered down? (PHY_CTL 00000000)
>> -----------------------------------
>>
>> The correct handling should operate BXT_PHY_CTL_FAMILY and
>> BXT_PHY_CTL on the same DDI.
>>
>> v2: Use correct reg define. The naming looks confusing, however
>>      current i915_reg.h bind DPIO_PHY0 to _PHY_CTL_FAMILY_DDI and
>>      bind DPIO_PHY1 to _PHY_CTL_FAMILY_EDP, pairing to
>>      _BXT_PHY_CTL_DDI_A and _BXT_PHY_CTL_DDI_B respectively.
>>
>> Signed-off-by: Colin Xu <colin.xu at intel.com>
>> ---
>>   drivers/gpu/drm/i915/gvt/handlers.c | 16 +++++++++++++---
>>   1 file changed, 13 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gvt/handlers.c 
>> b/drivers/gpu/drm/i915/gvt/handlers.c
>> index b47986a22332..cf2a4020949d 100644
>> --- a/drivers/gpu/drm/i915/gvt/handlers.c
>> +++ b/drivers/gpu/drm/i915/gvt/handlers.c
>> @@ -1526,9 +1526,17 @@ static int bxt_phy_ctl_family_write(struct 
>> intel_vgpu *vgpu,
>>       u32 v = *(u32 *)p_data;
>>       u32 data = v & COMMON_RESET_DIS ? BXT_PHY_LANE_ENABLED : 0;
>>   -    vgpu_vreg(vgpu, _BXT_PHY_CTL_DDI_A) = data;
>> -    vgpu_vreg(vgpu, _BXT_PHY_CTL_DDI_B) = data;
>> -    vgpu_vreg(vgpu, _BXT_PHY_CTL_DDI_C) = data;
>> +    switch (offset) {
>> +    case _PHY_CTL_FAMILY_DDI:
>> +        vgpu_vreg(vgpu, _BXT_PHY_CTL_DDI_A) = data;
>> +        break;
>> +    case _PHY_CTL_FAMILY_EDP:
>> +        vgpu_vreg(vgpu, _BXT_PHY_CTL_DDI_B) = data;
>> +        break;
>> +    case _PHY_CTL_FAMILY_DDI_C:
>> +        vgpu_vreg(vgpu, _BXT_PHY_CTL_DDI_C) = data;
> In bxt_ddi_phy_info and related comments, port B and C shared the same
> _PHY_CTL_FAMILY_DDI, so when enabling _PHY_CTL_FAMILY_DDI,both of DDI_B
>  and DDI_C should be enabled, and _PHY_CTL_FAMILY_EDP should be DDI_A. 

Thanks for the correction. I made a mistake that BXT_PHY_CTL() looks up DDI from port, but not DPIO_PHYx.
I'll update the patch to correct vreg mapping.

Yes,  port B & C share the same PHY. To summary, the relationship is defined as: (bxt_power_wells)

dpio-common-a:
     >>> DPIO_PHY1
     >>> BXT_DPIO_CMN_A_POWER_DOMAINS
     >>> POWER_DOMAIN_PORT_DDI_A_LANES
     >>> PORT_A

dpio-common-bc:
     >>> DPIO_PHY0
     >>> BXT_DPIO_CMN_BC_POWER_DOMAINS
     >>> POWER_DOMAIN_PORT_DDI_B_LANES | POWER_DOMAIN_PORT_DDI_C_LANES
     >>> PORT_B or PORT_C

Marco BXT_PHY_CTL_FAMILY and BXT_PHY_CTL are defined as: (i915_reg.h)
#define BXT_PHY_CTL_FAMILY(phy)	_MMIO_PHY3((phy), _PHY_CTL_FAMILY_DDI, \
	_PHY_CTL_FAMILY_EDP, \
	_PHY_CTL_FAMILY_DDI_C)		
#define BXT_PHY_CTL(port)	_MMIO_PORT(port, _BXT_PHY_CTL_DDI_A, \
	_BXT_PHY_CTL_DDI_B)

So when  _bxt_ddi_phy_init() calls on DPIO_PHY1 and DPIO_PHY0, it actually programs:
DPIO_PHY1 >>> _PHY_CTL_FAMILY_EDP
DPIO_PHY0 >>> _PHY_CTL_FAMILY_DDI

Later when intel_ddi_get_hw_state() poll status from PORT_x, it actually reads:
PORT_A >>>  _BXT_PHY_CTL_DDI_A
PORT_B >>>  _BXT_PHY_CTL_DDI_B
PORT_C >>>  _BXT_PHY_CTL_DDI_C

Thus, reset vreg _PHY_CTL_FAMILY_EDP should trigger setting _BXT_PHY_CTL_DDI_A,
and reset vreg  _PHY_CTL_FAMILY_DDI should trigger setting both _BXT_PHY_CTL_DDI_B
and _BXT_PHY_CTL_DDI_C.

>> +        break;
>> +    }
>>         vgpu_vreg(vgpu, offset) = v;
>>   @@ -3058,6 +3066,8 @@ static int init_bxt_mmio_info(struct 
>> intel_gvt *gvt)
>>           NULL, bxt_phy_ctl_family_write);
>>       MMIO_DH(BXT_PHY_CTL_FAMILY(DPIO_PHY1), D_BXT,
>>           NULL, bxt_phy_ctl_family_write);
>> +    MMIO_DH(BXT_PHY_CTL_FAMILY(DPIO_PHY2), D_BXT,
>> +        NULL, bxt_phy_ctl_family_write);
> Don't need to add it, BXT doesn't support BXT_PHY_CTL_FAMILY(DPIO_PHY2).

Yes it looks so. I'll remove BXT_PHY_CTL_FAMILY(DPIO_PHY2) handling.

>
>>       MMIO_D(BXT_PHY_CTL(PORT_A), D_BXT);
>>       MMIO_D(BXT_PHY_CTL(PORT_B), D_BXT);
>>       MMIO_D(BXT_PHY_CTL(PORT_C), D_BXT);
>
-- 
Best Regards,
Colin Xu