This series is tidying up and adding device-tree support for the
DesignWare DW-APB-timers block.
Changes:
v3:
- change the compatible name
- squash down pwm count patch
- fixup patch naming
v2:
- fix #pwm-cells count to be 3
- fix indetation
- merge the two clock patches
- add HAS_IOMEM as a config dependency
In preparation for adding other bus support, move the allocation
of the pwm struct out of the main driver code.
Signed-off-by: Ben Dooks <[email protected]>
---
drivers/pwm/pwm-dwc.c | 24 +++++++++++++++++-------
1 file changed, 17 insertions(+), 7 deletions(-)
diff --git a/drivers/pwm/pwm-dwc.c b/drivers/pwm/pwm-dwc.c
index c706ef9a7ba1..61f11e0a9319 100644
--- a/drivers/pwm/pwm-dwc.c
+++ b/drivers/pwm/pwm-dwc.c
@@ -196,13 +196,29 @@ static const struct pwm_ops dwc_pwm_ops = {
.owner = THIS_MODULE,
};
+static struct dwc_pwm *dwc_pwm_alloc(struct device *dev)
+{
+ struct dwc_pwm *dwc;
+
+ dwc = devm_kzalloc(dev, sizeof(*dwc), GFP_KERNEL);
+ if (!dwc)
+ return NULL;
+
+ dwc->chip.dev = dev;
+ dwc->chip.ops = &dwc_pwm_ops;
+ dwc->chip.npwm = DWC_TIMERS_TOTAL;
+
+ dev_set_drvdata(dev, dwc);
+ return dwc;
+}
+
static int dwc_pwm_probe(struct pci_dev *pci, const struct pci_device_id *id)
{
struct device *dev = &pci->dev;
struct dwc_pwm *dwc;
int ret;
- dwc = devm_kzalloc(dev, sizeof(*dwc), GFP_KERNEL);
+ dwc = dwc_pwm_alloc(dev);
if (!dwc)
return -ENOMEM;
@@ -226,12 +242,6 @@ static int dwc_pwm_probe(struct pci_dev *pci, const struct pci_device_id *id)
return -ENOMEM;
}
- pci_set_drvdata(pci, dwc);
-
- dwc->chip.dev = dev;
- dwc->chip.ops = &dwc_pwm_ops;
- dwc->chip.npwm = DWC_TIMERS_TOTAL;
-
ret = pwmchip_add(&dwc->chip);
if (ret)
return ret;
--
2.35.1
Add a configurable clock base rate for the pwm as when being built
for non-PCI the block may be sourced from an internal clock.
Signed-off-by: Ben Dooks <[email protected]>
---
v2:
- removed the ifdef and merged the other clock patch in here
---
drivers/pwm/pwm-dwc.c | 22 +++++++++++++++++-----
1 file changed, 17 insertions(+), 5 deletions(-)
diff --git a/drivers/pwm/pwm-dwc.c b/drivers/pwm/pwm-dwc.c
index d5f2df6fee62..5c319d0e3d52 100644
--- a/drivers/pwm/pwm-dwc.c
+++ b/drivers/pwm/pwm-dwc.c
@@ -18,6 +18,7 @@
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/pci.h>
+#include <linux/clk.h>
#include <linux/platform_device.h>
#include <linux/pm_runtime.h>
#include <linux/pwm.h>
@@ -35,7 +36,6 @@
#define DWC_TIMERS_COMP_VERSION 0xac
#define DWC_TIMERS_TOTAL 8
-#define DWC_CLK_PERIOD_NS 10
/* Timer Control Register */
#define DWC_TIM_CTRL_EN BIT(0)
@@ -54,6 +54,8 @@ struct dwc_pwm_ctx {
struct dwc_pwm {
struct pwm_chip chip;
void __iomem *base;
+ struct clk *clk;
+ unsigned int clk_ns;
struct dwc_pwm_ctx ctx[DWC_TIMERS_TOTAL];
};
#define to_dwc_pwm(p) (container_of((p), struct dwc_pwm, chip))
@@ -96,13 +98,13 @@ static int __dwc_pwm_configure_timer(struct dwc_pwm *dwc,
* periods and check are the result within HW limits between 1 and
* 2^32 periods.
*/
- tmp = DIV_ROUND_CLOSEST_ULL(state->duty_cycle, DWC_CLK_PERIOD_NS);
+ tmp = DIV_ROUND_CLOSEST_ULL(state->duty_cycle, dwc->clk_ns);
if (tmp < 1 || tmp > (1ULL << 32))
return -ERANGE;
low = tmp - 1;
tmp = DIV_ROUND_CLOSEST_ULL(state->period - state->duty_cycle,
- DWC_CLK_PERIOD_NS);
+ dwc->clk_ns);
if (tmp < 1 || tmp > (1ULL << 32))
return -ERANGE;
high = tmp - 1;
@@ -177,12 +179,12 @@ static void dwc_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
duty = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT(pwm->hwpwm));
duty += 1;
- duty *= DWC_CLK_PERIOD_NS;
+ duty *= dwc->clk_ns;
state->duty_cycle = duty;
period = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT2(pwm->hwpwm));
period += 1;
- period *= DWC_CLK_PERIOD_NS;
+ period *= dwc->clk_ns;
period += duty;
state->period = period;
@@ -205,6 +207,7 @@ static struct dwc_pwm *dwc_pwm_alloc(struct device *dev)
if (!dwc)
return NULL;
+ dwc->clk_ns = 10;
dwc->chip.dev = dev;
dwc->chip.ops = &dwc_pwm_ops;
dwc->chip.npwm = DWC_TIMERS_TOTAL;
@@ -336,6 +339,14 @@ static int dwc_pwm_plat_probe(struct platform_device *pdev)
return dev_err_probe(dev, PTR_ERR(dwc->base),
"failed to map IO\n");
+ dwc->clk = devm_clk_get(dev, "timer");
+ if (IS_ERR(dwc->clk))
+ return dev_err_probe(dev, PTR_ERR(dwc->clk),
+ "failed to get timer clock\n");
+
+ clk_prepare_enable(dwc->clk);
+ dwc->clk_ns = 1000000000 / clk_get_rate(dwc->clk);
+
ret = pwmchip_add(&dwc->chip);
if (ret)
return ret;
@@ -347,6 +358,7 @@ static int dwc_pwm_plat_remove(struct platform_device *pdev)
{
struct dwc_pwm *dwc = platform_get_drvdata(pdev);
+ clk_disable_unprepare(dwc->clk);
pwmchip_remove(&dwc->chip);
return 0;
}
--
2.35.1
The dwc pwm controller can be used in non-PCI systems, so allow
either platform or OF based probing.
Signed-off-by: Ben Dooks <[email protected]>
---
v3:
- changed compatible name
---
drivers/pwm/Kconfig | 5 ++--
drivers/pwm/pwm-dwc.c | 53 +++++++++++++++++++++++++++++++++++++++++++
2 files changed, 56 insertions(+), 2 deletions(-)
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 60d13a949bc5..b8717877a524 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -176,9 +176,10 @@ config PWM_CROS_EC
config PWM_DWC
tristate "DesignWare PWM Controller"
- depends on PCI
+ depends on PCI || OF
help
- PWM driver for Synopsys DWC PWM Controller attached to a PCI bus.
+ PWM driver for Synopsys DWC PWM Controller attached to either a
+ PCI or platform bus.
To compile this driver as a module, choose M here: the module
will be called pwm-dwc.
diff --git a/drivers/pwm/pwm-dwc.c b/drivers/pwm/pwm-dwc.c
index 61f11e0a9319..d5f2df6fee62 100644
--- a/drivers/pwm/pwm-dwc.c
+++ b/drivers/pwm/pwm-dwc.c
@@ -18,6 +18,7 @@
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/pci.h>
+#include <linux/platform_device.h>
#include <linux/pm_runtime.h>
#include <linux/pwm.h>
@@ -319,6 +320,58 @@ static struct pci_driver dwc_pwm_driver = {
module_pci_driver(dwc_pwm_driver);
+#ifdef CONFIG_OF
+static int dwc_pwm_plat_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct dwc_pwm *dwc;
+ int ret;
+
+ dwc = dwc_pwm_alloc(dev);
+ if (!dwc)
+ return -ENOMEM;
+
+ dwc->base = devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(dwc->base))
+ return dev_err_probe(dev, PTR_ERR(dwc->base),
+ "failed to map IO\n");
+
+ ret = pwmchip_add(&dwc->chip);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
+static int dwc_pwm_plat_remove(struct platform_device *pdev)
+{
+ struct dwc_pwm *dwc = platform_get_drvdata(pdev);
+
+ pwmchip_remove(&dwc->chip);
+ return 0;
+}
+
+static const struct of_device_id dwc_pwm_dt_ids[] = {
+ { .compatible = "snps,dw-apb-timers-pwm2" },
+ { },
+};
+MODULE_DEVICE_TABLE(of, dwc_pwm_dt_ids);
+
+static struct platform_driver dwc_pwm_plat_driver = {
+ .driver = {
+ .name = "dwc-pwm",
+ .of_match_table = dwc_pwm_dt_ids,
+ },
+ .probe = dwc_pwm_plat_probe,
+ .remove = dwc_pwm_plat_remove,
+};
+
+module_platform_driver(dwc_pwm_plat_driver);
+
+MODULE_ALIAS("platform:dwc-pwm");
+#endif /* CONFIG_OF */
+
+
MODULE_AUTHOR("Felipe Balbi (Intel)");
MODULE_AUTHOR("Jarkko Nikula <[email protected]>");
MODULE_AUTHOR("Raymond Tan <[email protected]>");
--
2.35.1
Add documentation for the bindings for Synopsys' DesignWare PWM block
as we will be adding DT/platform support to the Linux driver soon.
Signed-off-by: Ben Dooks <[email protected]>
---
v3:
- add description and example
- merge the snps,pwm-number into this patch
- rename snps,pwm to snps,dw-apb-timers-pwm2
v2:
- fix #pwm-cells to be 3
- fix indentation and ordering issues
---
.../bindings/pwm/snps,dw-apb-timers-pwm2.yaml | 66 +++++++++++++++++++
1 file changed, 66 insertions(+)
create mode 100644 Documentation/devicetree/bindings/pwm/snps,dw-apb-timers-pwm2.yaml
diff --git a/Documentation/devicetree/bindings/pwm/snps,dw-apb-timers-pwm2.yaml b/Documentation/devicetree/bindings/pwm/snps,dw-apb-timers-pwm2.yaml
new file mode 100644
index 000000000000..a81f2c2e485c
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/snps,dw-apb-timers-pwm2.yaml
@@ -0,0 +1,66 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright (C) 2022 SiFive, Inc.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pwm/snps,dw-apb-tiners-pwm2.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Synopsys DW-APB timers PWM controller
+
+maintainers:
+ - Ben Dooks <[email protected]>
+
+description:
+ This describes the DesignWare APB timers module when used in the PWM
+ mode. The IP core can be generated with various options which can
+ control the functionality, the number of PWMs available and other
+ internal controls the designer requires.
+
+ The IP block has a version register so this can be used for detection
+ instead of having to encode the IP version number in the device tree
+ comaptible.
+
+allOf:
+ - $ref: pwm.yaml#
+
+properties:
+ compatible:
+ const: snps,dw-apb-timers-pwm2
+
+ "#pwm-cells":
+ const: 3
+
+ clocks:
+ items:
+ - description: Interface bus clock
+ - description: PWM reference clock
+
+ clock-names:
+ items:
+ - const: bus
+ - const: timer
+
+ snps,pwm-number:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ description: The number of PWM channels configured for this instance
+ enum: [1, 2, 3, 4, 5, 6, 7, 8]
+
+required:
+ - "#pwm-cells"
+ - compatible
+ - reg
+ - clocks
+ - clock-names
+
+additionalProperties: false
+
+
+examples:
+ - |
+ pwm: pwm@180000 {
+ #pwm-cells = <3>;
+ compatible = "snps,dw-apb-timers-pwm2";
+ reg = <0x180000 0x200>;
+ clocks = <&bus &timer>;
+ clock-names = "bus", "timer";
+ };
--
2.35.1
Add snps,pwm-number property to indicate if the block does not have
all 8 of the PWM blocks.
Not sure if this should be a general PWM property consider optional
for all PWM types, so have added a specific one here (there is only
one other controller with a property for PWM count at the moment)
Signed-off-by: Ben Dooks <[email protected]>
---
drivers/pwm/pwm-dwc.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/pwm/pwm-dwc.c b/drivers/pwm/pwm-dwc.c
index 5c319d0e3d52..5edfb8f8acbf 100644
--- a/drivers/pwm/pwm-dwc.c
+++ b/drivers/pwm/pwm-dwc.c
@@ -328,12 +328,20 @@ static int dwc_pwm_plat_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
struct dwc_pwm *dwc;
+ u32 nr_pwm;
int ret;
dwc = dwc_pwm_alloc(dev);
if (!dwc)
return -ENOMEM;
+ if (!device_property_read_u32(dev, "snps,pwm-number", &nr_pwm)) {
+ if (nr_pwm > DWC_TIMERS_TOTAL)
+ dev_err(dev, "too many PWMs specified (%d)\n", nr_pwm);
+ else
+ dwc->chip.npwm = nr_pwm;
+ }
+
dwc->base = devm_platform_ioremap_resource(pdev, 0);
if (IS_ERR(dwc->base))
return dev_err_probe(dev, PTR_ERR(dwc->base),
--
2.35.1
If we are not in PWM mode, then the output is technically a 50%
output based on a single timer instead of the high-low based on
the two counters. Add a check for the PWM mode in dwc_pwm_get_state()
and if DWC_TIM_CTRL_PWM is not set, then return a 50% cycle.
This may only be an issue on initialisation, as the rest of the
code currently assumes we're always going to have the extended
PWM mode using two counters.
Signed-off-by: Ben Dooks <[email protected]>
---
drivers/pwm/pwm-dwc.c | 34 +++++++++++++++++++++++-----------
1 file changed, 23 insertions(+), 11 deletions(-)
diff --git a/drivers/pwm/pwm-dwc.c b/drivers/pwm/pwm-dwc.c
index 5edfb8f8acbf..49e666be7afd 100644
--- a/drivers/pwm/pwm-dwc.c
+++ b/drivers/pwm/pwm-dwc.c
@@ -171,23 +171,35 @@ static void dwc_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
{
struct dwc_pwm *dwc = to_dwc_pwm(chip);
u64 duty, period;
+ u32 ctrl, ld, ld2;
pm_runtime_get_sync(chip->dev);
- state->enabled = !!(dwc_pwm_readl(dwc,
- DWC_TIM_CTRL(pwm->hwpwm)) & DWC_TIM_CTRL_EN);
+ ctrl = dwc_pwm_readl(dwc, DWC_TIM_CTRL(pwm->hwpwm));
+ ld = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT(pwm->hwpwm));
+ ld2 = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT2(pwm->hwpwm));
- duty = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT(pwm->hwpwm));
- duty += 1;
- duty *= dwc->clk_ns;
- state->duty_cycle = duty;
+ state->enabled = !!(ctrl & DWC_TIM_CTRL_EN);
- period = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT2(pwm->hwpwm));
- period += 1;
- period *= dwc->clk_ns;
- period += duty;
- state->period = period;
+ /* If we're not in PWM, technically the output is a 50-50
+ * based on the timer load-count only.
+ */
+ if (ctrl & DWC_TIM_CTRL_PWM) {
+ duty = ld;
+ duty += 1;
+ duty *= dwc->clk_ns;
+
+ period = ld2;
+ period += 1;
+ period *= dwc->clk_ns;
+ period += duty;
+ } else {
+ duty = (ld + 1) * dwc->clk_ns;
+ period = duty * 2;
+ }
+ state->period = period;
+ state->duty_cycle = duty;
state->polarity = PWM_POLARITY_INVERSED;
pm_runtime_put_sync(chip->dev);
--
2.35.1
Allow dwc driver to be built with COMPILE_TEST should allow
better coverage when build testing.
Signed-off-by: Ben Dooks <[email protected]>
---
v3:
- add HAS_IOMEM depdency for compile testing
---
drivers/pwm/Kconfig | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index b8717877a524..05718e0faac9 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -176,7 +176,8 @@ config PWM_CROS_EC
config PWM_DWC
tristate "DesignWare PWM Controller"
- depends on PCI || OF
+ depends on PCI || OF || COMPILE_TEST
+ depends on HAS_IOMEM
help
PWM driver for Synopsys DWC PWM Controller attached to either a
PCI or platform bus.
--
2.35.1
The dwc_pwm_probe() assignes dev to be &pci->dev but then uses
&pci->dev throughout the function. Change these all to the be
'dev' variable to make lines shorter.
Signed-off-by: Ben Dooks <[email protected]>
Acked-by: Uwe Kleine-König <[email protected]>
---
drivers/pwm/pwm-dwc.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/pwm/pwm-dwc.c b/drivers/pwm/pwm-dwc.c
index 7568300bb11e..c706ef9a7ba1 100644
--- a/drivers/pwm/pwm-dwc.c
+++ b/drivers/pwm/pwm-dwc.c
@@ -202,14 +202,13 @@ static int dwc_pwm_probe(struct pci_dev *pci, const struct pci_device_id *id)
struct dwc_pwm *dwc;
int ret;
- dwc = devm_kzalloc(&pci->dev, sizeof(*dwc), GFP_KERNEL);
+ dwc = devm_kzalloc(dev, sizeof(*dwc), GFP_KERNEL);
if (!dwc)
return -ENOMEM;
ret = pcim_enable_device(pci);
if (ret) {
- dev_err(&pci->dev,
- "Failed to enable device (%pe)\n", ERR_PTR(ret));
+ dev_err(dev, "Failed to enable device (%pe)\n", ERR_PTR(ret));
return ret;
}
@@ -217,14 +216,13 @@ static int dwc_pwm_probe(struct pci_dev *pci, const struct pci_device_id *id)
ret = pcim_iomap_regions(pci, BIT(0), pci_name(pci));
if (ret) {
- dev_err(&pci->dev,
- "Failed to iomap PCI BAR (%pe)\n", ERR_PTR(ret));
+ dev_err(dev, "Failed to iomap PCI BAR (%pe)\n", ERR_PTR(ret));
return ret;
}
dwc->base = pcim_iomap_table(pci)[0];
if (!dwc->base) {
- dev_err(&pci->dev, "Base address missing\n");
+ dev_err(dev, "Base address missing\n");
return -ENOMEM;
}
--
2.35.1
Hi Ben,
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 v5.19 next-20220805]
[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/Ben-Dooks/dt-bindings-pwm-Document-Synopsys-DesignWare-snps-pwm-dw-apb-timers-pwm2/20220806-015142
base: https://git.kernel.org/pub/scm/linux/kernel/git/thierry.reding/linux-pwm.git for-next
config: x86_64-randconfig-a002 (https://download.01.org/0day-ci/archive/20220806/[email protected]/config)
compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/3bd100d711908b7d16a2c4793b4f5b597acb8d7f
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Ben-Dooks/dt-bindings-pwm-Document-Synopsys-DesignWare-snps-pwm-dw-apb-timers-pwm2/20220806-015142
git checkout 3bd100d711908b7d16a2c4793b4f5b597acb8d7f
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/pwm/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
In file included from drivers/pwm/pwm-dwc.c:19:
include/linux/module.h:131:49: error: redefinition of '__inittest'
131 | static inline initcall_t __maybe_unused __inittest(void) \
| ^~~~~~~~~~
include/linux/device/driver.h:266:1: note: in expansion of macro 'module_init'
266 | module_init(__driver##_init); \
| ^~~~~~~~~~~
include/linux/platform_device.h:264:9: note: in expansion of macro 'module_driver'
264 | module_driver(__platform_driver, platform_driver_register, \
| ^~~~~~~~~~~~~
drivers/pwm/pwm-dwc.c:369:1: note: in expansion of macro 'module_platform_driver'
369 | module_platform_driver(dwc_pwm_plat_driver);
| ^~~~~~~~~~~~~~~~~~~~~~
include/linux/module.h:131:49: note: previous definition of '__inittest' with type 'int (*(void))(void)'
131 | static inline initcall_t __maybe_unused __inittest(void) \
| ^~~~~~~~~~
include/linux/device/driver.h:266:1: note: in expansion of macro 'module_init'
266 | module_init(__driver##_init); \
| ^~~~~~~~~~~
include/linux/pci.h:1481:9: note: in expansion of macro 'module_driver'
1481 | module_driver(__pci_driver, pci_register_driver, pci_unregister_driver)
| ^~~~~~~~~~~~~
drivers/pwm/pwm-dwc.c:321:1: note: in expansion of macro 'module_pci_driver'
321 | module_pci_driver(dwc_pwm_driver);
| ^~~~~~~~~~~~~~~~~
include/linux/module.h:133:13: error: redefinition of 'init_module'
133 | int init_module(void) __copy(initfn) \
| ^~~~~~~~~~~
include/linux/device/driver.h:266:1: note: in expansion of macro 'module_init'
266 | module_init(__driver##_init); \
| ^~~~~~~~~~~
include/linux/platform_device.h:264:9: note: in expansion of macro 'module_driver'
264 | module_driver(__platform_driver, platform_driver_register, \
| ^~~~~~~~~~~~~
drivers/pwm/pwm-dwc.c:369:1: note: in expansion of macro 'module_platform_driver'
369 | module_platform_driver(dwc_pwm_plat_driver);
| ^~~~~~~~~~~~~~~~~~~~~~
include/linux/module.h:133:13: note: previous definition of 'init_module' with type 'int(void)'
133 | int init_module(void) __copy(initfn) \
| ^~~~~~~~~~~
include/linux/device/driver.h:266:1: note: in expansion of macro 'module_init'
266 | module_init(__driver##_init); \
| ^~~~~~~~~~~
include/linux/pci.h:1481:9: note: in expansion of macro 'module_driver'
1481 | module_driver(__pci_driver, pci_register_driver, pci_unregister_driver)
| ^~~~~~~~~~~~~
drivers/pwm/pwm-dwc.c:321:1: note: in expansion of macro 'module_pci_driver'
321 | module_pci_driver(dwc_pwm_driver);
| ^~~~~~~~~~~~~~~~~
>> include/linux/module.h:139:49: error: redefinition of '__exittest'
139 | static inline exitcall_t __maybe_unused __exittest(void) \
| ^~~~~~~~~~
include/linux/device/driver.h:271:1: note: in expansion of macro 'module_exit'
271 | module_exit(__driver##_exit);
| ^~~~~~~~~~~
include/linux/platform_device.h:264:9: note: in expansion of macro 'module_driver'
264 | module_driver(__platform_driver, platform_driver_register, \
| ^~~~~~~~~~~~~
drivers/pwm/pwm-dwc.c:369:1: note: in expansion of macro 'module_platform_driver'
369 | module_platform_driver(dwc_pwm_plat_driver);
| ^~~~~~~~~~~~~~~~~~~~~~
include/linux/module.h:139:49: note: previous definition of '__exittest' with type 'void (*(void))(void)'
139 | static inline exitcall_t __maybe_unused __exittest(void) \
| ^~~~~~~~~~
include/linux/device/driver.h:271:1: note: in expansion of macro 'module_exit'
271 | module_exit(__driver##_exit);
| ^~~~~~~~~~~
include/linux/pci.h:1481:9: note: in expansion of macro 'module_driver'
1481 | module_driver(__pci_driver, pci_register_driver, pci_unregister_driver)
| ^~~~~~~~~~~~~
drivers/pwm/pwm-dwc.c:321:1: note: in expansion of macro 'module_pci_driver'
321 | module_pci_driver(dwc_pwm_driver);
| ^~~~~~~~~~~~~~~~~
>> include/linux/module.h:141:14: error: redefinition of 'cleanup_module'
141 | void cleanup_module(void) __copy(exitfn) \
| ^~~~~~~~~~~~~~
include/linux/device/driver.h:271:1: note: in expansion of macro 'module_exit'
271 | module_exit(__driver##_exit);
| ^~~~~~~~~~~
include/linux/platform_device.h:264:9: note: in expansion of macro 'module_driver'
264 | module_driver(__platform_driver, platform_driver_register, \
| ^~~~~~~~~~~~~
drivers/pwm/pwm-dwc.c:369:1: note: in expansion of macro 'module_platform_driver'
369 | module_platform_driver(dwc_pwm_plat_driver);
| ^~~~~~~~~~~~~~~~~~~~~~
include/linux/module.h:141:14: note: previous definition of 'cleanup_module' with type 'void(void)'
141 | void cleanup_module(void) __copy(exitfn) \
| ^~~~~~~~~~~~~~
include/linux/device/driver.h:271:1: note: in expansion of macro 'module_exit'
271 | module_exit(__driver##_exit);
| ^~~~~~~~~~~
include/linux/pci.h:1481:9: note: in expansion of macro 'module_driver'
1481 | module_driver(__pci_driver, pci_register_driver, pci_unregister_driver)
| ^~~~~~~~~~~~~
drivers/pwm/pwm-dwc.c:321:1: note: in expansion of macro 'module_pci_driver'
321 | module_pci_driver(dwc_pwm_driver);
| ^~~~~~~~~~~~~~~~~
vim +/__exittest +139 include/linux/module.h
0fd972a7d91d6e Paul Gortmaker 2015-05-01 128
0fd972a7d91d6e Paul Gortmaker 2015-05-01 129 /* Each module must use one module_init(). */
0fd972a7d91d6e Paul Gortmaker 2015-05-01 130 #define module_init(initfn) \
1f318a8bafcfba Arnd Bergmann 2017-02-01 131 static inline initcall_t __maybe_unused __inittest(void) \
0fd972a7d91d6e Paul Gortmaker 2015-05-01 132 { return initfn; } \
cf68fffb66d60d Sami Tolvanen 2021-04-08 133 int init_module(void) __copy(initfn) \
cf68fffb66d60d Sami Tolvanen 2021-04-08 134 __attribute__((alias(#initfn))); \
cf68fffb66d60d Sami Tolvanen 2021-04-08 135 __CFI_ADDRESSABLE(init_module, __initdata);
0fd972a7d91d6e Paul Gortmaker 2015-05-01 136
0fd972a7d91d6e Paul Gortmaker 2015-05-01 137 /* This is only required if you want to be unloadable. */
0fd972a7d91d6e Paul Gortmaker 2015-05-01 138 #define module_exit(exitfn) \
1f318a8bafcfba Arnd Bergmann 2017-02-01 @139 static inline exitcall_t __maybe_unused __exittest(void) \
0fd972a7d91d6e Paul Gortmaker 2015-05-01 140 { return exitfn; } \
cf68fffb66d60d Sami Tolvanen 2021-04-08 @141 void cleanup_module(void) __copy(exitfn) \
cf68fffb66d60d Sami Tolvanen 2021-04-08 142 __attribute__((alias(#exitfn))); \
cf68fffb66d60d Sami Tolvanen 2021-04-08 143 __CFI_ADDRESSABLE(cleanup_module, __exitdata);
0fd972a7d91d6e Paul Gortmaker 2015-05-01 144
--
0-DAY CI Kernel Test Service
https://01.org/lkp
Hi Ben,
I love your patch! Perhaps something to improve:
[auto build test WARNING on thierry-reding-pwm/for-next]
[also build test WARNING on linus/master v5.19 next-20220805]
[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/Ben-Dooks/dt-bindings-pwm-Document-Synopsys-DesignWare-snps-pwm-dw-apb-timers-pwm2/20220806-015142
base: https://git.kernel.org/pub/scm/linux/kernel/git/thierry.reding/linux-pwm.git for-next
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20220806/[email protected]/config)
compiler: m68k-linux-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/3bd100d711908b7d16a2c4793b4f5b597acb8d7f
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Ben-Dooks/dt-bindings-pwm-Document-Synopsys-DesignWare-snps-pwm-dw-apb-timers-pwm2/20220806-015142
git checkout 3bd100d711908b7d16a2c4793b4f5b597acb8d7f
# 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=m68k SHELL=/bin/bash drivers/pwm/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
>> drivers/pwm/pwm-dwc.c:321:1: warning: data definition has no type or storage class
321 | module_pci_driver(dwc_pwm_driver);
| ^~~~~~~~~~~~~~~~~
drivers/pwm/pwm-dwc.c:321:1: error: type defaults to 'int' in declaration of 'module_pci_driver' [-Werror=implicit-int]
>> drivers/pwm/pwm-dwc.c:321:1: warning: parameter names (without types) in function declaration
drivers/pwm/pwm-dwc.c:311:26: warning: 'dwc_pwm_driver' defined but not used [-Wunused-variable]
311 | static struct pci_driver dwc_pwm_driver = {
| ^~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +321 drivers/pwm/pwm-dwc.c
1ed2b3fca64516 Jarkko Nikula 2020-10-02 320
1ed2b3fca64516 Jarkko Nikula 2020-10-02 @321 module_pci_driver(dwc_pwm_driver);
1ed2b3fca64516 Jarkko Nikula 2020-10-02 322
--
0-DAY CI Kernel Test Service
https://01.org/lkp
On Fri, Aug 05, 2022 at 05:50:29PM +0100, Ben Dooks wrote:
> The dwc pwm controller can be used in non-PCI systems, so allow
> either platform or OF based probing.
>
> Signed-off-by: Ben Dooks <[email protected]>
> ---
> v3:
> - changed compatible name
> ---
> drivers/pwm/Kconfig | 5 ++--
> drivers/pwm/pwm-dwc.c | 53 +++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 56 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 60d13a949bc5..b8717877a524 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -176,9 +176,10 @@ config PWM_CROS_EC
>
> config PWM_DWC
> tristate "DesignWare PWM Controller"
> - depends on PCI
> + depends on PCI || OF
> help
> - PWM driver for Synopsys DWC PWM Controller attached to a PCI bus.
> + PWM driver for Synopsys DWC PWM Controller attached to either a
> + PCI or platform bus.
>
> To compile this driver as a module, choose M here: the module
> will be called pwm-dwc.
> diff --git a/drivers/pwm/pwm-dwc.c b/drivers/pwm/pwm-dwc.c
> index 61f11e0a9319..d5f2df6fee62 100644
> --- a/drivers/pwm/pwm-dwc.c
> +++ b/drivers/pwm/pwm-dwc.c
> @@ -18,6 +18,7 @@
> #include <linux/kernel.h>
> #include <linux/module.h>
> #include <linux/pci.h>
> +#include <linux/platform_device.h>
> #include <linux/pm_runtime.h>
> #include <linux/pwm.h>
>
> @@ -319,6 +320,58 @@ static struct pci_driver dwc_pwm_driver = {
>
> module_pci_driver(dwc_pwm_driver);
>
> +#ifdef CONFIG_OF
> +static int dwc_pwm_plat_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct dwc_pwm *dwc;
> + int ret;
> +
> + dwc = dwc_pwm_alloc(dev);
> + if (!dwc)
> + return -ENOMEM;
> +
> + dwc->base = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(dwc->base))
> + return dev_err_probe(dev, PTR_ERR(dwc->base),
> + "failed to map IO\n");
devm_platform_ioremap_resource already provides an error message, so
drop the message here.
> + ret = pwmchip_add(&dwc->chip);
> + if (ret)
> + return ret;
> +
> + return 0;
the last three code lines are equivalent to just
return ret;
An error message would be nice after pwmchip_add failed.
> +}
> +
> +static int dwc_pwm_plat_remove(struct platform_device *pdev)
> +{
> + struct dwc_pwm *dwc = platform_get_drvdata(pdev);
> +
> + pwmchip_remove(&dwc->chip);
> + return 0;
> +}
Please consider using devm_pwmchip_add() which makes it unnecessary to
provide a remove callback.
> +static const struct of_device_id dwc_pwm_dt_ids[] = {
> + { .compatible = "snps,dw-apb-timers-pwm2" },
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, dwc_pwm_dt_ids);
> +
> +static struct platform_driver dwc_pwm_plat_driver = {
> + .driver = {
> + .name = "dwc-pwm",
> + .of_match_table = dwc_pwm_dt_ids,
I'm not a fan of aligning the = signs, for sure, don't use tabs for one
and spaces for the other. If you ask me, use a single space.
> + },
> + .probe = dwc_pwm_plat_probe,
> + .remove = dwc_pwm_plat_remove,
> +};
> +
> +module_platform_driver(dwc_pwm_plat_driver);
> +
> +MODULE_ALIAS("platform:dwc-pwm");
> +#endif /* CONFIG_OF */
> +
> +
> MODULE_AUTHOR("Felipe Balbi (Intel)");
> MODULE_AUTHOR("Jarkko Nikula <[email protected]>");
> MODULE_AUTHOR("Raymond Tan <[email protected]>");
>
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Hello Ben,
On Fri, Aug 05, 2022 at 05:50:31PM +0100, Ben Dooks wrote:
> Add a configurable clock base rate for the pwm as when being built
> for non-PCI the block may be sourced from an internal clock.
>
> Signed-off-by: Ben Dooks <[email protected]>
> ---
> v2:
> - removed the ifdef and merged the other clock patch in here
> ---
> drivers/pwm/pwm-dwc.c | 22 +++++++++++++++++-----
> 1 file changed, 17 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/pwm/pwm-dwc.c b/drivers/pwm/pwm-dwc.c
> index d5f2df6fee62..5c319d0e3d52 100644
> --- a/drivers/pwm/pwm-dwc.c
> +++ b/drivers/pwm/pwm-dwc.c
> @@ -18,6 +18,7 @@
> #include <linux/kernel.h>
> #include <linux/module.h>
> #include <linux/pci.h>
> +#include <linux/clk.h>
> #include <linux/platform_device.h>
> #include <linux/pm_runtime.h>
> #include <linux/pwm.h>
> @@ -35,7 +36,6 @@
> #define DWC_TIMERS_COMP_VERSION 0xac
>
> #define DWC_TIMERS_TOTAL 8
> -#define DWC_CLK_PERIOD_NS 10
>
> /* Timer Control Register */
> #define DWC_TIM_CTRL_EN BIT(0)
> @@ -54,6 +54,8 @@ struct dwc_pwm_ctx {
> struct dwc_pwm {
> struct pwm_chip chip;
> void __iomem *base;
> + struct clk *clk;
> + unsigned int clk_ns;
> struct dwc_pwm_ctx ctx[DWC_TIMERS_TOTAL];
> };
> #define to_dwc_pwm(p) (container_of((p), struct dwc_pwm, chip))
> @@ -96,13 +98,13 @@ static int __dwc_pwm_configure_timer(struct dwc_pwm *dwc,
> * periods and check are the result within HW limits between 1 and
> * 2^32 periods.
> */
> - tmp = DIV_ROUND_CLOSEST_ULL(state->duty_cycle, DWC_CLK_PERIOD_NS);
> + tmp = DIV_ROUND_CLOSEST_ULL(state->duty_cycle, dwc->clk_ns);
> if (tmp < 1 || tmp > (1ULL << 32))
> return -ERANGE;
> low = tmp - 1;
>
> tmp = DIV_ROUND_CLOSEST_ULL(state->period - state->duty_cycle,
> - DWC_CLK_PERIOD_NS);
> + dwc->clk_ns);
You're loosing precision here as clk_ns is already the result of a
division. We're having
dwc->clk_ns = 1000000000 / clk_get_rate(dwc->clk);
from dwc_pwm_plat_probe() (in the platform case).
Consider clk_rate = 285714285 and state->period - state->duty_cycle =
300000. Then you get tmp = 100000 while the exact result would be:
300000 * 285714285 / 1000000000 = 85714.2855
Note that even doing
dwc->clk_ns = DIV_ROUND_CLOSEST(1000000000, clk_get_rate(dwc->clk))
only somewhat weakens the problem, with the above numbers you then get
75000.
Also note that rounding closest is also wrong in the calculation of tmp
because the driver is supposed to implement the biggest period not
bigger than the requested period and for that period implement the
biggest duty cycle not bigger than the requested duty cycle.
Can the hardware emit 0% relative duty cycle (e.g. by disabling)?
> if (tmp < 1 || tmp > (1ULL << 32))
> return -ERANGE;
> high = tmp - 1;
> @@ -177,12 +179,12 @@ static void dwc_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
>
> duty = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT(pwm->hwpwm));
> duty += 1;
> - duty *= DWC_CLK_PERIOD_NS;
> + duty *= dwc->clk_ns;
> state->duty_cycle = duty;
>
> period = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT2(pwm->hwpwm));
> period += 1;
> - period *= DWC_CLK_PERIOD_NS;
> + period *= dwc->clk_ns;
> period += duty;
> state->period = period;
>
> @@ -205,6 +207,7 @@ static struct dwc_pwm *dwc_pwm_alloc(struct device *dev)
> if (!dwc)
> return NULL;
>
> + dwc->clk_ns = 10;
> dwc->chip.dev = dev;
> dwc->chip.ops = &dwc_pwm_ops;
> dwc->chip.npwm = DWC_TIMERS_TOTAL;
> @@ -336,6 +339,14 @@ static int dwc_pwm_plat_probe(struct platform_device *pdev)
> return dev_err_probe(dev, PTR_ERR(dwc->base),
> "failed to map IO\n");
>
> + dwc->clk = devm_clk_get(dev, "timer");
> + if (IS_ERR(dwc->clk))
> + return dev_err_probe(dev, PTR_ERR(dwc->clk),
> + "failed to get timer clock\n");
> +
> + clk_prepare_enable(dwc->clk);
If you used devm_clk_get_enabled() you wouldn't need to care separately
for enabling. (If you stick to separate calls, please add error checking
for clk_prepare_enable().)
> + dwc->clk_ns = 1000000000 / clk_get_rate(dwc->clk);
s/1000000000/NSEC_PER_SEC/
> +
> ret = pwmchip_add(&dwc->chip);
> if (ret)
> return ret;
> @@ -347,6 +358,7 @@ static int dwc_pwm_plat_remove(struct platform_device *pdev)
> {
> struct dwc_pwm *dwc = platform_get_drvdata(pdev);
>
> + clk_disable_unprepare(dwc->clk);
> pwmchip_remove(&dwc->chip);
This is wrong, you must not disable the clock before calling
pwmchip_remove() as the PWM is supposed to stay functional until
pwmchip_remove() returns.
> return 0;
> }
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Hi Ben,
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 v5.19 next-20220805]
[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/Ben-Dooks/dt-bindings-pwm-Document-Synopsys-DesignWare-snps-pwm-dw-apb-timers-pwm2/20220806-015142
base: https://git.kernel.org/pub/scm/linux/kernel/git/thierry.reding/linux-pwm.git for-next
config: hexagon-allyesconfig (https://download.01.org/0day-ci/archive/20220806/[email protected]/config)
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 26dd42705c2af0b8f6e5d6cdb32c9bd5ed9524eb)
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/3bd100d711908b7d16a2c4793b4f5b597acb8d7f
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Ben-Dooks/dt-bindings-pwm-Document-Synopsys-DesignWare-snps-pwm-dw-apb-timers-pwm2/20220806-015142
git checkout 3bd100d711908b7d16a2c4793b4f5b597acb8d7f
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
>> drivers/pwm/pwm-dwc.c:321:1: error: type specifier missing, defaults to 'int'; ISO C99 and later do not support implicit int [-Wimplicit-int]
module_pci_driver(dwc_pwm_driver);
^
int
>> drivers/pwm/pwm-dwc.c:321:19: error: a parameter list without types is only allowed in a function definition
module_pci_driver(dwc_pwm_driver);
^
2 errors generated.
vim +/int +321 drivers/pwm/pwm-dwc.c
1ed2b3fca64516 Jarkko Nikula 2020-10-02 320
1ed2b3fca64516 Jarkko Nikula 2020-10-02 @321 module_pci_driver(dwc_pwm_driver);
1ed2b3fca64516 Jarkko Nikula 2020-10-02 322
--
0-DAY CI Kernel Test Service
https://01.org/lkp
Hello,
On Fri, Aug 05, 2022 at 05:50:33PM +0100, Ben Dooks wrote:
> If we are not in PWM mode, then the output is technically a 50%
> output based on a single timer instead of the high-low based on
> the two counters. Add a check for the PWM mode in dwc_pwm_get_state()
> and if DWC_TIM_CTRL_PWM is not set, then return a 50% cycle.
>
> This may only be an issue on initialisation, as the rest of the
> code currently assumes we're always going to have the extended
> PWM mode using two counters.
>
> Signed-off-by: Ben Dooks <[email protected]>
> ---
> drivers/pwm/pwm-dwc.c | 34 +++++++++++++++++++++++-----------
> 1 file changed, 23 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/pwm/pwm-dwc.c b/drivers/pwm/pwm-dwc.c
> index 5edfb8f8acbf..49e666be7afd 100644
> --- a/drivers/pwm/pwm-dwc.c
> +++ b/drivers/pwm/pwm-dwc.c
> @@ -171,23 +171,35 @@ static void dwc_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> {
> struct dwc_pwm *dwc = to_dwc_pwm(chip);
> u64 duty, period;
> + u32 ctrl, ld, ld2;
>
> pm_runtime_get_sync(chip->dev);
>
> - state->enabled = !!(dwc_pwm_readl(dwc,
> - DWC_TIM_CTRL(pwm->hwpwm)) & DWC_TIM_CTRL_EN);
> + ctrl = dwc_pwm_readl(dwc, DWC_TIM_CTRL(pwm->hwpwm));
> + ld = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT(pwm->hwpwm));
> + ld2 = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT2(pwm->hwpwm));
>
> - duty = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT(pwm->hwpwm));
> - duty += 1;
> - duty *= dwc->clk_ns;
> - state->duty_cycle = duty;
> + state->enabled = !!(ctrl & DWC_TIM_CTRL_EN);
>
> - period = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT2(pwm->hwpwm));
> - period += 1;
> - period *= dwc->clk_ns;
> - period += duty;
> - state->period = period;
> + /* If we're not in PWM, technically the output is a 50-50
Huh, I expected checkpatch to warn about that. AFAIK the /* is supposed
to be on a line for itself?!
> + * based on the timer load-count only.
> + */
> + if (ctrl & DWC_TIM_CTRL_PWM) {
> + duty = ld;
> + duty += 1;
> + duty *= dwc->clk_ns;
I would prefer to write that as:
duty = (ld + 1) * dwc->clk_ns;
given that todays compilers are clever enough to optimize that just fine
and this version is better readable for humans.
> +
> + period = ld2;
> + period += 1;
> + period *= dwc->clk_ns;
> + period += duty;
> + } else {
> + duty = (ld + 1) * dwc->clk_ns;
> + period = duty * 2;
> + }
>
> + state->period = period;
> + state->duty_cycle = duty;
> state->polarity = PWM_POLARITY_INVERSED;
>
> pm_runtime_put_sync(chip->dev);
> --
> 2.35.1
I'm marking all patches in this series as "changes requested" even
though not all patches were commented. I assume that you continue to
care for all of them for the next revision. Please make sure to pass -v4
to git format-patch (or git send-email) then.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Hi Ben,
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 v5.19 next-20220805]
[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/Ben-Dooks/dt-bindings-pwm-Document-Synopsys-DesignWare-snps-pwm-dw-apb-timers-pwm2/20220806-015142
base: https://git.kernel.org/pub/scm/linux/kernel/git/thierry.reding/linux-pwm.git for-next
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20220807/[email protected]/config)
compiler: m68k-linux-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/3bd100d711908b7d16a2c4793b4f5b597acb8d7f
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Ben-Dooks/dt-bindings-pwm-Document-Synopsys-DesignWare-snps-pwm-dw-apb-timers-pwm2/20220806-015142
git checkout 3bd100d711908b7d16a2c4793b4f5b597acb8d7f
# 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=m68k SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
drivers/pwm/pwm-dwc.c:321:1: warning: data definition has no type or storage class
321 | module_pci_driver(dwc_pwm_driver);
| ^~~~~~~~~~~~~~~~~
>> drivers/pwm/pwm-dwc.c:321:1: error: type defaults to 'int' in declaration of 'module_pci_driver' [-Werror=implicit-int]
drivers/pwm/pwm-dwc.c:321:1: warning: parameter names (without types) in function declaration
drivers/pwm/pwm-dwc.c:311:26: warning: 'dwc_pwm_driver' defined but not used [-Wunused-variable]
311 | static struct pci_driver dwc_pwm_driver = {
| ^~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +321 drivers/pwm/pwm-dwc.c
1ed2b3fca64516 Jarkko Nikula 2020-10-02 320
1ed2b3fca64516 Jarkko Nikula 2020-10-02 @321 module_pci_driver(dwc_pwm_driver);
1ed2b3fca64516 Jarkko Nikula 2020-10-02 322
--
0-DAY CI Kernel Test Service
https://01.org/lkp
On Fri, 05 Aug 2022 17:50:26 +0100, Ben Dooks wrote:
> Add documentation for the bindings for Synopsys' DesignWare PWM block
> as we will be adding DT/platform support to the Linux driver soon.
>
> Signed-off-by: Ben Dooks <[email protected]>
> ---
> v3:
> - add description and example
> - merge the snps,pwm-number into this patch
> - rename snps,pwm to snps,dw-apb-timers-pwm2
> v2:
> - fix #pwm-cells to be 3
> - fix indentation and ordering issues
> ---
> .../bindings/pwm/snps,dw-apb-timers-pwm2.yaml | 66 +++++++++++++++++++
> 1 file changed, 66 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/pwm/snps,dw-apb-timers-pwm2.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/pwm/snps,dw-apb-timers-pwm2.yaml: $id: relative path/filename doesn't match actual path or filename
expected: http://devicetree.org/schemas/pwm/snps,dw-apb-timers-pwm2.yaml#
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pwm/snps,dw-apb-timers-pwm2.example.dtb: pwm@180000: 'reg' does not match any of the regexes: 'pinctrl-[0-9]+'
From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pwm/snps,dw-apb-timers-pwm2.yaml
doc reference errors (make refcheckdocs):
See https://patchwork.ozlabs.org/patch/
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, 08 Aug 2022, Lee Jones wrote:
> On Fri, 05 Aug 2022, Ben Dooks wrote:
>
> > This series is tidying up and adding device-tree support for the
> > DesignWare DW-APB-timers block.
> >
> > Changes:
> >
> > v3:
> > - change the compatible name
> > - squash down pwm count patch
> > - fixup patch naming
> >
> > v2:
> > - fix #pwm-cells count to be 3
> > - fix indetation
> > - merge the two clock patches
> > - add HAS_IOMEM as a config dependency
>
> Can you use the front-cover option provided by Git please Ben?
git format-patch --cover-letter ...
> It gives you proper formatting and a diff-stat that is very useful.
--
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
On 08/08/2022 09:02, Lee Jones wrote:
> On Mon, 08 Aug 2022, Lee Jones wrote:
>
>> On Fri, 05 Aug 2022, Ben Dooks wrote:
>>
>>> This series is tidying up and adding device-tree support for the
>>> DesignWare DW-APB-timers block.
>>>
>>> Changes:
>>>
>>> v3:
>>> - change the compatible name
>>> - squash down pwm count patch
>>> - fixup patch naming
>>>
>>> v2:
>>> - fix #pwm-cells count to be 3
>>> - fix indetation
>>> - merge the two clock patches
>>> - add HAS_IOMEM as a config dependency
>>
>> Can you use the front-cover option provided by Git please Ben?
>
> git format-patch --cover-letter ...
I thought git-send-email --compose did that.
On Fri, 05 Aug 2022, Ben Dooks wrote:
> This series is tidying up and adding device-tree support for the
> DesignWare DW-APB-timers block.
>
> Changes:
>
> v3:
> - change the compatible name
> - squash down pwm count patch
> - fixup patch naming
>
> v2:
> - fix #pwm-cells count to be 3
> - fix indetation
> - merge the two clock patches
> - add HAS_IOMEM as a config dependency
Can you use the front-cover option provided by Git please Ben?
It gives you proper formatting and a diff-stat that is very useful.
--
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
On Mon, 08 Aug 2022, Ben Dooks wrote:
> On 08/08/2022 09:02, Lee Jones wrote:
> > On Mon, 08 Aug 2022, Lee Jones wrote:
> >
> > > On Fri, 05 Aug 2022, Ben Dooks wrote:
> > >
> > > > This series is tidying up and adding device-tree support for the
> > > > DesignWare DW-APB-timers block.
> > > >
> > > > Changes:
> > > >
> > > > v3:
> > > > - change the compatible name
> > > > - squash down pwm count patch
> > > > - fixup patch naming
> > > >
> > > > v2:
> > > > - fix #pwm-cells count to be 3
> > > > - fix indetation
> > > > - merge the two clock patches
> > > > - add HAS_IOMEM as a config dependency
> > >
> > > Can you use the front-cover option provided by Git please Ben?
> >
> > git format-patch --cover-letter ...
>
> I thought git-send-email --compose did that.
gse's --compose will open up your editor on the series before sending.
gfp's --cover-letter creates the 0th patch with a nice format.
--
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog
On 8/5/22 19:50, Ben Dooks wrote:
> The dwc_pwm_probe() assignes dev to be &pci->dev but then uses
> &pci->dev throughout the function. Change these all to the be
> 'dev' variable to make lines shorter.
>
> Signed-off-by: Ben Dooks <[email protected]>
> Acked-by: Uwe Kleine-König <[email protected]>
> ---
> drivers/pwm/pwm-dwc.c | 10 ++++------
> 1 file changed, 4 insertions(+), 6 deletions(-)
>
Acked-by: Jarkko Nikula <[email protected]>
Hi
On 8/5/22 19:50, Ben Dooks wrote:
> The dwc pwm controller can be used in non-PCI systems, so allow
> either platform or OF based probing.
>
> Signed-off-by: Ben Dooks <[email protected]>
> ---
> v3:
> - changed compatible name
> ---
> drivers/pwm/Kconfig | 5 ++--
> drivers/pwm/pwm-dwc.c | 53 +++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 56 insertions(+), 2 deletions(-)
>
...
> @@ -319,6 +320,58 @@ static struct pci_driver dwc_pwm_driver = {
>
> module_pci_driver(dwc_pwm_driver);
...
> +module_platform_driver(dwc_pwm_plat_driver);
> +
These module_X_driver() macros cannot coexist in the same module and
ideally the same kernel built should support probing both PCI and OF
based systems in runtime, i.e. putting those macros under #ifdef is not
ideal.
Usually this is solved either by common code has the platform device
probing (with or without the OF support) and the PCI part is in another
module which adds the platform device(s) or both platform and PCI device
code are in own modules and call common code.
On 08/08/2022 15:36, Jarkko Nikula wrote:
> Hi
>
> On 8/5/22 19:50, Ben Dooks wrote:
>> The dwc pwm controller can be used in non-PCI systems, so allow
>> either platform or OF based probing.
>>
>> Signed-off-by: Ben Dooks <[email protected]>
>> ---
>> v3:
>> - changed compatible name
>> ---
>> drivers/pwm/Kconfig | 5 ++--
>> drivers/pwm/pwm-dwc.c | 53 +++++++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 56 insertions(+), 2 deletions(-)
>>
> ...
>
>> @@ -319,6 +320,58 @@ static struct pci_driver dwc_pwm_driver = {
>> module_pci_driver(dwc_pwm_driver);
> ...
>> +module_platform_driver(dwc_pwm_plat_driver);
>> +
>
> These module_X_driver() macros cannot coexist in the same module and
> ideally the same kernel built should support probing both PCI and OF
> based systems in runtime, i.e. putting those macros under #ifdef is not
> ideal.
>
> Usually this is solved either by common code has the platform device
> probing (with or without the OF support) and the PCI part is in another
> module which adds the platform device(s) or both platform and PCI device
> code are in own modules and call common code.
Yeah, just realised that.
I'm not sure it is worth splitting the driver as most of the probe code
is pretty small so it might be worth just having one init function that
adds both the pcie and the of drivers.
--
Ben
On 06/08/2022 11:07, Uwe Kleine-König wrote:
> Hello Ben,
>
> On Fri, Aug 05, 2022 at 05:50:31PM +0100, Ben Dooks wrote:
>> Add a configurable clock base rate for the pwm as when being built
>> for non-PCI the block may be sourced from an internal clock.
>>
>> Signed-off-by: Ben Dooks <[email protected]>
>> ---
>> v2:
>> - removed the ifdef and merged the other clock patch in here
>> ---
>> drivers/pwm/pwm-dwc.c | 22 +++++++++++++++++-----
>> 1 file changed, 17 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/pwm/pwm-dwc.c b/drivers/pwm/pwm-dwc.c
>> index d5f2df6fee62..5c319d0e3d52 100644
>> --- a/drivers/pwm/pwm-dwc.c
>> +++ b/drivers/pwm/pwm-dwc.c
>> @@ -18,6 +18,7 @@
>> #include <linux/kernel.h>
>> #include <linux/module.h>
>> #include <linux/pci.h>
>> +#include <linux/clk.h>
>> #include <linux/platform_device.h>
>> #include <linux/pm_runtime.h>
>> #include <linux/pwm.h>
>> @@ -35,7 +36,6 @@
>> #define DWC_TIMERS_COMP_VERSION 0xac
>>
>> #define DWC_TIMERS_TOTAL 8
>> -#define DWC_CLK_PERIOD_NS 10
>>
>> /* Timer Control Register */
>> #define DWC_TIM_CTRL_EN BIT(0)
>> @@ -54,6 +54,8 @@ struct dwc_pwm_ctx {
>> struct dwc_pwm {
>> struct pwm_chip chip;
>> void __iomem *base;
>> + struct clk *clk;
>> + unsigned int clk_ns;
>> struct dwc_pwm_ctx ctx[DWC_TIMERS_TOTAL];
>> };
>> #define to_dwc_pwm(p) (container_of((p), struct dwc_pwm, chip))
>> @@ -96,13 +98,13 @@ static int __dwc_pwm_configure_timer(struct dwc_pwm *dwc,
>> * periods and check are the result within HW limits between 1 and
>> * 2^32 periods.
>> */
>> - tmp = DIV_ROUND_CLOSEST_ULL(state->duty_cycle, DWC_CLK_PERIOD_NS);
>> + tmp = DIV_ROUND_CLOSEST_ULL(state->duty_cycle, dwc->clk_ns);
>> if (tmp < 1 || tmp > (1ULL << 32))
>> return -ERANGE;
>> low = tmp - 1;
>>
>> tmp = DIV_ROUND_CLOSEST_ULL(state->period - state->duty_cycle,
>> - DWC_CLK_PERIOD_NS);
>> + dwc->clk_ns);
>
> You're loosing precision here as clk_ns is already the result of a
> division. We're having
>
> dwc->clk_ns = 1000000000 / clk_get_rate(dwc->clk);
>
> from dwc_pwm_plat_probe() (in the platform case).
>
> Consider clk_rate = 285714285 and state->period - state->duty_cycle =
> 300000. Then you get tmp = 100000 while the exact result would be:
>
> 300000 * 285714285 / 1000000000 = 85714.2855
>
> Note that even doing
>
> dwc->clk_ns = DIV_ROUND_CLOSEST(1000000000, clk_get_rate(dwc->clk))
>
> only somewhat weakens the problem, with the above numbers you then get
> 75000.
>
> Also note that rounding closest is also wrong in the calculation of tmp
> because the driver is supposed to implement the biggest period not
> bigger than the requested period and for that period implement the
> biggest duty cycle not bigger than the requested duty cycle.
>
> Can the hardware emit 0% relative duty cycle (e.g. by disabling)?
Not sure, we do have an IP build option to look at for 0/100% but
this is not enabled for the PCI case.
Given everything else, I would rather fix the division and accuracy
issues once we've got the changes under review sorted.
>
>> if (tmp < 1 || tmp > (1ULL << 32))
>> return -ERANGE;
>> high = tmp - 1;
>> @@ -177,12 +179,12 @@ static void dwc_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
>>
>> duty = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT(pwm->hwpwm));
>> duty += 1;
>> - duty *= DWC_CLK_PERIOD_NS;
>> + duty *= dwc->clk_ns;
>> state->duty_cycle = duty;
>>
>> period = dwc_pwm_readl(dwc, DWC_TIM_LD_CNT2(pwm->hwpwm));
>> period += 1;
>> - period *= DWC_CLK_PERIOD_NS;
>> + period *= dwc->clk_ns;
>> period += duty;
>> state->period = period;
>>
>> @@ -205,6 +207,7 @@ static struct dwc_pwm *dwc_pwm_alloc(struct device *dev)
>> if (!dwc)
>> return NULL;
>>
>> + dwc->clk_ns = 10;
>> dwc->chip.dev = dev;
>> dwc->chip.ops = &dwc_pwm_ops;
>> dwc->chip.npwm = DWC_TIMERS_TOTAL;
>> @@ -336,6 +339,14 @@ static int dwc_pwm_plat_probe(struct platform_device *pdev)
>> return dev_err_probe(dev, PTR_ERR(dwc->base),
>> "failed to map IO\n");
>>
>> + dwc->clk = devm_clk_get(dev, "timer");
>> + if (IS_ERR(dwc->clk))
>> + return dev_err_probe(dev, PTR_ERR(dwc->clk),
>> + "failed to get timer clock\n");
>> +
>> + clk_prepare_enable(dwc->clk);
>
> If you used devm_clk_get_enabled() you wouldn't need to care separately
> for enabling. (If you stick to separate calls, please add error checking
> for clk_prepare_enable().)
ok, will use.
>> + dwc->clk_ns = 1000000000 / clk_get_rate(dwc->clk);
>
> s/1000000000/NSEC_PER_SEC/
ok, fixed.
>> +
>> ret = pwmchip_add(&dwc->chip);
>> if (ret)
>> return ret;
>> @@ -347,6 +358,7 @@ static int dwc_pwm_plat_remove(struct platform_device *pdev)
>> {
>> struct dwc_pwm *dwc = platform_get_drvdata(pdev);
>>
>> + clk_disable_unprepare(dwc->clk);
>> pwmchip_remove(&dwc->chip);
>
> This is wrong, you must not disable the clock before calling
> pwmchip_remove() as the PWM is supposed to stay functional until
> pwmchip_remove() returns.
I've moved to devm_clk_get_enabled and devm_pwmchip_add()
>
>> return 0;
>> }
>