| Message ID | 20241018-en7581-pinctrl-v8-0-b676b966a1d1@kernel.org |
|---|---|
| Headers | show |
| Series | Add mfd, pinctrl and pwm support to EN7581 SoC | expand |
Il 18/10/24 15:19, Lorenzo Bianconi ha scritto: > Introduce device-tree binding documentation for Airoha EN7581 pwm > controller. > > Co-developed-by: Christian Marangi <ansuelsmth@gmail.com> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> > --- > .../devicetree/bindings/pwm/airoha,en7581-pwm.yaml | 61 ++++++++++++++++++++++ > 1 file changed, 61 insertions(+) > > diff --git a/Documentation/devicetree/bindings/pwm/airoha,en7581-pwm.yaml b/Documentation/devicetree/bindings/pwm/airoha,en7581-pwm.yaml > new file mode 100644 > index 0000000000000000000000000000000000000000..fb68c10b037b840a571a2ceee57f13cbae78da66 > --- /dev/null > +++ b/Documentation/devicetree/bindings/pwm/airoha,en7581-pwm.yaml > @@ -0,0 +1,61 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/pwm/airoha,en7581-pwm.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Airoha EN7581 PWM Controller > + > +maintainers: > + - Lorenzo Bianconi <lorenzo@kernel.org> > + > +allOf: > + - $ref: pwm.yaml# > + > +properties: > + compatible: > + const: airoha,en7581-pwm > + > + "#pwm-cells": > + const: 3 > + > + airoha,74hc595-mode: > + description: Set the PWM to handle attached shift register chip 74HC595. > + I think that you can either indent your description or you just don't; this means that - if you don't - you shouldn't add extra blank lines. In any case, that's left for the bindings maintainer to check - as for the missing new properties I complained about in the previous version, you can get my Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> Cheers, Angelo > + With this disabled, PWM assume a 74HC164 chip attached. > + > + The main difference between the 2 chip is the presence of a latch pin > + that needs to triggered to apply the configuration and PWM needs to > + account for that. > + type: boolean > + > + airoha,sipo-clock-divisor: > + description: Declare Shift Register chip clock divisor (clock source is > + from SoC APB Clock) > + $ref: /schemas/types.yaml#/definitions/uint32 > + default: 32 > + enum: [4, 8, 16, 32] > + > + airoha,sipo-clock-delay: > + description: Declare Serial GPIO Clock delay. > + This can be needed to permit the attached shift register to correctly > + setup and apply settings. Value must NOT be greater than > + "airoha,sipo-clock-divisor" / 2 > + $ref: /schemas/types.yaml#/definitions/uint32 > + default: 1 > + minimum: 1 > + maximum: 16 > + > +required: > + - compatible > + - "#pwm-cells" > + > +additionalProperties: false > + > +examples: > + - | > + pwm { > + compatible = "airoha,en7581-pwm"; > + > + #pwm-cells = <3>; > + }; >
Il 18/10/24 15:19, Lorenzo Bianconi ha scritto: > This patch adds the chip-scu document bindings for EN7581 SoC. > The airoha chip-scu block provides a configuration interface for clock, > io-muxing and other functionalities used by multiple controllers (e.g. > clock, pinctrl, ecc.) on EN7581 SoC. > > Reviewed-by: Rob Herring (Arm) <robh@kernel.org> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
On Fri, Oct 18, 2024 at 03:19:04PM +0200, Lorenzo Bianconi wrote: > Introduce device-tree binding documentation for Airoha EN7581 pwm > controller. > > Co-developed-by: Christian Marangi <ansuelsmth@gmail.com> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> > --- > .../devicetree/bindings/pwm/airoha,en7581-pwm.yaml | 61 ++++++++++++++++++++++ > 1 file changed, 61 insertions(+) > > diff --git a/Documentation/devicetree/bindings/pwm/airoha,en7581-pwm.yaml b/Documentation/devicetree/bindings/pwm/airoha,en7581-pwm.yaml > new file mode 100644 > index 0000000000000000000000000000000000000000..fb68c10b037b840a571a2ceee57f13cbae78da66 > --- /dev/null > +++ b/Documentation/devicetree/bindings/pwm/airoha,en7581-pwm.yaml > @@ -0,0 +1,61 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/pwm/airoha,en7581-pwm.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Airoha EN7581 PWM Controller > + > +maintainers: > + - Lorenzo Bianconi <lorenzo@kernel.org> > + > +allOf: > + - $ref: pwm.yaml# > + > +properties: > + compatible: > + const: airoha,en7581-pwm > + > + "#pwm-cells": > + const: 3 > + > + airoha,74hc595-mode: > + description: Set the PWM to handle attached shift register chip 74HC595. > + > + With this disabled, PWM assume a 74HC164 chip attached. > + > + The main difference between the 2 chip is the presence of a latch pin > + that needs to triggered to apply the configuration and PWM needs to > + account for that. > + type: boolean > + > + airoha,sipo-clock-divisor: > + description: Declare Shift Register chip clock divisor (clock source is > + from SoC APB Clock) Where is the clock source defined? You can specify the PWM frequency in PWM cells and should be able to get the APB Clock frequency. Then you can calculate the divider. > + $ref: /schemas/types.yaml#/definitions/uint32 > + default: 32 > + enum: [4, 8, 16, 32] > + > + airoha,sipo-clock-delay: > + description: Declare Serial GPIO Clock delay. > + This can be needed to permit the attached shift register to correctly > + setup and apply settings. Value must NOT be greater than > + "airoha,sipo-clock-divisor" / 2 > + $ref: /schemas/types.yaml#/definitions/uint32 > + default: 1 > + minimum: 1 > + maximum: 16 > + > +required: > + - compatible > + - "#pwm-cells" > + > +additionalProperties: false > + > +examples: > + - | > + pwm { > + compatible = "airoha,en7581-pwm"; > + > + #pwm-cells = <3>; > + }; > > -- > 2.47.0 >
On Mon, Oct 21, 2024 at 02:00:53PM -0500, Rob Herring wrote: > On Fri, Oct 18, 2024 at 03:19:04PM +0200, Lorenzo Bianconi wrote: > > Introduce device-tree binding documentation for Airoha EN7581 pwm > > controller. > > > > Co-developed-by: Christian Marangi <ansuelsmth@gmail.com> > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> > > --- > > .../devicetree/bindings/pwm/airoha,en7581-pwm.yaml | 61 ++++++++++++++++++++++ > > 1 file changed, 61 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/pwm/airoha,en7581-pwm.yaml b/Documentation/devicetree/bindings/pwm/airoha,en7581-pwm.yaml > > new file mode 100644 > > index 0000000000000000000000000000000000000000..fb68c10b037b840a571a2ceee57f13cbae78da66 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/pwm/airoha,en7581-pwm.yaml > > @@ -0,0 +1,61 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/pwm/airoha,en7581-pwm.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Airoha EN7581 PWM Controller > > + > > +maintainers: > > + - Lorenzo Bianconi <lorenzo@kernel.org> > > + > > +allOf: > > + - $ref: pwm.yaml# > > + > > +properties: > > + compatible: > > + const: airoha,en7581-pwm > > + > > + "#pwm-cells": > > + const: 3 > > + > > + airoha,74hc595-mode: > > + description: Set the PWM to handle attached shift register chip 74HC595. > > + > > + With this disabled, PWM assume a 74HC164 chip attached. > > + > > + The main difference between the 2 chip is the presence of a latch pin > > + that needs to triggered to apply the configuration and PWM needs to > > + account for that. > > + type: boolean > > + > > + airoha,sipo-clock-divisor: > > + description: Declare Shift Register chip clock divisor (clock source is > > + from SoC APB Clock) > > Where is the clock source defined? > > You can specify the PWM frequency in PWM cells and should be able to get > the APB Clock frequency. Then you can calculate the divider. > Hi Rob, this property is related to the Shift Register chip and is not related to the clock of the PWM. It's really to configure the clock that will be feed to Shift Register chip if for whatever reason one OEM mount a different kind (but still register compatible) and requires to run at higher clock rate. We can consider hardcoding it if really needed but considering the case with 2 different kind of shift register supported, I assume configuring this might be needed on some corner case Devices. For the clock we are not 100% but we might have an idea of what is the source, but still it will be just referenced and enabled in the driver (it's always enabled). Hope I can get some hint by you on how to proceed. Is it ok with: - Defining the attached clock - Keep the property ? > > + $ref: /schemas/types.yaml#/definitions/uint32 > > + default: 32 > > + enum: [4, 8, 16, 32] > > + > > + airoha,sipo-clock-delay: > > + description: Declare Serial GPIO Clock delay. > > + This can be needed to permit the attached shift register to correctly > > + setup and apply settings. Value must NOT be greater than > > + "airoha,sipo-clock-divisor" / 2 > > + $ref: /schemas/types.yaml#/definitions/uint32 > > + default: 1 > > + minimum: 1 > > + maximum: 16 > > + > > +required: > > + - compatible > > + - "#pwm-cells" > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > + pwm { > > + compatible = "airoha,en7581-pwm"; > > + > > + #pwm-cells = <3>; > > + }; > > > > -- > > 2.47.0 > >
On 21/10/2024 21:00, Rob Herring wrote: >> + airoha,sipo-clock-divisor: >> + description: Declare Shift Register chip clock divisor (clock source is >> + from SoC APB Clock) > Where is the clock source defined? > > You can specify the PWM frequency in PWM cells and should be able to get > the APB Clock frequency. Then you can calculate the divider. Hi this controls SRCLK for shift registers. https://www.ti.com/lit/ds/symlink/sn74hc595.pdf page 8 has the limits for this chip. I am fine with removal of this option. The main use of this pwm driver is for leds and for that purpose the default is ok. But I will try to get a value for the clock source, I guess with that value the option can stay in. MvH Benjamin Larsson
On 21/10/2024 21:00, Rob Herring wrote: >> + airoha,sipo-clock-divisor: >> + description: Declare Shift Register chip clock divisor (clock source is >> + from SoC APB Clock) > Where is the clock source defined? > By measurement the clock was found to be 125MHz. MvH Benjamin Larsson
On Tue, Oct 22, 2024 at 10:02:05PM +0200, Benjamin Larsson wrote: > On 21/10/2024 21:00, Rob Herring wrote: > > > + airoha,sipo-clock-divisor: > > > + description: Declare Shift Register chip clock divisor (clock source is > > > + from SoC APB Clock) > > Where is the clock source defined? > > > By measurement the clock was found to be 125MHz. What I mean is the clock input should be a 'clocks' property. Assuming this is a clock input to the PWM which I'm not so sure about given the other replies. Rob
On Tue, Oct 22, 2024 at 04:08:58PM -0500, Rob Herring wrote: > On Tue, Oct 22, 2024 at 10:02:05PM +0200, Benjamin Larsson wrote: > > On 21/10/2024 21:00, Rob Herring wrote: > > > > + airoha,sipo-clock-divisor: > > > > + description: Declare Shift Register chip clock divisor (clock source is > > > > + from SoC APB Clock) > > > Where is the clock source defined? > > > > > By measurement the clock was found to be 125MHz. > > What I mean is the clock input should be a 'clocks' property. Assuming > this is a clock input to the PWM which I'm not so sure about given the > other replies. > Yep it's not, we are just dropping this property and hardcoding it to a sensible value in the driver. Sorry for the confusion and thanks for the clarification.
On Tue, Oct 22, 2024 at 06:06:06PM +0200, Christian Marangi wrote: > On Mon, Oct 21, 2024 at 02:00:53PM -0500, Rob Herring wrote: > > On Fri, Oct 18, 2024 at 03:19:04PM +0200, Lorenzo Bianconi wrote: > > > Introduce device-tree binding documentation for Airoha EN7581 pwm > > > controller. > > > > > > Co-developed-by: Christian Marangi <ansuelsmth@gmail.com> > > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com> > > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> > > > --- > > > .../devicetree/bindings/pwm/airoha,en7581-pwm.yaml | 61 ++++++++++++++++++++++ > > > 1 file changed, 61 insertions(+) > > > > > > diff --git a/Documentation/devicetree/bindings/pwm/airoha,en7581-pwm.yaml b/Documentation/devicetree/bindings/pwm/airoha,en7581-pwm.yaml > > > new file mode 100644 > > > index 0000000000000000000000000000000000000000..fb68c10b037b840a571a2ceee57f13cbae78da66 > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/pwm/airoha,en7581-pwm.yaml > > > @@ -0,0 +1,61 @@ > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > > +%YAML 1.2 > > > +--- > > > +$id: http://devicetree.org/schemas/pwm/airoha,en7581-pwm.yaml# > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > + > > > +title: Airoha EN7581 PWM Controller > > > + > > > +maintainers: > > > + - Lorenzo Bianconi <lorenzo@kernel.org> > > > + > > > +allOf: > > > + - $ref: pwm.yaml# > > > + > > > +properties: > > > + compatible: > > > + const: airoha,en7581-pwm > > > + > > > + "#pwm-cells": > > > + const: 3 > > > + > > > + airoha,74hc595-mode: > > > + description: Set the PWM to handle attached shift register chip 74HC595. > > > + > > > + With this disabled, PWM assume a 74HC164 chip attached. > > > + > > > + The main difference between the 2 chip is the presence of a latch pin > > > + that needs to triggered to apply the configuration and PWM needs to > > > + account for that. > > > + type: boolean > > > + > > > + airoha,sipo-clock-divisor: > > > + description: Declare Shift Register chip clock divisor (clock source is > > > + from SoC APB Clock) > > > > Where is the clock source defined? > > > > You can specify the PWM frequency in PWM cells and should be able to get > > the APB Clock frequency. Then you can calculate the divider. > > > > Hi Rob, > > this property is related to the Shift Register chip and is not related > to the clock of the PWM. > > It's really to configure the clock that will be feed to Shift Register > chip if for whatever reason one OEM mount a different kind (but still > register compatible) and requires to run at higher clock rate. > > We can consider hardcoding it if really needed but considering the case > with 2 different kind of shift register supported, I assume configuring > this might be needed on some corner case Devices. > > For the clock we are not 100% but we might have an idea of what is the > source, but still it will be just referenced and enabled in the driver > (it's always enabled). > > Hope I can get some hint by you on how to proceed. > > Is it ok with: > > - Defining the attached clock > - Keep the property > > ? So how is the PWM involved? I'm going to need a picture. If this external shift register chip can be attached to any PWM and clock providers, then perhaps it needs to be its own node with a 'pwms' property and clock source. I would suggest you go back to the version without these properties and that I already reviewed, then discuss adding them separately. Rob
On 22/10/2024 23:08, Rob Herring wrote: > On Tue, Oct 22, 2024 at 10:02:05PM +0200, Benjamin Larsson wrote: >> On 21/10/2024 21:00, Rob Herring wrote: >>>> + airoha,sipo-clock-divisor: >>>> + description: Declare Shift Register chip clock divisor (clock source is >>>> + from SoC APB Clock) >>> Where is the clock source defined? >>> >> By measurement the clock was found to be 125MHz. > What I mean is the clock input should be a 'clocks' property. Assuming > this is a clock input to the PWM which I'm not so sure about given the > other replies. > > Rob Hi, the SoC has 8 pwm generators. The pwm signals from the generators can be muxed out almost everywhere. One place is on the pins of a shift register chip. This setting configures the clock signal this shift register chip is fed by the SoC. The main scenario for the shift register is to drive leds. For that the default value is perfectly fine and the property can be dropped. MvH Benjamin Larsson
On 22/10/2024 23:17, Rob Herring wrote: > So how is the PWM involved? I'm going to need a picture. > > If this external shift register chip can be attached to any PWM and > clock providers, then perhaps it needs to be its own node with a 'pwms' > property and clock source. > > I would suggest you go back to the version without these properties > and that I already reviewed, then discuss adding them separately. > > Rob Hi, to answer your question. The shift register is not attached to any PWM output, there is a specific pin function (mux) that the shift register needs to be connected to for the (pwm capable) gpio hardware to be able to drive it. If that is done the shift register pins can output pwm signals. So if you are low on gpio pins for leds you can connect a shift register and serialize the state of leds connected to the shift register. You can even connect several shift registers chips in a chain with a total of 17 outputs. The kernel already has support to drive shift register leds but with pwm you can do fancy stuff like dimming and color mixing. MvH Benjamin Larsson
Introduce airoha-mfd driver in order to load pinctrl and pwm drivers for EN7581 SoC. airoha-mfd is needed since both pinctrl and pwm drivers needs to access the same memory block (gpio memory region) to configure {gio,irq}_chip and pwm functionalities respectively, so model them as childs of a parent mfd driver. Current EN7581 pinctrl driver supports the following functionalities: - pin multiplexing via chip_scu syscon - pin pull-up, pull-down, open-drain, current strength, {input,output}_enable, output_{low,high} via chip_scu syscon - gpio controller - irq controller --- Changes in v8: - pwm: add missing properties documentation - Link to v7: https://lore.kernel.org/r/20241016-en7581-pinctrl-v7-0-4ff611f263a7@kernel.org Changes in v7: - pinctrl: cosmetics - pinctrl: fix compilation warning - Link to v6: https://lore.kernel.org/r/20241013-en7581-pinctrl-v6-0-2048e2d099c2@kernel.org Changes in v6: - pwm: rely on regmap APIs - pwm: introduce compatible string - pinctrl: introduce compatible string - remove airoha-mfd driver - add airoha,en7581-pinctrl binding - add airoha,en7581-pwm binding - update airoha,en7581-gpio-sysctl binding - Link to v5: https://lore.kernel.org/r/20241001-en7581-pinctrl-v5-0-dc1ce542b6c6@kernel.org Changes in v5: - use spin_lock in airoha_pinctrl_rmw instead of a mutex since it can run in interrupt context - remove unused includes in pinctrl driver - since the irq_chip is immutable, allocate the gpio_irq_chip struct statically in pinctrl driver - rely on regmap APIs in pinctrl driver but keep the spin_lock local to the driver - rely on guard/guard_scope APIs in pinctrl driver - improve naming convention pinctrl driver - introduce airoha_pinconf_set_pin_value utility routine - Link to v4: https://lore.kernel.org/r/20240911-en7581-pinctrl-v4-0-60ac93d760bb@kernel.org Changes in v4: - add 'Limitation' description in pwm driver - fix comments in pwm driver - rely on mfd->base __iomem pointer in pwm driver, modify register offsets according to it and get rid of sgpio_cfg, flash_cfg and cycle_cfg pointers - simplify register utility routines in pwm driver - use 'generator' instead of 'waveform' suffix for pwm routines - fix possible overflow calculating duty cycle in pwm driver - do not modify pwm state in free callback in pwm driver - cap the maximum period in pwm driver - do not allow inverse polarity in pwm driver - do not set of_xlate callback in the pwm driver and allow the stack to do it - fix MAINTAINERS file for airoha pinctrl driver - fix undefined reference to __ffsdi2 in pinctrl driver - simplify airoha,en7581-gpio-sysctl.yam binding - Link to v3: https://lore.kernel.org/r/20240831-en7581-pinctrl-v3-0-98eebfb4da66@kernel.org Changes in v3: - introduce airoha-mfd driver - add pwm driver to the same series - model pinctrl and pwm drivers as childs of a parent mfd driver. - access chip-scu memory region in pinctrl driver via syscon - introduce a single airoha,en7581-gpio-sysctl.yaml binding and get rid of dedicated bindings for pinctrl and pwm - add airoha,en7581-chip-scu.yaml binding do the series - Link to v2: https://lore.kernel.org/r/20240822-en7581-pinctrl-v2-0-ba1559173a7f@kernel.org Changes in v2: - Fix compilation errors - Collapse some register mappings for gpio and irq controllers - update dt-bindings according to new register mapping - fix some dt-bindings errors - Link to v1: https://lore.kernel.org/all/cover.1723392444.git.lorenzo@kernel.org/ --- Benjamin Larsson (1): pwm: airoha: Add support for EN7581 SoC Christian Marangi (1): dt-bindings: mfd: Add support for Airoha EN7581 GPIO System Controller Lorenzo Bianconi (4): dt-bindings: arm: airoha: Add the chip-scu node for EN7581 SoC dt-bindings: pinctrl: airoha: Add EN7581 pinctrl dt-bindings: pwm: airoha: Add EN7581 pwm pinctrl: airoha: Add support for EN7581 SoC .../bindings/arm/airoha,en7581-chip-scu.yaml | 42 + .../bindings/mfd/airoha,en7581-gpio-sysctl.yaml | 90 + .../bindings/pinctrl/airoha,en7581-pinctrl.yaml | 400 +++ .../devicetree/bindings/pwm/airoha,en7581-pwm.yaml | 61 + MAINTAINERS | 7 + drivers/pinctrl/mediatek/Kconfig | 17 +- drivers/pinctrl/mediatek/Makefile | 1 + drivers/pinctrl/mediatek/pinctrl-airoha.c | 2970 ++++++++++++++++++++ drivers/pwm/Kconfig | 11 + drivers/pwm/Makefile | 1 + drivers/pwm/pwm-airoha.c | 421 +++ 11 files changed, 4020 insertions(+), 1 deletion(-) --- base-commit: e4188772459fec428bf85ce6711a0147387c1455 change-id: 20240818-en7581-pinctrl-1bf120154be0 prerequisite-change-id: 20240705-for-6-11-bpf-a349efc08df8:v2 Best regards,