diff mbox series

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

Message ID 20190726184045.14669-4-jernej.skrabec@siol.net (mailing list archive)
State New, archived
Headers show
Series pwm: sun4i: Add support for Allwinner H6 | expand

Commit Message

Jernej Škrabec July 26, 2019, 6:40 p.m. UTC
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(+)

Comments

Maxime Ripard July 27, 2019, 10:46 a.m. UTC | #1
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
Jernej Škrabec July 27, 2019, 2:15 p.m. UTC | #2
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@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).

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

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

Best regards,
Jernej

> 
> Maxime
> 
> --
> Maxime Ripard, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
Chen-Yu Tsai July 27, 2019, 2:27 p.m. UTC | #3
On Sat, Jul 27, 2019 at 6:46 PM Maxime Ripard <mripard@kernel.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@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).

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
Uwe Kleine-König July 29, 2019, 6:38 a.m. UTC | #4
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
Jernej Škrabec July 29, 2019, 3:48 p.m. UTC | #5
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@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.

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
Uwe Kleine-König July 29, 2019, 4:14 p.m. UTC | #6
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);
	}
 
> 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
Maxime Ripard July 29, 2019, 4:45 p.m. UTC | #7
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
Uwe Kleine-König July 29, 2019, 7:04 p.m. UTC | #8
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
diff mbox series

Patch

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;