| Message ID | 1486712654-15431-4-git-send-email-zyw@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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)) {
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)) { > > > >
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
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
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 --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)) {
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(+)