Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Received-SPF: Pass (protection.outlook.com: domain of xilinx.com designates
 149.199.62.198 as permitted sender) receiver=protection.outlook.com;
 client-ip=149.199.62.198; helo=xsj-pvapexch01.xlnx.xilinx.com;
Envelope-to: thierry.reding@gmail.com,
 lee.jones@linaro.org,
 linux-arm-kernel@lists.infradead.org,
 linux-kernel@vger.kernel.org,
 devicetree@vger.kernel.org,
 linux-pwm@vger.kernel.org,
 robh@kernel.org,
 sean.anderson@seco.com,
 u.kleine-koenig@pengutronix.de
Subject: Re: [PATCH 2/2] pwm: Add support for Xilinx AXI Timer
To:     =?UTF-8?Q?Uwe_Kleine-K=c3=b6nig?= <u.kleine-koenig@pengutronix.de>,
        Sean Anderson <sean.anderson@seco.com>,
        Rob Herring <robh@kernel.org>
CC:     Michal Simek <michal.simek@xilinx.com>,
        <linux-pwm@vger.kernel.org>, <devicetree@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>,
        <linux-arm-kernel@lists.infradead.org>,
        Lee Jones <lee.jones@linaro.org>,
        Thierry Reding <thierry.reding@gmail.com>
References: <20210503214413.3145015-1-sean.anderson@seco.com>
 <20210503214413.3145015-2-sean.anderson@seco.com>
 <20210504085112.edyy6loprfzejrjl@pengutronix.de>
 <dc6d9f40-a913-90c4-9675-0f84f789ab61@xilinx.com>
 <71694d6a-21d8-2b31-0e66-2dfea52a6390@seco.com>
 <20210504161334.tzylwyiz4k2tcztq@pengutronix.de>
From:   Michal Simek <michal.simek@xilinx.com>
Message-ID: <5e67c893-6261-bdfd-a51f-2c17f671294b@xilinx.com>
Date:   Wed, 5 May 2021 08:31:01 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.10.0
MIME-Version: 1.0
In-Reply-To: <20210504161334.tzylwyiz4k2tcztq@pengutronix.de>
Content-Type: text/plain; charset="windows-1252"
Content-Language: en-US
Content-Transfer-Encoding: 8bit
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 May 2021 06:31:06.1690
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: aeb15e7d-e313-4601-395f-08d90f8f5cea
X-MS-Exchange-CrossTenant-Id: 657af505-d5df-48d0-8300-c31994686c5c
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=657af505-d5df-48d0-8300-c31994686c5c;Ip=[149.199.62.198];Helo=[xsj-pvapexch01.xlnx.xilinx.com]
X-MS-Exchange-CrossTenant-AuthSource: BN1NAM02FT035.eop-nam02.prod.protection.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM5PR02MB3337
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi,

On 5/4/21 6:13 PM, Uwe Kleine-K?nig wrote:
> Hello Sean,
> 
> [Adding Rob to the list of recipents, as he for sure has a valuable
> opinion on this matter.]
> 
> On Tue, May 04, 2021 at 11:57:20AM -0400, Sean Anderson wrote:
>> On 5/4/21 8:32 AM, Michal Simek wrote:
>>> On 5/4/21 10:51 AM, Uwe Kleine-K?nig wrote:
>>>> On Mon, May 03, 2021 at 05:44:13PM -0400, Sean Anderson wrote:
>>>>> This adds PWM support for Xilinx LogiCORE IP AXI soft timers commonly
>>>>> found on Xilinx FPGAs. There is another driver for this device located
>>>>> at arch/microblaze/kernel/timer.c, but it is only used for timekeeping.
>>>>> This driver was written with reference to Xilinx DS764 for v1.03.a [1].
>>>>>
>>>>> [1] https://www.xilinx.com/support/documentation/ip_documentation/axi_timer/v1_03_a/axi_timer_ds764.pdf
>>>>>
>>>>> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
>>>>> ---
>>>>>
>>>>>   arch/arm64/configs/defconfig |   1 +
>>>>>   drivers/pwm/Kconfig          |  11 ++
>>>>>   drivers/pwm/Makefile         |   1 +
>>>>>   drivers/pwm/pwm-xilinx.c     | 322 +++++++++++++++++++++++++++++++++++
>>>>>   4 files changed, 335 insertions(+)
>>>>>   create mode 100644 drivers/pwm/pwm-xilinx.c
>>>>>
>>>>> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
>>>>> index 08c6f769df9a..81794209f287 100644
>>>>> --- a/arch/arm64/configs/defconfig
>>>>> +++ b/arch/arm64/configs/defconfig
>>>>> @@ -1083,6 +1083,7 @@ CONFIG_PWM_SAMSUNG=y
>>>>>   CONFIG_PWM_SL28CPLD=m
>>>>>   CONFIG_PWM_SUN4I=m
>>>>>   CONFIG_PWM_TEGRA=m
>>>>> +CONFIG_PWM_XILINX=m
>>>>>   CONFIG_SL28CPLD_INTC=y
>>>>>   CONFIG_QCOM_PDC=y
>>>>>   CONFIG_RESET_IMX7=y
>>>>
>>>> I think this should go into a separate patch once this driver is
>>>> accepted. This can then go via the ARM people.
>>>>
>>>>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>>>>> index d3371ac7b871..01e62928f4bf 100644
>>>>> --- a/drivers/pwm/Kconfig
>>>>> +++ b/drivers/pwm/Kconfig
>>>>> @@ -628,4 +628,15 @@ config PWM_VT8500
>>>>>   	  To compile this driver as a module, choose M here: the module
>>>>>   	  will be called pwm-vt8500.
>>>>>
>>>>> +config PWM_XILINX
>>>>> +	tristate "Xilinx AXI Timer PWM support"
>>>>> +	depends on !MICROBLAZE
>>>>
>>>> I don't understand this dependency.
>>>
>>> The dependency is clear here because microblaze has already driver for
>>> this timer here arch/microblaze/kernel/timer.c.
> 
> Then at least add a comment.

I don't think that comment will really solve this. We should never
duplicate driver for the same IP to two locations.

Maybe this should be MFD driver.


> 
>>> And that's exactly pointing to the way how this should be done.
>>> IP itself is single or dual timer and in case of dual timer you can
>>> select if there is pwm output and use it for PWM generation.
>>>
>>> It means it is timer with PMW together.
>>> I didn't have a time but Uwe likely knows this better how to design it.
>>>
>>> I see that gpio-mvebu driver instantiate pwm driver. Maybe that's the
>>> way to go.
>>
>> I think drivers/clocksource/samsung_pwm_timer.c and
>> drivers/pwm/pwm-samsung.c provide another example for how to go about
>> this.
> 
> I recently had a similar problem (with code that isn't (yet) in
> mainline), where a timer can be used as a counter. I chose to change the
> compatible. Transferred to this example this would mean to use e.g.
> 
> 	static const struct of_device_id xilinx_pwm_of_match[] = {
> 		{ .compatible = "xlnx,xps-timer-pwm-1.00.a" },
> 		...
> 	};
> 
> and if you want to use the hardware as a PWM, you overwrite the
> compatible in your machine.dts.

It is HW selection inside that IP. It means you will get dt properly
when PWM output is selected. I understand that this shortcut can work
but I don't think it is proper design.


> Not sure however that this is nice enough to be accepted by the dt
> people?!

up to Rob.

Thanks,
Michal