Hi Luca, > > Hi Lucas, > > [+Cc: Sandor] > > On Wed, 6 Sep 2023 20:42:11 +0200 > Lucas Stach <l.stach@xxxxxxxxxxxxxx> 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@xxxxxxx>. > > > > Tested-by: Luca Ceresoli <luca.ceresoli@xxxxxxxxxxx> (v2) > > Also for v3: > [On custom board based on MSC SM2S-IMX8PLUS SMARC module] > Tested-by: Luca Ceresoli <luca.ceresoli@xxxxxxxxxxx> > > 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
