[PATCH 0/4] Move DP phy switch to PHY driver

[PATCH 0/4] Move DP phy switch to PHY driver

[Chris Zhong]

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.

If the board has 2 Type-C ports, the DP driver get the phy id from
devm_of_phy_get_by_index, and then control this switch according to
this id. But some others board only has one Type-C port, it may be PHY 0
or PHY 1. The dts node id can not tell us the correct PHY id. Hence move
this switch to PHY driver, the PHY driver can distinguish between PHY 0
and PHY 1, and then write the correct register bit.



Chris Zhong (4):
  Documentation: bindings: add uphy-dp-sel for Rockchip USB Type-C PHY
  arm64: dts: rockchip: add rockchip,uphy-dp-sel for Type-C phy
  phy: rockchip-typec: support DP phy switch
  drm/rockchip: cdn-dp: remove the DP phy switch

 Documentation/devicetree/bindings/phy/phy-rockchip-typec.txt | 5 +++++
 arch/arm64/boot/dts/rockchip/rk3399.dtsi                     | 2 ++
 drivers/gpu/drm/rockchip/cdn-dp-core.c                       | 7 -------
 drivers/phy/phy-rockchip-typec.c                             | 9 +++++++++
 4 files changed, 16 insertions(+), 7 deletions(-)

-- 
2.6.3

[PATCH 1/4] Documentation: bindings: add uphy-dp-sel for Rockchip USB Type-C PHY

[Chris Zhong]

rockchip,uphy-dp-sel is the register of type-c phy enable DP function.

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

 Documentation/devicetree/bindings/phy/phy-rockchip-typec.txt | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/phy/phy-rockchip-typec.txt b/Documentation/devicetree/bindings/phy/phy-rockchip-typec.txt
index 6ea867e..c3be83b 100644
--- a/Documentation/devicetree/bindings/phy/phy-rockchip-typec.txt
+++ b/Documentation/devicetree/bindings/phy/phy-rockchip-typec.txt
@@ -33,6 +33,9 @@ offset, enable bit, write mask bit.
  - rockchip,pipe-status : the register of type-c phy pipe status.
    for type-c phy0, it must be <0xe5c0 0 0>;
    for type-c phy1, it must be <0xe5c0 16 16>;
+ - rockchip,uphy-dp-sel : the register of type-c phy enable DP function
+   for type-c phy0, it must be <0x6268 19 19>;
+   for type-c phy1, it must be <0x6268 3 19>;
 
 Required nodes : a sub-node is required for each port the phy provides.
 		 The sub-node name is used to identify dp or usb3 port,
@@ -62,6 +65,7 @@ Example:
 		rockchip,usb3tousb2-en = <0xe580 3 19>;
 		rockchip,external-psm = <0xe588 14 30>;
 		rockchip,pipe-status = <0xe5c0 0 0>;
+		rockchip,uphy-dp-sel = <0x6268 19 19>;
 
 		tcphy0_dp: dp-port {
 			#phy-cells = <0>;
@@ -90,6 +94,7 @@ Example:
 		rockchip,usb3tousb2-en = <0xe58c 3 19>;
 		rockchip,external-psm = <0xe594 14 30>;
 		rockchip,pipe-status = <0xe5c0 16 16>;
+		rockchip,uphy-dp-sel = <0x6268 3 19>;
 
 		tcphy1_dp: dp-port {
 			#phy-cells = <0>;
-- 
2.6.3

[PATCH 2/4] arm64: dts: rockchip: add rockchip,uphy-dp-sel for Type-C phy

[Chris Zhong]

rockchip,uphy-dp-sel is the register of type-c phy enable DP function.

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

 arch/arm64/boot/dts/rockchip/rk3399.dtsi | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
index 8e6d1bd..7e8aa8c 100644
--- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi
@@ -1225,6 +1225,7 @@
 		rockchip,usb3tousb2-en = <0xe580 3 19>;
 		rockchip,external-psm = <0xe588 14 30>;
 		rockchip,pipe-status = <0xe5c0 0 0>;
+		rockchip,uphy-dp-sel = <0x6268 19 19>;
 		status = "disabled";
 
 		tcphy0_dp: dp-port {
@@ -1254,6 +1255,7 @@
 		rockchip,usb3tousb2-en = <0xe58c 3 19>;
 		rockchip,external-psm = <0xe594 14 30>;
 		rockchip,pipe-status = <0xe5c0 16 16>;
+		rockchip,uphy-dp-sel = <0x6268 3 19>;
 		status = "disabled";
 
 		tcphy1_dp: dp-port {
-- 
2.6.3

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

[Chris Zhong]

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,
 				 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)) {
-- 
2.6.3

[PATCH 4/4] drm/rockchip: cdn-dp: remove the DP phy switch

[Chris Zhong]

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.

If the board has 2 Type-C ports, the DP driver get the phy id from
devm_of_phy_get_by_index, and then control this switch according to
this id. But some others board only has one Type-C port, it may be PHY 0
or PHY 1. The dts node id can not tell us the correct PHY id. Hence move
this switch to PHY driver, the PHY driver can distinguish between PHY 0
and PHY 1, and then write the correct register bit.

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

 drivers/gpu/drm/rockchip/cdn-dp-core.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/cdn-dp-core.c b/drivers/gpu/drm/rockchip/cdn-dp-core.c
index 9ab67a6..d3f6e6b 100644
--- a/drivers/gpu/drm/rockchip/cdn-dp-core.c
+++ b/drivers/gpu/drm/rockchip/cdn-dp-core.c
@@ -43,8 +43,6 @@
 #define GRF_SOC_CON9		0x6224
 #define DP_SEL_VOP_LIT		BIT(12)
 #define GRF_SOC_CON26		0x6268
-#define UPHY_SEL_BIT		3
-#define UPHY_SEL_MASK		BIT(19)
 #define DPTX_HPD_SEL		(3 << 12)
 #define DPTX_HPD_DEL		(2 << 12)
 #define DPTX_HPD_SEL_MASK	(3 << 28)
@@ -403,11 +401,6 @@ static int cdn_dp_enable_phy(struct cdn_dp_device *dp, struct cdn_dp_port *port)
 	union extcon_property_value property;
 	int ret;
 
-	ret = cdn_dp_grf_write(dp, GRF_SOC_CON26,
-			       (port->id << UPHY_SEL_BIT) | UPHY_SEL_MASK);
-	if (ret)
-		return ret;
-
 	if (!port->phy_enabled) {
 		ret = phy_power_on(port->phy);
 		if (ret) {
-- 
2.6.3

Re: [PATCH 1/4] Documentation: bindings: add uphy-dp-sel for Rockchip USB Type-C PHY

[Rob Herring]

On Fri, Feb 10, 2017 at 03:44:11PM +0800, Chris Zhong wrote:
> rockchip,uphy-dp-sel is the register of type-c phy enable DP function.

"dt-bindings: phy:" is the preferred subject prefix.

> 
> Signed-off-by: Chris Zhong <zyw@rock-chips.com>
> ---
> 
>  Documentation/devicetree/bindings/phy/phy-rockchip-typec.txt | 5 +++++
>  1 file changed, 5 insertions(+)

Otherwise,

Acked-by: Rob Herring <robh@kernel.org> 

Re: [PATCH 1/4] Documentation: bindings: add uphy-dp-sel for Rockchip USB Type-C PHY

[Chris Zhong]

Hi Rob

On 02/16/2017 10:20 AM, Rob Herring wrote:
> On Fri, Feb 10, 2017 at 03:44:11PM +0800, Chris Zhong wrote:
>> rockchip,uphy-dp-sel is the register of type-c phy enable DP function.
> "dt-bindings: phy:" is the preferred subject prefix.
OK, I will change the header next version.
dt-bindings: phy: add uphy-dp-sel for Rockchip USB Type-C PHY.

>
>> Signed-off-by: Chris Zhong <zyw@rock-chips.com>
>> ---
>>
>>   Documentation/devicetree/bindings/phy/phy-rockchip-typec.txt | 5 +++++
>>   1 file changed, 5 insertions(+)
> Otherwise,
>
> Acked-by: Rob Herring <robh@kernel.org>
>
>
>

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

[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?

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

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

[Heiko Stübner]

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)) {

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

[Chris Zhong]

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)) {
>
>
>
>

-- 
Chris Zhong

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

[Brian Norris]

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

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

[Heiko Stübner]

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

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

[Brian Norris]

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

Re: [PATCH 0/4] Move DP phy switch to PHY driver

[Doug Anderson]

Hi,

On Thu, Feb 9, 2017 at 11:44 PM, Chris Zhong <zyw@rock-chips.com> 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.
>
> If the board has 2 Type-C ports, the DP driver get the phy id from
> devm_of_phy_get_by_index, and then control this switch according to
> this id. But some others board only has one Type-C port, it may be PHY 0
> or PHY 1. The dts node id can not tell us the correct PHY id. Hence move
> this switch to PHY driver, the PHY driver can distinguish between PHY 0
> and PHY 1, and then write the correct register bit.
>
>
>
> Chris Zhong (4):
>   Documentation: bindings: add uphy-dp-sel for Rockchip USB Type-C PHY
>   arm64: dts: rockchip: add rockchip,uphy-dp-sel for Type-C phy
>   phy: rockchip-typec: support DP phy switch
>   drm/rockchip: cdn-dp: remove the DP phy switch
>
>  Documentation/devicetree/bindings/phy/phy-rockchip-typec.txt | 5 +++++
>  arch/arm64/boot/dts/rockchip/rk3399.dtsi                     | 2 ++
>  drivers/gpu/drm/rockchip/cdn-dp-core.c                       | 7 -------
>  drivers/phy/phy-rockchip-typec.c                             | 9 +++++++++
>  4 files changed, 16 insertions(+), 7 deletions(-)

What ever happened to this series?  It seemed like it just dropped on
the floor...

There was a bit of contention on patch #3
<https://patchwork.kernel.org/patch/9566095/> about the fact that we
were specifying addresses in the device tree vs. hardcoding them in
the driver.  Any way we can just make a decision and go with it?


-Doug

Re: [PATCH 0/4] Move DP phy switch to PHY driver

[Chris Zhong]

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.



On 2017年11月29日 07:32, Doug Anderson wrote:
> Hi,
>
> On Thu, Feb 9, 2017 at 11:44 PM, Chris Zhong <zyw@rock-chips.com> 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.
>>
>> If the board has 2 Type-C ports, the DP driver get the phy id from
>> devm_of_phy_get_by_index, and then control this switch according to
>> this id. But some others board only has one Type-C port, it may be PHY 0
>> or PHY 1. The dts node id can not tell us the correct PHY id. Hence move
>> this switch to PHY driver, the PHY driver can distinguish between PHY 0
>> and PHY 1, and then write the correct register bit.
>>
>>
>>
>> Chris Zhong (4):
>>    Documentation: bindings: add uphy-dp-sel for Rockchip USB Type-C PHY
>>    arm64: dts: rockchip: add rockchip,uphy-dp-sel for Type-C phy
>>    phy: rockchip-typec: support DP phy switch
>>    drm/rockchip: cdn-dp: remove the DP phy switch
>>
>>   Documentation/devicetree/bindings/phy/phy-rockchip-typec.txt | 5 +++++
>>   arch/arm64/boot/dts/rockchip/rk3399.dtsi                     | 2 ++
>>   drivers/gpu/drm/rockchip/cdn-dp-core.c                       | 7 -------
>>   drivers/phy/phy-rockchip-typec.c                             | 9 +++++++++
>>   4 files changed, 16 insertions(+), 7 deletions(-)
> What ever happened to this series?  It seemed like it just dropped on
> the floor...
>
> There was a bit of contention on patch #3
> <https://patchwork.kernel.org/patch/9566095/> about the fact that we
> were specifying addresses in the device tree vs. hardcoding them in
> the driver.  Any way we can just make a decision and go with it?
>
>
> -Doug
>
>
>

-- 
Chris Zhong

Re: [PATCH 0/4] Move DP phy switch to PHY driver

[Doug Anderson]

Hi,

On Wed, Nov 29, 2017 at 6:27 PM, Chris Zhong <zyw@rock-chips.com> 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

Re: [PATCH 0/4] Move DP phy switch to PHY driver

[Heiko Stuebner]

Am Freitag, 1. Dezember 2017, 13:42:46 CET schrieb Doug Anderson:
> Hi,
> 
> On Wed, Nov 29, 2017 at 6:27 PM, Chris Zhong <zyw@rock-chips.com> 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

Re: [PATCH 0/4] Move DP phy switch to PHY driver

[Chris Zhong]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=gb18030; format=flowed, Size: 2991 bytes --]

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 <zyw@rock-chips.com> 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

Re: [PATCH 0/4] Move DP phy switch to PHY driver

[Heiko Stübner]

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 <zyw@rock-chips.com> 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

Re: [PATCH 0/4] Move DP phy switch to PHY driver

[Doug Anderson]

Hi,

On Sun, Dec 3, 2017 at 11:46 PM, Heiko Stübner <heiko@sntech.de> 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 <zyw@rock-chips.com> 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

Re: [PATCH 0/4] Move DP phy switch to PHY driver

[Heiko Stübner]

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 <heiko@sntech.de> 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 <zyw@rock-chips.com> 
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

Re: [PATCH 0/4] Move DP phy switch to PHY driver

[Kishon Vijay Abraham I]

Hi,

On Friday 10 February 2017 01:14 PM, 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.
> 
> If the board has 2 Type-C ports, the DP driver get the phy id from
> devm_of_phy_get_by_index, and then control this switch according to
> this id. But some others board only has one Type-C port, it may be PHY 0
> or PHY 1. The dts node id can not tell us the correct PHY id. Hence move
> this switch to PHY driver, the PHY driver can distinguish between PHY 0
> and PHY 1, and then write the correct register bit.
> 
> 

Changed subject of "Documentation: bindings: add uphy-dp-sel for Rockchip USB
Type-C PHY" as suggested by Rob, rebased "phy: rockchip-typec: support DP phy
switch" to the latest kernel and merged.

Thanks
Kishon

> 
> Chris Zhong (4):
>   Documentation: bindings: add uphy-dp-sel for Rockchip USB Type-C PHY
>   arm64: dts: rockchip: add rockchip,uphy-dp-sel for Type-C phy
>   phy: rockchip-typec: support DP phy switch
>   drm/rockchip: cdn-dp: remove the DP phy switch
> 
>  Documentation/devicetree/bindings/phy/phy-rockchip-typec.txt | 5 +++++
>  arch/arm64/boot/dts/rockchip/rk3399.dtsi                     | 2 ++
>  drivers/gpu/drm/rockchip/cdn-dp-core.c                       | 7 -------
>  drivers/phy/phy-rockchip-typec.c                             | 9 +++++++++
>  4 files changed, 16 insertions(+), 7 deletions(-)
> 

Re: [PATCH 0/4] Move DP phy switch to PHY driver

[Heiko Stübner]

Hi Kishon,

Am Freitag, 16. Februar 2018, 12:04:42 CET schrieb Kishon Vijay Abraham I:
> On Friday 10 February 2017 01:14 PM, 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.
> > 
> > If the board has 2 Type-C ports, the DP driver get the phy id from
> > devm_of_phy_get_by_index, and then control this switch according to
> > this id. But some others board only has one Type-C port, it may be PHY 0
> > or PHY 1. The dts node id can not tell us the correct PHY id. Hence move
> > this switch to PHY driver, the PHY driver can distinguish between PHY 0
> > and PHY 1, and then write the correct register bit.
> 
> Changed subject of "Documentation: bindings: add uphy-dp-sel for Rockchip
> USB Type-C PHY" as suggested by Rob, rebased "phy: rockchip-typec: support
> DP phy switch" to the latest kernel and merged.

I'm not sure how far along you are with your merging, but you might want to 
revert this one.

In your inbox you should find more recent threads about the type-c phy
where Rob strongly suggested moving the whole grf registers out of the
dt and into the driver.

Enric Balletbo did just that and posted a series (including the dp-move)
yesterday (refined in a v2 from today).

Alternatively Enric could rebase his series on top of that recent change.


Heiko

Re: [PATCH 0/4] Move DP phy switch to PHY driver

[Kishon Vijay Abraham I]

On Friday 16 February 2018 06:35 PM, Heiko Stübner wrote:
> Hi Kishon,
> 
> Am Freitag, 16. Februar 2018, 12:04:42 CET schrieb Kishon Vijay Abraham I:
>> On Friday 10 February 2017 01:14 PM, 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.
>>>
>>> If the board has 2 Type-C ports, the DP driver get the phy id from
>>> devm_of_phy_get_by_index, and then control this switch according to
>>> this id. But some others board only has one Type-C port, it may be PHY 0
>>> or PHY 1. The dts node id can not tell us the correct PHY id. Hence move
>>> this switch to PHY driver, the PHY driver can distinguish between PHY 0
>>> and PHY 1, and then write the correct register bit.
>>
>> Changed subject of "Documentation: bindings: add uphy-dp-sel for Rockchip
>> USB Type-C PHY" as suggested by Rob, rebased "phy: rockchip-typec: support
>> DP phy switch" to the latest kernel and merged.
> 
> I'm not sure how far along you are with your merging, but you might want to 
> revert this one.
> 
> In your inbox you should find more recent threads about the type-c phy
> where Rob strongly suggested moving the whole grf registers out of the
> dt and into the driver.
> 
> Enric Balletbo did just that and posted a series (including the dp-move)
> yesterday (refined in a v2 from today).
> 
> Alternatively Enric could rebase his series on top of that recent change.

I can drop this series and take Enric's series. All of them are still in my
local system.

Thanks
Kishon