Message-ID: <3693d66b-801e-dc31-15ee-e010f2577655@lenbrook.com>
Date:   Tue, 14 Feb 2023 09:51:24 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Subject: Re: [PATCH] clk: imx: pll14xx: fix recalc_rate for negative kdiv
Content-Language: en-US
To:     Stephen Boyd <sboyd@kernel.org>, Abel Vesa <abelvesa@kernel.org>,
        Fabio Estevam <festevam@gmail.com>,
        Michael Turquette <mturquette@baylibre.com>,
        NXP Linux Team <linux-imx@nxp.com>,
        Pengutronix Kernel Team <kernel@pengutronix.de>,
        Sascha Hauer <s.hauer@pengutronix.de>,
        Shawn Guo <shawnguo@kernel.org>,
        linux-arm-kernel@lists.infradead.org, linux-clk@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20221210203835.9714-1-kgroeneveld@lenbrook.com>
 <21e4cb835f678faa911ef97d28931246.sboyd@kernel.org>
From:   Kevin Groeneveld <kgroeneveld@lenbrook.com>
In-Reply-To: <21e4cb835f678faa911ef97d28931246.sboyd@kernel.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?NVhHaGdoYkxDbjJ5VnNaOGdCZzAzdkZpYi9Qb1VhMUZ6b2szRFFyU1A0a3BC?=
 =?utf-8?B?UHdwK3ZzTit2YXBtdVVDUExGOUlabGRIWDJXSXVGWFFtcG85dWlWMVAvT0d4?=
 =?utf-8?B?aFV0WTJsckloVU9OWi9KSVlnRWcvYUx0UUxPVVNBN0RYK1NZUW4rcDRaUWtx?=
 =?utf-8?B?eU5Od0dYVFhvNkJ0VU16YWdiNWZEK1VlYlBqMFdIT0RxZWI3ZCtuZ3pqOW5u?=
 =?utf-8?B?aFJXTDZQQm9Jc1RrRmVXZUlTd2dtYkJudUFwcmpmemtrd2ZCU0NVeTRpdVNB?=
 =?utf-8?B?dk13LzU1QkZYWUh5TDkrWENZcGtxVllOSXc5Uzl0RHYremJkckVBV3UwcnhC?=
 =?utf-8?B?aVFFUUY1Qi9uTWRLZUcxTEJEQUxKby9lNXB0M1FZejQ0RGIwSTRzdDFKckxL?=
 =?utf-8?B?VmVZMjUvNkprU0xXT0tLWW02TlJMeXNiVU5GREY0NnNickluQjQ3bFYweVNL?=
 =?utf-8?B?d1o0YnoyUHE3OWFnUGZ2WUwrN1cxcXFQVDFIZXhadDNpRlRycCttN3k0NUdL?=
 =?utf-8?B?MHF2U243dU1OYU01aW56TmphMEJhako3YXNOOHRmeCtybkFuSG44RWpMazBS?=
 =?utf-8?B?TTZFYjJWT3pjWE8zaVBXOC84VmZEa3JqRUN6Rk1YZ0JJTWhTOGVyVXpDMGhW?=
 =?utf-8?B?THh2VjM5bGY3c0dVU3djbms2MGYzWnhOTEVWMlViUWNjWkVKbVpVdzRsTnJH?=
 =?utf-8?B?UnBZc09mak52dFJ3WnhCY1hXVnJtcENqYU5KMHFnN2dvMFFEdzhLaFp4SFl3?=
 =?utf-8?B?ZHdvVEJrRGM4Vi9Yb0MvNXRtL2xBZ1UyZzlJVVlVUjNyVitiZHd4NHg2bTJt?=
 =?utf-8?B?SDB5bGN3WjVKamF0RGpQeW5tSDhMZWwvekVQYVU5MXRqZW1aajExTFVNK2tY?=
 =?utf-8?B?ckpjWnlheTRSQjJmZzROZ1BzbTJWT3dBQ0tQUUdSTW93a1NHbkdlL3g0TExJ?=
 =?utf-8?B?ZnNLVHdYaXVPMTBmSDluQk5xaFZlSm1POGRTOTQrbmU5aGhucjlmWnRnYXZN?=
 =?utf-8?B?dXJ3QWJXWWNtY3MwOUgwckhscFUvaUxzbHdHUTlwekppNlhWL3dxcGVEWkY3?=
 =?utf-8?B?Uy9WMjg0VUtNTTRBTmw3emdGU3Izazc4dlAvMVVNZFJZRDBib0NsNXV0TUtX?=
 =?utf-8?B?TWtxQ3VvbTVWb09XTjdDTnhGWmFZdFZ4ZytndXRGMnY3Y2dwUlE1QkZKUkIw?=
 =?utf-8?B?T3JIbm40azRNVGZuUjFacm5DWmxQK3lQdytyOFJMMEVHQjFaWVRkSzFxTTky?=
 =?utf-8?B?RUVRRnlHUjZ6U0F4YnpXV1ZMQm9YL2tybUx1VzArRWMrZlZXMnJGRTU1aHFF?=
 =?utf-8?B?SGxuRVcvYmxkR2xkRDZJYUhEOUZ3eGJtT0k1QXhEKzUram5JS1BQRnhBVkli?=
 =?utf-8?B?QTNXVkllM21Cb2pwSkkvTFRya2hBeWRZdEVuNzFYV0FwVnc1YmRocUM4SlZD?=
 =?utf-8?B?QVRXMmU5M0gwUHJxYnZMOGNHTTh6Mk5xc0NGWUtDcEZqd0ZhQWJYRmNSWFZh?=
 =?utf-8?B?ckk0RkwyMTBBbWd3NXRtMFFMUVY3M2FxZEl3R2FYUUJwYzZ5RHpIcXpYV1R6?=
 =?utf-8?B?VEJ0RHNadmU2aXlMbFNqTzlzdlpFbEVmTGJhZ0xjWmZSMEZhczZvYzV3ZFZm?=
 =?utf-8?B?L2xaMWtFeFpPQThLL2JWV1lXZ3VEcFVsUkhWeVpYTm9rOExTVXlOc3hVL3Ny?=
 =?utf-8?B?anBmL2ZCVXBNTy91UVg1QTkxc2o2MU1zOGtEbWk0QUpOZmt1RGJzOEJLanJ3?=
 =?utf-8?B?TzByZ2pnVVFJelBtbEdrRlptc0ZkakdEQWl5OHlhMGliclpQNGdaQmJSem1O?=
 =?utf-8?B?T3BlVHBNY2thVGdleWZId2k4eVIrSVQ2M1NyT1dHMVNRNDNBem1McGFHQWRR?=
 =?utf-8?B?SjdYVGhtZVl2eW5zSU9FVzlNcCtvTmZRKy92MEdXcWtyYStzcW1rREVucTlM?=
 =?utf-8?B?cEpjanllNis4SHRRcXN1c0lqQlRERHlGam5Sd0FxdnRONmtrUExMTTJuTVJN?=
 =?utf-8?B?NDBHQUJqZ1ZvbUI5K3dKaXdTb2Y4MlJNUGRpUkh2ODdiNzNDVjJYamFBQi9B?=
 =?utf-8?B?b1E5VDF6S1FqWFV6R2ZOYk11Z0RsM0s1WXNHbzd0eHYydWorUWhWUEYrb3hi?=
 =?utf-8?B?Q2owVnJNZkpHYTM5WFlvWVJBcW1EenBBZ3hST28zMmZ6ZFFDM0ZhcnNNN0R4?=
 =?utf-8?B?R1E9PQ==?=
X-OriginatorOrg: lenbrook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: d321b4fd-2263-401a-549a-08db0e9b0624
X-MS-Exchange-CrossTenant-AuthSource: YT4PR01MB9670.CANPRD01.PROD.OUTLOOK.COM
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 Feb 2023 14:51:58.9914
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3089fb55-f9f3-4ac8-ba44-52ac0e467cb6
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: Az81y49N+Uah/lfUpDOEhyeS4suyVUraXEejuTsodzbOHYLjLemkaV+l7BuLPnZsJY5ixHAHJScUVObPV9xJPuzS+0k86bTxcOWzXeH7Ehk=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: YT1PR01MB9209
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 2023-02-10 18:24, Stephen Boyd wrote:
> Quoting Kevin Groeneveld (2022-12-10 12:38:35)
>> kdiv is a signed 16 bit value in the DEV_CTL1 register. Commit
>> 53990cf9d5b4 ("clk: imx: pll14xx: consolidate rate calculation") changed
>> the kdiv variable from a short int to just int. When the value read from
>> the DIV_CTL1 register is assigned directly to an int the sign of the value
>> is lost resulting in incorrect results when the value is negative. Adding
> 
> Can the field be negative?

Yes it can be. That is how I found the issue as I had a negative value 
in imx_pll14xx_rate_table which used to work but broke with the above 
mentioned commit.

The i.MX8MM reference manual states:
 >Formula for Fraction PLLOUT:
 >* FOUT=((m+k/65536) x FIN) /(p x 2s)
 >* Where, 1 ≤ p ≤ 63, 64 ≤ m ≤ 1023, 0 ≤ s ≤ 6, -32768 ≤ k ≤ 32767
 >* p, m, and s are unsigned integers. k is a two's complement integer.

The comments in the imx_pll14xx_calc_settings function also mention the 
same range for kdiv. And this function also uses negative values. For 
example:

 >/* Then see if we can get the desired rate by only adjusting kdiv 
(glitch free) */
 >rate_min = pll14xx_calc_rate(pll, mdiv, pdiv, sdiv, KDIV_MIN, prate);
 >rate_max = pll14xx_calc_rate(pll, mdiv, pdiv, sdiv, KDIV_MAX, prate);

Where KDIV_MIN is defined as SHRT_MIN which is negative.


Kevin