[PATCH 1/7] drm/exynos: move hdmiphy related code to hdmiphy driver
Rahul Sharma
r.sh.open at gmail.com
Tue Sep 10 01:27:51 PDT 2013
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.
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
>>>> >
>>>
More information about the dri-devel
mailing list