diff mbox

[3/4] phy: rockchip-typec: support DP phy switch

Message ID 1486712654-15431-4-git-send-email-zyw@rock-chips.com
State New, archived
Headers show

Commit Message

Chris Zhong Feb. 10, 2017, 7:44 a.m. UTC
There are 2 Type-c PHYs in RK3399, but only one DP controller. Hence
only one PHY can connect to DP controller at one time, the other should
be disconnected. The GRF_SOC_CON26 register has a switch bit to do it,
set this bit means enable PHY 1, clear this bit means enable PHY 0.

Signed-off-by: Chris Zhong <zyw@rock-chips.com>
---

 drivers/phy/phy-rockchip-typec.c | 9 +++++++++
 1 file changed, 9 insertions(+)

Comments

Brian Norris March 9, 2017, 12:39 a.m. UTC | #1
On Fri, Feb 10, 2017 at 03:44:13PM +0800, Chris Zhong wrote:
> There are 2 Type-c PHYs in RK3399, but only one DP controller. Hence
> only one PHY can connect to DP controller at one time, the other should
> be disconnected. The GRF_SOC_CON26 register has a switch bit to do it,
> set this bit means enable PHY 1, clear this bit means enable PHY 0.
> 
> Signed-off-by: Chris Zhong <zyw@rock-chips.com>
> ---
> 
>  drivers/phy/phy-rockchip-typec.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/phy/phy-rockchip-typec.c b/drivers/phy/phy-rockchip-typec.c
> index 7cfb0f8..1604aaa 100644
> --- a/drivers/phy/phy-rockchip-typec.c
> +++ b/drivers/phy/phy-rockchip-typec.c
> @@ -267,6 +267,7 @@ struct rockchip_usb3phy_port_cfg {
>  	struct usb3phy_reg usb3tousb2_en;
>  	struct usb3phy_reg external_psm;
>  	struct usb3phy_reg pipe_status;
> +	struct usb3phy_reg uphy_dp_sel;
>  };
>  
>  struct rockchip_typec_phy {
> @@ -736,6 +737,7 @@ static const struct phy_ops rockchip_usb3_phy_ops = {
>  static int rockchip_dp_phy_power_on(struct phy *phy)
>  {
>  	struct rockchip_typec_phy *tcphy = phy_get_drvdata(phy);
> +	struct rockchip_usb3phy_port_cfg *cfg = &tcphy->port_cfgs;
>  	int new_mode, ret = 0;
>  	u32 val;
>  
> @@ -766,6 +768,8 @@ static int rockchip_dp_phy_power_on(struct phy *phy)
>  		tcphy_phy_init(tcphy, new_mode);
>  	}
>  
> +	property_enable(tcphy, &cfg->uphy_dp_sel, 1);
> +
>  	ret = readx_poll_timeout(readl, tcphy->base + DP_MODE_CTL,

Idea for future work: this should just be readl_poll_timeout() here, and
throughout the driver.

>  				 val, val & DP_MODE_A2, 1000,
>  				 PHY_MODE_SET_TIMEOUT);
> @@ -869,6 +873,11 @@ static int tcphy_parse_dt(struct rockchip_typec_phy *tcphy,
>  	if (ret)
>  		return ret;
>  
> +	ret = tcphy_get_param(dev, &cfg->uphy_dp_sel,
> +			      "rockchip,uphy-dp-sel");
> +	if (ret)
> +		return ret;

What about existing device trees? You're essentially adding this
new property and requiring it at the same time.

Or are we considering no RK3399 DP stable at the moment? I guess we
haven't actually merged any device trees that support this yet, no?

Brian

> +
>  	tcphy->grf_regs = syscon_regmap_lookup_by_phandle(dev->of_node,
>  							  "rockchip,grf");
>  	if (IS_ERR(tcphy->grf_regs)) {
> -- 
> 2.6.3
> 
> 
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
Heiko Stübner March 9, 2017, 1:02 a.m. UTC | #2
Am Mittwoch, 8. März 2017, 16:39:23 CET schrieb Brian Norris:
> On Fri, Feb 10, 2017 at 03:44:13PM +0800, Chris Zhong wrote:
> > There are 2 Type-c PHYs in RK3399, but only one DP controller. Hence
> > only one PHY can connect to DP controller at one time, the other should
> > be disconnected. The GRF_SOC_CON26 register has a switch bit to do it,
> > set this bit means enable PHY 1, clear this bit means enable PHY 0.
> > 
> > Signed-off-by: Chris Zhong <zyw@rock-chips.com>
> > ---
> > 
> >  drivers/phy/phy-rockchip-typec.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/drivers/phy/phy-rockchip-typec.c
> > b/drivers/phy/phy-rockchip-typec.c index 7cfb0f8..1604aaa 100644
> > --- a/drivers/phy/phy-rockchip-typec.c
> > +++ b/drivers/phy/phy-rockchip-typec.c
> > @@ -267,6 +267,7 @@ struct rockchip_usb3phy_port_cfg {
> > 
> >  	struct usb3phy_reg usb3tousb2_en;
> >  	struct usb3phy_reg external_psm;
> >  	struct usb3phy_reg pipe_status;
> > 
> > +	struct usb3phy_reg uphy_dp_sel;
> > 
> >  };
> >  
> >  struct rockchip_typec_phy {
> > 
> > @@ -736,6 +737,7 @@ static const struct phy_ops rockchip_usb3_phy_ops = {
> > 
> >  static int rockchip_dp_phy_power_on(struct phy *phy)
> >  {
> >  
> >  	struct rockchip_typec_phy *tcphy = phy_get_drvdata(phy);
> > 
> > +	struct rockchip_usb3phy_port_cfg *cfg = &tcphy->port_cfgs;
> > 
> >  	int new_mode, ret = 0;
> >  	u32 val;
> > 
> > @@ -766,6 +768,8 @@ static int rockchip_dp_phy_power_on(struct phy *phy)
> > 
> >  		tcphy_phy_init(tcphy, new_mode);
> >  	
> >  	}
> > 
> > +	property_enable(tcphy, &cfg->uphy_dp_sel, 1);
> > +
> > 
> >  	ret = readx_poll_timeout(readl, tcphy->base + DP_MODE_CTL,
> 
> Idea for future work: this should just be readl_poll_timeout() here, and
> throughout the driver.
> 
> >  				 val, val & DP_MODE_A2, 1000,
> >  				 PHY_MODE_SET_TIMEOUT);
> > 
> > @@ -869,6 +873,11 @@ static int tcphy_parse_dt(struct rockchip_typec_phy
> > *tcphy,> 
> >  	if (ret)
> >  	
> >  		return ret;
> > 
> > +	ret = tcphy_get_param(dev, &cfg->uphy_dp_sel,
> > +			      "rockchip,uphy-dp-sel");
> > +	if (ret)
> > +		return ret;
> 
> What about existing device trees? You're essentially adding this
> new property and requiring it at the same time.
> 
> Or are we considering no RK3399 DP stable at the moment? I guess we
> haven't actually merged any device trees that support this yet, no?

An interesting situation we're in here. On the one hand, you're right this 
breaks "backwards compatiblity".

But on the other hand, the type-c phy is currently very much unused. The only 
current board rk3399-evb.dts does not enable them (so they're disabled 
everywhere) and we have neither dwc3 nor dp nodes in any rk3399 devicetrees so 
far. Also Rob was ok with the binding change :-) .

So from my pov, I'd say it _should_ be ok, as nothing is using the phys at all 
yet and thus there is nothing that could get broken.


Heiko

> 
> Brian
> 
> > +
> > 
> >  	tcphy->grf_regs = syscon_regmap_lookup_by_phandle(dev->of_node,
> >  	
> >  							  "rockchip,grf");
> >  	
> >  	if (IS_ERR(tcphy->grf_regs)) {
Chris Zhong March 9, 2017, 2:19 a.m. UTC | #3
Hi Heiko and Brain

On 03/09/2017 09:02 AM, Heiko Stübner wrote:
> Am Mittwoch, 8. März 2017, 16:39:23 CET schrieb Brian Norris:
>> On Fri, Feb 10, 2017 at 03:44:13PM +0800, Chris Zhong wrote:
>>> There are 2 Type-c PHYs in RK3399, but only one DP controller. Hence
>>> only one PHY can connect to DP controller at one time, the other should
>>> be disconnected. The GRF_SOC_CON26 register has a switch bit to do it,
>>> set this bit means enable PHY 1, clear this bit means enable PHY 0.
>>>
>>> Signed-off-by: Chris Zhong <zyw@rock-chips.com>
>>> ---
>>>
>>>   drivers/phy/phy-rockchip-typec.c | 9 +++++++++
>>>   1 file changed, 9 insertions(+)
>>>
>>> diff --git a/drivers/phy/phy-rockchip-typec.c
>>> b/drivers/phy/phy-rockchip-typec.c index 7cfb0f8..1604aaa 100644
>>> --- a/drivers/phy/phy-rockchip-typec.c
>>> +++ b/drivers/phy/phy-rockchip-typec.c
>>> @@ -267,6 +267,7 @@ struct rockchip_usb3phy_port_cfg {
>>>
>>>   	struct usb3phy_reg usb3tousb2_en;
>>>   	struct usb3phy_reg external_psm;
>>>   	struct usb3phy_reg pipe_status;
>>>
>>> +	struct usb3phy_reg uphy_dp_sel;
>>>
>>>   };
>>>   
>>>   struct rockchip_typec_phy {
>>>
>>> @@ -736,6 +737,7 @@ static const struct phy_ops rockchip_usb3_phy_ops = {
>>>
>>>   static int rockchip_dp_phy_power_on(struct phy *phy)
>>>   {
>>>   
>>>   	struct rockchip_typec_phy *tcphy = phy_get_drvdata(phy);
>>>
>>> +	struct rockchip_usb3phy_port_cfg *cfg = &tcphy->port_cfgs;
>>>
>>>   	int new_mode, ret = 0;
>>>   	u32 val;
>>>
>>> @@ -766,6 +768,8 @@ static int rockchip_dp_phy_power_on(struct phy *phy)
>>>
>>>   		tcphy_phy_init(tcphy, new_mode);
>>>   	
>>>   	}
>>>
>>> +	property_enable(tcphy, &cfg->uphy_dp_sel, 1);
>>> +
>>>
>>>   	ret = readx_poll_timeout(readl, tcphy->base + DP_MODE_CTL,
>> Idea for future work: this should just be readl_poll_timeout() here, and
>> throughout the driver.
Yes, the readl_poll_timeout is better, if next version series is needed, 
I am going to
add it in a separate patch behind this patch.
>>>   				 val, val & DP_MODE_A2, 1000,
>>>   				 PHY_MODE_SET_TIMEOUT);
>>>
>>> @@ -869,6 +873,11 @@ static int tcphy_parse_dt(struct rockchip_typec_phy
>>> *tcphy,>
>>>   	if (ret)
>>>   	
>>>   		return ret;
>>>
>>> +	ret = tcphy_get_param(dev, &cfg->uphy_dp_sel,
>>> +			      "rockchip,uphy-dp-sel");
>>> +	if (ret)
>>> +		return ret;
>> What about existing device trees? You're essentially adding this
>> new property and requiring it at the same time.
>>
>> Or are we considering no RK3399 DP stable at the moment? I guess we
>> haven't actually merged any device trees that support this yet, no?
> An interesting situation we're in here. On the one hand, you're right this
> breaks "backwards compatiblity".
>
> But on the other hand, the type-c phy is currently very much unused. The only
> current board rk3399-evb.dts does not enable them (so they're disabled
> everywhere) and we have neither dwc3 nor dp nodes in any rk3399 devicetrees so
> far. Also Rob was ok with the binding change :-) .
>
> So from my pov, I'd say it _should_ be ok, as nothing is using the phys at all
> yet and thus there is nothing that could get broken.
>
>
> Heiko
Thanks Heiko. On the other hand, these is no any display node at rk3399 
dtsi,
so I can not add the DP node, do you or Mark.Yao have plan to complete it?

>
>> Brian
>>
>>> +
>>>
>>>   	tcphy->grf_regs = syscon_regmap_lookup_by_phandle(dev->of_node,
>>>   	
>>>   							  "rockchip,grf");
>>>   	
>>>   	if (IS_ERR(tcphy->grf_regs)) {
>
>
>
>
Brian Norris March 9, 2017, 3:10 a.m. UTC | #4
Hi,

On Thu, Mar 09, 2017 at 02:02:54AM +0100, Heiko Stuebner wrote:
> Am Mittwoch, 8. März 2017, 16:39:23 CET schrieb Brian Norris:
> > On Fri, Feb 10, 2017 at 03:44:13PM +0800, Chris Zhong wrote:
> > > There are 2 Type-c PHYs in RK3399, but only one DP controller. Hence
> > > only one PHY can connect to DP controller at one time, the other should
> > > be disconnected. The GRF_SOC_CON26 register has a switch bit to do it,
> > > set this bit means enable PHY 1, clear this bit means enable PHY 0.
> > > 
> > > Signed-off-by: Chris Zhong <zyw@rock-chips.com>
> > > ---
> > > 
> > >  drivers/phy/phy-rockchip-typec.c | 9 +++++++++
> > >  1 file changed, 9 insertions(+)
> > > 
> > > diff --git a/drivers/phy/phy-rockchip-typec.c
> > > b/drivers/phy/phy-rockchip-typec.c index 7cfb0f8..1604aaa 100644
> > > --- a/drivers/phy/phy-rockchip-typec.c
> > > +++ b/drivers/phy/phy-rockchip-typec.c

...

> > > @@ -869,6 +873,11 @@ static int tcphy_parse_dt(struct rockchip_typec_phy
> > > *tcphy,> 
> > >  	if (ret)
> > >  	
> > >  		return ret;
> > > 
> > > +	ret = tcphy_get_param(dev, &cfg->uphy_dp_sel,
> > > +			      "rockchip,uphy-dp-sel");
> > > +	if (ret)
> > > +		return ret;
> > 
> > What about existing device trees? You're essentially adding this
> > new property and requiring it at the same time.
> > 
> > Or are we considering no RK3399 DP stable at the moment? I guess we
> > haven't actually merged any device trees that support this yet, no?
> 
> An interesting situation we're in here. On the one hand, you're right this 
> breaks "backwards compatiblity".
> 
> But on the other hand, the type-c phy is currently very much unused. The only 
> current board rk3399-evb.dts does not enable them (so they're disabled 
> everywhere) and we have neither dwc3 nor dp nodes in any rk3399 devicetrees so 
> far. Also Rob was ok with the binding change :-) .
> 
> So from my pov, I'd say it _should_ be ok, as nothing is using the phys at all 
> yet and thus there is nothing that could get broken.

Yeah, I guess it's OK... but BTW out-of-tree DTs are perfectly
legit, once the bindings are accepted.

Another random point of contention (not worth too much, as the pattern
is already set), but why do these deserve DT properties at all? The
device already has a "rk3399" compatible property, so can't we derive
GRF offsets from that?

Brian
Heiko Stübner March 9, 2017, 8:31 a.m. UTC | #5
Hi Brian,

Am Mittwoch, 8. März 2017, 19:10:50 CET schrieb Brian Norris:
> On Thu, Mar 09, 2017 at 02:02:54AM +0100, Heiko Stuebner wrote:
> > Am Mittwoch, 8. März 2017, 16:39:23 CET schrieb Brian Norris:
> > > On Fri, Feb 10, 2017 at 03:44:13PM +0800, Chris Zhong wrote:
> > > > There are 2 Type-c PHYs in RK3399, but only one DP controller. Hence
> > > > only one PHY can connect to DP controller at one time, the other
> > > > should
> > > > be disconnected. The GRF_SOC_CON26 register has a switch bit to do it,
> > > > set this bit means enable PHY 1, clear this bit means enable PHY 0.
> > > > 
> > > > Signed-off-by: Chris Zhong <zyw@rock-chips.com>
> > > > ---
> > > > 
> > > >  drivers/phy/phy-rockchip-typec.c | 9 +++++++++
> > > >  1 file changed, 9 insertions(+)
> > > > 
> > > > diff --git a/drivers/phy/phy-rockchip-typec.c
> > > > b/drivers/phy/phy-rockchip-typec.c index 7cfb0f8..1604aaa 100644
> > > > --- a/drivers/phy/phy-rockchip-typec.c
> > > > +++ b/drivers/phy/phy-rockchip-typec.c
> 
> ...
> 
> > > > @@ -869,6 +873,11 @@ static int tcphy_parse_dt(struct
> > > > rockchip_typec_phy
> > > > *tcphy,>
> > > > 
> > > >  	if (ret)
> > > >  	
> > > >  		return ret;
> > > > 
> > > > +	ret = tcphy_get_param(dev, &cfg->uphy_dp_sel,
> > > > +			      "rockchip,uphy-dp-sel");
> > > > +	if (ret)
> > > > +		return ret;
> > > 
> > > What about existing device trees? You're essentially adding this
> > > new property and requiring it at the same time.
> > > 
> > > Or are we considering no RK3399 DP stable at the moment? I guess we
> > > haven't actually merged any device trees that support this yet, no?
> > 
> > An interesting situation we're in here. On the one hand, you're right this
> > breaks "backwards compatiblity".
> > 
> > But on the other hand, the type-c phy is currently very much unused. The
> > only current board rk3399-evb.dts does not enable them (so they're
> > disabled everywhere) and we have neither dwc3 nor dp nodes in any rk3399
> > devicetrees so far. Also Rob was ok with the binding change :-) .
> > 
> > So from my pov, I'd say it _should_ be ok, as nothing is using the phys at
> > all yet and thus there is nothing that could get broken.
> 
> Yeah, I guess it's OK... but BTW out-of-tree DTs are perfectly
> legit, once the bindings are accepted.
> 
> Another random point of contention (not worth too much, as the pattern
> is already set), but why do these deserve DT properties at all? The
> device already has a "rk3399" compatible property, so can't we derive
> GRF offsets from that?

I'm definitly with you in this regard.

But it seems like there is some sort of current trend of moving more stuff 
into the dt again. I vaguely remember phy and (or) dt-maintainers preferring 
to have these definitions in the dt for the typec-phy.

See also the recent mail from Olof concerning the grf initialization and maybe 
not having per-soc definitions in the driver, where I'm trying to keep things 
out of the dt a bit more strongly :-) .

So yes, personally I would definitly prefer not having to much GRF-stuff leak 
into the dt. Simply because the GRF has always been very unstable over time 
[=you always find one more bit that needs tuning] and to not cause things like 
the above. But as you said I guess we're to late for the typec-phy.


Heiko
Brian Norris March 9, 2017, 11:35 p.m. UTC | #6
On Thu, Mar 09, 2017 at 09:31:33AM +0100, Heiko Stuebner wrote:
> Am Mittwoch, 8. März 2017, 19:10:50 CET schrieb Brian Norris:
> > Another random point of contention (not worth too much, as the pattern
> > is already set), but why do these deserve DT properties at all? The
> > device already has a "rk3399" compatible property, so can't we derive
> > GRF offsets from that?
> 
> I'm definitly with you in this regard.
> 
> But it seems like there is some sort of current trend of moving more stuff 
> into the dt again. I vaguely remember phy and (or) dt-maintainers preferring 
> to have these definitions in the dt for the typec-phy.

Hmm, not sure I understand that one. You're just going to multiply the
variations of DT props you have to support, instead of simply
multiplying a (non-ABI) table within the driver. The latter seems much
more stable. Oh well, not my subsystem.

> See also the recent mail from Olof concerning the grf initialization and maybe 
> not having per-soc definitions in the driver, where I'm trying to keep things 
> out of the dt a bit more strongly :-) .

Thanks for the pointer. I looked up (and replied to) that one.

Either I don't understand exactly what he's saying or... I'd like to
know what he's smoking. You can't just wish away the fact that the GRF
is truly a "general register file" and will change forever (and so will
our understanding of how to use it). Kernels always need to update for
new chipsets. Device Tree Bindings Are Forever (TM).

Now let's see if Olof reads my reply which points back to this
thread...and now to the above comment :)

> So yes, personally I would definitly prefer not having to much GRF-stuff leak 
> into the dt. Simply because the GRF has always been very unstable over time 
> [=you always find one more bit that needs tuning] and to not cause things like 
> the above. But as you said I guess we're to late for the typec-phy.

I'm with you.

Brian
diff mbox

Patch

diff --git a/drivers/phy/phy-rockchip-typec.c b/drivers/phy/phy-rockchip-typec.c
index 7cfb0f8..1604aaa 100644
--- a/drivers/phy/phy-rockchip-typec.c
+++ b/drivers/phy/phy-rockchip-typec.c
@@ -267,6 +267,7 @@  struct rockchip_usb3phy_port_cfg {
 	struct usb3phy_reg usb3tousb2_en;
 	struct usb3phy_reg external_psm;
 	struct usb3phy_reg pipe_status;
+	struct usb3phy_reg uphy_dp_sel;
 };
 
 struct rockchip_typec_phy {
@@ -736,6 +737,7 @@  static const struct phy_ops rockchip_usb3_phy_ops = {
 static int rockchip_dp_phy_power_on(struct phy *phy)
 {
 	struct rockchip_typec_phy *tcphy = phy_get_drvdata(phy);
+	struct rockchip_usb3phy_port_cfg *cfg = &tcphy->port_cfgs;
 	int new_mode, ret = 0;
 	u32 val;
 
@@ -766,6 +768,8 @@  static int rockchip_dp_phy_power_on(struct phy *phy)
 		tcphy_phy_init(tcphy, new_mode);
 	}
 
+	property_enable(tcphy, &cfg->uphy_dp_sel, 1);
+
 	ret = readx_poll_timeout(readl, tcphy->base + DP_MODE_CTL,
 				 val, val & DP_MODE_A2, 1000,
 				 PHY_MODE_SET_TIMEOUT);
@@ -869,6 +873,11 @@  static int tcphy_parse_dt(struct rockchip_typec_phy *tcphy,
 	if (ret)
 		return ret;
 
+	ret = tcphy_get_param(dev, &cfg->uphy_dp_sel,
+			      "rockchip,uphy-dp-sel");
+	if (ret)
+		return ret;
+
 	tcphy->grf_regs = syscon_regmap_lookup_by_phandle(dev->of_node,
 							  "rockchip,grf");
 	if (IS_ERR(tcphy->grf_regs)) {