RE: [PATCH 1/7] drm/exynos: move hdmiphy related code to hdmiphy driver

[Inki Dae <inki.dae@samsung.com>]

> >> >> >> +static struct hdmiphy_config hdmiphy_4210_configs[] = {
> >> >> >> +       {
> >> >> >> +               .pixel_clock = 27000000,
> >> >> >> +               .conf = {
> >> >> >> +                       0x01, 0x05, 0x00, 0xD8, 0x10, 0x1C, 0x30,
> > 0x40,
> >> >> >> +                       0x6B, 0x10, 0x02, 0x51, 0xDF, 0xF2, 0x54,
> > 0x87,
> >> >> >> +                       0x84, 0x00, 0x30, 0x38, 0x00, 0x08, 0x10,
> > 0xE0,
> >> >> >> +                       0x22, 0x40, 0xE3, 0x26, 0x00, 0x00, 0x00,
> > 0x00,
> >> >> >> +               },
> >> >> >> +       },
> >> >> >> +       {
> >> >> >> +               .pixel_clock = 27027000,
> >> >> >> +               .conf = {
> >> >> >> +                       0x01, 0x05, 0x00, 0xD4, 0x10, 0x9C, 0x09,
> > 0x64,
> >> >> >> +                       0x6B, 0x10, 0x02, 0x51, 0xDF, 0xF2, 0x54,
> > 0x87,
> >> >> >> +                       0x84, 0x00, 0x30, 0x38, 0x00, 0x08, 0x10,
> > 0xE0,
> >> >> >> +                       0x22, 0x40, 0xE3, 0x26, 0x00, 0x00, 0x00,
> > 0x00,
> >> >> >> +               },
> >> >> >> +       },
> >> >> >> +       {
> >> >> >> +               .pixel_clock = 74176000,
> >> >> >> +               .conf = {
> >> >> >> +                       0x01, 0x05, 0x00, 0xD8, 0x10, 0x9C, 0xef,
> > 0x5B,
> >> >> >> +                       0x6D, 0x10, 0x01, 0x51, 0xef, 0xF3, 0x54,
> > 0xb9,
> >> >> >> +                       0x84, 0x00, 0x30, 0x38, 0x00, 0x08, 0x10,
> > 0xE0,
> >> >> >> +                       0x22, 0x40, 0xa5, 0x26, 0x01, 0x00, 0x00,
> > 0x00,
> >> >> >> +               },
> >> >> >> +       },
> >> >> >> +       {
> >> >> >> +               .pixel_clock = 74250000,
> >> >> >> +               .conf = {
> >> >> >> +                       0x01, 0x05, 0x00, 0xd8, 0x10, 0x9c, 0xf8,
> > 0x40,
> >> >> >> +                       0x6a, 0x10, 0x01, 0x51, 0xff, 0xf1, 0x54,
> > 0xba,
> >> >> >> +                       0x84, 0x00, 0x10, 0x38, 0x00, 0x08, 0x10,
> > 0xe0,
> >> >> >> +                       0x22, 0x40, 0xa4, 0x26, 0x01, 0x00, 0x00,
> > 0x00,
> >> >> >> +               },
> >> >> >> +       },
> >> >> >> +       {
> >> >> >> +               .pixel_clock = 148500000,
> >> >> >> +               .conf = {
> >> >> >> +                       0x01, 0x05, 0x00, 0xD8, 0x10, 0x9C, 0xf8,
> > 0x40,
> >> >> >> +                       0x6A, 0x18, 0x00, 0x51, 0xff, 0xF1, 0x54,
> > 0xba,
> >> >> >> +                       0x84, 0x00, 0x10, 0x38, 0x00, 0x08, 0x10,
> > 0xE0,
> >> >> >> +                       0x22, 0x40, 0xa4, 0x26, 0x02, 0x00, 0x00,
> > 0x00,
> >> >> >> +               },
> >> >> >> +       },
> >> >> >> +};
> >> >> >> +
> >> >> >
> >> >> > Are you aware of the effort to move these to dt? Since these are
> >> >> > board-specific values, it seems incorrect to apply them
> universally.
> >> >> > Shirish has uploaded a patch to the chromium review site to push
> >> these
> >> >> > into dt (https://chromium-review.googlesource.com/#/c/65581).
> Maybe
> >> >> > you can work that into your patch set?
> >> >> >
> >> >
> >> > Are these really board-specific values?
> >>
> >> According to your hardware people:
> >>
> >> "If the signal pattern doesn't change for new board, the phy setting
> >> is same as the current board. But if changed, you need to confirm with
> >> measurement of signals, because it may vary slightly by resistance of
> >> board pattern"
> >>
> >
> > Right. it seems that the phy configuration should be adjusted according
> to
> > PCB environment: OSC clock rate, 24MHz or 27MHz, could be decided by PCB
> > even though most PCBs use 27MHz.
> >
> >> That indicates to me that we might need to tweak these on a per-board
> >> basis.
> >>
> >> In the 5420 datasheet, there are a few register descriptions available
> >> for the phy. 0x145D0004 is CLK_SEL which seems like it would be
> >> board-specific, same with TX_* registers.
> >>
> >
> > And we can select HDMI Tx PHY internal PLL input clock by setting
> CLK_SEL.
> > Ok, Shirish's patch set is reasonable to me. However, that patch set
> should
> > be rebased on top of Rahul's patch set. Shirish and Rahul, please re-
> post
> > your patch set after discussing how to rebase these patch set.
> >
> > Thanks,
> > Inki Dae
> >
> 
> In that case, we need to test the phy confs for all the exynos boards,
> supported in
> mainline. Probably needs a analyser as well to precisely compare the
> deviation.

Shirish patch adds phy config data only to arndale and smdk5250 boards, and
these config data should have each board specific values. Therefore, for
other boards, shouldn't correct phy config data suitable to their boards be
added to their board dts files? Is the above analyzer really needed?

> Shirish patch is only for 5420 Peach board. Else, to start with we can
> mark
> phyconf as optional property which overrides the default Phy Confs for
> given SoC.

Hm.... you mean that hdmiphy driver use the default phy config data in
driver; most boards use the same data, and only in special case; a board
uses different OSC clock rate, the hdmiphy driver use phy config data from
dts file checking hdmiphy-confs property?


> 	
> regards,
> Rahul Sharma.
> 
> >> Sean
> >>
> >>