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: u.kleine-koenig@pengutronix.de,
 thierry.reding@gmail.com,
 lee.jones@linaro.org,
 alvaro.gamez@hazent.com,
 linux-kernel@vger.kernel.org,
 linux-arm-kernel@lists.infradead.org,
 devicetree@vger.kernel.org,
 linux-pwm@vger.kernel.org,
 sean.anderson@seco.com
Subject: Re: [PATCH v2 2/2] pwm: Add support for Xilinx AXI Timer
To:     Sean Anderson <sean.anderson@seco.com>,
        Michal Simek <michal.simek@xilinx.com>,
        <linux-pwm@vger.kernel.org>, <devicetree@vger.kernel.org>
CC:     <linux-arm-kernel@lists.infradead.org>,
        <linux-kernel@vger.kernel.org>,
        Alvaro Gamez <alvaro.gamez@hazent.com>,
        Lee Jones <lee.jones@linaro.org>,
        Thierry Reding <thierry.reding@gmail.com>,
        =?UTF-8?Q?Uwe_Kleine-K=c3=b6nig?= <u.kleine-koenig@pengutronix.de>
References: <20210504184925.3399934-1-sean.anderson@seco.com>
 <20210504184925.3399934-2-sean.anderson@seco.com>
 <e3782bc5-bcd9-5eb8-e89b-e4e52ed2e3cb@xilinx.com>
 <1bfde199-617a-343c-10ed-4c436bfd908f@seco.com>
 <ff8eb398-fd49-fdb8-447e-2f6270cb006d@xilinx.com>
 <448097ba-7616-ccca-7219-6532dac04dbd@seco.com>
From:   Michal Simek <michal.simek@xilinx.com>
Message-ID: <12409453-c6f2-1731-a45a-7ef74dabe982@xilinx.com>
Date:   Mon, 10 May 2021 12:20:34 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.10.1
MIME-Version: 1.0
In-Reply-To: <448097ba-7616-ccca-7219-6532dac04dbd@seco.com>
Content-Type: text/plain; charset="utf-8"
Content-Language: en-US
Content-Transfer-Encoding: 8bit
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 10 May 2021 10:20:49.8611
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 316e901a-373d-4ba0-555a-08d9139d48b1
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: BL2NAM02FT032.eop-nam02.prod.protection.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY5PR02MB7042
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi,

On 5/7/21 12:36 AM, Sean Anderson wrote:
> 
> 
> On 5/6/21 12:54 PM, Michal Simek wrote:
>> Hi,
>>
>> On 5/6/21 4:28 PM, Sean Anderson wrote:
>>>
>>>
>>> On 5/5/21 2:37 AM, Michal Simek wrote:
>>>>
>>>>
>>>> On 5/4/21 8:49 PM, 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>
>>>>> ---
>>>>> I tried adding a XILINX_PWM_ prefix to all the defines, but IMO it
>>>>> really hurt readability. That prefix almost doubles the size the
>>>>> defines, and is particularly excessive in something like
>>>>> XILINX_PWM_TCSR_RUN_MASK.
>>>>>
>>>>> Changes in v2:
>>>>> - Don't compile this module by default for arm64
>>>>> - Add dependencies on COMMON_CLK and HAS_IOMEM
>>>>> - Add comment explaining why we depend on !MICROBLAZE
>>>>> - Add comment describing device
>>>>> - Rename TCSR_(SET|CLEAR) to TCSR_RUN_(SET|CLEAR)
>>>>> - Use NSEC_TO_SEC instead of defining our own
>>>>> - Use TCSR_RUN_MASK to check if the PWM is enabled, as suggested by
> Uwe
>>>>> - Cast dividends to u64 to avoid overflow
>>>>> - Check for over- and underflow when calculating TLR
>>>>> - Set xilinx_pwm_ops.owner
>>>>> - Don't set pwmchip.base to -1
>>>>> - Check range of xlnx,count-width
>>>>> - Ensure the clock is always running when the pwm is registered
>>>>> - Remove debugfs file :l
>>>>> - Report errors with dev_error_probe
>>>>>
>>>>>     drivers/pwm/Kconfig      |  13 ++
>>>>>     drivers/pwm/Makefile     |   1 +
>>>>>     drivers/pwm/pwm-xilinx.c | 301
> +++++++++++++++++++++++++++++++++++++++
>>>>>     3 files changed, 315 insertions(+)
>>>>>     create mode 100644 drivers/pwm/pwm-xilinx.c
>>>>
>>>> Without looking below another driver which target the same IP is just
>>>> wrong that's why NACK from me.
>>>
>>> Can you elaborate on this position a bit more? I don't think a rework of
>>> the microblaze driver should hold back this one. They cannot be enabled
>>> at the same time. I think it is OK to leave the work of making them
>>> coexist for a future series (written by someone with microblaze hardware
>>> to test on).
>>
>> I am here to test it on Microblaze. In a lot of cases you don't have
>> access to all HW you should test things on but that's why others can
>> help with this.
> 
> Ok, can you convert the microblaze driver then? I'm afraid I can't work
> on a driver if I don't have a system to test it on. There are too many
> small bugs which can creep in without anything to work with. If you are
> insistant that there must be no driver duplication (even temporarily),
> then you should help with the deduplication :)
> 
> I would also be willing to try and get a microblaze qemu setup working,
> but I have found no good instructions for doing so with mainline linux.
> The best I found was [1]. Do you have a working setup for this?


You can look at Guenter's files which he uses for testing here.
http://server.roeck-us.net/qemu/microblazeel/

Or you can use Xilinx petalinux distribution or Yocto layer which should
have qemu integrated.

Thanks,
Michal