* [PATCH v2 0/7] Add support for H6 PWM
@ 2019-11-03 20:33 Clément Péron
2019-11-03 20:33 ` [PATCH v2 1/7] dt-bindings: pwm: allwinner: Add H6 PWM description Clément Péron
` (6 more replies)
0 siblings, 7 replies; 33+ messages in thread
From: Clément Péron @ 2019-11-03 20:33 UTC (permalink / raw)
To: Thierry Reding, 'Uwe Kleine-König',
Rob Herring, Mark Rutland, Maxime Ripard, Chen-Yu Tsai
Cc: linux-pwm, Clément Péron, linux-kernel,
linux-arm-kernel, devicetree
Hi,
This is a rework of Jernej's previous work[1] taking account all the
previous remarks.
Bindings is still strict but probe in the driver are now optionnals.
If someone could confirm that the PWM is not broken, as my board
doesn't output it.
I didn't add the acked-tags as there are big changes.
Thanks,
Clément
Jernej's cover:
Allwinner H6 SoC has PWM core which is basically the same as that found
in A20, it's just depends on additional bus clock and reset line.
This series adds support for it and extends PWM driver functionality in
a way that it's now possible to bypass whole core and output PWM source
clock directly as a PWM signal. This functionality is needed by AC200
chip, which is bundled in same physical package as H6 SoC, to serve as a
clock source of 24 MHz. AC200 clock input pin is bonded internally to
the second PWM channel.
I would be grateful if anyone can test this patch series for any kind of
regression on other SoCs.
[1]: https://patchwork.kernel.org/cover/11061737/
Changes in v2:
- Remove allOf in Documentation
- Add H6 example in Documentation
- Change clock name from "pwm" to "mod"
- Change reset quirk to optional probe
- Change bus_clock quirk to optional probe
- Add limitation comment about mod_clk_output
- Add quirk for mod_clk_output
- Change bypass formula
Clément Péron (1):
[DO NOT MERGE] arm64: allwinner: h6: enable Beelink GS1 PWM
Jernej Skrabec (6):
dt-bindings: pwm: allwinner: Add H6 PWM description
pwm: sun4i: Add an optional probe for reset line
pwm: sun4i: Add an optional probe for bus clock
pwm: sun4i: Add support to output source clock directly
pwm: sun4i: Add support for H6 PWM
arm64: dts: allwinner: h6: Add PWM node
.../bindings/pwm/allwinner,sun4i-a10-pwm.yaml | 45 ++++++-
.../dts/allwinner/sun50i-h6-beelink-gs1.dts | 4 +
arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 10 ++
drivers/pwm/pwm-sun4i.c | 116 +++++++++++++++++-
4 files changed, 171 insertions(+), 4 deletions(-)
--
2.20.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH v2 1/7] dt-bindings: pwm: allwinner: Add H6 PWM description
2019-11-03 20:33 [PATCH v2 0/7] Add support for H6 PWM Clément Péron
@ 2019-11-03 20:33 ` Clément Péron
2019-11-04 8:03 ` Uwe Kleine-König
2019-11-03 20:33 ` [PATCH v2 2/7] pwm: sun4i: Add an optional probe for reset line Clément Péron
` (5 subsequent siblings)
6 siblings, 1 reply; 33+ messages in thread
From: Clément Péron @ 2019-11-03 20:33 UTC (permalink / raw)
To: Thierry Reding, 'Uwe Kleine-König',
Rob Herring, Mark Rutland, Maxime Ripard, Chen-Yu Tsai
Cc: linux-pwm, Jernej Skrabec, devicetree, linux-kernel,
Clément Péron, linux-arm-kernel
From: Jernej Skrabec <jernej.skrabec@siol.net>
H6 PWM block is basically the same as A20 PWM, except that it also has
bus clock and reset line which needs to be handled accordingly.
Expand Allwinner PWM binding with H6 PWM specifics.
Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
Signed-off-by: Clément Péron <peron.clem@gmail.com>
---
.../bindings/pwm/allwinner,sun4i-a10-pwm.yaml | 45 ++++++++++++++++++-
1 file changed, 44 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/pwm/allwinner,sun4i-a10-pwm.yaml b/Documentation/devicetree/bindings/pwm/allwinner,sun4i-a10-pwm.yaml
index 0ac52f83a58c..bf36ea509f31 100644
--- a/Documentation/devicetree/bindings/pwm/allwinner,sun4i-a10-pwm.yaml
+++ b/Documentation/devicetree/bindings/pwm/allwinner,sun4i-a10-pwm.yaml
@@ -30,13 +30,46 @@ properties:
- items:
- const: allwinner,sun50i-h5-pwm
- const: allwinner,sun5i-a13-pwm
+ - const: allwinner,sun50i-h6-pwm
reg:
maxItems: 1
- clocks:
+ # Even though it only applies to subschemas under the conditionals,
+ # not listing them here will trigger a warning because of the
+ # additionalsProperties set to false.
+ clocks: true
+ clock-names: true
+ resets:
maxItems: 1
+ if:
+ properties:
+ compatible:
+ contains:
+ const: allwinner,sun50i-h6-pwm
+
+ then:
+ properties:
+ clocks:
+ items:
+ - description: Module Clock
+ - description: Bus Clock
+
+ clock-names:
+ items:
+ - const: mod
+ - const: bus
+
+ required:
+ - clock-names
+ - resets
+
+ else:
+ properties:
+ clocks:
+ maxItems: 1
+
required:
- "#pwm-cells"
- compatible
@@ -54,4 +87,14 @@ examples:
#pwm-cells = <3>;
};
+ - |
+ pwm@300a000 {
+ compatible = "allwinner,sun50i-h6-pwm";
+ reg = <0x0300a000 0x400>;
+ clocks = <&osc24M>, <&ccu CLK_BUS_PWM>;
+ clock-names = "mod", "bus";
+ resets = <&ccu RST_BUS_PWM>;
+ #pwm-cells = <3>;
+ };
+
...
--
2.20.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v2 2/7] pwm: sun4i: Add an optional probe for reset line
2019-11-03 20:33 [PATCH v2 0/7] Add support for H6 PWM Clément Péron
2019-11-03 20:33 ` [PATCH v2 1/7] dt-bindings: pwm: allwinner: Add H6 PWM description Clément Péron
@ 2019-11-03 20:33 ` Clément Péron
2019-11-04 8:11 ` Uwe Kleine-König
2019-11-03 20:33 ` [PATCH v2 3/7] pwm: sun4i: Add an optional probe for bus clock Clément Péron
` (4 subsequent siblings)
6 siblings, 1 reply; 33+ messages in thread
From: Clément Péron @ 2019-11-03 20:33 UTC (permalink / raw)
To: Thierry Reding, 'Uwe Kleine-König',
Rob Herring, Mark Rutland, Maxime Ripard, Chen-Yu Tsai
Cc: linux-pwm, Jernej Skrabec, devicetree, linux-kernel,
Clément Péron, linux-arm-kernel
From: Jernej Skrabec <jernej.skrabec@siol.net>
H6 PWM core needs deasserted reset line in order to work.
Add an optional probe for it.
Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
Signed-off-by: Clément Péron <peron.clem@gmail.com>
---
drivers/pwm/pwm-sun4i.c | 32 ++++++++++++++++++++++++++++++--
1 file changed, 30 insertions(+), 2 deletions(-)
diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
index 6f5840a1a82d..d194b8ebdb00 100644
--- a/drivers/pwm/pwm-sun4i.c
+++ b/drivers/pwm/pwm-sun4i.c
@@ -16,6 +16,7 @@
#include <linux/of_device.h>
#include <linux/platform_device.h>
#include <linux/pwm.h>
+#include <linux/reset.h>
#include <linux/slab.h>
#include <linux/spinlock.h>
#include <linux/time.h>
@@ -78,6 +79,7 @@ struct sun4i_pwm_data {
struct sun4i_pwm_chip {
struct pwm_chip chip;
struct clk *clk;
+ struct reset_control *rst;
void __iomem *base;
spinlock_t ctrl_lock;
const struct sun4i_pwm_data *data;
@@ -365,6 +367,20 @@ static int sun4i_pwm_probe(struct platform_device *pdev)
if (IS_ERR(pwm->clk))
return PTR_ERR(pwm->clk);
+ pwm->rst = devm_reset_control_get_optional(&pdev->dev, NULL);
+ if (IS_ERR(pwm->rst)) {
+ if (PTR_ERR(pwm->rst) == -EPROBE_DEFER)
+ return PTR_ERR(pwm->rst);
+ dev_info(&pdev->dev, "no reset control found\n");
+ }
+
+ /* Deassert reset */
+ ret = reset_control_deassert(pwm->rst);
+ if (ret) {
+ dev_err(&pdev->dev, "Cannot deassert reset control\n");
+ return ret;
+ }
+
pwm->chip.dev = &pdev->dev;
pwm->chip.ops = &sun4i_pwm_ops;
pwm->chip.base = -1;
@@ -377,19 +393,31 @@ static int sun4i_pwm_probe(struct platform_device *pdev)
ret = pwmchip_add(&pwm->chip);
if (ret < 0) {
dev_err(&pdev->dev, "failed to add PWM chip: %d\n", ret);
- return ret;
+ goto err_pwm_add;
}
platform_set_drvdata(pdev, pwm);
return 0;
+
+err_pwm_add:
+ reset_control_assert(pwm->rst);
+
+ return ret;
}
static int sun4i_pwm_remove(struct platform_device *pdev)
{
struct sun4i_pwm_chip *pwm = platform_get_drvdata(pdev);
+ int ret;
+
+ ret = pwmchip_remove(&pwm->chip);
+ if (ret)
+ return ret;
+
+ reset_control_assert(pwm->rst);
- return pwmchip_remove(&pwm->chip);
+ return 0;
}
static struct platform_driver sun4i_pwm_driver = {
--
2.20.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v2 3/7] pwm: sun4i: Add an optional probe for bus clock
2019-11-03 20:33 [PATCH v2 0/7] Add support for H6 PWM Clément Péron
2019-11-03 20:33 ` [PATCH v2 1/7] dt-bindings: pwm: allwinner: Add H6 PWM description Clément Péron
2019-11-03 20:33 ` [PATCH v2 2/7] pwm: sun4i: Add an optional probe for reset line Clément Péron
@ 2019-11-03 20:33 ` Clément Péron
2019-11-04 8:24 ` Uwe Kleine-König
2019-11-03 20:33 ` [PATCH v2 4/7] pwm: sun4i: Add support to output source clock directly Clément Péron
` (3 subsequent siblings)
6 siblings, 1 reply; 33+ messages in thread
From: Clément Péron @ 2019-11-03 20:33 UTC (permalink / raw)
To: Thierry Reding, 'Uwe Kleine-König',
Rob Herring, Mark Rutland, Maxime Ripard, Chen-Yu Tsai
Cc: linux-pwm, Jernej Skrabec, devicetree, linux-kernel,
Clément Péron, linux-arm-kernel
From: Jernej Skrabec <jernej.skrabec@siol.net>
H6 PWM core needs bus clock to be enabled in order to work.
Add an optional probe for it and a fallback for previous
bindings without name on module clock.
Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
Signed-off-by: Clément Péron <peron.clem@gmail.com>
---
drivers/pwm/pwm-sun4i.c | 36 ++++++++++++++++++++++++++++++++++++
1 file changed, 36 insertions(+)
diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
index d194b8ebdb00..b5e7ac364f59 100644
--- a/drivers/pwm/pwm-sun4i.c
+++ b/drivers/pwm/pwm-sun4i.c
@@ -78,6 +78,7 @@ struct sun4i_pwm_data {
struct sun4i_pwm_chip {
struct pwm_chip chip;
+ struct clk *bus_clk;
struct clk *clk;
struct reset_control *rst;
void __iomem *base;
@@ -367,6 +368,31 @@ static int sun4i_pwm_probe(struct platform_device *pdev)
if (IS_ERR(pwm->clk))
return PTR_ERR(pwm->clk);
+ /* Get all clocks and reset line */
+ pwm->clk = devm_clk_get_optional(&pdev->dev, "mod");
+ if (IS_ERR(pwm->clk)) {
+ dev_err(&pdev->dev, "get clock failed %ld\n",
+ PTR_ERR(pwm->clk));
+ return PTR_ERR(pwm->clk);
+ }
+
+ /* Fallback for old dtbs with a single clock and no name */
+ if (!pwm->clk) {
+ pwm->clk = devm_clk_get(&pdev->dev, NULL);
+ if (IS_ERR(pwm->clk)) {
+ dev_err(&pdev->dev, "get clock failed %ld\n",
+ PTR_ERR(pwm->clk));
+ return PTR_ERR(pwm->clk);
+ }
+ }
+
+ pwm->bus_clk = devm_clk_get_optional(&pdev->dev, "bus");
+ if (IS_ERR(pwm->bus_clk)) {
+ dev_err(&pdev->dev, "get bus_clock failed %ld\n",
+ PTR_ERR(pwm->bus_clk));
+ return PTR_ERR(pwm->bus_clk);
+ }
+
pwm->rst = devm_reset_control_get_optional(&pdev->dev, NULL);
if (IS_ERR(pwm->rst)) {
if (PTR_ERR(pwm->rst) == -EPROBE_DEFER)
@@ -381,6 +407,13 @@ static int sun4i_pwm_probe(struct platform_device *pdev)
return ret;
}
+ /* Enable bus clock */
+ ret = clk_prepare_enable(pwm->bus_clk);
+ if (ret) {
+ dev_err(&pdev->dev, "Cannot prepare_enable bus_clk\n");
+ goto err_bus;
+ }
+
pwm->chip.dev = &pdev->dev;
pwm->chip.ops = &sun4i_pwm_ops;
pwm->chip.base = -1;
@@ -401,6 +434,8 @@ static int sun4i_pwm_probe(struct platform_device *pdev)
return 0;
err_pwm_add:
+ clk_disable_unprepare(pwm->bus_clk);
+err_bus:
reset_control_assert(pwm->rst);
return ret;
@@ -415,6 +450,7 @@ static int sun4i_pwm_remove(struct platform_device *pdev)
if (ret)
return ret;
+ clk_disable_unprepare(pwm->bus_clk);
reset_control_assert(pwm->rst);
return 0;
--
2.20.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v2 4/7] pwm: sun4i: Add support to output source clock directly
2019-11-03 20:33 [PATCH v2 0/7] Add support for H6 PWM Clément Péron
` (2 preceding siblings ...)
2019-11-03 20:33 ` [PATCH v2 3/7] pwm: sun4i: Add an optional probe for bus clock Clément Péron
@ 2019-11-03 20:33 ` Clément Péron
2019-11-03 22:30 ` kbuild test robot
` (2 more replies)
2019-11-03 20:33 ` [PATCH v2 5/7] pwm: sun4i: Add support for H6 PWM Clément Péron
` (2 subsequent siblings)
6 siblings, 3 replies; 33+ messages in thread
From: Clément Péron @ 2019-11-03 20:33 UTC (permalink / raw)
To: Thierry Reding, 'Uwe Kleine-König',
Rob Herring, Mark Rutland, Maxime Ripard, Chen-Yu Tsai
Cc: linux-pwm, Jernej Skrabec, devicetree, linux-kernel,
Clément Péron, linux-arm-kernel
From: Jernej Skrabec <jernej.skrabec@siol.net>
PWM core has an option to bypass whole logic and output unchanged source
clock as PWM output. This is achieved by enabling bypass bit.
Note that when bypass is enabled, no other setting has any meaning, not
even enable bit.
This mode of operation is needed to achieve high enough frequency to
serve as clock source for AC200 chip, which is integrated into same
package as H6 SoC.
Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
Signed-off-by: Clément Péron <peron.clem@gmail.com>
---
drivers/pwm/pwm-sun4i.c | 39 ++++++++++++++++++++++++++++++++++++++-
1 file changed, 38 insertions(+), 1 deletion(-)
diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
index b5e7ac364f59..2441574674d9 100644
--- a/drivers/pwm/pwm-sun4i.c
+++ b/drivers/pwm/pwm-sun4i.c
@@ -3,6 +3,10 @@
* Driver for Allwinner sun4i Pulse Width Modulation Controller
*
* Copyright (C) 2014 Alexandre Belloni <alexandre.belloni@free-electrons.com>
+ *
+ * Limitations:
+ * - When outputing the source clock directly, the PWM logic will be bypassed
+ * and the currently running period is not guaranted to be completed
*/
#include <linux/bitops.h>
@@ -73,6 +77,7 @@ static const u32 prescaler_table[] = {
struct sun4i_pwm_data {
bool has_prescaler_bypass;
+ bool has_direct_mod_clk_output;
unsigned int npwm;
};
@@ -118,6 +123,20 @@ static void sun4i_pwm_get_state(struct pwm_chip *chip,
val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
+ /*
+ * PWM chapter in H6 manual has a diagram which explains that if bypass
+ * bit is set, no other setting has any meaning. Even more, experiment
+ * proved that also enable bit is ignored in this case.
+ */
+ if ((val & BIT_CH(PWM_BYPASS, pwm->hwpwm)) &&
+ data->has_direct_mod_clk_output) {
+ state->period = DIV_ROUND_CLOSEST_ULL(NSEC_PER_SEC, clk_rate);
+ state->duty_cycle = state->period / 2;
+ state->polarity = PWM_POLARITY_NORMAL;
+ state->enabled = true;
+ return;
+ }
+
if ((PWM_REG_PRESCAL(val, pwm->hwpwm) == PWM_PRESCAL_MASK) &&
sun4i_pwm->data->has_prescaler_bypass)
prescaler = 1;
@@ -203,7 +222,8 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
{
struct sun4i_pwm_chip *sun4i_pwm = to_sun4i_pwm_chip(chip);
struct pwm_state cstate;
- u32 ctrl;
+ u32 ctrl, clk_rate;
+ bool bypass;
int ret;
unsigned int delay_us;
unsigned long now;
@@ -218,6 +238,16 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
}
}
+ /*
+ * Although it would make much more sense to check for bypass in
+ * sun4i_pwm_calculate(), value of bypass bit also depends on "enabled".
+ * Period is allowed to be rounded up or down.
+ */
+ clk_rate = clk_get_rate(sun4i_pwm->clk);
+ bypass = ((state->period * clk_rate >= NSEC_PER_SEC &&
+ state->period * clk_rate < NSEC_PER_SEC + clk_rate) &&
+ state->enabled);
+
spin_lock(&sun4i_pwm->ctrl_lock);
ctrl = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
@@ -265,6 +295,13 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
ctrl &= ~BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
}
+ if (sun4i_pwm->data->has_direct_mod_clk_output) {
+ if (bypass)
+ ctrl |= BIT_CH(PWM_BYPASS, pwm->hwpwm);
+ else
+ ctrl &= ~BIT_CH(PWM_BYPASS, pwm->hwpwm);
+ }
+
sun4i_pwm_writel(sun4i_pwm, ctrl, PWM_CTRL_REG);
spin_unlock(&sun4i_pwm->ctrl_lock);
--
2.20.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v2 5/7] pwm: sun4i: Add support for H6 PWM
2019-11-03 20:33 [PATCH v2 0/7] Add support for H6 PWM Clément Péron
` (3 preceding siblings ...)
2019-11-03 20:33 ` [PATCH v2 4/7] pwm: sun4i: Add support to output source clock directly Clément Péron
@ 2019-11-03 20:33 ` Clément Péron
2019-11-03 20:33 ` [PATCH v2 6/7] arm64: dts: allwinner: h6: Add PWM node Clément Péron
2019-11-03 20:33 ` [PATCH v2 7/7] [DO NOT MERGE] arm64: allwinner: h6: enable Beelink GS1 PWM Clément Péron
6 siblings, 0 replies; 33+ messages in thread
From: Clément Péron @ 2019-11-03 20:33 UTC (permalink / raw)
To: Thierry Reding, 'Uwe Kleine-König',
Rob Herring, Mark Rutland, Maxime Ripard, Chen-Yu Tsai
Cc: linux-pwm, Jernej Skrabec, devicetree, linux-kernel,
Clément Péron, linux-arm-kernel
From: Jernej Skrabec <jernej.skrabec@siol.net>
Now that sun4i PWM driver supports deasserting reset line and enabling
bus clock, support for H6 PWM can be added.
Note that while H6 PWM has two channels, only first one is wired to
output pin. Second channel is used as a clock source to companion AC200
chip which is bundled into same package.
Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
Signed-off-by: Clément Péron <peron.clem@gmail.com>
---
drivers/pwm/pwm-sun4i.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
index 2441574674d9..0ccc93ce33f4 100644
--- a/drivers/pwm/pwm-sun4i.c
+++ b/drivers/pwm/pwm-sun4i.c
@@ -360,6 +360,12 @@ static const struct sun4i_pwm_data sun4i_pwm_single_bypass = {
.npwm = 1,
};
+static const struct sun4i_pwm_data sun50i_h6_pwm_data = {
+ .has_prescaler_bypass = true,
+ .has_direct_mod_clk_output = true,
+ .npwm = 2,
+};
+
static const struct of_device_id sun4i_pwm_dt_ids[] = {
{
.compatible = "allwinner,sun4i-a10-pwm",
@@ -376,6 +382,9 @@ static const struct of_device_id sun4i_pwm_dt_ids[] = {
}, {
.compatible = "allwinner,sun8i-h3-pwm",
.data = &sun4i_pwm_single_bypass,
+ }, {
+ .compatible = "allwinner,sun50i-h6-pwm",
+ .data = &sun50i_h6_pwm_data,
}, {
/* sentinel */
},
--
2.20.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v2 6/7] arm64: dts: allwinner: h6: Add PWM node
2019-11-03 20:33 [PATCH v2 0/7] Add support for H6 PWM Clément Péron
` (4 preceding siblings ...)
2019-11-03 20:33 ` [PATCH v2 5/7] pwm: sun4i: Add support for H6 PWM Clément Péron
@ 2019-11-03 20:33 ` Clément Péron
2019-11-03 20:33 ` [PATCH v2 7/7] [DO NOT MERGE] arm64: allwinner: h6: enable Beelink GS1 PWM Clément Péron
6 siblings, 0 replies; 33+ messages in thread
From: Clément Péron @ 2019-11-03 20:33 UTC (permalink / raw)
To: Thierry Reding, 'Uwe Kleine-König',
Rob Herring, Mark Rutland, Maxime Ripard, Chen-Yu Tsai
Cc: linux-pwm, Jernej Skrabec, devicetree, linux-kernel,
Clément Péron, linux-arm-kernel
From: Jernej Skrabec <jernej.skrabec@siol.net>
Allwinner H6 PWM is similar to that in A20 except that it has additional
bus clock and reset line.
Note that first PWM channel is connected to output pin and second
channel is used internally, as a clock source to AC200 co-packaged chip.
This means that any combination of these two channels can be used and
thus it doesn't make sense to add pinctrl nodes at this point.
Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
Signed-off-by: Clément Péron <peron.clem@gmail.com>
---
arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
index 0d5ea19336a1..b0d9ee1ead13 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
@@ -222,6 +222,16 @@
status = "disabled";
};
+ pwm: pwm@300a000 {
+ compatible = "allwinner,sun50i-h6-pwm";
+ reg = <0x0300a000 0x400>;
+ clocks = <&osc24M>, <&ccu CLK_BUS_PWM>;
+ clock-names = "mod", "bus";
+ resets = <&ccu RST_BUS_PWM>;
+ #pwm-cells = <3>;
+ status = "disabled";
+ };
+
pio: pinctrl@300b000 {
compatible = "allwinner,sun50i-h6-pinctrl";
reg = <0x0300b000 0x400>;
--
2.20.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH v2 7/7] [DO NOT MERGE] arm64: allwinner: h6: enable Beelink GS1 PWM
2019-11-03 20:33 [PATCH v2 0/7] Add support for H6 PWM Clément Péron
` (5 preceding siblings ...)
2019-11-03 20:33 ` [PATCH v2 6/7] arm64: dts: allwinner: h6: Add PWM node Clément Péron
@ 2019-11-03 20:33 ` Clément Péron
6 siblings, 0 replies; 33+ messages in thread
From: Clément Péron @ 2019-11-03 20:33 UTC (permalink / raw)
To: Thierry Reding, 'Uwe Kleine-König',
Rob Herring, Mark Rutland, Maxime Ripard, Chen-Yu Tsai
Cc: linux-pwm, Clément Péron, linux-kernel,
linux-arm-kernel, devicetree
Signed-off-by: Clément Péron <peron.clem@gmail.com>
---
arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts b/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts
index 1d05d570142f..38aba7e5bbd9 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts
+++ b/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts
@@ -131,6 +131,10 @@
vcc-pg-supply = <®_aldo1>;
};
+&pwm {
+ status = "okay";
+};
+
&r_i2c {
status = "okay";
--
2.20.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH v2 4/7] pwm: sun4i: Add support to output source clock directly
2019-11-03 20:33 ` [PATCH v2 4/7] pwm: sun4i: Add support to output source clock directly Clément Péron
@ 2019-11-03 22:30 ` kbuild test robot
2019-11-03 22:41 ` Clément Péron
2019-11-03 22:58 ` kbuild test robot
2019-11-04 8:38 ` Uwe Kleine-König
2 siblings, 1 reply; 33+ messages in thread
From: kbuild test robot @ 2019-11-03 22:30 UTC (permalink / raw)
To: Clément Péron
Cc: Mark Rutland, linux-pwm, Jernej Skrabec, kbuild-all, devicetree,
linux-kernel, Maxime Ripard, Rob Herring, Chen-Yu Tsai,
Thierry Reding, Clément Péron,
'Uwe Kleine-König',
linux-arm-kernel
[-- Attachment #1: Type: text/plain, Size: 3626 bytes --]
Hi "Clément,
I love your patch! Yet something to improve:
[auto build test ERROR on sunxi/sunxi/for-next]
[also build test ERROR on v5.4-rc5 next-20191031]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Cl-ment-P-ron/Add-support-for-H6-PWM/20191104-043621
base: https://git.kernel.org/pub/scm/linux/kernel/git/sunxi/linux.git sunxi/for-next
config: riscv-allmodconfig (attached as .config)
compiler: riscv64-linux-gcc (GCC) 7.4.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.4.0 make.cross ARCH=riscv
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
drivers//pwm/pwm-sun4i.c: In function 'sun4i_pwm_get_state':
>> drivers//pwm/pwm-sun4i.c:132:6: error: 'data' undeclared (first use in this function)
data->has_direct_mod_clk_output) {
^~~~
drivers//pwm/pwm-sun4i.c:132:6: note: each undeclared identifier is reported only once for each function it appears in
vim +/data +132 drivers//pwm/pwm-sun4i.c
112
113 static void sun4i_pwm_get_state(struct pwm_chip *chip,
114 struct pwm_device *pwm,
115 struct pwm_state *state)
116 {
117 struct sun4i_pwm_chip *sun4i_pwm = to_sun4i_pwm_chip(chip);
118 u64 clk_rate, tmp;
119 u32 val;
120 unsigned int prescaler;
121
122 clk_rate = clk_get_rate(sun4i_pwm->clk);
123
124 val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
125
126 /*
127 * PWM chapter in H6 manual has a diagram which explains that if bypass
128 * bit is set, no other setting has any meaning. Even more, experiment
129 * proved that also enable bit is ignored in this case.
130 */
131 if ((val & BIT_CH(PWM_BYPASS, pwm->hwpwm)) &&
> 132 data->has_direct_mod_clk_output) {
133 state->period = DIV_ROUND_CLOSEST_ULL(NSEC_PER_SEC, clk_rate);
134 state->duty_cycle = state->period / 2;
135 state->polarity = PWM_POLARITY_NORMAL;
136 state->enabled = true;
137 return;
138 }
139
140 if ((PWM_REG_PRESCAL(val, pwm->hwpwm) == PWM_PRESCAL_MASK) &&
141 sun4i_pwm->data->has_prescaler_bypass)
142 prescaler = 1;
143 else
144 prescaler = prescaler_table[PWM_REG_PRESCAL(val, pwm->hwpwm)];
145
146 if (prescaler == 0)
147 return;
148
149 if (val & BIT_CH(PWM_ACT_STATE, pwm->hwpwm))
150 state->polarity = PWM_POLARITY_NORMAL;
151 else
152 state->polarity = PWM_POLARITY_INVERSED;
153
154 if ((val & BIT_CH(PWM_CLK_GATING | PWM_EN, pwm->hwpwm)) ==
155 BIT_CH(PWM_CLK_GATING | PWM_EN, pwm->hwpwm))
156 state->enabled = true;
157 else
158 state->enabled = false;
159
160 val = sun4i_pwm_readl(sun4i_pwm, PWM_CH_PRD(pwm->hwpwm));
161
162 tmp = prescaler * NSEC_PER_SEC * PWM_REG_DTY(val);
163 state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, clk_rate);
164
165 tmp = prescaler * NSEC_PER_SEC * PWM_REG_PRD(val);
166 state->period = DIV_ROUND_CLOSEST_ULL(tmp, clk_rate);
167 }
168
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 59985 bytes --]
[-- Attachment #3: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 4/7] pwm: sun4i: Add support to output source clock directly
2019-11-03 22:30 ` kbuild test robot
@ 2019-11-03 22:41 ` Clément Péron
0 siblings, 0 replies; 33+ messages in thread
From: Clément Péron @ 2019-11-03 22:41 UTC (permalink / raw)
To: kbuild test robot
Cc: Mark Rutland, linux-pwm, Jernej Skrabec, kbuild-all, devicetree,
linux-kernel, Maxime Ripard, Rob Herring, Chen-Yu Tsai,
Thierry Reding, Uwe Kleine-König, linux-arm-kernel
Hi,
On Sun, 3 Nov 2019 at 23:30, kbuild test robot <lkp@intel.com> wrote:
>
> Hi "Clément,
>
> I love your patch! Yet something to improve:
>
> [auto build test ERROR on sunxi/sunxi/for-next]
> [also build test ERROR on v5.4-rc5 next-20191031]
> [if your patch is applied to the wrong git tree, please drop us a note to help
> improve the system. BTW, we also suggest to use '--base' option to specify the
> base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
>
> url: https://github.com/0day-ci/linux/commits/Cl-ment-P-ron/Add-support-for-H6-PWM/20191104-043621
> base: https://git.kernel.org/pub/scm/linux/kernel/git/sunxi/linux.git sunxi/for-next
> config: riscv-allmodconfig (attached as .config)
> compiler: riscv64-linux-gcc (GCC) 7.4.0
> reproduce:
> wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # save the attached .config to linux build tree
> GCC_VERSION=7.4.0 make.cross ARCH=riscv
>
> If you fix the issue, kindly add following tag
> Reported-by: kbuild test robot <lkp@intel.com>
>
> All errors (new ones prefixed by >>):
>
> drivers//pwm/pwm-sun4i.c: In function 'sun4i_pwm_get_state':
> >> drivers//pwm/pwm-sun4i.c:132:6: error: 'data' undeclared (first use in this function)
> data->has_direct_mod_clk_output) {
> ^~~~
Arg, bad last minute indent fix :
This should be "sun4i_pwm->data->has_direct_mod_clk_output"
Sorry for that,
Clément
> drivers//pwm/pwm-sun4i.c:132:6: note: each undeclared identifier is reported only once for each function it appears in
>
> vim +/data +132 drivers//pwm/pwm-sun4i.c
>
> 112
> 113 static void sun4i_pwm_get_state(struct pwm_chip *chip,
> 114 struct pwm_device *pwm,
> 115 struct pwm_state *state)
> 116 {
> 117 struct sun4i_pwm_chip *sun4i_pwm = to_sun4i_pwm_chip(chip);
> 118 u64 clk_rate, tmp;
> 119 u32 val;
> 120 unsigned int prescaler;
> 121
> 122 clk_rate = clk_get_rate(sun4i_pwm->clk);
> 123
> 124 val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
> 125
> 126 /*
> 127 * PWM chapter in H6 manual has a diagram which explains that if bypass
> 128 * bit is set, no other setting has any meaning. Even more, experiment
> 129 * proved that also enable bit is ignored in this case.
> 130 */
> 131 if ((val & BIT_CH(PWM_BYPASS, pwm->hwpwm)) &&
> > 132 data->has_direct_mod_clk_output) {
> 133 state->period = DIV_ROUND_CLOSEST_ULL(NSEC_PER_SEC, clk_rate);
> 134 state->duty_cycle = state->period / 2;
> 135 state->polarity = PWM_POLARITY_NORMAL;
> 136 state->enabled = true;
> 137 return;
> 138 }
> 139
> 140 if ((PWM_REG_PRESCAL(val, pwm->hwpwm) == PWM_PRESCAL_MASK) &&
> 141 sun4i_pwm->data->has_prescaler_bypass)
> 142 prescaler = 1;
> 143 else
> 144 prescaler = prescaler_table[PWM_REG_PRESCAL(val, pwm->hwpwm)];
> 145
> 146 if (prescaler == 0)
> 147 return;
> 148
> 149 if (val & BIT_CH(PWM_ACT_STATE, pwm->hwpwm))
> 150 state->polarity = PWM_POLARITY_NORMAL;
> 151 else
> 152 state->polarity = PWM_POLARITY_INVERSED;
> 153
> 154 if ((val & BIT_CH(PWM_CLK_GATING | PWM_EN, pwm->hwpwm)) ==
> 155 BIT_CH(PWM_CLK_GATING | PWM_EN, pwm->hwpwm))
> 156 state->enabled = true;
> 157 else
> 158 state->enabled = false;
> 159
> 160 val = sun4i_pwm_readl(sun4i_pwm, PWM_CH_PRD(pwm->hwpwm));
> 161
> 162 tmp = prescaler * NSEC_PER_SEC * PWM_REG_DTY(val);
> 163 state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, clk_rate);
> 164
> 165 tmp = prescaler * NSEC_PER_SEC * PWM_REG_PRD(val);
> 166 state->period = DIV_ROUND_CLOSEST_ULL(tmp, clk_rate);
> 167 }
> 168
>
> ---
> 0-DAY kernel test infrastructure Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all Intel Corporation
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 4/7] pwm: sun4i: Add support to output source clock directly
2019-11-03 20:33 ` [PATCH v2 4/7] pwm: sun4i: Add support to output source clock directly Clément Péron
2019-11-03 22:30 ` kbuild test robot
@ 2019-11-03 22:58 ` kbuild test robot
2019-11-04 8:38 ` Uwe Kleine-König
2 siblings, 0 replies; 33+ messages in thread
From: kbuild test robot @ 2019-11-03 22:58 UTC (permalink / raw)
To: Clément Péron
Cc: Mark Rutland, linux-pwm, Jernej Skrabec, kbuild-all, devicetree,
linux-kernel, Maxime Ripard, Rob Herring, Chen-Yu Tsai,
Thierry Reding, Clément Péron,
'Uwe Kleine-König',
linux-arm-kernel
[-- Attachment #1: Type: text/plain, Size: 3650 bytes --]
Hi "Clément,
I love your patch! Yet something to improve:
[auto build test ERROR on sunxi/sunxi/for-next]
[also build test ERROR on v5.4-rc5 next-20191031]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Cl-ment-P-ron/Add-support-for-H6-PWM/20191104-043621
base: https://git.kernel.org/pub/scm/linux/kernel/git/sunxi/linux.git sunxi/for-next
config: mips-allmodconfig (attached as .config)
compiler: mips-linux-gcc (GCC) 7.4.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.4.0 make.cross ARCH=mips
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
drivers//pwm/pwm-sun4i.c: In function 'sun4i_pwm_get_state':
>> drivers//pwm/pwm-sun4i.c:132:6: error: 'data' undeclared (first use in this function); did you mean 'path'?
data->has_direct_mod_clk_output) {
^~~~
path
drivers//pwm/pwm-sun4i.c:132:6: note: each undeclared identifier is reported only once for each function it appears in
vim +132 drivers//pwm/pwm-sun4i.c
112
113 static void sun4i_pwm_get_state(struct pwm_chip *chip,
114 struct pwm_device *pwm,
115 struct pwm_state *state)
116 {
117 struct sun4i_pwm_chip *sun4i_pwm = to_sun4i_pwm_chip(chip);
118 u64 clk_rate, tmp;
119 u32 val;
120 unsigned int prescaler;
121
122 clk_rate = clk_get_rate(sun4i_pwm->clk);
123
124 val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
125
126 /*
127 * PWM chapter in H6 manual has a diagram which explains that if bypass
128 * bit is set, no other setting has any meaning. Even more, experiment
129 * proved that also enable bit is ignored in this case.
130 */
131 if ((val & BIT_CH(PWM_BYPASS, pwm->hwpwm)) &&
> 132 data->has_direct_mod_clk_output) {
133 state->period = DIV_ROUND_CLOSEST_ULL(NSEC_PER_SEC, clk_rate);
134 state->duty_cycle = state->period / 2;
135 state->polarity = PWM_POLARITY_NORMAL;
136 state->enabled = true;
137 return;
138 }
139
140 if ((PWM_REG_PRESCAL(val, pwm->hwpwm) == PWM_PRESCAL_MASK) &&
141 sun4i_pwm->data->has_prescaler_bypass)
142 prescaler = 1;
143 else
144 prescaler = prescaler_table[PWM_REG_PRESCAL(val, pwm->hwpwm)];
145
146 if (prescaler == 0)
147 return;
148
149 if (val & BIT_CH(PWM_ACT_STATE, pwm->hwpwm))
150 state->polarity = PWM_POLARITY_NORMAL;
151 else
152 state->polarity = PWM_POLARITY_INVERSED;
153
154 if ((val & BIT_CH(PWM_CLK_GATING | PWM_EN, pwm->hwpwm)) ==
155 BIT_CH(PWM_CLK_GATING | PWM_EN, pwm->hwpwm))
156 state->enabled = true;
157 else
158 state->enabled = false;
159
160 val = sun4i_pwm_readl(sun4i_pwm, PWM_CH_PRD(pwm->hwpwm));
161
162 tmp = prescaler * NSEC_PER_SEC * PWM_REG_DTY(val);
163 state->duty_cycle = DIV_ROUND_CLOSEST_ULL(tmp, clk_rate);
164
165 tmp = prescaler * NSEC_PER_SEC * PWM_REG_PRD(val);
166 state->period = DIV_ROUND_CLOSEST_ULL(tmp, clk_rate);
167 }
168
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 62105 bytes --]
[-- Attachment #3: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 1/7] dt-bindings: pwm: allwinner: Add H6 PWM description
2019-11-03 20:33 ` [PATCH v2 1/7] dt-bindings: pwm: allwinner: Add H6 PWM description Clément Péron
@ 2019-11-04 8:03 ` Uwe Kleine-König
2019-11-04 17:49 ` Clément Péron
2019-11-05 11:11 ` Maxime Ripard
0 siblings, 2 replies; 33+ messages in thread
From: Uwe Kleine-König @ 2019-11-04 8:03 UTC (permalink / raw)
To: Clément Péron
Cc: Mark Rutland, linux-pwm, Jernej Skrabec, devicetree,
linux-kernel, Maxime Ripard, Rob Herring, Chen-Yu Tsai,
Thierry Reding, kernel, linux-arm-kernel
On Sun, Nov 03, 2019 at 09:33:28PM +0100, Clément Péron wrote:
> From: Jernej Skrabec <jernej.skrabec@siol.net>
>
> H6 PWM block is basically the same as A20 PWM, except that it also has
> bus clock and reset line which needs to be handled accordingly.
>
> Expand Allwinner PWM binding with H6 PWM specifics.
>
> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> Signed-off-by: Clément Péron <peron.clem@gmail.com>
> ---
> .../bindings/pwm/allwinner,sun4i-a10-pwm.yaml | 45 ++++++++++++++++++-
> 1 file changed, 44 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/pwm/allwinner,sun4i-a10-pwm.yaml b/Documentation/devicetree/bindings/pwm/allwinner,sun4i-a10-pwm.yaml
> index 0ac52f83a58c..bf36ea509f31 100644
> --- a/Documentation/devicetree/bindings/pwm/allwinner,sun4i-a10-pwm.yaml
> +++ b/Documentation/devicetree/bindings/pwm/allwinner,sun4i-a10-pwm.yaml
> @@ -30,13 +30,46 @@ properties:
> - items:
> - const: allwinner,sun50i-h5-pwm
> - const: allwinner,sun5i-a13-pwm
> + - const: allwinner,sun50i-h6-pwm
>
> reg:
> maxItems: 1
>
> - clocks:
> + # Even though it only applies to subschemas under the conditionals,
> + # not listing them here will trigger a warning because of the
> + # additionalsProperties set to false.
> + clocks: true
> + clock-names: true
> + resets:
> maxItems: 1
>
> + if:
> + properties:
> + compatible:
> + contains:
> + const: allwinner,sun50i-h6-pwm
> +
> + then:
> + properties:
> + clocks:
> + items:
> + - description: Module Clock
> + - description: Bus Clock
> +
> + clock-names:
> + items:
> + - const: mod
> + - const: bus
> +
> + required:
> + - clock-names
> + - resets
> +
> + else:
> + properties:
> + clocks:
> + maxItems: 1
> +
I guess this hunk says "If this is a allwinner,sun50i-h6-pwm, a mod and
bus clock is required.", right?
I wonder if it is sensible to require a clock-names property in the else
branch, too. This would make it obvious if the clock there corresponds
to the "mod" or the "bus" clock on H6. (I guess it's "mod".)
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 2/7] pwm: sun4i: Add an optional probe for reset line
2019-11-03 20:33 ` [PATCH v2 2/7] pwm: sun4i: Add an optional probe for reset line Clément Péron
@ 2019-11-04 8:11 ` Uwe Kleine-König
2019-11-04 17:50 ` Clément Péron
2019-11-05 7:01 ` Philipp Zabel
0 siblings, 2 replies; 33+ messages in thread
From: Uwe Kleine-König @ 2019-11-04 8:11 UTC (permalink / raw)
To: Clément Péron
Cc: Mark Rutland, linux-pwm, Jernej Skrabec, devicetree,
linux-kernel, Maxime Ripard, Rob Herring, Chen-Yu Tsai,
Thierry Reding, Philipp Zabel, linux-arm-kernel
Hello,
adding Philipp Zabel (= reset controller maintainer) to Cc: and so I'm
not stripping the uncommented parts of the patch.
On Sun, Nov 03, 2019 at 09:33:29PM +0100, Clément Péron wrote:
> From: Jernej Skrabec <jernej.skrabec@siol.net>
>
> H6 PWM core needs deasserted reset line in order to work.
>
> Add an optional probe for it.
>
> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> Signed-off-by: Clément Péron <peron.clem@gmail.com>
> ---
> drivers/pwm/pwm-sun4i.c | 32 ++++++++++++++++++++++++++++++--
> 1 file changed, 30 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> index 6f5840a1a82d..d194b8ebdb00 100644
> --- a/drivers/pwm/pwm-sun4i.c
> +++ b/drivers/pwm/pwm-sun4i.c
> @@ -16,6 +16,7 @@
> #include <linux/of_device.h>
> #include <linux/platform_device.h>
> #include <linux/pwm.h>
> +#include <linux/reset.h>
> #include <linux/slab.h>
> #include <linux/spinlock.h>
> #include <linux/time.h>
> @@ -78,6 +79,7 @@ struct sun4i_pwm_data {
> struct sun4i_pwm_chip {
> struct pwm_chip chip;
> struct clk *clk;
> + struct reset_control *rst;
> void __iomem *base;
> spinlock_t ctrl_lock;
> const struct sun4i_pwm_data *data;
> @@ -365,6 +367,20 @@ static int sun4i_pwm_probe(struct platform_device *pdev)
> if (IS_ERR(pwm->clk))
> return PTR_ERR(pwm->clk);
>
> + pwm->rst = devm_reset_control_get_optional(&pdev->dev, NULL);
> + if (IS_ERR(pwm->rst)) {
> + if (PTR_ERR(pwm->rst) == -EPROBE_DEFER)
> + return PTR_ERR(pwm->rst);
> + dev_info(&pdev->dev, "no reset control found\n");
I would degrade this to a dev_dbg. Otherwise this spams the log for all
unaffected machines. devm_reset_control_get_optional() is defined in a
section that has a comment "These inline function calls will be removed
once all consumers have been moved over to the new explicit API.", so I
guess you want devm_reset_control_get_optional_exclusive or even
devm_reset_control_get_optional_shared here.
@Philipp: maybe a check in checkpatch that warns about introduction of
such new instances would be good?!
> + }
> +
> + /* Deassert reset */
> + ret = reset_control_deassert(pwm->rst);
> + if (ret) {
> + dev_err(&pdev->dev, "Cannot deassert reset control\n");
> + return ret;
> + }
> +
> pwm->chip.dev = &pdev->dev;
> pwm->chip.ops = &sun4i_pwm_ops;
> pwm->chip.base = -1;
> @@ -377,19 +393,31 @@ static int sun4i_pwm_probe(struct platform_device *pdev)
> ret = pwmchip_add(&pwm->chip);
> if (ret < 0) {
> dev_err(&pdev->dev, "failed to add PWM chip: %d\n", ret);
> - return ret;
> + goto err_pwm_add;
> }
>
> platform_set_drvdata(pdev, pwm);
>
> return 0;
> +
> +err_pwm_add:
> + reset_control_assert(pwm->rst);
> +
> + return ret;
> }
>
> static int sun4i_pwm_remove(struct platform_device *pdev)
> {
> struct sun4i_pwm_chip *pwm = platform_get_drvdata(pdev);
> + int ret;
> +
> + ret = pwmchip_remove(&pwm->chip);
> + if (ret)
> + return ret;
> +
> + reset_control_assert(pwm->rst);
>
> - return pwmchip_remove(&pwm->chip);
> + return 0;
> }
>
> static struct platform_driver sun4i_pwm_driver = {
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 3/7] pwm: sun4i: Add an optional probe for bus clock
2019-11-03 20:33 ` [PATCH v2 3/7] pwm: sun4i: Add an optional probe for bus clock Clément Péron
@ 2019-11-04 8:24 ` Uwe Kleine-König
2019-11-04 18:07 ` Clément Péron
0 siblings, 1 reply; 33+ messages in thread
From: Uwe Kleine-König @ 2019-11-04 8:24 UTC (permalink / raw)
To: Clément Péron
Cc: Mark Rutland, linux-pwm, Jernej Skrabec, devicetree,
linux-kernel, Maxime Ripard, Rob Herring, Chen-Yu Tsai,
Thierry Reding, kernel, linux-arm-kernel
Hello,
On Sun, Nov 03, 2019 at 09:33:30PM +0100, Clément Péron wrote:
> From: Jernej Skrabec <jernej.skrabec@siol.net>
>
> H6 PWM core needs bus clock to be enabled in order to work.
>
> Add an optional probe for it and a fallback for previous
> bindings without name on module clock.
>
> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> Signed-off-by: Clément Péron <peron.clem@gmail.com>
> ---
> drivers/pwm/pwm-sun4i.c | 36 ++++++++++++++++++++++++++++++++++++
> 1 file changed, 36 insertions(+)
>
> diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> index d194b8ebdb00..b5e7ac364f59 100644
> --- a/drivers/pwm/pwm-sun4i.c
> +++ b/drivers/pwm/pwm-sun4i.c
> @@ -78,6 +78,7 @@ struct sun4i_pwm_data {
>
> struct sun4i_pwm_chip {
> struct pwm_chip chip;
> + struct clk *bus_clk;
> struct clk *clk;
> struct reset_control *rst;
> void __iomem *base;
> @@ -367,6 +368,31 @@ static int sun4i_pwm_probe(struct platform_device *pdev)
Adding more context here:
| pwm->clk = devm_clk_get(&pdev->dev, NULL);
> if (IS_ERR(pwm->clk))
> return PTR_ERR(pwm->clk);
>
> + /* Get all clocks and reset line */
> + pwm->clk = devm_clk_get_optional(&pdev->dev, "mod");
> + if (IS_ERR(pwm->clk)) {
> + dev_err(&pdev->dev, "get clock failed %ld\n",
> + PTR_ERR(pwm->clk));
> + return PTR_ERR(pwm->clk);
> + }
I guess you want to drop the first assignment to pwm->clk.
> + /* Fallback for old dtbs with a single clock and no name */
> + if (!pwm->clk) {
> + pwm->clk = devm_clk_get(&pdev->dev, NULL);
> + if (IS_ERR(pwm->clk)) {
> + dev_err(&pdev->dev, "get clock failed %ld\n",
> + PTR_ERR(pwm->clk));
> + return PTR_ERR(pwm->clk);
> + }
> + }
There is a slight change of behaviour if I'm not mistaken. If you have
this:
clocks = <&clk1>;
clock-names = "mod";
pwm {
compatible = "allwinner,sun4i-a10-pwm"
clocks = <&clk2>;
}
you now use clk1 instead of clk2 before.
Assuming this is only a theoretical problem, at least pointing this out
in the commit log would be good I think.
> + pwm->bus_clk = devm_clk_get_optional(&pdev->dev, "bus");
> + if (IS_ERR(pwm->bus_clk)) {
> + dev_err(&pdev->dev, "get bus_clock failed %ld\n",
> + PTR_ERR(pwm->bus_clk));
> + return PTR_ERR(pwm->bus_clk);
> + }
> +
> pwm->rst = devm_reset_control_get_optional(&pdev->dev, NULL);
> if (IS_ERR(pwm->rst)) {
> if (PTR_ERR(pwm->rst) == -EPROBE_DEFER)
> @@ -381,6 +407,13 @@ static int sun4i_pwm_probe(struct platform_device *pdev)
> return ret;
> }
>
> + /* Enable bus clock */
> + ret = clk_prepare_enable(pwm->bus_clk);
> + if (ret) {
> + dev_err(&pdev->dev, "Cannot prepare_enable bus_clk\n");
I'd do s/prepare_enable/prepare and enable/ here.
> + goto err_bus;
> + }
> +
> pwm->chip.dev = &pdev->dev;
> pwm->chip.ops = &sun4i_pwm_ops;
> pwm->chip.base = -1;
> @@ -401,6 +434,8 @@ static int sun4i_pwm_probe(struct platform_device *pdev)
> return 0;
>
> err_pwm_add:
> + clk_disable_unprepare(pwm->bus_clk);
> +err_bus:
> reset_control_assert(pwm->rst);
>
> return ret;
What is that clock used for? Is it required to access the hardware
registers? Or is it only required while the PWM is enabled? If so you
could enable the clock more finegrainded.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 4/7] pwm: sun4i: Add support to output source clock directly
2019-11-03 20:33 ` [PATCH v2 4/7] pwm: sun4i: Add support to output source clock directly Clément Péron
2019-11-03 22:30 ` kbuild test robot
2019-11-03 22:58 ` kbuild test robot
@ 2019-11-04 8:38 ` Uwe Kleine-König
2019-11-04 21:28 ` Clément Péron
2 siblings, 1 reply; 33+ messages in thread
From: Uwe Kleine-König @ 2019-11-04 8:38 UTC (permalink / raw)
To: Clément Péron
Cc: Mark Rutland, linux-pwm, Jernej Skrabec, devicetree,
linux-kernel, Maxime Ripard, Rob Herring, Chen-Yu Tsai,
Thierry Reding, linux-arm-kernel
On Sun, Nov 03, 2019 at 09:33:31PM +0100, Clément Péron wrote:
> From: Jernej Skrabec <jernej.skrabec@siol.net>
>
> PWM core has an option to bypass whole logic and output unchanged source
> clock as PWM output. This is achieved by enabling bypass bit.
>
> Note that when bypass is enabled, no other setting has any meaning, not
> even enable bit.
>
> This mode of operation is needed to achieve high enough frequency to
> serve as clock source for AC200 chip, which is integrated into same
> package as H6 SoC.
I think the , should be dropped.
> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> Signed-off-by: Clément Péron <peron.clem@gmail.com>
> ---
> drivers/pwm/pwm-sun4i.c | 39 ++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 38 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> index b5e7ac364f59..2441574674d9 100644
> --- a/drivers/pwm/pwm-sun4i.c
> +++ b/drivers/pwm/pwm-sun4i.c
> @@ -3,6 +3,10 @@
> * Driver for Allwinner sun4i Pulse Width Modulation Controller
> *
> * Copyright (C) 2014 Alexandre Belloni <alexandre.belloni@free-electrons.com>
> + *
> + * Limitations:
> + * - When outputing the source clock directly, the PWM logic will be bypassed
> + * and the currently running period is not guaranted to be completed
Typo: guaranted -> guaranteed
> */
>
> #include <linux/bitops.h>
> @@ -73,6 +77,7 @@ static const u32 prescaler_table[] = {
>
> struct sun4i_pwm_data {
> bool has_prescaler_bypass;
> + bool has_direct_mod_clk_output;
> unsigned int npwm;
> };
>
> @@ -118,6 +123,20 @@ static void sun4i_pwm_get_state(struct pwm_chip *chip,
>
> val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
>
> + /*
> + * PWM chapter in H6 manual has a diagram which explains that if bypass
> + * bit is set, no other setting has any meaning. Even more, experiment
> + * proved that also enable bit is ignored in this case.
> + */
> + if ((val & BIT_CH(PWM_BYPASS, pwm->hwpwm)) &&
> + data->has_direct_mod_clk_output) {
> + state->period = DIV_ROUND_CLOSEST_ULL(NSEC_PER_SEC, clk_rate);
> + state->duty_cycle = state->period / 2;
> + state->polarity = PWM_POLARITY_NORMAL;
> + state->enabled = true;
> + return;
> + }
Not sure how the rest of sun4i_pwm_get_state behaves, but I would prefer
to let .get_state() round up which together with .apply_state() rounding
down yields sound behaviour.
> +
> if ((PWM_REG_PRESCAL(val, pwm->hwpwm) == PWM_PRESCAL_MASK) &&
> sun4i_pwm->data->has_prescaler_bypass)
> prescaler = 1;
> @@ -203,7 +222,8 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> {
> struct sun4i_pwm_chip *sun4i_pwm = to_sun4i_pwm_chip(chip);
> struct pwm_state cstate;
> - u32 ctrl;
> + u32 ctrl, clk_rate;
> + bool bypass;
> int ret;
> unsigned int delay_us;
> unsigned long now;
> @@ -218,6 +238,16 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> }
> }
>
> + /*
> + * Although it would make much more sense to check for bypass in
> + * sun4i_pwm_calculate(), value of bypass bit also depends on "enabled".
> + * Period is allowed to be rounded up or down.
> + */
> + clk_rate = clk_get_rate(sun4i_pwm->clk);
> + bypass = ((state->period * clk_rate >= NSEC_PER_SEC &&
> + state->period * clk_rate < NSEC_PER_SEC + clk_rate) &&
> + state->enabled);
I guess the compiler is smart enough here, but checking for
state->enabled is cheaper than the other checks, so putting this at the
start of the expression seems sensible.
The comment doesn't match the code. You don't round up state->period.
(This is good, please fix the comment.) I think dropping the check
state->period * clk_rate < NSEC_PER_SEC + clk_rate
would be fine, too.
I'd like to have a check for
state->duty_cycle * clk_rate >= NSEC_PER_SEC / 2 &&
state->duty_cycle * clk_rate < NSEC_PER_SEC
here. If this isn't true rather disable the PWM or output a 100% duty
cycle with a larger period.
> +
> spin_lock(&sun4i_pwm->ctrl_lock);
> ctrl = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
>
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 1/7] dt-bindings: pwm: allwinner: Add H6 PWM description
2019-11-04 8:03 ` Uwe Kleine-König
@ 2019-11-04 17:49 ` Clément Péron
2019-11-05 11:11 ` Maxime Ripard
1 sibling, 0 replies; 33+ messages in thread
From: Clément Péron @ 2019-11-04 17:49 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Mark Rutland, linux-pwm, Jernej Skrabec, devicetree,
linux-kernel, Maxime Ripard, Rob Herring, Chen-Yu Tsai,
Thierry Reding, Pengutronix Kernel Team, linux-arm-kernel
On Mon, 4 Nov 2019 at 09:04, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> On Sun, Nov 03, 2019 at 09:33:28PM +0100, Clément Péron wrote:
> > From: Jernej Skrabec <jernej.skrabec@siol.net>
> >
> > H6 PWM block is basically the same as A20 PWM, except that it also has
> > bus clock and reset line which needs to be handled accordingly.
> >
> > Expand Allwinner PWM binding with H6 PWM specifics.
> >
> > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> > Signed-off-by: Clément Péron <peron.clem@gmail.com>
> > ---
> > .../bindings/pwm/allwinner,sun4i-a10-pwm.yaml | 45 ++++++++++++++++++-
> > 1 file changed, 44 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/devicetree/bindings/pwm/allwinner,sun4i-a10-pwm.yaml b/Documentation/devicetree/bindings/pwm/allwinner,sun4i-a10-pwm.yaml
> > index 0ac52f83a58c..bf36ea509f31 100644
> > --- a/Documentation/devicetree/bindings/pwm/allwinner,sun4i-a10-pwm.yaml
> > +++ b/Documentation/devicetree/bindings/pwm/allwinner,sun4i-a10-pwm.yaml
> > @@ -30,13 +30,46 @@ properties:
> > - items:
> > - const: allwinner,sun50i-h5-pwm
> > - const: allwinner,sun5i-a13-pwm
> > + - const: allwinner,sun50i-h6-pwm
> >
> > reg:
> > maxItems: 1
> >
> > - clocks:
> > + # Even though it only applies to subschemas under the conditionals,
> > + # not listing them here will trigger a warning because of the
> > + # additionalsProperties set to false.
> > + clocks: true
> > + clock-names: true
> > + resets:
> > maxItems: 1
> >
> > + if:
> > + properties:
> > + compatible:
> > + contains:
> > + const: allwinner,sun50i-h6-pwm
> > +
> > + then:
> > + properties:
> > + clocks:
> > + items:
> > + - description: Module Clock
> > + - description: Bus Clock
> > +
> > + clock-names:
> > + items:
> > + - const: mod
> > + - const: bus
> > +
> > + required:
> > + - clock-names
> > + - resets
> > +
> > + else:
> > + properties:
> > + clocks:
> > + maxItems: 1
> > +
>
> I guess this hunk says "If this is a allwinner,sun50i-h6-pwm, a mod and
> bus clock is required.", right?
Correct.
>
>
> I wonder if it is sensible to require a clock-names property in the else
> branch, too. This would make it obvious if the clock there corresponds
> to the "mod" or the "bus" clock on H6. (I guess it's "mod".)
This will also require to change example and all the current allwinner
device-tree that have a PWM declared.
Regards,
Clément
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-König |
> Industrial Linux Solutions | http://www.pengutronix.de/ |
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 2/7] pwm: sun4i: Add an optional probe for reset line
2019-11-04 8:11 ` Uwe Kleine-König
@ 2019-11-04 17:50 ` Clément Péron
2019-11-05 7:01 ` Philipp Zabel
1 sibling, 0 replies; 33+ messages in thread
From: Clément Péron @ 2019-11-04 17:50 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Mark Rutland, linux-pwm, Jernej Skrabec, devicetree,
linux-kernel, Maxime Ripard, Rob Herring, Chen-Yu Tsai,
Thierry Reding, Philipp Zabel, linux-arm-kernel
On Mon, 4 Nov 2019 at 09:11, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Hello,
>
> adding Philipp Zabel (= reset controller maintainer) to Cc: and so I'm
> not stripping the uncommented parts of the patch.
>
> On Sun, Nov 03, 2019 at 09:33:29PM +0100, Clément Péron wrote:
> > From: Jernej Skrabec <jernej.skrabec@siol.net>
> >
> > H6 PWM core needs deasserted reset line in order to work.
> >
> > Add an optional probe for it.
> >
> > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> > Signed-off-by: Clément Péron <peron.clem@gmail.com>
> > ---
> > drivers/pwm/pwm-sun4i.c | 32 ++++++++++++++++++++++++++++++--
> > 1 file changed, 30 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> > index 6f5840a1a82d..d194b8ebdb00 100644
> > --- a/drivers/pwm/pwm-sun4i.c
> > +++ b/drivers/pwm/pwm-sun4i.c
> > @@ -16,6 +16,7 @@
> > #include <linux/of_device.h>
> > #include <linux/platform_device.h>
> > #include <linux/pwm.h>
> > +#include <linux/reset.h>
> > #include <linux/slab.h>
> > #include <linux/spinlock.h>
> > #include <linux/time.h>
> > @@ -78,6 +79,7 @@ struct sun4i_pwm_data {
> > struct sun4i_pwm_chip {
> > struct pwm_chip chip;
> > struct clk *clk;
> > + struct reset_control *rst;
> > void __iomem *base;
> > spinlock_t ctrl_lock;
> > const struct sun4i_pwm_data *data;
> > @@ -365,6 +367,20 @@ static int sun4i_pwm_probe(struct platform_device *pdev)
> > if (IS_ERR(pwm->clk))
> > return PTR_ERR(pwm->clk);
> >
> > + pwm->rst = devm_reset_control_get_optional(&pdev->dev, NULL);
> > + if (IS_ERR(pwm->rst)) {
> > + if (PTR_ERR(pwm->rst) == -EPROBE_DEFER)
> > + return PTR_ERR(pwm->rst);
> > + dev_info(&pdev->dev, "no reset control found\n");
>
> I would degrade this to a dev_dbg. Otherwise this spams the log for all
> unaffected machines. devm_reset_control_get_optional() is defined in a
> section that has a comment "These inline function calls will be removed
> once all consumers have been moved over to the new explicit API.", so I
> guess you want devm_reset_control_get_optional_exclusive or even
> devm_reset_control_get_optional_shared here.
Thanks for pointing this, I will change it.
>
> @Philipp: maybe a check in checkpatch that warns about introduction of
> such new instances would be good?!
>
> > + }
> > +
> > + /* Deassert reset */
> > + ret = reset_control_deassert(pwm->rst);
> > + if (ret) {
> > + dev_err(&pdev->dev, "Cannot deassert reset control\n");
> > + return ret;
> > + }
> > +
> > pwm->chip.dev = &pdev->dev;
> > pwm->chip.ops = &sun4i_pwm_ops;
> > pwm->chip.base = -1;
> > @@ -377,19 +393,31 @@ static int sun4i_pwm_probe(struct platform_device *pdev)
> > ret = pwmchip_add(&pwm->chip);
> > if (ret < 0) {
> > dev_err(&pdev->dev, "failed to add PWM chip: %d\n", ret);
> > - return ret;
> > + goto err_pwm_add;
> > }
> >
> > platform_set_drvdata(pdev, pwm);
> >
> > return 0;
> > +
> > +err_pwm_add:
> > + reset_control_assert(pwm->rst);
> > +
> > + return ret;
> > }
> >
> > static int sun4i_pwm_remove(struct platform_device *pdev)
> > {
> > struct sun4i_pwm_chip *pwm = platform_get_drvdata(pdev);
> > + int ret;
> > +
> > + ret = pwmchip_remove(&pwm->chip);
> > + if (ret)
> > + return ret;
> > +
> > + reset_control_assert(pwm->rst);
> >
> > - return pwmchip_remove(&pwm->chip);
> > + return 0;
> > }
> >
> > static struct platform_driver sun4i_pwm_driver = {
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-König |
> Industrial Linux Solutions | http://www.pengutronix.de/ |
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 3/7] pwm: sun4i: Add an optional probe for bus clock
2019-11-04 8:24 ` Uwe Kleine-König
@ 2019-11-04 18:07 ` Clément Péron
2019-11-04 20:10 ` Uwe Kleine-König
0 siblings, 1 reply; 33+ messages in thread
From: Clément Péron @ 2019-11-04 18:07 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Mark Rutland, linux-pwm, Jernej Skrabec, devicetree,
linux-kernel, Maxime Ripard, Rob Herring, Chen-Yu Tsai,
Thierry Reding, Pengutronix Kernel Team, linux-arm-kernel
Hi,
On Mon, 4 Nov 2019 at 09:24, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Hello,
>
> On Sun, Nov 03, 2019 at 09:33:30PM +0100, Clément Péron wrote:
> > From: Jernej Skrabec <jernej.skrabec@siol.net>
> >
> > H6 PWM core needs bus clock to be enabled in order to work.
> >
> > Add an optional probe for it and a fallback for previous
> > bindings without name on module clock.
> >
> > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> > Signed-off-by: Clément Péron <peron.clem@gmail.com>
> > ---
> > drivers/pwm/pwm-sun4i.c | 36 ++++++++++++++++++++++++++++++++++++
> > 1 file changed, 36 insertions(+)
> >
> > diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> > index d194b8ebdb00..b5e7ac364f59 100644
> > --- a/drivers/pwm/pwm-sun4i.c
> > +++ b/drivers/pwm/pwm-sun4i.c
> > @@ -78,6 +78,7 @@ struct sun4i_pwm_data {
> >
> > struct sun4i_pwm_chip {
> > struct pwm_chip chip;
> > + struct clk *bus_clk;
> > struct clk *clk;
> > struct reset_control *rst;
> > void __iomem *base;
> > @@ -367,6 +368,31 @@ static int sun4i_pwm_probe(struct platform_device *pdev)
>
> Adding more context here:
>
> | pwm->clk = devm_clk_get(&pdev->dev, NULL);
> > if (IS_ERR(pwm->clk))
> > return PTR_ERR(pwm->clk);
> >
> > + /* Get all clocks and reset line */
> > + pwm->clk = devm_clk_get_optional(&pdev->dev, "mod");
> > + if (IS_ERR(pwm->clk)) {
> > + dev_err(&pdev->dev, "get clock failed %ld\n",
> > + PTR_ERR(pwm->clk));
> > + return PTR_ERR(pwm->clk);
> > + }
>
> I guess you want to drop the first assignment to pwm->clk.
devm_clk_get_optional will return NULL if there is no entry, I don't
get where I need to drop it assignment.
>
> > + /* Fallback for old dtbs with a single clock and no name */
> > + if (!pwm->clk) {
> > + pwm->clk = devm_clk_get(&pdev->dev, NULL);
> > + if (IS_ERR(pwm->clk)) {
> > + dev_err(&pdev->dev, "get clock failed %ld\n",
> > + PTR_ERR(pwm->clk));
> > + return PTR_ERR(pwm->clk);
> > + }
> > + }
>
> There is a slight change of behaviour if I'm not mistaken. If you have
> this:
>
> clocks = <&clk1>;
> clock-names = "mod";
>
> pwm {
> compatible = "allwinner,sun4i-a10-pwm"
> clocks = <&clk2>;
> }
>
> you now use clk1 instead of clk2 before.
>
> Assuming this is only a theoretical problem, at least pointing this out
> in the commit log would be good I think.
Yes it's correct and as you said the driver don't check for a correct
device tree,
that why it's now optional probe.
Let's assume that's the device-tree is correct, I will add a comment
in the commit log.
>
> > + pwm->bus_clk = devm_clk_get_optional(&pdev->dev, "bus");
> > + if (IS_ERR(pwm->bus_clk)) {
> > + dev_err(&pdev->dev, "get bus_clock failed %ld\n",
> > + PTR_ERR(pwm->bus_clk));
> > + return PTR_ERR(pwm->bus_clk);
> > + }
> > +
> > pwm->rst = devm_reset_control_get_optional(&pdev->dev, NULL);
> > if (IS_ERR(pwm->rst)) {
> > if (PTR_ERR(pwm->rst) == -EPROBE_DEFER)
> > @@ -381,6 +407,13 @@ static int sun4i_pwm_probe(struct platform_device *pdev)
> > return ret;
> > }
> >
> > + /* Enable bus clock */
> > + ret = clk_prepare_enable(pwm->bus_clk);
> > + if (ret) {
> > + dev_err(&pdev->dev, "Cannot prepare_enable bus_clk\n");
>
> I'd do s/prepare_enable/prepare and enable/ here.
Ok
>
> > + goto err_bus;
> > + }
> > +
> > pwm->chip.dev = &pdev->dev;
> > pwm->chip.ops = &sun4i_pwm_ops;
> > pwm->chip.base = -1;
> > @@ -401,6 +434,8 @@ static int sun4i_pwm_probe(struct platform_device *pdev)
> > return 0;
> >
> > err_pwm_add:
> > + clk_disable_unprepare(pwm->bus_clk);
> > +err_bus:
> > reset_control_assert(pwm->rst);
> >
> > return ret;
>
> What is that clock used for? Is it required to access the hardware
> registers? Or is it only required while the PWM is enabled? If so you
> could enable the clock more finegrainded.
Regarding the datasheet it's required to access the hardware.
page 261 : https://linux-sunxi.org/File:Allwinner_H6_V200_User_Manual_V1.1.pdf
Regards,
Clément
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-König |
> Industrial Linux Solutions | http://www.pengutronix.de/ |
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 3/7] pwm: sun4i: Add an optional probe for bus clock
2019-11-04 18:07 ` Clément Péron
@ 2019-11-04 20:10 ` Uwe Kleine-König
2019-11-04 20:19 ` Jernej Škrabec
0 siblings, 1 reply; 33+ messages in thread
From: Uwe Kleine-König @ 2019-11-04 20:10 UTC (permalink / raw)
To: Clément Péron
Cc: Mark Rutland, linux-pwm, Jernej Skrabec, devicetree,
linux-kernel, Maxime Ripard, Rob Herring, Chen-Yu Tsai,
Thierry Reding, Pengutronix Kernel Team, linux-arm-kernel
Hello Clément,
On Mon, Nov 04, 2019 at 07:07:00PM +0100, Clément Péron wrote:
> On Mon, 4 Nov 2019 at 09:24, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > On Sun, Nov 03, 2019 at 09:33:30PM +0100, Clément Péron wrote:
> > > From: Jernej Skrabec <jernej.skrabec@siol.net>
> > >
> > > H6 PWM core needs bus clock to be enabled in order to work.
> > >
> > > Add an optional probe for it and a fallback for previous
> > > bindings without name on module clock.
> > >
> > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> > > Signed-off-by: Clément Péron <peron.clem@gmail.com>
> > > ---
> > > drivers/pwm/pwm-sun4i.c | 36 ++++++++++++++++++++++++++++++++++++
> > > 1 file changed, 36 insertions(+)
> > >
> > > diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> > > index d194b8ebdb00..b5e7ac364f59 100644
> > > --- a/drivers/pwm/pwm-sun4i.c
> > > +++ b/drivers/pwm/pwm-sun4i.c
> > > @@ -78,6 +78,7 @@ struct sun4i_pwm_data {
> > >
> > > struct sun4i_pwm_chip {
> > > struct pwm_chip chip;
> > > + struct clk *bus_clk;
> > > struct clk *clk;
> > > struct reset_control *rst;
> > > void __iomem *base;
> > > @@ -367,6 +368,31 @@ static int sun4i_pwm_probe(struct platform_device *pdev)
> >
> > Adding more context here:
> >
> > | pwm->clk = devm_clk_get(&pdev->dev, NULL);
> > > if (IS_ERR(pwm->clk))
> > > return PTR_ERR(pwm->clk);
> > >
> > > + /* Get all clocks and reset line */
> > > + pwm->clk = devm_clk_get_optional(&pdev->dev, "mod");
> > > + if (IS_ERR(pwm->clk)) {
> > > + dev_err(&pdev->dev, "get clock failed %ld\n",
> > > + PTR_ERR(pwm->clk));
> > > + return PTR_ERR(pwm->clk);
> > > + }
> >
> > I guess you want to drop the first assignment to pwm->clk.
>
> devm_clk_get_optional will return NULL if there is no entry, I don't
> get where I need to drop it assignment.
With your patch the code looks as follows:
pwm->clk = devm_clk_get(&pdev->dev, NULL);
if (IS_ERR(pwm->clk))
return PTR_ERR(pwm->clk);
/* Get all clocks and reset line */
pwm->clk = devm_clk_get_optional(&pdev->dev, "mod");
...
The assignment to pwm->clk above the comment is the one I suggested to
drop.
> > > + /* Fallback for old dtbs with a single clock and no name */
> > > + if (!pwm->clk) {
> > > + pwm->clk = devm_clk_get(&pdev->dev, NULL);
> > > + if (IS_ERR(pwm->clk)) {
> > > + dev_err(&pdev->dev, "get clock failed %ld\n",
> > > + PTR_ERR(pwm->clk));
> > > + return PTR_ERR(pwm->clk);
> > > + }
> > > + }
> >
> > There is a slight change of behaviour if I'm not mistaken. If you have
> > this:
> >
> > clocks = <&clk1>;
> > clock-names = "mod";
> >
> > pwm {
> > compatible = "allwinner,sun4i-a10-pwm"
> > clocks = <&clk2>;
> > }
> >
> > you now use clk1 instead of clk2 before.
> >
> > Assuming this is only a theoretical problem, at least pointing this out
> > in the commit log would be good I think.
>
> Yes it's correct and as you said the driver don't check for a correct
> device tree, that why it's now optional probe.
> Let's assume that's the device-tree is correct, I will add a comment
> in the commit log.
If the mod clock was shared by all peripherals on the bus this would be
IMHO quite elegant. Probably it depends on what you mean by saying
"incorrect" if this snippet is incorrect. (It can be part of a valid dtb
that even complies to the binding documentation. However that's not how
any existing allwinner hardware looks like.) But let's stop arguing as
we agree it's a corner case and if you mention it in the commit log
we're both happy.
> > What is that clock used for? Is it required to access the hardware
> > registers? Or is it only required while the PWM is enabled? If so you
> > could enable the clock more finegrainded.
>
> Regarding the datasheet it's required to access the hardware.
> page 261 : https://linux-sunxi.org/File:Allwinner_H6_V200_User_Manual_V1.1.pdf
So enabling the bus clock is called "open APB1 Bus gating" in that
manual? If I understand that correctly the bus clock then only need to
be on while accessing the registers and could be disabled once the
hardware is programmed and running.
Can you please describe that in a comment. Something like:
/*
* We're keeping the bus clock on for the sake of simplicity.
* Actually it only needs to be on for hardware register
* accesses.
*/
should be fine. This way it's at least obvious that the handling could
be improved.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 3/7] pwm: sun4i: Add an optional probe for bus clock
2019-11-04 20:10 ` Uwe Kleine-König
@ 2019-11-04 20:19 ` Jernej Škrabec
2019-11-04 20:27 ` Clément Péron
0 siblings, 1 reply; 33+ messages in thread
From: Jernej Škrabec @ 2019-11-04 20:19 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Mark Rutland, linux-pwm, devicetree, linux-kernel, Maxime Ripard,
Rob Herring, Chen-Yu Tsai, Thierry Reding,
Clément Péron, Pengutronix Kernel Team,
linux-arm-kernel
Dne ponedeljek, 04. november 2019 ob 21:10:52 CET je Uwe Kleine-König
napisal(a):
> Hello Clément,
>
> On Mon, Nov 04, 2019 at 07:07:00PM +0100, Clément Péron wrote:
> > On Mon, 4 Nov 2019 at 09:24, Uwe Kleine-König
> >
> > <u.kleine-koenig@pengutronix.de> wrote:
> > > On Sun, Nov 03, 2019 at 09:33:30PM +0100, Clément Péron wrote:
> > > > From: Jernej Skrabec <jernej.skrabec@siol.net>
> > > >
> > > > H6 PWM core needs bus clock to be enabled in order to work.
> > > >
> > > > Add an optional probe for it and a fallback for previous
> > > > bindings without name on module clock.
> > > >
> > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> > > > Signed-off-by: Clément Péron <peron.clem@gmail.com>
> > > > ---
> > > >
> > > > drivers/pwm/pwm-sun4i.c | 36 ++++++++++++++++++++++++++++++++++++
> > > > 1 file changed, 36 insertions(+)
> > > >
> > > > diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> > > > index d194b8ebdb00..b5e7ac364f59 100644
> > > > --- a/drivers/pwm/pwm-sun4i.c
> > > > +++ b/drivers/pwm/pwm-sun4i.c
> > > > @@ -78,6 +78,7 @@ struct sun4i_pwm_data {
> > > >
> > > > struct sun4i_pwm_chip {
> > > >
> > > > struct pwm_chip chip;
> > > >
> > > > + struct clk *bus_clk;
> > > >
> > > > struct clk *clk;
> > > > struct reset_control *rst;
> > > > void __iomem *base;
> > > >
> > > > @@ -367,6 +368,31 @@ static int sun4i_pwm_probe(struct platform_device
> > > > *pdev)> >
> > > Adding more context here:
> > > | pwm->clk = devm_clk_get(&pdev->dev, NULL);
> > > |
> > > > if (IS_ERR(pwm->clk))
> > > >
> > > > return PTR_ERR(pwm->clk);
> > > >
> > > > + /* Get all clocks and reset line */
> > > > + pwm->clk = devm_clk_get_optional(&pdev->dev, "mod");
> > > > + if (IS_ERR(pwm->clk)) {
> > > > + dev_err(&pdev->dev, "get clock failed %ld\n",
> > > > + PTR_ERR(pwm->clk));
> > > > + return PTR_ERR(pwm->clk);
> > > > + }
> > >
> > > I guess you want to drop the first assignment to pwm->clk.
> >
> > devm_clk_get_optional will return NULL if there is no entry, I don't
> > get where I need to drop it assignment.
>
> With your patch the code looks as follows:
>
> pwm->clk = devm_clk_get(&pdev->dev, NULL);
> if (IS_ERR(pwm->clk))
> return PTR_ERR(pwm->clk);
>
> /* Get all clocks and reset line */
> pwm->clk = devm_clk_get_optional(&pdev->dev, "mod");
Actually, it's the other way around, e.g. "mod" clock is checked first.
> ...
>
> The assignment to pwm->clk above the comment is the one I suggested to
> drop.
Neither can be dropped. DT files for other SoCs don't have clock-names
property, so search for "mod" clock will fail and then fallback option without
name is used.
Best regards,
Jernej
>
> > > > + /* Fallback for old dtbs with a single clock and no name */
> > > > + if (!pwm->clk) {
> > > > + pwm->clk = devm_clk_get(&pdev->dev, NULL);
> > > > + if (IS_ERR(pwm->clk)) {
> > > > + dev_err(&pdev->dev, "get clock failed %ld\n",
> > > > + PTR_ERR(pwm->clk));
> > > > + return PTR_ERR(pwm->clk);
> > > > + }
> > > > + }
> > >
> > > There is a slight change of behaviour if I'm not mistaken. If you have
> > >
> > > this:
> > > clocks = <&clk1>;
> > > clock-names = "mod";
> > >
> > > pwm {
> > >
> > > compatible = "allwinner,sun4i-a10-pwm"
> > > clocks = <&clk2>;
> > >
> > > }
> > >
> > > you now use clk1 instead of clk2 before.
> > >
> > > Assuming this is only a theoretical problem, at least pointing this out
> > > in the commit log would be good I think.
> >
> > Yes it's correct and as you said the driver don't check for a correct
> > device tree, that why it's now optional probe.
> > Let's assume that's the device-tree is correct, I will add a comment
> > in the commit log.
>
> If the mod clock was shared by all peripherals on the bus this would be
> IMHO quite elegant. Probably it depends on what you mean by saying
> "incorrect" if this snippet is incorrect. (It can be part of a valid dtb
> that even complies to the binding documentation. However that's not how
> any existing allwinner hardware looks like.) But let's stop arguing as
> we agree it's a corner case and if you mention it in the commit log
> we're both happy.
>
> > > What is that clock used for? Is it required to access the hardware
> > > registers? Or is it only required while the PWM is enabled? If so you
> > > could enable the clock more finegrainded.
> >
> > Regarding the datasheet it's required to access the hardware.
> > page 261 :
> > https://linux-sunxi.org/File:Allwinner_H6_V200_User_Manual_V1.1.pdf
> So enabling the bus clock is called "open APB1 Bus gating" in that
> manual? If I understand that correctly the bus clock then only need to
> be on while accessing the registers and could be disabled once the
> hardware is programmed and running.
>
> Can you please describe that in a comment. Something like:
>
> /*
> * We're keeping the bus clock on for the sake of simplicity.
> * Actually it only needs to be on for hardware register
> * accesses.
> */
>
> should be fine. This way it's at least obvious that the handling could
> be improved.
>
> Best regards
> Uwe
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 3/7] pwm: sun4i: Add an optional probe for bus clock
2019-11-04 20:19 ` Jernej Škrabec
@ 2019-11-04 20:27 ` Clément Péron
2019-11-04 20:38 ` Jernej Škrabec
0 siblings, 1 reply; 33+ messages in thread
From: Clément Péron @ 2019-11-04 20:27 UTC (permalink / raw)
To: Jernej Škrabec
Cc: Mark Rutland, linux-pwm, devicetree, linux-kernel, Maxime Ripard,
Rob Herring, Chen-Yu Tsai, Thierry Reding,
Pengutronix Kernel Team, Uwe Kleine-König, linux-arm-kernel
Hi,
On Mon, 4 Nov 2019 at 21:19, Jernej Škrabec <jernej.skrabec@siol.net> wrote:
>
> Dne ponedeljek, 04. november 2019 ob 21:10:52 CET je Uwe Kleine-König
> napisal(a):
> > Hello Clément,
> >
> > On Mon, Nov 04, 2019 at 07:07:00PM +0100, Clément Péron wrote:
> > > On Mon, 4 Nov 2019 at 09:24, Uwe Kleine-König
> > >
> > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > On Sun, Nov 03, 2019 at 09:33:30PM +0100, Clément Péron wrote:
> > > > > From: Jernej Skrabec <jernej.skrabec@siol.net>
> > > > >
> > > > > H6 PWM core needs bus clock to be enabled in order to work.
> > > > >
> > > > > Add an optional probe for it and a fallback for previous
> > > > > bindings without name on module clock.
> > > > >
> > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> > > > > Signed-off-by: Clément Péron <peron.clem@gmail.com>
> > > > > ---
> > > > >
> > > > > drivers/pwm/pwm-sun4i.c | 36 ++++++++++++++++++++++++++++++++++++
> > > > > 1 file changed, 36 insertions(+)
> > > > >
> > > > > diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> > > > > index d194b8ebdb00..b5e7ac364f59 100644
> > > > > --- a/drivers/pwm/pwm-sun4i.c
> > > > > +++ b/drivers/pwm/pwm-sun4i.c
> > > > > @@ -78,6 +78,7 @@ struct sun4i_pwm_data {
> > > > >
> > > > > struct sun4i_pwm_chip {
> > > > >
> > > > > struct pwm_chip chip;
> > > > >
> > > > > + struct clk *bus_clk;
> > > > >
> > > > > struct clk *clk;
> > > > > struct reset_control *rst;
> > > > > void __iomem *base;
> > > > >
> > > > > @@ -367,6 +368,31 @@ static int sun4i_pwm_probe(struct platform_device
> > > > > *pdev)> >
> > > > Adding more context here:
> > > > | pwm->clk = devm_clk_get(&pdev->dev, NULL);
> > > > |
> > > > > if (IS_ERR(pwm->clk))
> > > > >
> > > > > return PTR_ERR(pwm->clk);
> > > > >
> > > > > + /* Get all clocks and reset line */
> > > > > + pwm->clk = devm_clk_get_optional(&pdev->dev, "mod");
> > > > > + if (IS_ERR(pwm->clk)) {
> > > > > + dev_err(&pdev->dev, "get clock failed %ld\n",
> > > > > + PTR_ERR(pwm->clk));
> > > > > + return PTR_ERR(pwm->clk);
> > > > > + }
> > > >
> > > > I guess you want to drop the first assignment to pwm->clk.
> > >
> > > devm_clk_get_optional will return NULL if there is no entry, I don't
> > > get where I need to drop it assignment.
> >
> > With your patch the code looks as follows:
> >
> > pwm->clk = devm_clk_get(&pdev->dev, NULL);
> > if (IS_ERR(pwm->clk))
> > return PTR_ERR(pwm->clk);
> >
> > /* Get all clocks and reset line */
> > pwm->clk = devm_clk_get_optional(&pdev->dev, "mod");
>
> Actually, it's the other way around, e.g. "mod" clock is checked first.
The first devm_clk_get is indeed wrong, I will remove it!
>
> > ...
> >
> > The assignment to pwm->clk above the comment is the one I suggested to
> > drop.
>
> Neither can be dropped. DT files for other SoCs don't have clock-names
> property, so search for "mod" clock will fail and then fallback option without
> name is used.
>
> Best regards,
> Jernej
>
> >
> > > > > + /* Fallback for old dtbs with a single clock and no name */
> > > > > + if (!pwm->clk) {
> > > > > + pwm->clk = devm_clk_get(&pdev->dev, NULL);
> > > > > + if (IS_ERR(pwm->clk)) {
> > > > > + dev_err(&pdev->dev, "get clock failed %ld\n",
> > > > > + PTR_ERR(pwm->clk));
> > > > > + return PTR_ERR(pwm->clk);
> > > > > + }
> > > > > + }
> > > >
> > > > There is a slight change of behaviour if I'm not mistaken. If you have
> > > >
> > > > this:
> > > > clocks = <&clk1>;
> > > > clock-names = "mod";
> > > >
> > > > pwm {
> > > >
> > > > compatible = "allwinner,sun4i-a10-pwm"
> > > > clocks = <&clk2>;
> > > >
> > > > }
> > > >
> > > > you now use clk1 instead of clk2 before.
> > > >
> > > > Assuming this is only a theoretical problem, at least pointing this out
> > > > in the commit log would be good I think.
> > >
> > > Yes it's correct and as you said the driver don't check for a correct
> > > device tree, that why it's now optional probe.
> > > Let's assume that's the device-tree is correct, I will add a comment
> > > in the commit log.
> >
> > If the mod clock was shared by all peripherals on the bus this would be
> > IMHO quite elegant. Probably it depends on what you mean by saying
> > "incorrect" if this snippet is incorrect. (It can be part of a valid dtb
> > that even complies to the binding documentation. However that's not how
> > any existing allwinner hardware looks like.) But let's stop arguing as
> > we agree it's a corner case and if you mention it in the commit log
> > we're both happy.
> >
> > > > What is that clock used for? Is it required to access the hardware
> > > > registers? Or is it only required while the PWM is enabled? If so you
> > > > could enable the clock more finegrainded.
> > >
> > > Regarding the datasheet it's required to access the hardware.
> > > page 261 :
> > > https://linux-sunxi.org/File:Allwinner_H6_V200_User_Manual_V1.1.pdf
> > So enabling the bus clock is called "open APB1 Bus gating" in that
> > manual? If I understand that correctly the bus clock then only need to
> > be on while accessing the registers and could be disabled once the
> > hardware is programmed and running.
> >
> > Can you please describe that in a comment. Something like:
> >
> > /*
> > * We're keeping the bus clock on for the sake of simplicity.
> > * Actually it only needs to be on for hardware register
> > * accesses.
> > */
> >
> > should be fine. This way it's at least obvious that the handling could
> > be improved.
> >
> > Best regards
> > Uwe
>
>
>
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 3/7] pwm: sun4i: Add an optional probe for bus clock
2019-11-04 20:27 ` Clément Péron
@ 2019-11-04 20:38 ` Jernej Škrabec
0 siblings, 0 replies; 33+ messages in thread
From: Jernej Škrabec @ 2019-11-04 20:38 UTC (permalink / raw)
To: Clément Péron
Cc: Mark Rutland, linux-pwm, devicetree, linux-kernel, Maxime Ripard,
Rob Herring, Chen-Yu Tsai, Thierry Reding,
Pengutronix Kernel Team, Uwe Kleine-König, linux-arm-kernel
Dne ponedeljek, 04. november 2019 ob 21:27:04 CET je Clément Péron napisal(a):
> Hi,
>
> On Mon, 4 Nov 2019 at 21:19, Jernej Škrabec <jernej.skrabec@siol.net> wrote:
> > Dne ponedeljek, 04. november 2019 ob 21:10:52 CET je Uwe Kleine-König
> >
> > napisal(a):
> > > Hello Clément,
> > >
> > > On Mon, Nov 04, 2019 at 07:07:00PM +0100, Clément Péron wrote:
> > > > On Mon, 4 Nov 2019 at 09:24, Uwe Kleine-König
> > > >
> > > > <u.kleine-koenig@pengutronix.de> wrote:
> > > > > On Sun, Nov 03, 2019 at 09:33:30PM +0100, Clément Péron wrote:
> > > > > > From: Jernej Skrabec <jernej.skrabec@siol.net>
> > > > > >
> > > > > > H6 PWM core needs bus clock to be enabled in order to work.
> > > > > >
> > > > > > Add an optional probe for it and a fallback for previous
> > > > > > bindings without name on module clock.
> > > > > >
> > > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> > > > > > Signed-off-by: Clément Péron <peron.clem@gmail.com>
> > > > > > ---
> > > > > >
> > > > > > drivers/pwm/pwm-sun4i.c | 36 ++++++++++++++++++++++++++++++++++++
> > > > > > 1 file changed, 36 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> > > > > > index d194b8ebdb00..b5e7ac364f59 100644
> > > > > > --- a/drivers/pwm/pwm-sun4i.c
> > > > > > +++ b/drivers/pwm/pwm-sun4i.c
> > > > > > @@ -78,6 +78,7 @@ struct sun4i_pwm_data {
> > > > > >
> > > > > > struct sun4i_pwm_chip {
> > > > > >
> > > > > > struct pwm_chip chip;
> > > > > >
> > > > > > + struct clk *bus_clk;
> > > > > >
> > > > > > struct clk *clk;
> > > > > > struct reset_control *rst;
> > > > > > void __iomem *base;
> > > > > >
> > > > > > @@ -367,6 +368,31 @@ static int sun4i_pwm_probe(struct
> > > > > > platform_device
> > > > > > *pdev)> >
> > > > >
> > > > > Adding more context here:
> > > > > | pwm->clk = devm_clk_get(&pdev->dev, NULL);
> > > > > |
> > > > > > if (IS_ERR(pwm->clk))
> > > > > >
> > > > > > return PTR_ERR(pwm->clk);
> > > > > >
> > > > > > + /* Get all clocks and reset line */
> > > > > > + pwm->clk = devm_clk_get_optional(&pdev->dev, "mod");
> > > > > > + if (IS_ERR(pwm->clk)) {
> > > > > > + dev_err(&pdev->dev, "get clock failed %ld\n",
> > > > > > + PTR_ERR(pwm->clk));
> > > > > > + return PTR_ERR(pwm->clk);
> > > > > > + }
> > > > >
> > > > > I guess you want to drop the first assignment to pwm->clk.
> > > >
> > > > devm_clk_get_optional will return NULL if there is no entry, I don't
> > > > get where I need to drop it assignment.
> > >
> > > With your patch the code looks as follows:
> > > pwm->clk = devm_clk_get(&pdev->dev, NULL);
> > > if (IS_ERR(pwm->clk))
> > >
> > > return PTR_ERR(pwm->clk);
> > >
> > > /* Get all clocks and reset line */
> > > pwm->clk = devm_clk_get_optional(&pdev->dev, "mod");
> >
> > Actually, it's the other way around, e.g. "mod" clock is checked first.
>
> The first devm_clk_get is indeed wrong, I will remove it!
Sorry, I missed that too. Yeah, it should be removed.
Best regards,
Jernej
>
> > > ...
> > >
> > > The assignment to pwm->clk above the comment is the one I suggested to
> > > drop.
> >
> > Neither can be dropped. DT files for other SoCs don't have clock-names
> > property, so search for "mod" clock will fail and then fallback option
> > without name is used.
> >
> > Best regards,
> > Jernej
> >
> > > > > > + /* Fallback for old dtbs with a single clock and no name */
> > > > > > + if (!pwm->clk) {
> > > > > > + pwm->clk = devm_clk_get(&pdev->dev, NULL);
> > > > > > + if (IS_ERR(pwm->clk)) {
> > > > > > + dev_err(&pdev->dev, "get clock failed
> > > > > > %ld\n",
> > > > > > + PTR_ERR(pwm->clk));
> > > > > > + return PTR_ERR(pwm->clk);
> > > > > > + }
> > > > > > + }
> > > > >
> > > > > There is a slight change of behaviour if I'm not mistaken. If you
> > > > > have
> > > > >
> > > > > this:
> > > > > clocks = <&clk1>;
> > > > > clock-names = "mod";
> > > > >
> > > > > pwm {
> > > > >
> > > > > compatible = "allwinner,sun4i-a10-pwm"
> > > > > clocks = <&clk2>;
> > > > >
> > > > > }
> > > > >
> > > > > you now use clk1 instead of clk2 before.
> > > > >
> > > > > Assuming this is only a theoretical problem, at least pointing this
> > > > > out
> > > > > in the commit log would be good I think.
> > > >
> > > > Yes it's correct and as you said the driver don't check for a correct
> > > > device tree, that why it's now optional probe.
> > > > Let's assume that's the device-tree is correct, I will add a comment
> > > > in the commit log.
> > >
> > > If the mod clock was shared by all peripherals on the bus this would be
> > > IMHO quite elegant. Probably it depends on what you mean by saying
> > > "incorrect" if this snippet is incorrect. (It can be part of a valid dtb
> > > that even complies to the binding documentation. However that's not how
> > > any existing allwinner hardware looks like.) But let's stop arguing as
> > > we agree it's a corner case and if you mention it in the commit log
> > > we're both happy.
> > >
> > > > > What is that clock used for? Is it required to access the hardware
> > > > > registers? Or is it only required while the PWM is enabled? If so
> > > > > you
> > > > > could enable the clock more finegrainded.
> > > >
> > > > Regarding the datasheet it's required to access the hardware.
> > > > page 261 :
> > > > https://linux-sunxi.org/File:Allwinner_H6_V200_User_Manual_V1.1.pdf
> > >
> > > So enabling the bus clock is called "open APB1 Bus gating" in that
> > > manual? If I understand that correctly the bus clock then only need to
> > > be on while accessing the registers and could be disabled once the
> > > hardware is programmed and running.
> > >
> > > Can you please describe that in a comment. Something like:
> > > /*
> > >
> > > * We're keeping the bus clock on for the sake of simplicity.
> > > * Actually it only needs to be on for hardware register
> > > * accesses.
> > > */
> > >
> > > should be fine. This way it's at least obvious that the handling could
> > > be improved.
> > >
> > > Best regards
> > > Uwe
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 4/7] pwm: sun4i: Add support to output source clock directly
2019-11-04 8:38 ` Uwe Kleine-König
@ 2019-11-04 21:28 ` Clément Péron
2019-11-05 7:29 ` Uwe Kleine-König
0 siblings, 1 reply; 33+ messages in thread
From: Clément Péron @ 2019-11-04 21:28 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Mark Rutland, linux-pwm, Jernej Skrabec, devicetree,
linux-kernel, Maxime Ripard, Rob Herring, Chen-Yu Tsai,
Thierry Reding, linux-arm-kernel
Hi Uwe
On Mon, 4 Nov 2019 at 09:38, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> On Sun, Nov 03, 2019 at 09:33:31PM +0100, Clément Péron wrote:
> > From: Jernej Skrabec <jernej.skrabec@siol.net>
> >
> > PWM core has an option to bypass whole logic and output unchanged source
> > clock as PWM output. This is achieved by enabling bypass bit.
> >
> > Note that when bypass is enabled, no other setting has any meaning, not
> > even enable bit.
> >
> > This mode of operation is needed to achieve high enough frequency to
> > serve as clock source for AC200 chip, which is integrated into same
> > package as H6 SoC.
>
> I think the , should be dropped.
>
> > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> > Signed-off-by: Clément Péron <peron.clem@gmail.com>
> > ---
> > drivers/pwm/pwm-sun4i.c | 39 ++++++++++++++++++++++++++++++++++++++-
> > 1 file changed, 38 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> > index b5e7ac364f59..2441574674d9 100644
> > --- a/drivers/pwm/pwm-sun4i.c
> > +++ b/drivers/pwm/pwm-sun4i.c
> > @@ -3,6 +3,10 @@
> > * Driver for Allwinner sun4i Pulse Width Modulation Controller
> > *
> > * Copyright (C) 2014 Alexandre Belloni <alexandre.belloni@free-electrons.com>
> > + *
> > + * Limitations:
> > + * - When outputing the source clock directly, the PWM logic will be bypassed
> > + * and the currently running period is not guaranted to be completed
>
> Typo: guaranted -> guaranteed
>
> > */
> >
> > #include <linux/bitops.h>
> > @@ -73,6 +77,7 @@ static const u32 prescaler_table[] = {
> >
> > struct sun4i_pwm_data {
> > bool has_prescaler_bypass;
> > + bool has_direct_mod_clk_output;
> > unsigned int npwm;
> > };
> >
> > @@ -118,6 +123,20 @@ static void sun4i_pwm_get_state(struct pwm_chip *chip,
> >
> > val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
> >
> > + /*
> > + * PWM chapter in H6 manual has a diagram which explains that if bypass
> > + * bit is set, no other setting has any meaning. Even more, experiment
> > + * proved that also enable bit is ignored in this case.
> > + */
> > + if ((val & BIT_CH(PWM_BYPASS, pwm->hwpwm)) &&
> > + data->has_direct_mod_clk_output) {
> > + state->period = DIV_ROUND_CLOSEST_ULL(NSEC_PER_SEC, clk_rate);
> > + state->duty_cycle = state->period / 2;
> > + state->polarity = PWM_POLARITY_NORMAL;
> > + state->enabled = true;
> > + return;
> > + }
>
> Not sure how the rest of sun4i_pwm_get_state behaves, but I would prefer
> to let .get_state() round up which together with .apply_state() rounding
> down yields sound behaviour.
Ok
>
> > +
> > if ((PWM_REG_PRESCAL(val, pwm->hwpwm) == PWM_PRESCAL_MASK) &&
> > sun4i_pwm->data->has_prescaler_bypass)
> > prescaler = 1;
> > @@ -203,7 +222,8 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > {
> > struct sun4i_pwm_chip *sun4i_pwm = to_sun4i_pwm_chip(chip);
> > struct pwm_state cstate;
> > - u32 ctrl;
> > + u32 ctrl, clk_rate;
> > + bool bypass;
> > int ret;
> > unsigned int delay_us;
> > unsigned long now;
> > @@ -218,6 +238,16 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > }
> > }
> >
> > + /*
> > + * Although it would make much more sense to check for bypass in
> > + * sun4i_pwm_calculate(), value of bypass bit also depends on "enabled".
> > + * Period is allowed to be rounded up or down.
> > + */
> > + clk_rate = clk_get_rate(sun4i_pwm->clk);
> > + bypass = ((state->period * clk_rate >= NSEC_PER_SEC &&
> > + state->period * clk_rate < NSEC_PER_SEC + clk_rate) &&
> > + state->enabled);
>
> I guess the compiler is smart enough here, but checking for
> state->enabled is cheaper than the other checks, so putting this at the
> start of the expression seems sensible.
>
> The comment doesn't match the code. You don't round up state->period.
> (This is good, please fix the comment.) I think dropping the check
>
> state->period * clk_rate < NSEC_PER_SEC + clk_rate
>
> would be fine, too.
Ok
>
> I'd like to have a check for
>
> state->duty_cycle * clk_rate >= NSEC_PER_SEC / 2 &&
> state->duty_cycle * clk_rate < NSEC_PER_SEC
>
> here. If this isn't true rather disable the PWM or output a 100% duty
> cycle with a larger period.
Why not just having the duty_cycle is 50% only ?
state->duty_cycle * 2 == state->period;
Regards,
Clement
>
> > +
> > spin_lock(&sun4i_pwm->ctrl_lock);
> > ctrl = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
> >
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-König |
> Industrial Linux Solutions | http://www.pengutronix.de/ |
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 2/7] pwm: sun4i: Add an optional probe for reset line
2019-11-04 8:11 ` Uwe Kleine-König
2019-11-04 17:50 ` Clément Péron
@ 2019-11-05 7:01 ` Philipp Zabel
2019-11-05 13:03 ` Clément Péron
1 sibling, 1 reply; 33+ messages in thread
From: Philipp Zabel @ 2019-11-05 7:01 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Mark Rutland, linux-pwm, Jernej Skrabec, devicetree,
linux-kernel, Maxime Ripard, Rob Herring, Chen-Yu Tsai,
Thierry Reding, Clément Péron, linux-arm-kernel
On Mon, Nov 04, 2019 at 09:11:57AM +0100, Uwe Kleine-König wrote:
> Hello,
>
> adding Philipp Zabel (= reset controller maintainer) to Cc: and so I'm
> not stripping the uncommented parts of the patch.
>
> On Sun, Nov 03, 2019 at 09:33:29PM +0100, Clément Péron wrote:
> > From: Jernej Skrabec <jernej.skrabec@siol.net>
> >
> > H6 PWM core needs deasserted reset line in order to work.
> >
> > Add an optional probe for it.
> >
> > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> > Signed-off-by: Clément Péron <peron.clem@gmail.com>
> > ---
> > drivers/pwm/pwm-sun4i.c | 32 ++++++++++++++++++++++++++++++--
> > 1 file changed, 30 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> > index 6f5840a1a82d..d194b8ebdb00 100644
> > --- a/drivers/pwm/pwm-sun4i.c
> > +++ b/drivers/pwm/pwm-sun4i.c
> > @@ -16,6 +16,7 @@
> > #include <linux/of_device.h>
> > #include <linux/platform_device.h>
> > #include <linux/pwm.h>
> > +#include <linux/reset.h>
> > #include <linux/slab.h>
> > #include <linux/spinlock.h>
> > #include <linux/time.h>
> > @@ -78,6 +79,7 @@ struct sun4i_pwm_data {
> > struct sun4i_pwm_chip {
> > struct pwm_chip chip;
> > struct clk *clk;
> > + struct reset_control *rst;
> > void __iomem *base;
> > spinlock_t ctrl_lock;
> > const struct sun4i_pwm_data *data;
> > @@ -365,6 +367,20 @@ static int sun4i_pwm_probe(struct platform_device *pdev)
> > if (IS_ERR(pwm->clk))
> > return PTR_ERR(pwm->clk);
> >
> > + pwm->rst = devm_reset_control_get_optional(&pdev->dev, NULL);
> > + if (IS_ERR(pwm->rst)) {
> > + if (PTR_ERR(pwm->rst) == -EPROBE_DEFER)
> > + return PTR_ERR(pwm->rst);
> > + dev_info(&pdev->dev, "no reset control found\n");
>
> I would degrade this to a dev_dbg. Otherwise this spams the log for all
> unaffected machines.
The _optional variants return NULL if the reset is not specified in the
device tree, so this is not "no reset control found", but a real error
that should be returned.
> devm_reset_control_get_optional() is defined in a section that has a
> comment "These inline function calls will be removed once all
> consumers have been moved over to the new explicit API.", so I guess
> you want devm_reset_control_get_optional_exclusive or even
> devm_reset_control_get_optional_shared here.
Correct. If this driver deasserts in probe() and asserts the reset in
remove(), this can use the refcounting _shared variant.
> @Philipp: maybe a check in checkpatch that warns about introduction of
> such new instances would be good?!
Yes, that would be helpful.
regards
Philipp
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 4/7] pwm: sun4i: Add support to output source clock directly
2019-11-04 21:28 ` Clément Péron
@ 2019-11-05 7:29 ` Uwe Kleine-König
2019-11-05 12:58 ` Clément Péron
0 siblings, 1 reply; 33+ messages in thread
From: Uwe Kleine-König @ 2019-11-05 7:29 UTC (permalink / raw)
To: Clément Péron
Cc: Mark Rutland, linux-pwm, Jernej Skrabec, devicetree,
linux-kernel, Maxime Ripard, Rob Herring, Chen-Yu Tsai,
Thierry Reding, linux-arm-kernel
Hi Clément,
On Mon, Nov 04, 2019 at 10:28:54PM +0100, Clément Péron wrote:
> On Mon, 4 Nov 2019 at 09:38, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > On Sun, Nov 03, 2019 at 09:33:31PM +0100, Clément Péron wrote:
> > > From: Jernej Skrabec <jernej.skrabec@siol.net>
> > >
> > > PWM core has an option to bypass whole logic and output unchanged source
> > > clock as PWM output. This is achieved by enabling bypass bit.
> > >
> > > Note that when bypass is enabled, no other setting has any meaning, not
> > > even enable bit.
> > >
> > > This mode of operation is needed to achieve high enough frequency to
> > > serve as clock source for AC200 chip, which is integrated into same
> > > package as H6 SoC.
> >
> > I think the , should be dropped.
> >
> > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> > > Signed-off-by: Clément Péron <peron.clem@gmail.com>
> > > ---
> > > drivers/pwm/pwm-sun4i.c | 39 ++++++++++++++++++++++++++++++++++++++-
> > > 1 file changed, 38 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> > > index b5e7ac364f59..2441574674d9 100644
> > > --- a/drivers/pwm/pwm-sun4i.c
> > > +++ b/drivers/pwm/pwm-sun4i.c
> > > @@ -3,6 +3,10 @@
> > > * Driver for Allwinner sun4i Pulse Width Modulation Controller
> > > *
> > > * Copyright (C) 2014 Alexandre Belloni <alexandre.belloni@free-electrons.com>
> > > + *
> > > + * Limitations:
> > > + * - When outputing the source clock directly, the PWM logic will be bypassed
> > > + * and the currently running period is not guaranted to be completed
> >
> > Typo: guaranted -> guaranteed
> >
> > > */
> > >
> > > #include <linux/bitops.h>
> > > @@ -73,6 +77,7 @@ static const u32 prescaler_table[] = {
> > >
> > > struct sun4i_pwm_data {
> > > bool has_prescaler_bypass;
> > > + bool has_direct_mod_clk_output;
> > > unsigned int npwm;
> > > };
> > >
> > > @@ -118,6 +123,20 @@ static void sun4i_pwm_get_state(struct pwm_chip *chip,
> > >
> > > val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
> > >
> > > + /*
> > > + * PWM chapter in H6 manual has a diagram which explains that if bypass
> > > + * bit is set, no other setting has any meaning. Even more, experiment
> > > + * proved that also enable bit is ignored in this case.
> > > + */
> > > + if ((val & BIT_CH(PWM_BYPASS, pwm->hwpwm)) &&
> > > + data->has_direct_mod_clk_output) {
> > > + state->period = DIV_ROUND_CLOSEST_ULL(NSEC_PER_SEC, clk_rate);
> > > + state->duty_cycle = state->period / 2;
> > > + state->polarity = PWM_POLARITY_NORMAL;
> > > + state->enabled = true;
> > > + return;
> > > + }
> >
> > Not sure how the rest of sun4i_pwm_get_state behaves, but I would prefer
> > to let .get_state() round up which together with .apply_state() rounding
> > down yields sound behaviour.
> Ok
> >
> > > +
> > > if ((PWM_REG_PRESCAL(val, pwm->hwpwm) == PWM_PRESCAL_MASK) &&
> > > sun4i_pwm->data->has_prescaler_bypass)
> > > prescaler = 1;
> > > @@ -203,7 +222,8 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > > {
> > > struct sun4i_pwm_chip *sun4i_pwm = to_sun4i_pwm_chip(chip);
> > > struct pwm_state cstate;
> > > - u32 ctrl;
> > > + u32 ctrl, clk_rate;
> > > + bool bypass;
> > > int ret;
> > > unsigned int delay_us;
> > > unsigned long now;
> > > @@ -218,6 +238,16 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > > }
> > > }
> > >
> > > + /*
> > > + * Although it would make much more sense to check for bypass in
> > > + * sun4i_pwm_calculate(), value of bypass bit also depends on "enabled".
> > > + * Period is allowed to be rounded up or down.
> > > + */
> > > + clk_rate = clk_get_rate(sun4i_pwm->clk);
> > > + bypass = ((state->period * clk_rate >= NSEC_PER_SEC &&
> > > + state->period * clk_rate < NSEC_PER_SEC + clk_rate) &&
> > > + state->enabled);
> >
> > I guess the compiler is smart enough here, but checking for
> > state->enabled is cheaper than the other checks, so putting this at the
> > start of the expression seems sensible.
> >
> > The comment doesn't match the code. You don't round up state->period.
> > (This is good, please fix the comment.) I think dropping the check
> >
> > state->period * clk_rate < NSEC_PER_SEC + clk_rate
> >
> > would be fine, too.
> Ok
>
> >
> > I'd like to have a check for
> >
> > state->duty_cycle * clk_rate >= NSEC_PER_SEC / 2 &&
> > state->duty_cycle * clk_rate < NSEC_PER_SEC
> >
> > here. If this isn't true rather disable the PWM or output a 100% duty
> > cycle with a larger period.
>
> Why not just having the duty_cycle is 50% only ?
> state->duty_cycle * 2 == state->period;
Yeah, for the bypass case you can only provide a 50% duty cycle. The
problem you have to address is that you cannot rely on your consumer to
request only 50% duty cycles. So you have to implement some behaviour if
your consumer requests period = 1 / clk_rate and 20% duty cycle.
Where I want to get the pwm framework as a whole is to let lowlevel
drivers round down both duty_cycle and period to the next possible values
in their .apply callback to be able to provide a more uniform behaviour
for consumers. So here this would mean:
- 1 / clk_rate <= state->period < smallest value without bypass &&
0 <= state->duty_cycle < state->period / 2
=> provide a constant 0
- 1 / clk_rate <= state->period < smallest value without bypass &&
state->period / 2 <= state->duty_cycle < state->period
=> use bypass mode providing 50% duty cycle
- 1 / clk_rate <= state->period < smallest value without bypass &&
state->period == state->duty_cycle
=> provide a constant 1
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 1/7] dt-bindings: pwm: allwinner: Add H6 PWM description
2019-11-04 8:03 ` Uwe Kleine-König
2019-11-04 17:49 ` Clément Péron
@ 2019-11-05 11:11 ` Maxime Ripard
2019-11-05 12:34 ` Clément Péron
1 sibling, 1 reply; 33+ messages in thread
From: Maxime Ripard @ 2019-11-05 11:11 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Mark Rutland, linux-pwm, Jernej Skrabec, devicetree,
linux-kernel, Rob Herring, Chen-Yu Tsai, Thierry Reding,
Clément Péron, kernel, linux-arm-kernel
[-- Attachment #1.1: Type: text/plain, Size: 3142 bytes --]
Hi Clement, Uwe,
On Mon, Nov 04, 2019 at 09:03:59AM +0100, Uwe Kleine-König wrote:
> On Sun, Nov 03, 2019 at 09:33:28PM +0100, Clément Péron wrote:
> > From: Jernej Skrabec <jernej.skrabec@siol.net>
> >
> > H6 PWM block is basically the same as A20 PWM, except that it also has
> > bus clock and reset line which needs to be handled accordingly.
> >
> > Expand Allwinner PWM binding with H6 PWM specifics.
> >
> > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> > Signed-off-by: Clément Péron <peron.clem@gmail.com>
> > ---
> > .../bindings/pwm/allwinner,sun4i-a10-pwm.yaml | 45 ++++++++++++++++++-
> > 1 file changed, 44 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/devicetree/bindings/pwm/allwinner,sun4i-a10-pwm.yaml b/Documentation/devicetree/bindings/pwm/allwinner,sun4i-a10-pwm.yaml
> > index 0ac52f83a58c..bf36ea509f31 100644
> > --- a/Documentation/devicetree/bindings/pwm/allwinner,sun4i-a10-pwm.yaml
> > +++ b/Documentation/devicetree/bindings/pwm/allwinner,sun4i-a10-pwm.yaml
> > @@ -30,13 +30,46 @@ properties:
> > - items:
> > - const: allwinner,sun50i-h5-pwm
> > - const: allwinner,sun5i-a13-pwm
> > + - const: allwinner,sun50i-h6-pwm
> >
> > reg:
> > maxItems: 1
> >
> > - clocks:
> > + # Even though it only applies to subschemas under the conditionals,
> > + # not listing them here will trigger a warning because of the
> > + # additionalsProperties set to false.
> > + clocks: true
> > + clock-names: true
> > + resets:
> > maxItems: 1
> >
> > + if:
> > + properties:
> > + compatible:
> > + contains:
> > + const: allwinner,sun50i-h6-pwm
> > +
> > + then:
> > + properties:
> > + clocks:
> > + items:
> > + - description: Module Clock
> > + - description: Bus Clock
> > +
> > + clock-names:
> > + items:
> > + - const: mod
> > + - const: bus
> > +
> > + required:
> > + - clock-names
> > + - resets
> > +
> > + else:
> > + properties:
> > + clocks:
> > + maxItems: 1
> > +
>
> I guess this hunk says "If this is a allwinner,sun50i-h6-pwm, a mod and
> bus clock is required.", right?
>
> I wonder if it is sensible to require a clock-names property in the else
> branch, too. This would make it obvious if the clock there corresponds
> to the "mod" or the "bus" clock on H6. (I guess it's "mod".)
This can be done a bit differently and could address your concerns
Something like
properties:
...
clocks:
minItems: 1
maxItems: 2
items:
- description: Bus Clock
- description: Module Clock
required:
- clocks
if:
...
then:
properties:
clocks:
maxItems: 2
clocks-names:
items:
- const: mod
- const: bus
required:
- clock-names
else:
properties:
clocks:
maxItems: 1
That way, the definition of the order and which clock is which is
pretty obvious in both cases, and we don't get any weird warnings.
Maxime
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 1/7] dt-bindings: pwm: allwinner: Add H6 PWM description
2019-11-05 11:11 ` Maxime Ripard
@ 2019-11-05 12:34 ` Clément Péron
2019-11-05 17:32 ` Maxime Ripard
0 siblings, 1 reply; 33+ messages in thread
From: Clément Péron @ 2019-11-05 12:34 UTC (permalink / raw)
To: Maxime Ripard
Cc: Mark Rutland, linux-pwm, Jernej Skrabec, devicetree,
linux-kernel, Rob Herring, Chen-Yu Tsai, Thierry Reding,
Pengutronix Kernel Team, Uwe Kleine-König, linux-arm-kernel
Hi Maxime,
On Tue, 5 Nov 2019 at 12:11, Maxime Ripard <mripard@kernel.org> wrote:
>
> Hi Clement, Uwe,
>
> On Mon, Nov 04, 2019 at 09:03:59AM +0100, Uwe Kleine-König wrote:
> > On Sun, Nov 03, 2019 at 09:33:28PM +0100, Clément Péron wrote:
> > > From: Jernej Skrabec <jernej.skrabec@siol.net>
> > >
> > > H6 PWM block is basically the same as A20 PWM, except that it also has
> > > bus clock and reset line which needs to be handled accordingly.
> > >
> > > Expand Allwinner PWM binding with H6 PWM specifics.
> > >
> > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> > > Signed-off-by: Clément Péron <peron.clem@gmail.com>
> > > ---
> > > .../bindings/pwm/allwinner,sun4i-a10-pwm.yaml | 45 ++++++++++++++++++-
> > > 1 file changed, 44 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/Documentation/devicetree/bindings/pwm/allwinner,sun4i-a10-pwm.yaml b/Documentation/devicetree/bindings/pwm/allwinner,sun4i-a10-pwm.yaml
> > > index 0ac52f83a58c..bf36ea509f31 100644
> > > --- a/Documentation/devicetree/bindings/pwm/allwinner,sun4i-a10-pwm.yaml
> > > +++ b/Documentation/devicetree/bindings/pwm/allwinner,sun4i-a10-pwm.yaml
> > > @@ -30,13 +30,46 @@ properties:
> > > - items:
> > > - const: allwinner,sun50i-h5-pwm
> > > - const: allwinner,sun5i-a13-pwm
> > > + - const: allwinner,sun50i-h6-pwm
> > >
> > > reg:
> > > maxItems: 1
> > >
> > > - clocks:
> > > + # Even though it only applies to subschemas under the conditionals,
> > > + # not listing them here will trigger a warning because of the
> > > + # additionalsProperties set to false.
> > > + clocks: true
> > > + clock-names: true
> > > + resets:
> > > maxItems: 1
> > >
> > > + if:
> > > + properties:
> > > + compatible:
> > > + contains:
> > > + const: allwinner,sun50i-h6-pwm
> > > +
> > > + then:
> > > + properties:
> > > + clocks:
> > > + items:
> > > + - description: Module Clock
> > > + - description: Bus Clock
> > > +
> > > + clock-names:
> > > + items:
> > > + - const: mod
> > > + - const: bus
> > > +
> > > + required:
> > > + - clock-names
> > > + - resets
> > > +
> > > + else:
> > > + properties:
> > > + clocks:
> > > + maxItems: 1
> > > +
> >
> > I guess this hunk says "If this is a allwinner,sun50i-h6-pwm, a mod and
> > bus clock is required.", right?
> >
> > I wonder if it is sensible to require a clock-names property in the else
> > branch, too. This would make it obvious if the clock there corresponds
> > to the "mod" or the "bus" clock on H6. (I guess it's "mod".)
>
> This can be done a bit differently and could address your concerns
>
> Something like
>
> properties:
> ...
>
> clocks:
> minItems: 1
> maxItems: 2
> items:
> - description: Bus Clock
> - description: Module Clock
>
> required:
> - clocks
>
> if:
> ...
>
> then:
> properties:
> clocks:
> maxItems: 2
Here we should set minItems to 2 right ?
so Max = Min = 2
Regards,
Clément
>
> clocks-names:
> items:
> - const: mod
> - const: bus
>
> required:
> - clock-names
>
> else:
> properties:
> clocks:
> maxItems: 1
>
> That way, the definition of the order and which clock is which is
> pretty obvious in both cases, and we don't get any weird warnings.
>
> Maxime
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 4/7] pwm: sun4i: Add support to output source clock directly
2019-11-05 7:29 ` Uwe Kleine-König
@ 2019-11-05 12:58 ` Clément Péron
2019-11-05 13:12 ` Uwe Kleine-König
0 siblings, 1 reply; 33+ messages in thread
From: Clément Péron @ 2019-11-05 12:58 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Mark Rutland, linux-pwm, Jernej Skrabec, devicetree,
linux-kernel, Maxime Ripard, Rob Herring, Chen-Yu Tsai,
Thierry Reding, linux-arm-kernel
Hi Uwe,
On Tue, 5 Nov 2019 at 08:29, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Hi Clément,
>
> On Mon, Nov 04, 2019 at 10:28:54PM +0100, Clément Péron wrote:
> > On Mon, 4 Nov 2019 at 09:38, Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> > > On Sun, Nov 03, 2019 at 09:33:31PM +0100, Clément Péron wrote:
> > > > From: Jernej Skrabec <jernej.skrabec@siol.net>
> > > >
> > > > PWM core has an option to bypass whole logic and output unchanged source
> > > > clock as PWM output. This is achieved by enabling bypass bit.
> > > >
> > > > Note that when bypass is enabled, no other setting has any meaning, not
> > > > even enable bit.
> > > >
> > > > This mode of operation is needed to achieve high enough frequency to
> > > > serve as clock source for AC200 chip, which is integrated into same
> > > > package as H6 SoC.
> > >
> > > I think the , should be dropped.
> > >
> > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> > > > Signed-off-by: Clément Péron <peron.clem@gmail.com>
> > > > ---
> > > > drivers/pwm/pwm-sun4i.c | 39 ++++++++++++++++++++++++++++++++++++++-
> > > > 1 file changed, 38 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> > > > index b5e7ac364f59..2441574674d9 100644
> > > > --- a/drivers/pwm/pwm-sun4i.c
> > > > +++ b/drivers/pwm/pwm-sun4i.c
> > > > @@ -3,6 +3,10 @@
> > > > * Driver for Allwinner sun4i Pulse Width Modulation Controller
> > > > *
> > > > * Copyright (C) 2014 Alexandre Belloni <alexandre.belloni@free-electrons.com>
> > > > + *
> > > > + * Limitations:
> > > > + * - When outputing the source clock directly, the PWM logic will be bypassed
> > > > + * and the currently running period is not guaranted to be completed
> > >
> > > Typo: guaranted -> guaranteed
> > >
> > > > */
> > > >
> > > > #include <linux/bitops.h>
> > > > @@ -73,6 +77,7 @@ static const u32 prescaler_table[] = {
> > > >
> > > > struct sun4i_pwm_data {
> > > > bool has_prescaler_bypass;
> > > > + bool has_direct_mod_clk_output;
> > > > unsigned int npwm;
> > > > };
> > > >
> > > > @@ -118,6 +123,20 @@ static void sun4i_pwm_get_state(struct pwm_chip *chip,
> > > >
> > > > val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
> > > >
> > > > + /*
> > > > + * PWM chapter in H6 manual has a diagram which explains that if bypass
> > > > + * bit is set, no other setting has any meaning. Even more, experiment
> > > > + * proved that also enable bit is ignored in this case.
> > > > + */
> > > > + if ((val & BIT_CH(PWM_BYPASS, pwm->hwpwm)) &&
> > > > + data->has_direct_mod_clk_output) {
> > > > + state->period = DIV_ROUND_CLOSEST_ULL(NSEC_PER_SEC, clk_rate);
> > > > + state->duty_cycle = state->period / 2;
> > > > + state->polarity = PWM_POLARITY_NORMAL;
> > > > + state->enabled = true;
> > > > + return;
> > > > + }
> > >
> > > Not sure how the rest of sun4i_pwm_get_state behaves, but I would prefer
> > > to let .get_state() round up which together with .apply_state() rounding
> > > down yields sound behaviour.
> > Ok
> > >
> > > > +
> > > > if ((PWM_REG_PRESCAL(val, pwm->hwpwm) == PWM_PRESCAL_MASK) &&
> > > > sun4i_pwm->data->has_prescaler_bypass)
> > > > prescaler = 1;
> > > > @@ -203,7 +222,8 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > > > {
> > > > struct sun4i_pwm_chip *sun4i_pwm = to_sun4i_pwm_chip(chip);
> > > > struct pwm_state cstate;
> > > > - u32 ctrl;
> > > > + u32 ctrl, clk_rate;
> > > > + bool bypass;
> > > > int ret;
> > > > unsigned int delay_us;
> > > > unsigned long now;
> > > > @@ -218,6 +238,16 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> > > > }
> > > > }
> > > >
> > > > + /*
> > > > + * Although it would make much more sense to check for bypass in
> > > > + * sun4i_pwm_calculate(), value of bypass bit also depends on "enabled".
> > > > + * Period is allowed to be rounded up or down.
> > > > + */
> > > > + clk_rate = clk_get_rate(sun4i_pwm->clk);
> > > > + bypass = ((state->period * clk_rate >= NSEC_PER_SEC &&
> > > > + state->period * clk_rate < NSEC_PER_SEC + clk_rate) &&
> > > > + state->enabled);
> > >
> > > I guess the compiler is smart enough here, but checking for
> > > state->enabled is cheaper than the other checks, so putting this at the
> > > start of the expression seems sensible.
> > >
> > > The comment doesn't match the code. You don't round up state->period.
> > > (This is good, please fix the comment.) I think dropping the check
> > >
> > > state->period * clk_rate < NSEC_PER_SEC + clk_rate
> > >
> > > would be fine, too.
> > Ok
> >
> > >
> > > I'd like to have a check for
> > >
> > > state->duty_cycle * clk_rate >= NSEC_PER_SEC / 2 &&
> > > state->duty_cycle * clk_rate < NSEC_PER_SEC
> > >
> > > here. If this isn't true rather disable the PWM or output a 100% duty
> > > cycle with a larger period.
> >
> > Why not just having the duty_cycle is 50% only ?
> > state->duty_cycle * 2 == state->period;
>
> Yeah, for the bypass case you can only provide a 50% duty cycle. The
> problem you have to address is that you cannot rely on your consumer to
> request only 50% duty cycles. So you have to implement some behaviour if
> your consumer requests period = 1 / clk_rate and 20% duty cycle.
So you request to add a new patch in this series for fixing the actual
PWM behavior at corner case?
This series just want to add a new device and a new bypass
functionality and I can't measure the output of PWM and testing it
properly.
Can this be done in another patch/series ?
Regards,
Clément
>
> Where I want to get the pwm framework as a whole is to let lowlevel
> drivers round down both duty_cycle and period to the next possible values
> in their .apply callback to be able to provide a more uniform behaviour
> for consumers. So here this would mean:
>
> - 1 / clk_rate <= state->period < smallest value without bypass &&
> 0 <= state->duty_cycle < state->period / 2
> => provide a constant 0
>
> - 1 / clk_rate <= state->period < smallest value without bypass &&
> state->period / 2 <= state->duty_cycle < state->period
> => use bypass mode providing 50% duty cycle
>
> - 1 / clk_rate <= state->period < smallest value without bypass &&
> state->period == state->duty_cycle
> => provide a constant 1
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-König |
> Industrial Linux Solutions | http://www.pengutronix.de/ |
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 2/7] pwm: sun4i: Add an optional probe for reset line
2019-11-05 7:01 ` Philipp Zabel
@ 2019-11-05 13:03 ` Clément Péron
0 siblings, 0 replies; 33+ messages in thread
From: Clément Péron @ 2019-11-05 13:03 UTC (permalink / raw)
To: Philipp Zabel
Cc: Mark Rutland, linux-pwm, Jernej Skrabec, devicetree,
linux-kernel, Maxime Ripard, Rob Herring, Chen-Yu Tsai,
Thierry Reding, Uwe Kleine-König, linux-arm-kernel
Hi Philipp,
On Tue, 5 Nov 2019 at 08:01, Philipp Zabel <pza@pengutronix.de> wrote:
>
> On Mon, Nov 04, 2019 at 09:11:57AM +0100, Uwe Kleine-König wrote:
> > Hello,
> >
> > adding Philipp Zabel (= reset controller maintainer) to Cc: and so I'm
> > not stripping the uncommented parts of the patch.
> >
> > On Sun, Nov 03, 2019 at 09:33:29PM +0100, Clément Péron wrote:
> > > From: Jernej Skrabec <jernej.skrabec@siol.net>
> > >
> > > H6 PWM core needs deasserted reset line in order to work.
> > >
> > > Add an optional probe for it.
> > >
> > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> > > Signed-off-by: Clément Péron <peron.clem@gmail.com>
> > > ---
> > > drivers/pwm/pwm-sun4i.c | 32 ++++++++++++++++++++++++++++++--
> > > 1 file changed, 30 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> > > index 6f5840a1a82d..d194b8ebdb00 100644
> > > --- a/drivers/pwm/pwm-sun4i.c
> > > +++ b/drivers/pwm/pwm-sun4i.c
> > > @@ -16,6 +16,7 @@
> > > #include <linux/of_device.h>
> > > #include <linux/platform_device.h>
> > > #include <linux/pwm.h>
> > > +#include <linux/reset.h>
> > > #include <linux/slab.h>
> > > #include <linux/spinlock.h>
> > > #include <linux/time.h>
> > > @@ -78,6 +79,7 @@ struct sun4i_pwm_data {
> > > struct sun4i_pwm_chip {
> > > struct pwm_chip chip;
> > > struct clk *clk;
> > > + struct reset_control *rst;
> > > void __iomem *base;
> > > spinlock_t ctrl_lock;
> > > const struct sun4i_pwm_data *data;
> > > @@ -365,6 +367,20 @@ static int sun4i_pwm_probe(struct platform_device *pdev)
> > > if (IS_ERR(pwm->clk))
> > > return PTR_ERR(pwm->clk);
> > >
> > > + pwm->rst = devm_reset_control_get_optional(&pdev->dev, NULL);
> > > + if (IS_ERR(pwm->rst)) {
> > > + if (PTR_ERR(pwm->rst) == -EPROBE_DEFER)
> > > + return PTR_ERR(pwm->rst);
> > > + dev_info(&pdev->dev, "no reset control found\n");
> >
> > I would degrade this to a dev_dbg. Otherwise this spams the log for all
> > unaffected machines.
>
> The _optional variants return NULL if the reset is not specified in the
> device tree, so this is not "no reset control found", but a real error
> that should be returned.
Correct,
Thanks for the catch,
Clément
>
> > devm_reset_control_get_optional() is defined in a section that has a
> > comment "These inline function calls will be removed once all
> > consumers have been moved over to the new explicit API.", so I guess
> > you want devm_reset_control_get_optional_exclusive or even
> > devm_reset_control_get_optional_shared here.
>
> Correct. If this driver deasserts in probe() and asserts the reset in
> remove(), this can use the refcounting _shared variant.
>
> > @Philipp: maybe a check in checkpatch that warns about introduction of
> > such new instances would be good?!
>
> Yes, that would be helpful.
>
> regards
> Philipp
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 4/7] pwm: sun4i: Add support to output source clock directly
2019-11-05 12:58 ` Clément Péron
@ 2019-11-05 13:12 ` Uwe Kleine-König
2019-11-05 13:12 ` Clément Péron
0 siblings, 1 reply; 33+ messages in thread
From: Uwe Kleine-König @ 2019-11-05 13:12 UTC (permalink / raw)
To: Clément Péron
Cc: Mark Rutland, linux-pwm, Jernej Skrabec, devicetree,
linux-kernel, Maxime Ripard, Rob Herring, Chen-Yu Tsai,
Thierry Reding, linux-arm-kernel
Hello Clément,
On Tue, Nov 05, 2019 at 01:58:31PM +0100, Clément Péron wrote:
> This series just want to add a new device and a new bypass
> functionality and I can't measure the output of PWM and testing it
> properly.
> Can this be done in another patch/series ?
I'm fine if you implement the bypass stuff with this logic and keep the
other stuff as is.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 4/7] pwm: sun4i: Add support to output source clock directly
2019-11-05 13:12 ` Uwe Kleine-König
@ 2019-11-05 13:12 ` Clément Péron
0 siblings, 0 replies; 33+ messages in thread
From: Clément Péron @ 2019-11-05 13:12 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: Mark Rutland, linux-pwm, Jernej Skrabec, devicetree,
linux-kernel, Maxime Ripard, Rob Herring, Chen-Yu Tsai,
Thierry Reding, linux-arm-kernel
On Tue, 5 Nov 2019 at 14:12, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Hello Clément,
>
> On Tue, Nov 05, 2019 at 01:58:31PM +0100, Clément Péron wrote:
> > This series just want to add a new device and a new bypass
> > functionality and I can't measure the output of PWM and testing it
> > properly.
> > Can this be done in another patch/series ?
>
> I'm fine if you implement the bypass stuff with this logic and keep the
> other stuff as is.
Thanks,
Clément
>
> Best regards
> Uwe
>
> --
> Pengutronix e.K. | Uwe Kleine-König |
> Industrial Linux Solutions | http://www.pengutronix.de/ |
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 1/7] dt-bindings: pwm: allwinner: Add H6 PWM description
2019-11-05 12:34 ` Clément Péron
@ 2019-11-05 17:32 ` Maxime Ripard
2019-11-06 9:25 ` Clément Péron
0 siblings, 1 reply; 33+ messages in thread
From: Maxime Ripard @ 2019-11-05 17:32 UTC (permalink / raw)
To: Clément Péron
Cc: Mark Rutland, linux-pwm, Jernej Skrabec, devicetree,
linux-kernel, Rob Herring, Chen-Yu Tsai, Thierry Reding,
Pengutronix Kernel Team, Uwe Kleine-König, linux-arm-kernel
On Tue, Nov 05, 2019 at 01:34:37PM +0100, Clément Péron wrote:
> On Tue, 5 Nov 2019 at 12:11, Maxime Ripard <mripard@kernel.org> wrote:
> >
> > Hi Clement, Uwe,
> >
> > On Mon, Nov 04, 2019 at 09:03:59AM +0100, Uwe Kleine-König wrote:
> > > On Sun, Nov 03, 2019 at 09:33:28PM +0100, Clément Péron wrote:
> > > > From: Jernej Skrabec <jernej.skrabec@siol.net>
> > > >
> > > > H6 PWM block is basically the same as A20 PWM, except that it also has
> > > > bus clock and reset line which needs to be handled accordingly.
> > > >
> > > > Expand Allwinner PWM binding with H6 PWM specifics.
> > > >
> > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> > > > Signed-off-by: Clément Péron <peron.clem@gmail.com>
> > > > ---
> > > > .../bindings/pwm/allwinner,sun4i-a10-pwm.yaml | 45 ++++++++++++++++++-
> > > > 1 file changed, 44 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/pwm/allwinner,sun4i-a10-pwm.yaml b/Documentation/devicetree/bindings/pwm/allwinner,sun4i-a10-pwm.yaml
> > > > index 0ac52f83a58c..bf36ea509f31 100644
> > > > --- a/Documentation/devicetree/bindings/pwm/allwinner,sun4i-a10-pwm.yaml
> > > > +++ b/Documentation/devicetree/bindings/pwm/allwinner,sun4i-a10-pwm.yaml
> > > > @@ -30,13 +30,46 @@ properties:
> > > > - items:
> > > > - const: allwinner,sun50i-h5-pwm
> > > > - const: allwinner,sun5i-a13-pwm
> > > > + - const: allwinner,sun50i-h6-pwm
> > > >
> > > > reg:
> > > > maxItems: 1
> > > >
> > > > - clocks:
> > > > + # Even though it only applies to subschemas under the conditionals,
> > > > + # not listing them here will trigger a warning because of the
> > > > + # additionalsProperties set to false.
> > > > + clocks: true
> > > > + clock-names: true
> > > > + resets:
> > > > maxItems: 1
> > > >
> > > > + if:
> > > > + properties:
> > > > + compatible:
> > > > + contains:
> > > > + const: allwinner,sun50i-h6-pwm
> > > > +
> > > > + then:
> > > > + properties:
> > > > + clocks:
> > > > + items:
> > > > + - description: Module Clock
> > > > + - description: Bus Clock
> > > > +
> > > > + clock-names:
> > > > + items:
> > > > + - const: mod
> > > > + - const: bus
> > > > +
> > > > + required:
> > > > + - clock-names
> > > > + - resets
> > > > +
> > > > + else:
> > > > + properties:
> > > > + clocks:
> > > > + maxItems: 1
> > > > +
> > >
> > > I guess this hunk says "If this is a allwinner,sun50i-h6-pwm, a mod and
> > > bus clock is required.", right?
> > >
> > > I wonder if it is sensible to require a clock-names property in the else
> > > branch, too. This would make it obvious if the clock there corresponds
> > > to the "mod" or the "bus" clock on H6. (I guess it's "mod".)
> >
> > This can be done a bit differently and could address your concerns
> >
> > Something like
> >
> > properties:
> > ...
> >
> > clocks:
> > minItems: 1
> > maxItems: 2
> > items:
> > - description: Bus Clock
> > - description: Module Clock
> >
> > required:
> > - clocks
> >
> > if:
> > ...
> >
> > then:
> > properties:
> > clocks:
> > maxItems: 2
>
> Here we should set minItems to 2 right ?
> so Max = Min = 2
It's done automatically by the tooling when the other is missing.
Maxime
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v2 1/7] dt-bindings: pwm: allwinner: Add H6 PWM description
2019-11-05 17:32 ` Maxime Ripard
@ 2019-11-06 9:25 ` Clément Péron
0 siblings, 0 replies; 33+ messages in thread
From: Clément Péron @ 2019-11-06 9:25 UTC (permalink / raw)
To: Maxime Ripard
Cc: Mark Rutland, linux-pwm, Jernej Skrabec, devicetree,
linux-kernel, Rob Herring, Chen-Yu Tsai, Thierry Reding,
Pengutronix Kernel Team, Uwe Kleine-König, linux-arm-kernel
Hi,
On Tue, 5 Nov 2019 at 18:32, Maxime Ripard <mripard@kernel.org> wrote:
>
> On Tue, Nov 05, 2019 at 01:34:37PM +0100, Clément Péron wrote:
> > On Tue, 5 Nov 2019 at 12:11, Maxime Ripard <mripard@kernel.org> wrote:
> > >
> > > Hi Clement, Uwe,
> > >
> > > On Mon, Nov 04, 2019 at 09:03:59AM +0100, Uwe Kleine-König wrote:
> > > > On Sun, Nov 03, 2019 at 09:33:28PM +0100, Clément Péron wrote:
> > > > > From: Jernej Skrabec <jernej.skrabec@siol.net>
> > > > >
> > > > > H6 PWM block is basically the same as A20 PWM, except that it also has
> > > > > bus clock and reset line which needs to be handled accordingly.
> > > > >
> > > > > Expand Allwinner PWM binding with H6 PWM specifics.
> > > > >
> > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> > > > > Signed-off-by: Clément Péron <peron.clem@gmail.com>
> > > > > ---
> > > > > .../bindings/pwm/allwinner,sun4i-a10-pwm.yaml | 45 ++++++++++++++++++-
> > > > > 1 file changed, 44 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/pwm/allwinner,sun4i-a10-pwm.yaml b/Documentation/devicetree/bindings/pwm/allwinner,sun4i-a10-pwm.yaml
> > > > > index 0ac52f83a58c..bf36ea509f31 100644
> > > > > --- a/Documentation/devicetree/bindings/pwm/allwinner,sun4i-a10-pwm.yaml
> > > > > +++ b/Documentation/devicetree/bindings/pwm/allwinner,sun4i-a10-pwm.yaml
> > > > > @@ -30,13 +30,46 @@ properties:
> > > > > - items:
> > > > > - const: allwinner,sun50i-h5-pwm
> > > > > - const: allwinner,sun5i-a13-pwm
> > > > > + - const: allwinner,sun50i-h6-pwm
> > > > >
> > > > > reg:
> > > > > maxItems: 1
> > > > >
> > > > > - clocks:
> > > > > + # Even though it only applies to subschemas under the conditionals,
> > > > > + # not listing them here will trigger a warning because of the
> > > > > + # additionalsProperties set to false.
> > > > > + clocks: true
> > > > > + clock-names: true
> > > > > + resets:
> > > > > maxItems: 1
> > > > >
> > > > > + if:
> > > > > + properties:
> > > > > + compatible:
> > > > > + contains:
> > > > > + const: allwinner,sun50i-h6-pwm
> > > > > +
> > > > > + then:
> > > > > + properties:
> > > > > + clocks:
> > > > > + items:
> > > > > + - description: Module Clock
> > > > > + - description: Bus Clock
> > > > > +
> > > > > + clock-names:
> > > > > + items:
> > > > > + - const: mod
> > > > > + - const: bus
> > > > > +
> > > > > + required:
> > > > > + - clock-names
> > > > > + - resets
> > > > > +
> > > > > + else:
> > > > > + properties:
> > > > > + clocks:
> > > > > + maxItems: 1
> > > > > +
> > > >
> > > > I guess this hunk says "If this is a allwinner,sun50i-h6-pwm, a mod and
> > > > bus clock is required.", right?
> > > >
> > > > I wonder if it is sensible to require a clock-names property in the else
> > > > branch, too. This would make it obvious if the clock there corresponds
> > > > to the "mod" or the "bus" clock on H6. (I guess it's "mod".)
> > >
> > > This can be done a bit differently and could address your concerns
> > >
> > > Something like
> > >
> > > properties:
> > > ...
> > >
> > > clocks:
> > > minItems: 1
> > > maxItems: 2
> > > items:
> > > - description: Bus Clock
> > > - description: Module Clock
> > >
> > > required:
> > > - clocks
> > >
> > > if:
> > > ...
> > >
> > > then:
> > > properties:
> > > clocks:
> > > maxItems: 2
> >
> > Here we should set minItems to 2 right ?
> > so Max = Min = 2
>
> It's done automatically by the tooling when the other is missing.
Ok thanks,
I will update in v4.
Regards,
Clément
>
> Maxime
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 33+ messages in thread
end of thread, other threads:[~2019-11-06 9:25 UTC | newest]
Thread overview: 33+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-03 20:33 [PATCH v2 0/7] Add support for H6 PWM Clément Péron
2019-11-03 20:33 ` [PATCH v2 1/7] dt-bindings: pwm: allwinner: Add H6 PWM description Clément Péron
2019-11-04 8:03 ` Uwe Kleine-König
2019-11-04 17:49 ` Clément Péron
2019-11-05 11:11 ` Maxime Ripard
2019-11-05 12:34 ` Clément Péron
2019-11-05 17:32 ` Maxime Ripard
2019-11-06 9:25 ` Clément Péron
2019-11-03 20:33 ` [PATCH v2 2/7] pwm: sun4i: Add an optional probe for reset line Clément Péron
2019-11-04 8:11 ` Uwe Kleine-König
2019-11-04 17:50 ` Clément Péron
2019-11-05 7:01 ` Philipp Zabel
2019-11-05 13:03 ` Clément Péron
2019-11-03 20:33 ` [PATCH v2 3/7] pwm: sun4i: Add an optional probe for bus clock Clément Péron
2019-11-04 8:24 ` Uwe Kleine-König
2019-11-04 18:07 ` Clément Péron
2019-11-04 20:10 ` Uwe Kleine-König
2019-11-04 20:19 ` Jernej Škrabec
2019-11-04 20:27 ` Clément Péron
2019-11-04 20:38 ` Jernej Škrabec
2019-11-03 20:33 ` [PATCH v2 4/7] pwm: sun4i: Add support to output source clock directly Clément Péron
2019-11-03 22:30 ` kbuild test robot
2019-11-03 22:41 ` Clément Péron
2019-11-03 22:58 ` kbuild test robot
2019-11-04 8:38 ` Uwe Kleine-König
2019-11-04 21:28 ` Clément Péron
2019-11-05 7:29 ` Uwe Kleine-König
2019-11-05 12:58 ` Clément Péron
2019-11-05 13:12 ` Uwe Kleine-König
2019-11-05 13:12 ` Clément Péron
2019-11-03 20:33 ` [PATCH v2 5/7] pwm: sun4i: Add support for H6 PWM Clément Péron
2019-11-03 20:33 ` [PATCH v2 6/7] arm64: dts: allwinner: h6: Add PWM node Clément Péron
2019-11-03 20:33 ` [PATCH v2 7/7] [DO NOT MERGE] arm64: allwinner: h6: enable Beelink GS1 PWM Clément Péron
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).