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

[Quentin Schulz]

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 <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 :-)
> 

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 <quentin.schulz at cherry.de>

Thanks!
Quentin