Hello Sandor, Lucas,

On Fri, 15 Sep 2023 01:49:42 +0000
Sandor Yu [off-list ref] wrote:

Hi Luca, 

quoted

Hi Lucas,

[+Cc: Sandor]

On Wed,  6 Sep 2023 20:42:11 +0200
Lucas Stach [off-list ref] wrote:
  

quoted

This adds the driver for the Samsung HDMI PHY found on the i.MX8MP
SoC. Based on downstream implementation from Sandor Yu
[off-list ref].

Tested-by: Luca Ceresoli <luca.ceresoli@bootlin.com> (v2)  

Also for v3:
[On custom board based on MSC SM2S-IMX8PLUS SMARC module]
Tested-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

I have a few notes however, see below.
  

quoted

+#define PHY_REG_14           0x38
+#define  REG14_TOL_MASK              GENMASK(7, 4)
+#define  REG14_RP_CODE_MASK  GENMASK(2, 1)  

According to the latest reference manual currently available on the NXP
website (Rev. 1, 06/2021), this should be GENMASK(3, 1). This is somewhat
nitpicking as the only possible value documented is 2. But let's continue...
  

I agree, according the RM it should be GENMASK(3, 1).

quoted

quoted

+#define PHY_REG_33           0x84
+#define  REG33_MODE_SET_DONE BIT(7)
+#define  REG33_FIX_DA                BIT(1)  

Here the reference manual is very different:

MODE_SET_DONE BIT(4)
TX_INV2       BIT(3)
TX_INV1       BIT(2)
TX_INV0       BIT(1)
MON_RXD       BIT(0)
bits 7-5 are reserved

...which is strange: in the code you are always writing 0 in bit 4, which
according to the docs means MODE_SET_DONE is always "Assert forced
global reset". Thus I guess your definitions come from the downstream driver
which, as it sadly happens, is more authoritative than the docs. :-/

Sandor, can you confirm this, or provide any clarifications?  

There is a doc issue on the i.MX8MP latest RM, actually the fields for REG33 should be:
REG33 fields:
--------------------------------------------------------------------------------
|    Field	   |       Description
--------------------------------------------------------------------------------
|     7         | 0 - Assert forced global reset
|MODE_SET_DONE | 1 - Release forced global reset             
--------------------------------------------------------------------------------
|     6        | 0 - There are not any change in 20bit data from TXD2 port     
|TX_INV2	       | 1 Invert polarity of 10bit data of 20bit data from TXD2 port
--------------------------------------------------------------------------------
|     5        | 0 - There are not any change in 20bit data from TXD1 port     
|TX_INV1       | 1 - Invert polarity of 10bit data of 20bit data from TXD1 port
--------------------------------------------------------------------------------
|     4        | 0 - There are not any change in 20bit data from TXD0 port      
|TX_INV0       | 1 - Invert polarity of 10bit data of 20bit data from TXD0 port 
--------------------------------------------------------------------------------
|    3         | 0 : TESTOUT[0] - BIST_ON                                    
|MON_RXD     | TESTOUT[1] - BIST_ERROR                                     
|              | TESTOUT[2] - DES_CLK_SEL[0]                                 
|              | TESTOUT[3] - DES_CLK_SEL[1]                                 
|              | TESTOUT[4] - TX_CLK_SEL[0]                                  
|              | TESTOUT[5] - TX_CLK_SEL[1]                                  
|              | TESTOUT[6] - TX_CLK_SEL[2]                                  
|              | TESTOUT[7] - TX_CLK_SEL[3]                                  
|              | TESTOUT[8] - PLL_COARSE_LOCK_DONE                           
|              | TESTOUT[9] - PLL_FINE_LOCK_DONE                             
|              | TESTOUT[10] - Divided by 10 clock (source clock : PIXEL_CLK)
|              | TESTOUT[11] - Divided by 10 clock (source clock : TMDS_CLK) 
|              | 1 : TESTOUT[0:9] - 10bit of RXDATA                          
|              | TESTOUT[10] - Divided by 10 clock (source clock : PIXEL_CLK)
|              | TESTOUT[11] - Divided by 10 clock (source clock : TMDS_CLK) 
--------------------------------------------------------------------------------
|    2         |
|FIX_DB        | Disable de-skew function 1 - Select fixed phase 2
--------------------------------------------------------------------------------
|    1         |
|FIX_DA        | Disable de-skew function 1 - Select fixed phase 1
--------------------------------------------------------------------------------
|    0         | 0 : There are not any change in 20bit data from TESTIN port    
|ALTER         | 1: Invert polarity of 10bit data of 20bit data from TESTIN port
--------------------------------------------------------------------------------

The issue will be fixed in the next i.MX8MP RM release, sorry for the inconvenience.

Thanks for the prompt clarification!

I'm not sure whether this should deserve a comment such as "/*
Reference manual Rev. 1, 06/2021 has incorrect documentation of this
register */". It depends on the time the fixed RM will take before it's
public, so up to you Lucas.

Bottom line, I think only the PHY_REG_14 fix is needed to this patch.

Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel