Re: [LINUX PATCH v2 1/3] clocksource: timer-cadence-ttc: Do not probe TTC device configured as PWM

[Michal Simek <michal.simek@amd.com>]

On 11/24/23 12:35, Krzysztof Kozlowski wrote:
> On 24/11/2023 12:03, Sayyed, Mubin wrote:
>> Hi Krzysztof,
>>
>>> -----Original Message-----
>>> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>> Sent: Wednesday, November 15, 2023 5:41 PM
>>> To: Sayyed, Mubin <mubin.sayyed@amd.com>
>>> Cc: linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
>>> devicetree@vger.kernel.org; linux-pwm@vger.kernel.org; git (AMD-Xilinx)
>>> <git@amd.com>; mubin10@gmail.com; krzysztof.kozlowski+dt@linaro.org;
>>> u.kleine-koenig@pengutronix.de; thierry.reding@gmail.com;
>>> robh+dt@kernel.org; conor+dt@kernel.org; tglx@linutronix.de;
>>> daniel.lezcano@linaro.org; Simek, Michal <michal.simek@amd.com>
>>> Subject: Re: [LINUX PATCH v2 1/3] clocksource: timer-cadence-ttc: Do not probe
>>> TTC device configured as PWM
>>>
>>> On 15/11/2023 06:55, Sayyed, Mubin wrote:
>>>>>> +	/*
>>>>>> +	 * If pwm-cells property is present in TTC node,
>>>>>> +	 * it would be treated as PWM device.
>>>>>> +	 */
>>>>>> +	if (of_property_read_bool(timer, "#pwm-cells"))
>>>>>> +		return -ENODEV;
>>>>>
>>>>> You will introduce dmesg errors, so regressions.
>>>>>
>>>> [Mubin]: I will change it to "return 0" to avoid dmesg errors.
>>>
>>> No, because solution is wrong.
>>>
>>>>
>>>>> This does not look right. What you want is to bind one device driver
>>>>> and choose different functionality based on properties.
>>>> [Mubin]:  I am doing it based on earlier discussion related to AXI Timer PWM
>>> driver.  It was suggested to use #pwm-cells property for identifying role of
>>> device(PWM/clocksource) https://lore.kernel.org/linux-
>>> devicetree/20210513021631.GA878860@robh.at.kernel.org/.
>>>
>>> You are mixing bindings with driver. I said here about driver and yes - you must
>>> use pwm-cells to differentiate that. It's obvious.
>>>
>>> So again, one driver binding.
>> [Mubin]: I will explore whether mfd framework can be used to handle this.
> 
> You do not need MFD for this, because you do not have a really MFD. This
> is just one device, so I expect here one driver. Why do you need
> multiple drivers (which also would solve that problem but why?)?

this driver is following pattern which is xps-timer (soff IP)
Documentation/devicetree/bindings/timer/xlnx,xps-timer.yaml

which has two drivers in the kernel.
On for clocksource
arch/microblaze/kernel/timer.c
and pwm one
drivers/pwm/pwm-xilinx.c

clocksource driver will be at some point moved to drivers/clocksource because 
that's what will be used in connection to MicroBlaze V.

I have looked at TTC and functionality wise it is related to
Documentation/devicetree/bindings/mfd/st,stm32-timers.yaml
or
Documentation/devicetree/bindings/mfd/st,stm32-lptimer.yaml

which are based on MFD.
Timer there is only clockevent not clocksource but it shouldn't really matter.

The biggest issue what I see is that ttc clocksource driver is used on arm32 
Zynq family for a lot of years. It means moving to different binding based on 
mfd would require keeping support for old dt binding too.
That would be from my point of view thing to start with. What do you think what 
would be the best way forward?

But I need to do my homework first to see what functionality that IP has but I 
am quite sure there could be at least multiple PMWs.

Thanks,
Michal