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

[Luca Ceresoli <luca.ceresoli@bootlin.com>]

Hello Sandor, Lucas,

On Fri, 15 Sep 2023 01:49:42 +0000
Sandor Yu <sandor.yu@nxp.com> wrote:

> Hi Luca, 
> 
> > 
> > Hi Lucas,
> > 
> > [+Cc: Sandor]
> > 
> > On Wed,  6 Sep 2023 20:42:11 +0200
> > Lucas Stach <l.stach@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@nxp.com>.
> > >
> > > 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.
> >   
> > > +#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.

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-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy