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

(off-list ancestor, not in this archive)

2013/9/27 Rahul Sharma [off-list ref]

On 16 September 2013 18:10, Inki Dae [off-list ref] 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 [off-list ref] wrote:
On Thu, Sep 5, 2013 at 11:37 PM, Rahul Sharma [off-list ref]
wrote:
On 5 September 2013 19:20, Inki Dae [off-list ref] 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 [off-list ref]
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 [off-list ref]
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

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help