[LINUX PATCH v2 0/3] Add initial support for TTC PWM driver

[LINUX PATCH v2 0/3] Add initial support for TTC PWM driver

[Mubin Sayyed]

Adds initial TTC PWM driver. New driver would use same compatible string
as that of drivers/clocksource/timer-cadence-ttc.c.

Specific TTC device would be identified as clocksource/clockeven or PWM
device based on the pwm-cells poperty. Clocksource TTC driver checks for
pwm-cells property, if found, it just returns without acting on it.

link for v1:
https://lore.kernel.org/lkml/DM4PR12MB5938774A495A246EE5557BEF9DC69@DM4PR12MB5938.namprd12.prod.outlook.com/T/

Mubin Sayyed (3):
  clocksource: timer-cadence-ttc: Do not probe TTC device configured as
    PWM
  dt-bindings: timer: Add bindings for TTC PWM
  pwm: pwm-cadence: Add support for TTC PWM

 .../devicetree/bindings/timer/cdns,ttc.yaml   |  22 +-
 drivers/clocksource/timer-cadence-ttc.c       |   7 +
 drivers/pwm/Kconfig                           |  11 +
 drivers/pwm/Makefile                          |   1 +
 drivers/pwm/pwm-cadence.c                     | 370 ++++++++++++++++++
 5 files changed, 410 insertions(+), 1 deletion(-)
 create mode 100644 drivers/pwm/pwm-cadence.c

-- 
2.25.1

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

[Mubin Sayyed]

TTC device can act either as clocksource/clockevent or
PWM generator, it would be decided by pwm-cells property.
TTC PWM feature would be supported through separate driver
based on PWM framework.

If pwm-cells property is present in TTC node, it would be
treated as PWM device, and clocksource driver should just
skip it.

Signed-off-by: Mubin Sayyed <mubin.sayyed@amd.com>
---
Changes for v2:
    - Added comment regarding pwm-cells property
---
 drivers/clocksource/timer-cadence-ttc.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/clocksource/timer-cadence-ttc.c b/drivers/clocksource/timer-cadence-ttc.c
index 32daaac9b132..f8fcb1a4bdd0 100644
--- a/drivers/clocksource/timer-cadence-ttc.c
+++ b/drivers/clocksource/timer-cadence-ttc.c
@@ -477,6 +477,13 @@ static int __init ttc_timer_probe(struct platform_device *pdev)
 	u32 timer_width = 16;
 	struct device_node *timer = pdev->dev.of_node;
 
+	/*
+	 * 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;
+
 	if (initialized)
 		return 0;
 
-- 
2.25.1

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

[Krzysztof Kozlowski]

On 14/11/2023 13:47, Mubin Sayyed wrote:
> TTC device can act either as clocksource/clockevent or
> PWM generator, it would be decided by pwm-cells property.
> TTC PWM feature would be supported through separate driver
> based on PWM framework.
> 
> If pwm-cells property is present in TTC node, it would be
> treated as PWM device, and clocksource driver should just
> skip it.
> 
> Signed-off-by: Mubin Sayyed <mubin.sayyed@amd.com>
> ---
> Changes for v2:
>     - Added comment regarding pwm-cells property
> ---
>  drivers/clocksource/timer-cadence-ttc.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/clocksource/timer-cadence-ttc.c b/drivers/clocksource/timer-cadence-ttc.c
> index 32daaac9b132..f8fcb1a4bdd0 100644
> --- a/drivers/clocksource/timer-cadence-ttc.c
> +++ b/drivers/clocksource/timer-cadence-ttc.c
> @@ -477,6 +477,13 @@ static int __init ttc_timer_probe(struct platform_device *pdev)
>  	u32 timer_width = 16;
>  	struct device_node *timer = pdev->dev.of_node;
>  
> +	/*
> +	 * 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.

This does not look right. What you want is to bind one device driver and
choose different functionality based on properties.

Best regards,
Krzysztof

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

[Sayyed, Mubin]

Hi,

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: Wednesday, November 15, 2023 2:41 AM
> To: Sayyed, Mubin <mubin.sayyed@amd.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>
> 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
> Subject: Re: [LINUX PATCH v2 1/3] clocksource: timer-cadence-ttc: Do not
> probe TTC device configured as PWM
> 
> On 14/11/2023 13:47, Mubin Sayyed wrote:
> > TTC device can act either as clocksource/clockevent or PWM generator,
> > it would be decided by pwm-cells property.
> > TTC PWM feature would be supported through separate driver based on
> > PWM framework.
> >
> > If pwm-cells property is present in TTC node, it would be treated as
> > PWM device, and clocksource driver should just skip it.
> >
> > Signed-off-by: Mubin Sayyed <mubin.sayyed@amd.com>
> > ---
> > Changes for v2:
> >     - Added comment regarding pwm-cells property
> > ---
> >  drivers/clocksource/timer-cadence-ttc.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/drivers/clocksource/timer-cadence-ttc.c
> > b/drivers/clocksource/timer-cadence-ttc.c
> > index 32daaac9b132..f8fcb1a4bdd0 100644
> > --- a/drivers/clocksource/timer-cadence-ttc.c
> > +++ b/drivers/clocksource/timer-cadence-ttc.c
> > @@ -477,6 +477,13 @@ static int __init ttc_timer_probe(struct
> platform_device *pdev)
> >  	u32 timer_width = 16;
> >  	struct device_node *timer = pdev->dev.of_node;
> >
> > +	/*
> > +	 * 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.

> 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/. 
	
Thanks,
Mubin
> 
> Best regards,
> Krzysztof

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

[Krzysztof Kozlowski]

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.

Wrap your emails to mailing list discussion style.

Best regards,
Krzysztof

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

[Sayyed, Mubin]

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.

Thanks,
Mubin
> 
> Wrap your emails to mailing list discussion style.
> 
> Best regards,
> Krzysztof

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

[Krzysztof Kozlowski]

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

Best regards,
Krzysztof

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

[Michal Simek]

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

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

[Sayyed, Mubin]

Hi,

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: Friday, November 24, 2023 5:06 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 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?)?
Cadence TTC IP can be used as timer(clocksource/clockevent) and PWM device.
We have drivers/clocksource/timer-cadence-ttc.c for clocksource/clockevent functionality. 
New driver for PWM functionality will be added to drivers/pwm/pwm-cadence.c (3/3 of this
Series).  In given SoC,  multiple instances of TTC IP are possible(ZynqMP  Ultrscale SoC has 4
Instances), few of them could be configured as clocksource/clockevent devices and others
as PWM ones. So,  cloksource as well as PWM drivers for cadence TTC IP would be enabled in 
the kernel. 

Now in this scenario, each TTC device would be matching with 2 drivers, clocksource and PWM, since
compatible string is same.  If I don’t add #pwm-cells checking in clocksource driver and return 
-ENODEV based on that, each device would always bind with clocksource driver. PWM driver 
would never probe since clocksource driver probes ahead of PWM one in probing order.

I am exploring mfd to deal with said scenario. Do you see any better way to handle this? 

Thanks,
Mubin

> 
> Best regards,
> Krzysztof

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

[Krzysztof Kozlowski]

On 24/11/2023 13:07, Sayyed, Mubin wrote:

>>>>>> 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?)?
> Cadence TTC IP can be used as timer(clocksource/clockevent) and PWM device.
> We have drivers/clocksource/timer-cadence-ttc.c for clocksource/clockevent functionality. 
> New driver for PWM functionality will be added to drivers/pwm/pwm-cadence.c (3/3 of this
> Series).  In given SoC,  multiple instances of TTC IP are possible(ZynqMP  Ultrscale SoC has 4
> Instances), few of them could be configured as clocksource/clockevent devices and others
> as PWM ones. So,  cloksource as well as PWM drivers for cadence TTC IP would be enabled in 
> the kernel. 
> 
> Now in this scenario, each TTC device would be matching with 2 drivers, clocksource and PWM, since
> compatible string is same.  If I don’t add #pwm-cells checking in clocksource driver and return 
> -ENODEV based on that, each device would always bind with clocksource driver. PWM driver 
> would never probe since clocksource driver probes ahead of PWM one in probing order.

None of these above explain why you need two drivers.

> 
> I am exploring mfd to deal with said scenario. Do you see any better way to handle this? 

You basically repeated previous sentence about MFD without answering.
Yeah, better way could be to have one driver. Why you cannot have it
that way?


Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 24/11/2023 17:24, Krzysztof Kozlowski wrote:
>>>>> 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?)?
>> Cadence TTC IP can be used as timer(clocksource/clockevent) and PWM device.
>> We have drivers/clocksource/timer-cadence-ttc.c for clocksource/clockevent functionality. 
>> New driver for PWM functionality will be added to drivers/pwm/pwm-cadence.c (3/3 of this
>> Series).  In given SoC,  multiple instances of TTC IP are possible(ZynqMP  Ultrscale SoC has 4
>> Instances), few of them could be configured as clocksource/clockevent devices and others
>> as PWM ones. So,  cloksource as well as PWM drivers for cadence TTC IP would be enabled in 
>> the kernel. 
>>
>> Now in this scenario, each TTC device would be matching with 2 drivers, clocksource and PWM, since
>> compatible string is same.  If I don’t add #pwm-cells checking in clocksource driver and return 
>> -ENODEV based on that, each device would always bind with clocksource driver. PWM driver 
>> would never probe since clocksource driver probes ahead of PWM one in probing order.
> 
> None of these above explain why you need two drivers.

And please do not answer to this with again: "I have two drivers...".

> 
>>
>> I am exploring mfd to deal with said scenario. Do you see any better way to handle this? 
> 
> You basically repeated previous sentence about MFD without answering.
> Yeah, better way could be to have one driver. Why you cannot have it
> that way?
> 
> 
> Best regards,
> Krzysztof
> 

Best regards,
Krzysztof

[LINUX PATCH v2 2/3] dt-bindings: timer: Add bindings for TTC PWM

[Mubin Sayyed]

Cadence TTC can act as PWM device, it is supported through
separate PWM framework based driver. Decision to configure
specific TTC device as PWM or clocksource/clockevent would
be done based on presence of "#pwm-cells" property.

Also, interrupt property is not required for TTC PWM driver.
Update bindings to support TTC PWM configuration.

Signed-off-by: Mubin Sayyed <mubin.sayyed@amd.com>
---
Changes for v2:
  Update subject
  Modify #pwm-cells to constant 3
  Update example to use generic name
---
 .../devicetree/bindings/timer/cdns,ttc.yaml   | 22 ++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/timer/cdns,ttc.yaml b/Documentation/devicetree/bindings/timer/cdns,ttc.yaml
index dbba780c9b02..da342464d32e 100644
--- a/Documentation/devicetree/bindings/timer/cdns,ttc.yaml
+++ b/Documentation/devicetree/bindings/timer/cdns,ttc.yaml
@@ -32,12 +32,23 @@ properties:
     description: |
       Bit width of the timer, necessary if not 16.
 
+  "#pwm-cells":
+    const: 3
+
 required:
   - compatible
   - reg
-  - interrupts
   - clocks
 
+allOf:
+  - if:
+      not:
+        required:
+          - "#pwm-cells"
+    then:
+      required:
+        - interrupts
+
 additionalProperties: false
 
 examples:
@@ -50,3 +61,12 @@ examples:
         clocks = <&cpu_clk 3>;
         timer-width = <32>;
     };
+
+  - |
+    pwm: pwm@f8002000 {
+        compatible = "cdns,ttc";
+        reg = <0xf8002000 0x1000>;
+        clocks = <&cpu_clk 3>;
+        timer-width = <32>;
+        #pwm-cells = <3>;
+    };
-- 
2.25.1

Re: [LINUX PATCH v2 2/3] dt-bindings: timer: Add bindings for TTC PWM

[Krzysztof Kozlowski]

On 14/11/2023 13:47, Mubin Sayyed wrote:
> Cadence TTC can act as PWM device, it is supported through
> separate PWM framework based driver. Decision to configure
> specific TTC device as PWM or clocksource/clockevent would
> be done based on presence of "#pwm-cells" property.

This is a friendly reminder during the review process.

It seems my or other reviewer's previous comments were not fully
addressed. Maybe the feedback got lost between the quotes, maybe you
just forgot to apply it. Please go back to the previous discussion and
either implement all requested changes or keep discussing them.

Thank you.

Missed comments: subject. I don't understand why Cadence was dropped,
but "bindings" stayed. Opposite of what I asked.

Rest looks good, so with above fixed:

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>


---

This is an automated instruction, just in case, because many review tags
are being ignored. If you know the process, you can skip it (please do
not feel offended by me posting it here - no bad intentions intended).
If you do not know the process, here is a short explanation:

Please add Acked-by/Reviewed-by/Tested-by tags when posting new
versions, under or above your Signed-off-by tag. Tag is "received", when
provided in a message replied to you on the mailing list. Tools like b4
can help here. However, there's no need to repost patches *only* to add
the tags. The upstream maintainer will do that for tags received on the
version they apply.

https://elixir.bootlin.com/linux/v6.5-rc3/source/Documentation/process/submitting-patches.rst#L577

Best regards,
Krzysztof

[LINUX PATCH v2 3/3] pwm: pwm-cadence: Add support for TTC PWM

[Mubin Sayyed]

Cadence TTC timer can be configured as clocksource/clockevent
or PWM device. Specific TTC device would be configured as PWM
device, if pwm-cells property is present in the device tree
node.

In case of Zynq, ZynqMP and Versal SoC's, each TTC device has 3
timers/counters, so maximum 3 PWM channels can be configured for
each TTC IP instance. Also, output of 0th PWM channel of each TTC
device can be routed to MIO or EMIO, and output of 2nd and 3rd
PWM channel can be routed only to EMIO.

Period for given PWM channel is configured through interval timer
and duty cycle through match counter.

Details for cadence TTC IP can be found in Zynq UltraScale+ TRM.

Signed-off-by: Mubin Sayyed <mubin.sayyed@amd.com>
---
Refer link given below for Zynq UltraScale+ TRM
https://docs.xilinx.com/r/en-US/ug1085-zynq-ultrascale-trm

Changes for v2:
 Use maybe_unused attribute for ttc_pwm_of_match_driver structure
 Add new function ttc_pwm_set_polarity
 Removed calls to pwm_get_state
 Replace DIV_ROUNF_CLOSEST with mul_u64_u64_div_u64
 Modify ttc_pwm_apply to remove while loop in prescalar logic
 and avoid glitch
 Calculate rate in probe and add it to private structure for further
 Drop ttc_pwm_of_xlate
 Replace of_clk_get with devm_clk_get_enabled
 Drop _OFFSET and _MASK from definitions
 Keep Kconfig and Makefile changes alphabetically sorted
 Use remove_new instead of remove
 Document limitations in driver file
---
 drivers/pwm/Kconfig       |  11 ++
 drivers/pwm/Makefile      |   1 +
 drivers/pwm/pwm-cadence.c | 370 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 382 insertions(+)
 create mode 100644 drivers/pwm/pwm-cadence.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 8ebcddf91f7b..7fd493f06496 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -152,6 +152,17 @@ config PWM_BRCMSTB
 	  To compile this driver as a module, choose M Here: the module
 	  will be called pwm-brcmstb.c.
 
+config PWM_CADENCE
+        tristate "Cadence PWM support"
+        depends on OF
+        depends on COMMON_CLK
+        help
+          Generic PWM framework driver for cadence TTC IP found on
+          Xilinx Zynq/ZynqMP/Versal SOCs. Each TTC device has 3 PWM
+          channels. Output of 0th PWM channel of each TTC device can
+          be routed to MIO or EMIO, and output of 1st and 2nd PWM
+          channels can be routed only to EMIO.
+
 config PWM_CLK
 	tristate "Clock based PWM support"
 	depends on HAVE_CLK || COMPILE_TEST
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index c822389c2a24..a8a11dbcb00f 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -11,6 +11,7 @@ obj-$(CONFIG_PWM_BCM_KONA)	+= pwm-bcm-kona.o
 obj-$(CONFIG_PWM_BCM2835)	+= pwm-bcm2835.o
 obj-$(CONFIG_PWM_BERLIN)	+= pwm-berlin.o
 obj-$(CONFIG_PWM_BRCMSTB)	+= pwm-brcmstb.o
+obj-$(CONFIG_PWM_CADENCE)	+= pwm-cadence.o
 obj-$(CONFIG_PWM_CLK)		+= pwm-clk.o
 obj-$(CONFIG_PWM_CLPS711X)	+= pwm-clps711x.o
 obj-$(CONFIG_PWM_CRC)		+= pwm-crc.o
diff --git a/drivers/pwm/pwm-cadence.c b/drivers/pwm/pwm-cadence.c
new file mode 100644
index 000000000000..12aaa004bf7f
--- /dev/null
+++ b/drivers/pwm/pwm-cadence.c
@@ -0,0 +1,370 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver to configure cadence TTC timer as PWM
+ * generator
+ *
+ * Limitations:
+ * - When PWM is stopped, timer counter gets stopped immediately. This
+ *   doesn't allow the current PWM period to complete and stops abruptly.
+ * - Disabled PWM emits inactive level.
+ * - When user requests a change in  any parameter of PWM (period/duty cycle/polarity)
+ *   while PWM is in enabled state:
+ *	- PWM is stopped abruptly.
+ *	- Requested parameter is changed.
+ *	- Fresh PWM cycle is started.
+ *
+ * Copyright (C) 2023, Advanced Micro Devices, Inc.
+ */
+
+#include <linux/clk.h>
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/of_address.h>
+
+#define TTC_CLK_CNTRL		0x00
+#define TTC_CNT_CNTRL		0x0C
+#define TTC_MATCH_CNT_VAL	0x30
+#define TTC_COUNT_VAL		0x18
+#define TTC_INTR_VAL		0x24
+#define TTC_ISR			0x54
+#define TTC_IER			0x60
+#define TTC_PWM_CHANNEL		0x4
+
+#define TTC_CLK_CNTRL_CSRC		BIT(5)
+#define TTC_CLK_CNTRL_PSV		GENMASK(4, 1)
+#define TTC_CLK_CNTRL_PS_EN		BIT(0)
+
+#define TTC_CNTR_CTRL_DIS		BIT(0)
+#define TTC_CNTR_CTRL_INTR_MODE_EN	BIT(1)
+#define TTC_CNTR_CTRL_MATCH_MODE_EN	BIT(3)
+#define TTC_CNTR_CTRL_RST		BIT(4)
+#define TTC_CNTR_CTRL_WAVE_EN	BIT(5)
+#define TTC_CNTR_CTRL_WAVE_POL	BIT(6)
+
+#define TTC_CNTR_CTRL_WAVE_POL_SHIFT	6
+#define TTC_CNTR_CTRL_PRESCALE_SHIFT	1
+#define TTC_PWM_MAX_CH			3
+
+/**
+ * struct ttc_pwm_priv - Private data for TTC PWM drivers
+ * @chip:	PWM chip structure representing PWM controller
+ * @clk:	TTC input clock
+ * @rate:	TTC input clock rate
+ * @max:	Maximum value of the counters
+ * @base:	Base address of TTC instance
+ */
+struct ttc_pwm_priv {
+	struct pwm_chip chip;
+	struct clk *clk;
+	unsigned long rate;
+	u32 max;
+	void __iomem *base;
+};
+
+static inline u32 ttc_pwm_readl(struct ttc_pwm_priv *priv,
+				unsigned long offset)
+{
+	return readl_relaxed(priv->base + offset);
+}
+
+static inline void ttc_pwm_writel(struct ttc_pwm_priv *priv,
+				  unsigned long offset,
+				  unsigned long val)
+{
+	writel_relaxed(val, priv->base + offset);
+}
+
+static inline u32 ttc_pwm_ch_readl(struct ttc_pwm_priv *priv,
+				   unsigned int chnum,
+				   unsigned long offset)
+{
+	unsigned long pwm_ch_offset = offset +
+				       (TTC_PWM_CHANNEL * chnum);
+
+	return ttc_pwm_readl(priv, pwm_ch_offset);
+}
+
+static inline void ttc_pwm_ch_writel(struct ttc_pwm_priv *priv,
+				     unsigned int chnum,
+				     unsigned long offset,
+				     unsigned long val)
+{
+	unsigned long pwm_ch_offset = offset +
+				       (TTC_PWM_CHANNEL * chnum);
+
+	ttc_pwm_writel(priv, pwm_ch_offset, val);
+}
+
+static inline struct ttc_pwm_priv *xilinx_pwm_chip_to_priv(struct pwm_chip *chip)
+{
+	return container_of(chip, struct ttc_pwm_priv, chip);
+}
+
+static void ttc_pwm_enable(struct ttc_pwm_priv *priv, struct pwm_device *pwm)
+{
+	u32 ctrl_reg;
+
+	ctrl_reg = ttc_pwm_ch_readl(priv, pwm->hwpwm, TTC_CNT_CNTRL);
+	ctrl_reg |= (TTC_CNTR_CTRL_INTR_MODE_EN
+				 | TTC_CNTR_CTRL_MATCH_MODE_EN | TTC_CNTR_CTRL_RST);
+	ctrl_reg &= ~(TTC_CNTR_CTRL_DIS | TTC_CNTR_CTRL_WAVE_EN);
+	ttc_pwm_ch_writel(priv, pwm->hwpwm, TTC_CNT_CNTRL, ctrl_reg);
+}
+
+static void ttc_pwm_disable(struct ttc_pwm_priv *priv, struct pwm_device *pwm)
+{
+	u32 ctrl_reg;
+
+	ctrl_reg = ttc_pwm_ch_readl(priv, pwm->hwpwm, TTC_CNT_CNTRL);
+	ctrl_reg |= TTC_CNTR_CTRL_DIS;
+
+	ttc_pwm_ch_writel(priv, pwm->hwpwm, TTC_CNT_CNTRL, ctrl_reg);
+}
+
+static void ttc_pwm_set_polarity(struct ttc_pwm_priv *priv, struct pwm_device *pwm,
+				 enum pwm_polarity polarity)
+{
+	u32 ctrl_reg;
+
+	ctrl_reg = ttc_pwm_ch_readl(priv, pwm->hwpwm, TTC_CNT_CNTRL);
+
+	if (polarity == PWM_POLARITY_NORMAL)
+		ctrl_reg |= TTC_CNTR_CTRL_WAVE_POL;
+	else
+		ctrl_reg &= (~TTC_CNTR_CTRL_WAVE_POL);
+
+	ttc_pwm_ch_writel(priv, pwm->hwpwm, TTC_CNT_CNTRL, ctrl_reg);
+}
+
+static void ttc_pwm_set_counters(struct ttc_pwm_priv *priv,
+				 struct pwm_device *pwm,
+				 u32 period_cycles,
+				 u32 duty_cycles)
+{
+	/* Set up period */
+	ttc_pwm_ch_writel(priv, pwm->hwpwm, TTC_INTR_VAL, period_cycles);
+
+	/* Set up duty cycle */
+	ttc_pwm_ch_writel(priv, pwm->hwpwm, TTC_MATCH_CNT_VAL, duty_cycles);
+}
+
+static void ttc_pwm_set_prescalar(struct ttc_pwm_priv *priv,
+				  struct pwm_device *pwm,
+				  u32 div, bool is_enable)
+{
+	u32 clk_reg;
+
+	if (is_enable) {
+		/* Set up prescalar */
+		clk_reg = ttc_pwm_ch_readl(priv, pwm->hwpwm, TTC_CLK_CNTRL);
+		clk_reg &= ~TTC_CLK_CNTRL_PSV;
+		clk_reg |= (div << TTC_CNTR_CTRL_PRESCALE_SHIFT);
+		clk_reg |= TTC_CLK_CNTRL_PS_EN;
+		ttc_pwm_ch_writel(priv, pwm->hwpwm, TTC_CLK_CNTRL, clk_reg);
+	} else {
+		/* Disable prescalar */
+		clk_reg = ttc_pwm_ch_readl(priv, pwm->hwpwm, TTC_CLK_CNTRL);
+		clk_reg &= ~TTC_CLK_CNTRL_PS_EN;
+		ttc_pwm_ch_writel(priv, pwm->hwpwm, TTC_CLK_CNTRL, clk_reg);
+	}
+}
+
+static int ttc_pwm_apply(struct pwm_chip *chip,
+			 struct pwm_device *pwm,
+			 const struct pwm_state *state)
+{
+	struct ttc_pwm_priv *priv = xilinx_pwm_chip_to_priv(chip);
+	u64 duty_cycles, period_cycles;
+	struct pwm_state cstate;
+	unsigned long rate;
+	bool flag = false;
+	u32 div = 0;
+
+	cstate = pwm->state;
+
+	if (state->polarity != cstate.polarity) {
+		if (cstate.enabled)
+			ttc_pwm_disable(priv, pwm);
+
+		ttc_pwm_set_polarity(priv, pwm, state->polarity);
+	}
+
+	rate = priv->rate;
+
+	/* Prevent overflow by limiting to the maximum possible period */
+	period_cycles = min_t(u64, state->period, ULONG_MAX * NSEC_PER_SEC);
+	period_cycles = mul_u64_u64_div_u64(period_cycles, rate, NSEC_PER_SEC);
+
+	if (period_cycles > priv->max) {
+		/*
+		 * Prescale frequency to fit requested period cycles within limit.
+		 * Prescalar divides input clock by 2^(prescale_value + 1). Maximum
+		 * supported prescalar value is 15.
+		 */
+		div = mul_u64_u64_div_u64(state->period, rate, (NSEC_PER_SEC * priv->max));
+		div = order_base_2(div);
+		if (div)
+			div -= 1;
+
+		if (div > 15)
+			return -ERANGE;
+
+		rate = DIV_ROUND_CLOSEST(rate, BIT(div + 1));
+		period_cycles = mul_u64_u64_div_u64(state->period, rate,
+						    NSEC_PER_SEC);
+		flag = true;
+	}
+
+	if (cstate.enabled)
+		ttc_pwm_disable(priv, pwm);
+
+	duty_cycles = mul_u64_u64_div_u64(state->duty_cycle, rate,
+					  NSEC_PER_SEC);
+	ttc_pwm_set_counters(priv, pwm, period_cycles, duty_cycles);
+
+	ttc_pwm_set_prescalar(priv, pwm, div, flag);
+
+	if (state->enabled)
+		ttc_pwm_enable(priv, pwm);
+	else
+		ttc_pwm_disable(priv, pwm);
+
+	return 0;
+}
+
+static int ttc_pwm_get_state(struct pwm_chip *chip,
+			     struct pwm_device *pwm,
+			     struct pwm_state *state)
+{
+	struct ttc_pwm_priv *priv = xilinx_pwm_chip_to_priv(chip);
+	u32 value, pres_en, pres = 1;
+	unsigned long rate;
+	u64 tmp;
+
+	value = ttc_pwm_ch_readl(priv, pwm->hwpwm, TTC_CNT_CNTRL);
+
+	if (value & TTC_CNTR_CTRL_WAVE_POL)
+		state->polarity = PWM_POLARITY_NORMAL;
+	else
+		state->polarity = PWM_POLARITY_INVERSED;
+
+	if (value & TTC_CNTR_CTRL_DIS)
+		state->enabled = false;
+	else
+		state->enabled = true;
+
+	rate = priv->rate;
+
+	pres_en =  ttc_pwm_ch_readl(priv, pwm->hwpwm, TTC_CLK_CNTRL);
+	pres_en	&= TTC_CLK_CNTRL_PS_EN;
+
+	if (pres_en) {
+		pres = ttc_pwm_ch_readl(priv, pwm->hwpwm, TTC_CLK_CNTRL) & TTC_CLK_CNTRL_PSV;
+		pres >>= TTC_CNTR_CTRL_PRESCALE_SHIFT;
+		/* If prescale is enabled, the count rate is divided by 2^(pres + 1) */
+		pres = BIT(pres + 1);
+	}
+
+	tmp = ttc_pwm_ch_readl(priv, pwm->hwpwm, TTC_INTR_VAL);
+	tmp *= pres;
+	state->period = DIV64_U64_ROUND_UP(tmp * NSEC_PER_SEC, rate);
+
+	tmp = ttc_pwm_ch_readl(priv, pwm->hwpwm, TTC_MATCH_CNT_VAL);
+	tmp *= pres;
+	state->duty_cycle = DIV64_U64_ROUND_UP(tmp * NSEC_PER_SEC, rate);
+
+	return 0;
+}
+
+static const struct pwm_ops ttc_pwm_ops = {
+	.apply = ttc_pwm_apply,
+	.get_state = ttc_pwm_get_state,
+	.owner = THIS_MODULE,
+};
+
+static int ttc_pwm_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	u32 pwm_cells, timer_width;
+	struct ttc_pwm_priv *priv;
+	int ret;
+
+	/*
+	 * If pwm-cells property is not present in TTC node,
+	 * it would be treated as clocksource/clockevent
+	 * device.
+	 */
+	ret = of_property_read_u32(np, "#pwm-cells", &pwm_cells);
+	if (ret == -EINVAL)
+		return -ENODEV;
+
+	if (ret)
+		return dev_err_probe(dev, ret, "could not read #pwm-cells\n");
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(priv->base))
+		return PTR_ERR(priv->base);
+
+	ret = of_property_read_u32(np, "timer-width", &timer_width);
+	if (ret)
+		timer_width = 16;
+
+	priv->max = BIT(timer_width) - 1;
+
+	priv->clk = devm_clk_get_enabled(dev, NULL);
+	if (IS_ERR(priv->clk))
+		return dev_err_probe(dev, PTR_ERR(priv->clk),
+				     "ERROR: timer input clock not found\n");
+
+	priv->rate = clk_get_rate(priv->clk);
+
+	clk_rate_exclusive_get(priv->clk);
+
+	priv->chip.dev = dev;
+	priv->chip.ops = &ttc_pwm_ops;
+	priv->chip.npwm = TTC_PWM_MAX_CH;
+	ret = pwmchip_add(&priv->chip);
+	if (ret) {
+		clk_rate_exclusive_put(priv->clk);
+		return dev_err_probe(dev, ret, "Could not register PWM chip\n");
+	}
+
+	platform_set_drvdata(pdev, priv);
+
+	return 0;
+}
+
+static void ttc_pwm_remove(struct platform_device *pdev)
+{
+	struct ttc_pwm_priv *priv = platform_get_drvdata(pdev);
+
+	pwmchip_remove(&priv->chip);
+	clk_rate_exclusive_put(priv->clk);
+}
+
+static const struct of_device_id __maybe_unused ttc_pwm_of_match[] = {
+	{ .compatible = "cdns,ttc"},
+	{},
+};
+MODULE_DEVICE_TABLE(of, ttc_pwm_of_match);
+
+static struct platform_driver ttc_pwm_driver = {
+	.probe = ttc_pwm_probe,
+	.remove_new = ttc_pwm_remove,
+	.driver = {
+		.name = "ttc-pwm",
+		.of_match_table = of_match_ptr(ttc_pwm_of_match),
+	},
+};
+module_platform_driver(ttc_pwm_driver);
+
+MODULE_AUTHOR("Mubin Sayyed <mubin.sayyed@amd.com>");
+MODULE_DESCRIPTION("Cadence TTC PWM driver");
+MODULE_LICENSE("GPL");
-- 
2.25.1

Re: [LINUX PATCH v2 3/3] pwm: pwm-cadence: Add support for TTC PWM

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 6357 bytes --]

Hello,

On Tue, Nov 14, 2023 at 06:17:48PM +0530, Mubin Sayyed wrote:
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 8ebcddf91f7b..7fd493f06496 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -152,6 +152,17 @@ config PWM_BRCMSTB
>  	  To compile this driver as a module, choose M Here: the module
>  	  will be called pwm-brcmstb.c.
>  
> +config PWM_CADENCE
> +        tristate "Cadence PWM support"
> +        depends on OF
> +        depends on COMMON_CLK

An additional dependency on a SoC || COMPILE_TEST would be good to spare
users on (say) mips and x86 the question for PWM_CADENCE during
oldconfig.

> +        help
> +          Generic PWM framework driver for cadence TTC IP found on
> +          Xilinx Zynq/ZynqMP/Versal SOCs. Each TTC device has 3 PWM
> +          channels. Output of 0th PWM channel of each TTC device can
> +          be routed to MIO or EMIO, and output of 1st and 2nd PWM
> +          channels can be routed only to EMIO.
> +
>  config PWM_CLK
>  	tristate "Clock based PWM support"
>  	depends on HAVE_CLK || COMPILE_TEST
> [...]
> diff --git a/drivers/pwm/pwm-cadence.c b/drivers/pwm/pwm-cadence.c
> new file mode 100644
> index 000000000000..12aaa004bf7f
> --- /dev/null
> +++ b/drivers/pwm/pwm-cadence.c
> @@ -0,0 +1,370 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver to configure cadence TTC timer as PWM
> + * generator
> + *
> + * Limitations:
> + * - When PWM is stopped, timer counter gets stopped immediately. This
> + *   doesn't allow the current PWM period to complete and stops abruptly.
> + * - Disabled PWM emits inactive level.
> + * - When user requests a change in  any parameter of PWM (period/duty cycle/polarity)

s/  / /

> + *   while PWM is in enabled state:
> + *	- PWM is stopped abruptly.
> + *	- Requested parameter is changed.
> + *	- Fresh PWM cycle is started.
> + *
> + * Copyright (C) 2023, Advanced Micro Devices, Inc.
> + */
> +
> [...]
> +static int ttc_pwm_apply(struct pwm_chip *chip,
> +			 struct pwm_device *pwm,
> +			 const struct pwm_state *state)
> +{
> +	struct ttc_pwm_priv *priv = xilinx_pwm_chip_to_priv(chip);
> +	u64 duty_cycles, period_cycles;
> +	struct pwm_state cstate;
> +	unsigned long rate;
> +	bool flag = false;
> +	u32 div = 0;
> +
> +	cstate = pwm->state;

You only use cstate.enabled, so there is no need to copy the whole
struct to the stack.

> +	if (state->polarity != cstate.polarity) {
> +		if (cstate.enabled)
> +			ttc_pwm_disable(priv, pwm);

If you set cstate.enabled = false here you can save another call to
ttc_pwm_disable() below.

> +		ttc_pwm_set_polarity(priv, pwm, state->polarity);
> +	}
> +
> +	rate = priv->rate;
> +
> +	/* Prevent overflow by limiting to the maximum possible period */
> +	period_cycles = min_t(u64, state->period, ULONG_MAX * NSEC_PER_SEC);
> +	period_cycles = mul_u64_u64_div_u64(period_cycles, rate, NSEC_PER_SEC);
> +
> +	if (period_cycles > priv->max) {
> +		/*
> +		 * Prescale frequency to fit requested period cycles within limit.
> +		 * Prescalar divides input clock by 2^(prescale_value + 1). Maximum
> +		 * supported prescalar value is 15.
> +		 */
> +		div = mul_u64_u64_div_u64(state->period, rate, (NSEC_PER_SEC * priv->max));
> +		div = order_base_2(div);
> +		if (div)
> +			div -= 1;
> +
> +		if (div > 15)
> +			return -ERANGE;
> +
> +		rate = DIV_ROUND_CLOSEST(rate, BIT(div + 1));
> +		period_cycles = mul_u64_u64_div_u64(state->period, rate,
> +						    NSEC_PER_SEC);

.apply() is supposed to yield the biggest possible period that isn't
bigger than the requested period.

I didn't do the complete maths, but I suspect this to be wrong for
several reasons:

 - div gets big if state->period is big. So div > 15 shouldn't result in
   -ERANGE but setting in a possible big period.
 - if (div) div -= 1; smells like you configure a too big period if
   div=0 was calculated. (Then you should return -ERANGE)
 - ROUND_CLOSEST is nearly always wrong in .apply()

If you test your driver with CONFIG_PWM_DEBUG enabled and then something
like:

	echo 0 > /sys/class/pwm/pwmchip0/export
	cd /sys/class/pwm/pwmchip0/pwm0
	echo 0 > duty_cycle
	seq 10000 500000 | while read p; do
		echo p > period
	done
	seq 500000 10000 -1 | while read p; do
		echo p > period
	done

should help you to get this right. (Pick a reasonable range for period
and test in 1ns steps.)

> +		flag = true;
> +	}
> +
> [...]
>
> +static int ttc_pwm_get_state(struct pwm_chip *chip,
> +			     struct pwm_device *pwm,
> +			     struct pwm_state *state)
> +{
> +	struct ttc_pwm_priv *priv = xilinx_pwm_chip_to_priv(chip);
> +	u32 value, pres_en, pres = 1;
> +	unsigned long rate;
> +	u64 tmp;
> +
> +	value = ttc_pwm_ch_readl(priv, pwm->hwpwm, TTC_CNT_CNTRL);
> +
> +	if (value & TTC_CNTR_CTRL_WAVE_POL)
> +		state->polarity = PWM_POLARITY_NORMAL;
> +	else
> +		state->polarity = PWM_POLARITY_INVERSED;
> +
> +	if (value & TTC_CNTR_CTRL_DIS)
> +		state->enabled = false;
> +	else
> +		state->enabled = true;
> +
> +	rate = priv->rate;
> +
> +	pres_en =  ttc_pwm_ch_readl(priv, pwm->hwpwm, TTC_CLK_CNTRL);

s/  / /

> +	pres_en	&= TTC_CLK_CNTRL_PS_EN;
> +
> +	if (pres_en) {
> +		pres = ttc_pwm_ch_readl(priv, pwm->hwpwm, TTC_CLK_CNTRL) & TTC_CLK_CNTRL_PSV;
> +		pres >>= TTC_CNTR_CTRL_PRESCALE_SHIFT;

Consider using FIELD_GET

> +		/* If prescale is enabled, the count rate is divided by 2^(pres + 1) */
> +		pres = BIT(pres + 1);
> +	}
> +
> [...]
> +
> +static const struct pwm_ops ttc_pwm_ops = {
> +	.apply = ttc_pwm_apply,
> +	.get_state = ttc_pwm_get_state,
> +	.owner = THIS_MODULE,

Assigning .owner isn't needed any more since
384461abcab6602abc06c2dfb8fb99beeeaa12b0.

> +};
> [...]
> +static void ttc_pwm_remove(struct platform_device *pdev)
> +{
> +	struct ttc_pwm_priv *priv = platform_get_drvdata(pdev);
> +
> +	pwmchip_remove(&priv->chip);
> +	clk_rate_exclusive_put(priv->clk);

Hmm, if there was a devm_clk_rate_exclusive_get, you could get rid of
the remove callback.

> +}

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [LINUX PATCH v2 3/3] pwm: pwm-cadence: Add support for TTC PWM

[Krzysztof Kozlowski]

On 14/11/2023 13:47, Mubin Sayyed wrote:
> Cadence TTC timer can be configured as clocksource/clockevent
> or PWM device. Specific TTC device would be configured as PWM
> device, if pwm-cells property is present in the device tree
> node.
> 

...

> +
> +static struct platform_driver ttc_pwm_driver = {
> +	.probe = ttc_pwm_probe,
> +	.remove_new = ttc_pwm_remove,
> +	.driver = {
> +		.name = "ttc-pwm",
> +		.of_match_table = of_match_ptr(ttc_pwm_of_match),

Drop of_match_ptr(), you will have here warnings.

Best regards,
Krzysztof