[EXT] Re: [PATCH v3 2/2] phy: freescale: add Samsung HDMI PHY

[Sandor Yu]

Hi Luca, 

> 
> Hi Lucas,
> 
> [+Cc: Sandor]
> 
> On Wed,  6 Sep 2023 20:42:11 +0200
> Lucas Stach <l.stach at pengutronix.de> wrote:
> 
> > This adds the driver for the Samsung HDMI PHY found on the i.MX8MP
> > SoC. Based on downstream implementation from Sandor Yu
> > <Sandor.yu at nxp.com>.
> >
> > Tested-by: Luca Ceresoli <luca.ceresoli at bootlin.com> (v2)
> 
> Also for v3:
> [On custom board based on MSC SM2S-IMX8PLUS SMARC module]
> Tested-by: Luca Ceresoli <luca.ceresoli at bootlin.com>
> 
> I have a few notes however, see below.
> 
> > +#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).

> > +#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.

B,R
Sandor
> 
> Otherwise LGTM.
> 
> Luca
> 
> --
> Luca Ceresoli, Bootlin
> Embedded Linux and Kernel engineering
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbootlin
> .com%2F&data=05%7C01%7CSandor.yu%40nxp.com%7Cbf51f9e57dc04a2bc
> ac808dbb567d4d0%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C
> 638303229786923737%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAw
> MDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C
> %7C&sdata=c6qIGRnNn8zzx5drqE%2BG%2FQjG3XkECsXgsdN97acXIIU%3D&
> reserved=0