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

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

> -----Original Message-----
> From: linux-samsung-soc-owner@vger.kernel.org [mailto:linux-samsung-soc-
> owner@vger.kernel.org] On Behalf Of Rahul Sharma
> Sent: Thursday, September 05, 2013 3:04 PM
> To: Inki Dae
> Cc: Sean Paul; Rahul Sharma; linux-samsung-soc; dri-devel; kgene.kim;
> sw0312.kim; Lucas Stach; Tomasz Figa; Sylwester Nawrocki; sunil joshi;
> Shirish S
> Subject: Re: [PATCH 1/7] drm/exynos: move hdmiphy related code to hdmiphy
> driver
> 
> On 5 September 2013 10:52, Inki Dae <inki.dae@samsung.com> wrote:
> >> >> >> >> +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?
> >
> 
> Sorry, I had only seen his patches for chromium tree. In mainline
> version, he added for 5250 as well. But both sets (for arndale and
> smdk) are exactly same as original 5250 configs which also works
> with 4412 origen.
> 
> Some problem has been identified during conformance testing for
> 5420 peach board, which happens with analyser. It was always
> working fine on the TV sets that I have. @Shirish/Sean please
> correct me if wrong.
> 
> >> 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?
> >
> 
> Yes. I meant same. I don't see the real need to duplicate so much
> of data in all board dts files. We can add it for a particular board, if
> really required.
> 

Yes, reasonable to me. It's not good that board dts files have same phy
config data. How about using the phy config data from dts file if
hdmiphy-confs property exists, otherwise using default phy config data then?
This means that we don't need to remove the phy config data from driver;
that will be used as default values.

Rahul, let's go if there is other opinion. we SHOULD MERGE these this
time.:)

Thanks,
Inki Dae

> Regards,
> Rahul Sharma.
> 
> >
> >>
> >> regards,
> >> Rahul Sharma.
> >>
> >> >> Sean
> >> >>
> >> >>
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-
> soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html