2013/9/27 Rahul Sharma <r.sh.o...@gmail.com> > On 16 September 2013 18:10, Inki Dae <inki....@samsung.com> wrote: > > CCing devicetree, > > > >> -----Original Message----- > >> From: Rahul Sharma [mailto:r.sh.o...@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 <seanp...@chromium.org> wrote: > >> > On Thu, Sep 5, 2013 at 11:37 PM, Rahul Sharma <r.sh.o...@gmail.com> > >> wrote: > >> >> On 5 September 2013 19:20, Inki Dae <inki....@samsung.com> wrote: > >> >>> > >> >>> > >> >>>> -----Original Message----- > >> >>>> From: Sean Paul [mailto:seanp...@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....@samsung.com> > > wrote: > >> >>>> > > >> >>>> > > >> >>>> >> -----Original Message----- > >> >>>> >> From: linux-samsung-soc-ow...@vger.kernel.org [mailto:linux- > >> samsung- > >> >>>> soc- > >> >>>> >> ow...@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....@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 majord...@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 >
_______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
