[PATCH 0/6] pwm: sun4i: Add support for Allwinner H6

135 views
Skip to first unread message

Jernej Skrabec

unread,
Jul 26, 2019, 2:40:56 PM7/26/19
to thierry...@gmail.com, mri...@kernel.org, we...@csie.org, rob...@kernel.org, mark.r...@arm.com, linu...@vger.kernel.org, devic...@vger.kernel.org, linux-ar...@lists.infradead.org, linux-...@vger.kernel.org, linux...@googlegroups.com
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.

Please take a look.

Best regards,
Jernej

Jernej Skrabec (6):
dt-bindings: pwm: allwinner: Add H6 PWM description
pwm: sun4i: Add a quirk for reset line
pwm: sun4i: Add a quirk for bus clock
pwm: sun4i: Add support for H6 PWM
pwm: sun4i: Add support to output source clock directly
arm64: dts: allwinner: h6: Add PWM node

.../bindings/pwm/allwinner,sun4i-a10-pwm.yaml | 36 +++++++-
arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 10 +++
drivers/pwm/pwm-sun4i.c | 83 ++++++++++++++++++-
3 files changed, 125 insertions(+), 4 deletions(-)

--
2.22.0

Jernej Skrabec

unread,
Jul 26, 2019, 2:40:58 PM7/26/19
to thierry...@gmail.com, mri...@kernel.org, we...@csie.org, rob...@kernel.org, mark.r...@arm.com, linu...@vger.kernel.org, devic...@vger.kernel.org, linux-ar...@lists.infradead.org, linux-...@vger.kernel.org, linux...@googlegroups.com
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....@siol.net>
---
.../bindings/pwm/allwinner,sun4i-a10-pwm.yaml | 36 ++++++++++++++++++-
1 file changed, 35 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..deca5d81802f 100644
--- a/Documentation/devicetree/bindings/pwm/allwinner,sun4i-a10-pwm.yaml
+++ b/Documentation/devicetree/bindings/pwm/allwinner,sun4i-a10-pwm.yaml
@@ -30,13 +30,47 @@ 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

+allOf:
+ - if:
+ properties:
+ compatible:
+ contains:
+ const: allwinner,sun50i-h6-pwm
+
+ then:
+ properties:
+ clocks:
+ items:
+ - description: Module Clock
+ - description: Bus Clock
+
+ clock-names:
+ items:
+ - const: pwm
+ - const: bus
+
+ required:
+ - clock-names
+ - resets
+
+ else:
+ properties:
+ clocks:
+ maxItems: 1
+
required:
- "#pwm-cells"
- compatible
--
2.22.0

Jernej Skrabec

unread,
Jul 26, 2019, 2:41:01 PM7/26/19
to thierry...@gmail.com, mri...@kernel.org, we...@csie.org, rob...@kernel.org, mark.r...@arm.com, linu...@vger.kernel.org, devic...@vger.kernel.org, linux-ar...@lists.infradead.org, linux-...@vger.kernel.org, linux...@googlegroups.com
H6 PWM core needs deasserted reset line in order to work.

Add a quirk for it.

Signed-off-by: Jernej Skrabec <jernej....@siol.net>
---
drivers/pwm/pwm-sun4i.c | 27 +++++++++++++++++++++++++--
1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
index de78c824bbfd..1b7be8fbde86 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>
@@ -72,12 +73,14 @@ static const u32 prescaler_table[] = {

struct sun4i_pwm_data {
bool has_prescaler_bypass;
+ bool has_reset;
unsigned int npwm;
};

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;
@@ -371,6 +374,14 @@ static int sun4i_pwm_probe(struct platform_device *pdev)
if (IS_ERR(pwm->clk))
return PTR_ERR(pwm->clk);

+ if (pwm->data->has_reset) {
+ pwm->rst = devm_reset_control_get(&pdev->dev, NULL);
+ if (IS_ERR(pwm->rst))
+ return PTR_ERR(pwm->rst);
+
+ reset_control_deassert(pwm->rst);
+ }
+
pwm->chip.dev = &pdev->dev;
pwm->chip.ops = &sun4i_pwm_ops;
pwm->chip.base = -1;
@@ -383,19 +394,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;

- return pwmchip_remove(&pwm->chip);
+ reset_control_assert(pwm->rst);
+
+ return 0;
}

static struct platform_driver sun4i_pwm_driver = {
--
2.22.0

Jernej Skrabec

unread,
Jul 26, 2019, 2:41:03 PM7/26/19
to thierry...@gmail.com, mri...@kernel.org, we...@csie.org, rob...@kernel.org, mark.r...@arm.com, linu...@vger.kernel.org, devic...@vger.kernel.org, linux-ar...@lists.infradead.org, linux-...@vger.kernel.org, linux...@googlegroups.com
H6 PWM core needs bus clock to be enabled in order to work.

Add a quirk for it.

Signed-off-by: Jernej Skrabec <jernej....@siol.net>
---
drivers/pwm/pwm-sun4i.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)

diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
index 1b7be8fbde86..7d3ac3f2dc3f 100644
--- a/drivers/pwm/pwm-sun4i.c
+++ b/drivers/pwm/pwm-sun4i.c
@@ -72,6 +72,7 @@ static const u32 prescaler_table[] = {
};

struct sun4i_pwm_data {
+ bool has_bus_clock;
bool has_prescaler_bypass;
bool has_reset;
unsigned int npwm;
@@ -79,6 +80,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;
@@ -382,6 +384,16 @@ static int sun4i_pwm_probe(struct platform_device *pdev)
reset_control_deassert(pwm->rst);
}

+ if (pwm->data->has_bus_clock) {
+ pwm->bus_clk = devm_clk_get(&pdev->dev, "bus");
+ if (IS_ERR(pwm->bus_clk)) {
+ ret = PTR_ERR(pwm->bus_clk);
+ goto err_bus;
+ }
+
+ clk_prepare_enable(pwm->bus_clk);
+ }
+
pwm->chip.dev = &pdev->dev;
pwm->chip.ops = &sun4i_pwm_ops;
pwm->chip.base = -1;
@@ -402,6 +414,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;
@@ -416,6 +430,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.22.0

Jernej Skrabec

unread,
Jul 26, 2019, 2:41:05 PM7/26/19
to thierry...@gmail.com, mri...@kernel.org, we...@csie.org, rob...@kernel.org, mark.r...@arm.com, linu...@vger.kernel.org, devic...@vger.kernel.org, linux-ar...@lists.infradead.org, linux-...@vger.kernel.org, linux...@googlegroups.com
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....@siol.net>
---
drivers/pwm/pwm-sun4i.c | 10 ++++++++++
1 file changed, 10 insertions(+)

diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
index 7d3ac3f2dc3f..9e0eca79ff88 100644
--- a/drivers/pwm/pwm-sun4i.c
+++ b/drivers/pwm/pwm-sun4i.c
@@ -331,6 +331,13 @@ static const struct sun4i_pwm_data sun4i_pwm_single_bypass = {
.npwm = 1,
};

+static const struct sun4i_pwm_data sun50i_pwm_dual_bypass_clk_rst = {
+ .has_bus_clock = true,
+ .has_prescaler_bypass = true,
+ .has_reset = true,
+ .npwm = 2,
+};
+
static const struct of_device_id sun4i_pwm_dt_ids[] = {
{
.compatible = "allwinner,sun4i-a10-pwm",
@@ -347,6 +354,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_pwm_dual_bypass_clk_rst,
}, {
/* sentinel */
},
--
2.22.0

Jernej Skrabec

unread,
Jul 26, 2019, 2:41:07 PM7/26/19
to thierry...@gmail.com, mri...@kernel.org, we...@csie.org, rob...@kernel.org, mark.r...@arm.com, linu...@vger.kernel.org, devic...@vger.kernel.org, linux-ar...@lists.infradead.org, linux-...@vger.kernel.org, linux...@googlegroups.com
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....@siol.net>
---
drivers/pwm/pwm-sun4i.c | 31 ++++++++++++++++++++++++++++++-
1 file changed, 30 insertions(+), 1 deletion(-)

diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
index 9e0eca79ff88..848cff26f385 100644
--- a/drivers/pwm/pwm-sun4i.c
+++ b/drivers/pwm/pwm-sun4i.c
@@ -120,6 +120,19 @@ 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)) {
+ 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;
@@ -211,7 +224,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;
@@ -226,6 +240,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 == NSEC_PER_SEC / clk_rate ||
+ state->period == DIV_ROUND_UP(NSEC_PER_SEC, clk_rate)) &&
+ state->enabled;
+
spin_lock(&sun4i_pwm->ctrl_lock);
ctrl = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);

@@ -273,6 +297,11 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
ctrl &= ~BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
}

+ 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.22.0

Jernej Skrabec

unread,
Jul 26, 2019, 2:41:11 PM7/26/19
to thierry...@gmail.com, mri...@kernel.org, we...@csie.org, rob...@kernel.org, mark.r...@arm.com, linu...@vger.kernel.org, devic...@vger.kernel.org, linux-ar...@lists.infradead.org, linux-...@vger.kernel.org, linux...@googlegroups.com
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....@siol.net>
---
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 e8bed58e7246..c1abd805cfdc 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
@@ -229,6 +229,16 @@
status = "disabled";
};

+ pwm: pwm@300a000 {
+ compatible = "allwinner,sun50i-h6-pwm";
+ reg = <0x0300a000 0x400>;
+ clocks = <&osc24M>, <&ccu CLK_BUS_PWM>;
+ clock-names = "pwm", "bus";
+ resets = <&ccu RST_BUS_PWM>;
+ #pwm-cells = <3>;
+ status = "disabled";
+ };
+
pio: pinctrl@300b000 {
compatible = "allwinner,sun50i-h6-pinctrl";
reg = <0x0300b000 0x400>;
--
2.22.0

Jernej Škrabec

unread,
Jul 27, 2019, 10:15:53 AM7/27/19
to Maxime Ripard, thierry...@gmail.com, we...@csie.org, rob...@kernel.org, mark.r...@arm.com, linu...@vger.kernel.org, devic...@vger.kernel.org, linux-ar...@lists.infradead.org, linux-...@vger.kernel.org, linux...@googlegroups.com
Dne sobota, 27. julij 2019 ob 12:46:28 CEST je Maxime Ripard napisal(a):
> Hi,
> The patch itself looks fine, but you should clarify which clock is
> being used by the old driver.
>
> My guess is that the "new" clock is actually the mod one, while the
> old one was both the clock of the register interface (bus) and the
> clock of the PWM generation logic (mod).

Well, I checked few datasheets and nowhere is explicitly stated what is the
bus clock, but I would make same guess as you.

Anyway, since you requested that order of the clocks has to be changed, I have
to separately obtain clocks if there is bus clock present too or not. If it
is, both clocks have to be obtained by name, and if not, old code without name
can be used.

Best regards,
Jernej

>
> Maxime
>
> --
> Maxime Ripard, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com




Chen-Yu Tsai

unread,
Jul 27, 2019, 10:27:41 AM7/27/19
to Maxime Ripard, Jernej Skrabec, Thierry Reding, Rob Herring, Mark Rutland, linu...@vger.kernel.org, devicetree, linux-arm-kernel, linux-kernel, linux-sunxi
On Sat, Jul 27, 2019 at 6:46 PM Maxime Ripard <mri...@kernel.org> wrote:
>
> Hi,
>
> On Fri, Jul 26, 2019 at 08:40:42PM +0200, Jernej Skrabec wrote:
> The patch itself looks fine, but you should clarify which clock is
> being used by the old driver.
>
> My guess is that the "new" clock is actually the mod one, while the
> old one was both the clock of the register interface (bus) and the
> clock of the PWM generation logic (mod).

The H6 datasheet explicitly states:

The clock source of PWM is OSC24M. The PWM is on APB1 Bus. Ensure
that open APB1 Bus gating and de-assert reset signal when accessed
to the PWM.

Older datasheets do not have anything about bus gating or resets. However
with slightly newer ones that have a system bus tree diagram, we can see
that PWM is on APB1 (or APB0/APBS for R_PWM). We can assume there is no
bus gate and thus it is directly attached to APB1, and that we never
modeled this part.

So the new clock is definitely the bus gate. You might want to introduce
a patch renaming sun4i_pwm_data.clk to sun4i_pwm_data.mod_clk before this
one.

ChenYu

Jernej Škrabec

unread,
Jul 27, 2019, 10:28:42 AM7/27/19
to Maxime Ripard, thierry...@gmail.com, we...@csie.org, rob...@kernel.org, mark.r...@arm.com, linu...@vger.kernel.org, devic...@vger.kernel.org, linux-ar...@lists.infradead.org, linux-...@vger.kernel.org, linux...@googlegroups.com
Dne sobota, 27. julij 2019 ob 12:50:08 CEST je Maxime Ripard napisal(a):
> On Fri, Jul 26, 2019 at 08:40:44PM +0200, Jernej Skrabec wrote:
> > 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....@siol.net>
>
> It doesn't seem to be available on the A10 (at least) though. The A13
> seem to have it, so you should probably check that, and make that
> conditional to the compatible if not available on all of them.

Ok, can you suggest the name for the quirk? "has_bypass" is suspiciously
similar to "has_prescaler_bypass".

Also, how to name these sun4i_pwm_data structures? Now that there are (will
be) three new quirks, name of the structure would be just too long, like
"sun50i_pwm_dual_prescaler_bypass_clk_rst_bypass".

Chen-Yu Tsai

unread,
Jul 27, 2019, 10:54:47 AM7/27/19
to Jernej Skrabec, Maxime Ripard, Thierry Reding, Rob Herring, Mark Rutland, linu...@vger.kernel.org, devicetree, linux-arm-kernel, linux-kernel, linux-sunxi
On Sat, Jul 27, 2019 at 10:28 PM Jernej Škrabec <jernej....@siol.net> wrote:
>
> Dne sobota, 27. julij 2019 ob 12:50:08 CEST je Maxime Ripard napisal(a):
> > On Fri, Jul 26, 2019 at 08:40:44PM +0200, Jernej Skrabec wrote:
> > > 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....@siol.net>
> >
> > It doesn't seem to be available on the A10 (at least) though. The A13
> > seem to have it, so you should probably check that, and make that
> > conditional to the compatible if not available on all of them.
>
> Ok, can you suggest the name for the quirk? "has_bypass" is suspiciously
> similar to "has_prescaler_bypass".

has_direct_mod_clk_output?

> Also, how to name these sun4i_pwm_data structures? Now that there are (will
> be) three new quirks, name of the structure would be just too long, like
> "sun50i_pwm_dual_prescaler_bypass_clk_rst_bypass".

Just use the SoC model. Any later ones that have the same quirks will likely
use the same compatible string anyway.

ChenYu

Uwe Kleine-König

unread,
Jul 29, 2019, 2:36:44 AM7/29/19
to Jernej Skrabec, thierry...@gmail.com, mri...@kernel.org, we...@csie.org, rob...@kernel.org, mark.r...@arm.com, linu...@vger.kernel.org, devic...@vger.kernel.org, linux-ar...@lists.infradead.org, linux-...@vger.kernel.org, linux...@googlegroups.com, Philipp Zabel
Cc += reset framework maintainer

Hello Jernej,
I wonder why there is a need to track if a given chip needs a reset
line. I'd just use devm_reset_control_get_optional() and drop the
.has_reset member in struct sun4i_pwm_data.
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |

Uwe Kleine-König

unread,
Jul 29, 2019, 2:38:39 AM7/29/19
to Jernej Skrabec, thierry...@gmail.com, mri...@kernel.org, we...@csie.org, rob...@kernel.org, mark.r...@arm.com, linu...@vger.kernel.org, devic...@vger.kernel.org, linux-ar...@lists.infradead.org, linux-...@vger.kernel.org, linux...@googlegroups.com, ker...@pengutronix.de
Hello,

On Fri, Jul 26, 2019 at 08:40:42PM +0200, Jernej Skrabec wrote:
Similar to my suggestion in patch 2: I'd use devm_clk_get_optional() and
drop struct sun4i_pwm_data::has_bus_clock.

Best regards
Uwe

Uwe Kleine-König

unread,
Jul 29, 2019, 2:40:41 AM7/29/19
to Jernej Skrabec, thierry...@gmail.com, mri...@kernel.org, we...@csie.org, rob...@kernel.org, mark.r...@arm.com, linu...@vger.kernel.org, devic...@vger.kernel.org, linux-ar...@lists.infradead.org, linux-...@vger.kernel.org, linux...@googlegroups.com, ker...@pengutronix.de
If you follow my suggestion for the two previous patches, you can just
use:

compatible = "allwinner,sun50i-h6-pwm", "allwinner,sun5i-a10s-pwm";

and drop this patch.

Best regards
Uwe

> }, {
> /* sentinel */
> },
> --
> 2.22.0
>
>

Chen-Yu Tsai

unread,
Jul 29, 2019, 2:43:39 AM7/29/19
to Uwe Kleine-König, Jernej Skrabec, Thierry Reding, Maxime Ripard, Rob Herring, Mark Rutland, linu...@vger.kernel.org, devicetree, linux-arm-kernel, linux-kernel, linux-sunxi, Philipp Zabel
Because it's not optional for this platform, i.e. it won't work if
the reset control (or clk, in the next patch) is somehow missing from
the device tree.

ChenYu

Uwe Kleine-König

unread,
Jul 29, 2019, 3:06:18 AM7/29/19
to Jernej Skrabec, thierry...@gmail.com, mri...@kernel.org, we...@csie.org, rob...@kernel.org, mark.r...@arm.com, linu...@vger.kernel.org, devic...@vger.kernel.org, linux-ar...@lists.infradead.org, linux-...@vger.kernel.org, linux...@googlegroups.com
On Fri, Jul 26, 2019 at 08:40:44PM +0200, Jernej Skrabec wrote:
Every driver seems to implement rounding the way its driver considers it
sensible. @Thierry: This is another patch where it would be good to have
a global directive about how rounding is supposed to work to provide the
users an reliable and uniform way to work with PWMs.

> + clk_rate = clk_get_rate(sun4i_pwm->clk);
> + bypass = (state->period == NSEC_PER_SEC / clk_rate ||
> + state->period == DIV_ROUND_UP(NSEC_PER_SEC, clk_rate)) &&
> + state->enabled;

Not sure if the compiler is clever enough to notice the obvious
optimisation with this code construct, but you can write this test in a
more clever way which has zero instead of up to two divisions. Something
like:

bypass = ((state->period * clk_rate >= NSEC_PER_SEC &&
state->period * clk_rate < NSEC_PER_SEC + clk_rate) &&
state->enabled);

In the commit log you write the motivation for using bypass is that it
allows to implement higher frequency then with the "normal" operation.
As you don't skip calculating the normal parameters requesting such a
high-frequency setting either errors out or doesn't catch the impossible
request. In both cases there is something to fix.

> +
> spin_lock(&sun4i_pwm->ctrl_lock);
> ctrl = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
>
> @@ -273,6 +297,11 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> ctrl &= ~BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
> }
>
> + if (bypass)
> + ctrl |= BIT_CH(PWM_BYPASS, pwm->hwpwm);
> + else
> + ctrl &= ~BIT_CH(PWM_BYPASS, pwm->hwpwm);
> +

Does switching on (or off) the bypass bit complete the currently running
period?

> sun4i_pwm_writel(sun4i_pwm, ctrl, PWM_CTRL_REG);
>
> spin_unlock(&sun4i_pwm->ctrl_lock);

Best regards
Uwe

Uwe Kleine-König

unread,
Jul 29, 2019, 3:12:31 AM7/29/19
to Chen-Yu Tsai, Jernej Skrabec, Thierry Reding, Maxime Ripard, Rob Herring, Mark Rutland, linu...@vger.kernel.org, devicetree, linux-arm-kernel, linux-kernel, linux-sunxi, Philipp Zabel
Hello,

On Mon, Jul 29, 2019 at 02:43:23PM +0800, Chen-Yu Tsai wrote:
> On Mon, Jul 29, 2019 at 2:36 PM Uwe Kleine-König
> <u.klein...@pengutronix.de> wrote:
> > On Fri, Jul 26, 2019 at 08:40:41PM +0200, Jernej Skrabec wrote:
> > > @@ -371,6 +374,14 @@ static int sun4i_pwm_probe(struct platform_device *pdev)
> > > if (IS_ERR(pwm->clk))
> > > return PTR_ERR(pwm->clk);
> > >
> > > + if (pwm->data->has_reset) {
> > > + pwm->rst = devm_reset_control_get(&pdev->dev, NULL);
> > > + if (IS_ERR(pwm->rst))
> > > + return PTR_ERR(pwm->rst);
> > > +
> > > + reset_control_deassert(pwm->rst);
> > > + }
> > > +
> >
> > I wonder why there is a need to track if a given chip needs a reset
> > line. I'd just use devm_reset_control_get_optional() and drop the
> > .has_reset member in struct sun4i_pwm_data.
>
> Because it's not optional for this platform, i.e. it won't work if
> the reset control (or clk, in the next patch) is somehow missing from
> the device tree.

If the device tree is wrong it is considered ok that the driver doesn't
behave correctly. So this is not a problem you need (or should) care
about.

Best regards
Uwe

Philipp Zabel

unread,
Jul 29, 2019, 6:19:11 AM7/29/19
to Uwe Kleine-König, Chen-Yu Tsai, Jernej Skrabec, Thierry Reding, Maxime Ripard, Rob Herring, Mark Rutland, linu...@vger.kernel.org, devicetree, linux-arm-kernel, linux-kernel, linux-sunxi
Hi,
I agree with this. Catching missing DT properties and other device tree
validation is not the job of device drivers. The _optional request
variants were introduced to simplify drivers that require the reset line
on some platforms and not on others.

I would ask to explicitly state whether the driver needs full control
over the moment of (de)assertion of the reset signal, or whether the
only requirement is that the reset signal stays deasserted while the PWM
driver is active, by using devm_reset_control_get_optional_exclusive or
devm_reset_control_get_optional_shared to request the reset control.

regards
Philipp

Jernej Škrabec

unread,
Jul 29, 2019, 11:48:41 AM7/29/19
to Uwe Kleine-König, thierry...@gmail.com, mri...@kernel.org, we...@csie.org, rob...@kernel.org, mark.r...@arm.com, linu...@vger.kernel.org, devic...@vger.kernel.org, linux-ar...@lists.infradead.org, linux-...@vger.kernel.org, linux...@googlegroups.com, ker...@pengutronix.de
Hi Uwe,

Dne ponedeljek, 29. julij 2019 ob 08:38:25 CEST je Uwe Kleine-König
napisal(a):
This one is not so simple. This patch has incorrect logic. Correct logic would
be to use "devm_clk_get(&pdev->dev, NULL)" for variants without bus clock as
it is done already and "devm_clk_get(&pdev->dev, "bus")" and
"devm_clk_get(&pdev->dev, "mod")" for variants with bus clock.

You see, DT nodes for other variants don't have clock-names property at all.
If it would be specified, it would be "mod". So, DT nodes for other variants
have "mod" clock specified on first place (the only one), while H6 DT node will
have "mod" clock specified on second place (see one of e-mails from Maxime), so
"NULL" can't be used instead of "mod" in both cases.

So I would say quirk is beneficial to know if you have to look up clocks by
name or you just take first clock specified.

Best regards,
Jernej

>
> Best regards
> Uwe




Jernej Škrabec

unread,
Jul 29, 2019, 11:55:57 AM7/29/19
to Uwe Kleine-König, thierry...@gmail.com, mri...@kernel.org, we...@csie.org, rob...@kernel.org, mark.r...@arm.com, linu...@vger.kernel.org, devic...@vger.kernel.org, linux-ar...@lists.infradead.org, linux-...@vger.kernel.org, linux...@googlegroups.com, ker...@pengutronix.de
Hi Uwe,

Dne ponedeljek, 29. julij 2019 ob 08:40:30 CEST je Uwe Kleine-König
napisal(a):
Maxime found out that it's not compatible with A10s due to difference in bypass
bit, but yes, I know what you mean.

Since H6 requires reset line and bus clock to be specified, it's not compatible
from DT binding side. New yaml based binding must somehow know that in order
to be able to validate DT node, so it needs standalone compatible. However,
depending on conclusions of other discussions, this new compatible can be
associated with already available quirks structure or have it's own.

Best regards,
Jernej

Uwe Kleine-König

unread,
Jul 29, 2019, 12:07:28 PM7/29/19
to Jernej Škrabec, mark.r...@arm.com, linu...@vger.kernel.org, devic...@vger.kernel.org, linux...@googlegroups.com, linux-...@vger.kernel.org, mri...@kernel.org, we...@csie.org, rob...@kernel.org, thierry...@gmail.com, ker...@pengutronix.de, linux-ar...@lists.infradead.org
I cannot follow. You should be able to specify in the binding that the
reset line and bus clock is optional. Then allwinner,sun50i-h6-pwm
without a reset line and bus clock also verifies, but this doesn't
really hurt (and who knows, maybe the next allwinner chip needs exactly
this).

Best regards
Uwe

Chen-Yu Tsai

unread,
Jul 29, 2019, 12:09:57 PM7/29/19
to Uwe Kleine-König, Jernej Škrabec, Mark Rutland, linu...@vger.kernel.org, devicetree, linux-sunxi, linux-kernel, Maxime Ripard, Rob Herring, Thierry Reding, Sascha Hauer, linux-arm-kernel
It is not optional. It will not work if either the clocks or reset controls
are missing. How would these be optional anyway? Either it's connected and
thus required, or it's not and therefore should be omitted from the
description.

ChenYu

Uwe Kleine-König

unread,
Jul 29, 2019, 12:14:39 PM7/29/19
to Jernej Škrabec, mark.r...@arm.com, linu...@vger.kernel.org, devic...@vger.kernel.org, linux...@googlegroups.com, linux-...@vger.kernel.org, mri...@kernel.org, we...@csie.org, rob...@kernel.org, thierry...@gmail.com, ker...@pengutronix.de, linux-ar...@lists.infradead.org
Hello Jernej,
Then maybe something like the following?:

busclk = devm_clk_get_optional(..., "bus");
modclk = devm_clk_get_optional(..., "mod");

/*
* old dtbs might have a single clock but no clock names. Fall
* back to this for compatibility reasons.
*/
if (!modclk) {
modclk = devm_clk_get(..., NULL);
}

> You see, DT nodes for other variants don't have clock-names property at all.
> If it would be specified, it would be "mod". So, DT nodes for other variants
> have "mod" clock specified on first place (the only one), while H6 DT node will
> have "mod" clock specified on second place (see one of e-mails from Maxime), so
> "NULL" can't be used instead of "mod" in both cases.
>
> So I would say quirk is beneficial to know if you have to look up clocks by
> name or you just take first clock specified.

Jernej Škrabec

unread,
Jul 29, 2019, 12:16:59 PM7/29/19
to Uwe Kleine-König, thierry...@gmail.com, mri...@kernel.org, we...@csie.org, rob...@kernel.org, mark.r...@arm.com, linu...@vger.kernel.org, devic...@vger.kernel.org, linux-ar...@lists.infradead.org, linux-...@vger.kernel.org, linux...@googlegroups.com
Hi Uwe,

Dne ponedeljek, 29. julij 2019 ob 09:06:05 CEST je Uwe Kleine-König
napisal(a):
It's the latter, otherwise it wouldn't work for my case. I'll fix the check and
skip additional logic.

>
> > +
> >
> > spin_lock(&sun4i_pwm->ctrl_lock);
> > ctrl = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
> >
> > @@ -273,6 +297,11 @@ static int sun4i_pwm_apply(struct pwm_chip *chip,
> > struct pwm_device *pwm,>
> > ctrl &= ~BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
> >
> > }
> >
> > + if (bypass)
> > + ctrl |= BIT_CH(PWM_BYPASS, pwm->hwpwm);
> > + else
> > + ctrl &= ~BIT_CH(PWM_BYPASS, pwm->hwpwm);
> > +
>
> Does switching on (or off) the bypass bit complete the currently running
> period?

I don't really know. If I understand correctly, it just bypasses PWM logic
completely, so I would say it doesn't complete the currently running period.

Take a look at chapter 3.9.2 http://linux-sunxi.org/
File:Allwinner_H6_V200_User_Manual_V1.1.pdf

Best regards,
Jernej

Uwe Kleine-König

unread,
Jul 29, 2019, 12:24:36 PM7/29/19
to Chen-Yu Tsai, Mark Rutland, linu...@vger.kernel.org, Jernej Škrabec, devicetree, linux-kernel, Maxime Ripard, linux-sunxi, Rob Herring, Thierry Reding, Sascha Hauer, linux-arm-kernel
Hello,
[Just arguing about the clock here, the argumentation is analogous for
the reset control.]

From the driver's perspective it's optional: There are devices with and
without a bus clock. This doesn't mean that you can just ignore this
clock if it's specified. It's optional in the sense "If dt doesn't
specify it, then assume this is a device that doesn't have it and so you
don't need to handle it." but not in the sense "it doesn't matter if
you handle it or not.".

Other than that I'm on your side. So for example I think it's not
optimal that gpiod_get_optional returns NULL if GPIOLIB=n or that
devm_reset_control_get_optional returns NULL if RESET_CONTROLLER=n
because this hides exactly the kind of problem you point out here.

Uwe Kleine-König

unread,
Jul 29, 2019, 12:29:30 PM7/29/19
to Jernej Škrabec, thierry...@gmail.com, mri...@kernel.org, we...@csie.org, rob...@kernel.org, mark.r...@arm.com, linu...@vger.kernel.org, devic...@vger.kernel.org, linux-ar...@lists.infradead.org, linux-...@vger.kernel.org, linux...@googlegroups.com
Hello,
Great.

> > > +
> > >
> > > spin_lock(&sun4i_pwm->ctrl_lock);
> > > ctrl = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
> > >
> > > @@ -273,6 +297,11 @@ static int sun4i_pwm_apply(struct pwm_chip *chip,
> > > struct pwm_device *pwm,>
> > > ctrl &= ~BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
> > >
> > > }
> > >
> > > + if (bypass)
> > > + ctrl |= BIT_CH(PWM_BYPASS, pwm->hwpwm);
> > > + else
> > > + ctrl &= ~BIT_CH(PWM_BYPASS, pwm->hwpwm);
> > > +
> >
> > Does switching on (or off) the bypass bit complete the currently running
> > period?
>
> I don't really know. If I understand correctly, it just bypasses PWM logic
> completely, so I would say it doesn't complete the currently running period.

This is a bug. It's part of the promise of the PWM API that started
periods are completed. Please at least document this limitation at the
top of the driver. drivers/pwm/pwm-sifive.c has an example you might
want to use as a template.

Jernej Škrabec

unread,
Jul 29, 2019, 12:40:19 PM7/29/19
to linux...@googlegroups.com, u.klein...@pengutronix.de, Chen-Yu Tsai, Mark Rutland, linu...@vger.kernel.org, devicetree, linux-kernel, Maxime Ripard, Rob Herring, Thierry Reding, Sascha Hauer, linux-arm-kernel
Dne ponedeljek, 29. julij 2019 ob 18:24:28 CEST je Uwe Kleine-König
I think there's misunderstanding. I only argued that we can't use

compatible = "allwinner,sun50i-h6-pwm",
"allwinner,sun5i-a10s-pwm";

as you suggested and only

compatible = "allwinner,sun50i-h6-pwm";

will work. Not because of driver itself (it can still use _optional()
variants), but because of DT binding, which should be able to validate H6 PWM
node - reset and bus clock references are required in this case.

Uwe Kleine-König

unread,
Jul 29, 2019, 2:21:03 PM7/29/19
to Maxime Ripard, Chen-Yu Tsai, Jernej Skrabec, Thierry Reding, Rob Herring, Mark Rutland, linu...@vger.kernel.org, devicetree, linux-arm-kernel, linux-kernel, linux-sunxi, Philipp Zabel
On Mon, Jul 29, 2019 at 06:37:15PM +0200, Maxime Ripard wrote:
> To some extent that's true, but if we can make the life easier for
> everyone by reporting an error and bailing out instead of silently
> ignoring that, continuing to probe and just ending up with a
> completely broken system for no apparent reason, then why not just do
> that?
>
> I mean, all it takes is three lines of code.

<pedantic>Actually it's more because you need to track for each variant
if it needs the clock (or reset stuff) or not.</pedantic>

It's a weighing between "simpler driver" and "catch unlikely errors".
(If the SoC's .dtsi includes the needed stuff, an error here is really
unlikely, isn't it?)

So to some degree it's subjective which one is better. I tend to prefer
"simpler driver". Also when catching this type of error you have to do
it right twice (in the .dtsi and the driver) while with my approach
there is only a single place that defines what is right, which is a good
design according to what I learned during my studies.

> It's no different than just calling clk_get, and testing the return
> code to see if it's there or not. I wouldn't call that check when you
> depend on a clock "validating the DT". It's just making sure that all
> the resources needed for you to operate properly are there, which is a
> pretty common thing to do.

This is different. As a driver author you are allowed (or even supposed)
to assume that the device tree (or ACPI or platform data ...) is right
and completely defines the stuff for the driven hardware to work
correctly. You must not assume that clk_get() succeeds unconditionally.

Uwe Kleine-König

unread,
Jul 29, 2019, 2:40:46 PM7/29/19
to Jernej Škrabec, linux...@googlegroups.com, Chen-Yu Tsai, Mark Rutland, linu...@vger.kernel.org, devicetree, linux-kernel, Maxime Ripard, Rob Herring, Thierry Reding, ker...@pengutronix.de, linux-arm-kernel
I think I understood. In my eyes there is no need to let validation of
the DT bindings catch a missing "optional" property that is needed on
H6.

You have to draw the line somewhere which information the driver has
hard-coded and what is only provided by the device tree and just assumed
to be correct by the driver. You argue the driver should know that if it
cares for a "allwinner,sun50i-h6-pwm" device it should know (and check)
that there is a clock named "bus" and a resets property that links to a
reset controller. How is that different from checking that the base
address is 0x0300a000 or that the "pwm" clock is the osc24M clock
running at 24 MHz? This isn't checked in the driver or the dt schema.
Still if the device tree got one of them wrong this yields an
non-working pwm device that isn't catched in the driver.

Jernej Škrabec

unread,
Jul 29, 2019, 2:46:28 PM7/29/19
to Uwe Kleine-König, linux...@googlegroups.com, Chen-Yu Tsai, Mark Rutland, linu...@vger.kernel.org, devicetree, linux-kernel, Maxime Ripard, Rob Herring, Thierry Reding, ker...@pengutronix.de, linux-arm-kernel
Dne ponedeljek, 29. julij 2019 ob 20:40:41 CEST je Uwe Kleine-König
No, in this thread I argue that DT validation tool, executed by

make ARCH=arm64 dtbs_check

should catch that. This is not a driver, but DT binding described in YAML.

Best regards,
Jernej

Uwe Kleine-König

unread,
Jul 29, 2019, 2:51:11 PM7/29/19
to Jernej Škrabec, linux...@googlegroups.com, Chen-Yu Tsai, Mark Rutland, linu...@vger.kernel.org, devicetree, linux-kernel, Maxime Ripard, Rob Herring, Thierry Reding, ker...@pengutronix.de, linux-arm-kernel
The argumentation is the same. dtbs_check doesn't notice if the base
address of your "allwinner,sun50i-h6-pwm" device is wrong. So why should
it catch a missing reset controller phandle?

Uwe Kleine-König

unread,
Jul 29, 2019, 3:04:09 PM7/29/19
to Maxime Ripard, Jernej Škrabec, mark.r...@arm.com, linu...@vger.kernel.org, devic...@vger.kernel.org, linux...@googlegroups.com, linux-...@vger.kernel.org, we...@csie.org, rob...@kernel.org, thierry...@gmail.com, ker...@pengutronix.de, linux-ar...@lists.infradead.org
Hello Maxime,

On Mon, Jul 29, 2019 at 06:45:16PM +0200, Maxime Ripard wrote:
> On Mon, Jul 29, 2019 at 06:14:35PM +0200, Uwe Kleine-König wrote:
> > Then maybe something like the following?:
> >
> > busclk = devm_clk_get_optional(..., "bus");
> > modclk = devm_clk_get_optional(..., "mod");
> >
> > /*
> > * old dtbs might have a single clock but no clock names. Fall
> > * back to this for compatibility reasons.
> > */
> > if (!modclk) {
> > modclk = devm_clk_get(..., NULL);
> > }
>
> Again, there's nothing optional about these clocks. You need a
> particular set of clocks for a given generation, and a separate set of
> them on another generation of SoCs.

It depends on the way how "optional" is understood. The semantic of
"optional" as it is used and implemented by devm_clk_get_optional (and
gpiod_get_optional and devm_reset_control_get_optional) is different
than yours when saying "on H6 the clock is not optional". If it was
about the "it doesn't matter if it's taken care of or not" semantic you
seem to mean the function would be useless and no driver would need to
actually use it. In the sense of the functions listed above "optional"
means: Some devices need it, others don't. Using this semantic the "bus"
clock is optional.

> It really isn't about DT validation. We're really making sure that the
> device can be operational. It's as much of a validation step than
> making sure we have mapped registers (reg), or an interrupt if we had
> any.

Do you agree with Jernej in the other end of this thread? If so I don't
think that repeating the same arguments here is sensible. Please read
what I wrote there.

Jernej Škrabec

unread,
Jul 29, 2019, 6:04:51 PM7/29/19
to Uwe Kleine-König, linux...@googlegroups.com, Chen-Yu Tsai, Mark Rutland, linu...@vger.kernel.org, devicetree, linux-kernel, Maxime Ripard, Rob Herring, Thierry Reding, ker...@pengutronix.de, linux-arm-kernel
Dne ponedeljek, 29. julij 2019 ob 20:51:08 CEST je Uwe Kleine-König
Of course checking actual values of node properties doesn't make sense in
dtbs_check, otherwise we would have million DT bindings. If you have 10 copies
of the same IP core, of course you would use same compatible, but actual
register ranges, interrupts, etc. would be different in DT nodes.

At this point I would make same argument as were made before, but there is no
point going in circles. I'm interested what have DT maintainers to say.

Best regards,
Jernej


Uwe Kleine-König

unread,
Jul 30, 2019, 4:09:04 AM7/30/19
to Rob Herring, Frank Rowand, linux...@googlegroups.com, Chen-Yu Tsai, Jernej Škrabec, Mark Rutland, linu...@vger.kernel.org, devicetree, linux-kernel, Maxime Ripard, Thierry Reding, ker...@pengutronix.de, linux-arm-kernel
Hello Rob and Frank,

Maxime and Jernej on one side and me on the other cannot agree about a
detail in the change to the bindings here. I'm trying to objectively
summarize the situation for you to help deciding what is the right thing
to do here.

TLDR: The sun4i pwm driver is extended to support a new variant of that
device on the H6 SoC. Compared to the earlier supported variants
allwinner,sun50i-h6-pwm on H6 needs to handle a reset controller and an
additional clock.

The two positions are:

- We need a new compatible because only then the driver and/or the dt
schema checker can check that each "allwinner,sun50i-h6-pwm" device
has a reset property and a "bus" clock; and the earlier variants
don't.

- The driver can be simpler and the device specific knowledge is only
in a single place (the dt) if the device tree is considered valid and
a reset property and "bus" clock is used iff it's provided in the
device tree without additional comparison for the compatible.

Now our arguments seem to go in circles and Jernej was interested in
your position. That's something I agree with ;-) Can you please share
your view?

Find below some context about the arguments.

Best regards
Uwe

On Tue, Jul 30, 2019 at 12:04:47AM +0200, Jernej Škrabec wrote:
> Dne ponedeljek, 29. julij 2019 ob 20:51:08 CEST je Uwe Kleine-König
> napisal(a):
> > On Mon, Jul 29, 2019 at 08:46:25PM +0200, Jernej Škrabec wrote:
> > > Dne ponedeljek, 29. julij 2019 ob 20:40:41 CEST je Uwe Kleine-König
> > > napisal(a):
> > > > On Mon, Jul 29, 2019 at 06:40:15PM +0200, Jernej Škrabec wrote:
> > > > > Dne ponedeljek, 29. julij 2019 ob 18:24:28 CEST je Uwe Kleine-König
> > > > > napisal(a):
(i.e. use devm_clk_get_optional instead of using devm_clk_get iff the
compatible is allwinner,sun50i-h6-pwm; analogous for the reset
controller.)

Chen-Yu Tsai

unread,
Jul 30, 2019, 4:33:12 AM7/30/19
to Uwe Kleine-König, Rob Herring, Frank Rowand, linux-sunxi, Jernej Škrabec, Mark Rutland, linu...@vger.kernel.org, devicetree, linux-kernel, Maxime Ripard, Thierry Reding, Sascha Hauer, linux-arm-kernel
On Tue, Jul 30, 2019 at 4:09 PM Uwe Kleine-König
<u.klein...@pengutronix.de> wrote:
>
> Hello Rob and Frank,
>
> Maxime and Jernej on one side and me on the other cannot agree about a
> detail in the change to the bindings here. I'm trying to objectively
> summarize the situation for you to help deciding what is the right thing
> to do here.
>
> TLDR: The sun4i pwm driver is extended to support a new variant of that
> device on the H6 SoC. Compared to the earlier supported variants
> allwinner,sun50i-h6-pwm on H6 needs to handle a reset controller and an
> additional clock.
>
> The two positions are:
>
> - We need a new compatible because only then the driver and/or the dt
> schema checker can check that each "allwinner,sun50i-h6-pwm" device
> has a reset property and a "bus" clock; and the earlier variants
> don't.
>
> - The driver can be simpler and the device specific knowledge is only
> in a single place (the dt) if the device tree is considered valid and
> a reset property and "bus" clock is used iff it's provided in the
> device tree without additional comparison for the compatible.
>
> Now our arguments seem to go in circles and Jernej was interested in
> your position. That's something I agree with ;-) Can you please share
> your view?
>
> Find below some context about the arguments.

A bit more context on the failure modes:

If the reset control is missing, anything done to hardware will be
silently ignored, since any writes to the registers are ignored.

On the other hand, if the bus clock is missing and otherwise not enabled,
accessing the device's registers could actually stall the whole system.

ChenYu
> --
> You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi...@googlegroups.com.
> To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/20190730080900.hhxrqun7vk4nsj2h%40pengutronix.de.

Uwe Kleine-König

unread,
Jul 31, 2019, 2:52:36 AM7/31/19
to Maxime Ripard, Rob Herring, Frank Rowand, linux...@googlegroups.com, Chen-Yu Tsai, Jernej Škrabec, Mark Rutland, linu...@vger.kernel.org, devicetree, linux-kernel, Thierry Reding, ker...@pengutronix.de, linux-arm-kernel
On Tue, Jul 30, 2019 at 07:06:01PM +0200, Maxime Ripard wrote:
> On Tue, Jul 30, 2019 at 10:09:00AM +0200, Uwe Kleine-König wrote:
> > Hello Rob and Frank,
> >
> > Maxime and Jernej on one side and me on the other cannot agree about a
> > detail in the change to the bindings here. I'm trying to objectively
> > summarize the situation for you to help deciding what is the right thing
> > to do here.
> >
> > TLDR: The sun4i pwm driver is extended to support a new variant of that
> > device on the H6 SoC. Compared to the earlier supported variants
> > allwinner,sun50i-h6-pwm on H6 needs to handle a reset controller and an
> > additional clock.
> >
> > The two positions are:
> >
> > - We need a new compatible because only then the driver and/or the dt
> > schema checker can check that each "allwinner,sun50i-h6-pwm" device
> > has a reset property and a "bus" clock; and the earlier variants
> > don't.
>
> There is two topics here, really. The binding itself really must have
> those properties as required.
>
> You had an analogy before that we shouldn't really do that, since it
> would be verification and that it would be similar to checking whether
> the register range was right. This analogy isn't correct, a better one
> would be checking that the register range exists in the first place.

The relevant difference is that *all* devices supported by the driver
have to have a register range. Compared to that only a subset of the
devices have to have a bus clock.

> Indeed, if you don't have a register range, you have no register to
> write to, and that's a showstopper for any driver. I'm pretty sure we
> all agree on that. But on those SoCs, like Chen-Yu said, having no
> resets or clocks properties result in an equally bad result: either
> any write to that device is completely ignored (missing reset), or the
> system completely (and silently) locks up (missing bus clock).
>
> We *have* to catch that somehow and not let anything like that happen.

IIUC both the clock and the reset stuff is SoC specific, so it's the
same for all machines with the H6, right? So assuming this is correctly
contained in the h6.dtsi, in which cases can this go wrong? I only see
the cases that the dts author includes the wrong dtsi or overrides
stuff. In the first case a non-working PWM is probably one of the
smaller problems and the second is something we're not really able to
catch.

But even if each machine's dts author has to get this right, I don't
think the dts schema is the right place to assert this.

> That being said, we can't really validate that the clock pointed is
> the right one, just like we can't really check that the register range
> is the proper one. Or rather, we could, but it's completely
> impractical and we do agree on that as well.
>
> Having the bus clock and reset line being required however is only a
> few lines in the binding though, and is very practical.
>
> > - The driver can be simpler and the device specific knowledge is only
> > in a single place (the dt) if the device tree is considered valid and
> > a reset property and "bus" clock is used iff it's provided in the
> > device tree without additional comparison for the compatible.
>
> And now we have the discussion on how it's implemented in a
> driver. Since it's pretty cheap to implement (only a couple of lines:
> two for the if block, one for the additional field in the structure,
> one for each SoC using that) and have huge benefits (not silently
> locking up the system at boot), then I'd say we should go for it.

Right, it's only a few lines. Still it (IMHO needlessly) complicates the
driver. From the driver's POV the device tree defines the
characteristics of the device and if the dts defines an h6-pwm without a
bus clock then maybe this is the PWM on the next generation SoC that
doesn't need it. And maybe you're happy in a few year's time when you
don't have to touch the driver again for this next generation SoC
because the driver is not only simpler but also flexible enough to
handle the new PWM without adaptions.

All in all I don't care much about the dt schema stuff, I want to keep
the driver simple. So if we agree that the schema ensures that the h6
pwms have a reset and a bus clock and we just use reset_get_optional and
clk_get_optional that's a compromise I can agree to.

Best regards
Uwe

Uwe Kleine-König

unread,
Aug 12, 2019, 6:47:09 AM8/12/19
to Maxime Ripard, Mark Rutland, linu...@vger.kernel.org, Jernej Škrabec, devicetree, Chen-Yu Tsai, linux-kernel, linux...@googlegroups.com, Rob Herring, Thierry Reding, ker...@pengutronix.de, Frank Rowand, linux-arm-kernel
Hello Maxime,

the idea of my mail was to summarize quickly the discussion for the dt
people to give their judgement to stop us circling in a discussion about
the always same points.

I suggest we stop the discussion here now and wait for a reply from them
instead.

Jernej Škrabec

unread,
Aug 12, 2019, 6:51:08 AM8/12/19
to Uwe Kleine-König, Maxime Ripard, Mark Rutland, linu...@vger.kernel.org, devicetree, Chen-Yu Tsai, linux-kernel, linux...@googlegroups.com, Rob Herring, Thierry Reding, ker...@pengutronix.de, Frank Rowand, linux-arm-kernel
Dne ponedeljek, 12. avgust 2019 ob 12:47:00 CEST je Uwe Kleine-König
napisal(a):
> Hello Maxime,
>
> the idea of my mail was to summarize quickly the discussion for the dt
> people to give their judgement to stop us circling in a discussion about
> the always same points.
>
> I suggest we stop the discussion here now and wait for a reply from them
> instead.

Shouldn't we just go with compromise solution you suggested and Maxime
accepted? I would like to send new version in time for 5.4.
Reply all
Reply to author
Forward
0 new messages