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. 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 >> >>>> > >> >>> > -- 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
