Re: [RFC PATCH v4] media: v4l2-common: Add a helper for obtaining the clock producer

[Hans de Goede <hdegoede@redhat.com>]

Hi Mehdi,

On 14-May-25 10:25 AM, Mehdi Djait wrote:
> Hello Hans,
> 
> thank you for the comment.
> 
> On Sat, May 10, 2025 at 04:21:09PM +0200, Hans de Goede wrote:
>> Hi Mehdi,
>>
>> On 21-Mar-25 2:03 PM, Mehdi Djait wrote:
>>> Introduce a helper for v4l2 sensor drivers on both DT- and ACPI-based
>>> platforms to retrieve a reference to the clock producer from firmware.
>>>
>>> This helper behaves the same as clk_get_optional() except where there is
>>> no clock producer like in ACPI-based platforms.
>>>
>>> For ACPI-based platforms the function will read the "clock-frequency"
>>> ACPI _DSD property and register a fixed frequency clock with the frequency
>>> indicated in the property.
>>>
>>> Signed-off-by: Mehdi Djait <mehdi.djait@linux.intel.com>
>>
>> This certainly looks quite useful, thank you for working
>> on this.
>>
>> Note on some IPU3 platforms where the clk is provided by
>> a clk-generator which is part of a special sensor-PMIC
>> the situation is a bit more complicated.
>>
>> Basically if there is both a clk provider and a clock-frequency
>> property then the clock-frequency value should be set as freq
>> to the clk-provider, see:
>>
> 
> is it even possible to get a reference to the clock producer in ACPI
> systems or am I missing something here ?

Yes in some special cases it is possible to get a reference to
a clock provider on ACPI. E.g. one is provided by:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/platform/x86/intel/int3472/tps68470.c

on x86 ACPI systems using that sensor PMIC such as the
"Microsoft Surface Go" and "Microsoft Surface Go 2"

> Here is what I gathered online for the discussion leading to this patch:
> ----------------------------------------------------------------------------------------------------------------------------------------------
> ClockInput resource added to ACPI v6.5: https://uefi.org/specs/ACPI/6.5/19_ASL_Reference.html#clockinput-clock-input-resource-descriptor-macro
> - commit adding ClockInput resource to acpica: https://github.com/acpica/acpica/commit/661feab5ee01a34af95a389a18c82e79f1aba05a
> - commit kernel upstream: 520d4a0ee5b6d9c7a1258ace6caa13a94ac35ef8 "ACPICA: add support
>   for ClockInput resource (v6.5)"

Ah I see where the confusion is coming from, the clk-provider does not come
directly from ACPI, it comes from the PMIC driver and the PMIC driver also
adds a clk-lookup table entry to associate it with the PMIC.

> this does not mean we can use it: I found this out-of-tree patch to supports fixed clock sources
> https://github.com/niyas-sait/linux-acpi/blob/main/0001-acpi-add-clock-bindings-for-fixed-clock-resources.patch
> it was not sent to the acpi mailing list. It was mentioned in this
> dicussion: https://lore.kernel.org/linux-kernel/78763d69bae04204b2af37201b09f8b5@huawei.com/
>
> Another interesting link: https://linaro.atlassian.net/wiki/spaces/CLIENTPC/pages/28822175758/ACPI+Clock+Input+Resources
> ----------------------------------------------------------------------------------------------------------------------------------------------
> 
> link for the dicussion: https://lore.kernel.org/linux-media/20250220154909.152538-1-mehdi.djait@linux.intel.com/

These 2 links are not relevant, the clk-provider is not directly coming from
ACPI instead the clk is registered by the PMIC driver for the clk-generator
part of the PMIC.


>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/media/i2c/ov8865.c#n3020
>>
>> for an example of a driver which handles this case.
> 
> So if I understood the above correctly: in the ov8865 IPU3/ACPI case:
> 
> 1) sensor->extclk is NULL because the clock producer is not available in
> ACPI systems. ClockInput() ACPI resource was introduced to acpica after
> the ov8865 patch and the resource is not even being used in the upstream kernel.

In this specific case it will be not NULL because the PMIC driver
provides a clk-provider and creates a clk-lookup to match that
to the ov8865 sensor.

> 2) the sensor->extclk_rate will be set from reading
> the clock-frequency _DSD property in:
> 
> 	ret = fwnode_property_read_u32(dev_fwnode(dev), "clock-frequency", &rate);

This bit is correct, the special thing here is that the PMIC
clk-provider is programmable so the sensor-driver needs to
set it to the rate returned by reading "clock-frequency"/

So basically first call both:

1. devm_get_clk()
2. fwnode_property_read_u32(dev_fwnode(dev), "clock-frequency", &rate);

and in the special case when *both* succeed do a clk_set_rate()
call on the returned clk setting it to "rate".

Regards,

Hans