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 <[email protected]>
---
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 <[email protected]>
Thanks!
Quentin
