diff mbox series

[v8,12/26] pwm: jz4740: Allow selection of PWM channels 0 and 1

Message ID 20181212220922.18759-13-paul@crapouillou.net (mailing list archive)
State Superseded
Delegated to: Paul Burton
Headers show
Series Ingenic TCU patchset v8 | expand

Commit Message

Paul Cercueil Dec. 12, 2018, 10:09 p.m. UTC
The TCU channels 0 and 1 were previously reserved for system tasks, and
thus unavailable for PWM.

The driver will now only allow a PWM channel to be requested if memory
resources corresponding to the register area of the channel were
supplied to the driver. This allows the TCU channels to be reserved for
system tasks from within the devicetree.

Signed-off-by: Paul Cercueil <paul@crapouillou.net>
---

Notes:
     v6: New patch
    
     v7: No change

     v8: ingenic_tcu_[request,release]_channel are dropped. We now use
         the memory resources provided to the driver to detect whether
	 or not we are allowed to use the TCU channel.

 drivers/pwm/pwm-jz4740.c | 38 +++++++++++++++++++++++++++++++++-----
 1 file changed, 33 insertions(+), 5 deletions(-)

Comments

Uwe Kleine-König Dec. 13, 2018, 9:18 a.m. UTC | #1
On Wed, Dec 12, 2018 at 11:09:07PM +0100, Paul Cercueil wrote:
> The TCU channels 0 and 1 were previously reserved for system tasks, and
> thus unavailable for PWM.
> 
> The driver will now only allow a PWM channel to be requested if memory
> resources corresponding to the register area of the channel were
> supplied to the driver. This allows the TCU channels to be reserved for
> system tasks from within the devicetree.
> 
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>

While there is someone caring for this driver I'd like to complete (a
bit) my picture about the different capabilities and specialities of the
supported PWMs. So I have a few questions:

Is there a publicly available reference manual for this device? (If
yes, adding a link to the driver would be great.)

jz4740_pwm_config looks as if the currently running period isn't
completed before the new config is in effect. Is that correct? If yes,
can this be fixed? A similar question for set_polarity: Does setting the
JZ_TIMER_CTRL_PWM_ACTIVE_LOW bit in the control register take effect
immediately or is this shadowed until the next period starts?

How does the device's output behave after the PWM is disabled?
Does it complete the currently running period? How does the output
behave then? (active/inactive/high/low/high-z?)

> @@ -42,11 +68,7 @@ static int jz4740_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
>  	char clk_name[16];
>  	int ret;
>  
> -	/*
> -	 * Timers 0 and 1 are used for system tasks, so they are unavailable
> -	 * for use as PWMs.
> -	 */
> -	if (pwm->hwpwm < 2)
> +	if (!jz4740_pwm_can_use_chn(jz, pwm->hwpwm))
>  		return -EBUSY;

Maybe EBUSY isn't the best choice here. If the needed register space for
the requested pwm is not included in the memory resources provided to
the device I'd prefer ENXIO or ENODEV.

>  	snprintf(clk_name, sizeof(clk_name), "timer%u", pwm->hwpwm);
> @@ -208,6 +230,12 @@ static int jz4740_pwm_probe(struct platform_device *pdev)
>  		return -EINVAL;
>  	}
>  
> +	jz4740->parent_res = platform_get_resource(
> +				to_platform_device(dev->parent),
> +				IORESOURCE_MEM, 0);
> +	if (!jz4740->parent_res)
> +		return -EINVAL;
> +
>  	jz4740->chip.dev = dev;
>  	jz4740->chip.ops = &jz4740_pwm_ops;
>  	jz4740->chip.npwm = NUM_PWM;

Thanks
Uwe
Paul Cercueil Dec. 13, 2018, 1:58 p.m. UTC | #2
Hi,

Le jeu. 13 déc. 2018 à 10:18, Uwe Kleine-König 
<u.kleine-koenig@pengutronix.de> a écrit :
> On Wed, Dec 12, 2018 at 11:09:07PM +0100, Paul Cercueil wrote:
>>  The TCU channels 0 and 1 were previously reserved for system tasks, 
>> and
>>  thus unavailable for PWM.
>> 
>>  The driver will now only allow a PWM channel to be requested if 
>> memory
>>  resources corresponding to the register area of the channel were
>>  supplied to the driver. This allows the TCU channels to be reserved 
>> for
>>  system tasks from within the devicetree.
>> 
>>  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> 
> While there is someone caring for this driver I'd like to complete (a
> bit) my picture about the different capabilities and specialities of 
> the
> supported PWMs. So I have a few questions:
> 
> Is there a publicly available reference manual for this device? (If
> yes, adding a link to the driver would be great.)

I have them here: https://zcrc.me/~paul/jz_docs/

> jz4740_pwm_config looks as if the currently running period isn't
> completed before the new config is in effect. Is that correct? If yes,
> can this be fixed? A similar question for set_polarity: Does setting 
> the
> JZ_TIMER_CTRL_PWM_ACTIVE_LOW bit in the control register take effect
> immediately or is this shadowed until the next period starts?

I don't really know. We only use this driver for a rumble motor and 
backlight.
Somebody would have to check with a logic analyzer.

> How does the device's output behave after the PWM is disabled?
> Does it complete the currently running period? How does the output
> behave then? (active/inactive/high/low/high-z?)

There's a bit to toggle between "graceful" shutdown (bit clear) and 
"abrupt"
shutdown (bit set). TCSR bit 9. I think that graceful shutdown will 
complete
the running period, then keep the level active. Abrupt shutdown will 
keep the
current level of the line.

>>  @@ -42,11 +68,7 @@ static int jz4740_pwm_request(struct pwm_chip 
>> *chip, struct pwm_device *pwm)
>>   	char clk_name[16];
>>   	int ret;
>> 
>>  -	/*
>>  -	 * Timers 0 and 1 are used for system tasks, so they are 
>> unavailable
>>  -	 * for use as PWMs.
>>  -	 */
>>  -	if (pwm->hwpwm < 2)
>>  +	if (!jz4740_pwm_can_use_chn(jz, pwm->hwpwm))
>>   		return -EBUSY;
> 
> Maybe EBUSY isn't the best choice here. If the needed register space 
> for
> the requested pwm is not included in the memory resources provided to
> the device I'd prefer ENXIO or ENODEV.

The idea was that if we don't get the register space we need, that means
the channel is used for something else, hence the EBUSY. Should I switch
it to ENXIO?

>>   	snprintf(clk_name, sizeof(clk_name), "timer%u", pwm->hwpwm);
>>  @@ -208,6 +230,12 @@ static int jz4740_pwm_probe(struct 
>> platform_device *pdev)
>>   		return -EINVAL;
>>   	}
>> 
>>  +	jz4740->parent_res = platform_get_resource(
>>  +				to_platform_device(dev->parent),
>>  +				IORESOURCE_MEM, 0);
>>  +	if (!jz4740->parent_res)
>>  +		return -EINVAL;
>>  +
>>   	jz4740->chip.dev = dev;
>>   	jz4740->chip.ops = &jz4740_pwm_ops;
>>   	jz4740->chip.npwm = NUM_PWM;
> 
> Thanks
> Uwe
> 
> --
> Pengutronix e.K.                           | Uwe Kleine-König        
>     |
> Industrial Linux Solutions                 | 
> http://www.pengutronix.de/  |
Uwe Kleine-König Dec. 13, 2018, 8:32 p.m. UTC | #3
On Thu, Dec 13, 2018 at 02:58:31PM +0100, Paul Cercueil wrote:
> Hi,
> 
> Le jeu. 13 déc. 2018 à 10:18, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> a écrit :
> > On Wed, Dec 12, 2018 at 11:09:07PM +0100, Paul Cercueil wrote:
> > >  The TCU channels 0 and 1 were previously reserved for system tasks,
> > > and
> > >  thus unavailable for PWM.
> > > 
> > >  The driver will now only allow a PWM channel to be requested if
> > > memory
> > >  resources corresponding to the register area of the channel were
> > >  supplied to the driver. This allows the TCU channels to be reserved
> > > for
> > >  system tasks from within the devicetree.
> > > 
> > >  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> > 
> > While there is someone caring for this driver I'd like to complete (a
> > bit) my picture about the different capabilities and specialities of the
> > supported PWMs. So I have a few questions:
> > 
> > Is there a publicly available reference manual for this device? (If
> > yes, adding a link to the driver would be great.)
> 
> I have them here: https://zcrc.me/~paul/jz_docs/

Is this link good enough to add it to the driver? From a quick view I'd
say this is another pwm implementation that gets active on pwm_disable().
Can you confirm this?

> > jz4740_pwm_config looks as if the currently running period isn't
> > completed before the new config is in effect. Is that correct? If yes,
> > can this be fixed? A similar question for set_polarity: Does setting the
> > JZ_TIMER_CTRL_PWM_ACTIVE_LOW bit in the control register take effect
> > immediately or is this shadowed until the next period starts?
> 
> I don't really know. We only use this driver for a rumble motor and
> backlight.
> Somebody would have to check with a logic analyzer.

depending on the possible timings you might also be able to test this
e.g. by setting:

	duty_cycle=1ms, period=5s

and then

	duty_cycle=5s, period=5s

. If the implementation is right your display should be dark for nearly
5 seconds. (And the second call to pwm_apply should also block until the
display is on.)

> > How does the device's output behave after the PWM is disabled?
> > Does it complete the currently running period? How does the output
> > behave then? (active/inactive/high/low/high-z?)
> 
> There's a bit to toggle between "graceful" shutdown (bit clear) and "abrupt"
> shutdown (bit set). TCSR bit 9. I think that graceful shutdown will complete
> the running period, then keep the level active. Abrupt shutdown will keep
> the
> current level of the line.

Can you confirm the things you think above? I'd like to have them
eventually documented in the driver.

> > >  @@ -42,11 +68,7 @@ static int jz4740_pwm_request(struct pwm_chip
> > > *chip, struct pwm_device *pwm)
> > >   	char clk_name[16];
> > >   	int ret;
> > > 
> > >  -	/*
> > >  -	 * Timers 0 and 1 are used for system tasks, so they are
> > > unavailable
> > >  -	 * for use as PWMs.
> > >  -	 */
> > >  -	if (pwm->hwpwm < 2)
> > >  +	if (!jz4740_pwm_can_use_chn(jz, pwm->hwpwm))
> > >   		return -EBUSY;
> > 
> > Maybe EBUSY isn't the best choice here. If the needed register space for
> > the requested pwm is not included in the memory resources provided to
> > the device I'd prefer ENXIO or ENODEV.
> 
> The idea was that if we don't get the register space we need, that means
> the channel is used for something else, hence the EBUSY. Should I switch
> it to ENXIO?

I understand your reasoning, but I think it's misleading. If I get
-EBUSY from a PWM driver I'd start searching for another user of said
resource. With ENXIO or ENODEV it's more obvious that the driver isn't
responsible for the resource that was requested.

Best regards
Uwe
Paul Cercueil Dec. 16, 2018, 1:36 p.m. UTC | #4
Hi,

Le jeu. 13 déc. 2018 à 21:32, Uwe Kleine-König 
<u.kleine-koenig@pengutronix.de> a écrit :
> On Thu, Dec 13, 2018 at 02:58:31PM +0100, Paul Cercueil wrote:
>>  Hi,
>> 
>>  Le jeu. 13 déc. 2018 à 10:18, Uwe Kleine-König
>>  <u.kleine-koenig@pengutronix.de> a écrit :
>>  > On Wed, Dec 12, 2018 at 11:09:07PM +0100, Paul Cercueil wrote:
>>  > >  The TCU channels 0 and 1 were previously reserved for system 
>> tasks,
>>  > > and
>>  > >  thus unavailable for PWM.
>>  > >
>>  > >  The driver will now only allow a PWM channel to be requested if
>>  > > memory
>>  > >  resources corresponding to the register area of the channel 
>> were
>>  > >  supplied to the driver. This allows the TCU channels to be 
>> reserved
>>  > > for
>>  > >  system tasks from within the devicetree.
>>  > >
>>  > >  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
>>  >
>>  > While there is someone caring for this driver I'd like to 
>> complete (a
>>  > bit) my picture about the different capabilities and specialities 
>> of the
>>  > supported PWMs. So I have a few questions:
>>  >
>>  > Is there a publicly available reference manual for this device? 
>> (If
>>  > yes, adding a link to the driver would be great.)
>> 
>>  I have them here: https://zcrc.me/~paul/jz_docs/
> 
> Is this link good enough to add it to the driver? From a quick view 
> I'd
> say this is another pwm implementation that gets active on 
> pwm_disable().
> Can you confirm this?

It's my website, so if these get moved, I can update the link.

What do you mean it gets active on pwm_disable()? If pwm_disable() gets 
called
the PWM line goes back to inactive state, which is what it should do.

>>  > jz4740_pwm_config looks as if the currently running period isn't
>>  > completed before the new config is in effect. Is that correct? If 
>> yes,
>>  > can this be fixed? A similar question for set_polarity: Does 
>> setting the
>>  > JZ_TIMER_CTRL_PWM_ACTIVE_LOW bit in the control register take 
>> effect
>>  > immediately or is this shadowed until the next period starts?
>> 
>>  I don't really know. We only use this driver for a rumble motor and
>>  backlight.
>>  Somebody would have to check with a logic analyzer.
> 
> depending on the possible timings you might also be able to test this
> e.g. by setting:
> 
> 	duty_cycle=1ms, period=5s
> 
> and then
> 
> 	duty_cycle=5s, period=5s
> 
> . If the implementation is right your display should be dark for 
> nearly
> 5 seconds. (And the second call to pwm_apply should also block until 
> the
> display is on.)

So it switches to full ON as soon as I set the duty cycle to 5s. Same 
for
the polarity, it is updated as soon as the register is written. So the
registers are not shadowed.

>>  > How does the device's output behave after the PWM is disabled?
>>  > Does it complete the currently running period? How does the output
>>  > behave then? (active/inactive/high/low/high-z?)
>> 
>>  There's a bit to toggle between "graceful" shutdown (bit clear) and 
>> "abrupt"
>>  shutdown (bit set). TCSR bit 9. I think that graceful shutdown will 
>> complete
>>  the running period, then keep the level active. Abrupt shutdown 
>> will keep
>>  the
>>  current level of the line.
> 
> Can you confirm the things you think above? I'd like to have them
> eventually documented in the driver.

 From what I can see, with "abrupt" shutdown the line will always 
return to
its inactive state (be it low or high, depending on the polarity). 
Setting
this bit to "graceful" shutdown, the only difference is that the 
hardware
will keep its current state, active or inactive. That's why we use the
"abrupt" shutdown in the PWM driver.

>>  > >  @@ -42,11 +68,7 @@ static int jz4740_pwm_request(struct 
>> pwm_chip
>>  > > *chip, struct pwm_device *pwm)
>>  > >   	char clk_name[16];
>>  > >   	int ret;
>>  > >
>>  > >  -	/*
>>  > >  -	 * Timers 0 and 1 are used for system tasks, so they are
>>  > > unavailable
>>  > >  -	 * for use as PWMs.
>>  > >  -	 */
>>  > >  -	if (pwm->hwpwm < 2)
>>  > >  +	if (!jz4740_pwm_can_use_chn(jz, pwm->hwpwm))
>>  > >   		return -EBUSY;
>>  >
>>  > Maybe EBUSY isn't the best choice here. If the needed register 
>> space for
>>  > the requested pwm is not included in the memory resources 
>> provided to
>>  > the device I'd prefer ENXIO or ENODEV.
>> 
>>  The idea was that if we don't get the register space we need, that 
>> means
>>  the channel is used for something else, hence the EBUSY. Should I 
>> switch
>>  it to ENXIO?
> 
> I understand your reasoning, but I think it's misleading. If I get
> -EBUSY from a PWM driver I'd start searching for another user of said
> resource. With ENXIO or ENODEV it's more obvious that the driver isn't
> responsible for the resource that was requested.

OK.

> Best regards
> Uwe
> 
> --
> Pengutronix e.K.                           | Uwe Kleine-König        
>     |
> Industrial Linux Solutions                 | 
> http://www.pengutronix.de/  |
Uwe Kleine-König Dec. 17, 2018, 7:43 a.m. UTC | #5
Hello Paul,

On Sun, Dec 16, 2018 at 02:36:03PM +0100, Paul Cercueil wrote:
> Le jeu. 13 déc. 2018 à 21:32, Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> a écrit :
> > On Thu, Dec 13, 2018 at 02:58:31PM +0100, Paul Cercueil wrote:
> > >  Hi,
> > > 
> > >  Le jeu. 13 déc. 2018 à 10:18, Uwe Kleine-König
> > >  <u.kleine-koenig@pengutronix.de> a écrit :
> > >  > On Wed, Dec 12, 2018 at 11:09:07PM +0100, Paul Cercueil wrote:
> > >  > >  The TCU channels 0 and 1 were previously reserved for system
> > > tasks,
> > >  > > and
> > >  > >  thus unavailable for PWM.
> > >  > >
> > >  > >  The driver will now only allow a PWM channel to be requested if
> > >  > > memory
> > >  > >  resources corresponding to the register area of the channel
> > > were
> > >  > >  supplied to the driver. This allows the TCU channels to be
> > > reserved
> > >  > > for
> > >  > >  system tasks from within the devicetree.
> > >  > >
> > >  > >  Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> > >  >
> > >  > While there is someone caring for this driver I'd like to
> > > complete (a
> > >  > bit) my picture about the different capabilities and specialities
> > > of the
> > >  > supported PWMs. So I have a few questions:
> > >  >
> > >  > Is there a publicly available reference manual for this device?
> > > (If
> > >  > yes, adding a link to the driver would be great.)
> > > 
> > >  I have them here: https://zcrc.me/~paul/jz_docs/
> > 
> > Is this link good enough to add it to the driver? From a quick view I'd
> > say this is another pwm implementation that gets active on
> > pwm_disable().
> > Can you confirm this?
> 
> It's my website, so if these get moved, I can update the link.
> 
> What do you mean it gets active on pwm_disable()? If pwm_disable() gets
> called the PWM line goes back to inactive state, which is what it
> should do.

The register description for TCSRn.PWM_EN reads:

	1: PWM pin output enable
	0: PWM pin output disable, and the PWM pin will be set to the
	   initial level according to INITL

As I read the manual (but that differes from the driver) you should use
INITL=1 for an uninverted PWM such that the period starts with the
output being 1. And with that I'd expect that the output goes high on
disable. Given that the driver inverts this and sets INITL (called
JZ_TIMER_CTRL_PWM_ACTIVE_LOW in the driver) for an inverted pwm that
behaviour is ok. With this approach the bug is not the level when
pwm_disable was called but that the period doesn't start with the active
part of the period.

> > >  > jz4740_pwm_config looks as if the currently running period isn't
> > >  > completed before the new config is in effect. Is that correct? If
> > > yes,
> > >  > can this be fixed? A similar question for set_polarity: Does
> > > setting the
> > >  > JZ_TIMER_CTRL_PWM_ACTIVE_LOW bit in the control register take
> > > effect
> > >  > immediately or is this shadowed until the next period starts?
> > > 
> > >  I don't really know. We only use this driver for a rumble motor and
> > >  backlight.
> > >  Somebody would have to check with a logic analyzer.
> > 
> > depending on the possible timings you might also be able to test this
> > e.g. by setting:
> > 
> > 	duty_cycle=1ms, period=5s
> > 
> > and then
> > 
> > 	duty_cycle=5s, period=5s
> > 
> > . If the implementation is right your display should be dark for nearly
> > 5 seconds. (And the second call to pwm_apply should also block until the
> > display is on.)
> 
> So it switches to full ON as soon as I set the duty cycle to 5s. Same for
> the polarity, it is updated as soon as the register is written. So the
> registers are not shadowed.

Then I think the right thing to do is to gracefully disable the hardware
on a duty/period change first to ensure the currently running period is
completed before the next configuration gets active.

> > >  > How does the device's output behave after the PWM is disabled?
> > >  > Does it complete the currently running period? How does the output
> > >  > behave then? (active/inactive/high/low/high-z?)
> > > 
> > >  There's a bit to toggle between "graceful" shutdown (bit clear) and
> > > "abrupt"
> > >  shutdown (bit set). TCSR bit 9. I think that graceful shutdown will
> > > complete
> > >  the running period, then keep the level active. Abrupt shutdown
> > > will keep
> > >  the
> > >  current level of the line.
> > 
> > Can you confirm the things you think above? I'd like to have them
> > eventually documented in the driver.
> 
> From what I can see, with "abrupt" shutdown the line will always return to
> its inactive state (be it low or high, depending on the polarity). Setting
> this bit to "graceful" shutdown, the only difference is that the hardware
> will keep its current state, active or inactive. That's why we use the
> "abrupt" shutdown in the PWM driver.

That sounds backwards from your last mail and the reference manual. As I
read the manual "graceful" means the period is completed until FULL
matches. And I understand "aprupt" as "immediatly freeze". If this is
the case the names in the manual are well chosen.

For the pwm framework the intended behaviour is the graceful one. (For
both, disable and duty/period change.)

Best regards
Uwe
diff mbox series

Patch

diff --git a/drivers/pwm/pwm-jz4740.c b/drivers/pwm/pwm-jz4740.c
index 6b865c14f789..7b12e5628f4f 100644
--- a/drivers/pwm/pwm-jz4740.c
+++ b/drivers/pwm/pwm-jz4740.c
@@ -28,6 +28,7 @@  struct jz4740_pwm_chip {
 	struct pwm_chip chip;
 	struct clk *clks[NUM_PWM];
 	struct regmap *map;
+	struct resource *parent_res;
 };
 
 static inline struct jz4740_pwm_chip *to_jz4740(struct pwm_chip *chip)
@@ -35,6 +36,31 @@  static inline struct jz4740_pwm_chip *to_jz4740(struct pwm_chip *chip)
 	return container_of(chip, struct jz4740_pwm_chip, chip);
 }
 
+static bool jz4740_pwm_can_use_chn(struct jz4740_pwm_chip *jz, unsigned int chn)
+{
+	struct platform_device *pdev = to_platform_device(jz->chip.dev);
+	struct resource chn_res, *res;
+	unsigned int i;
+
+	chn_res.start = jz->parent_res->start + TCU_REG_TDFRc(chn);
+	chn_res.end = chn_res.start + TCU_CHANNEL_STRIDE - 1;
+	chn_res.flags = IORESOURCE_MEM;
+
+	/*
+	 * Walk the list of resources, find if there's one that contains the
+	 * registers for the requested TCU channel
+	 */
+	for (i = 0; ; i++) {
+		res = platform_get_resource(pdev, IORESOURCE_MEM, i);
+		if (!res)
+			break;
+		if (resource_contains(res, &chn_res))
+			return true;
+	}
+
+	return false;
+}
+
 static int jz4740_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
 {
 	struct jz4740_pwm_chip *jz = to_jz4740(chip);
@@ -42,11 +68,7 @@  static int jz4740_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
 	char clk_name[16];
 	int ret;
 
-	/*
-	 * Timers 0 and 1 are used for system tasks, so they are unavailable
-	 * for use as PWMs.
-	 */
-	if (pwm->hwpwm < 2)
+	if (!jz4740_pwm_can_use_chn(jz, pwm->hwpwm))
 		return -EBUSY;
 
 	snprintf(clk_name, sizeof(clk_name), "timer%u", pwm->hwpwm);
@@ -208,6 +230,12 @@  static int jz4740_pwm_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
+	jz4740->parent_res = platform_get_resource(
+				to_platform_device(dev->parent),
+				IORESOURCE_MEM, 0);
+	if (!jz4740->parent_res)
+		return -EINVAL;
+
 	jz4740->chip.dev = dev;
 	jz4740->chip.ops = &jz4740_pwm_ops;
 	jz4740->chip.npwm = NUM_PWM;