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.
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
