| Message ID | 20220131114726.973690-1-conor.dooley@microchip.com |
|---|---|
| Headers | show |
| Series | Update the Icicle Kit device tree | expand |
On Mon, 31 Jan 2022 11:47:18 +0000, conor.dooley@microchip.com wrote: > From: Conor Dooley <conor.dooley@microchip.com> > > Add device tree bindings for the i2c controller on > the Microchip PolarFire SoC. > > Reviewed-by: Rob Herring <robh@kernel.org> > > Signed-off-by: Daire McNamara <daire.mcnamara@microchip.com> > Signed-off-by: Conor Dooley <conor.dooley@microchip.com> > --- > .../bindings/i2c/microchip,mpfs-i2c.yaml | 57 +++++++++++++++++++ > 1 file changed, 57 insertions(+) > create mode 100644 Documentation/devicetree/bindings/i2c/microchip,mpfs-i2c.yaml > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: ./Documentation/devicetree/bindings/i2c/microchip,mpfs-i2c.yaml:19:9: [warning] wrong indentation: expected 10 but found 8 (indentation) dtschema/dtc warnings/errors: Documentation/devicetree/bindings/i2c/microchip,mpfs-i2c.example.dts:19:18: fatal error: dt-bindings/clock/microchip,mpfs-clock.h: No such file or directory 19 | #include <dt-bindings/clock/microchip,mpfs-clock.h> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ compilation terminated. make[1]: *** [scripts/Makefile.lib:378: Documentation/devicetree/bindings/i2c/microchip,mpfs-i2c.example.dt.yaml] Error 1 make[1]: *** Waiting for unfinished jobs.... make: *** [Makefile:1398: dt_binding_check] Error 2 doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/patch/1586674 This check can fail if there are any dependencies. The base for a patch series is generally the most recent rc1. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit.
On Mon, 31 Jan 2022 11:47:19 +0000, conor.dooley@microchip.com wrote: > From: Conor Dooley <conor.dooley@microchip.com> > > Add device tree bindings for the real time clock on > the Microchip PolarFire SoC. > > Signed-off-by: Daire McNamara <daire.mcnamara@microchip.com> > Signed-off-by: Conor Dooley <conor.dooley@microchip.com> > --- > .../bindings/rtc/microchip,mfps-rtc.yaml | 58 +++++++++++++++++++ > 1 file changed, 58 insertions(+) > create mode 100644 Documentation/devicetree/bindings/rtc/microchip,mfps-rtc.yaml > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: dtschema/dtc warnings/errors: Documentation/devicetree/bindings/rtc/microchip,mfps-rtc.example.dts:19:18: fatal error: dt-bindings/clock/microchip,mpfs-clock.h: No such file or directory 19 | #include <dt-bindings/clock/microchip,mpfs-clock.h> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ compilation terminated. make[1]: *** [scripts/Makefile.lib:378: Documentation/devicetree/bindings/rtc/microchip,mfps-rtc.example.dt.yaml] Error 1 make[1]: *** Waiting for unfinished jobs.... make: *** [Makefile:1398: dt_binding_check] Error 2 doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/patch/1586704 This check can fail if there are any dependencies. The base for a patch series is generally the most recent rc1. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit.
On Mon, Jan 31, 2022 at 5:45 AM <conor.dooley@microchip.com> wrote: > > From: Conor Dooley <conor.dooley@microchip.com> > > Add device tree bindings for the i2c controller on > the Microchip PolarFire SoC. > > Reviewed-by: Rob Herring <robh@kernel.org> > There should not be a blank line here. Also, tags should be in chronological order typically. If Daire sent this patch out with my tag, then the order is correct. If I gave it on a version you sent, then it goes between Daire's and your S-o-b which is the case here. Rob > Signed-off-by: Daire McNamara <daire.mcnamara@microchip.com> > Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
On Mon, Jan 31, 2022 at 11:47:21AM +0000, conor.dooley@microchip.com wrote: > From: Conor Dooley <conor.dooley@microchip.com> > > Add device tree bindings for the Microchip fpga fabric based "core" PWM > controller. > > Reviewed-by: Rob Herring <robh@kernel.org> > > Signed-off-by: Conor Dooley <conor.dooley@microchip.com> > --- > .../bindings/pwm/microchip,corepwm.yaml | 75 +++++++++++++++++++ > 1 file changed, 75 insertions(+) > create mode 100644 Documentation/devicetree/bindings/pwm/microchip,corepwm.yaml > > diff --git a/Documentation/devicetree/bindings/pwm/microchip,corepwm.yaml b/Documentation/devicetree/bindings/pwm/microchip,corepwm.yaml > new file mode 100644 > index 000000000000..26a77cde2465 > --- /dev/null > +++ b/Documentation/devicetree/bindings/pwm/microchip,corepwm.yaml > @@ -0,0 +1,75 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > + > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/pwm/microchip,corepwm.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Microchip ip core PWM controller bindings > + > +maintainers: > + - Conor Dooley <conor.dooley@microchip.com> > + > +description: | > + corePWM is an 16 channel pulse width modulator FPGA IP > + > + https://www.microsemi.com/existing-parts/parts/152118 > + > +properties: > + compatible: > + items: > + - const: microchip,corepwm-rtl-v4 > + > + reg: > + maxItems: 1 > + > + clocks: > + maxItems: 1 > + > + "#pwm-cells": > + const: 2 > + > + microchip,sync-update: > + description: | > + In synchronous mode, all channels are updated at the beginning of the PWM period. > + Asynchronous mode is relevant to applications such as LED control, where > + synchronous updates are not required. Asynchronous mode lowers the area size, > + reducing shadow register requirements. This can be set at run time, provided > + SHADOW_REG_EN is asserted. SHADOW_REG_EN is set by the FPGA bitstream programmed > + to the device. > + Each bit corresponds to a PWM channel & represents whether synchronous mode is > + possible for the PWM channel. > + > + $ref: /schemas/types.yaml#/definitions/uint16 > + default: 0 I'm not sure I understand this correctly. This is a soft-core and you can synthesize it either with or without the ability to do synchronous updates or not, right? All 16 channels share the same period length and in the simple implementation changing the duty cycle is done at once (maybe introducing a glitch) and in the more expensive implementation there is a register to implement both variants? > + microchip,dac-mode: > + description: | > + Optional, per-channel Low Ripple DAC mode is possible on this IP core. It creates > + a minimum period pulse train whose High/Low average is that of the chosen duty > + cycle. This "DAC" will have far better bandwidth and ripple performance than the > + standard PWM algorithm can achieve. > + Each bit corresponds to a PWM channel & represents whether dac mode is enabled > + that PWM channel. In the last sentence a "for" is missing? These two properties are not detectable in software? Best regards Uwe
>On 01/02/2022 07:58, Uwe Kleine-König wrote: >> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >> On Mon, Jan 31, 2022 at 11:47:21AM +0000, conor.dooley@microchip.com wrote: >> From: Conor Dooley <conor.dooley@microchip.com> >> >> Add device tree bindings for the Microchip fpga fabric based "core" PWM >> controller. >> >> Reviewed-by: Rob Herring <robh@kernel.org> >> >> Signed-off-by: Conor Dooley <conor.dooley@microchip.com> >> --- >> .../bindings/pwm/microchip,corepwm.yaml | 75 +++++++++++++++++++ <snip> >> + microchip,sync-update: >> + description: | >> + In synchronous mode, all channels are updated at the beginning of the PWM period. >> + Asynchronous mode is relevant to applications such as LED control, where >> + synchronous updates are not required. Asynchronous mode lowers the area size, >> + reducing shadow register requirements. This can be set at run time, provided >> + SHADOW_REG_EN is asserted. SHADOW_REG_EN is set by the FPGA bitstream programmed >> + to the device. >> + Each bit corresponds to a PWM channel & represents whether synchronous mode is >> + possible for the PWM channel. >> + >> + $ref: /schemas/types.yaml#/definitions/uint16 >> + default: 0 > >I'm not sure I understand this correctly. This is a soft-core and you >can synthesize it either with or without the ability to do synchronous >updates or not, right? All 16 channels share the same period length and >in the simple implementation changing the duty cycle is done at once >(maybe introducing a glitch) and in the more expensive implementation >there is a register to implement both variants? Correct. If the IP is instantiated with SHADOW_REG_ENx=1, both registers that control the duty cycle for channel x have a second "shadow reg" synthesised. At runtime a bit wide register exposed to APB can be used to toggle on/off synchronised mode for all channels it has been synthesised for. I will reword this description since it is not clear. >> + microchip,dac-mode: >> + description: | >> + Optional, per-channel Low Ripple DAC mode is possible on this IP core. It creates >> + a minimum period pulse train whose High/Low average is that of the chosen duty >> + cycle. This "DAC" will have far better bandwidth and ripple performance than the >> + standard PWM algorithm can achieve. >> + Each bit corresponds to a PWM channel & represents whether dac mode is enabled >> + that PWM channel. > >In the last sentence a "for" is missing? It is missing, thanks. >These two properties are not detectable in software? Unfortunately not. THe configuration for these options are only accessible in the fpga design. You make a good point however & they really should be visible to software. I'll suggest that for future revisions of this IP that both configurations are accessible over APB Thanks, Conor.
Hi Conor, On Wed, Feb 2, 2022 at 1:33 PM <conor.dooley@microchip.com> wrote: > >On 01/02/2022 07:58, Uwe Kleine-König wrote: > >> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > >> On Mon, Jan 31, 2022 at 11:47:21AM +0000, conor.dooley@microchip.com wrote: > >> From: Conor Dooley <conor.dooley@microchip.com> > >> > >> Add device tree bindings for the Microchip fpga fabric based "core" PWM > >> controller. > >> > >> Reviewed-by: Rob Herring <robh@kernel.org> > >> > >> Signed-off-by: Conor Dooley <conor.dooley@microchip.com> > >> --- > >> .../bindings/pwm/microchip,corepwm.yaml | 75 +++++++++++++++++++ > >> + microchip,sync-update: > >> + description: | > >> + In synchronous mode, all channels are updated at the beginning of the PWM period. > >> + Asynchronous mode is relevant to applications such as LED control, where > >> + synchronous updates are not required. Asynchronous mode lowers the area size, > >> + reducing shadow register requirements. This can be set at run time, provided > >> + SHADOW_REG_EN is asserted. SHADOW_REG_EN is set by the FPGA bitstream programmed > >> + to the device. > >> + Each bit corresponds to a PWM channel & represents whether synchronous mode is > >> + possible for the PWM channel. > >> + > >> + $ref: /schemas/types.yaml#/definitions/uint16 > >> + default: 0 > > > >I'm not sure I understand this correctly. This is a soft-core and you > >can synthesize it either with or without the ability to do synchronous > >updates or not, right? All 16 channels share the same period length and > >in the simple implementation changing the duty cycle is done at once > >(maybe introducing a glitch) and in the more expensive implementation > >there is a register to implement both variants? > > Correct. If the IP is instantiated with SHADOW_REG_ENx=1, both > registers that control the duty cycle for channel x have a second > "shadow reg" synthesised. At runtime a bit wide register exposed to > APB can be used to toggle on/off synchronised mode for all channels > it has been synthesised for. > > I will reword this description since it is not clear. Shouldn't it use a different compatible value instead? Differentiation by properties is not recommended, as it's easy to miss a difference. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On 02/02/2022 13:28, Geert Uytterhoeven wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > Hi Conor, > > On Wed, Feb 2, 2022 at 1:33 PM <conor.dooley@microchip.com> wrote: >>> On 01/02/2022 07:58, Uwe Kleine-König wrote: >>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >>>> On Mon, Jan 31, 2022 at 11:47:21AM +0000, conor.dooley@microchip.com wrote: >>>> From: Conor Dooley <conor.dooley@microchip.com> >>>> >>>> Add device tree bindings for the Microchip fpga fabric based "core" PWM >>>> controller. >>>> >>>> Reviewed-by: Rob Herring <robh@kernel.org> >>>> >>>> Signed-off-by: Conor Dooley <conor.dooley@microchip.com> >>>> --- >>>> .../bindings/pwm/microchip,corepwm.yaml | 75 +++++++++++++++++++ > >>>> + microchip,sync-update: >>>> + description: | >>>> + In synchronous mode, all channels are updated at the beginning of the PWM period. >>>> + Asynchronous mode is relevant to applications such as LED control, where >>>> + synchronous updates are not required. Asynchronous mode lowers the area size, >>>> + reducing shadow register requirements. This can be set at run time, provided >>>> + SHADOW_REG_EN is asserted. SHADOW_REG_EN is set by the FPGA bitstream programmed >>>> + to the device. >>>> + Each bit corresponds to a PWM channel & represents whether synchronous mode is >>>> + possible for the PWM channel. >>>> + >>>> + $ref: /schemas/types.yaml#/definitions/uint16 >>>> + default: 0 >>> >>> I'm not sure I understand this correctly. This is a soft-core and you >>> can synthesize it either with or without the ability to do synchronous >>> updates or not, right? All 16 channels share the same period length and >>> in the simple implementation changing the duty cycle is done at once >>> (maybe introducing a glitch) and in the more expensive implementation >>> there is a register to implement both variants? >> >> Correct. If the IP is instantiated with SHADOW_REG_ENx=1, both >> registers that control the duty cycle for channel x have a second >> "shadow reg" synthesised. At runtime a bit wide register exposed to >> APB can be used to toggle on/off synchronised mode for all channels >> it has been synthesised for. >> >> I will reword this description since it is not clear. > > Shouldn't it use a different compatible value instead? > Differentiation by properties is not recommended, as it's easy to > miss a difference. Either you have something in mind that I've not thought of, or I've done a bad job of explaining again. The buffer/"shadow" registers are synthesised on a per channel basis, so any combination of the 16 channels may have this capability. The same applies to the DAC mode, per channel there too. Conor.
Hi Conor, On Wed, Feb 2, 2022 at 2:46 PM <Conor.Dooley@microchip.com> wrote: > On 02/02/2022 13:28, Geert Uytterhoeven wrote: > > On Wed, Feb 2, 2022 at 1:33 PM <conor.dooley@microchip.com> wrote: > >>> On 01/02/2022 07:58, Uwe Kleine-König wrote: > >>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > >>>> On Mon, Jan 31, 2022 at 11:47:21AM +0000, conor.dooley@microchip.com wrote: > >>>> From: Conor Dooley <conor.dooley@microchip.com> > >>>> > >>>> Add device tree bindings for the Microchip fpga fabric based "core" PWM > >>>> controller. > >>>> > >>>> Reviewed-by: Rob Herring <robh@kernel.org> > >>>> > >>>> Signed-off-by: Conor Dooley <conor.dooley@microchip.com> > >>>> --- > >>>> .../bindings/pwm/microchip,corepwm.yaml | 75 +++++++++++++++++++ > > > >>>> + microchip,sync-update: > >>>> + description: | > >>>> + In synchronous mode, all channels are updated at the beginning of the PWM period. > >>>> + Asynchronous mode is relevant to applications such as LED control, where > >>>> + synchronous updates are not required. Asynchronous mode lowers the area size, > >>>> + reducing shadow register requirements. This can be set at run time, provided > >>>> + SHADOW_REG_EN is asserted. SHADOW_REG_EN is set by the FPGA bitstream programmed > >>>> + to the device. > >>>> + Each bit corresponds to a PWM channel & represents whether synchronous mode is > >>>> + possible for the PWM channel. > >>>> + > >>>> + $ref: /schemas/types.yaml#/definitions/uint16 > >>>> + default: 0 > >>> > >>> I'm not sure I understand this correctly. This is a soft-core and you > >>> can synthesize it either with or without the ability to do synchronous > >>> updates or not, right? All 16 channels share the same period length and > >>> in the simple implementation changing the duty cycle is done at once > >>> (maybe introducing a glitch) and in the more expensive implementation > >>> there is a register to implement both variants? > >> > >> Correct. If the IP is instantiated with SHADOW_REG_ENx=1, both > >> registers that control the duty cycle for channel x have a second > >> "shadow reg" synthesised. At runtime a bit wide register exposed to > >> APB can be used to toggle on/off synchronised mode for all channels > >> it has been synthesised for. > >> > >> I will reword this description since it is not clear. > > > > Shouldn't it use a different compatible value instead? > > Differentiation by properties is not recommended, as it's easy to > > miss a difference. > > Either you have something in mind that I've not thought of, or I've done > a bad job of explaining again. The buffer/"shadow" registers are > synthesised on a per channel basis, so any combination of the 16 > channels may have this capability. The same applies to the DAC mode, per > channel there too. Oops, hadn't noticed this is per channel. Indeed, then a different compatible value is futile. So since "microchip,sync-update" is a bitmask, perhaps it should be called "microchip,sync-update-mask"? Same for "microchip,dac-mode" -> "microchip,dac-mode-mask"? Also, using different integer sizes than uint32 is frowned upon, unless there is a very good reason to do so. I can imagine a future version would support more channels, and then uint16 becomes a limitation. For both: Rob? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On 02/02/2022 14:02, Geert Uytterhoeven wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > On Wed, Feb 2, 2022 at 2:46 PM <Conor.Dooley@microchip.com> wrote: >> On 02/02/2022 13:28, Geert Uytterhoeven wrote: >>> On Wed, Feb 2, 2022 at 1:33 PM <conor.dooley@microchip.com> wrote: >>>>> On 01/02/2022 07:58, Uwe Kleine-König wrote: >>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >>>>>> On Mon, Jan 31, 2022 at 11:47:21AM +0000, conor.dooley@microchip.com wrote: >>>>>> From: Conor Dooley <conor.dooley@microchip.com> >>>>>> >>>>>> Add device tree bindings for the Microchip fpga fabric based "core" PWM >>>>>> controller. >>>>>> >>>>>> Reviewed-by: Rob Herring <robh@kernel.org> >>>>>> >>>>>> Signed-off-by: Conor Dooley <conor.dooley@microchip.com> >>>>>> --- >>>>>> .../bindings/pwm/microchip,corepwm.yaml | 75 +++++++++++++++++++ >>> >>>>>> + microchip,sync-update: >>>>>> + description: | >>>>>> + In synchronous mode, all channels are updated at the beginning of the PWM period. >>>>>> + Asynchronous mode is relevant to applications such as LED control, where >>>>>> + synchronous updates are not required. Asynchronous mode lowers the area size, >>>>>> + reducing shadow register requirements. This can be set at run time, provided >>>>>> + SHADOW_REG_EN is asserted. SHADOW_REG_EN is set by the FPGA bitstream programmed >>>>>> + to the device. >>>>>> + Each bit corresponds to a PWM channel & represents whether synchronous mode is >>>>>> + possible for the PWM channel. >>>>>> + >>>>>> + $ref: /schemas/types.yaml#/definitions/uint16 >>>>>> + default: 0 >>>>> >>>>> I'm not sure I understand this correctly. This is a soft-core and you >>>>> can synthesize it either with or without the ability to do synchronous >>>>> updates or not, right? All 16 channels share the same period length and >>>>> in the simple implementation changing the duty cycle is done at once >>>>> (maybe introducing a glitch) and in the more expensive implementation >>>>> there is a register to implement both variants? >>>> >>>> Correct. If the IP is instantiated with SHADOW_REG_ENx=1, both >>>> registers that control the duty cycle for channel x have a second >>>> "shadow reg" synthesised. At runtime a bit wide register exposed to >>>> APB can be used to toggle on/off synchronised mode for all channels >>>> it has been synthesised for. >>>> >>>> I will reword this description since it is not clear. >>> >>> Shouldn't it use a different compatible value instead? >>> Differentiation by properties is not recommended, as it's easy to >>> miss a difference. >> >> Either you have something in mind that I've not thought of, or I've done >> a bad job of explaining again. The buffer/"shadow" registers are >> synthesised on a per channel basis, so any combination of the 16 >> channels may have this capability. The same applies to the DAC mode, per >> channel there too. > > Oops, hadn't noticed this is per channel. Indeed, then a different > compatible value is futile. > So since "microchip,sync-update" is a bitmask, perhaps it should be > called "microchip,sync-update-mask"? > Same for "microchip,dac-mode" -> "microchip,dac-mode-mask"? Adding -mask sounds good to me. > Also, using different integer sizes than uint32 is frowned upon, unless > there is a very good reason to do so. I can imagine a future version > would support more channels, and then uint16 becomes a limitation. Sure, uint32 it is. > For both: Rob? Both of these properties fall under the "DO attempt to make bindings complete even if a driver doesn’t support some features" category, so I am perfectly happy to change these properties to whatever is convention (or ultimately drop them for the sake of the remainder of the series). Thanks, Conor.
On Mon, 31 Jan 2022 03:47:15 PST (-0800), conor.dooley@microchip.com wrote: > From: Conor Dooley <conor.dooley@microchip.com> > > This series updates the Microchip Icicle Kit device tree by adding a > host of peripherals, and some updates to the memory map. In addition, > the device tree has been split into a third part, which contains "soft" > peripherals that are in the fpga fabric. > > Several of the entries are for peripherals that have not get had their > drivers upstreamed, so in those cases the dt bindings are included where > appropriate in order to avoid the many "DT compatible string <x> appears > un-documented" errors. > > Depends on mpfs clock driver binding (on clk/next) to provide > dt-bindings/clock/microchip,mpfs-clock.h > and on the other changes to the icicle/mpfs device tree from geert > that are already in linux/riscv/for-next. > > Additionally, the interrupt-extended warnings on the plic/clint are > cleared by [1] & [2]. > > [1] https://lore.kernel.org/linux-riscv/cover.1639744468.git.geert@linux-m68k.org/ > [2] https://lore.kernel.org/linux-riscv/cover.1639744106.git.geert@linux-m68k.org/ > > Changes from v4: > - dont include icicle_kit_defconfig, accidentally added in v3 > - drop prescaler from mpfs-rtc & calculate the value instead > - use corei2c as a fallback device for mpfs-i2c > - drop spi dt-binding (on spi-next) > commit 2da187304e556ac59cf2dacb323cc78ded988169 > - drop usb dt-binding (on usb-next) > > Changes from v3: > - drop "mailbox: change mailbox-mpfs compatible string", already upstream: > commit f10b1fc0161cd99e ("mailbox: change mailbox-mpfs compatible string") > - fix copy paste error in microchip,mpfs-mailbox dt-binding > - remove whitespace in syscontroller dt entry > > Changes from v2: > - dropped plic int header & corresponding defines in dts{,i} > - use $ref to drmode in mpfs-musb binding > - split changes to dts{,i} again: functional changes to existing > elements now are in a new patch > - drop num-cs property in mpfs-spi binding > - dont make the system controller a simple-mfd > - move the separate bindings for rng/generic system services into the > system controller binding > - added an instance corei2c as i2c2 in the fabric dtsi > - add version numbering to corepwm and corei2c compat string (-rtl-vN) > > Conor Dooley (12): > dt-bindings: soc/microchip: update syscontroller compatibles > dt-bindings: soc/microchip: add services as children of sys ctrlr > dt-bindings: i2c: add bindings for microchip mpfs i2c > dt-bindings: rtc: add bindings for microchip mpfs rtc > dt-bindings: gpio: add bindings for microchip mpfs gpio > dt-bindings: pwm: add microchip corepwm binding > riscv: dts: microchip: use clk defines for icicle kit > riscv: dts: microchip: add fpga fabric section to icicle kit > riscv: dts: microchip: refactor icicle kit device tree > riscv: dts: microchip: update peripherals in icicle kit device tree > riscv: dts: microchip: add new peripherals to icicle kit device tree > MAINTAINERS: update riscv/microchip entry > > .../bindings/gpio/microchip,mpfs-gpio.yaml | 80 ++++++ > .../bindings/i2c/microchip,mpfs-i2c.yaml | 57 ++++ > ...ilbox.yaml => microchip,mpfs-mailbox.yaml} | 6 +- > .../bindings/pwm/microchip,corepwm.yaml | 75 +++++ > .../bindings/rtc/microchip,mfps-rtc.yaml | 58 ++++ > .../microchip,mpfs-sys-controller.yaml | 72 +++++ > ...icrochip,polarfire-soc-sys-controller.yaml | 35 --- > MAINTAINERS | 2 + > .../dts/microchip/microchip-mpfs-fabric.dtsi | 25 ++ > .../microchip/microchip-mpfs-icicle-kit.dts | 115 ++++++-- > .../boot/dts/microchip/microchip-mpfs.dtsi | 262 +++++++++++++++--- > 11 files changed, 683 insertions(+), 104 deletions(-) > create mode 100644 Documentation/devicetree/bindings/gpio/microchip,mpfs-gpio.yaml > create mode 100644 Documentation/devicetree/bindings/i2c/microchip,mpfs-i2c.yaml > rename Documentation/devicetree/bindings/mailbox/{microchip,polarfire-soc-mailbox.yaml => microchip,mpfs-mailbox.yaml} (82%) > create mode 100644 Documentation/devicetree/bindings/pwm/microchip,corepwm.yaml > create mode 100644 Documentation/devicetree/bindings/rtc/microchip,mfps-rtc.yaml > create mode 100644 Documentation/devicetree/bindings/soc/microchip/microchip,mpfs-sys-controller.yaml > delete mode 100644 Documentation/devicetree/bindings/soc/microchip/microchip,polarfire-soc-sys-controller.yaml > create mode 100644 arch/riscv/boot/dts/microchip/microchip-mpfs-fabric.dtsi Looks like Rob still has some feedback that still needs to be addressed. I'm happy to take these via the RISC-V tree when the bindings are set (assuming the DTs match whatever gets agreed upons), but also fine if someone else wants to take it so Acked-by: Palmer Dabbelt <palmer@rivosinc.com> Either way, I'm going to drop this (and the v4, which was at the top of my inbox) as it looks like there'll be at least a v6. Thanks!
On Mon, Jan 31, 2022 at 03:55:32PM +0000, Conor.Dooley@microchip.com wrote: > On 31/01/2022 15:39, Rob Herring wrote: > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > > > On Mon, Jan 31, 2022 at 5:45 AM <conor.dooley@microchip.com> wrote: > >> > >> From: Conor Dooley <conor.dooley@microchip.com> > >> > >> Add device tree bindings for the i2c controller on > >> the Microchip PolarFire SoC. > >> > >> Reviewed-by: Rob Herring <robh@kernel.org> > >> > > > > There should not be a blank line here. > > > > Also, tags should be in chronological order typically. If Daire sent > > this patch out with my tag, then the order is correct. If I gave it on > > a version you sent, then it goes between Daire's and your S-o-b which > > is the case here. > Oh, thanks. Probably been messing this up right/left/centre. > > On another note, I know I'm still missing a RB still on some of the > bindings, but what is the acceptance path for this series? > Any left over bindings not taken by subsystems via yourself and the dts > changes via Palmer's tree? They should go via subsystems. I can take if you want, but not with missing dependencies. I need my tree working. Rob
On 04/02/2022 22:45, Rob Herring wrote: > On Mon, Jan 31, 2022 at 03:55:32PM +0000, Conor.Dooley@microchip.com wrote: >> On 31/01/2022 15:39, Rob Herring wrote: >>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >>> >>> On Mon, Jan 31, 2022 at 5:45 AM <conor.dooley@microchip.com> wrote: >>>> >>>> From: Conor Dooley <conor.dooley@microchip.com> >>>> >>>> Add device tree bindings for the i2c controller on >>>> the Microchip PolarFire SoC. >>>> >>>> Reviewed-by: Rob Herring <robh@kernel.org> >>>> >>> >>> There should not be a blank line here. >>> >>> Also, tags should be in chronological order typically. If Daire sent >>> this patch out with my tag, then the order is correct. If I gave it on >>> a version you sent, then it goes between Daire's and your S-o-b which >>> is the case here. >> Oh, thanks. Probably been messing this up right/left/centre. >> >> On another note, I know I'm still missing a RB still on some of the >> bindings, but what is the acceptance path for this series? >> Any left over bindings not taken by subsystems via yourself and the dts >> changes via Palmer's tree? > > They should go via subsystems. I can take if you want, but not with > missing dependencies. I need my tree working. I should just drop the dependency on the clock binding from the examples, not really much reason to have it there. Still a dep. for the device tree itself but would just make it easier for the bindings themselves. Conor.
Geert, Uwe,
Hopefully the following does a better job of explaining the two parameters?
Thanks,
Conor.
microchip,sync-update-mask:
description: |
Depending on how the IP is instantiated, there are two modes of
operation. In synchronous mode, all channels are updated at the
beginning of the PWM period, and in asynchronous mode updates
happen as the control registers are written. A 16 bit wide
"SHADOW_REG_EN" parameter of the IP core controls whether
synchronous mode is possible for each channel, and is set by the
bitstream programmed to the FPGA. If the IP core is instantiated
with SHADOW_REG_ENx=1, both registers that control the duty cycle
for channel x have a second "shadow"/buffer reg synthesised.
At runtime a bit wide register exposed to APB can be used to toggle
on/off synchronised mode for all channels it has been synthesised
for.
Each bit corresponds to a PWM channel & represents whether
synchronous mode is possible for that channel.
$ref: /schemas/types.yaml#/definitions/uint32
default: 0
microchip,dac-mode-mask:
description: |
Optional, per-channel Low Ripple DAC mode is possible on this IP
core. It creates a minimum period pulse train whose High/Low
average is that of the chosen duty cycle. This "DAC" will have far
better bandwidth and ripple performance than the standard PWM
algorithm can achieve. A 16 bit DAC_MODE module parameter of the IP
core, set at instantiation and by the bitstream programmed to the
FPGA, determines whether a given channel operates in regular PWM or
DAC mode.
Each bit corresponds to a PWM channel & represents whether DAC mode
is enabled for that channel.
$ref: /schemas/types.yaml#/definitions/uint32
default: 0
On 02/02/2022 14:37, Conor.Dooley@microchip.com wrote:
> On 02/02/2022 14:02, Geert Uytterhoeven wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>> On Wed, Feb 2, 2022 at 2:46 PM <Conor.Dooley@microchip.com> wrote:
>>> On 02/02/2022 13:28, Geert Uytterhoeven wrote:
>>>> On Wed, Feb 2, 2022 at 1:33 PM <conor.dooley@microchip.com> wrote:
>>>>>> On 01/02/2022 07:58, Uwe Kleine-König wrote:
>>>>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>>>> On Mon, Jan 31, 2022 at 11:47:21AM +0000, conor.dooley@microchip.com wrote:
>>>>>>> From: Conor Dooley <conor.dooley@microchip.com>
>>>>>>>
>>>>>>> Add device tree bindings for the Microchip fpga fabric based "core" PWM
>>>>>>> controller.
>>>>>>>
>>>>>>> Reviewed-by: Rob Herring <robh@kernel.org>
>>>>>>>
>>>>>>> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
>>>>>>> ---
>>>>>>> .../bindings/pwm/microchip,corepwm.yaml | 75 +++++++++++++++++++
>>>>
>>>>>>> + microchip,sync-update:
>>>>>>> + description: |
>>>>>>> + In synchronous mode, all channels are updated at the beginning of the PWM period.
>>>>>>> + Asynchronous mode is relevant to applications such as LED control, where
>>>>>>> + synchronous updates are not required. Asynchronous mode lowers the area size,
>>>>>>> + reducing shadow register requirements. This can be set at run time, provided
>>>>>>> + SHADOW_REG_EN is asserted. SHADOW_REG_EN is set by the FPGA bitstream programmed
>>>>>>> + to the device.
>>>>>>> + Each bit corresponds to a PWM channel & represents whether synchronous mode is
>>>>>>> + possible for the PWM channel.
>>>>>>> +
>>>>>>> + $ref: /schemas/types.yaml#/definitions/uint16
>>>>>>> + default: 0
>>>>>>
>>>>>> I'm not sure I understand this correctly. This is a soft-core and you
>>>>>> can synthesize it either with or without the ability to do synchronous
>>>>>> updates or not, right? All 16 channels share the same period length and
>>>>>> in the simple implementation changing the duty cycle is done at once
>>>>>> (maybe introducing a glitch) and in the more expensive implementation
>>>>>> there is a register to implement both variants?
>>>>>
>>>>> Correct. If the IP is instantiated with SHADOW_REG_ENx=1, both
>>>>> registers that control the duty cycle for channel x have a second
>>>>> "shadow reg" synthesised. At runtime a bit wide register exposed to
>>>>> APB can be used to toggle on/off synchronised mode for all channels
>>>>> it has been synthesised for.
>>>>>
>>>>> I will reword this description since it is not clear.
>>>>
>>>> Shouldn't it use a different compatible value instead?
>>>> Differentiation by properties is not recommended, as it's easy to
>>>> miss a difference.
>>>
>>> Either you have something in mind that I've not thought of, or I've done
>>> a bad job of explaining again. The buffer/"shadow" registers are
>>> synthesised on a per channel basis, so any combination of the 16
>>> channels may have this capability. The same applies to the DAC mode, per
>>> channel there too.
>>
>> Oops, hadn't noticed this is per channel. Indeed, then a different
>> compatible value is futile.
>> So since "microchip,sync-update" is a bitmask, perhaps it should be
>> called "microchip,sync-update-mask"?
>> Same for "microchip,dac-mode" -> "microchip,dac-mode-mask"?
>
> Adding -mask sounds good to me.
>
>> Also, using different integer sizes than uint32 is frowned upon, unless
>> there is a very good reason to do so. I can imagine a future version
>> would support more channels, and then uint16 becomes a limitation.
>
> Sure, uint32 it is.
>
>> For both: Rob?
>
> Both of these properties fall under the "DO attempt to make bindings
> complete even if a driver doesn’t support some features" category, so I
> am perfectly happy to change these properties to whatever is convention
> (or ultimately drop them for the sake of the remainder of the series).
>
> Thanks,
> Conor.
>
From: Conor Dooley <conor.dooley@microchip.com> This series updates the Microchip Icicle Kit device tree by adding a host of peripherals, and some updates to the memory map. In addition, the device tree has been split into a third part, which contains "soft" peripherals that are in the fpga fabric. Several of the entries are for peripherals that have not get had their drivers upstreamed, so in those cases the dt bindings are included where appropriate in order to avoid the many "DT compatible string <x> appears un-documented" errors. Depends on mpfs clock driver binding (on clk/next) to provide dt-bindings/clock/microchip,mpfs-clock.h and on the other changes to the icicle/mpfs device tree from geert that are already in linux/riscv/for-next. Additionally, the interrupt-extended warnings on the plic/clint are cleared by [1] & [2]. [1] https://lore.kernel.org/linux-riscv/cover.1639744468.git.geert@linux-m68k.org/ [2] https://lore.kernel.org/linux-riscv/cover.1639744106.git.geert@linux-m68k.org/ Changes from v4: - dont include icicle_kit_defconfig, accidentally added in v3 - drop prescaler from mpfs-rtc & calculate the value instead - use corei2c as a fallback device for mpfs-i2c - drop spi dt-binding (on spi-next) commit 2da187304e556ac59cf2dacb323cc78ded988169 - drop usb dt-binding (on usb-next) Changes from v3: - drop "mailbox: change mailbox-mpfs compatible string", already upstream: commit f10b1fc0161cd99e ("mailbox: change mailbox-mpfs compatible string") - fix copy paste error in microchip,mpfs-mailbox dt-binding - remove whitespace in syscontroller dt entry Changes from v2: - dropped plic int header & corresponding defines in dts{,i} - use $ref to drmode in mpfs-musb binding - split changes to dts{,i} again: functional changes to existing elements now are in a new patch - drop num-cs property in mpfs-spi binding - dont make the system controller a simple-mfd - move the separate bindings for rng/generic system services into the system controller binding - added an instance corei2c as i2c2 in the fabric dtsi - add version numbering to corepwm and corei2c compat string (-rtl-vN) Conor Dooley (12): dt-bindings: soc/microchip: update syscontroller compatibles dt-bindings: soc/microchip: add services as children of sys ctrlr dt-bindings: i2c: add bindings for microchip mpfs i2c dt-bindings: rtc: add bindings for microchip mpfs rtc dt-bindings: gpio: add bindings for microchip mpfs gpio dt-bindings: pwm: add microchip corepwm binding riscv: dts: microchip: use clk defines for icicle kit riscv: dts: microchip: add fpga fabric section to icicle kit riscv: dts: microchip: refactor icicle kit device tree riscv: dts: microchip: update peripherals in icicle kit device tree riscv: dts: microchip: add new peripherals to icicle kit device tree MAINTAINERS: update riscv/microchip entry .../bindings/gpio/microchip,mpfs-gpio.yaml | 80 ++++++ .../bindings/i2c/microchip,mpfs-i2c.yaml | 57 ++++ ...ilbox.yaml => microchip,mpfs-mailbox.yaml} | 6 +- .../bindings/pwm/microchip,corepwm.yaml | 75 +++++ .../bindings/rtc/microchip,mfps-rtc.yaml | 58 ++++ .../microchip,mpfs-sys-controller.yaml | 72 +++++ ...icrochip,polarfire-soc-sys-controller.yaml | 35 --- MAINTAINERS | 2 + .../dts/microchip/microchip-mpfs-fabric.dtsi | 25 ++ .../microchip/microchip-mpfs-icicle-kit.dts | 115 ++++++-- .../boot/dts/microchip/microchip-mpfs.dtsi | 262 +++++++++++++++--- 11 files changed, 683 insertions(+), 104 deletions(-) create mode 100644 Documentation/devicetree/bindings/gpio/microchip,mpfs-gpio.yaml create mode 100644 Documentation/devicetree/bindings/i2c/microchip,mpfs-i2c.yaml rename Documentation/devicetree/bindings/mailbox/{microchip,polarfire-soc-mailbox.yaml => microchip,mpfs-mailbox.yaml} (82%) create mode 100644 Documentation/devicetree/bindings/pwm/microchip,corepwm.yaml create mode 100644 Documentation/devicetree/bindings/rtc/microchip,mfps-rtc.yaml create mode 100644 Documentation/devicetree/bindings/soc/microchip/microchip,mpfs-sys-controller.yaml delete mode 100644 Documentation/devicetree/bindings/soc/microchip/microchip,polarfire-soc-sys-controller.yaml create mode 100644 arch/riscv/boot/dts/microchip/microchip-mpfs-fabric.dtsi