[PATCH 1/7] drm/exynos: move hdmiphy related code to hdmiphy driver
Inki Dae
inki.dae at samsung.com
Sat Sep 28 09:10:42 PDT 2013
2013/9/27 Rahul Sharma <r.sh.open at gmail.com>
> On 16 September 2013 18:10, Inki Dae <inki.dae at samsung.com> wrote:
> > CCing devicetree,
> >
> >> -----Original Message-----
> >> From: Rahul Sharma [mailto:r.sh.open at 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 at chromium.org> wrote:
> >> > On Thu, Sep 5, 2013 at 11:37 PM, Rahul Sharma <r.sh.open at gmail.com>
> >> wrote:
> >> >> On 5 September 2013 19:20, Inki Dae <inki.dae at samsung.com> wrote:
> >> >>>
> >> >>>
> >> >>>> -----Original Message-----
> >> >>>> From: Sean Paul [mailto:seanpaul at 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 at samsung.com>
> > wrote:
> >> >>>> >
> >> >>>> >
> >> >>>> >> -----Original Message-----
> >> >>>> >> From: linux-samsung-soc-owner at vger.kernel.org [mailto:linux-
> >> samsung-
> >> >>>> soc-
> >> >>>> >> owner at 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 at 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 at vger.kernel.org
> >> >>>> >> More majordomo info at
> http://vger.kernel.org/majordomo-info.html
> >> >>>> >
> >> >>>
> >
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20130929/1884b552/attachment-0001.html>
More information about the dri-devel
mailing list