| Message ID | 1454128014-22866-5-git-send-email-drivshin.allworx@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2016-01-30 5:26 GMT+01:00 David Rivshin (Allworx) <drivshin.allworx@gmail.com>: > From: David Rivshin <drivshin@allworx.com> > > After going through the math and constraints checking to compute load and > match values, it is helpful to know what the resultant period and duty > cycle are. > > Signed-off-by: David Rivshin <drivshin@allworx.com> > --- > > I found this helpful while testing the other changes, so I included it in > case it may be of use to someone in the future. I would have no issues with > dropping this if it's not considered worthwhile. It's useful, but converting it as a sysfs attribute would be great ! Neil -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, 30 Jan 2016 15:51:06 +0100 Neil Armstrong <narmstrong@baylibre.com> wrote: > 2016-01-30 5:26 GMT+01:00 David Rivshin (Allworx) > <drivshin.allworx@gmail.com>: > > From: David Rivshin <drivshin@allworx.com> > > > > After going through the math and constraints checking to compute > > load and match values, it is helpful to know what the resultant > > period and duty cycle are. > > > > Signed-off-by: David Rivshin <drivshin@allworx.com> > > --- > > > > I found this helpful while testing the other changes, so I included > > it in case it may be of use to someone in the future. I would have > > no issues with dropping this if it's not considered worthwhile. > > It's useful, but converting it as a sysfs attribute would be great ! Hrm, yes that is an interesting thought. I imagine that many PWM devices have similar constraints, so perhaps that would be best handled generically in the pwm core? Maybe as new optional get_*() ops, a modification to the config() op to add output params, or just updating new fields in the struct pwm directly? -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, * David Rivshin (Allworx) <drivshin.allworx@gmail.com> [160201 10:23]: > On Sat, 30 Jan 2016 15:51:06 +0100 > Neil Armstrong <narmstrong@baylibre.com> wrote: > > > 2016-01-30 5:26 GMT+01:00 David Rivshin (Allworx) > > <drivshin.allworx@gmail.com>: > > > From: David Rivshin <drivshin@allworx.com> > > > > > > After going through the math and constraints checking to compute > > > load and match values, it is helpful to know what the resultant > > > period and duty cycle are. > > > > > > Signed-off-by: David Rivshin <drivshin@allworx.com> > > > --- > > > > > > I found this helpful while testing the other changes, so I included > > > it in case it may be of use to someone in the future. I would have > > > no issues with dropping this if it's not considered worthwhile. > > > > It's useful, but converting it as a sysfs attribute would be great ! > > Hrm, yes that is an interesting thought. I imagine that many PWM > devices have similar constraints, so perhaps that would be best > handled generically in the pwm core? Maybe as new optional get_*() > ops, a modification to the config() op to add output params, or just > updating new fields in the struct pwm directly? Yeah for /sys entry it should be Linux generic. The other option is to use debugfs for driver specific things. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Feb 01, 2016 at 10:59:52AM -0800, Tony Lindgren wrote: > Hi, > > * David Rivshin (Allworx) <drivshin.allworx@gmail.com> [160201 10:23]: > > On Sat, 30 Jan 2016 15:51:06 +0100 > > Neil Armstrong <narmstrong@baylibre.com> wrote: > > > > > 2016-01-30 5:26 GMT+01:00 David Rivshin (Allworx) > > > <drivshin.allworx@gmail.com>: > > > > From: David Rivshin <drivshin@allworx.com> > > > > > > > > After going through the math and constraints checking to compute > > > > load and match values, it is helpful to know what the resultant > > > > period and duty cycle are. > > > > > > > > Signed-off-by: David Rivshin <drivshin@allworx.com> > > > > --- > > > > > > > > I found this helpful while testing the other changes, so I included > > > > it in case it may be of use to someone in the future. I would have > > > > no issues with dropping this if it's not considered worthwhile. > > > > > > It's useful, but converting it as a sysfs attribute would be great ! > > > > Hrm, yes that is an interesting thought. I imagine that many PWM > > devices have similar constraints, so perhaps that would be best > > handled generically in the pwm core? Maybe as new optional get_*() > > ops, a modification to the config() op to add output params, or just > > updating new fields in the struct pwm directly? > > Yeah for /sys entry it should be Linux generic. The other option > is to use debugfs for driver specific things. Let's go with debugfs for this one. This is used for diagnostic purposes and not typically needed when configuring a PWM. While other drivers may compute similar hardware-specific values, there's nothing generic to them. The period and duty cycle are the generic input values and those are already exposed via sysfs. Thierry
On Tue, 2 Feb 2016 17:23:30 +0100 Thierry Reding <thierry.reding@gmail.com> wrote: > On Mon, Feb 01, 2016 at 10:59:52AM -0800, Tony Lindgren wrote: > > Hi, > > > > * David Rivshin (Allworx) <drivshin.allworx@gmail.com> [160201 > > 10:23]: > > > On Sat, 30 Jan 2016 15:51:06 +0100 > > > Neil Armstrong <narmstrong@baylibre.com> wrote: > > > > > > > 2016-01-30 5:26 GMT+01:00 David Rivshin (Allworx) > > > > <drivshin.allworx@gmail.com>: > > > > > From: David Rivshin <drivshin@allworx.com> > > > > > > > > > > After going through the math and constraints checking to > > > > > compute load and match values, it is helpful to know what the > > > > > resultant period and duty cycle are. > > > > > > > > > > Signed-off-by: David Rivshin <drivshin@allworx.com> > > > > > --- > > > > > > > > > > I found this helpful while testing the other changes, so I > > > > > included it in case it may be of use to someone in the > > > > > future. I would have no issues with dropping this if it's not > > > > > considered worthwhile. > > > > > > > > It's useful, but converting it as a sysfs attribute would be > > > > great ! > > > > > > Hrm, yes that is an interesting thought. I imagine that many PWM > > > devices have similar constraints, so perhaps that would be best > > > handled generically in the pwm core? Maybe as new optional > > > get_*() ops, a modification to the config() op to add output > > > params, or just updating new fields in the struct pwm directly? > > > > Yeah for /sys entry it should be Linux generic. The other option > > is to use debugfs for driver specific things. > > Let's go with debugfs for this one. This is used for diagnostic I had looked at using the dbg_show() op to add some additional data. One thing that discouraged me from going down that path was that it replaced the call to pwm_dbg_show() for that chip. I wouldn't want to loose the existing output, as it is useful, but also didn't like the thought of duplicating the logic/formatting. I think some way to have both the standard output and add extra output somewhere (e.g. same line, line after each pwm_device, block after all pwm_devices on the chip) would make that path more attractive. Or were you thinking of a separate (per-chip) debugfs file for this type of information? > purposes and not typically needed when configuring a PWM. While other > drivers may compute similar hardware-specific values, there's nothing > generic to them. The period and duty cycle are the generic input > values and those are already exposed via sysfs. Just to clarify, what I was thinking of as generic was the concept that "period/duty I asked for" and "period/duty I got" may be different, sometimes to a substantial degree. I could imagine userspace wanting to know the latter, which is what I think Neil was suggesting. For the sake of an example, the input clock for a dmtimer is sometimes (often?) 32768Hz, which means that the real period and duty_cycle output are limited to being a multiple of ~61035.2ns (16384Hz). So an attempt to set a period of 100000ns (10KHz) would result in either 61035.2ns, or 122070.4ns (8192Hz), depending on the implementation of the driver (and patch 2 changes behavior from the former to the latter). You can also have cases where you desired a 33% duty cycle, but got a 25% duty cycle instead. In a quick look, I see substantially similar calculations (i.e. clk_rate*period_ns/10^9) in most of the other pwm drivers, which makes sense for any that are programmed in terms of some input clock. This type of calculation almost guarantees that requested and actual period/ duty_cycle are going to be different to some degree. Some drivers look like they have even more complicated behavior, apparently due to other hardware constraints. For instance, fsl-ftm (among others) looks to be using a power-of-2 prescaler to fit the period_cycles into a 16bit field. Of course if the input clock rate for these types of devices is sufficiently high (enough that the desired period is many input clock cycles), then the difference between "desired" and "actual" is probably small enough that noone cares. I'm not sure how common it is that a) there is a substantial difference, and b) userspace cares about it, and c) userspace didn't carefully select an achievable value already If noone has brought it up before, then I'd guess the answer is "not very". -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Feb 02, 2016 at 06:44:43PM -0500, David Rivshin (Allworx) wrote: > On Tue, 2 Feb 2016 17:23:30 +0100 > Thierry Reding <thierry.reding@gmail.com> wrote: > > > On Mon, Feb 01, 2016 at 10:59:52AM -0800, Tony Lindgren wrote: > > > Hi, > > > > > > * David Rivshin (Allworx) <drivshin.allworx@gmail.com> [160201 > > > 10:23]: > > > > On Sat, 30 Jan 2016 15:51:06 +0100 > > > > Neil Armstrong <narmstrong@baylibre.com> wrote: > > > > > > > > > 2016-01-30 5:26 GMT+01:00 David Rivshin (Allworx) > > > > > <drivshin.allworx@gmail.com>: > > > > > > From: David Rivshin <drivshin@allworx.com> > > > > > > > > > > > > After going through the math and constraints checking to > > > > > > compute load and match values, it is helpful to know what the > > > > > > resultant period and duty cycle are. > > > > > > > > > > > > Signed-off-by: David Rivshin <drivshin@allworx.com> > > > > > > --- > > > > > > > > > > > > I found this helpful while testing the other changes, so I > > > > > > included it in case it may be of use to someone in the > > > > > > future. I would have no issues with dropping this if it's not > > > > > > considered worthwhile. > > > > > > > > > > It's useful, but converting it as a sysfs attribute would be > > > > > great ! > > > > > > > > Hrm, yes that is an interesting thought. I imagine that many PWM > > > > devices have similar constraints, so perhaps that would be best > > > > handled generically in the pwm core? Maybe as new optional > > > > get_*() ops, a modification to the config() op to add output > > > > params, or just updating new fields in the struct pwm directly? > > > > > > Yeah for /sys entry it should be Linux generic. The other option > > > is to use debugfs for driver specific things. > > > > Let's go with debugfs for this one. This is used for diagnostic > > I had looked at using the dbg_show() op to add some additional data. > One thing that discouraged me from going down that path was that > it replaced the call to pwm_dbg_show() for that chip. I wouldn't > want to loose the existing output, as it is useful, but also didn't > like the thought of duplicating the logic/formatting. I think some > way to have both the standard output and add extra output somewhere > (e.g. same line, line after each pwm_device, block after all > pwm_devices on the chip) would make that path more attractive. > > Or were you thinking of a separate (per-chip) debugfs file for this > type of information? Yes, generally I'd prefer a separate, chip-specific debugfs file for extra information. To be honest I'm not entirely sure of the usefulness of the pwm_dbg_show() or letting drivers override it. But that's slightly off-topic. However... > > purposes and not typically needed when configuring a PWM. While other > > drivers may compute similar hardware-specific values, there's nothing > > generic to them. The period and duty cycle are the generic input > > values and those are already exposed via sysfs. > > Just to clarify, what I was thinking of as generic was the concept > that "period/duty I asked for" and "period/duty I got" may be > different, sometimes to a substantial degree. I could imagine > userspace wanting to know the latter, which is what I think Neil > was suggesting. > > For the sake of an example, the input clock for a dmtimer is sometimes > (often?) 32768Hz, which means that the real period and duty_cycle output > are limited to being a multiple of ~61035.2ns (16384Hz). So an attempt > to set a period of 100000ns (10KHz) would result in either 61035.2ns, or > 122070.4ns (8192Hz), depending on the implementation of the driver (and > patch 2 changes behavior from the former to the latter). You can also > have cases where you desired a 33% duty cycle, but got a 25% duty cycle > instead. > > In a quick look, I see substantially similar calculations (i.e. > clk_rate*period_ns/10^9) in most of the other pwm drivers, which makes > sense for any that are programmed in terms of some input clock. This > type of calculation almost guarantees that requested and actual period/ > duty_cycle are going to be different to some degree. Some drivers look > like they have even more complicated behavior, apparently due to other > hardware constraints. For instance, fsl-ftm (among others) looks to be > using a power-of-2 prescaler to fit the period_cycles into a 16bit field. > > Of course if the input clock rate for these types of devices is > sufficiently high (enough that the desired period is many input clock > cycles), then the difference between "desired" and "actual" is probably > small enough that noone cares. I'm not sure how common it is that > a) there is a substantial difference, and > b) userspace cares about it, and > c) userspace didn't carefully select an achievable value already > If noone has brought it up before, then I'd guess the answer is "not very". ... I think perhaps a better way yet would be to have drivers update the period and duty cycle to the actual values. That way there won't be a delta between what's really being programmed to hardware and what shows up in sysfs. It would also give users a chance to check if what's been programmed is within an acceptable range or not. Thierry
On Wed, 3 Feb 2016 15:14:54 +0100 Thierry Reding <thierry.reding@gmail.com> wrote: > On Tue, Feb 02, 2016 at 06:44:43PM -0500, David Rivshin (Allworx) > wrote: > > On Tue, 2 Feb 2016 17:23:30 +0100 > > Thierry Reding <thierry.reding@gmail.com> wrote: > > > > > On Mon, Feb 01, 2016 at 10:59:52AM -0800, Tony Lindgren wrote: > > > > Hi, > > > > > > > > * David Rivshin (Allworx) <drivshin.allworx@gmail.com> [160201 > > > > 10:23]: > > > > > On Sat, 30 Jan 2016 15:51:06 +0100 > > > > > Neil Armstrong <narmstrong@baylibre.com> wrote: > > > > > > > > > > > 2016-01-30 5:26 GMT+01:00 David Rivshin (Allworx) > > > > > > <drivshin.allworx@gmail.com>: > > > > > > > From: David Rivshin <drivshin@allworx.com> > > > > > > > > > > > > > > After going through the math and constraints checking to > > > > > > > compute load and match values, it is helpful to know what > > > > > > > the resultant period and duty cycle are. > > > > > > > > > > > > > > Signed-off-by: David Rivshin <drivshin@allworx.com> > > > > > > > --- > > > > > > > > > > > > > > I found this helpful while testing the other changes, so I > > > > > > > included it in case it may be of use to someone in the > > > > > > > future. I would have no issues with dropping this if it's > > > > > > > not considered worthwhile. > > > > > > > > > > > > It's useful, but converting it as a sysfs attribute would be > > > > > > great ! > > > > > > > > > > Hrm, yes that is an interesting thought. I imagine that many > > > > > PWM devices have similar constraints, so perhaps that would > > > > > be best handled generically in the pwm core? Maybe as new > > > > > optional get_*() ops, a modification to the config() op to > > > > > add output params, or just updating new fields in the struct > > > > > pwm directly? > > > > > > > > Yeah for /sys entry it should be Linux generic. The other option > > > > is to use debugfs for driver specific things. > > > > > > Let's go with debugfs for this one. This is used for diagnostic > > > > I had looked at using the dbg_show() op to add some additional > > data. One thing that discouraged me from going down that path was > > that it replaced the call to pwm_dbg_show() for that chip. I > > wouldn't want to loose the existing output, as it is useful, but > > also didn't like the thought of duplicating the logic/formatting. I > > think some way to have both the standard output and add extra > > output somewhere (e.g. same line, line after each pwm_device, block > > after all pwm_devices on the chip) would make that path more > > attractive. > > > > Or were you thinking of a separate (per-chip) debugfs file for this > > type of information? > > Yes, generally I'd prefer a separate, chip-specific debugfs file for Makes sense. > extra information. To be honest I'm not entirely sure of the > usefulness of the pwm_dbg_show() or letting drivers override it. But I did notice that no (in-tree) drivers were currently using pwm_dbg_show()... > that's slightly off-topic. However... > > > > purposes and not typically needed when configuring a PWM. While > > > other drivers may compute similar hardware-specific values, > > > there's nothing generic to them. The period and duty cycle are > > > the generic input values and those are already exposed via > > > sysfs. > > > > Just to clarify, what I was thinking of as generic was the concept > > that "period/duty I asked for" and "period/duty I got" may be > > different, sometimes to a substantial degree. I could imagine > > userspace wanting to know the latter, which is what I think Neil > > was suggesting. > > > > For the sake of an example, the input clock for a dmtimer is > > sometimes (often?) 32768Hz, which means that the real period and > > duty_cycle output are limited to being a multiple of ~61035.2ns > > (16384Hz). So an attempt to set a period of 100000ns (10KHz) would > > result in either 61035.2ns, or 122070.4ns (8192Hz), depending on > > the implementation of the driver (and patch 2 changes behavior from > > the former to the latter). You can also have cases where you > > desired a 33% duty cycle, but got a 25% duty cycle instead. > > > > In a quick look, I see substantially similar calculations (i.e. > > clk_rate*period_ns/10^9) in most of the other pwm drivers, which > > makes sense for any that are programmed in terms of some input > > clock. This type of calculation almost guarantees that requested > > and actual period/ duty_cycle are going to be different to some > > degree. Some drivers look like they have even more complicated > > behavior, apparently due to other hardware constraints. For > > instance, fsl-ftm (among others) looks to be using a power-of-2 > > prescaler to fit the period_cycles into a 16bit field. > > > > Of course if the input clock rate for these types of devices is > > sufficiently high (enough that the desired period is many input > > clock cycles), then the difference between "desired" and "actual" > > is probably small enough that noone cares. I'm not sure how common > > it is that > > a) there is a substantial difference, and > > b) userspace cares about it, and > > c) userspace didn't carefully select an achievable value already > > If noone has brought it up before, then I'd guess the answer is > > "not very". > > ... I think perhaps a better way yet would be to have drivers update > the period and duty cycle to the actual values. That way there won't > be a delta between what's really being programmed to hardware and > what shows up in sysfs. It would also give users a chance to check if > what's been programmed is within an acceptable range or not. From a userspace ABI point of view, I was unsure whether it was considered OK for a R/W sysfs attribute of this nature to read back differently than it was set. To follow the previous example: # echo 100000 > period # cat period 122070 I didn't find any obvious precedents for that. Would it potentially confuse userspace? Is the answer different for new drivers vs existing ones? The obvious alternative would be a separate file (e.g. "actual_period", or somesuch name), which avoids those questions, but I can see how that could be considered less clean. I don't have any preference either way. If just updating the existing period/duty_cycle files with the actual values is the way to go, then there is a second question: should the struct pwm_device track both requested and actual values in separate fields for kernel-internal users? Or should the existing period/duty_cycle fields just be modified to hold the actual values? Either way, I think any changes along these lines would probably be cleaner ontop of the atomic update work (assuming some form of that gets merged). And since it would also be outside the scope of this patchset, you may still want to consider the simple debug message in this patch as at least an interim state. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/05/2016 08:51 PM, David Rivshin (Allworx) wrote: > > Either way, I think any changes along these lines would probably be > cleaner ontop of the atomic update work (assuming some form of that > gets merged). And since it would also be outside the scope of this > patchset, you may still want to consider the simple debug message > in this patch as at least an interim state. > Sure, we should push these patches and rework this one later. Acked-by: Neil Armstrong <narmstrong@baylibre.com> Thanks, Neil -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/pwm/pwm-omap-dmtimer.c b/drivers/pwm/pwm-omap-dmtimer.c index 8c9953c..44e3dca 100644 --- a/drivers/pwm/pwm-omap-dmtimer.c +++ b/drivers/pwm/pwm-omap-dmtimer.c @@ -102,7 +102,8 @@ static int pwm_omap_dmtimer_config(struct pwm_chip *chip, unsigned long clk_rate; bool timer_active; - dev_dbg(chip->dev, "duty cycle: %d, period %d\n", duty_ns, period_ns); + dev_dbg(chip->dev, "requested duty cycle: %dns, period: %dns\n", + duty_ns, period_ns); mutex_lock(&omap->mutex); if (duty_ns == pwm_get_duty_cycle(pwm) && @@ -163,6 +164,12 @@ static int pwm_omap_dmtimer_config(struct pwm_chip *chip, duty_cycles = period_cycles - 1; } + dev_dbg(chip->dev, "effective duty cycle: %lldns, period: %lldns\n", + DIV_ROUND_CLOSEST_ULL((u64)NSEC_PER_SEC * duty_cycles, + clk_rate), + DIV_ROUND_CLOSEST_ULL((u64)NSEC_PER_SEC * period_cycles, + clk_rate)); + load_value = (DM_TIMER_MAX - period_cycles) + 1; match_value = load_value + duty_cycles - 1;