Re: [PATCH V4] clk: vc5: Add support for IDT VersaClock 5P49V5923 and 5P49V5933

[Marek Vasut <marek.vasut@gmail.com>]

On 01/11/2017 05:42 PM, Laurent Pinchart wrote:
> Hi Marek,

Hi!

> On Wednesday 11 Jan 2017 16:53:53 Marek Vasut wrote:
>> On 01/11/2017 04:41 PM, Laurent Pinchart wrote:
>>> On Wednesday 11 Jan 2017 15:37:11 Marek Vasut wrote:
>>>> On 01/10/2017 07:50 PM, Marek Vasut wrote:
>>>>> Add driver for IDT VersaClock 5 5P49V5923 and 5P49V5933 chips. These
>>>>> chips have two clock inputs, XTAL or CLK, which are muxed into single
>>>>> PLL/VCO input. In case of 5P49V5923, the XTAL in built into the chip
>>>>> while the 5P49V5923 requires external XTAL.
>>>>>
>>>>> The PLL feeds two fractional dividers. Each fractional divider feeds
>>>>> output mux, which allows selecting between clock from the fractional
>>>>> divider itself or from output mux on output N-1. In case of output
>>>>> mux 0, the output N-1 is instead connected to the output from the mux
>>>>> feeding the PLL.
>>>>>
>>>>> The driver thus far supports only the 5P49V5923 and 5P49V5933, while
>>>>> it should be easily extensible to the whole 5P49V59xx family of chips
>>>>> as they are all pretty similar.
>>>>>
>>>>> Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
>>>>> Cc: Michael Turquette <mturquette@baylibre.com>
>>>>> Cc: Stephen Boyd <sboyd@codeaurora.org>
> 
> [snip]
> 
>>>>> diff --git a/drivers/clk/clk-versaclock5.c
>>>>> b/drivers/clk/clk-versaclock5.c
>>>>> new file mode 100644
>>>>> index 000000000000..14f518e84d6d
>>>>> --- /dev/null
>>>>> +++ b/drivers/clk/clk-versaclock5.c
> 
> [snip]
> 
>>>>> +static long vc5_mux_round_rate(struct clk_hw *hw, unsigned long rate,
>>>>> +			       unsigned long *parent_rate)
>>>>> +{
>>>>> +	unsigned long idiv;
>>>>> +
>>>>> +	/* PLL cannot operate with input clock above 50 MHz. */
>>>>> +	if (rate > 50000000)
>>>>> +		return -EINVAL;
>>>
>>> As this is a PLL constraint, shouldn't it be enforced by
>>> vc5_pll_round_rate() instead ?
>>
>> vc5_pll_round_rate can only enforce PLL output frequency, not input
>> frequency AFAICT .
> 
> vc5_pll_round_rate() can modify the parent rate, that seems a good way to 
> enforce the requirement to me :-) On the other hand I've just realized that 
> there's an output directly wired to the mux, in which case the mux output rate 
> can be configured without taking the PLL into account. This probably calls for 
> enforcing the 50MHz requirement here.
> 
> How do you envision this all to work ? If the user modifies the OUT0_I2C_SELB 
> frequency, the mux divisor will be modified. Does the PLL then automatically 
> recalculate its divisor to keep the same output frequency ? Doesn't the PLL 
> input frequency get modified at least temporarily ?

This has to trigger the PLL rate recalculation (to keep in in the bounds
of supported frequencies) and propagate that down the clock
tree (to the FODs), yes.

>>>>> +	/* CLKIN within range of PLL input, feed directly to PLL. */
>>>>> +	if (*parent_rate <= 50000000)
>>>>> +		return *parent_rate;
>>>>> +
>>>>> +	idiv = DIV_ROUND_UP(*parent_rate, rate);
>>>>> +	if (idiv > 127)
>>>>> +		return -EINVAL;
>>>>> +
>>>>> +	return *parent_rate / idiv;
>>>>> +}
>>
>> [...]
>>
>>>>> +static struct clk_hw *
>>>>> +vc5_of_clk_get(struct of_phandle_args *clkspec, void *data)
>>>>> +{
>>>>> +	struct vc5_driver_data *vc5 = data;
>>>>> +	unsigned int idx = clkspec->args[0];
>>>>> +
>>>>> +	if (idx > 2) {
>>>>> +		pr_err("%s: invalid index %u\n", __func__, idx);
>>>
>>> Does this deserve an error message ?
>>
>> I think it does, the driver will yell if you use incorrect index in the
>> bindings.
> 
> Shouldn't it be the caller that complains ? I'd rather have an error message 
> in the CCF core than duplicating it in all clock drivers.

Good point, dropped.

>>>>> +		return ERR_PTR(-EINVAL);
>>>>> +	}
>>>>> +
>>>>> +	return &vc5->clk_out[idx].hw;
>>>>> +}
> 


-- 
Best regards,
Marek Vasut