[PATCH v2 03/22] clk: rockchip: rk3399: Improve support for SCLK_PCIEPHY_REF clock

[Quentin Schulz]

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