[PATCH 0/1] Convert period and duty cycle to u64

[PATCH 0/1] Convert period and duty cycle to u64

[Guru Das Srinagesh]

Reworked the change pushed upstream earlier [1] so as to not add an extension
to an obsolete API. With this change, pwm_ops->apply() can be used to set
pwm_state parameters as usual.

[1] https://lore.kernel.org/lkml/20190916140048.GB7488@ulmo/

Guru Das Srinagesh (1):
  pwm: Convert period and duty cycle to u64

 drivers/pwm/core.c  |  4 ++--
 drivers/pwm/sysfs.c | 10 +++++-----
 include/linux/pwm.h | 16 ++++++++--------
 3 files changed, 15 insertions(+), 15 deletions(-)

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 1/1] pwm: Convert period and duty cycle to u64

[Guru Das Srinagesh]

Because period and duty cycle are defined as ints with units of
nanoseconds, the maximum time duration that can be set is limited to
~2.147 seconds. Change their definitions to u64 so that higher durations
may be set.

Signed-off-by: Guru Das Srinagesh <gurus@codeaurora.org>
---
 drivers/pwm/core.c  |  4 ++--
 drivers/pwm/sysfs.c | 10 +++++-----
 include/linux/pwm.h | 16 ++++++++--------
 3 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 6ad51aa..dc79c03 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -1163,8 +1163,8 @@ static void pwm_dbg_show(struct pwm_chip *chip, struct seq_file *s)
 		if (state.enabled)
 			seq_puts(s, " enabled");
 
-		seq_printf(s, " period: %u ns", state.period);
-		seq_printf(s, " duty: %u ns", state.duty_cycle);
+		seq_printf(s, " period: %llu ns", state.period);
+		seq_printf(s, " duty: %llu ns", state.duty_cycle);
 		seq_printf(s, " polarity: %s",
 			   state.polarity ? "inverse" : "normal");
 
diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c
index 2389b86..3fb1610 100644
--- a/drivers/pwm/sysfs.c
+++ b/drivers/pwm/sysfs.c
@@ -42,7 +42,7 @@ static ssize_t period_show(struct device *child,
 
 	pwm_get_state(pwm, &state);
 
-	return sprintf(buf, "%u\n", state.period);
+	return sprintf(buf, "%llu\n", state.period);
 }
 
 static ssize_t period_store(struct device *child,
@@ -52,10 +52,10 @@ static ssize_t period_store(struct device *child,
 	struct pwm_export *export = child_to_pwm_export(child);
 	struct pwm_device *pwm = export->pwm;
 	struct pwm_state state;
-	unsigned int val;
+	u64 val;
 	int ret;
 
-	ret = kstrtouint(buf, 0, &val);
+	ret = kstrtou64(buf, 0, &val);
 	if (ret)
 		return ret;
 
@@ -77,7 +77,7 @@ static ssize_t duty_cycle_show(struct device *child,
 
 	pwm_get_state(pwm, &state);
 
-	return sprintf(buf, "%u\n", state.duty_cycle);
+	return sprintf(buf, "%llu\n", state.duty_cycle);
 }
 
 static ssize_t duty_cycle_store(struct device *child,
@@ -212,7 +212,7 @@ static ssize_t capture_show(struct device *child,
 	if (ret)
 		return ret;
 
-	return sprintf(buf, "%u %u\n", result.period, result.duty_cycle);
+	return sprintf(buf, "%llu %llu\n", result.period, result.duty_cycle);
 }
 
 static DEVICE_ATTR_RW(period);
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index b2c9c46..1efdd63 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -39,7 +39,7 @@ enum pwm_polarity {
  * current PWM hardware state.
  */
 struct pwm_args {
-	unsigned int period;
+	u64 period;
 	enum pwm_polarity polarity;
 };
 
@@ -56,8 +56,8 @@ enum {
  * @enabled: PWM enabled status
  */
 struct pwm_state {
-	unsigned int period;
-	unsigned int duty_cycle;
+	u64 period;
+	u64 duty_cycle;
 	enum pwm_polarity polarity;
 	bool enabled;
 };
@@ -105,13 +105,13 @@ static inline bool pwm_is_enabled(const struct pwm_device *pwm)
 	return state.enabled;
 }
 
-static inline void pwm_set_period(struct pwm_device *pwm, unsigned int period)
+static inline void pwm_set_period(struct pwm_device *pwm, u64 period)
 {
 	if (pwm)
 		pwm->state.period = period;
 }
 
-static inline unsigned int pwm_get_period(const struct pwm_device *pwm)
+static inline u64 pwm_get_period(const struct pwm_device *pwm)
 {
 	struct pwm_state state;
 
@@ -126,7 +126,7 @@ static inline void pwm_set_duty_cycle(struct pwm_device *pwm, unsigned int duty)
 		pwm->state.duty_cycle = duty;
 }
 
-static inline unsigned int pwm_get_duty_cycle(const struct pwm_device *pwm)
+static inline u64 pwm_get_duty_cycle(const struct pwm_device *pwm)
 {
 	struct pwm_state state;
 
@@ -308,8 +308,8 @@ struct pwm_chip {
  * @duty_cycle: duty cycle of the PWM signal (in nanoseconds)
  */
 struct pwm_capture {
-	unsigned int period;
-	unsigned int duty_cycle;
+	u64 period;
+	u64 duty_cycle;
 };
 
 #if IS_ENABLED(CONFIG_PWM)
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 1/1] pwm: Convert period and duty cycle to u64

[Uwe Kleine-König]

On Tue, Oct 15, 2019 at 07:11:39PM -0700, Guru Das Srinagesh wrote:
> Because period and duty cycle are defined as ints with units of
> nanoseconds, the maximum time duration that can be set is limited to
> ~2.147 seconds. Change their definitions to u64 so that higher durations
> may be set.
> 
> Signed-off-by: Guru Das Srinagesh <gurus@codeaurora.org>

Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Thanks
Uwe

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

Re: [PATCH 1/1] pwm: Convert period and duty cycle to u64

[Thierry Reding]

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

On Tue, Oct 15, 2019 at 07:11:39PM -0700, Guru Das Srinagesh wrote:
> Because period and duty cycle are defined as ints with units of
> nanoseconds, the maximum time duration that can be set is limited to
> ~2.147 seconds. Change their definitions to u64 so that higher durations
> may be set.
> 
> Signed-off-by: Guru Das Srinagesh <gurus@codeaurora.org>
> ---
>  drivers/pwm/core.c  |  4 ++--
>  drivers/pwm/sysfs.c | 10 +++++-----
>  include/linux/pwm.h | 16 ++++++++--------
>  3 files changed, 15 insertions(+), 15 deletions(-)

Applied, thanks.

Thierry

> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index 6ad51aa..dc79c03 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -1163,8 +1163,8 @@ static void pwm_dbg_show(struct pwm_chip *chip, struct seq_file *s)
>  		if (state.enabled)
>  			seq_puts(s, " enabled");
>  
> -		seq_printf(s, " period: %u ns", state.period);
> -		seq_printf(s, " duty: %u ns", state.duty_cycle);
> +		seq_printf(s, " period: %llu ns", state.period);
> +		seq_printf(s, " duty: %llu ns", state.duty_cycle);
>  		seq_printf(s, " polarity: %s",
>  			   state.polarity ? "inverse" : "normal");
>  
> diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c
> index 2389b86..3fb1610 100644
> --- a/drivers/pwm/sysfs.c
> +++ b/drivers/pwm/sysfs.c
> @@ -42,7 +42,7 @@ static ssize_t period_show(struct device *child,
>  
>  	pwm_get_state(pwm, &state);
>  
> -	return sprintf(buf, "%u\n", state.period);
> +	return sprintf(buf, "%llu\n", state.period);
>  }
>  
>  static ssize_t period_store(struct device *child,
> @@ -52,10 +52,10 @@ static ssize_t period_store(struct device *child,
>  	struct pwm_export *export = child_to_pwm_export(child);
>  	struct pwm_device *pwm = export->pwm;
>  	struct pwm_state state;
> -	unsigned int val;
> +	u64 val;
>  	int ret;
>  
> -	ret = kstrtouint(buf, 0, &val);
> +	ret = kstrtou64(buf, 0, &val);
>  	if (ret)
>  		return ret;
>  
> @@ -77,7 +77,7 @@ static ssize_t duty_cycle_show(struct device *child,
>  
>  	pwm_get_state(pwm, &state);
>  
> -	return sprintf(buf, "%u\n", state.duty_cycle);
> +	return sprintf(buf, "%llu\n", state.duty_cycle);
>  }
>  
>  static ssize_t duty_cycle_store(struct device *child,
> @@ -212,7 +212,7 @@ static ssize_t capture_show(struct device *child,
>  	if (ret)
>  		return ret;
>  
> -	return sprintf(buf, "%u %u\n", result.period, result.duty_cycle);
> +	return sprintf(buf, "%llu %llu\n", result.period, result.duty_cycle);
>  }
>  
>  static DEVICE_ATTR_RW(period);
> diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> index b2c9c46..1efdd63 100644
> --- a/include/linux/pwm.h
> +++ b/include/linux/pwm.h
> @@ -39,7 +39,7 @@ enum pwm_polarity {
>   * current PWM hardware state.
>   */
>  struct pwm_args {
> -	unsigned int period;
> +	u64 period;
>  	enum pwm_polarity polarity;
>  };
>  
> @@ -56,8 +56,8 @@ enum {
>   * @enabled: PWM enabled status
>   */
>  struct pwm_state {
> -	unsigned int period;
> -	unsigned int duty_cycle;
> +	u64 period;
> +	u64 duty_cycle;
>  	enum pwm_polarity polarity;
>  	bool enabled;
>  };
> @@ -105,13 +105,13 @@ static inline bool pwm_is_enabled(const struct pwm_device *pwm)
>  	return state.enabled;
>  }
>  
> -static inline void pwm_set_period(struct pwm_device *pwm, unsigned int period)
> +static inline void pwm_set_period(struct pwm_device *pwm, u64 period)
>  {
>  	if (pwm)
>  		pwm->state.period = period;
>  }
>  
> -static inline unsigned int pwm_get_period(const struct pwm_device *pwm)
> +static inline u64 pwm_get_period(const struct pwm_device *pwm)
>  {
>  	struct pwm_state state;
>  
> @@ -126,7 +126,7 @@ static inline void pwm_set_duty_cycle(struct pwm_device *pwm, unsigned int duty)
>  		pwm->state.duty_cycle = duty;
>  }
>  
> -static inline unsigned int pwm_get_duty_cycle(const struct pwm_device *pwm)
> +static inline u64 pwm_get_duty_cycle(const struct pwm_device *pwm)
>  {
>  	struct pwm_state state;
>  
> @@ -308,8 +308,8 @@ struct pwm_chip {
>   * @duty_cycle: duty cycle of the PWM signal (in nanoseconds)
>   */
>  struct pwm_capture {
> -	unsigned int period;
> -	unsigned int duty_cycle;
> +	u64 period;
> +	u64 duty_cycle;
>  };
>  
>  #if IS_ENABLED(CONFIG_PWM)
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

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

Re: [PATCH 1/1] pwm: Convert period and duty cycle to u64

[Thierry Reding]

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

On Tue, Oct 15, 2019 at 07:11:39PM -0700, Guru Das Srinagesh wrote:
> Because period and duty cycle are defined as ints with units of
> nanoseconds, the maximum time duration that can be set is limited to
> ~2.147 seconds. Change their definitions to u64 so that higher durations
> may be set.
> 
> Signed-off-by: Guru Das Srinagesh <gurus@codeaurora.org>
> ---
>  drivers/pwm/core.c  |  4 ++--
>  drivers/pwm/sysfs.c | 10 +++++-----
>  include/linux/pwm.h | 16 ++++++++--------
>  3 files changed, 15 insertions(+), 15 deletions(-)

Actually, we can't do that without further preparatory work. The reason
is that consumers use the period and duty_cycle members in computations
of their own, which lead to errors such as this:

	armv7l-unknown-linux-gnueabihf-ld: drivers/video/backlight/pwm_bl.o: in function `pwm_backlight_probe':
	pwm_bl.c:(.text+0x3b0): undefined reference to `__aeabi_uldivmod'

So I think we need to audit all consumers carefully and make sure that
they use do_div() where necessary to avoid such errors.

Thierry

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

Re: [PATCH 1/1] pwm: Convert period and duty cycle to u64

[Guru Das Srinagesh]

On Wed, Oct 16, 2019 at 12:15:39PM +0200, Thierry Reding wrote:
> On Tue, Oct 15, 2019 at 07:11:39PM -0700, Guru Das Srinagesh wrote:
> > Because period and duty cycle are defined as ints with units of
> > nanoseconds, the maximum time duration that can be set is limited to
> > ~2.147 seconds. Change their definitions to u64 so that higher durations
> > may be set.
> > 
> > Signed-off-by: Guru Das Srinagesh <gurus@codeaurora.org>
> > ---
> >  drivers/pwm/core.c  |  4 ++--
> >  drivers/pwm/sysfs.c | 10 +++++-----
> >  include/linux/pwm.h | 16 ++++++++--------
> >  3 files changed, 15 insertions(+), 15 deletions(-)
> 
> Actually, we can't do that without further preparatory work. The reason
> is that consumers use the period and duty_cycle members in computations
> of their own, which lead to errors such as this:
> 
> 	armv7l-unknown-linux-gnueabihf-ld: drivers/video/backlight/pwm_bl.o: in function `pwm_backlight_probe':
> 	pwm_bl.c:(.text+0x3b0): undefined reference to `__aeabi_uldivmod'
> 
> So I think we need to audit all consumers carefully and make sure that
> they use do_div() where necessary to avoid such errors.
> 
> Thierry

Hi Thierry,

I would like to try doing the preparatory work by fixing the errors seen
in consumers so that this u64 patch may be applied without issues.

Before sending the patch, I tried "make"-ing for arm, arm64 and i386
architectures to check for compilation/linking errors and encountered
none. I see that the above error arises from using a cross-compiler for
arm v7, which I haven't tried yet.

Could you please provide details of the compile tests that you run at
your end? I could then try to reproduce the errors you see in the
consumer drivers and fix them. Please do share any other ideas or
suggestions you may have in this regard.

Thank you.

Guru Das.

Re: [PATCH 1/1] pwm: Convert period and duty cycle to u64

[Thierry Reding]

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

On Wed, Oct 16, 2019 at 11:02:47PM -0700, Guru Das Srinagesh wrote:
> On Wed, Oct 16, 2019 at 12:15:39PM +0200, Thierry Reding wrote:
> > On Tue, Oct 15, 2019 at 07:11:39PM -0700, Guru Das Srinagesh wrote:
> > > Because period and duty cycle are defined as ints with units of
> > > nanoseconds, the maximum time duration that can be set is limited to
> > > ~2.147 seconds. Change their definitions to u64 so that higher durations
> > > may be set.
> > > 
> > > Signed-off-by: Guru Das Srinagesh <gurus@codeaurora.org>
> > > ---
> > >  drivers/pwm/core.c  |  4 ++--
> > >  drivers/pwm/sysfs.c | 10 +++++-----
> > >  include/linux/pwm.h | 16 ++++++++--------
> > >  3 files changed, 15 insertions(+), 15 deletions(-)
> > 
> > Actually, we can't do that without further preparatory work. The reason
> > is that consumers use the period and duty_cycle members in computations
> > of their own, which lead to errors such as this:
> > 
> > 	armv7l-unknown-linux-gnueabihf-ld: drivers/video/backlight/pwm_bl.o: in function `pwm_backlight_probe':
> > 	pwm_bl.c:(.text+0x3b0): undefined reference to `__aeabi_uldivmod'
> > 
> > So I think we need to audit all consumers carefully and make sure that
> > they use do_div() where necessary to avoid such errors.
> > 
> > Thierry
> 
> Hi Thierry,
> 
> I would like to try doing the preparatory work by fixing the errors seen
> in consumers so that this u64 patch may be applied without issues.
> 
> Before sending the patch, I tried "make"-ing for arm, arm64 and i386
> architectures to check for compilation/linking errors and encountered
> none. I see that the above error arises from using a cross-compiler for
> arm v7, which I haven't tried yet.
> 
> Could you please provide details of the compile tests that you run at
> your end? I could then try to reproduce the errors you see in the
> consumer drivers and fix them. Please do share any other ideas or
> suggestions you may have in this regard.

I keep a set of scripts in the pwm/ subdirectory of the following
repository:

	https://github.com/thierryreding/scripts

Typically what I do is run:

	$ /path/to/scripts.git/pwm/build --jobs 13 --color

That requires a bit of setup for the cross-compilers. I have the
following in my ~/.cross-compile file:

	path: $HOME/pbs-stage1/bin:$HOME/toolchain/avr32/bin:$HOME/toolchain/unicore32/bin
	arm: armv7l-unknown-linux-gnueabihf-
	arm64: aarch64-unknown-linux-gnu-
	avr32: avr32-
	blackfin: bfin-unknown-elf-
	mips: mips-linux-gnu-
	unicore32: unicore32-linux-
	riscv: riscv64-linux-gnu-
	x86:
	x86_64:

The blackfin and unicore32 builds are expected to fail because the
blackfin architecture was removed and there's no recent enough kernel
publicly available for unicore32.

The last two entries in .cross-compile indicate that builds are native,
so regular gcc from the build system will be used.

Most of these compilers I've built from scratch using pbs-stage1:

	https://github.com/thierryreding/pbs-stage1

Note that I don't guarantee that that build system works for anyone but
myself, but I'd be happy to hear feedback if you decide to use it. That
said, you can probably find prebuilt toolchains for all of the above in
a number of locations, like:

	https://mirrors.edge.kernel.org/pub/tools/crosstool/

or:

	https://toolchains.bootlin.com/

Thierry

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

Re: [PATCH 1/1] pwm: Convert period and duty cycle to u64

[Guru Das Srinagesh]

Hi Thierry,

On Thu, Oct 17, 2019 at 12:43:13PM +0200, Thierry Reding wrote:
> On Wed, Oct 16, 2019 at 11:02:47PM -0700, Guru Das Srinagesh wrote:
> > On Wed, Oct 16, 2019 at 12:15:39PM +0200, Thierry Reding wrote:
> > > On Tue, Oct 15, 2019 at 07:11:39PM -0700, Guru Das Srinagesh wrote:
> > > > Because period and duty cycle are defined as ints with units of
> > > > nanoseconds, the maximum time duration that can be set is limited to
> > > > ~2.147 seconds. Change their definitions to u64 so that higher durations
> > > > may be set.
> > > > 
> > > > Signed-off-by: Guru Das Srinagesh <gurus@codeaurora.org>
> > > > ---
> > > >  drivers/pwm/core.c  |  4 ++--
> > > >  drivers/pwm/sysfs.c | 10 +++++-----
> > > >  include/linux/pwm.h | 16 ++++++++--------
> > > >  3 files changed, 15 insertions(+), 15 deletions(-)
> > > 
> > > Actually, we can't do that without further preparatory work. The reason
> > > is that consumers use the period and duty_cycle members in computations
> > > of their own, which lead to errors such as this:
> > > 
> > > 	armv7l-unknown-linux-gnueabihf-ld: drivers/video/backlight/pwm_bl.o: in function `pwm_backlight_probe':
> > > 	pwm_bl.c:(.text+0x3b0): undefined reference to `__aeabi_uldivmod'
> > > 
> > > So I think we need to audit all consumers carefully and make sure that
> > > they use do_div() where necessary to avoid such errors.
> > > 
> > > Thierry
> > 
> > Hi Thierry,
> > 
> > I would like to try doing the preparatory work by fixing the errors seen
> > in consumers so that this u64 patch may be applied without issues.
> > 
> > Before sending the patch, I tried "make"-ing for arm, arm64 and i386
> > architectures to check for compilation/linking errors and encountered
> > none. I see that the above error arises from using a cross-compiler for
> > arm v7, which I haven't tried yet.
> > 
> > Could you please provide details of the compile tests that you run at
> > your end? I could then try to reproduce the errors you see in the
> > consumer drivers and fix them. Please do share any other ideas or
> > suggestions you may have in this regard.
> 
> I keep a set of scripts in the pwm/ subdirectory of the following
> repository:
> 
> 	https://github.com/thierryreding/scripts
> 
> Typically what I do is run:
> 
> 	$ /path/to/scripts.git/pwm/build --jobs 13 --color
> 
> That requires a bit of setup for the cross-compilers. I have the
> following in my ~/.cross-compile file:
> 
> 	path: $HOME/pbs-stage1/bin:$HOME/toolchain/avr32/bin:$HOME/toolchain/unicore32/bin
> 	arm: armv7l-unknown-linux-gnueabihf-
> 	arm64: aarch64-unknown-linux-gnu-
> 	avr32: avr32-
> 	blackfin: bfin-unknown-elf-
> 	mips: mips-linux-gnu-
> 	unicore32: unicore32-linux-
> 	riscv: riscv64-linux-gnu-
> 	x86:
> 	x86_64:
> 
> The blackfin and unicore32 builds are expected to fail because the
> blackfin architecture was removed and there's no recent enough kernel
> publicly available for unicore32.
> 
> The last two entries in .cross-compile indicate that builds are native,
> so regular gcc from the build system will be used.
> 
> Most of these compilers I've built from scratch using pbs-stage1:
> 
> 	https://github.com/thierryreding/pbs-stage1
> 
> Note that I don't guarantee that that build system works for anyone but
> myself, but I'd be happy to hear feedback if you decide to use it. That
> said, you can probably find prebuilt toolchains for all of the above in
> a number of locations, like:
> 
> 	https://mirrors.edge.kernel.org/pub/tools/crosstool/
> 
> or:
> 
> 	https://toolchains.bootlin.com/
> 
> Thierry

I tried replicating your compilation setup and found that it worked
right out of the box with no serious issues. I decided to build the
compilers from scratch and only had to update my make to the latest
version and also install help2man and u-boot-tools on my Ubuntu machine.

I found your setup very easy to use on the whole and very well designed.
I added a "set -x" to the /path/to/scripts.git/build/pwm script in order
to figure out how it worked initially. 

I didn't add the lines for unicore32 and blackfin in my ~/.cross-compile
file as you had indicated that they were expected to fail. It was very
convenient for me to run the build command for a specific arch by
appending its name to the end of the command you provided and thus
verify that the compilation errors I was getting were getting fixed for
that arch. Then by simply dropping the architecture's name from the end
I could run the build command for all archs - very cool.

That said, I wasn't able to compile-test avr32 and x86_64. I couldn't
find avr32 in the targets folder of the pbs-stage1 git repo and so
couldn't build it from scratch, and I couldn't find a pre-built version
either. There isn't a config for x86_64 in
/path/to/scripts.git/pwm/configs and so the build/pwm script wasn't
picking it up even though I had added a blank line for it in
~/.cross-compile. Those are the only two issues I encountered with
replicating your setup.

Thank you.

Guru Das.

Re: [PATCH 1/1] pwm: Convert period and duty cycle to u64

[Thierry Reding]

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

On Fri, Nov 15, 2019 at 01:27:45AM -0800, Guru Das Srinagesh wrote:
> Hi Thierry,
> 
> On Thu, Oct 17, 2019 at 12:43:13PM +0200, Thierry Reding wrote:
> > On Wed, Oct 16, 2019 at 11:02:47PM -0700, Guru Das Srinagesh wrote:
> > > On Wed, Oct 16, 2019 at 12:15:39PM +0200, Thierry Reding wrote:
> > > > On Tue, Oct 15, 2019 at 07:11:39PM -0700, Guru Das Srinagesh wrote:
> > > > > Because period and duty cycle are defined as ints with units of
> > > > > nanoseconds, the maximum time duration that can be set is limited to
> > > > > ~2.147 seconds. Change their definitions to u64 so that higher durations
> > > > > may be set.
> > > > > 
> > > > > Signed-off-by: Guru Das Srinagesh <gurus@codeaurora.org>
> > > > > ---
> > > > >  drivers/pwm/core.c  |  4 ++--
> > > > >  drivers/pwm/sysfs.c | 10 +++++-----
> > > > >  include/linux/pwm.h | 16 ++++++++--------
> > > > >  3 files changed, 15 insertions(+), 15 deletions(-)
> > > > 
> > > > Actually, we can't do that without further preparatory work. The reason
> > > > is that consumers use the period and duty_cycle members in computations
> > > > of their own, which lead to errors such as this:
> > > > 
> > > > 	armv7l-unknown-linux-gnueabihf-ld: drivers/video/backlight/pwm_bl.o: in function `pwm_backlight_probe':
> > > > 	pwm_bl.c:(.text+0x3b0): undefined reference to `__aeabi_uldivmod'
> > > > 
> > > > So I think we need to audit all consumers carefully and make sure that
> > > > they use do_div() where necessary to avoid such errors.
> > > > 
> > > > Thierry
> > > 
> > > Hi Thierry,
> > > 
> > > I would like to try doing the preparatory work by fixing the errors seen
> > > in consumers so that this u64 patch may be applied without issues.
> > > 
> > > Before sending the patch, I tried "make"-ing for arm, arm64 and i386
> > > architectures to check for compilation/linking errors and encountered
> > > none. I see that the above error arises from using a cross-compiler for
> > > arm v7, which I haven't tried yet.
> > > 
> > > Could you please provide details of the compile tests that you run at
> > > your end? I could then try to reproduce the errors you see in the
> > > consumer drivers and fix them. Please do share any other ideas or
> > > suggestions you may have in this regard.
> > 
> > I keep a set of scripts in the pwm/ subdirectory of the following
> > repository:
> > 
> > 	https://github.com/thierryreding/scripts
> > 
> > Typically what I do is run:
> > 
> > 	$ /path/to/scripts.git/pwm/build --jobs 13 --color
> > 
> > That requires a bit of setup for the cross-compilers. I have the
> > following in my ~/.cross-compile file:
> > 
> > 	path: $HOME/pbs-stage1/bin:$HOME/toolchain/avr32/bin:$HOME/toolchain/unicore32/bin
> > 	arm: armv7l-unknown-linux-gnueabihf-
> > 	arm64: aarch64-unknown-linux-gnu-
> > 	avr32: avr32-
> > 	blackfin: bfin-unknown-elf-
> > 	mips: mips-linux-gnu-
> > 	unicore32: unicore32-linux-
> > 	riscv: riscv64-linux-gnu-
> > 	x86:
> > 	x86_64:
> > 
> > The blackfin and unicore32 builds are expected to fail because the
> > blackfin architecture was removed and there's no recent enough kernel
> > publicly available for unicore32.
> > 
> > The last two entries in .cross-compile indicate that builds are native,
> > so regular gcc from the build system will be used.
> > 
> > Most of these compilers I've built from scratch using pbs-stage1:
> > 
> > 	https://github.com/thierryreding/pbs-stage1
> > 
> > Note that I don't guarantee that that build system works for anyone but
> > myself, but I'd be happy to hear feedback if you decide to use it. That
> > said, you can probably find prebuilt toolchains for all of the above in
> > a number of locations, like:
> > 
> > 	https://mirrors.edge.kernel.org/pub/tools/crosstool/
> > 
> > or:
> > 
> > 	https://toolchains.bootlin.com/
> > 
> > Thierry
> 
> I tried replicating your compilation setup and found that it worked
> right out of the box with no serious issues. I decided to build the
> compilers from scratch and only had to update my make to the latest
> version and also install help2man and u-boot-tools on my Ubuntu machine.
> 
> I found your setup very easy to use on the whole and very well designed.
> I added a "set -x" to the /path/to/scripts.git/build/pwm script in order
> to figure out how it worked initially. 
> 
> I didn't add the lines for unicore32 and blackfin in my ~/.cross-compile
> file as you had indicated that they were expected to fail. It was very
> convenient for me to run the build command for a specific arch by
> appending its name to the end of the command you provided and thus
> verify that the compilation errors I was getting were getting fixed for
> that arch. Then by simply dropping the architecture's name from the end
> I could run the build command for all archs - very cool.

Great that it's working for you!

> That said, I wasn't able to compile-test avr32 and x86_64. I couldn't
> find avr32 in the targets folder of the pbs-stage1 git repo and so
> couldn't build it from scratch, and I couldn't find a pre-built version
> either.

I use a version that I once downloaded from the Atmel website, though
that was 4 years ago and it doesn't look like they're distributing that
file anymore. I never managed to build a custom version of the cross-
compiler for that because support was never merged into mainline of the
GNU tools.

In any case, we don't really have to worry about that very much because
AVR32 support was dropped in Linux v4.12.

> There isn't a config for x86_64 in
> /path/to/scripts.git/pwm/configs and so the build/pwm script wasn't
> picking it up even though I had added a blank line for it in
> ~/.cross-compile. Those are the only two issues I encountered with
> replicating your setup.

I could probably add a 64-bit x86 configuration to make sure we've got
that covered as well.

Thierry

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