Hi Andre, Thank you for your reply. 2024年6月3日(月) 9:10 Andre Przywara <andre.przywara@xxxxxxx>: > > On Sun, 2 Jun 2024 15:15:13 +0900 > Hironori KIKUCHI <kikuchan98@xxxxxxxxx> wrote: > > Hi Kikuchan, > > > Hi Krzysztof, > > > > > On 31/05/2024 19:57, Hironori KIKUCHI wrote: > > > > Hello, > > > > > > > >>> This patch adds new options to select a clock source and DIV_M register > > > >>> value for each coupled PWM channels. > > > >> > > > >> Please do not use "This commit/patch/change", but imperative mood. See > > > >> longer explanation here: > > > >> https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95 > > > >> > > > >> Bindings are before their users. This should not be last patch, because > > > >> this implies there is no user. > > > > > > > > I'm sorry, I'll fix them. > > > > > > > >> This applies to all variants? Or the one you add? Confused... > > > > > > > > Apologies for confusing you. This applies to all variants. > > > > > > > >> > > > >>> > > > >>> Signed-off-by: Hironori KIKUCHI <kikuchan98@xxxxxxxxx> > > > >>> --- > > > >>> .../bindings/pwm/allwinner,sun20i-pwm.yaml | 19 +++++++++++++++++++ > > > >>> 1 file changed, 19 insertions(+) > > > >>> > > > >>> diff --git a/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml b/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml > > > >>> index b9b6d7e7c87..436a1d344ab 100644 > > > >>> --- a/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml > > > >>> +++ b/Documentation/devicetree/bindings/pwm/allwinner,sun20i-pwm.yaml > > > >>> @@ -45,6 +45,25 @@ properties: > > > >>> description: The number of PWM channels configured for this instance > > > >>> enum: [6, 9] > > > >>> > > > >>> + allwinner,pwm-pair-clock-sources: > > > >>> + description: The clock source names for each PWM pair > > > >>> + items: > > > >>> + enum: [hosc, apb] > > > >>> + minItems: 1 > > > >>> + maxItems: 8 > > > >> > > > >> Missing type... and add 8 of such items to your example to make it complete. > > > > > > > > Thank you. I'll fix it. > > > > > > > >> > > > >>> + > > > >>> + allwinner,pwm-pair-clock-prescales: > > > >>> + description: The prescale (DIV_M register) values for each PWM pair > > > >>> + $ref: /schemas/types.yaml#/definitions/uint32-matrix > > > >>> + items: > > > >>> + items: > > > >>> + minimum: 0 > > > >>> + maximum: 8 > > > >>> + minItems: 1 > > > >>> + maxItems: 1 > > > >>> + minItems: 1 > > > >>> + maxItems: 8 > > > >> > > > >> This does not look like matrix but array. > > > > > > > > I wanted to specify values like this: > > > > > > > > allwinner,pwm-pair-clock-prescales = <0>, <1>, <3>; > > > > allwinner,pwm-pair-clock-sources = "hosc", "apb", "hosc": > > > > > > > > These should correspond to each PWM pair. > > > > This way, I thought we might be able to visually understand the relationship > > > > between prescalers and sources, like clock-names and clocks. > > > > > > > > Is this notation uncommon, perhaps? > > > > > > It's still an array. > > > > Oh I understood and clear. Thank you. > > > > > >> Why clock DIV cannot be deduced from typical PWM attributes + clock > > > >> frequency? > > > > > > > > This SoC's PWM system has one shared prescaler and clock source for each pair > > > > of PWM channels. I should have noted this earlier, sorry. > > > > > > > > Actually, the original v9 patch automatically deduced the DIV value > > > > from the frequency. > > > > However, because the two channels share a single prescaler, once one channel is > > > > enabled, it affects and restricts the DIV value for the other channel > > > > in the pair. > > > > This introduces a problem of determining which channel should set the shared DIV > > > > value. The original behavior was that the first channel enabled would win. > > > > > > There's nothing bad in this. > > > > > > > > > > > Instead, this patch try to resolve the issue by specifying these > > > > values for each PWM > > > > pairs deterministically. > > > > That's why it requires the new options. > > > > > > This does not solve that wrong divider can be programmed for second > > > channel in each pair. > > > > > > > Let me illustrate the connection of a paired PWM channels to be sure. > > > > . +------+ +--------------+ +------+ > > . + HOSC +--+ +--+ prescale_k 0 +--+ PWM0 | > > . +------+ | +-------+ | +--------------+ +------+ > > . +--+ DIV_M +--+ > > . +------+ | +-------+ | +--------------+ +------+ > > . + APBx +--+ +--+ prescale_k 1 +--+ PWM1 | > > . +------+ +--------------+ +------+ > > . CLK_SRC > > > > The PWM0 and PWM1 share DIV_M and CLK_SRC for them, and (not > > illustrated) PWM2 and PWM3 share another DIV_M and another CLK_SRC for > > them, and so on. > > The DIV_M ranges from 0 to 8 and is used as a 1 / 2^DIV_M prescaler, > > prescale_k ranges from 0 to 255 and is used as a 1 / (prescale_k + 1) > > prescaler. > > > > In the original v9 patch, enabling PWM0 determines CLK_SRC and > > calculates DIV_M from the period that is going to be set. > > Once the CLK_SRC and DIV_M are fixed, they cannot be changed until > > both channels are disabled, unless PWM0 is the only enabled channel. > > > > Looks good so far, but there is a pitfall. > > > > Selecting CLK_SRC and DIV_M means it defines the PWM resolution of the > > period and duty cycle for the pair of the PWM channels. > > In other words, the resolution is determined by the (most likely the > > very first) period, which can be arbitrary. > > So I understand the problem, but I don't think expressing this in the > devicetree is the right solution. It seems like a tempting pragmatical > approach, but it sounds like the wrong way: this is not a hardware > *description* of any kind, but rather a way to describe a certain user > intention or a configuration. So this looks like a rather embedded > approach to me, where you have a certain fixed register setup in mind, > and want to somehow force this to the hardware. > Another problem with this approach is that it doesn't really cover the > sysfs interface, which is very dynamic by nature. ... Indeed. You're right. Now I've realized it was a bad idea. It should be done in sysfs or sysctl perhaps. > > I have some questions / ideas, and would love to hear some feedback on > them: > - If some PWM channels are "linked", I don't think there is much we can > do about it: it's a hardware limitation. The details of that is > already "encoded" in the compatible string, I'd say, so there is no > need for further description in the devicetree. Any PWM user on those > boards would probably need to know about the shortcomings, and either > use different channels for wildly different PWM setups, or accept > that there are actually only three freely programmable PWM channels. > - Does the PWM subsystem already have a way to model linked channels? > Maybe that problem is solved already elsewhere? > - Previous Allwinner PWM IP was restricted to use the 24 MHz > oscillator only, and people seem to have survived with that. Can we > not just restrict ourselves to one clock source for those linked > channels? I would assume that the PWM frequency is less important > than the duty cycle? > - Can't we just return an error if some conflicting setup requests are > made? At the expense of this seeming somewhat random to the user, > because it depends on the order of requests? But people could then > react on the returned error value? > > In general, I wonder what the real use cases are, maybe it's not a > problem in real life? Do you have a concrete issue at hand, or is this > just thinking about all potential use cases - which is honourable, but > maybe a bit over the top here? IMHO, it is sufficient to use fixed CLK_SRC and DIV_M values for this driver, since the default values (CLK_SRC == hosc and DIV_M == 0) already provide enough range in real life. What I really care about is minimizing complexity and avoiding surprises. Although the original method enables an incredibly wide range of the period, it introduces unpredictability in resolution and inequity in a pair due to a race in the order of enabling, as a drawback. If the primary concern is achieving such a wide range, then I think the original method is the most suitable option. > > Cheers, > Andre > Best regards, kikuchan.
