Hi,
On Wed, Nov 29, 2017 at 6:27 PM, Chris Zhong <[email protected]> wrote:
> Hi Doug
>
> Thank you for mentioning this patch.
>
> I think the focus of the discussion is: can we put the grf control bit to
> dts.
>
> The RK3399 has 2 Type-C phy, but only one DP controller, this "uphy_dp_sel"
>
> can help to switch these 2 phy. So I think this bit can be considered as a
> part of
>
> Type-C phy, these 2 phy have different bits, just similar to other bits
> (such as "pipe-status").
>
> Put them to DTS file might be a accepted practice.
I guess the first step would be finding the person to make a decision.
Is that Heiko? Olof? Kishon? Rob?. As I see it there are a few
options:
1. Land this series as-is. This makes the new bit work just like all
the other ones next to it. If anyone happens to try to use an old
device tree on a new kernel they'll break. Seems rather unlikely
given that the whole type C PHY is not really fully functional
upstream, but technically this is a no-no from a device tree
perspective.
2. Change the series to make this property optional. If it's not
there then the code behaves like it always did. This would address
the "compatibility" problem but likely wouldn't actually help any real
people, and it would be extra work.
3. Redo the driver to deprecate all the old offsets / bits and just
put the table in the driver, keyed off the compatible string and base
address if the IO memory.
I can't make this decision. It's up to those folks who would be
landing the patch and I'd be happy with any of them. What I'm less
happy with, however, is the indecision preventing forward progress.
We should pick one of the above things and land it. My own personal
bias is #1: just land the series. No real people will be hurt and
it's just adding another property that matches the ones next to it.
>From a long term perspective (AKA how I'd write the next driver like
this) I personally lean towards to "tables in the driver, not in the
device tree" but quite honestly I'm happy to take whatever direction
the maintainers give.
-Doug
Am Freitag, 1. Dezember 2017, 13:42:46 CET schrieb Doug Anderson:
> Hi,
>
> On Wed, Nov 29, 2017 at 6:27 PM, Chris Zhong <[email protected]> wrote:
> > Hi Doug
> >
> > Thank you for mentioning this patch.
> >
> > I think the focus of the discussion is: can we put the grf control bit to
> > dts.
> >
> > The RK3399 has 2 Type-C phy, but only one DP controller, this "uphy_dp_sel"
> >
> > can help to switch these 2 phy. So I think this bit can be considered as a
> > part of
> >
> > Type-C phy, these 2 phy have different bits, just similar to other bits
> > (such as "pipe-status").
> >
> > Put them to DTS file might be a accepted practice.
>
> I guess the first step would be finding the person to make a decision.
> Is that Heiko? Olof? Kishon? Rob?. As I see it there are a few
> options:
>
> 1. Land this series as-is. This makes the new bit work just like all
> the other ones next to it. If anyone happens to try to use an old
> device tree on a new kernel they'll break. Seems rather unlikely
> given that the whole type C PHY is not really fully functional
> upstream, but technically this is a no-no from a device tree
> perspective.
>
> 2. Change the series to make this property optional. If it's not
> there then the code behaves like it always did. This would address
> the "compatibility" problem but likely wouldn't actually help any real
> people, and it would be extra work.
>
> 3. Redo the driver to deprecate all the old offsets / bits and just
> put the table in the driver, keyed off the compatible string and base
> address if the IO memory.
>
>
> I can't make this decision. It's up to those folks who would be
> landing the patch and I'd be happy with any of them. What I'm less
> happy with, however, is the indecision preventing forward progress.
> We should pick one of the above things and land it. My own personal
> bias is #1: just land the series. No real people will be hurt and
> it's just adding another property that matches the ones next to it.
I'd second that #1 . That whole type-c phy thingy never fully worked in
the past (some for the never used dp output), so personally I don't have
issues with going that route.
> From a long term perspective (AKA how I'd write the next driver like
> this) I personally lean towards to "tables in the driver, not in the
> device tree" but quite honestly I'm happy to take whatever direction
> the maintainers give.
It looks like we're in agreement here :-) . GRF stuff should not leak into
the devicetree, as it causes endless headaches later. But I guess we'll
need to live with the ones that happened so far.
Heiko
Hi Heiko
On 2017??12??02?? 05:58, Heiko Stuebner wrote:
> Am Freitag, 1. Dezember 2017, 13:42:46 CET schrieb Doug Anderson:
>> Hi,
>>
>> On Wed, Nov 29, 2017 at 6:27 PM, Chris Zhong <[email protected]> wrote:
>>> Hi Doug
>>>
>>> Thank you for mentioning this patch.
>>>
>>> I think the focus of the discussion is: can we put the grf control bit to
>>> dts.
>>>
>>> The RK3399 has 2 Type-C phy, but only one DP controller, this "uphy_dp_sel"
>>>
>>> can help to switch these 2 phy. So I think this bit can be considered as a
>>> part of
>>>
>>> Type-C phy, these 2 phy have different bits, just similar to other bits
>>> (such as "pipe-status").
>>>
>>> Put them to DTS file might be a accepted practice.
>> I guess the first step would be finding the person to make a decision.
>> Is that Heiko? Olof? Kishon? Rob?. As I see it there are a few
>> options:
>>
>> 1. Land this series as-is. This makes the new bit work just like all
>> the other ones next to it. If anyone happens to try to use an old
>> device tree on a new kernel they'll break. Seems rather unlikely
>> given that the whole type C PHY is not really fully functional
>> upstream, but technically this is a no-no from a device tree
>> perspective.
>>
>> 2. Change the series to make this property optional. If it's not
>> there then the code behaves like it always did. This would address
>> the "compatibility" problem but likely wouldn't actually help any real
>> people, and it would be extra work.
>>
>> 3. Redo the driver to deprecate all the old offsets / bits and just
>> put the table in the driver, keyed off the compatible string and base
>> address if the IO memory.
>>
>>
>> I can't make this decision. It's up to those folks who would be
>> landing the patch and I'd be happy with any of them. What I'm less
>> happy with, however, is the indecision preventing forward progress.
>> We should pick one of the above things and land it. My own personal
>> bias is #1: just land the series. No real people will be hurt and
>> it's just adding another property that matches the ones next to it.
> I'd second that #1 . That whole type-c phy thingy never fully worked in
> the past (some for the never used dp output), so personally I don't have
> issues with going that route.
>
>
>> From a long term perspective (AKA how I'd write the next driver like
>> this) I personally lean towards to "tables in the driver, not in the
>> device tree" but quite honestly I'm happy to take whatever direction
>> the maintainers give.
> It looks like we're in agreement here :-) . GRF stuff should not leak into
> the devicetree, as it causes endless headaches later. But I guess we'll
> need to live with the ones that happened so far.
>
So, the first step is: move all the private property of tcphy to
drivers/phy/rockchip/phy-rockchip-typec.c.
Second step: new a member: uphy-dp-sel.
In my mind, we should have discussed these properties before, and then I
moved them all into DTS.
>
>
>
>
--
Chris Zhong
Hi Chris,
Am Montag, 4. Dezember 2017, 10:47:08 CET schrieb Chris Zhong:
> On 2017年12月02日 05:58, Heiko Stuebner wrote:
> > Am Freitag, 1. Dezember 2017, 13:42:46 CET schrieb Doug Anderson:
> >> Hi,
> >>
> >> On Wed, Nov 29, 2017 at 6:27 PM, Chris Zhong <[email protected]> wrote:
> >>> Hi Doug
> >>>
> >>> Thank you for mentioning this patch.
> >>>
> >>> I think the focus of the discussion is: can we put the grf control bit
> >>> to
> >>> dts.
> >>>
> >>> The RK3399 has 2 Type-C phy, but only one DP controller, this
> >>> "uphy_dp_sel"
> >>>
> >>> can help to switch these 2 phy. So I think this bit can be considered as
> >>> a
> >>> part of
> >>>
> >>> Type-C phy, these 2 phy have different bits, just similar to other bits
> >>> (such as "pipe-status").
> >>>
> >>> Put them to DTS file might be a accepted practice.
> >>
> >> I guess the first step would be finding the person to make a decision.
> >> Is that Heiko? Olof? Kishon? Rob?. As I see it there are a few
> >> options:
> >>
> >> 1. Land this series as-is. This makes the new bit work just like all
> >> the other ones next to it. If anyone happens to try to use an old
> >> device tree on a new kernel they'll break. Seems rather unlikely
> >> given that the whole type C PHY is not really fully functional
> >> upstream, but technically this is a no-no from a device tree
> >> perspective.
> >>
> >> 2. Change the series to make this property optional. If it's not
> >> there then the code behaves like it always did. This would address
> >> the "compatibility" problem but likely wouldn't actually help any real
> >> people, and it would be extra work.
> >>
> >> 3. Redo the driver to deprecate all the old offsets / bits and just
> >> put the table in the driver, keyed off the compatible string and base
> >> address if the IO memory.
> >>
> >>
> >> I can't make this decision. It's up to those folks who would be
> >> landing the patch and I'd be happy with any of them. What I'm less
> >> happy with, however, is the indecision preventing forward progress.
> >> We should pick one of the above things and land it. My own personal
> >> bias is #1: just land the series. No real people will be hurt and
> >> it's just adding another property that matches the ones next to it.
> >
> > I'd second that #1 . That whole type-c phy thingy never fully worked in
> > the past (some for the never used dp output), so personally I don't have
> > issues with going that route.
> >
> >> From a long term perspective (AKA how I'd write the next driver like
> >>
> >> this) I personally lean towards to "tables in the driver, not in the
> >> device tree" but quite honestly I'm happy to take whatever direction
> >> the maintainers give.
> >
> > It looks like we're in agreement here :-) . GRF stuff should not leak into
> > the devicetree, as it causes endless headaches later. But I guess we'll
> > need to live with the ones that happened so far.
>
> So, the first step is: move all the private property of tcphy to
> drivers/phy/rockchip/phy-rockchip-typec.c.
> Second step: new a member: uphy-dp-sel.
> In my mind, we should have discussed these properties before, and then I
> moved them all into DTS.
Actually, I was agreeing with Doug, that we probably don't need to rework the
type-c phy driver. As most properties for it are in the devicetree right now
we'll need to support them for backwards-compatiblity anyway.
And yes, there probably was discussion over dts vs. driver-table when the
type-c driver was introduced, but I either missed it or wasn't firm enough
back then ;-) .
Hence the "we'll need to live with it" for the type-c phy, but should not
do similar things in future drivers.
Heiko
Hi,
On Sun, Dec 3, 2017 at 11:46 PM, Heiko Stübner <[email protected]> wrote:
> Hi Chris,
>
> Am Montag, 4. Dezember 2017, 10:47:08 CET schrieb Chris Zhong:
>> On 2017年12月02日 05:58, Heiko Stuebner wrote:
>> > Am Freitag, 1. Dezember 2017, 13:42:46 CET schrieb Doug Anderson:
>> >> Hi,
>> >>
>> >> On Wed, Nov 29, 2017 at 6:27 PM, Chris Zhong <[email protected]> wrote:
>> >>> Hi Doug
>> >>>
>> >>> Thank you for mentioning this patch.
>> >>>
>> >>> I think the focus of the discussion is: can we put the grf control bit
>> >>> to
>> >>> dts.
>> >>>
>> >>> The RK3399 has 2 Type-C phy, but only one DP controller, this
>> >>> "uphy_dp_sel"
>> >>>
>> >>> can help to switch these 2 phy. So I think this bit can be considered as
>> >>> a
>> >>> part of
>> >>>
>> >>> Type-C phy, these 2 phy have different bits, just similar to other bits
>> >>> (such as "pipe-status").
>> >>>
>> >>> Put them to DTS file might be a accepted practice.
>> >>
>> >> I guess the first step would be finding the person to make a decision.
>> >> Is that Heiko? Olof? Kishon? Rob?. As I see it there are a few
>> >> options:
>> >>
>> >> 1. Land this series as-is. This makes the new bit work just like all
>> >> the other ones next to it. If anyone happens to try to use an old
>> >> device tree on a new kernel they'll break. Seems rather unlikely
>> >> given that the whole type C PHY is not really fully functional
>> >> upstream, but technically this is a no-no from a device tree
>> >> perspective.
>> >>
>> >> 2. Change the series to make this property optional. If it's not
>> >> there then the code behaves like it always did. This would address
>> >> the "compatibility" problem but likely wouldn't actually help any real
>> >> people, and it would be extra work.
>> >>
>> >> 3. Redo the driver to deprecate all the old offsets / bits and just
>> >> put the table in the driver, keyed off the compatible string and base
>> >> address if the IO memory.
>> >>
>> >>
>> >> I can't make this decision. It's up to those folks who would be
>> >> landing the patch and I'd be happy with any of them. What I'm less
>> >> happy with, however, is the indecision preventing forward progress.
>> >> We should pick one of the above things and land it. My own personal
>> >> bias is #1: just land the series. No real people will be hurt and
>> >> it's just adding another property that matches the ones next to it.
>> >
>> > I'd second that #1 . That whole type-c phy thingy never fully worked in
>> > the past (some for the never used dp output), so personally I don't have
>> > issues with going that route.
>> >
>> >> From a long term perspective (AKA how I'd write the next driver like
>> >>
>> >> this) I personally lean towards to "tables in the driver, not in the
>> >> device tree" but quite honestly I'm happy to take whatever direction
>> >> the maintainers give.
>> >
>> > It looks like we're in agreement here :-) . GRF stuff should not leak into
>> > the devicetree, as it causes endless headaches later. But I guess we'll
>> > need to live with the ones that happened so far.
>>
>> So, the first step is: move all the private property of tcphy to
>> drivers/phy/rockchip/phy-rockchip-typec.c.
>> Second step: new a member: uphy-dp-sel.
>> In my mind, we should have discussed these properties before, and then I
>> moved them all into DTS.
>
> Actually, I was agreeing with Doug, that we probably don't need to rework the
> type-c phy driver. As most properties for it are in the devicetree right now
> we'll need to support them for backwards-compatiblity anyway.
>
> And yes, there probably was discussion over dts vs. driver-table when the
> type-c driver was introduced, but I either missed it or wasn't firm enough
> back then ;-) .
>
> Hence the "we'll need to live with it" for the type-c phy, but should not
> do similar things in future drivers.
So I guess now we're just waiting for some agreement from Kishon that
he's willing to land the PHY change? Heiko: presumably you could
apply the DP change to drm-misc? ...or is there some other process
needed there?
-Doug
Hi,
Am Montag, 4. Dezember 2017, 08:08:31 CET schrieb Doug Anderson:
> On Sun, Dec 3, 2017 at 11:46 PM, Heiko Stübner <[email protected]> wrote:
> > Am Montag, 4. Dezember 2017, 10:47:08 CET schrieb Chris Zhong:
> >> On 2017年12月02日 05:58, Heiko Stuebner wrote:
> >> > Am Freitag, 1. Dezember 2017, 13:42:46 CET schrieb Doug Anderson:
> >> >> On Wed, Nov 29, 2017 at 6:27 PM, Chris Zhong <[email protected]>
wrote:
> >> >>> Thank you for mentioning this patch.
> >> >>>
> >> >>> I think the focus of the discussion is: can we put the grf control
> >> >>> bit
> >> >>> to
> >> >>> dts.
> >> >>>
> >> >>> The RK3399 has 2 Type-C phy, but only one DP controller, this
> >> >>> "uphy_dp_sel"
> >> >>>
> >> >>> can help to switch these 2 phy. So I think this bit can be considered
> >> >>> as
> >> >>> a
> >> >>> part of
> >> >>>
> >> >>> Type-C phy, these 2 phy have different bits, just similar to other
> >> >>> bits
> >> >>> (such as "pipe-status").
> >> >>>
> >> >>> Put them to DTS file might be a accepted practice.
> >> >>
> >> >> I guess the first step would be finding the person to make a decision.
> >> >> Is that Heiko? Olof? Kishon? Rob?. As I see it there are a few
> >> >> options:
> >> >>
> >> >> 1. Land this series as-is. This makes the new bit work just like all
> >> >> the other ones next to it. If anyone happens to try to use an old
> >> >> device tree on a new kernel they'll break. Seems rather unlikely
> >> >> given that the whole type C PHY is not really fully functional
> >> >> upstream, but technically this is a no-no from a device tree
> >> >> perspective.
> >> >>
> >> >> 2. Change the series to make this property optional. If it's not
> >> >> there then the code behaves like it always did. This would address
> >> >> the "compatibility" problem but likely wouldn't actually help any real
> >> >> people, and it would be extra work.
> >> >>
> >> >> 3. Redo the driver to deprecate all the old offsets / bits and just
> >> >> put the table in the driver, keyed off the compatible string and base
> >> >> address if the IO memory.
> >> >>
> >> >>
> >> >> I can't make this decision. It's up to those folks who would be
> >> >> landing the patch and I'd be happy with any of them. What I'm less
> >> >> happy with, however, is the indecision preventing forward progress.
> >> >> We should pick one of the above things and land it. My own personal
> >> >> bias is #1: just land the series. No real people will be hurt and
> >> >> it's just adding another property that matches the ones next to it.
> >> >
> >> > I'd second that #1 . That whole type-c phy thingy never fully worked in
> >> > the past (some for the never used dp output), so personally I don't
> >> > have
> >> > issues with going that route.
> >> >
> >> >> From a long term perspective (AKA how I'd write the next driver like
> >> >>
> >> >> this) I personally lean towards to "tables in the driver, not in the
> >> >> device tree" but quite honestly I'm happy to take whatever direction
> >> >> the maintainers give.
> >> >
> >> > It looks like we're in agreement here :-) . GRF stuff should not leak
> >> > into
> >> > the devicetree, as it causes endless headaches later. But I guess we'll
> >> > need to live with the ones that happened so far.
> >>
> >> So, the first step is: move all the private property of tcphy to
> >> drivers/phy/rockchip/phy-rockchip-typec.c.
> >> Second step: new a member: uphy-dp-sel.
> >> In my mind, we should have discussed these properties before, and then I
> >> moved them all into DTS.
> >
> > Actually, I was agreeing with Doug, that we probably don't need to rework
> > the type-c phy driver. As most properties for it are in the devicetree
> > right now we'll need to support them for backwards-compatiblity anyway.
> >
> > And yes, there probably was discussion over dts vs. driver-table when the
> > type-c driver was introduced, but I either missed it or wasn't firm enough
> > back then ;-) .
> >
> > Hence the "we'll need to live with it" for the type-c phy, but should not
> > do similar things in future drivers.
>
> So I guess now we're just waiting for some agreement from Kishon that
> he's willing to land the PHY change? Heiko: presumably you could
> apply the DP change to drm-misc? ...or is there some other process
> needed there?
I was lagging behind a bit with the drm-misc account request but have
done so now. So once I got the hang of how drm-misc works and Kishon
has picked the phy-part I can most likely push the drm part (or Sandy,
depending on who is faster).
As for process, I don't think there is special care necessary. When
you get the intermediate case of phy-change but no drm-change
everything will just revert to how it works now anyway.
Heiko