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

[Jonas Karlman]

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

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