Hello Sandor, Lucas, On Fri, 15 Sep 2023 01:49:42 +0000 Sandor Yu <sandor.yu@xxxxxxx> wrote: > 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. 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
