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;
From:   Sean Anderson <sean.anderson@seco.com>
Subject: Re: [PATCH v7 2/3] clocksource: Rewrite Xilinx AXI timer driver
To:     Michal Simek <michal.simek@xilinx.com>, linux-pwm@vger.kernel.org,
        devicetree@vger.kernel.org,
        Thierry Reding <thierry.reding@gmail.com>
Cc:     =?UTF-8?Q?Uwe_Kleine-K=c3=b6nig?= <u.kleine-koenig@pengutronix.de>,
        Lee Jones <lee.jones@linaro.org>, linux-kernel@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org,
        Alvaro Gamez <alvaro.gamez@hazent.com>,
        Daniel Lezcano <daniel.lezcano@linaro.org>,
        Thomas Gleixner <tglx@linutronix.de>
References: <20210916180544.2873770-1-sean.anderson@seco.com>
 <20210916180544.2873770-2-sean.anderson@seco.com>
 <696e2f8b-1737-0686-40cb-575a8fa2fa61@xilinx.com>
Message-ID: <667279a0-1a11-06ad-c764-d9150a5db593@seco.com>
Date:   Tue, 5 Oct 2021 18:08:24 -0400
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
In-Reply-To: <696e2f8b-1737-0686-40cb-575a8fa2fa61@xilinx.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?Y3ZxWCt2M2FKZ0JNcXFlYTBPRzREWU5BTTlRRjFkdEo3d0orOFpOWTlQcExT?=
 =?utf-8?B?VGwzRFBTcnZ4SXlOTWwyUzZGcVRoM2Vyb3BxQjhId1RMd2xmYThwSDBqWWg0?=
 =?utf-8?B?NzJjKzRrc2RLdUtNMzNLd096Umo5Q0JqbSt1d2lTbi9VWi84aGNvVmw1K0t6?=
 =?utf-8?B?UkpNNDRKSUpPbmdPSWJBeEZyYXpRRHZYU1R4d092dm1DZWg2NUZvQnBsY2VC?=
 =?utf-8?B?YXRYWDRnMHVJWjZwbUJ3RnVRZ01hakZRWXAwZHZnd3ZKZzdLMnR6aGE1SVNl?=
 =?utf-8?B?QUZlcnVtOThkZEZWMlNzaEZzRUEwMHgySzhESnNmNnB3a001ZlVpcG90TkMw?=
 =?utf-8?B?bkUvY1c2RVhNbkdYekdmbW02SmQwcmhhcXVlay9mSHh6RzhUcjhGWkJiQnAv?=
 =?utf-8?B?TjE3STg4cFdyVm5NZXpBVGVGc1U2U2VHYXNmUUk0TGFiU0h2ZXNrWks5eGkx?=
 =?utf-8?B?UWpabFNFL1orUE9hMG1nV292eUJPbHhUMTRYUmxMTGJzaWtwaG5vaUdXQjZG?=
 =?utf-8?B?WXJLSFdmY2FodndBMmdKYXo0TmJoNjRiOHF2VVp5OG5tRFBXSWNnVUNJWmYr?=
 =?utf-8?B?eE1xOW9LUWRxbFZ5TGY1Uk9OdHg2cUxSVXZBQzVRVmdKdy9KdHZoOWdmdks3?=
 =?utf-8?B?UDNXc01BMlZPbk1xTGJKTDlqVXl4YkZjNWRQSDlSMmNrQ2RYZ0RYYmF4bVVN?=
 =?utf-8?B?SEh0d01WWFdxWUk4d09jZDVCUi9SZ251aUJSSXkxbStCNU1xWnZKaUpFbHpQ?=
 =?utf-8?B?ZzArc0lNSTRyU3RYcWxOWlV6Ti91V3dCVlRwS2docXlRNkY1YzlSekpkdFI0?=
 =?utf-8?B?N0NVQ0tLcEtybmMxV3NNaEJIUFljZnBuNFBWWDVKbHRjMVlROE5MZUlXVEpo?=
 =?utf-8?B?SnVjY1M0M21NdXpvN0NxaDdHTlFCVzBKaEhCRjB0RVhSaWFrSmpoeW5mNkhY?=
 =?utf-8?B?Uk1nUVpxU2JObVpuMEdncFVjdVRlYmN6eXBNU012MnM3U1hXREFKVE92Qzkw?=
 =?utf-8?B?WFRkMVF1R0RQcVB0RmpjTXZmRExzcVpQeUJ3ZyttdFFLRkxORURWOGVtTGRu?=
 =?utf-8?B?Rng4OUxHdGcrakpQT1VEeThnNTRLUjhpZnh5Y1BsR2E5STQ1cHlneGZ2Zkpm?=
 =?utf-8?B?L1U2RWlyV0E1ZFBSNXBXV1JZTWpMNW04bkxlcUxRcDc4bm9jTjNlV3piK0Rr?=
 =?utf-8?B?aVN1QUt2MmltTGM3TnM4T3VoMTI3OW1tRUFXSVN1WHdmVllPK0pTYlBSdmQr?=
 =?utf-8?B?M0I1M254UWY0elhWRzlXNXdWdzlxb1BsbU96YkNuUnNXTWJXTmNlUzJ2dXp5?=
 =?utf-8?B?d1ZBSEEzRGlSQU1KWis5NkRIZ1JWZjlkSng3ZXBxYnI5V204dmpOY1prY1dj?=
 =?utf-8?B?bldDQVl3emNMSmRQU095M2YwU0xJNkFNOGlXRlJ2U0hmdkZYZDI2VkgzY2h0?=
 =?utf-8?B?RjQ0ZzgvT0xJTUYwZThZZWd1NjJjaElVVUdjUnlValRTd3IzUXQ1K2tEOWZO?=
 =?utf-8?B?SzZvajA5SGZCYnExZ21KSE03cVVKV3ZHeVI1aGUrdVE4UVBaQVNOR1Z3bnpV?=
 =?utf-8?B?enpZR3BLUG55NkxnQ3JGUHErVFgvUTlaWWxOcEViTU1vN3Z0Qms2bHFPTzVj?=
 =?utf-8?B?SGtuMENCYnhSWjhWZTEvTWFhcUs5VXBYTUpybkR0RVM2M0dia0FWK2F3cVRP?=
 =?utf-8?B?NTJnZ2xueXhiUHp4SnhPUGlDNlY2RWIyQjJwRTJFMU04Mk9INy9DbnhKUGpk?=
 =?utf-8?Q?ca/ddRdfQTUSXaYHFhz66z9ie5EBlFrQvZP1q2F?=
X-OriginatorOrg: seco.com
X-MS-Exchange-CrossTenant-Network-Message-Id: dcce67c5-5d44-4917-afde-08d9884ca974
X-MS-Exchange-CrossTenant-AuthSource: DB7PR03MB4523.eurprd03.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 Oct 2021 22:08:29.2343
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: bebe97c3-6438-442e-ade3-ff17aa50e733
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: pZqr/1E5OtuCn++EtARBXuwt/HYs5QmD/Y1GvhJaBwqiuhRRr3v1a7agE8Ku4/ldouh5x0NTCxKFCO/k6CjdyQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB6PR03MB3045
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 9/24/21 2:52 AM, Michal Simek wrote:
> Dear Sean,
>
> On 9/16/21 8:05 PM, Sean Anderson wrote:
>> This rewrites the Xilinx AXI timer driver to be more platform agnostic.
>> Some common code has been split off so it can be reused. These routines
>> currently live in drivers/mfd. The largest changes are summarized below:
>>
>> - We now support any number of timer devices, possibly with only one
>>   counter each. The first counter will be used as a clocksource. Every
>>   other counter will be used as a clockevent. This allocation scheme was
>>   chosen arbitrarily.
>> - We do not use timer_of_init because we need to perform some tasks in
>>   between different stages. For example, we must ensure that ->read and
>>   ->write are initialized before registering the irq. This can only happen
>>   after we have gotten the register base (to detect endianness). We also
>>   have a rather unusual clock initialization sequence in order to remain
>>   backwards compatible. Due to this, it's ok for the initial clock request
>>   to fail, and we do not want other initialization to be undone. Lastly, it
>>   is more convenient to do one allocation for xilinx_clockevent_device than
>>   to do one for timer_of and one for xilinx_timer_priv.
>> - We now pay attention to xlnx,count-width and handle smaller width timers.
>>   The default remains 32.
>> - We access registers using regmap. This automatically deals with
>>   endianness issues, so we no longer have to use our own wrappers. It
>>   also provides locking for clockevents which have to worry about being
>>   interrupted in the middle of a read/modify/write.
>>
>> Note that while the existing timer driver always sets the cpumask to cpu
>> 0, this version sets it to all possible CPUs. I believe this is correct
>> for multiprocessor systems where the timer is not physically wired to a
>> particular CPU's interrupt line. For uniprocessor systems (like most
>> microblaze systems) this makes no difference.
>>
>> Signed-off-by: Sean Anderson <sean.anderson@seco.com>
>> ---
>> This has been tested on microblaze qemu.
>>
>> Changes in v7:
>> - Add dependency on OF_ADDRESS
>>
>> Changes in v6:
>> - Add __init* attributes
>> - Export common symbols
>> - Fix goto'ing incorrect label for cleanup
>> - Remove duplicate regmap_config
>> - Round to closest period in xilinx_timer_get_period to ensure proper
>>   semantics for xilinx_pwm_get_state
>>
>> Changes in v5:
>> - Fix some overflows when setting the max value for clockevent and
>>   sched_clock
>> - Just use clk_register_fixed_rate instead of the "private" version
>> - Remove duplicate register definitions
>> - Remove xilinx_timer_tlr_period
>> - Remove xlnx,axi-timer-2.0 compatible string
>> - Require that callers check arguments to xilinx_timer_tlr_cycles
>> - Use regmap to deal with endianness issues as suggested by Lee
>>
>> Changes in v4:
>> - Break out clock* drivers into their own file
>>
>>  MAINTAINERS                               |   6 +
>>  arch/microblaze/kernel/Makefile           |   3 +-
>>  arch/microblaze/kernel/timer.c            | 326 ----------------------
>>  drivers/clocksource/Kconfig               |  13 +
>>  drivers/clocksource/Makefile              |   1 +
>>  drivers/clocksource/timer-xilinx-common.c |  71 +++++
>>  drivers/clocksource/timer-xilinx.c        | 323 +++++++++++++++++++++
>>  include/clocksource/timer-xilinx.h        |  91 ++++++
>>  8 files changed, 506 insertions(+), 328 deletions(-)
>>  delete mode 100644 arch/microblaze/kernel/timer.c
>>  create mode 100644 drivers/clocksource/timer-xilinx-common.c
>>  create mode 100644 drivers/clocksource/timer-xilinx.c
>>  create mode 100644 include/clocksource/timer-xilinx.h
>
>
> I have said it couple of times. I won't accept this in this form.
> I have no problem to move this driver out of microblaze. But I want to
> see transition from current state to new state and check it with baby
> steps which are bisectable if any problem happens.
> Because in this style we end in this patch and it will take some time to
> find out what it is failing.

Unfortunately, I do not have the time do do this at the moment. Because
these drivers are independent in nature, I propose to drop these changes
to the timer driver, but leave the common functions split out. In the
future, I (or you) may come back and make the changes in this patch in
an incremental fashion. The only change necessary for this driver would
be to check for #pwm-cells.

--Sean