| Message ID | 20211017013343.3385923-4-david@lechnology.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | counter: ti-eqep: implement features for speed measurement | expand |
On Sat, 16 Oct 2021 20:33:38 -0500 David Lechner <david@lechnology.com> wrote: > This adds support to the TI eQEP counter driver for the Unit Timer. > The Unit Timer is a device-level extension that provides a timer to be > used for speed calculations. The sysfs interface for the Unit Timer is > new and will be documented in a later commit. It contains a R/W time > attribute for the current time, a R/W period attribute for the timeout > period and a R/W enable attribute to start/stop the timer. It also > implements a timeout event on the chrdev interface that is triggered > each time the period timeout is reached. > > Signed-off-by: David Lechner <david@lechnology.com> No comments on the interface in here as leaving that for William / later. A few minor comments on the implementation. Thanks, Jonathan > --- > drivers/counter/ti-eqep.c | 132 ++++++++++++++++++++++++++++++++++- > include/uapi/linux/counter.h | 2 + > 2 files changed, 133 insertions(+), 1 deletion(-) ... > +static int ti_eqep_unit_timer_time_write(struct counter_device *counter, > + u64 value) > +{ > + struct ti_eqep_cnt *priv = counter->priv; > + u32 qutmr; > + > + /* convert nanoseconds to timer ticks */ > + qutmr = value = mul_u64_u32_div(value, priv->sysclkout_rate, NSEC_PER_SEC); Hmm. This pattern strikes me as 'too clever' and also likely to trip up static checkers who will moan about the truncation if they don't understand this trick. I think I'd prefer you just put the answer in an u64 and then do a simple bounds check before casting down. > + if (qutmr != value) > + return -ERANGE; > + > + regmap_write(priv->regmap32, QUTMR, qutmr); > + > + return 0; > +} > + ... > static irqreturn_t ti_eqep_irq_handler(int irq, void *dev_id) > { > struct ti_eqep_cnt *priv = dev_id; > @@ -474,6 +580,8 @@ static irqreturn_t ti_eqep_irq_handler(int irq, void *dev_id) > if (qflg & QFLG_QDC) > counter_push_event(counter, COUNTER_EVENT_DIRECTION_CHANGE, 0); > > + if (qflg & QFLG_UTO) > + counter_push_event(counter, COUNTER_EVENT_TIMEOUT, 0); > > regmap_set_bits(priv->regmap16, QCLR, ~0); > > @@ -500,6 +608,7 @@ static int ti_eqep_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > struct ti_eqep_cnt *priv; > + struct clk *clk; > void __iomem *base; > int err; > int irq; > @@ -508,6 +617,24 @@ static int ti_eqep_probe(struct platform_device *pdev) > if (!priv) > return -ENOMEM; > > + clk = devm_clk_get(dev, "sysclkout"); > + if (IS_ERR(clk)) { > + if (PTR_ERR(clk) != -EPROBE_DEFER) > + dev_err(dev, "failed to get sysclkout"); dev_err_probe() which both removes most of this boilerplate and stashes the reason for the deferred probe such that it can be checked when debugging. > + return PTR_ERR(clk); > + } No need to enable the clock? > + > + priv->sysclkout_rate = clk_get_rate(clk); > + if (priv->sysclkout_rate == 0) { > + dev_err(dev, "failed to get sysclkout rate"); > + /* prevent divide by zero */ > + priv->sysclkout_rate = 1; > + /* > + * This error is not expected and the driver is mostly usable > + * without clock rate anyway, so don't exit here. > + */ > + } > + > > /**
On Sat, Oct 16, 2021 at 08:33:38PM -0500, David Lechner wrote: > This adds support to the TI eQEP counter driver for the Unit Timer. > The Unit Timer is a device-level extension that provides a timer to be > used for speed calculations. The sysfs interface for the Unit Timer is > new and will be documented in a later commit. It contains a R/W time > attribute for the current time, a R/W period attribute for the timeout > period and a R/W enable attribute to start/stop the timer. It also > implements a timeout event on the chrdev interface that is triggered > each time the period timeout is reached. > > Signed-off-by: David Lechner <david@lechnology.com> I'll comment on the sysfs interface in the respective docs patch. Some comments regarding this patch below. > --- > drivers/counter/ti-eqep.c | 132 ++++++++++++++++++++++++++++++++++- > include/uapi/linux/counter.h | 2 + > 2 files changed, 133 insertions(+), 1 deletion(-) > > diff --git a/drivers/counter/ti-eqep.c b/drivers/counter/ti-eqep.c > index 9881e5115da6..a4a5a4486329 100644 > --- a/drivers/counter/ti-eqep.c > +++ b/drivers/counter/ti-eqep.c > @@ -6,6 +6,7 @@ > */ > > #include <linux/bitops.h> > +#include <linux/clk.h> > #include <linux/counter.h> > #include <linux/interrupt.h> > #include <linux/kernel.h> > @@ -131,6 +132,7 @@ enum ti_eqep_count_func { > > struct ti_eqep_cnt { > struct counter_device counter; > + unsigned long sysclkout_rate; > struct regmap *regmap32; > struct regmap *regmap16; > }; > @@ -298,6 +300,9 @@ static int ti_eqep_events_configure(struct counter_device *counter) > case COUNTER_EVENT_DIRECTION_CHANGE: > qeint |= QEINT_QDC; > break; > + case COUNTER_EVENT_TIMEOUT: > + qeint |= QEINT_UTO; > + break; > } > } > > @@ -311,6 +316,7 @@ static int ti_eqep_watch_validate(struct counter_device *counter, > case COUNTER_EVENT_OVERFLOW: > case COUNTER_EVENT_UNDERFLOW: > case COUNTER_EVENT_DIRECTION_CHANGE: > + case COUNTER_EVENT_TIMEOUT: > return 0; > default: > return -EINVAL; > @@ -457,6 +463,106 @@ static struct counter_count ti_eqep_counts[] = { > }, > }; > > +static int ti_eqep_unit_timer_time_read(struct counter_device *counter, > + u64 *value) > +{ > + struct ti_eqep_cnt *priv = counter->priv; > + u32 qutmr; > + > + regmap_read(priv->regmap32, QUTMR, &qutmr); > + > + /* convert timer ticks to nanoseconds */ > + *value = mul_u64_u32_div(qutmr, NSEC_PER_SEC, priv->sysclkout_rate); > + > + return 0; > +} > + > +static int ti_eqep_unit_timer_time_write(struct counter_device *counter, > + u64 value) > +{ > + struct ti_eqep_cnt *priv = counter->priv; > + u32 qutmr; > + > + /* convert nanoseconds to timer ticks */ > + qutmr = value = mul_u64_u32_div(value, priv->sysclkout_rate, NSEC_PER_SEC); > + if (qutmr != value) > + return -ERANGE; > + > + regmap_write(priv->regmap32, QUTMR, qutmr); > + > + return 0; > +} > + > +static int ti_eqep_unit_timer_period_read(struct counter_device *counter, > + u64 *value) > +{ > + struct ti_eqep_cnt *priv = counter->priv; > + u32 quprd; > + > + regmap_read(priv->regmap32, QUPRD, &quprd); > + > + /* convert timer ticks to nanoseconds */ > + *value = mul_u64_u32_div(quprd, NSEC_PER_SEC, priv->sysclkout_rate); > + > + return 0; > +} > + > +static int ti_eqep_unit_timer_period_write(struct counter_device *counter, > + u64 value) > +{ > + struct ti_eqep_cnt *priv = counter->priv; > + u32 quprd; > + > + /* convert nanoseconds to timer ticks */ > + quprd = value = mul_u64_u32_div(value, priv->sysclkout_rate, NSEC_PER_SEC); > + if (quprd != value) > + return -ERANGE; > + > + /* protect against infinite unit timeout interrupts */ > + if (quprd == 0) > + return -EINVAL; I doubt there's any practical reason for a user to set the timer period to 0, but perhaps we should not try to protect users from themselves here. It's very obvious and expected that setting the timer period to 0 results in timeouts as quickly as possible, so really the user should be left to reap the fruits of their decision regardless of how asinine that decision is. > + > + regmap_write(priv->regmap32, QUPRD, quprd); > + > + return 0; > +} > + > +static int ti_eqep_unit_timer_enable_read(struct counter_device *counter, > + u8 *value) > +{ > + struct ti_eqep_cnt *priv = counter->priv; > + u32 qepctl; > + > + regmap_read(priv->regmap16, QEPCTL, &qepctl); > + *value = !!(qepctl & QEPCTL_UTE); > + > + return 0; > +} > + > +static int ti_eqep_unit_timer_enable_write(struct counter_device *counter, > + u8 value) > +{ > + struct ti_eqep_cnt *priv = counter->priv; > + > + if (value) > + regmap_set_bits(priv->regmap16, QEPCTL, QEPCTL_UTE); > + else > + regmap_clear_bits(priv->regmap16, QEPCTL, QEPCTL_UTE); > + > + return 0; > +} > + > +static struct counter_comp ti_eqep_device_ext[] = { > + COUNTER_COMP_DEVICE_U64("unit_timer_time", ti_eqep_unit_timer_time_read, > + ti_eqep_unit_timer_time_write), > + COUNTER_COMP_DEVICE_U64("unit_timer_period", > + ti_eqep_unit_timer_period_read, > + ti_eqep_unit_timer_period_write), > + COUNTER_COMP_DEVICE_BOOL("unit_timer_enable", > + ti_eqep_unit_timer_enable_read, > + ti_eqep_unit_timer_enable_write), > +}; > + > static irqreturn_t ti_eqep_irq_handler(int irq, void *dev_id) > { > struct ti_eqep_cnt *priv = dev_id; > @@ -474,6 +580,8 @@ static irqreturn_t ti_eqep_irq_handler(int irq, void *dev_id) > if (qflg & QFLG_QDC) > counter_push_event(counter, COUNTER_EVENT_DIRECTION_CHANGE, 0); > > + if (qflg & QFLG_UTO) > + counter_push_event(counter, COUNTER_EVENT_TIMEOUT, 0); > > regmap_set_bits(priv->regmap16, QCLR, ~0); Same comment here as the previous patches about clearing all interrupt flags. > > @@ -500,6 +608,7 @@ static int ti_eqep_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > struct ti_eqep_cnt *priv; > + struct clk *clk; > void __iomem *base; > int err; > int irq; > @@ -508,6 +617,24 @@ static int ti_eqep_probe(struct platform_device *pdev) > if (!priv) > return -ENOMEM; > > + clk = devm_clk_get(dev, "sysclkout"); > + if (IS_ERR(clk)) { > + if (PTR_ERR(clk) != -EPROBE_DEFER) > + dev_err(dev, "failed to get sysclkout"); > + return PTR_ERR(clk); > + } > + > + priv->sysclkout_rate = clk_get_rate(clk); > + if (priv->sysclkout_rate == 0) { > + dev_err(dev, "failed to get sysclkout rate"); > + /* prevent divide by zero */ > + priv->sysclkout_rate = 1; > + /* > + * This error is not expected and the driver is mostly usable > + * without clock rate anyway, so don't exit here. > + */ If the values for these new attributes are expected to be denominated in nanoseconds then we must guarantee that. You should certainly error out here if you can't guarantee such. Alternatively, you could provide an additional set of attributes that are denominated in units of raw timer ticks rather than nanoseconds. That way if you can't determine the clock rate you can simply have the nanosecond-denominated timer attributes return an EOPNOTSUPP error code or similar while still providing users with the raw timer ticks attributes. William Breathitt Gray > + } > + > base = devm_platform_ioremap_resource(pdev, 0); > if (IS_ERR(base)) > return PTR_ERR(base); > @@ -536,6 +663,8 @@ static int ti_eqep_probe(struct platform_device *pdev) > priv->counter.ops = &ti_eqep_counter_ops; > priv->counter.counts = ti_eqep_counts; > priv->counter.num_counts = ARRAY_SIZE(ti_eqep_counts); > + priv->counter.ext = ti_eqep_device_ext; > + priv->counter.num_ext = ARRAY_SIZE(ti_eqep_device_ext); > priv->counter.signals = ti_eqep_signals; > priv->counter.num_signals = ARRAY_SIZE(ti_eqep_signals); > priv->counter.priv = priv; > @@ -552,10 +681,11 @@ static int ti_eqep_probe(struct platform_device *pdev) > > /* > * We can end up with an interupt infinite loop (interrupts triggered > - * as soon as they are cleared) if we leave this at the default value > + * as soon as they are cleared) if we leave these at the default value > * of 0 and events are enabled. > */ > regmap_write(priv->regmap32, QPOSMAX, UINT_MAX); > + regmap_write(priv->regmap32, QUPRD, UINT_MAX); > > err = counter_register(&priv->counter); > if (err < 0) { > diff --git a/include/uapi/linux/counter.h b/include/uapi/linux/counter.h > index 36dd3b474d09..640d9719b88c 100644 > --- a/include/uapi/linux/counter.h > +++ b/include/uapi/linux/counter.h > @@ -63,6 +63,8 @@ enum counter_event_type { > COUNTER_EVENT_INDEX, > /* Direction change detected */ > COUNTER_EVENT_DIRECTION_CHANGE, > + /* Timer exceeded timeout */ > + COUNTER_EVENT_TIMEOUT, > }; > > /** > -- > 2.25.1 >
On 10/25/21 3:48 AM, William Breathitt Gray wrote: > On Sat, Oct 16, 2021 at 08:33:38PM -0500, David Lechner wrote: >> This adds support to the TI eQEP counter driver for the Unit Timer. >> The Unit Timer is a device-level extension that provides a timer to be >> used for speed calculations. The sysfs interface for the Unit Timer is >> new and will be documented in a later commit. It contains a R/W time >> attribute for the current time, a R/W period attribute for the timeout >> period and a R/W enable attribute to start/stop the timer. It also >> implements a timeout event on the chrdev interface that is triggered >> each time the period timeout is reached. >> >> Signed-off-by: David Lechner <david@lechnology.com> > > I'll comment on the sysfs interface in the respective docs patch. Some > comments regarding this patch below. > ... >> +static int ti_eqep_unit_timer_period_write(struct counter_device *counter, >> + u64 value) >> +{ >> + struct ti_eqep_cnt *priv = counter->priv; >> + u32 quprd; >> + >> + /* convert nanoseconds to timer ticks */ >> + quprd = value = mul_u64_u32_div(value, priv->sysclkout_rate, NSEC_PER_SEC); >> + if (quprd != value) >> + return -ERANGE; >> + >> + /* protect against infinite unit timeout interrupts */ >> + if (quprd == 0) >> + return -EINVAL; > > I doubt there's any practical reason for a user to set the timer period > to 0, but perhaps we should not try to protect users from themselves > here. It's very obvious and expected that setting the timer period to 0 > results in timeouts as quickly as possible, so really the user should be > left to reap the fruits of their decision regardless of how asinine that > decision is. Even if the operating system ceases operation because the interrupt handler keeps running because clearing the interrupt has no effect in this condition? ... >> @@ -500,6 +608,7 @@ static int ti_eqep_probe(struct platform_device *pdev) >> { >> struct device *dev = &pdev->dev; >> struct ti_eqep_cnt *priv; >> + struct clk *clk; >> void __iomem *base; >> int err; >> int irq; >> @@ -508,6 +617,24 @@ static int ti_eqep_probe(struct platform_device *pdev) >> if (!priv) >> return -ENOMEM; >> >> + clk = devm_clk_get(dev, "sysclkout"); >> + if (IS_ERR(clk)) { >> + if (PTR_ERR(clk) != -EPROBE_DEFER) >> + dev_err(dev, "failed to get sysclkout"); >> + return PTR_ERR(clk); >> + } >> + >> + priv->sysclkout_rate = clk_get_rate(clk); >> + if (priv->sysclkout_rate == 0) { >> + dev_err(dev, "failed to get sysclkout rate"); >> + /* prevent divide by zero */ >> + priv->sysclkout_rate = 1; >> + /* >> + * This error is not expected and the driver is mostly usable >> + * without clock rate anyway, so don't exit here. >> + */ > > If the values for these new attributes are expected to be denominated in > nanoseconds then we must guarantee that. You should certainly error out > here if you can't guarantee such. > > Alternatively, you could provide an additional set of attributes that > are denominated in units of raw timer ticks rather than nanoseconds. > That way if you can't determine the clock rate you can simply have the > nanosecond-denominated timer attributes return an EOPNOTSUPP error code > or similar while still providing users with the raw timer ticks > attributes. I think we should just fail here.
On Wed, Oct 27, 2021 at 10:28:59AM -0500, David Lechner wrote: > On 10/25/21 3:48 AM, William Breathitt Gray wrote: > > On Sat, Oct 16, 2021 at 08:33:38PM -0500, David Lechner wrote: > >> This adds support to the TI eQEP counter driver for the Unit Timer. > >> The Unit Timer is a device-level extension that provides a timer to be > >> used for speed calculations. The sysfs interface for the Unit Timer is > >> new and will be documented in a later commit. It contains a R/W time > >> attribute for the current time, a R/W period attribute for the timeout > >> period and a R/W enable attribute to start/stop the timer. It also > >> implements a timeout event on the chrdev interface that is triggered > >> each time the period timeout is reached. > >> > >> Signed-off-by: David Lechner <david@lechnology.com> > > > > I'll comment on the sysfs interface in the respective docs patch. Some > > comments regarding this patch below. > > > > ... > > >> +static int ti_eqep_unit_timer_period_write(struct counter_device *counter, > >> + u64 value) > >> +{ > >> + struct ti_eqep_cnt *priv = counter->priv; > >> + u32 quprd; > >> + > >> + /* convert nanoseconds to timer ticks */ > >> + quprd = value = mul_u64_u32_div(value, priv->sysclkout_rate, NSEC_PER_SEC); > >> + if (quprd != value) > >> + return -ERANGE; > >> + > >> + /* protect against infinite unit timeout interrupts */ > >> + if (quprd == 0) > >> + return -EINVAL; > > > > I doubt there's any practical reason for a user to set the timer period > > to 0, but perhaps we should not try to protect users from themselves > > here. It's very obvious and expected that setting the timer period to 0 > > results in timeouts as quickly as possible, so really the user should be > > left to reap the fruits of their decision regardless of how asinine that > > decision is. > > Even if the operating system ceases operation because the interrupt > handler keeps running because clearing the interrupt has no effect > in this condition? I don't disagree with you that configuring the device to repeatedly timeout without pause would be a waste of system resources. However, it is more appropriate for this protection to be located in a userspace application rather than the driver code here. The purpose of a driver is to expose the functionality of a device in an understandable and consistent manner. Drivers should not dictate what a user does with their device, but rather should help facilitate the user's control so that the device behaves as would be expected given such an interface. For this particular case, the device is capable of sending an interrupt when a timeout events occurs, and the timeout period can be adjusted; setting the timeout period lower and lower results in less and less time between timeout events. The behavior and result of setting the timeout period lower is well-defined and predictable; it is intuitive that configuring the timeout period to 0, the lowest value possible, results in the shortest time possible between timeouts: no pause at all. As long as the functionality of this device is exposed in such an understandable and consistent manner, the driver succeeds in serving its purpose. So I believe a timeout period of 0 is a valid configuration for this driver to allow, albeit a seemingly pointless one for users to actually choose. To that end, simply set the default value of QUPRD to non-zero on probe() as you do already in this patch and let the user be free to adjust if they so decide. William Breathitt Gray
On 10/28/21 2:48 AM, William Breathitt Gray wrote: > On Wed, Oct 27, 2021 at 10:28:59AM -0500, David Lechner wrote: >> On 10/25/21 3:48 AM, William Breathitt Gray wrote: >>> On Sat, Oct 16, 2021 at 08:33:38PM -0500, David Lechner wrote: >>>> This adds support to the TI eQEP counter driver for the Unit Timer. >>>> The Unit Timer is a device-level extension that provides a timer to be >>>> used for speed calculations. The sysfs interface for the Unit Timer is >>>> new and will be documented in a later commit. It contains a R/W time >>>> attribute for the current time, a R/W period attribute for the timeout >>>> period and a R/W enable attribute to start/stop the timer. It also >>>> implements a timeout event on the chrdev interface that is triggered >>>> each time the period timeout is reached. >>>> >>>> Signed-off-by: David Lechner <david@lechnology.com> >>> >>> I'll comment on the sysfs interface in the respective docs patch. Some >>> comments regarding this patch below. >>> >> >> ... >> >>>> +static int ti_eqep_unit_timer_period_write(struct counter_device *counter, >>>> + u64 value) >>>> +{ >>>> + struct ti_eqep_cnt *priv = counter->priv; >>>> + u32 quprd; >>>> + >>>> + /* convert nanoseconds to timer ticks */ >>>> + quprd = value = mul_u64_u32_div(value, priv->sysclkout_rate, NSEC_PER_SEC); >>>> + if (quprd != value) >>>> + return -ERANGE; >>>> + >>>> + /* protect against infinite unit timeout interrupts */ >>>> + if (quprd == 0) >>>> + return -EINVAL; >>> >>> I doubt there's any practical reason for a user to set the timer period >>> to 0, but perhaps we should not try to protect users from themselves >>> here. It's very obvious and expected that setting the timer period to 0 >>> results in timeouts as quickly as possible, so really the user should be >>> left to reap the fruits of their decision regardless of how asinine that >>> decision is. >> >> Even if the operating system ceases operation because the interrupt >> handler keeps running because clearing the interrupt has no effect >> in this condition? > > I don't disagree with you that configuring the device to repeatedly > timeout without pause would be a waste of system resources. However, it > is more appropriate for this protection to be located in a userspace > application rather than the driver code here. > > The purpose of a driver is to expose the functionality of a device in an > understandable and consistent manner. Drivers should not dictate what a > user does with their device, but rather should help facilitate the > user's control so that the device behaves as would be expected given > such an interface. > > For this particular case, the device is capable of sending an interrupt > when a timeout events occurs, and the timeout period can be adjusted; > setting the timeout period lower and lower results in less and less time > between timeout events. The behavior and result of setting the timeout > period lower is well-defined and predictable; it is intuitive that > configuring the timeout period to 0, the lowest value possible, results > in the shortest time possible between timeouts: no pause at all. > > As long as the functionality of this device is exposed in such an > understandable and consistent manner, the driver succeeds in serving its > purpose. So I believe a timeout period of 0 is a valid configuration > for this driver to allow, albeit a seemingly pointless one for users to > actually choose. To that end, simply set the default value of QUPRD to > non-zero on probe() as you do already in this patch and let the user be > free to adjust if they so decide. > > William Breathitt Gray > I disagree. I consider this a crash. The system becomes completely unusable and you have to pull power to turn it off, potentially leading to data loss and disk corruption.
On Thu, Oct 28, 2021 at 08:42:49AM -0500, David Lechner wrote: > On 10/28/21 2:48 AM, William Breathitt Gray wrote: > > On Wed, Oct 27, 2021 at 10:28:59AM -0500, David Lechner wrote: > >> On 10/25/21 3:48 AM, William Breathitt Gray wrote: > >>> On Sat, Oct 16, 2021 at 08:33:38PM -0500, David Lechner wrote: > >>>> This adds support to the TI eQEP counter driver for the Unit Timer. > >>>> The Unit Timer is a device-level extension that provides a timer to be > >>>> used for speed calculations. The sysfs interface for the Unit Timer is > >>>> new and will be documented in a later commit. It contains a R/W time > >>>> attribute for the current time, a R/W period attribute for the timeout > >>>> period and a R/W enable attribute to start/stop the timer. It also > >>>> implements a timeout event on the chrdev interface that is triggered > >>>> each time the period timeout is reached. > >>>> > >>>> Signed-off-by: David Lechner <david@lechnology.com> > >>> > >>> I'll comment on the sysfs interface in the respective docs patch. Some > >>> comments regarding this patch below. > >>> > >> > >> ... > >> > >>>> +static int ti_eqep_unit_timer_period_write(struct counter_device *counter, > >>>> + u64 value) > >>>> +{ > >>>> + struct ti_eqep_cnt *priv = counter->priv; > >>>> + u32 quprd; > >>>> + > >>>> + /* convert nanoseconds to timer ticks */ > >>>> + quprd = value = mul_u64_u32_div(value, priv->sysclkout_rate, NSEC_PER_SEC); > >>>> + if (quprd != value) > >>>> + return -ERANGE; > >>>> + > >>>> + /* protect against infinite unit timeout interrupts */ > >>>> + if (quprd == 0) > >>>> + return -EINVAL; > >>> > >>> I doubt there's any practical reason for a user to set the timer period > >>> to 0, but perhaps we should not try to protect users from themselves > >>> here. It's very obvious and expected that setting the timer period to 0 > >>> results in timeouts as quickly as possible, so really the user should be > >>> left to reap the fruits of their decision regardless of how asinine that > >>> decision is. > >> > >> Even if the operating system ceases operation because the interrupt > >> handler keeps running because clearing the interrupt has no effect > >> in this condition? > > > > I don't disagree with you that configuring the device to repeatedly > > timeout without pause would be a waste of system resources. However, it > > is more appropriate for this protection to be located in a userspace > > application rather than the driver code here. > > > > The purpose of a driver is to expose the functionality of a device in an > > understandable and consistent manner. Drivers should not dictate what a > > user does with their device, but rather should help facilitate the > > user's control so that the device behaves as would be expected given > > such an interface. > > > > For this particular case, the device is capable of sending an interrupt > > when a timeout events occurs, and the timeout period can be adjusted; > > setting the timeout period lower and lower results in less and less time > > between timeout events. The behavior and result of setting the timeout > > period lower is well-defined and predictable; it is intuitive that > > configuring the timeout period to 0, the lowest value possible, results > > in the shortest time possible between timeouts: no pause at all. > > > > As long as the functionality of this device is exposed in such an > > understandable and consistent manner, the driver succeeds in serving its > > purpose. So I believe a timeout period of 0 is a valid configuration > > for this driver to allow, albeit a seemingly pointless one for users to > > actually choose. To that end, simply set the default value of QUPRD to > > non-zero on probe() as you do already in this patch and let the user be > > free to adjust if they so decide. > > > > William Breathitt Gray > > > > I disagree. I consider this a crash. The system becomes completely > unusable and you have to pull power to turn it off, potentially > leading to data loss and disk corruption. I hope I'm not being excessively pedantic here -- I'll concede to a third opion on this if someone else wants to chime in -- but I want to ensure that we are not going outside the scope of what a driver should do. Note that any device is capable of flooding the system with interrupts (e.g. a counter operating on a high enough frequency overflowing a low ceiling), so I don't think that reason alone will pass muster. Nevertheless, it is important to define when a driver should return an error or not. Take for example, the period range check right above. If a user requests the device do something it cannot, such as counting down from a period value that is too high for it to represent internally, then it is appropriate to return an error: the device cannot perform the request and as such the request is not valid input for the driver. However, when we apply the same method to the zero value case -- a user requests a timeout period of 0 -- the device is capable of performing that request: the device is capable of waiting 0 nanoseconds and as such the request is a valid input for the driver because it can be performed by the device exactly as expected by the user. As long as the behavior of a request is well-defined, we must assume the user knows what they are doing, and thus should be permitted to request their device perform that behavior. A driver should not speculate on the intent of a user's actions. Restricting what a user can do with their device is a matter of configuration policy, and configuration policy belongs appropriately in userspace. Rather, the scope of a driver should be limited narrowly to exposure of a device functionality in a standard and predictable way. William Breathitt Gray
diff --git a/drivers/counter/ti-eqep.c b/drivers/counter/ti-eqep.c index 9881e5115da6..a4a5a4486329 100644 --- a/drivers/counter/ti-eqep.c +++ b/drivers/counter/ti-eqep.c @@ -6,6 +6,7 @@ */ #include <linux/bitops.h> +#include <linux/clk.h> #include <linux/counter.h> #include <linux/interrupt.h> #include <linux/kernel.h> @@ -131,6 +132,7 @@ enum ti_eqep_count_func { struct ti_eqep_cnt { struct counter_device counter; + unsigned long sysclkout_rate; struct regmap *regmap32; struct regmap *regmap16; }; @@ -298,6 +300,9 @@ static int ti_eqep_events_configure(struct counter_device *counter) case COUNTER_EVENT_DIRECTION_CHANGE: qeint |= QEINT_QDC; break; + case COUNTER_EVENT_TIMEOUT: + qeint |= QEINT_UTO; + break; } } @@ -311,6 +316,7 @@ static int ti_eqep_watch_validate(struct counter_device *counter, case COUNTER_EVENT_OVERFLOW: case COUNTER_EVENT_UNDERFLOW: case COUNTER_EVENT_DIRECTION_CHANGE: + case COUNTER_EVENT_TIMEOUT: return 0; default: return -EINVAL; @@ -457,6 +463,106 @@ static struct counter_count ti_eqep_counts[] = { }, }; +static int ti_eqep_unit_timer_time_read(struct counter_device *counter, + u64 *value) +{ + struct ti_eqep_cnt *priv = counter->priv; + u32 qutmr; + + regmap_read(priv->regmap32, QUTMR, &qutmr); + + /* convert timer ticks to nanoseconds */ + *value = mul_u64_u32_div(qutmr, NSEC_PER_SEC, priv->sysclkout_rate); + + return 0; +} + +static int ti_eqep_unit_timer_time_write(struct counter_device *counter, + u64 value) +{ + struct ti_eqep_cnt *priv = counter->priv; + u32 qutmr; + + /* convert nanoseconds to timer ticks */ + qutmr = value = mul_u64_u32_div(value, priv->sysclkout_rate, NSEC_PER_SEC); + if (qutmr != value) + return -ERANGE; + + regmap_write(priv->regmap32, QUTMR, qutmr); + + return 0; +} + +static int ti_eqep_unit_timer_period_read(struct counter_device *counter, + u64 *value) +{ + struct ti_eqep_cnt *priv = counter->priv; + u32 quprd; + + regmap_read(priv->regmap32, QUPRD, &quprd); + + /* convert timer ticks to nanoseconds */ + *value = mul_u64_u32_div(quprd, NSEC_PER_SEC, priv->sysclkout_rate); + + return 0; +} + +static int ti_eqep_unit_timer_period_write(struct counter_device *counter, + u64 value) +{ + struct ti_eqep_cnt *priv = counter->priv; + u32 quprd; + + /* convert nanoseconds to timer ticks */ + quprd = value = mul_u64_u32_div(value, priv->sysclkout_rate, NSEC_PER_SEC); + if (quprd != value) + return -ERANGE; + + /* protect against infinite unit timeout interrupts */ + if (quprd == 0) + return -EINVAL; + + regmap_write(priv->regmap32, QUPRD, quprd); + + return 0; +} + +static int ti_eqep_unit_timer_enable_read(struct counter_device *counter, + u8 *value) +{ + struct ti_eqep_cnt *priv = counter->priv; + u32 qepctl; + + regmap_read(priv->regmap16, QEPCTL, &qepctl); + *value = !!(qepctl & QEPCTL_UTE); + + return 0; +} + +static int ti_eqep_unit_timer_enable_write(struct counter_device *counter, + u8 value) +{ + struct ti_eqep_cnt *priv = counter->priv; + + if (value) + regmap_set_bits(priv->regmap16, QEPCTL, QEPCTL_UTE); + else + regmap_clear_bits(priv->regmap16, QEPCTL, QEPCTL_UTE); + + return 0; +} + +static struct counter_comp ti_eqep_device_ext[] = { + COUNTER_COMP_DEVICE_U64("unit_timer_time", ti_eqep_unit_timer_time_read, + ti_eqep_unit_timer_time_write), + COUNTER_COMP_DEVICE_U64("unit_timer_period", + ti_eqep_unit_timer_period_read, + ti_eqep_unit_timer_period_write), + COUNTER_COMP_DEVICE_BOOL("unit_timer_enable", + ti_eqep_unit_timer_enable_read, + ti_eqep_unit_timer_enable_write), +}; + static irqreturn_t ti_eqep_irq_handler(int irq, void *dev_id) { struct ti_eqep_cnt *priv = dev_id; @@ -474,6 +580,8 @@ static irqreturn_t ti_eqep_irq_handler(int irq, void *dev_id) if (qflg & QFLG_QDC) counter_push_event(counter, COUNTER_EVENT_DIRECTION_CHANGE, 0); + if (qflg & QFLG_UTO) + counter_push_event(counter, COUNTER_EVENT_TIMEOUT, 0); regmap_set_bits(priv->regmap16, QCLR, ~0); @@ -500,6 +608,7 @@ static int ti_eqep_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; struct ti_eqep_cnt *priv; + struct clk *clk; void __iomem *base; int err; int irq; @@ -508,6 +617,24 @@ static int ti_eqep_probe(struct platform_device *pdev) if (!priv) return -ENOMEM; + clk = devm_clk_get(dev, "sysclkout"); + if (IS_ERR(clk)) { + if (PTR_ERR(clk) != -EPROBE_DEFER) + dev_err(dev, "failed to get sysclkout"); + return PTR_ERR(clk); + } + + priv->sysclkout_rate = clk_get_rate(clk); + if (priv->sysclkout_rate == 0) { + dev_err(dev, "failed to get sysclkout rate"); + /* prevent divide by zero */ + priv->sysclkout_rate = 1; + /* + * This error is not expected and the driver is mostly usable + * without clock rate anyway, so don't exit here. + */ + } + base = devm_platform_ioremap_resource(pdev, 0); if (IS_ERR(base)) return PTR_ERR(base); @@ -536,6 +663,8 @@ static int ti_eqep_probe(struct platform_device *pdev) priv->counter.ops = &ti_eqep_counter_ops; priv->counter.counts = ti_eqep_counts; priv->counter.num_counts = ARRAY_SIZE(ti_eqep_counts); + priv->counter.ext = ti_eqep_device_ext; + priv->counter.num_ext = ARRAY_SIZE(ti_eqep_device_ext); priv->counter.signals = ti_eqep_signals; priv->counter.num_signals = ARRAY_SIZE(ti_eqep_signals); priv->counter.priv = priv; @@ -552,10 +681,11 @@ static int ti_eqep_probe(struct platform_device *pdev) /* * We can end up with an interupt infinite loop (interrupts triggered - * as soon as they are cleared) if we leave this at the default value + * as soon as they are cleared) if we leave these at the default value * of 0 and events are enabled. */ regmap_write(priv->regmap32, QPOSMAX, UINT_MAX); + regmap_write(priv->regmap32, QUPRD, UINT_MAX); err = counter_register(&priv->counter); if (err < 0) { diff --git a/include/uapi/linux/counter.h b/include/uapi/linux/counter.h index 36dd3b474d09..640d9719b88c 100644 --- a/include/uapi/linux/counter.h +++ b/include/uapi/linux/counter.h @@ -63,6 +63,8 @@ enum counter_event_type { COUNTER_EVENT_INDEX, /* Direction change detected */ COUNTER_EVENT_DIRECTION_CHANGE, + /* Timer exceeded timeout */ + COUNTER_EVENT_TIMEOUT, }; /**
This adds support to the TI eQEP counter driver for the Unit Timer. The Unit Timer is a device-level extension that provides a timer to be used for speed calculations. The sysfs interface for the Unit Timer is new and will be documented in a later commit. It contains a R/W time attribute for the current time, a R/W period attribute for the timeout period and a R/W enable attribute to start/stop the timer. It also implements a timeout event on the chrdev interface that is triggered each time the period timeout is reached. Signed-off-by: David Lechner <david@lechnology.com> --- drivers/counter/ti-eqep.c | 132 ++++++++++++++++++++++++++++++++++- include/uapi/linux/counter.h | 2 + 2 files changed, 133 insertions(+), 1 deletion(-)