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