Hi,
On Tue, Apr 02, 2019 at 11:06:21PM +0200, Jernej Skrabec wrote:
> Sometimes one of the nkmp factors is unused. This means that one of the
> factors shift and width values are set to 0. Current nkmp clock code
> generates a mask for each factor with GENMASK(width + shift - 1, shift).
> For unused factor this translates to GENMASK(-1, 0). This code is
> further expanded by C preprocessor to final version:
> (((~0UL) - (1UL << (0)) + 1) & (~0UL >> (BITS_PER_LONG - 1 - (-1))))
> or a bit simplified:
> (~0UL & (~0UL >> BITS_PER_LONG))
>
> It turns out that result of the second part (~0UL >> BITS_PER_LONG) is
> actually undefined by C standard, which clearly specifies:
>
> "If the value of the right operand is negative or is greater than or
> equal to the width of the promoted left operand, the behavior is
> undefined."
>
> Additionally, compiling kernel with aarch64-linux-gnu-gcc 8.3.0 gave
> different results whether literals or variables with same values as
> literals were used. GENMASK with literals -1 and 0 gives zero and with
> variables gives 0xFFFFFFFFFFFFFFF (~0UL). Because nkmp driver uses
> GENMASK with variables as parameter, expression calculates mask as ~0UL
> instead of 0. This has further consequences that LSB in register is
> always set to 1 (1 is neutral value for a factor and shift is 0).
>
> For example, H6 pll-de clock is set to 600 MHz by sun4i-drm driver, but
> due to this bug ends up being 300 MHz. Additionally, 300 MHz seems to be
> too low because following warning can be found in dmesg:
>
> [ 1.752763] WARNING: CPU: 2 PID: 41 at
> drivers/clk/sunxi-ng/ccu_common.c:41 ccu_helper_wait_for_lock.part.0+0x6c/0x90
> [ 1.763378] Modules linked in:
> [ 1.766441] CPU: 2 PID: 41 Comm: kworker/2:1 Not tainted
> 5.1.0-rc2-next-20190401 #138
> [ 1.774269] Hardware name: Pine H64 (DT)
> [ 1.778200] Workqueue: events deferred_probe_work_func
> [ 1.783341] pstate: 40000005 (nZcv daif -PAN -UAO)
> [ 1.788135] pc : ccu_helper_wait_for_lock.part.0+0x6c/0x90
> [ 1.793623] lr : ccu_helper_wait_for_lock.part.0+0x48/0x90
> [ 1.799107] sp : ffff000010f93840
> [ 1.802422] x29: ffff000010f93840 x28: 0000000000000000
> [ 1.807735] x27: ffff800073ce9d80 x26: ffff000010afd1b8
> [ 1.813049] x25: ffffffffffffffff x24: 00000000ffffffff
> [ 1.818362] x23: 0000000000000001 x22: ffff000010abd5c8
> [ 1.823675] x21: 0000000010000000 x20: 00000000685f367e
> [ 1.828987] x19: 0000000000001801 x18: 0000000000000001
> [ 1.834300] x17: 0000000000000001 x16: 0000000000000000
> [ 1.839613] x15: 0000000000000000 x14: ffff000010789858
> [ 1.844926] x13: 0000000000000000 x12: 0000000000000001
> [ 1.850239] x11: 0000000000000000 x10: 0000000000000970
> [ 1.855551] x9 : ffff000010f936c0 x8 : ffff800074cec0d0
> [ 1.860864] x7 : 0000800067117000 x6 : 0000000115c30b41
> [ 1.866177] x5 : 00ffffffffffffff x4 : 002c959300bfe500
> [ 1.871490] x3 : 0000000000000018 x2 : 0000000029aaaaab
> [ 1.876802] x1 : 00000000000002e6 x0 : 00000000686072bc
> [ 1.882114] Call trace:
> [ 1.884565] ccu_helper_wait_for_lock.part.0+0x6c/0x90
> [ 1.889705] ccu_helper_wait_for_lock+0x10/0x20
> [ 1.894236] ccu_nkmp_set_rate+0x244/0x2a8
> [ 1.898334] clk_change_rate+0x144/0x290
> [ 1.902258] clk_core_set_rate_nolock+0x180/0x1b8
> [ 1.906963] clk_set_rate+0x34/0xa0
> [ 1.910455] sun8i_mixer_bind+0x484/0x558
> [ 1.914466] component_bind_all+0x10c/0x230
> [ 1.918651] sun4i_drv_bind+0xc4/0x1a0
> [ 1.922401] try_to_bring_up_master+0x164/0x1c0
> [ 1.926932] __component_add+0xa0/0x168
> [ 1.930769] component_add+0x10/0x18
> [ 1.934346] sun8i_dw_hdmi_probe+0x18/0x20
> [ 1.938443] platform_drv_probe+0x50/0xa0
> [ 1.942455] really_probe+0xcc/0x280
> [ 1.946032] driver_probe_device+0x54/0xe8
> [ 1.950130] __device_attach_driver+0x80/0xb8
> [ 1.954488] bus_for_each_drv+0x78/0xc8
> [ 1.958326] __device_attach+0xd4/0x130
> [ 1.962163] device_initial_probe+0x10/0x18
> [ 1.966348] bus_probe_device+0x90/0x98
> [ 1.970185] deferred_probe_work_func+0x6c/0xa0
> [ 1.974720] process_one_work+0x1e0/0x320
> [ 1.978732] worker_thread+0x228/0x428
> [ 1.982484] kthread+0x120/0x128
> [ 1.985714] ret_from_fork+0x10/0x18
> [ 1.989290] ---[ end trace 9babd42e1ca4b84f ]---
>
> This commit solves the issue by first checking value of the factor
> width. If it is equal to 0 (unused factor), mask is set to 0, otherwise
> GENMASK() macro is used as before.
>
> Fixes: d897ef56faf9 ("clk: sunxi-ng: Mask nkmp factors when setting register")
> Signed-off-by: Jernej Skrabec <jernej.skra...@siol.net>
> ---
> drivers/clk/sunxi-ng/ccu_nkmp.c | 18 +++++++++++++-----
> 1 file changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/clk/sunxi-ng/ccu_nkmp.c b/drivers/clk/sunxi-ng/ccu_nkmp.c
> index 9b49adb20d07..69dfc6de1c4e 100644
> --- a/drivers/clk/sunxi-ng/ccu_nkmp.c
> +++ b/drivers/clk/sunxi-ng/ccu_nkmp.c
> @@ -167,7 +167,7 @@ static int ccu_nkmp_set_rate(struct clk_hw *hw, unsigned
> long rate,
> unsigned long parent_rate)
> {
> struct ccu_nkmp *nkmp = hw_to_ccu_nkmp(hw);
> - u32 n_mask, k_mask, m_mask, p_mask;
> + u32 n_mask = 0, k_mask = 0, m_mask = 0, p_mask = 0;
> struct _ccu_nkmp _nkmp;
> unsigned long flags;
> u32 reg;
> @@ -186,10 +186,18 @@ static int ccu_nkmp_set_rate(struct clk_hw *hw,
> unsigned long rate,
>
> ccu_nkmp_find_best(parent_rate, rate, &_nkmp);
>
> - n_mask = GENMASK(nkmp->n.width + nkmp->n.shift - 1, nkmp->n.shift);
> - k_mask = GENMASK(nkmp->k.width + nkmp->k.shift - 1, nkmp->k.shift);
> - m_mask = GENMASK(nkmp->m.width + nkmp->m.shift - 1, nkmp->m.shift);
> - p_mask = GENMASK(nkmp->p.width + nkmp->p.shift - 1, nkmp->p.shift);
> + if (nkmp->n.width)
> + n_mask = GENMASK(nkmp->n.width + nkmp->n.shift - 1,
> + nkmp->n.shift);
> + if (nkmp->k.width)
> + k_mask = GENMASK(nkmp->k.width + nkmp->k.shift - 1,
> + nkmp->k.shift);
> + if (nkmp->m.width)
> + m_mask = GENMASK(nkmp->m.width + nkmp->m.shift - 1,
> + nkmp->m.shift);
> + if (nkmp->p.width)
> + p_mask = GENMASK(nkmp->p.width + nkmp->p.shift - 1,
> + nkmp->p.shift);Thanks for the awesome commit log, and for digging this out. I've applied this patch. Could you write a comment just above the code to explain it as well so that it doesn't confuse anyone? You can send a subsequent patch. Thanks! Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
signature.asc
Description: PGP signature
