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

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

[Inki Dae]

CCing devicetree,

> -----Original Message-----
> From: Rahul Sharma [mailto:r.sh.open@gmail.com]
> Sent: Tuesday, September 10, 2013 5:28 PM
> To: Sean Paul
> Cc: Inki Dae; 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 6 September 2013 19:21, Sean Paul <seanpaul@chromium.org> wrote:
> > On Thu, Sep 5, 2013 at 11:37 PM, Rahul Sharma <r.sh.open@gmail.com>
> wrote:
> >> On 5 September 2013 19:20, Inki Dae <inki.dae@samsung.com> wrote:
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: Sean Paul [mailto:seanpaul@chromium.org]
> >>>> Sent: Thursday, September 05, 2013 10:20 PM
> >>>> To: Inki Dae
> >>>> Cc: Rahul Sharma; 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 Thu, Sep 5, 2013 at 2:19 AM, Inki Dae <inki.dae@samsung.com>
wrote:
> >>>> >
> >>>> >
> >>>> >> -----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.
> >>>> >
> >>>>
> >>>> Can you add the "default" configs to exynos5250.dtsi and
> >>>> exynos5420.dtsi, then overwrite it in the board file if it needs to
> be
> >>>> different?
> >>>>
> >>
> >> This will still introduce some duplication as 4412 and 5250 share same
> >> phy confs and have no common dtsi. Similar situation can arise for
> later
> >> SoCs in exynos series.
> >>
> >>>
> >>> Good idea but how about adding default-hdmiphy-config property to each
> board
> >>> dts file and removing phy config data from board dts file if they are
> same
> >>> as default one of driver? With this, hdmiphy driver checks if
> >>> default-hdmiphy-config property exists, and then use default config
> data if
> >>> exists. And if not exists, hdmiphy driver gets and uses board specific
> phy
> >>> config data from board dts file.
> >>> And it seems that the phy config values of Shirish's patch set are
> same as
> >>> default ones of driver. So we can remove the phy config data from each
> board
> >>> dts files and add default-hdmiphy-config property to there so that
> default
> >>> data of driver can be used.
> >>>
> >>>
> >>> Thanks,
> >>> Inki Dae
> >>>
> >>
> >> We can simplify it further by letting the driver use phy-conf
> >> property from DT. If phy-conf property is not available switch to
> >> default confs, provided in the driver. This way we don't need to add
> >> default-hdmiphy-config property to all board files.
> >>
> >
> > This is probably a worthwhile discussion to have on Shirish's patch
> > with devicetree-discuss. I'm unsure which is the preferred way to do
> > something like this.
> 
> I agree.
> 
> Shall we keep those patches for "phy conf from DT" independent to this
> series? Until this phy separation patches get merged, hdmi will remain
> broken for 5250 and 5420.
> 

Sorry for being late. I was so busy.

I have pondered over whether we should use default phy config values of
driver or not. My opinion is that we need to keep the default phy config
values in dts file because the values couldn't be used as default for all
boards: the default means that all boards should work well with the default
values, but wouldn't work. Therefore, the default values we are discussing
are specific to some boards even though most boards work well with the
values.

So I tend to agree with Sean Paul. Let's just add the default phy config
values to each board dts file that works well even though the values are
duplicated. For this, Rahul, we could rebase your patch set on top of
Shirish's patch.

Other opinions?

Thanks,
Inki Dae

> regards,
> Rahul Sharma.
> 
> >
> > Sean
> >
> >> The number of exceptions where we need to override the default confs
> >> is zero (as if now). 5420 based Peach and Smdk boards work exactly
> >> the same with same set of phy confs on 3 hdmi displays, I have. It
> >> may differ on Analyser. IMO we can defer this part till we have
> >> unacceptable analyser results for any specific board.
> >>
> >> Regards,
> >> Rahul Sharma.
> >>
> >>>> Sean
> >>>>
> >>>>
> >>>>
> >>>> > 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
> >>>> >
> >>>

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

[Rahul Sharma]

On 16 September 2013 18:10, Inki Dae <inki.dae@samsung.com> wrote:
> CCing devicetree,
>
>> -----Original Message-----
>> From: Rahul Sharma [mailto:r.sh.open@gmail.com]
>> Sent: Tuesday, September 10, 2013 5:28 PM
>> To: Sean Paul
>> Cc: Inki Dae; 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 6 September 2013 19:21, Sean Paul <seanpaul@chromium.org> wrote:
>> > On Thu, Sep 5, 2013 at 11:37 PM, Rahul Sharma <r.sh.open@gmail.com>
>> wrote:
>> >> On 5 September 2013 19:20, Inki Dae <inki.dae@samsung.com> wrote:
>> >>>
>> >>>
>> >>>> -----Original Message-----
>> >>>> From: Sean Paul [mailto:seanpaul@chromium.org]
>> >>>> Sent: Thursday, September 05, 2013 10:20 PM
>> >>>> To: Inki Dae
>> >>>> Cc: Rahul Sharma; 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 Thu, Sep 5, 2013 at 2:19 AM, Inki Dae <inki.dae@samsung.com>
> wrote:
>> >>>> >
>> >>>> >
>> >>>> >> -----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.
>> >>>> >
>> >>>>
>> >>>> Can you add the "default" configs to exynos5250.dtsi and
>> >>>> exynos5420.dtsi, then overwrite it in the board file if it needs to
>> be
>> >>>> different?
>> >>>>
>> >>
>> >> This will still introduce some duplication as 4412 and 5250 share same
>> >> phy confs and have no common dtsi. Similar situation can arise for
>> later
>> >> SoCs in exynos series.
>> >>
>> >>>
>> >>> Good idea but how about adding default-hdmiphy-config property to each
>> board
>> >>> dts file and removing phy config data from board dts file if they are
>> same
>> >>> as default one of driver? With this, hdmiphy driver checks if
>> >>> default-hdmiphy-config property exists, and then use default config
>> data if
>> >>> exists. And if not exists, hdmiphy driver gets and uses board specific
>> phy
>> >>> config data from board dts file.
>> >>> And it seems that the phy config values of Shirish's patch set are
>> same as
>> >>> default ones of driver. So we can remove the phy config data from each
>> board
>> >>> dts files and add default-hdmiphy-config property to there so that
>> default
>> >>> data of driver can be used.
>> >>>
>> >>>
>> >>> Thanks,
>> >>> Inki Dae
>> >>>
>> >>
>> >> We can simplify it further by letting the driver use phy-conf
>> >> property from DT. If phy-conf property is not available switch to
>> >> default confs, provided in the driver. This way we don't need to add
>> >> default-hdmiphy-config property to all board files.
>> >>
>> >
>> > This is probably a worthwhile discussion to have on Shirish's patch
>> > with devicetree-discuss. I'm unsure which is the preferred way to do
>> > something like this.
>>
>> I agree.
>>
>> Shall we keep those patches for "phy conf from DT" independent to this
>> series? Until this phy separation patches get merged, hdmi will remain
>> broken for 5250 and 5420.
>>
>
> Sorry for being late. I was so busy.
>
> I have pondered over whether we should use default phy config values of
> driver or not. My opinion is that we need to keep the default phy config
> values in dts file because the values couldn't be used as default for all
> boards: the default means that all boards should work well with the default
> values, but wouldn't work. Therefore, the default values we are discussing
> are specific to some boards even though most boards work well with the
> values.
>
> So I tend to agree with Sean Paul. Let's just add the default phy config
> values to each board dts file that works well even though the values are
> duplicated. For this, Rahul, we could rebase your patch set on top of
> Shirish's patch.
>
> Other opinions?
>

Any opinion from Device-Tree folks?

IMO, we should have same consensus on Shirish patches before proceeding.

Regards,
Rahul Sharma.

> Thanks,
> Inki Dae
>
>> regards,
>> Rahul Sharma.
>>
>> >
>> > Sean
>> >
>> >> The number of exceptions where we need to override the default confs
>> >> is zero (as if now). 5420 based Peach and Smdk boards work exactly
>> >> the same with same set of phy confs on 3 hdmi displays, I have. It
>> >> may differ on Analyser. IMO we can defer this part till we have
>> >> unacceptable analyser results for any specific board.
>> >>
>> >> Regards,
>> >> Rahul Sharma.
>> >>
>> >>>> Sean
>> >>>>
>> >>>>
>> >>>>
>> >>>> > 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
>> >>>> >
>> >>>
>

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

[Inki Dae]

[-- Attachment #1.1: Type: text/plain, Size: 15738 bytes --]

2013/9/27 Rahul Sharma <r.sh.open@gmail.com>

> On 16 September 2013 18:10, Inki Dae <inki.dae@samsung.com> wrote:
> > CCing devicetree,
> >
> >> -----Original Message-----
> >> From: Rahul Sharma [mailto:r.sh.open@gmail.com]
> >> Sent: Tuesday, September 10, 2013 5:28 PM
> >> To: Sean Paul
> >> Cc: Inki Dae; 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 6 September 2013 19:21, Sean Paul <seanpaul@chromium.org> wrote:
> >> > On Thu, Sep 5, 2013 at 11:37 PM, Rahul Sharma <r.sh.open@gmail.com>
> >> wrote:
> >> >> On 5 September 2013 19:20, Inki Dae <inki.dae@samsung.com> wrote:
> >> >>>
> >> >>>
> >> >>>> -----Original Message-----
> >> >>>> From: Sean Paul [mailto:seanpaul@chromium.org]
> >> >>>> Sent: Thursday, September 05, 2013 10:20 PM
> >> >>>> To: Inki Dae
> >> >>>> Cc: Rahul Sharma; 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 Thu, Sep 5, 2013 at 2:19 AM, Inki Dae <inki.dae@samsung.com>
> > wrote:
> >> >>>> >
> >> >>>> >
> >> >>>> >> -----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.
> >> >>>> >
> >> >>>>
> >> >>>> Can you add the "default" configs to exynos5250.dtsi and
> >> >>>> exynos5420.dtsi, then overwrite it in the board file if it needs to
> >> be
> >> >>>> different?
> >> >>>>
> >> >>
> >> >> This will still introduce some duplication as 4412 and 5250 share
> same
> >> >> phy confs and have no common dtsi. Similar situation can arise for
> >> later
> >> >> SoCs in exynos series.
> >> >>
> >> >>>
> >> >>> Good idea but how about adding default-hdmiphy-config property to
> each
> >> board
> >> >>> dts file and removing phy config data from board dts file if they
> are
> >> same
> >> >>> as default one of driver? With this, hdmiphy driver checks if
> >> >>> default-hdmiphy-config property exists, and then use default config
> >> data if
> >> >>> exists. And if not exists, hdmiphy driver gets and uses board
> specific
> >> phy
> >> >>> config data from board dts file.
> >> >>> And it seems that the phy config values of Shirish's patch set are
> >> same as
> >> >>> default ones of driver. So we can remove the phy config data from
> each
> >> board
> >> >>> dts files and add default-hdmiphy-config property to there so that
> >> default
> >> >>> data of driver can be used.
> >> >>>
> >> >>>
> >> >>> Thanks,
> >> >>> Inki Dae
> >> >>>
> >> >>
> >> >> We can simplify it further by letting the driver use phy-conf
> >> >> property from DT. If phy-conf property is not available switch to
> >> >> default confs, provided in the driver. This way we don't need to add
> >> >> default-hdmiphy-config property to all board files.
> >> >>
> >> >
> >> > This is probably a worthwhile discussion to have on Shirish's patch
> >> > with devicetree-discuss. I'm unsure which is the preferred way to do
> >> > something like this.
> >>
> >> I agree.
> >>
> >> Shall we keep those patches for "phy conf from DT" independent to this
> >> series? Until this phy separation patches get merged, hdmi will remain
> >> broken for 5250 and 5420.
> >>
> >
> > Sorry for being late. I was so busy.
> >
> > I have pondered over whether we should use default phy config values of
> > driver or not. My opinion is that we need to keep the default phy config
> > values in dts file because the values couldn't be used as default for all
> > boards: the default means that all boards should work well with the
> default
> > values, but wouldn't work. Therefore, the default values we are
> discussing
> > are specific to some boards even though most boards work well with the
> > values.
> >
> > So I tend to agree with Sean Paul. Let's just add the default phy config
> > values to each board dts file that works well even though the values are
> > duplicated. For this, Rahul, we could rebase your patch set on top of
> > Shirish's patch.
> >
> > Other opinions?
> >
>
> Any opinion from Device-Tree folks?
>
> IMO, we should have same consensus on Shirish patches before proceeding.
>
>
Rahul, it seems that DT people have no interest in this issue. So let's
have a consensus about this issue internally.

To Mr. Kyungmin, Sylwester, Kukjin Kim, and Tomasz,
How about keeping hdmiphy config data in each board dts file?

Thanks,
Inki Dae


> Regards,
> Rahul Sharma.
>
> > Thanks,
> > Inki Dae
> >
> >> regards,
> >> Rahul Sharma.
> >>
> >> >
> >> > Sean
> >> >
> >> >> The number of exceptions where we need to override the default confs
> >> >> is zero (as if now). 5420 based Peach and Smdk boards work exactly
> >> >> the same with same set of phy confs on 3 hdmi displays, I have. It
> >> >> may differ on Analyser. IMO we can defer this part till we have
> >> >> unacceptable analyser results for any specific board.
> >> >>
> >> >> Regards,
> >> >> Rahul Sharma.
> >> >>
> >> >>>> Sean
> >> >>>>
> >> >>>>
> >> >>>>
> >> >>>> > 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
> >> >>>> >
> >> >>>
> >
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>

[-- Attachment #1.2: Type: text/html, Size: 27720 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Sylwester Nawrocki]

On 09/28/2013 06:10 PM, Inki Dae wrote:
>> Any opinion from Device-Tree folks?
>>
>> IMO, we should have same consensus on Shirish patches before proceeding.
>
> Rahul, it seems that DT people have no interest in this issue. So let's
> have a consensus about this issue internally.
>
> To Mr. Kyungmin, Sylwester, Kukjin Kim, and Tomasz,
> How about keeping hdmiphy config data in each board dts file?

Please don't use HTML and quote only relevant part of e-mails. Otherwise
there are good chances your messages end up in people's spam box.

It often helps to Cc a DT binding maintainer directly.

Then, you consider moving the HDMI phy configuration to the device tree.
As Sean suggested in this thread:

">> +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,
>> +               },
>> +       },
[trimmed couple more entries]
>> +};
>>
> 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?"

The configuration data is 64 bytes of the register values IIUC. Would it be
possible to figure out exact meaning of each byte ? Do all of these bytes
need to be changed per board ? Perhaps we can have per SoC static tables in
the PHY driver and be overwriting only some of the bytes with values read
from device tree ?

AFAIR firmware-like binary blobs (register address/value pairs) are not
supposed to be stored in DT.

If there is no detailed documentation for theese PHY configuration tables
I guess there is no choice but to put these raw 64 bytes in DT. Presumably
this should be a an required property defined only by board dts, since those
values are PCB design dependent.

However, if not all boards need tweaking this configuration data, then it
could make sense to define recommended per-SoC tables in the driver and
overwrite from DT only if it is specified there for a specific board.
If we can come up with universal configuration for a SoC and selected pixel
clock frequency then it could likely be better to store that in the driver
rather than in DT.

--
Thanks,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Tomasz Figa]

On Monday 30 of September 2013 00:08:46 Sylwester Nawrocki wrote:
> On 09/28/2013 06:10 PM, Inki Dae wrote:
> >> Any opinion from Device-Tree folks?
> >> 
> >> IMO, we should have same consensus on Shirish patches before
> >> proceeding.> 
> > Rahul, it seems that DT people have no interest in this issue. So
> > let's
> > have a consensus about this issue internally.
> > 
> > To Mr. Kyungmin, Sylwester, Kukjin Kim, and Tomasz,
> > How about keeping hdmiphy config data in each board dts file?
> 
> Please don't use HTML and quote only relevant part of e-mails. Otherwise
> there are good chances your messages end up in people's spam box.
> 
> It often helps to Cc a DT binding maintainer directly.
> 
> Then, you consider moving the HDMI phy configuration to the device tree.
> As Sean suggested in this thread:
> 
> ">> +static struct hdmiphy_config hdmiphy_4210_configs[] = {

I'd like to only add that patches introducing or modifying a device tree 
binding need to be acked by at least one DT binding maintainer to be 
merged.

> >> +       {
> >> +               .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,
> >> +               },
> >> +       },
> 
> [trimmed couple more entries]
> 
> >> +};
> > 
> > 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?"
> 
> The configuration data is 64 bytes of the register values IIUC. Would it
> be possible to figure out exact meaning of each byte ?

This is definitely something that I would go for. Then for board specific 
data appropriate device tree properties could be defined, not just a 
binary blob.

Best regards,
Tomasz

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

[Inki Dae]

> -----Original Message-----
> From: linux-samsung-soc-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org [mailto:linux-samsung-soc-
> owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of Tomasz Figa
> Sent: Monday, September 30, 2013 8:13 AM
> To: Sylwester Nawrocki
> Cc: Inki Dae; Rahul Sharma; devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-samsung-soc;
> sw0312.kim; sunil joshi; dri-devel; kgene.kim; Shirish S; Sylwester
> Nawrocki; Rahul Sharma; Stephen Warren; Mark Rutland; Kumar Gala; Pawel
> Moll; Rob Herring; Sean Paul
> Subject: Re: [PATCH 1/7] drm/exynos: move hdmiphy related code to hdmiphy
> driver
> 
> On Monday 30 of September 2013 00:08:46 Sylwester Nawrocki wrote:
> > On 09/28/2013 06:10 PM, Inki Dae wrote:
> > >> Any opinion from Device-Tree folks?
> > >>
> > >> IMO, we should have same consensus on Shirish patches before
> > >> proceeding.>
> > > Rahul, it seems that DT people have no interest in this issue. So
> > > let's
> > > have a consensus about this issue internally.
> > >
> > > To Mr. Kyungmin, Sylwester, Kukjin Kim, and Tomasz,
> > > How about keeping hdmiphy config data in each board dts file?
> >
> > Please don't use HTML and quote only relevant part of e-mails. Otherwise
> > there are good chances your messages end up in people's spam box.
> >
> > It often helps to Cc a DT binding maintainer directly.
> >
> > Then, you consider moving the HDMI phy configuration to the device tree.
> > As Sean suggested in this thread:
> >
> > ">> +static struct hdmiphy_config hdmiphy_4210_configs[] = {
> 
> I'd like to only add that patches introducing or modifying a device tree
> binding need to be acked by at least one DT binding maintainer to be
> merged.
> 
> > >> +       {
> > >> +               .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,
> > >> +               },
> > >> +       },
> >
> > [trimmed couple more entries]
> >
> > >> +};
> > >
> > > 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?"
> >
> > The configuration data is 64 bytes of the register values IIUC. Would it
> > be possible to figure out exact meaning of each byte ?
> 
> This is definitely something that I would go for. Then for board specific
> data appropriate device tree properties could be defined, not just a
> binary blob.
> 

Agree. Thanks for your opinion.

Thanks,
Inki Dae

> Best regards,
> Tomasz
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-
> soc" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Inki Dae]

> -----Original Message-----
> From: Sylwester Nawrocki [mailto:sylvester.nawrocki-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org]
> Sent: Monday, September 30, 2013 7:09 AM
> To: Inki Dae
> Cc: Rahul Sharma; devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-samsung-soc;
> sw0312.kim; sunil joshi; dri-devel; kgene.kim; Shirish S; Sylwester
> Nawrocki; Rahul Sharma; Stephen Warren; Mark Rutland; Kumar Gala; Pawel
> Moll; Rob Herring; Sean Paul
> Subject: Re: [PATCH 1/7] drm/exynos: move hdmiphy related code to hdmiphy
> driver
> 
> On 09/28/2013 06:10 PM, Inki Dae wrote:
> >> Any opinion from Device-Tree folks?
> >>
> >> IMO, we should have same consensus on Shirish patches before
proceeding.
> >
> > Rahul, it seems that DT people have no interest in this issue. So let's
> > have a consensus about this issue internally.
> >
> > To Mr. Kyungmin, Sylwester, Kukjin Kim, and Tomasz,
> > How about keeping hdmiphy config data in each board dts file?
> 
> Please don't use HTML and quote only relevant part of e-mails. Otherwise
> there are good chances your messages end up in people's spam box.
> 

Ah, I missed checking text mode. Sorry about this. :)



> It often helps to Cc a DT binding maintainer directly.
> 

Good idea.

> Then, you consider moving the HDMI phy configuration to the device tree.
> As Sean suggested in this thread:
> 

Right.

> ">> +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,
> >> +               },
> >> +       },
> [trimmed couple more entries]
> >> +};
> >>
> > 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?"
> 
> The configuration data is 64 bytes of the register values IIUC. Would it
> be
> possible to figure out exact meaning of each byte ? Do all of these bytes

Right, but the user manual doesn't describe that enough so we might need to
inquire for what it means to design team. 

> need to be changed per board ? Perhaps we can have per SoC static tables
> in
> the PHY driver and be overwriting only some of the bytes with values read
> from device tree ?
> 
> AFAIR firmware-like binary blobs (register address/value pairs) are not
> supposed to be stored in DT.
> 
> If there is no detailed documentation for theese PHY configuration tables
> I guess there is no choice but to put these raw 64 bytes in DT. Presumably
> this should be a an required property defined only by board dts, since
> those
> values are PCB design dependent.
> 
> However, if not all boards need tweaking this configuration data, then it
> could make sense to define recommended per-SoC tables in the driver and
> overwrite from DT only if it is specified there for a specific board.
> If we can come up with universal configuration for a SoC and selected
> pixel
> clock frequency then it could likely be better to store that in the driver
> rather than in DT.
> 

Thanks you your opinion. However, we need to make sure how we take care of
the PHY configuration values. So I will have two steps to merge this pages
set.

To Rahul,
Could you post only your patch set regardless of Shirish's patch? I will
merge your patch set first because as is, Exynos drm hdmi driver is broken.
And, we need more discussions about Shirish patch. So I will not merge this
patch until we have a consensus about it.

To Shirish,
For your patch, it seems that you need to make sure to figure out exact
meaning of each byte of the PHY configuration values first. Maybe you need
to inquire for that to hardware or design team. And please separate the
values into common and specific parts if needed.


Thanks,
Inki Dae

> --
> Thanks,
> Sylwester

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Shirish S]

[-- Attachment #1.1: Type: text/plain, Size: 4697 bytes --]

Hi,
First of all sorry for the late response,


On Tue, Oct 1, 2013 at 10:09 AM, Inki Dae <inki.dae@samsung.com> wrote:

>
>
> > -----Original Message-----
> > From: Sylwester Nawrocki [mailto:sylvester.nawrocki@gmail.com]
> > Sent: Monday, September 30, 2013 7:09 AM
> > To: Inki Dae
> > Cc: Rahul Sharma; devicetree@vger.kernel.org; linux-samsung-soc;
> > sw0312.kim; sunil joshi; dri-devel; kgene.kim; Shirish S; Sylwester
> > Nawrocki; Rahul Sharma; Stephen Warren; Mark Rutland; Kumar Gala; Pawel
> > Moll; Rob Herring; Sean Paul
> > Subject: Re: [PATCH 1/7] drm/exynos: move hdmiphy related code to hdmiphy
> > driver
> >
> > On 09/28/2013 06:10 PM, Inki Dae wrote:
> > >> Any opinion from Device-Tree folks?
> > >>
> > >> IMO, we should have same consensus on Shirish patches before
> proceeding.
> > >
> > > Rahul, it seems that DT people have no interest in this issue. So let's
> > > have a consensus about this issue internally.
> > >
> > > To Mr. Kyungmin, Sylwester, Kukjin Kim, and Tomasz,
> > > How about keeping hdmiphy config data in each board dts file?
> >
> > Please don't use HTML and quote only relevant part of e-mails. Otherwise
> > there are good chances your messages end up in people's spam box.
> >
>
> Ah, I missed checking text mode. Sorry about this. :)
>
>
>
> > It often helps to Cc a DT binding maintainer directly.
> >
>
> Good idea.
>
> > Then, you consider moving the HDMI phy configuration to the device tree.
> > As Sean suggested in this thread:
> >
>
> Right.
>
> > ">> +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,
> > >> +               },
> > >> +       },
> > [trimmed couple more entries]
> > >> +};
> > >>
> > > 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?"
> >
> > The configuration data is 64 bytes of the register values IIUC. Would it
> > be
> > possible to figure out exact meaning of each byte ? Do all of these bytes
>
> Right, but the user manual doesn't describe that enough so we might need to
> inquire for what it means to design team.
>
> > need to be changed per board ? Perhaps we can have per SoC static tables
> > in
> > the PHY driver and be overwriting only some of the bytes with values read
> > from device tree ?
> >
> > AFAIR firmware-like binary blobs (register address/value pairs) are not
> > supposed to be stored in DT.
> >
> > If there is no detailed documentation for theese PHY configuration tables
> > I guess there is no choice but to put these raw 64 bytes in DT.
> Presumably
> > this should be a an required property defined only by board dts, since
> > those
> > values are PCB design dependent.
> >
> > However, if not all boards need tweaking this configuration data, then it
> > could make sense to define recommended per-SoC tables in the driver and
> > overwrite from DT only if it is specified there for a specific board.
> > If we can come up with universal configuration for a SoC and selected
> > pixel
> > clock frequency then it could likely be better to store that in the
> driver
> > rather than in DT.
> >
>
> Thanks you your opinion. However, we need to make sure how we take care of
> the PHY configuration values. So I will have two steps to merge this pages
> set.
>
> To Rahul,
> Could you post only your patch set regardless of Shirish's patch? I will
> merge your patch set first because as is, Exynos drm hdmi driver is broken.
> And, we need more discussions about Shirish patch. So I will not merge this
> patch until we have a consensus about it.
>
> To Shirish,
> For your patch, it seems that you need to make sure to figure out exact
> meaning of each byte of the PHY configuration values first. Maybe you need
> to inquire for that to hardware or design team. And please separate the
> values into common and specific parts if needed.
>
> Agreed, I shall request our hardware team to provide description about the
phy values, and will update the patch, once i receive the same.


>
> Thanks,
> Inki Dae
>
> > --
> > Thanks,
> > Sylwester
>
> Thanks,
Shirish S

[-- Attachment #1.2: Type: text/html, Size: 6313 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Shirish S]

[-- Attachment #1.1: Type: text/plain, Size: 5045 bytes --]

Hi,
I have uploaded patch set 2, with only required registers being moved to dt
file.
Thanks,
Shirish S


On Thu, Oct 3, 2013 at 7:58 AM, Shirish S <shirish@chromium.org> wrote:

> Hi,
> First of all sorry for the late response,
>
>
> On Tue, Oct 1, 2013 at 10:09 AM, Inki Dae <inki.dae@samsung.com> wrote:
>
>>
>>
>> > -----Original Message-----
>> > From: Sylwester Nawrocki [mailto:sylvester.nawrocki@gmail.com]
>> > Sent: Monday, September 30, 2013 7:09 AM
>> > To: Inki Dae
>> > Cc: Rahul Sharma; devicetree@vger.kernel.org; linux-samsung-soc;
>> > sw0312.kim; sunil joshi; dri-devel; kgene.kim; Shirish S; Sylwester
>> > Nawrocki; Rahul Sharma; Stephen Warren; Mark Rutland; Kumar Gala; Pawel
>> > Moll; Rob Herring; Sean Paul
>> > Subject: Re: [PATCH 1/7] drm/exynos: move hdmiphy related code to
>> hdmiphy
>> > driver
>> >
>> > On 09/28/2013 06:10 PM, Inki Dae wrote:
>> > >> Any opinion from Device-Tree folks?
>> > >>
>> > >> IMO, we should have same consensus on Shirish patches before
>> proceeding.
>> > >
>> > > Rahul, it seems that DT people have no interest in this issue. So
>> let's
>> > > have a consensus about this issue internally.
>> > >
>> > > To Mr. Kyungmin, Sylwester, Kukjin Kim, and Tomasz,
>> > > How about keeping hdmiphy config data in each board dts file?
>> >
>> > Please don't use HTML and quote only relevant part of e-mails. Otherwise
>> > there are good chances your messages end up in people's spam box.
>> >
>>
>> Ah, I missed checking text mode. Sorry about this. :)
>>
>>
>>
>> > It often helps to Cc a DT binding maintainer directly.
>> >
>>
>> Good idea.
>>
>> > Then, you consider moving the HDMI phy configuration to the device tree.
>> > As Sean suggested in this thread:
>> >
>>
>> Right.
>>
>> > ">> +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,
>> > >> +               },
>> > >> +       },
>> > [trimmed couple more entries]
>> > >> +};
>> > >>
>> > > 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?"
>> >
>> > The configuration data is 64 bytes of the register values IIUC. Would it
>> > be
>> > possible to figure out exact meaning of each byte ? Do all of these
>> bytes
>>
>> Right, but the user manual doesn't describe that enough so we might need
>> to
>> inquire for what it means to design team.
>>
>> > need to be changed per board ? Perhaps we can have per SoC static tables
>> > in
>> > the PHY driver and be overwriting only some of the bytes with values
>> read
>> > from device tree ?
>> >
>> > AFAIR firmware-like binary blobs (register address/value pairs) are not
>> > supposed to be stored in DT.
>> >
>> > If there is no detailed documentation for theese PHY configuration
>> tables
>> > I guess there is no choice but to put these raw 64 bytes in DT.
>> Presumably
>> > this should be a an required property defined only by board dts, since
>> > those
>> > values are PCB design dependent.
>> >
>> > However, if not all boards need tweaking this configuration data, then
>> it
>> > could make sense to define recommended per-SoC tables in the driver and
>> > overwrite from DT only if it is specified there for a specific board.
>> > If we can come up with universal configuration for a SoC and selected
>> > pixel
>> > clock frequency then it could likely be better to store that in the
>> driver
>> > rather than in DT.
>> >
>>
>> Thanks you your opinion. However, we need to make sure how we take care of
>> the PHY configuration values. So I will have two steps to merge this pages
>> set.
>>
>> To Rahul,
>> Could you post only your patch set regardless of Shirish's patch? I will
>> merge your patch set first because as is, Exynos drm hdmi driver is
>> broken.
>> And, we need more discussions about Shirish patch. So I will not merge
>> this
>> patch until we have a consensus about it.
>>
>> To Shirish,
>> For your patch, it seems that you need to make sure to figure out exact
>> meaning of each byte of the PHY configuration values first. Maybe you need
>> to inquire for that to hardware or design team. And please separate the
>> values into common and specific parts if needed.
>>
>> Agreed, I shall request our hardware team to provide description about
> the phy values, and will update the patch, once i receive the same.
>
>
>>
>> Thanks,
>> Inki Dae
>>
>> > --
>> > Thanks,
>> > Sylwester
>>
>> Thanks,
> Shirish S
>

[-- Attachment #1.2: Type: text/html, Size: 6813 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel