| Message ID | 20240501162308.875193-4-jonas@kwiboo.se |
|---|---|
| State | Accepted |
| Commit | e801d05bea6d99b2731f3c96edc754b36f75bcdc |
| Delegated to: | Kever Yang |
| Headers | show |
| Series | rockchip: rk3399: Sync DT with v6.8 and update defconfigs | expand |
On 2024/5/2 00:22, Jonas Karlman wrote: > rk3399-nanopi-4.dtsi try to set parent of and set rate to 100 MHz of the > SCLK_PCIEPHY_REF clock. > > The existing enable/disable ops for SCLK_PCIEPHY_REF currently force > use of 24 MHz parent and rate. > > Add improved support for setting parent and rate of the pciephy refclk > to driver to better support assign-clock props for pciephy refclk in DT. > > This limited implementation only support setting 24 or 100 MHz rate, > and expect npll and clk_pciephy_ref100m divider to use default values. > > Signed-off-by: Jonas Karlman <jonas@kwiboo.se> Reviewed-by: Kever Yang <kever.yang@rock-chips.com> Thanks, - Kever > --- > v2: Implement partial instead of dummy support for SCLK_PCIEPHY_REF > --- > drivers/clk/rockchip/clk_rk3399.c | 59 +++++++++++++++++++++++++++++-- > 1 file changed, 57 insertions(+), 2 deletions(-) > > diff --git a/drivers/clk/rockchip/clk_rk3399.c b/drivers/clk/rockchip/clk_rk3399.c > index 5934771b4096..0b3223611a32 100644 > --- a/drivers/clk/rockchip/clk_rk3399.c > +++ b/drivers/clk/rockchip/clk_rk3399.c > @@ -926,6 +926,26 @@ static ulong rk3399_saradc_set_clk(struct rockchip_cru *cru, uint hz) > return rk3399_saradc_get_clk(cru); > } > > +static ulong rk3399_pciephy_get_clk(struct rockchip_cru *cru) > +{ > + if (readl(&cru->clksel_con[18]) & BIT(10)) > + return 100 * MHz; > + else > + return OSC_HZ; > +} > + > +static ulong rk3399_pciephy_set_clk(struct rockchip_cru *cru, uint hz) > +{ > + if (hz == 100 * MHz) > + rk_setreg(&cru->clksel_con[18], BIT(10)); > + else if (hz == OSC_HZ) > + rk_clrreg(&cru->clksel_con[18], BIT(10)); > + else > + return -EINVAL; > + > + return rk3399_pciephy_get_clk(cru); > +} > + > static ulong rk3399_clk_get_rate(struct clk *clk) > { > struct rk3399_clk_priv *priv = dev_get_priv(clk->dev); > @@ -967,6 +987,9 @@ static ulong rk3399_clk_get_rate(struct clk *clk) > case SCLK_SARADC: > rate = rk3399_saradc_get_clk(priv->cru); > break; > + case SCLK_PCIEPHY_REF: > + rate = rk3399_pciephy_get_clk(priv->cru); > + break; > case ACLK_VIO: > case ACLK_HDCP: > case ACLK_GIC_PRE: > @@ -1058,6 +1081,9 @@ static ulong rk3399_clk_set_rate(struct clk *clk, ulong rate) > case SCLK_SARADC: > ret = rk3399_saradc_set_clk(priv->cru, rate); > break; > + case SCLK_PCIEPHY_REF: > + ret = rk3399_pciephy_set_clk(priv->cru, rate); > + break; > case ACLK_VIO: > case ACLK_HDCP: > case ACLK_GIC_PRE: > @@ -1108,12 +1134,39 @@ static int __maybe_unused rk3399_gmac_set_parent(struct clk *clk, > return -EINVAL; > } > > +static int __maybe_unused rk3399_pciephy_set_parent(struct clk *clk, > + struct clk *parent) > +{ > + struct rk3399_clk_priv *priv = dev_get_priv(clk->dev); > + const char *clock_output_name; > + int ret; > + > + if (parent->dev == clk->dev && parent->id == SCLK_PCIEPHY_REF100M) { > + rk_setreg(&priv->cru->clksel_con[18], BIT(10)); > + return 0; > + } > + > + ret = dev_read_string_index(parent->dev, "clock-output-names", > + parent->id, &clock_output_name); > + if (ret < 0) > + return -ENODATA; > + > + if (!strcmp(clock_output_name, "xin24m")) { > + rk_clrreg(&priv->cru->clksel_con[18], BIT(10)); > + return 0; > + } > + > + return -EINVAL; > +} > + > static int __maybe_unused rk3399_clk_set_parent(struct clk *clk, > struct clk *parent) > { > switch (clk->id) { > case SCLK_RMII_SRC: > return rk3399_gmac_set_parent(clk, parent); > + case SCLK_PCIEPHY_REF: > + return rk3399_pciephy_set_parent(clk, parent); > } > > debug("%s: unsupported clk %ld\n", __func__, clk->id); > @@ -1204,7 +1257,8 @@ static int rk3399_clk_enable(struct clk *clk) > rk_clrreg(&priv->cru->clkgate_con[13], BIT(7)); > break; > case SCLK_PCIEPHY_REF: > - rk_clrreg(&priv->cru->clksel_con[18], BIT(10)); > + if (readl(&priv->cru->clksel_con[18]) & BIT(10)) > + rk_clrreg(&priv->cru->clkgate_con[12], BIT(6)); > break; > default: > debug("%s: unsupported clk %ld\n", __func__, clk->id); > @@ -1298,7 +1352,8 @@ static int rk3399_clk_disable(struct clk *clk) > rk_setreg(&priv->cru->clkgate_con[13], BIT(7)); > break; > case SCLK_PCIEPHY_REF: > - rk_clrreg(&priv->cru->clksel_con[18], BIT(10)); > + if (readl(&priv->cru->clksel_con[18]) & BIT(10)) > + rk_setreg(&priv->cru->clkgate_con[12], BIT(6)); > break; > default: > debug("%s: unsupported clk %ld\n", __func__, clk->id);
Hi Jonas, On 5/1/24 6:22 PM, Jonas Karlman wrote: > rk3399-nanopi-4.dtsi try to set parent of and set rate to 100 MHz of the > SCLK_PCIEPHY_REF clock. > > The existing enable/disable ops for SCLK_PCIEPHY_REF currently force > use of 24 MHz parent and rate. > > Add improved support for setting parent and rate of the pciephy refclk > to driver to better support assign-clock props for pciephy refclk in DT. > > This limited implementation only support setting 24 or 100 MHz rate, > and expect npll and clk_pciephy_ref100m divider to use default values. > > Signed-off-by: Jonas Karlman <jonas@kwiboo.se> > --- > v2: Implement partial instead of dummy support for SCLK_PCIEPHY_REF > --- > drivers/clk/rockchip/clk_rk3399.c | 59 +++++++++++++++++++++++++++++-- > 1 file changed, 57 insertions(+), 2 deletions(-) > > diff --git a/drivers/clk/rockchip/clk_rk3399.c b/drivers/clk/rockchip/clk_rk3399.c > index 5934771b4096..0b3223611a32 100644 > --- a/drivers/clk/rockchip/clk_rk3399.c > +++ b/drivers/clk/rockchip/clk_rk3399.c > @@ -926,6 +926,26 @@ static ulong rk3399_saradc_set_clk(struct rockchip_cru *cru, uint hz) > return rk3399_saradc_get_clk(cru); > } > > +static ulong rk3399_pciephy_get_clk(struct rockchip_cru *cru) > +{ > + if (readl(&cru->clksel_con[18]) & BIT(10)) > + return 100 * MHz; > + else > + return OSC_HZ; Could avoid the else since all if blocks return, no other logic than the one matching the else can reach that part of the code. Therefore: """ if (readl(&cru->clksel_con[18]) & BIT(10)) return 100 * MHz; return OSC_HZ; """ works just as well. Could also be """ return (readl(&cru->clksel_con[18]) & BIT(10)) ? 100 * MHz : OSC_HZ; """ [...] > +static int __maybe_unused rk3399_pciephy_set_parent(struct clk *clk, > + struct clk *parent) > +{ > + struct rk3399_clk_priv *priv = dev_get_priv(clk->dev); > + const char *clock_output_name; > + int ret; > + > + if (parent->dev == clk->dev && parent->id == SCLK_PCIEPHY_REF100M) { > + rk_setreg(&priv->cru->clksel_con[18], BIT(10)); > + return 0; > + } > + > + ret = dev_read_string_index(parent->dev, "clock-output-names", > + parent->id, &clock_output_name); Are you sure this works? Considering that parent->id seems to store unique ids, like 167 for SCLK_PCIEPHY_REF100M, I doubt we should be using it for dev_read_string_index???? As per documentation: """ /** * dev_read_string_index() - obtain an indexed string from a string list * * @dev: device to examine * @propname: name of the property containing the string list * @index: index of the string to return * @outp: return location for the string * * Return: * length of string, if found or -ve error value if not found */ int dev_read_string_index(const struct udevice *dev, const char *propname, int index, const char **outp); """ So index here means the (index+1)'th string in the list of strings under the "propname" DT property, I doubt we have properties with 167+ strings in them? I realize that rk3399_gmac_set_parent also uses this, so I'm a bit puzzled right now... Don't you want to use dev_read_stringlist_search(parent->dev, "clock-output-names", "xin24m") instead? The rest looks good to me. Cheers, Quentin
Hi Quentin, On 2024-05-06 13:07, Quentin Schulz wrote: > Hi Jonas, > > On 5/1/24 6:22 PM, Jonas Karlman wrote: >> rk3399-nanopi-4.dtsi try to set parent of and set rate to 100 MHz of the >> SCLK_PCIEPHY_REF clock. >> >> The existing enable/disable ops for SCLK_PCIEPHY_REF currently force >> use of 24 MHz parent and rate. >> >> Add improved support for setting parent and rate of the pciephy refclk >> to driver to better support assign-clock props for pciephy refclk in DT. >> >> This limited implementation only support setting 24 or 100 MHz rate, >> and expect npll and clk_pciephy_ref100m divider to use default values. >> >> Signed-off-by: Jonas Karlman <jonas@kwiboo.se> >> --- >> v2: Implement partial instead of dummy support for SCLK_PCIEPHY_REF >> --- >> drivers/clk/rockchip/clk_rk3399.c | 59 +++++++++++++++++++++++++++++-- >> 1 file changed, 57 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/clk/rockchip/clk_rk3399.c b/drivers/clk/rockchip/clk_rk3399.c >> index 5934771b4096..0b3223611a32 100644 >> --- a/drivers/clk/rockchip/clk_rk3399.c >> +++ b/drivers/clk/rockchip/clk_rk3399.c >> @@ -926,6 +926,26 @@ static ulong rk3399_saradc_set_clk(struct rockchip_cru *cru, uint hz) >> return rk3399_saradc_get_clk(cru); >> } >> >> +static ulong rk3399_pciephy_get_clk(struct rockchip_cru *cru) >> +{ >> + if (readl(&cru->clksel_con[18]) & BIT(10)) >> + return 100 * MHz; >> + else >> + return OSC_HZ; > > Could avoid the else since all if blocks return, no other logic than the > one matching the else can reach that part of the code. > > Therefore: > > """ > if (readl(&cru->clksel_con[18]) & BIT(10)) > return 100 * MHz; > > return OSC_HZ; > """ > > works just as well. I was considering above format at first, but chose to align return statements for some reason. I can change to this format in a v3, if needed :-) > > Could also be > > """ > return (readl(&cru->clksel_con[18]) & BIT(10)) ? 100 * MHz : OSC_HZ; > """ > > [...] >> +static int __maybe_unused rk3399_pciephy_set_parent(struct clk *clk, >> + struct clk *parent) >> +{ >> + struct rk3399_clk_priv *priv = dev_get_priv(clk->dev); >> + const char *clock_output_name; >> + int ret; >> + >> + if (parent->dev == clk->dev && parent->id == SCLK_PCIEPHY_REF100M) { >> + rk_setreg(&priv->cru->clksel_con[18], BIT(10)); >> + return 0; >> + } >> + >> + ret = dev_read_string_index(parent->dev, "clock-output-names", >> + parent->id, &clock_output_name); > > Are you sure this works? It should work for the clk reference I tested, <&xin24m>, where the id will be 0, it should also work for e.g. <&rk809 1>, id will be 1. And if a &cru/&pmucru clk is referenced those nodes do not have the clock-output-names prop, so ret should be -EINVAL (or -ENODATA). > > Considering that parent->id seems to store unique ids, like 167 for > SCLK_PCIEPHY_REF100M, I doubt we should be using it for > dev_read_string_index???? > > As per documentation: > > """ > /** > * dev_read_string_index() - obtain an indexed string from a string list > * > * @dev: device to examine > * @propname: name of the property containing the string list > * @index: index of the string to return > * @outp: return location for the string > * > * Return: > * length of string, if found or -ve error value if not found > */ > int dev_read_string_index(const struct udevice *dev, const char *propname, > int index, const char **outp); > """ > > So index here means the (index+1)'th string in the list of strings under > the "propname" DT property, I doubt we have properties with 167+ strings > in them? I would expect the function to return -EINVAL or -ENODATA if it is called with an out-of-bounds index value. > > I realize that rk3399_gmac_set_parent also uses this, so I'm a bit > puzzled right now... > > Don't you want to use > > dev_read_stringlist_search(parent->dev, "clock-output-names", "xin24m") > > instead? I think the implementation is correct and is what other rk clk drivers do for gmac set parent ops. Using dev_read_stringlist_search() could possible match a name for "wrong" clock, e.g: clk_ref: clock { #clock-cells = <1>; clock-output-names = "xin32k", "xin24m"; }; here only <&clk_ref 1> should match and not <&clk_ref 0>. Regards, Jonas > > The rest looks good to me. > > Cheers, > Quentin
Hi Jonas, On 5/6/24 5:17 PM, Jonas Karlman wrote: > Hi Quentin, > > On 2024-05-06 13:07, Quentin Schulz wrote: >> Hi Jonas, >> >> On 5/1/24 6:22 PM, Jonas Karlman wrote: >>> rk3399-nanopi-4.dtsi try to set parent of and set rate to 100 MHz of the >>> SCLK_PCIEPHY_REF clock. >>> >>> The existing enable/disable ops for SCLK_PCIEPHY_REF currently force >>> use of 24 MHz parent and rate. >>> >>> Add improved support for setting parent and rate of the pciephy refclk >>> to driver to better support assign-clock props for pciephy refclk in DT. >>> >>> This limited implementation only support setting 24 or 100 MHz rate, >>> and expect npll and clk_pciephy_ref100m divider to use default values. >>> >>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se> >>> --- >>> v2: Implement partial instead of dummy support for SCLK_PCIEPHY_REF >>> --- >>> drivers/clk/rockchip/clk_rk3399.c | 59 +++++++++++++++++++++++++++++-- >>> 1 file changed, 57 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/clk/rockchip/clk_rk3399.c b/drivers/clk/rockchip/clk_rk3399.c >>> index 5934771b4096..0b3223611a32 100644 >>> --- a/drivers/clk/rockchip/clk_rk3399.c >>> +++ b/drivers/clk/rockchip/clk_rk3399.c >>> @@ -926,6 +926,26 @@ static ulong rk3399_saradc_set_clk(struct rockchip_cru *cru, uint hz) >>> return rk3399_saradc_get_clk(cru); >>> } >>> >>> +static ulong rk3399_pciephy_get_clk(struct rockchip_cru *cru) >>> +{ >>> + if (readl(&cru->clksel_con[18]) & BIT(10)) >>> + return 100 * MHz; >>> + else >>> + return OSC_HZ; >> >> Could avoid the else since all if blocks return, no other logic than the >> one matching the else can reach that part of the code. >> >> Therefore: >> >> """ >> if (readl(&cru->clksel_con[18]) & BIT(10)) >> return 100 * MHz; >> >> return OSC_HZ; >> """ >> >> works just as well. > > I was considering above format at first, but chose to align return > statements for some reason. > > I can change to this format in a v3, if needed :-) > Just a matter of taste :) >> >> Could also be >> >> """ >> return (readl(&cru->clksel_con[18]) & BIT(10)) ? 100 * MHz : OSC_HZ; >> """ >> >> [...] >>> +static int __maybe_unused rk3399_pciephy_set_parent(struct clk *clk, >>> + struct clk *parent) >>> +{ >>> + struct rk3399_clk_priv *priv = dev_get_priv(clk->dev); >>> + const char *clock_output_name; >>> + int ret; >>> + >>> + if (parent->dev == clk->dev && parent->id == SCLK_PCIEPHY_REF100M) { >>> + rk_setreg(&priv->cru->clksel_con[18], BIT(10)); >>> + return 0; >>> + } >>> + >>> + ret = dev_read_string_index(parent->dev, "clock-output-names", >>> + parent->id, &clock_output_name); >> >> Are you sure this works? > > It should work for the clk reference I tested, <&xin24m>, where the id > will be 0, it should also work for e.g. <&rk809 1>, id will be 1. > > And if a &cru/&pmucru clk is referenced those nodes do not have the > clock-output-names prop, so ret should be -EINVAL (or -ENODATA). > >> >> Considering that parent->id seems to store unique ids, like 167 for >> SCLK_PCIEPHY_REF100M, I doubt we should be using it for >> dev_read_string_index???? >> >> As per documentation: >> >> """ >> /** >> * dev_read_string_index() - obtain an indexed string from a string list >> * >> * @dev: device to examine >> * @propname: name of the property containing the string list >> * @index: index of the string to return >> * @outp: return location for the string >> * >> * Return: >> * length of string, if found or -ve error value if not found >> */ >> int dev_read_string_index(const struct udevice *dev, const char *propname, >> int index, const char **outp); >> """ >> >> So index here means the (index+1)'th string in the list of strings under >> the "propname" DT property, I doubt we have properties with 167+ strings >> in them? > > I would expect the function to return -EINVAL or -ENODATA if it is called > with an out-of-bounds index value. > -ENODATA it seems. -EINVAL if the property doesn't exist. >> >> I realize that rk3399_gmac_set_parent also uses this, so I'm a bit >> puzzled right now... >> >> Don't you want to use >> >> dev_read_stringlist_search(parent->dev, "clock-output-names", "xin24m") >> >> instead? > > I think the implementation is correct and is what other rk clk drivers > do for gmac set parent ops. > > Using dev_read_stringlist_search() could possible match a name for > "wrong" clock, e.g: > > clk_ref: clock { > #clock-cells = <1>; > clock-output-names = "xin32k", "xin24m"; > }; > > here only <&clk_ref 1> should match and not <&clk_ref 0>. > Ah well, one more brain fart :) I somehow mixed things up and was thinking about the values in clocks/assigned-clocks in one node needing to match the offset in clock-names of that same node, which made no sense. But we're talking about different things here :) I guess it's just a matter of taste between the implementation in your patch and the inverted logic which tries to find xin24m in clock-output-names and checks its index matches parent->id. Consistency is preferred, and the former is already used in other places, so let's keep this. Thanks for taking the time to explain. This does seem reasonable to me, so: Reviewed-by: Quentin Schulz <quentin.schulz@cherry.de> Thanks! Quentin
diff --git a/drivers/clk/rockchip/clk_rk3399.c b/drivers/clk/rockchip/clk_rk3399.c index 5934771b4096..0b3223611a32 100644 --- a/drivers/clk/rockchip/clk_rk3399.c +++ b/drivers/clk/rockchip/clk_rk3399.c @@ -926,6 +926,26 @@ static ulong rk3399_saradc_set_clk(struct rockchip_cru *cru, uint hz) return rk3399_saradc_get_clk(cru); } +static ulong rk3399_pciephy_get_clk(struct rockchip_cru *cru) +{ + if (readl(&cru->clksel_con[18]) & BIT(10)) + return 100 * MHz; + else + return OSC_HZ; +} + +static ulong rk3399_pciephy_set_clk(struct rockchip_cru *cru, uint hz) +{ + if (hz == 100 * MHz) + rk_setreg(&cru->clksel_con[18], BIT(10)); + else if (hz == OSC_HZ) + rk_clrreg(&cru->clksel_con[18], BIT(10)); + else + return -EINVAL; + + return rk3399_pciephy_get_clk(cru); +} + static ulong rk3399_clk_get_rate(struct clk *clk) { struct rk3399_clk_priv *priv = dev_get_priv(clk->dev); @@ -967,6 +987,9 @@ static ulong rk3399_clk_get_rate(struct clk *clk) case SCLK_SARADC: rate = rk3399_saradc_get_clk(priv->cru); break; + case SCLK_PCIEPHY_REF: + rate = rk3399_pciephy_get_clk(priv->cru); + break; case ACLK_VIO: case ACLK_HDCP: case ACLK_GIC_PRE: @@ -1058,6 +1081,9 @@ static ulong rk3399_clk_set_rate(struct clk *clk, ulong rate) case SCLK_SARADC: ret = rk3399_saradc_set_clk(priv->cru, rate); break; + case SCLK_PCIEPHY_REF: + ret = rk3399_pciephy_set_clk(priv->cru, rate); + break; case ACLK_VIO: case ACLK_HDCP: case ACLK_GIC_PRE: @@ -1108,12 +1134,39 @@ static int __maybe_unused rk3399_gmac_set_parent(struct clk *clk, return -EINVAL; } +static int __maybe_unused rk3399_pciephy_set_parent(struct clk *clk, + struct clk *parent) +{ + struct rk3399_clk_priv *priv = dev_get_priv(clk->dev); + const char *clock_output_name; + int ret; + + if (parent->dev == clk->dev && parent->id == SCLK_PCIEPHY_REF100M) { + rk_setreg(&priv->cru->clksel_con[18], BIT(10)); + return 0; + } + + ret = dev_read_string_index(parent->dev, "clock-output-names", + parent->id, &clock_output_name); + if (ret < 0) + return -ENODATA; + + if (!strcmp(clock_output_name, "xin24m")) { + rk_clrreg(&priv->cru->clksel_con[18], BIT(10)); + return 0; + } + + return -EINVAL; +} + static int __maybe_unused rk3399_clk_set_parent(struct clk *clk, struct clk *parent) { switch (clk->id) { case SCLK_RMII_SRC: return rk3399_gmac_set_parent(clk, parent); + case SCLK_PCIEPHY_REF: + return rk3399_pciephy_set_parent(clk, parent); } debug("%s: unsupported clk %ld\n", __func__, clk->id); @@ -1204,7 +1257,8 @@ static int rk3399_clk_enable(struct clk *clk) rk_clrreg(&priv->cru->clkgate_con[13], BIT(7)); break; case SCLK_PCIEPHY_REF: - rk_clrreg(&priv->cru->clksel_con[18], BIT(10)); + if (readl(&priv->cru->clksel_con[18]) & BIT(10)) + rk_clrreg(&priv->cru->clkgate_con[12], BIT(6)); break; default: debug("%s: unsupported clk %ld\n", __func__, clk->id); @@ -1298,7 +1352,8 @@ static int rk3399_clk_disable(struct clk *clk) rk_setreg(&priv->cru->clkgate_con[13], BIT(7)); break; case SCLK_PCIEPHY_REF: - rk_clrreg(&priv->cru->clksel_con[18], BIT(10)); + if (readl(&priv->cru->clksel_con[18]) & BIT(10)) + rk_setreg(&priv->cru->clkgate_con[12], BIT(6)); break; default: debug("%s: unsupported clk %ld\n", __func__, clk->id);
rk3399-nanopi-4.dtsi try to set parent of and set rate to 100 MHz of the SCLK_PCIEPHY_REF clock. The existing enable/disable ops for SCLK_PCIEPHY_REF currently force use of 24 MHz parent and rate. Add improved support for setting parent and rate of the pciephy refclk to driver to better support assign-clock props for pciephy refclk in DT. This limited implementation only support setting 24 or 100 MHz rate, and expect npll and clk_pciephy_ref100m divider to use default values. Signed-off-by: Jonas Karlman <jonas@kwiboo.se> --- v2: Implement partial instead of dummy support for SCLK_PCIEPHY_REF --- drivers/clk/rockchip/clk_rk3399.c | 59 +++++++++++++++++++++++++++++-- 1 file changed, 57 insertions(+), 2 deletions(-)