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

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

[Jernej Skrabec]

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

[PATCH 1/6] dt-bindings: pwm: allwinner: Add H6 PWM description

[Jernej Skrabec]

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>
---
 .../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

Re: [PATCH 1/6] dt-bindings: pwm: allwinner: Add H6 PWM description

[Maxime Ripard]

Hi,

On Fri, Jul 26, 2019 at 08:40:40PM +0200, Jernej Skrabec wrote:
> 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>
> ---
>  .../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:

There's only one condition, so you don't really need the allOf.

Maxime

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

[PATCH 2/6] pwm: sun4i: Add a quirk for reset line

[Jernej Skrabec]

H6 PWM core needs deasserted reset line in order to work.

Add a quirk for it.

Signed-off-by: Jernej Skrabec <jernej.skrabec-gGgVlfcn5nU@public.gmane.org>
---
 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

Re: [PATCH 2/6] pwm: sun4i: Add a quirk for reset line

[Maxime Ripard]

On Fri, Jul 26, 2019 at 08:40:41PM +0200, Jernej Skrabec wrote:
> H6 PWM core needs deasserted reset line in order to work.
>
> Add a quirk for it.
>
> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>

Acked-by: Maxime Ripard <maxime.ripard@bootlin.com>

Maxime

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

Re: [PATCH 2/6] pwm: sun4i: Add a quirk for reset line

[Uwe Kleine-König]

Cc += reset framework maintainer

Hello Jernej,

On Fri, Jul 26, 2019 at 08:40:41PM +0200, Jernej Skrabec wrote:
> H6 PWM core needs deasserted reset line in order to work.
> 
> Add a quirk for it.
> 
> Signed-off-by: Jernej Skrabec <jernej.skrabec-gGgVlfcn5nU@public.gmane.org>
> ---
>  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);
> +	}
> +

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.

>  	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 = {

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

-- 
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+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/20190729063630.rn325whatfnc3m7n%40pengutronix.de.

Re: [PATCH 2/6] pwm: sun4i: Add a quirk for reset line

[Chen-Yu Tsai]

On Mon, Jul 29, 2019 at 2:36 PM Uwe Kleine-König
<u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
>
> Cc += reset framework maintainer
>
> Hello Jernej,
>
> On Fri, Jul 26, 2019 at 08:40:41PM +0200, Jernej Skrabec wrote:
> > H6 PWM core needs deasserted reset line in order to work.
> >
> > Add a quirk for it.
> >
> > Signed-off-by: Jernej Skrabec <jernej.skrabec-gGgVlfcn5nU@public.gmane.org>
> > ---
> >  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);
> > +     }
> > +
>
> 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.

ChenYu

> >       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 = {
>
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |

-- 
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+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/CAGb2v65KOpivHQNkg%2BR2%3DD%3DejCJYnPdVcyHJZW-GJCR8j0Yk0g%40mail.gmail.com.

Re: [PATCH 2/6] pwm: sun4i: Add a quirk for reset line

[Uwe Kleine-König]

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.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> 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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

-- 
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+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/20190729071218.bukw7vxilqy523k3%40pengutronix.de.

Re: [PATCH 2/6] pwm: sun4i: Add a quirk for reset line

[Philipp Zabel]

Hi,

On Mon, 2019-07-29 at 09:12 +0200, Uwe Kleine-König wrote:
> 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.kleine-koenig@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.

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

Re: [PATCH 2/6] pwm: sun4i: Add a quirk for reset line

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2093 bytes --]

On Mon, Jul 29, 2019 at 09:12:18AM +0200, Uwe Kleine-König wrote:
> 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.kleine-koenig@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.

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.

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.

Maxime

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 2/6] pwm: sun4i: Add a quirk for reset line

[Uwe Kleine-König]

On Mon, Jul 29, 2019 at 06:37:15PM +0200, Maxime Ripard wrote:
> On Mon, Jul 29, 2019 at 09:12:18AM +0200, Uwe Kleine-König wrote:
> > 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.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> 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.
> 
> 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.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

-- 
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+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/20190729182046.6bwrterbxoceulrx%40pengutronix.de.

[PATCH 3/6] pwm: sun4i: Add a quirk for bus clock

[Jernej Skrabec]

H6 PWM core needs bus clock to be enabled in order to work.

Add a quirk for it.

Signed-off-by: Jernej Skrabec <jernej.skrabec-gGgVlfcn5nU@public.gmane.org>
---
 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

Re: [PATCH 3/6] pwm: sun4i: Add a quirk for bus clock

[Maxime Ripard]

Hi,

On Fri, Jul 26, 2019 at 08:40:42PM +0200, Jernej Skrabec wrote:
> H6 PWM core needs bus clock to be enabled in order to work.
>
> Add a quirk for it.
>
> Signed-off-by: Jernej Skrabec <jernej.skrabec@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);
> +	}
> +

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).

Maxime

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

Re: [PATCH 3/6] pwm: sun4i: Add a quirk for bus clock

[Jernej Škrabec]

Dne sobota, 27. julij 2019 ob 12:46:28 CEST je Maxime Ripard napisal(a):
> Hi,
> 
> On Fri, Jul 26, 2019 at 08:40:42PM +0200, Jernej Skrabec wrote:
> > H6 PWM core needs bus clock to be enabled in order to work.
> > 
> > Add a quirk for it.
> > 
> > Signed-off-by: Jernej Skrabec <jernej.skrabec-gGgVlfcn5nU@public.gmane.org>
> > ---
> > 
> >  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);
> > +	}
> > +
> 
> 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

Re: [PATCH 3/6] pwm: sun4i: Add a quirk for bus clock

[Chen-Yu Tsai]

On Sat, Jul 27, 2019 at 6:46 PM Maxime Ripard <mripard-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>
> Hi,
>
> On Fri, Jul 26, 2019 at 08:40:42PM +0200, Jernej Skrabec wrote:
> > H6 PWM core needs bus clock to be enabled in order to work.
> >
> > Add a quirk for it.
> >
> > Signed-off-by: Jernej Skrabec <jernej.skrabec-gGgVlfcn5nU@public.gmane.org>
> > ---
> >  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);
> > +     }
> > +
>
> 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

Re: [PATCH 3/6] pwm: sun4i: Add a quirk for bus clock

[Uwe Kleine-König]

Hello,

On Fri, Jul 26, 2019 at 08:40:42PM +0200, Jernej Skrabec wrote:
> H6 PWM core needs bus clock to be enabled in order to work.
> 
> Add a quirk for it.
> 
> Signed-off-by: Jernej Skrabec <jernej.skrabec@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");

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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 3/6] pwm: sun4i: Add a quirk for bus clock

[Jernej Škrabec]

Hi Uwe,

Dne ponedeljek, 29. julij 2019 ob 08:38:25 CEST je Uwe Kleine-König 
napisal(a):
> Hello,
> 
> On Fri, Jul 26, 2019 at 08:40:42PM +0200, Jernej Skrabec wrote:
> > H6 PWM core needs bus clock to be enabled in order to work.
> > 
> > Add a quirk for it.
> > 
> > Signed-off-by: Jernej Skrabec <jernej.skrabec-gGgVlfcn5nU@public.gmane.org>
> > ---
> > 
> >  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");
> 
> Similar to my suggestion in patch 2: I'd use devm_clk_get_optional() and
> drop struct sun4i_pwm_data::has_bus_clock.

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




-- 
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+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/4022372.WfP88Fa4Lu%40jernej-laptop.

Re: [PATCH 3/6] pwm: sun4i: Add a quirk for bus clock

[Uwe Kleine-König]

Hello Jernej,

On Mon, Jul 29, 2019 at 05:48:36PM +0200, Jernej Škrabec wrote:
> Dne ponedeljek, 29. julij 2019 ob 08:38:25 CEST je Uwe Kleine-König 
> napisal(a):
> > Hello,
> > 
> > On Fri, Jul 26, 2019 at 08:40:42PM +0200, Jernej Skrabec wrote:
> > > H6 PWM core needs bus clock to be enabled in order to work.
> > > 
> > > Add a quirk for it.
> > > 
> > > Signed-off-by: Jernej Skrabec <jernej.skrabec-gGgVlfcn5nU@public.gmane.org>
> > > ---
> > > 
> > >  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");
> > 
> > Similar to my suggestion in patch 2: I'd use devm_clk_get_optional() and
> > drop struct sun4i_pwm_data::has_bus_clock.
> 
> 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.

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.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

-- 
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+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/20190729161435.5bnj3ikocsyep4dd%40pengutronix.de.

Re: [PATCH 3/6] pwm: sun4i: Add a quirk for bus clock

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2979 bytes --]

On Mon, Jul 29, 2019 at 06:14:35PM +0200, Uwe Kleine-König wrote:
> Hello Jernej,
>
> On Mon, Jul 29, 2019 at 05:48:36PM +0200, Jernej Škrabec wrote:
> > Dne ponedeljek, 29. julij 2019 ob 08:38:25 CEST je Uwe Kleine-König
> > napisal(a):
> > > Hello,
> > >
> > > On Fri, Jul 26, 2019 at 08:40:42PM +0200, Jernej Skrabec wrote:
> > > > H6 PWM core needs bus clock to be enabled in order to work.
> > > >
> > > > Add a quirk for it.
> > > >
> > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@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");
> > >
> > > Similar to my suggestion in patch 2: I'd use devm_clk_get_optional() and
> > > drop struct sun4i_pwm_data::has_bus_clock.
> >
> > 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.
>
> 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 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.

Maxime

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 3/6] pwm: sun4i: Add a quirk for bus clock

[Uwe Kleine-König]

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.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

-- 
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+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/20190729190404.rnxw2aihmciafy6c%40pengutronix.de.

[PATCH 4/6] pwm: sun4i: Add support for H6 PWM

[Jernej Skrabec]

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-gGgVlfcn5nU@public.gmane.org>
---
 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

Re: [PATCH 4/6] pwm: sun4i: Add support for H6 PWM

[Uwe Kleine-König]

On Fri, Jul 26, 2019 at 08:40:43PM +0200, Jernej Skrabec wrote:
> 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-gGgVlfcn5nU@public.gmane.org>
> ---
>  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,

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
> 
> 

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

-- 
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+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/20190729064030.7uenld2kbof45zti%40pengutronix.de.

Re: [PATCH 4/6] pwm: sun4i: Add support for H6 PWM

[Jernej Škrabec]

Hi Uwe,

Dne ponedeljek, 29. julij 2019 ob 08:40:30 CEST je Uwe Kleine-König 
napisal(a):
> On Fri, Jul 26, 2019 at 08:40:43PM +0200, Jernej Skrabec wrote:
> > 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-gGgVlfcn5nU@public.gmane.org>
> > ---
> > 
> >  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,
> 
> 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.

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

> 
> Best regards
> Uwe
> 
> >  	}, {
> >  	
> >  		/* sentinel */
> >  	
> >  	},




-- 
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+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/223488703.0I5IR7NXoI%40jernej-laptop.

Re: [PATCH 4/6] pwm: sun4i: Add support for H6 PWM

[Uwe Kleine-König]

On Mon, Jul 29, 2019 at 05:55:52PM +0200, Jernej Škrabec wrote:
> Hi Uwe,
> 
> Dne ponedeljek, 29. julij 2019 ob 08:40:30 CEST je Uwe Kleine-König 
> napisal(a):
> > On Fri, Jul 26, 2019 at 08:40:43PM +0200, Jernej Skrabec wrote:
> > > 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-gGgVlfcn5nU@public.gmane.org>
> > > ---
> > > 
> > >  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,
> > 
> > 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.
> 
> 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.

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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

-- 
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+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/20190729160723.am3cs5pasi22pibi%40pengutronix.de.

Re: [linux-sunxi] Re: [PATCH 4/6] pwm: sun4i: Add support for H6 PWM

[Chen-Yu Tsai]

On Tue, Jul 30, 2019 at 12:07 AM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> On Mon, Jul 29, 2019 at 05:55:52PM +0200, Jernej Škrabec wrote:
> > Hi Uwe,
> >
> > Dne ponedeljek, 29. julij 2019 ob 08:40:30 CEST je Uwe Kleine-König
> > napisal(a):
> > > On Fri, Jul 26, 2019 at 08:40:43PM +0200, Jernej Skrabec wrote:
> > > > 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>
> > > > ---
> > > >
> > > >  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,
> > >
> > > 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.
> >
> > 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.
>
> 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).

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

Re: Re: [PATCH 4/6] pwm: sun4i: Add support for H6 PWM

[Uwe Kleine-König]

Hello,

On Tue, Jul 30, 2019 at 12:09:40AM +0800, Chen-Yu Tsai wrote:
> On Tue, Jul 30, 2019 at 12:07 AM Uwe Kleine-König
> <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> > On Mon, Jul 29, 2019 at 05:55:52PM +0200, Jernej Škrabec wrote:
> > > Dne ponedeljek, 29. julij 2019 ob 08:40:30 CEST je Uwe Kleine-König
> > > napisal(a):
> > > > On Fri, Jul 26, 2019 at 08:40:43PM +0200, Jernej Skrabec wrote:
> > > > > --- 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,
> > > >
> > > > 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.
> > >
> > > 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.
> >
> > 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).
> 
> 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.

[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.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

-- 
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+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/20190729162428.bxuzgxg5sjqptlbp%40pengutronix.de.

Re: Re: [PATCH 4/6] pwm: sun4i: Add support for H6 PWM

[Jernej Škrabec]

Dne ponedeljek, 29. julij 2019 ob 18:24:28 CEST je Uwe Kleine-König 
napisal(a):
> Hello,
> 
> On Tue, Jul 30, 2019 at 12:09:40AM +0800, Chen-Yu Tsai wrote:
> > On Tue, Jul 30, 2019 at 12:07 AM Uwe Kleine-König
> > 
> > <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> > > On Mon, Jul 29, 2019 at 05:55:52PM +0200, Jernej Škrabec wrote:
> > > > Dne ponedeljek, 29. julij 2019 ob 08:40:30 CEST je Uwe Kleine-König
> > > > 
> > > > napisal(a):
> > > > > On Fri, Jul 26, 2019 at 08:40:43PM +0200, Jernej Skrabec wrote:
> > > > > > --- 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,
> > > > > 
> > > > > 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.
> > > > 
> > > > 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.> > 
> > > 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).
> > 
> > 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.
> 
> [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.
>

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.

Best regards,
Jernej
 
> Best regards
> Uwe




-- 
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+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/2346193.MplWYqIveT%40jernej-laptop.

Re: Re: [PATCH 4/6] pwm: sun4i: Add support for H6 PWM

[Uwe Kleine-König]

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):
> > Hello,
> > 
> > On Tue, Jul 30, 2019 at 12:09:40AM +0800, Chen-Yu Tsai wrote:
> > > On Tue, Jul 30, 2019 at 12:07 AM Uwe Kleine-König
> > > 
> > > <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> > > > On Mon, Jul 29, 2019 at 05:55:52PM +0200, Jernej Škrabec wrote:
> > > > > Dne ponedeljek, 29. julij 2019 ob 08:40:30 CEST je Uwe Kleine-König
> > > > > 
> > > > > napisal(a):
> > > > > > On Fri, Jul 26, 2019 at 08:40:43PM +0200, Jernej Skrabec wrote:
> > > > > > > --- 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,
> > > > > > 
> > > > > > 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.
> > > > > 
> > > > > 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.> > 
> > > > 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).
> > > 
> > > 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.
> > 
> > [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.
> >
> 
> 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.

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.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

-- 
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+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/20190729184041.vlvfz3vz3ykhufdk%40pengutronix.de.

Re: Re: [PATCH 4/6] pwm: sun4i: Add support for H6 PWM

[Jernej Škrabec]

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):
> > > Hello,
> > > 
> > > On Tue, Jul 30, 2019 at 12:09:40AM +0800, Chen-Yu Tsai wrote:
> > > > On Tue, Jul 30, 2019 at 12:07 AM Uwe Kleine-König
> > > > 
> > > > <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> > > > > On Mon, Jul 29, 2019 at 05:55:52PM +0200, Jernej Škrabec wrote:
> > > > > > Dne ponedeljek, 29. julij 2019 ob 08:40:30 CEST je Uwe
> > > > > > Kleine-König
> > > > > > 
> > > > > > napisal(a):
> > > > > > > On Fri, Jul 26, 2019 at 08:40:43PM +0200, Jernej Skrabec wrote:
> > > > > > > > --- 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,
> > > > > > > 
> > > > > > > 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.
> > > > > > 
> > > > > > 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.> >
> > > > > 
> > > > > 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).
> > > > 
> > > > 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.
> > > 
> > > [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.
> > 
> > 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.
> 
> 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 

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

> 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.
> 
> Best regards
> Uwe




-- 
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+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/173825848.1FZsmuHfpq%40jernej-laptop.

Re: Re: [PATCH 4/6] pwm: sun4i: Add support for H6 PWM

[Uwe Kleine-König]

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):
> > > > Hello,
> > > > 
> > > > On Tue, Jul 30, 2019 at 12:09:40AM +0800, Chen-Yu Tsai wrote:
> > > > > On Tue, Jul 30, 2019 at 12:07 AM Uwe Kleine-König
> > > > > 
> > > > > <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> > > > > > On Mon, Jul 29, 2019 at 05:55:52PM +0200, Jernej Škrabec wrote:
> > > > > > > Dne ponedeljek, 29. julij 2019 ob 08:40:30 CEST je Uwe
> > > > > > > Kleine-König
> > > > > > > 
> > > > > > > napisal(a):
> > > > > > > > On Fri, Jul 26, 2019 at 08:40:43PM +0200, Jernej Skrabec wrote:
> > > > > > > > > --- 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,
> > > > > > > > 
> > > > > > > > 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.
> > > > > > > 
> > > > > > > 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.> >
> > > > > > 
> > > > > > 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).
> > > > > 
> > > > > 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.
> > > > 
> > > > [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.
> > > 
> > > 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.
> > 
> > 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 
> 
> 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.

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?

Best regards
Uwe


-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

-- 
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+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/20190729185108.tpilwoooxvi2z72e%40pengutronix.de.

Re: Re: [PATCH 4/6] pwm: sun4i: Add support for H6 PWM

[Jernej Škrabec]

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):
> > > > > Hello,
> > > > > 
> > > > > On Tue, Jul 30, 2019 at 12:09:40AM +0800, Chen-Yu Tsai wrote:
> > > > > > On Tue, Jul 30, 2019 at 12:07 AM Uwe Kleine-König
> > > > > > 
> > > > > > <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> > > > > > > On Mon, Jul 29, 2019 at 05:55:52PM +0200, Jernej Škrabec wrote:
> > > > > > > > Dne ponedeljek, 29. julij 2019 ob 08:40:30 CEST je Uwe
> > > > > > > > Kleine-König
> > > > > > > > 
> > > > > > > > napisal(a):
> > > > > > > > > On Fri, Jul 26, 2019 at 08:40:43PM +0200, Jernej Skrabec 
wrote:
> > > > > > > > > > --- 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,
> > > > > > > > > 
> > > > > > > > > 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.
> > > > > > > > 
> > > > > > > > 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.> >
> > > > > > > 
> > > > > > > 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).
> > > > > > 
> > > > > > 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.
> > > > > 
> > > > > [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.
> > > > 
> > > > 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.
> > > 
> > > 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
> > 
> > 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.
> 
> 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?

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


-- 
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+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/2452836.v7ux4bnEjb%40jernej-laptop.

Re: Re: [PATCH 4/6] pwm: sun4i: Add support for H6 PWM

[Uwe Kleine-König]

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):
> > > > > > On Tue, Jul 30, 2019 at 12:09:40AM +0800, Chen-Yu Tsai wrote:
> > > > > > > On Tue, Jul 30, 2019 at 12:07 AM Uwe Kleine-König
> > > > > > > <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> > > > > > > > On Mon, Jul 29, 2019 at 05:55:52PM +0200, Jernej Škrabec wrote:
> > > > > > > > > Dne ponedeljek, 29. julij 2019 ob 08:40:30 CEST je Uwe Kleine-König
> > > > > > > > > napisal(a):
> > > > > > > > > > On Fri, Jul 26, 2019 at 08:40:43PM +0200, Jernej Skrabec wrote:
> > > > > > > > > > > --- 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,
> > > > > > > > > > 
> > > > > > > > > > If you follow my suggestion for the two previous patches,

(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.)

> > > > > > > > > > you can just use:
> > > > > > > > > >
> > > > > > > > > >     compatible = "allwinner,sun50i-h6-pwm", "allwinner,sun5i-a10s-pwm";
> > > > > > > > > > 
> > > > > > > > > > and drop this patch.
> > > > > > > > > 
> > > > > > > > > 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.
> > > > > > > > 
> > > > > > > > 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).
> > > > > > > 
> > > > > > > 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.
> > > > > > 
> > > > > > [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.
> > > > > 
> > > > > 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.
> > > > 
> > > > 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
> > > 
> > > 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.
> > 
> > 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?
> 
> 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.

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

-- 
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+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/20190730080900.hhxrqun7vk4nsj2h%40pengutronix.de.

Re: Re: [PATCH 4/6] pwm: sun4i: Add support for H6 PWM

[Chen-Yu Tsai]

On Tue, Jul 30, 2019 at 4:09 PM Uwe Kleine-König
<u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> 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

> 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):
> > > > > > > On Tue, Jul 30, 2019 at 12:09:40AM +0800, Chen-Yu Tsai wrote:
> > > > > > > > On Tue, Jul 30, 2019 at 12:07 AM Uwe Kleine-König
> > > > > > > > <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> > > > > > > > > On Mon, Jul 29, 2019 at 05:55:52PM +0200, Jernej Škrabec wrote:
> > > > > > > > > > Dne ponedeljek, 29. julij 2019 ob 08:40:30 CEST je Uwe Kleine-König
> > > > > > > > > > napisal(a):
> > > > > > > > > > > On Fri, Jul 26, 2019 at 08:40:43PM +0200, Jernej Skrabec wrote:
> > > > > > > > > > > > --- 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,
> > > > > > > > > > >
> > > > > > > > > > > If you follow my suggestion for the two previous patches,
>
> (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.)
>
> > > > > > > > > > > you can just use:
> > > > > > > > > > >
> > > > > > > > > > >     compatible = "allwinner,sun50i-h6-pwm", "allwinner,sun5i-a10s-pwm";
> > > > > > > > > > >
> > > > > > > > > > > and drop this patch.
> > > > > > > > > >
> > > > > > > > > > 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.
> > > > > > > > >
> > > > > > > > > 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).
> > > > > > > >
> > > > > > > > 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.
> > > > > > >
> > > > > > > [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.
> > > > > >
> > > > > > 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.
> > > > >
> > > > > 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
> > > >
> > > > 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.
> > >
> > > 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?
> >
> > 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.
>
> --
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
>
> --
> 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+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
> To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/20190730080900.hhxrqun7vk4nsj2h%40pengutronix.de.

-- 
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+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/CAGb2v65jFdFZGLti4_B%3D2QPbtrj1b8wh63R5G3NpY_ndpJoV5g%40mail.gmail.com.

Re: [linux-sunxi] Re: [PATCH 4/6] pwm: sun4i: Add support for H6 PWM

[Maxime Ripard]

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.

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.

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.

Maxime

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

Re: Re: [PATCH 4/6] pwm: sun4i: Add support for H6 PWM

[Uwe Kleine-König]

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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

-- 
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+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/20190731065230.mqbtn5sfoxrkevw5%40pengutronix.de.

Re: [linux-sunxi] Re: [PATCH 4/6] pwm: sun4i: Add support for H6 PWM

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 5795 bytes --]

On Wed, Jul 31, 2019 at 08:52:30AM +0200, Uwe Kleine-König wrote:
> 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.

That's true, but it still have nothing to do with validating its
presence vs its content. We never even mentionned the latter.

> > 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?

Indeed

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

The bootloader passed by the bootloader is not meant for Linux but for
another OS, the bootloader loaded a DT not meant for mainline but some
BSP that happen to have the same compatible, the user has applied a
work in progress patch to its DT, and then updates the kernel, the
user applied a poorly written overlay, etc...

We really shouldn't support those cases in the first place, but a
silent lockup of the system is the worst way to treat those errors.

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

We shouldn't assert this *only* in the schema, but if it's cheap and
it can catch some mistakes, then why not?

Worst case scenario, the DTSI will be correct all the time, and it
will never generate any error. Just like 90% of all the constraints in
the schemas.

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

You've been doing SoC support for a while, how many times did this
truly happen to you, whithout a single change to the driver?

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

Fine, let's do that then

Maxime

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: Re: [PATCH 4/6] pwm: sun4i: Add support for H6 PWM

[Uwe Kleine-König]

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.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

-- 
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+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/20190812104700.vzpdxx3yddthiif5%40pengutronix.de.

Re: Re: [PATCH 4/6] pwm: sun4i: Add support for H6 PWM

[Jernej Škrabec]

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.

Best regards,
Jernej

> 
> Best regards
> Uwe




-- 
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+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/2877943.4JjSNuc4x8%40jernej-laptop.

[PATCH 5/6] pwm: sun4i: Add support to output source clock directly

[Jernej Skrabec]

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-gGgVlfcn5nU@public.gmane.org>
---
 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

Re: [PATCH 5/6] pwm: sun4i: Add support to output source clock directly

[Maxime Ripard]

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.skrabec@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.

Maxime

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

Re: [PATCH 5/6] pwm: sun4i: Add support to output source clock directly

[Jernej Škrabec]

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.skrabec-gGgVlfcn5nU@public.gmane.org>
> 
> 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". 

Best regards,
Jernej

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

Re: Re: [PATCH 5/6] pwm: sun4i: Add support to output source clock directly

[Chen-Yu Tsai]

On Sat, Jul 27, 2019 at 10:28 PM Jernej Škrabec <jernej.skrabec@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.skrabec-gGgVlfcn5nU@public.gmane.org>
> >
> > 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

-- 
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+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/CAGb2v65a9jF3QsEQgTim_XxXjhd9K0KwDRxtqYMqsiy2yWLeBg%40mail.gmail.com.

Re: [PATCH 5/6] pwm: sun4i: Add support to output source clock directly

[Uwe Kleine-König]

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.skrabec-gGgVlfcn5nU@public.gmane.org>
> ---
>  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.
> +	 */

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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

-- 
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+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/20190729070605.vlu7kgzn362ph2q3%40pengutronix.de.

Re: [PATCH 5/6] pwm: sun4i: Add support to output source clock directly

[Jernej Škrabec]

Hi Uwe,

Dne ponedeljek, 29. julij 2019 ob 09:06:05 CEST je Uwe Kleine-König 
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.skrabec-gGgVlfcn5nU@public.gmane.org>
> > ---
> > 
> >  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.
> > +	 */
> 
> 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.

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

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




-- 
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+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/2499807.IN78SsLMYo%40jernej-laptop.

Re: [PATCH 5/6] pwm: sun4i: Add support to output source clock directly

[Uwe Kleine-König]

Hello,

On Mon, Jul 29, 2019 at 06:16:55PM +0200, Jernej Škrabec wrote:
> Dne ponedeljek, 29. julij 2019 ob 09:06:05 CEST je Uwe Kleine-König 
> 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.skrabec@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.
> > > +	 */
> > 
> > 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.
> 
> It's the latter, otherwise it wouldn't work for my case. I'll fix the check and 
> skip additional logic.

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.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH 6/6] arm64: dts: allwinner: h6: Add PWM node

[Jernej Skrabec]

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>
---
 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

Re: [PATCH 6/6] arm64: dts: allwinner: h6: Add PWM node

[Maxime Ripard]

On Fri, Jul 26, 2019 at 08:40:45PM +0200, Jernej Skrabec wrote:
> 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>
> ---
>  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";

We always have the bus clock first, so I'd really like to keep
that. We also usually use mod for the second clock, and not the name
of the device itself.

Maxime

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