Microchip Soft IP corePWM driver
Subsystems (3)
Conor Dooley
maintainerv15 cover Microchip Soft IP corePWM driver
Hey Uwe, all,
Uwe & I had a long back and forth about period calculations on v13, my ultimate conclusion being that, after some testing of the "corrected" calculation in hardware, the original calculation was correct. I think we had gotten sucked into discussion the calculation of the period itself, when we were in fact trying to calculate a bound on the period instead. That discussion is here:
https://lore.kernel.org/linux-pwm/Y+ow8tfAHo1yv1XL@wendy/
There was, of course, another chat about the niche bits of this calculation on v14 too:
https://lore.kernel.org/linux-pwm/[email protected]/
Thanks,
Conor.
Changes since v14:
- change period_steps calculation logic to correctly handle the cases where tmp % (254 + 1) == 0, by swapping implicit truncation for explicit rounding upwards and subtracting zero
- special case periods < 1/clk_rate & add a note in limitations about this, although I think this issue wasn't present prior to v15's changes
- check for smaller suitable values of prescale, which picks the "more correct" value in about half of all cases, particularly those where tmp is large.
- explain what I mean by the "optimal" values for prescale/period steps re-fix use of defines
- add a comment about how sync_upd mode works
- make the use of period_steps and prescale consistently refer to the register values rather than, in comments, using these to mean the resulting values after 1 has been added
- drop the PREG_TO_VAL() macro, as most of its users are now gone & it only added to the register value versus "real" value problem
- report pwmchip_add() failures
Changes since v13:
- couple bits of cleanup to apply_locked(), suggested by Uwe
- move the overhead waiting for a change to be applied, for channels with shadow registers, to subsequent calls to apply(). This has the benefit of only waiting when two calls to apply() are close in time rather than eating the delay in every call.
Changes since v11:
- swap a "bare" multiply & divide for the corresponding helper to prevent overflow
- factor out duplicate clk rate acquisition & period calculation
- make the period calculation return void by checking the validity of the clock rate in the caller
- drop the binding & dt patch, they're on-track for v6.2 via my tree
Changes since v10:
- reword some comments
- try to assign the period if a disable is requested
- drop a cast around a u8 -> u16 conversion
- fix a check on period_steps that should be on the hw_ variant
- split up the period calculation in get_state() to fix the result on 32 bit
- add a rate variable in get_state() to only call get_rate() once
- redo the locking as suggested to make it more straightforward.
- stop checking for enablement in get_state() that was working around intended behaviour of the sysfs interface
Changes since v9:
- fixed the missing unlock that Dan reported
Changes since v8:
- fixed a(nother) raw 64 bit division (& built it for riscv32!)
- added a check to make sure we don't try to sleep for 0 us
Changes since v7:
- rebased on 6.0-rc1
- reworded comments you highlighted in v7
- fixed the overkill sleeping
- removed the unused variables in calc_duty
- added some extra comments to explain behaviours you questioned in v7
- make the mutexes un-interruptible
- fixed added the 1s you suggested for the if(period_locked) logic
- added setup of the channel_enabled shadowing
- fixed the period reporting for the negedge == posedge case in get_state() I had to add the enabled check, as otherwise it broke setting the period for the first time out of reset.
- added a test for invalid PERIOD_STEPS values, in which case we abort if we cannot fix the period
Changes from v6:
- Dropped an unused variable that I'd missed
- Actually check the return values of the mutex lock()s
- Re-rebased on -next for the MAINTAINERS patch (again...)
Changes from v5:
- switched to a mutex b/c we must sleep with the lock taken
- simplified the locking in apply() and added locking to get_state()
- reworked apply() as requested
- removed the loop in the period calculation (thanks Uwe!)
- add a copy of the enable registers in the driver to save on reads.
- remove the second (useless) write to sync_update
- added some missing rounding in get_state()
- couple other minor cleanups as requested in:
https://lore.kernel.org/linux-riscv/[email protected]/
Changes from v4:
- dropped some accidentally added files
Changes before v4:
https://lore.kernel.org/linux-pwm/[email protected]
Conor Dooley (2):
pwm: add microchip soft ip corePWM driver
MAINTAINERS: add pwm to PolarFire SoC entry
MAINTAINERS | 1 + drivers/pwm/Kconfig | 10 + drivers/pwm/Makefile | 1 + drivers/pwm/pwm-microchip-core.c | 507 +++++++++++++++++++++++++++++++ 4 files changed, 519 insertions(+) create mode 100644 drivers/pwm/pwm-microchip-core.c
Signature · 7 lines
2.39.2 _______________________________________________ linux-riscv mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-riscv
Conor Dooley
maintainerPATCH v15 1/2 pwm: add microchip soft ip corePWM driver
Add a driver that supports the Microchip FPGA "soft" PWM IP core.
--- drivers/pwm/Kconfig | 10 + drivers/pwm/Makefile | 1 + drivers/pwm/pwm-microchip-core.c | 507 +++++++++++++++++++++++++++++++ 3 files changed, 518 insertions(+) create mode 100644 drivers/pwm/pwm-microchip-core.c
drivers/pwm/Kconfig +10 -0
@@ -393,6 +393,16 @@ config PWM_MEDIATEK To compile this driver as a module, choose M here: the module will be called pwm-mediatek. +config PWM_MICROCHIP_CORE + tristate "Microchip corePWM PWM support" + depends on SOC_MICROCHIP_POLARFIRE || COMPILE_TEST + depends on HAS_IOMEM && OF + help + PWM driver for Microchip FPGA soft IP core. + + To compile this driver as a module, choose M here: the module + will be called pwm-microchip-core. + config PWM_MXS tristate "Freescale MXS PWM support" depends on ARCH_MXS || COMPILE_TEST
drivers/pwm/Makefile +1 -0
@@ -34,6 +34,7 @@ obj-$(CONFIG_PWM_LPSS_PCI) += pwm-lpss-pci.o obj-$(CONFIG_PWM_LPSS_PLATFORM) += pwm-lpss-platform.o obj-$(CONFIG_PWM_MESON) += pwm-meson.o obj-$(CONFIG_PWM_MEDIATEK) += pwm-mediatek.o +obj-$(CONFIG_PWM_MICROCHIP_CORE) += pwm-microchip-core.o obj-$(CONFIG_PWM_MTK_DISP) += pwm-mtk-disp.o obj-$(CONFIG_PWM_MXS) += pwm-mxs.o obj-$(CONFIG_PWM_NTXEC) += pwm-ntxec.o
drivers/pwm/pwm-microchip-core.c new file +507 -0
507 lines not on this page · raw message
Signature · 7 lines
2.39.2 _______________________________________________ linux-riscv mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-riscv
kernel test robot
build botRe: [PATCH v15 1/2] pwm: add microchip soft ip corePWM driver
I love your patch! Yet something to improve:
Hi Conor,
I love your patch! Yet something to improve:
[auto build test ERROR on thierry-reding-pwm/for-next]
[also build test ERROR on linus/master v6.3-rc4 next-20230331] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Conor-Dooley/pwm-add-microchip-soft-ip-corePWM-driver/20230330-151415 base: https://git.kernel.org/pub/scm/linux/kernel/git/thierry.reding/linux-pwm.git for-next patch link: https://lore.kernel.org/r/20230330071203.286972-2-conor.dooley%40microchip.com patch subject: [PATCH v15 1/2] pwm: add microchip soft ip corePWM driver config: arm-randconfig-c041-20230402 (https://download.01.org/0day-ci/archive/20230402/[email protected]/config) compiler: arm-linux-gnueabi-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/703dc5c3f6c78464be383c1fa31ecfb3ad683d84
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Conor-Dooley/pwm-add-microchip-soft-ip-corePWM-driver/20230330-151415 git checkout 703dc5c3f6c78464be383c1fa31ecfb3ad683d84
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arm olddefconfig COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All errors (new ones prefixed by >>):
arm-linux-gnueabi-ld: arm-linux-gnueabi-ld: DWARF error: could not find abbrev number 57 drivers/pwm/pwm-microchip-core.o: in function `mchp_core_pwm_apply_locked':
>> pwm-microchip-core.c:(.text+0x20a): undefined reference to `__aeabi_uldivmod'
Signature · 2 lines
0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests
Re: [PATCH v15 1/2] pwm: add microchip soft ip corePWM driver
In reply to Conor DooleyQuoted 11 lines · Conor Dooley
drivers/pwm/pwm-microchip-core.c new file +11
-0+/* +* Because 0xff is not a permitted value some error will seep into the +* calculation of prescale as prescale grows. Specifically, this error +* occurs where the remainder of the prescale calculation is less than +* prescale. +* For small values of prescale, only a handful of values will need +* correction, but overall this applies to almost half of the valid +* values for tmp. +* +* To keep the algorithm's decision making consistent, this case is +* checked for and the simple solution is to, in these cases,
drivers/pwm/pwm-microchip-core.c line 258
Quoted 10 lines
drivers/pwm/pwm-microchip-core.c new file +10
-0+* decrement prescale and check that the resulting value of period_steps +* is valid. +* +* period_steps can be computed from prescale: +* period * clk_rate +* period_steps = ----------------------------- - 1 +* NSEC_PER_SEC * (prescale + 1) +* +*/ +if (tmp % (MCHPCOREPWM_PERIOD_STEPS_MAX +1 ) < *prescale) {
Hmm, looks like 32-bit doesn't like this modulus.
I pushed things out for LKP to test before sending as I felt I'd not be allowed to do that operation, but got a build success email from it. I'm not sure why the mail wasn't sent as a reply to this, but <[email protected]> complains:
pwm-microchip-core.c:(.text+0x20a): undefined reference to `__aeabi_uldivmod'
I know that tmp < 65536 at this point, so if the general approach is fine, I can always cast it to a non 64-bit type without losing any information.
Quoted 9 lines
drivers/pwm/pwm-microchip-core.c new file +9
-0+u16 smaller_prescale = *prescale -1 ; + + *period_steps =div_u64 (tmp, smaller_prescale +1 ) -1 ; +if (*period_steps <255 ) { + *prescale = smaller_prescale; + +return 0 ; + } + }
Re: [PATCH v15 1/2] pwm: add microchip soft ip corePWM driver
Quoted 31 lines · Conor Dooley
Quoted 11 lines · Conor Dooley
drivers/pwm/pwm-microchip-core.c new file +11
-0+/* +* Because 0xff is not a permitted value some error will seep into the +* calculation of prescale as prescale grows. Specifically, this error +* occurs where the remainder of the prescale calculation is less than +* prescale. +* For small values of prescale, only a handful of values will need +* correction, but overall this applies to almost half of the valid +* values for tmp. +* +* To keep the algorithm's decision making consistent, this case is +* checked for and the simple solution is to, in these cases, drivers/pwm/pwm-microchip-core.c line 258
Quoted 10 lines
drivers/pwm/pwm-microchip-core.c new file +10
-0+* decrement prescale and check that the resulting value of period_steps +* is valid. +* +* period_steps can be computed from prescale: +* period * clk_rate +* period_steps = ----------------------------- - 1 +* NSEC_PER_SEC * (prescale + 1) +* +*/ +if (tmp % (MCHPCOREPWM_PERIOD_STEPS_MAX +1 ) < *prescale) {Hmm, looks like 32-bit doesn't like this modulus.
I pushed things out for LKP to test before sending as I felt I'd not be allowed to do that operation, but got a build success email from it. I'm not sure why the mail wasn't sent as a reply to this, but <[email protected]> complains:
pwm-microchip-core.c:(.text+0x20a): undefined reference to `__aeabi_uldivmod'
Quoted 3 lines
I know that tmp < 65536 at this point, so if the general approach is fine, I can always cast it to a non 64-bit type without losing any information.
Since you already use some of the helpers from linux/math64.h, perhaps you can use something like div_u64_rem() here?
Thierry
Quoted 10 lines
Quoted 10 lines
drivers/pwm/pwm-microchip-core.c new file +9
-0+u16 smaller_prescale = *prescale -1 ; + + *period_steps =div_u64 (tmp, smaller_prescale +1 ) -1 ; +if (*period_steps <255 ) { + *prescale = smaller_prescale; + +return 0 ; + } + }
Conor Dooley
maintainerRe: [PATCH v15 1/2] pwm: add microchip soft ip corePWM driver
Quoted 32 lines · Thierry Reding
Quoted 31 lines · Conor Dooley
Quoted 11 lines · Conor Dooley
drivers/pwm/pwm-microchip-core.c new file +11
-0+/* +* Because 0xff is not a permitted value some error will seep into the +* calculation of prescale as prescale grows. Specifically, this error +* occurs where the remainder of the prescale calculation is less than +* prescale. +* For small values of prescale, only a handful of values will need +* correction, but overall this applies to almost half of the valid +* values for tmp. +* +* To keep the algorithm's decision making consistent, this case is +* checked for and the simple solution is to, in these cases, drivers/pwm/pwm-microchip-core.c line 258
Quoted 10 lines
drivers/pwm/pwm-microchip-core.c new file +10
-0+* decrement prescale and check that the resulting value of period_steps +* is valid. +* +* period_steps can be computed from prescale: +* period * clk_rate +* period_steps = ----------------------------- - 1 +* NSEC_PER_SEC * (prescale + 1) +* +*/ +if (tmp % (MCHPCOREPWM_PERIOD_STEPS_MAX +1 ) < *prescale) {Hmm, looks like 32-bit doesn't like this modulus.
I pushed things out for LKP to test before sending as I felt I'd not be allowed to do that operation, but got a build success email from it. I'm not sure why the mail wasn't sent as a reply to this, but <[email protected]> complains:
pwm-microchip-core.c:(.text+0x20a): undefined reference to `__aeabi_uldivmod'
Quoted 6 lines
Quoted 3 lines
I know that tmp < 65536 at this point, so if the general approach is fine, I can always cast it to a non 64-bit type without losing any information.
Since you already use some of the helpers from
linux/math64.h, perhaps you can use something likediv_u64_rem()here?
I had actually initially tried to do that, before I added changed the calculation of prescale to use DIV64_U64_ROUND_UP():
| *prescale = DIV64_U64_ROUND_UP(tmp, MCHPCOREPWM_PERIOD_STEPS_MAX + 1) - 1;
I could I suppose add an additional:
| div_u64_rem(tmp, MCHPCOREPWM_PERIOD_STEPS_MAX + 1, &remainder);
and then just use the value of remainder in the if statement. Not the prettiest thing in the world I suppose, but should be 32-bit safe...
Thanks for the suggestion, I'm just surprised that LKP didn't complain when it built my tree...
_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv
Conor Dooley
maintainerPATCH v15 2/2 MAINTAINERS: add pwm to PolarFire SoC entry
In reply to Conor DooleyAdd the newly introduced pwm driver to the existing PolarFire SoC entry.
--- MAINTAINERS | 1 + 1 file changed, 1 insertion(+)
MAINTAINERS +1 -0
@@ -17993,6 +17993,7 @@ F: drivers/clk/microchip/clk-mpfs.c F: drivers/i2c/busses/i2c-microchip-corei2c.c F: drivers/mailbox/mailbox-mpfs.c F: drivers/pci/controller/pcie-microchip-host.c +F: drivers/pwm/pwm-microchip-core.c F: drivers/reset/reset-mpfs.c F: drivers/rtc/rtc-mpfs.c F: drivers/soc/microchip/mpfs-sys-controller.c
Signature · 7 lines
2.39.2 _______________________________________________ linux-riscv mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-riscv