| Message ID | 20240531141152.327592-6-kikuchan98@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add support for Allwinner H616 PWM | expand |
On 31/05/2024 16:11, Hironori KIKUCHI wrote: > 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. This applies to all variants? Or the one you add? Confused... > > Signed-off-by: Hironori KIKUCHI <kikuchan98@gmail.com> > --- > .../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. > + > + 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. Why clock DIV cannot be deduced from typical PWM attributes + clock frequency? Best regards, Krzysztof
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@gmail.com> > > --- > > .../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? > > 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. 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. > > Best regards, > Krzysztof Best regards, kikuchan.
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@gmail.com> >>> --- >>> .../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. > >> >> 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. Best regards, Krzysztof
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@gmail.com> > >>> --- > >>> .../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. Consider an application that uses PWM channels to generate a square wave in stereo. The very first musical note played defines the entire resolution for the subsequent notes. The music quality depends on the first note. The problem is, there is NO way to fixate the resolution to be used. The proposed method provides a simple way to deterministically fixate the resolution. (ofcourse, prescale_k is still calculated by period to be set) > Best regards, > Krzysztof Best regards, kikuchan.
On Sun, 2 Jun 2024 15:15:13 +0900 Hironori KIKUCHI <kikuchan98@gmail.com> 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@gmail.com> > > >>> --- > > >>> .../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. 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? Cheers, Andre > Consider an application that uses PWM channels to generate a square > wave in stereo. > The very first musical note played defines the entire resolution for > the subsequent notes. > The music quality depends on the first note. > > The problem is, there is NO way to fixate the resolution to be used. > > The proposed method provides a simple way to deterministically fixate > the resolution. > (ofcourse, prescale_k is still calculated by period to be set) > > > Best regards, > > Krzysztof > > Best regards, > kikuchan. >
Hi Andre, Thank you for your reply. 2024年6月3日(月) 9:10 Andre Przywara <andre.przywara@arm.com>: > > On Sun, 2 Jun 2024 15:15:13 +0900 > Hironori KIKUCHI <kikuchan98@gmail.com> 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@gmail.com> > > > >>> --- > > > >>> .../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.
Hello, On Mon, Jun 03, 2024 at 05:42:08PM +0900, Hironori KIKUCHI wrote: > 2024年6月3日(月) 9:10 Andre Przywara <andre.przywara@arm.com>: > > > > On Sun, 2 Jun 2024 15:15:13 +0900 > > Hironori KIKUCHI <kikuchan98@gmail.com> wrote: > > > > > > On 31/05/2024 19:57, Hironori KIKUCHI wrote: > > > > >>> --- > > > > >>> .../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. For clocks there is already a binding to assign a working configuration. assigned-clocks, assigned-clock-rates and assigned-clock-parents are the relevant properties. If you create a clk from the parent clock selector and mdiv, you can stick to the existing bindings. > > > [...] > > > > 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 don't think a driver specific knob somewhere is a practical solution. Best regards Uwe
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 + + 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 + allOf: - $ref: pwm.yaml#
This patch adds new options to select a clock source and DIV_M register value for each coupled PWM channels. Signed-off-by: Hironori KIKUCHI <kikuchan98@gmail.com> --- .../bindings/pwm/allwinner,sun20i-pwm.yaml | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)