From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-ed1-f44.google.com (mail-ed1-f44.google.com [209.85.208.44]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id BA4DD254863 for ; Fri, 28 Feb 2025 08:59:13 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.208.44 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1740733155; cv=none; b=qaIC8WoMB4DHajKf01cPvR1tDo7Am30CaBfKkDb9CSgAUzseY4E7BWllr97Dr0K/Hc5tZryYwjxxUbd2FtqR11wMPCpa8W229pIl+5layXE0Q5zwM6RgNArza381fGUwTNGdNjZ/nkSpfHzTUhDlrrT7KCu7FWUdxbuUfdeAyc4= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1740733155; c=relaxed/simple; bh=bLEtAyE5Aa/xsQIBOnXYf2Hy93VCBG3sU+stkJWUAMQ=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=rJzma/3pAT5xbKeVM4kqvHJqZmeaQuGfuZcVZxosKAFbIC8dxQJyu1kNh/LJNAhyRvNDTeUIEy5C1tEA2n3JkjKmjQS3jJH4aTyc+Z/jWxWHEyOriGi84+n1FWaiorywzfAG5m8r/YrpqsMOBAWM7l7iAI9U70Zv314D81jQU1w= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linaro.org; spf=pass smtp.mailfrom=linaro.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b=vR/k2l7J; arc=none smtp.client-ip=209.85.208.44 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linaro.org Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b="vR/k2l7J" Received: by mail-ed1-f44.google.com with SMTP id 4fb4d7f45d1cf-5e4ebc78da5so798421a12.2 for ; Fri, 28 Feb 2025 00:59:13 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1740733152; x=1741337952; darn=vger.kernel.org; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date:from:to :cc:subject:date:message-id:reply-to; bh=mo2JWZvfARQxbztcC0oXWR4xyuLy9YblRRhrM00mM5o=; b=vR/k2l7J/j5J2Hi8B4DtGrAlElh7t1Lq91fovjRQ6yxzQpXca7TKBNsiBx8mlW023o d7dVqaFFSMsGHcR4R4g5u+h+tw9T/Wj4cMYZkKa5RaDVWEBeQosxWgrEULN3GHSeIjFt MpJXougfTFrJMyY4MG3LyoZuzTx9V5sIAr9w/9wFTXTIFNRVmQhERkG/J+36daVqu6uI F+mYZq9UeDv0rIfefAUWafW0SkS1SqPPs/sTaCU+sNRaV6nE/J0EjJ7cHAeQEhUF5Z9W 7LFEAX8kNxASWxQ2Y3oAdCNllZqqIYXTOmkzXT0yYvWgmkTtS5S+Ibz0N2ibmm8AV4Bj cR8w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1740733152; x=1741337952; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=mo2JWZvfARQxbztcC0oXWR4xyuLy9YblRRhrM00mM5o=; b=mxqby2CiiaABEF86gfPDcciZ28/yrbILNFrFdsD4L8lPqksj2DjDPjqcCKssF97BgN +R0U45Y6X/KeXBIOj+Nc7g2bPDv2o9uLAIqH9dgu4L6qS/uA0Q0b3dlRy+1LZLn/jb9X I3iyFuX4iHMsAixMurpIbPFFYe8xf/eash0PYzaaCIBoMpHOvCyDTwf/+GHlIT97ldGx yDkbBRFdKrdj1bUNN6ORL7eMC5cD5yLxBS0j1pZgEDnNtc2IQcdakfq76N7jCsSxaD/K VJuXbFZMf+XoyPr+JKiAAlCMgRVek/vLGpddIfZhzAX/lJfX0q3YqnvscWsE6d5OxjIW 432Q== X-Forwarded-Encrypted: i=1; AJvYcCVvRwkNWHHORubVUVPAO5JtUyWKGX5NTQixrFLp1YvqxOD2zhOQAVc3fSJvuLhIZOZMNw+myMLIzhLpAh0=@vger.kernel.org X-Gm-Message-State: AOJu0YyroDc55DBqIKVJ7O/7IAeifa73TmChDIOGtMKY4a5vovhcUSWn arKDMvvBdlaZqjswGQ+DYsfrhq25aIRbDQwTY5GgIWnh5Qu00zNmiMbr19OANCg= X-Gm-Gg: ASbGncshxoX0o6T9E7svk1HiTjimMJX927f53FS0uF2EWRjYWZi0Xpb/sNi4PghgBGW LTpBuBh4ZnTMsX9vrI5b52lAR9RlocueXDoqCodgn6RDq/Vlri8YwUV9oRt4gtJMO5idjLHPUmW Ay0t45lgR+L3n55PHtfpf5WQ4+ZFDXF1M1JjWS60jE1OfuTl2G7qNKSh+M0cwXUyRtLVfd6625K KSGqzXmmwtM581sOiMG638o6VZwAKdGUYJXx3uVBvJ+uVGWqskXbC5zSeraaLJtF6NMZR0PwDN8 mbDZhp79Pj1Kdl61Ou0x6Qw= X-Google-Smtp-Source: AGHT+IG+ohc2Pb9yFbQaY7IHiQEBgBBf60B3jiO2zg3pP45+H+r70TxRLEMjkL+Z5vuXly0heAE9eQ== X-Received: by 2002:a17:907:7f22:b0:abb:b209:aba6 with SMTP id a640c23a62f3a-abf25da2d4amr224744566b.3.1740733151822; Fri, 28 Feb 2025 00:59:11 -0800 (PST) Received: from linaro.org ([62.231.96.41]) by smtp.gmail.com with ESMTPSA id a640c23a62f3a-abf3e117114sm24695766b.147.2025.02.28.00.59.10 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 28 Feb 2025 00:59:11 -0800 (PST) Date: Fri, 28 Feb 2025 10:59:09 +0200 From: Abel Vesa To: Uwe =?iso-8859-1?Q?Kleine-K=F6nig?= Cc: Sebastian Reichel , Lee Jones , Pavel Machek , Anjelique Melendez , Kamal Wadhwa , Jishnu Prakash , Bjorn Andersson , Konrad Dybcio , Johan Hovold , Pavel Machek , linux-leds@vger.kernel.org, linux-kernel@vger.kernel.org, stable@vger.kernel.org Subject: Re: [PATCH] leds: rgb: leds-qcom-lpg: Fix pwm resolution for Hi-Res PWMs Message-ID: References: <5euqboshlfwweie7tlaffajzg3siiy6bm3j4evr572ko54gtbv@7lan3vizskt3> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: On 25-02-27 19:09:39, Uwe Kleine-König wrote: > Hello, > > On Thu, Feb 27, 2025 at 07:05:14PM +0200, Abel Vesa wrote: > > On 25-02-27 18:32:41, Abel Vesa wrote: > > > On 25-02-27 17:44:35, Abel Vesa wrote: > > > > On 25-02-27 16:25:06, Uwe Kleine-König wrote: > > > > > Hello Abel, > > > > > > > > > > On Thu, Feb 27, 2025 at 04:26:14PM +0200, Abel Vesa wrote: > > > > > > On 25-02-27 10:58:47, Uwe Kleine-König wrote: > > > > > > > Can you please enable CONFIG_PWM_DEBUG, enable pwm tracing ( > > > > > > > > > > > > > > echo 1 > /sys/kernel/debug/tracing/events/pwm/enable > > > > > > > > > > > > > > ) then reproduce the problem and provide the output of > > > > > > > > > > > > > > cat /sys/kernel/debug/tracing/trace > > > > > > > > > > > > > > . > > > > > > > > > > > > $ cat trace > > > > > > # tracer: nop > > > > > > # > > > > > > # entries-in-buffer/entries-written: 13/13 #P:12 > > > > > > # > > > > > > # _-----=> irqs-off/BH-disabled > > > > > > # / _----=> need-resched > > > > > > # | / _---=> hardirq/softirq > > > > > > # || / _--=> preempt-depth > > > > > > # ||| / _-=> migrate-disable > > > > > > # |||| / delay > > > > > > # TASK-PID CPU# ||||| TIMESTAMP FUNCTION > > > > > > # | | | ||||| | | > > > > > > modprobe-203 [000] ..... 0.938668: pwm_get: pwmchip0.0: period=1066407 duty_cycle=533334 polarity=0 enabled=1 err=0 > > > > > > modprobe-203 [000] ..... 0.938775: pwm_apply: pwmchip0.0: period=5000000 duty_cycle=0 polarity=0 enabled=1 err=0 > > > > > > modprobe-203 [000] ..... 0.938821: pwm_get: pwmchip0.0: period=4266537 duty_cycle=0 polarity=0 enabled=1 err=0 > > > > > > modprobe-203 [000] ..... 0.938936: pwm_apply: pwmchip0.0: period=4266537 duty_cycle=0 polarity=0 enabled=1 err=0 > > > > > > modprobe-203 [000] ..... 0.938982: pwm_get: pwmchip0.0: period=4266537 duty_cycle=0 polarity=0 enabled=1 err=0 > > > > > > modprobe-203 [000] ..... 0.939274: pwm_apply: pwmchip0.0: period=5000000 duty_cycle=921458 polarity=0 enabled=1 err=0 > > > > > > modprobe-203 [000] ..... 0.939320: pwm_get: pwmchip0.0: period=4266537 duty_cycle=921355 polarity=0 enabled=1 err=0 > > > > > > modprobe-203 [000] ..... 0.939434: pwm_apply: pwmchip0.0: period=4266537 duty_cycle=921355 polarity=0 enabled=1 err=0 > > > > > > modprobe-203 [000] ..... 0.939480: pwm_get: pwmchip0.0: period=4266537 duty_cycle=921355 polarity=0 enabled=1 err=0 > > > > > > systemd-backlig-724 [006] ..... 9.079538: pwm_apply: pwmchip0.0: period=5000000 duty_cycle=5000000 polarity=0 enabled=1 err=0 > > > > > > systemd-backlig-724 [006] ..... 9.079585: pwm_get: pwmchip0.0: period=4266537 duty_cycle=4266537 polarity=0 enabled=1 err=0 > > > > > > systemd-backlig-724 [006] ..... 9.079698: pwm_apply: pwmchip0.0: period=4266537 duty_cycle=4266537 polarity=0 enabled=1 err=0 > > > > > > systemd-backlig-724 [006] ..... 9.079750: pwm_get: pwmchip0.0: period=4266537 duty_cycle=4266537 polarity=0 enabled=1 err=0 > > > > > > $ > > > > > > > > > > > > > > > > > > > > I didn't take a deeper dive in this driver combination, but here is a > > > > > > > description about what *should* happen: > > > > > > > > > > > > > > You're talking about period in MHz, the PWM abstraction uses > > > > > > > nanoseconds. So your summary translated to the PWM wording is (to the > > > > > > > best of my understanding): > > > > > > > > > > > > > > 1. PWM backlight driver requests PWM with .period = 200 ns and > > > > > > > .duty_cycle = 200 ns. > > > > > > > > > > > > > > 2. leds-qcom-lpg cannot pick 200 ns exactly and then chooses .period = > > > > > > > 1000000000 / 4.26666 MHz = 234.375 ns > > > > > > > > > > > > > > 3. leds-qcom-lpg then determines setting for requested .duty_cycle > > > > > > > based on .period = 200 ns which then ends up with something bogus. > > > > > > > > > > The trace looks better than what I expected. 2. is fine here because it > > > > > seems when Sebastian wrote "driver requests PWM with 5 MHz period" that > > > > > meant period = 5000000 ns. That was then rounded down to 4266537 ns. And > > > > > the request for period = 5000000 ns + duty_cycle = 5000000 ns was > > > > > serviced by configuring period = 4266537 ns + duty_cycle = 4266537 ns. > > > > > So that's a 100 % relative duty configuration as intended. > > > > > > > > > > So just from the traces I don't spot a problem. Do these logs not match > > > > > what actually happens on the signal? > > > > > > > > What I do not get is why do we expect 2 pwm_get() and 2 pwm_apply() > > > > calls each time ? > > > > > > OK, so the second pwm_apply() is due to CONFIG_PWM_DEBUG. > > ack. This is done just for the tests implemented in CONFIG_PWM_DEBUG, as > are the two pwm_get()s. > > > > But still, the first pwm_apply() requests duty cycle of 5MHz: > > 5 ms, yes. But it cannot give you 5 ms and so you get 4.266 ns. > > > > systemd-backlig-724 [006] ..... 9.079538: pwm_apply: pwmchip0.0: period=5000000 duty_cycle=5000000 polarity=0 enabled=1 err=0 > > > > > > So since the period is 4.26MHz, due to the knobs selected by the > > > provider, this duty cycle will result in a PWM value that is above the > > > selected resolution, as I already mentioned. > > "above the selected resolution"? Do you mean you don't get the exact > value that you requested? I think I understand your point now. You expectation is that the provider would remap the entire range of the period to whatever the HW can do. So in this case, when 5ms is requested as duty cycle from consumer, the provider will select the max value. What the current implementation of the leds-qcom-lpg does is that will expect a duty cycle request of up to 4.26ms. And according to you, even if the consumer requests 5ms, the leds-qcom-lpg driver should write the value of 255 (which is what the selected resolution allows (1 << 8) ) and not compute a higher value. I think this is wrong though. The fact that the pwm generic framework reports 5ms when it is actually 4.26ms should be considered wrong. For cases where the exact value of the duty cycle matters, this would not even make sense. Correct me if I'm wrong, but the pwm API should behave more like: The consumer should ask for the closest period the HW can actually do and then use that closest period from there on for every duty cycle request. This way, if the consumer initially wants 5ms but the provider can do only 4.26ms instead, at least the consumer would be able to correct its duty cycle requests based on what the HW says it can do. > > > On top of that, the duty cycle in debugfs is also reported as 5000000ns > > when in fact it is 4266666ns, as the trace shows. > > Yes. Consider that a relict from the times when there was no > pwm_get_state_hw(). Both values are interesting in different situations. > So just telling the real parameters isn't the optimal way forward > either. > > Something like the patch I showed in > https://lore.kernel.org/all/7bcnckef23w6g47ll5l3bktygedrcfvr7fk3qjuq2swtoffhec@zs4w4tuh6qvm/ And this patchset only adds the info of actual value that the HW is actually doing. So basically, the already existing state in this case will represent the "desired" state. > would make you a bit luckier I guess. Feel free to polish that one a bit > (e.g. by checking the return value of pwm_get_state_hw() and acting > sensible in reply to it) and send a proper patch. (A Suggested-by for me > is enough for such a patch, grab authorship yourself.) > > > > > Need to dig a bit further. > > > > > > > > But meanwhile, if the first pwm_apply() call goes all the way to the > > > > provider, then the duty cycle value, when translated to the actual PWM > > > > value that gets written to reg, will overflow. > > No it will not. The .duty_cycle value (also 5000000 ns) will reach the > lowlevel PWM driver together with .period = 5000000 ns. Both are rounded > down to 4266666ns. I see no overflow. Again, the consumer is being lied to. It expects 5ms and gets 4.26ms instead. Imagine a device that is controlled via PWM and needs exact duty cycle values in ms, what would the consumer driver do in this case? And to make things worse, when the consumer is asking for duty cycle of 4ms while the period requested is 5ms (which would be 80%), the period the provider will do is actually 4.26ms while the duty cycle would be ~3.41ms, which if the pwm step (reg value) doesn't allow, it will probably result in an actual value that is even further than what the consumer is expecting. So I'm thinking maybe the pwm should probably even ask the provider for what duty cycle it will provide based on provider's provided period and then decide if the resulting duty cycle is what it really wants. IIRC, this is more in line with what the CCF (common clocks framework) currently does. > > Best regards > Uwe