[v1] i2c: npcm7xx: Support changing bus speed using debugfs.

Systems that can dinamically add and remove slave devices
often need to change the bus speed in runtime.
This patch exposes the bus frequency to the user.
This feature can also be used for test automation.

Fixes: 56a1485b102e (i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver)
Signed-off-by: Tali Perry <tali.perry1@gmail.com>
---
 drivers/i2c/busses/i2c-npcm7xx.c | 36 ++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)


base-commit: 06d56c38d7d411c162e4d406a9864bed32e30e61

On Wed, Sep 30, 2020 at 10:13:42AM +0300, Tali Perry wrote:
> Systems that can dinamically add and remove slave devices

dynamically

> often need to change the bus speed in runtime.

> This patch exposes the bus frequency to the user.

Expose the bus frequency to the user.

> This feature can also be used for test automation.

In general I think that DT overlays or so should be user rather than this.
If we allow to change bus speed settings for debugging purposes it might make
sense to do this on framework level for all drivers which support that (via
additional callback or so).

> Fixes: 56a1485b102e (i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver)
> Signed-off-by: Tali Perry <tali.perry1@gmail.com>
> ---
>  drivers/i2c/busses/i2c-npcm7xx.c | 36 ++++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c
> index 2ad166355ec9..44e2340c1893 100644
> --- a/drivers/i2c/busses/i2c-npcm7xx.c
> +++ b/drivers/i2c/busses/i2c-npcm7xx.c
> @@ -2208,6 +2208,41 @@ static const struct i2c_algorithm npcm_i2c_algo = {
>  /* i2c debugfs directory: used to keep health monitor of i2c devices */
>  static struct dentry *npcm_i2c_debugfs_dir;
>  
> +static int i2c_speed_get(void *data, u64 *val)
> +{
> +	struct npcm_i2c *bus = data;
> +
> +	*val = (u64)bus->bus_freq;
> +	return 0;
> +}
> +
> +static int i2c_speed_set(void *data, u64 val)
> +{
> +	struct npcm_i2c *bus = data;
> +	int ret;
> +
> +	if (val < (u64)I2C_FREQ_MIN_HZ || val > (u64)I2C_FREQ_MAX_HZ)
> +		return -EINVAL;
> +
> +	if (val == (u64)bus->bus_freq)
> +		return 0;
> +
> +	i2c_lock_bus(&bus->adap, I2C_LOCK_ROOT_ADAPTER);
> +
> +	npcm_i2c_int_enable(bus, false);
> +
> +	ret = npcm_i2c_init_module(bus, I2C_MASTER, (u32)val);
> +
> +	i2c_unlock_bus(&bus->adap, I2C_LOCK_ROOT_ADAPTER);

While all these explicit castings?

> +
> +	if (ret)
> +		return -EAGAIN;
> +
> +	return 0;
> +}

> +

No need to have this blank line

> +DEFINE_DEBUGFS_ATTRIBUTE(i2c_clock_ops, i2c_speed_get, i2c_speed_set, "%lld\n");
> +
>  static void npcm_i2c_init_debugfs(struct platform_device *pdev,
>  				  struct npcm_i2c *bus)
>  {
> @@ -2223,6 +2258,7 @@ static void npcm_i2c_init_debugfs(struct platform_device *pdev,
>  	debugfs_create_u64("rec_succ_cnt", 0444, d, &bus->rec_succ_cnt);
>  	debugfs_create_u64("rec_fail_cnt", 0444, d, &bus->rec_fail_cnt);
>  	debugfs_create_u64("timeout_cnt", 0444, d, &bus->timeout_cnt);
> +	debugfs_create_file("i2c_speed", 0644, d, bus, &i2c_clock_ops);
>  
>  	bus->debugfs = d;
>  }

On Wed, Sep 30, 2020 at 12:31 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Wed, Sep 30, 2020 at 10:13:42AM +0300, Tali Perry wrote:
> > Systems that can dinamically add and remove slave devices
>
> dynamically
>
> > often need to change the bus speed in runtime.
>
> > This patch exposes the bus frequency to the user.
>
> Expose the bus frequency to the user.
>
> > This feature can also be used for test automation.
>
> In general I think that DT overlays or so should be user rather than this.
> If we allow to change bus speed settings for debugging purposes it might make
> sense to do this on framework level for all drivers which support that (via
> additional callback or so).

Do you mean adding something like this:

struct i2c_algorithm {
/*
* If an adapter algorithm can't do I2C-level access, set master_xfer
* to NULL. If an adapter algorithm can do SMBus access, set
* smbus_xfer. If set to NULL, the SMBus protocol is simulated
* using common I2C messages.
*
* master_xfer should return the number of messages successfully
* processed, or a negative value on error
*/
int (*master_xfer)(struct i2c_adapter *adap, struct i2c_msg *msgs,
  int num);
....
int (*set_speed)(struct i2c_adapter *adap, unsigned int speed);
....
/* To determine what the adapter supports */
u32 (*functionality)(struct i2c_adapter *adap);

...
};

And expose this feature in functionality?


>
> > Fixes: 56a1485b102e (i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver)
> > Signed-off-by: Tali Perry <tali.perry1@gmail.com>
> > ---
> >  drivers/i2c/busses/i2c-npcm7xx.c | 36 ++++++++++++++++++++++++++++++++
> >  1 file changed, 36 insertions(+)
> >
> > diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c
> > index 2ad166355ec9..44e2340c1893 100644
> > --- a/drivers/i2c/busses/i2c-npcm7xx.c
> > +++ b/drivers/i2c/busses/i2c-npcm7xx.c
> > @@ -2208,6 +2208,41 @@ static const struct i2c_algorithm npcm_i2c_algo = {
> >  /* i2c debugfs directory: used to keep health monitor of i2c devices */
> >  static struct dentry *npcm_i2c_debugfs_dir;
> >
> > +static int i2c_speed_get(void *data, u64 *val)
> > +{
> > +     struct npcm_i2c *bus = data;
> > +
> > +     *val = (u64)bus->bus_freq;
> > +     return 0;
> > +}
> > +
> > +static int i2c_speed_set(void *data, u64 val)
> > +{
> > +     struct npcm_i2c *bus = data;
> > +     int ret;
> > +
> > +     if (val < (u64)I2C_FREQ_MIN_HZ || val > (u64)I2C_FREQ_MAX_HZ)
> > +             return -EINVAL;
> > +
> > +     if (val == (u64)bus->bus_freq)
> > +             return 0;
> > +
> > +     i2c_lock_bus(&bus->adap, I2C_LOCK_ROOT_ADAPTER);
> > +
> > +     npcm_i2c_int_enable(bus, false);
> > +
> > +     ret = npcm_i2c_init_module(bus, I2C_MASTER, (u32)val);
> > +
> > +     i2c_unlock_bus(&bus->adap, I2C_LOCK_ROOT_ADAPTER);
>
> While all these explicit castings?
>
> > +
> > +     if (ret)
> > +             return -EAGAIN;
> > +
> > +     return 0;
> > +}
>
> > +
>
> No need to have this blank line
>
> > +DEFINE_DEBUGFS_ATTRIBUTE(i2c_clock_ops, i2c_speed_get, i2c_speed_set, "%lld\n");
> > +
> >  static void npcm_i2c_init_debugfs(struct platform_device *pdev,
> >                                 struct npcm_i2c *bus)
> >  {
> > @@ -2223,6 +2258,7 @@ static void npcm_i2c_init_debugfs(struct platform_device *pdev,
> >       debugfs_create_u64("rec_succ_cnt", 0444, d, &bus->rec_succ_cnt);
> >       debugfs_create_u64("rec_fail_cnt", 0444, d, &bus->rec_fail_cnt);
> >       debugfs_create_u64("timeout_cnt", 0444, d, &bus->timeout_cnt);
> > +     debugfs_create_file("i2c_speed", 0644, d, bus, &i2c_clock_ops);
> >
> >       bus->debugfs = d;
> >  }
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

On Thu, Oct 1, 2020 at 8:34 AM Tali Perry <tali.perry1@gmail.com> wrote:
> On Wed, Sep 30, 2020 at 12:31 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > On Wed, Sep 30, 2020 at 10:13:42AM +0300, Tali Perry wrote:
> > > Systems that can dinamically add and remove slave devices
> >
> > dynamically
> >
> > > often need to change the bus speed in runtime.
> >
> > > This patch exposes the bus frequency to the user.
> >
> > Expose the bus frequency to the user.
> >
> > > This feature can also be used for test automation.
> >
> > In general I think that DT overlays or so should be user rather than this.
> > If we allow to change bus speed settings for debugging purposes it might make
> > sense to do this on framework level for all drivers which support that (via
> > additional callback or so).
>
> Do you mean adding something like this:

Nope. I meant to use DT description for that. I²C core should cope
with DT already.
I do not understand why you need special nodes for that rather than DT
overlay which will change the speed for you.

Hi Andy,

Customers using BMC with complex i2c topology asked us to support
changing bus frequency at run time, for example same device will
communicate with one slave at 100Kbp/s and another with 400kbp/s and
maybe also with smae device at different speed (for example an i2c
mux).
This is not only for debug.
Can DT overlay support that?

On Thu, Oct 1, 2020 at 6:40 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Thu, Oct 1, 2020 at 8:34 AM Tali Perry <tali.perry1@gmail.com> wrote:
> > On Wed, Sep 30, 2020 at 12:31 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > >
> > > On Wed, Sep 30, 2020 at 10:13:42AM +0300, Tali Perry wrote:
> > > > Systems that can dinamically add and remove slave devices
> > >
> > > dynamically
> > >
> > > > often need to change the bus speed in runtime.
> > >
> > > > This patch exposes the bus frequency to the user.
> > >
> > > Expose the bus frequency to the user.
> > >
> > > > This feature can also be used for test automation.
> > >
> > > In general I think that DT overlays or so should be user rather than this.
> > > If we allow to change bus speed settings for debugging purposes it might make
> > > sense to do this on framework level for all drivers which support that (via
> > > additional callback or so).
> >
> > Do you mean adding something like this:
>
> Nope. I meant to use DT description for that. I²C core should cope
> with DT already.
> I do not understand why you need special nodes for that rather than DT
> overlay which will change the speed for you.
>
> --
> With Best Regards,
> Andy Shevchenko

On Thu, Oct 01, 2020 at 08:13:49PM +0300, Avi Fishman wrote:
> Hi Andy,
> 
> Customers using BMC with complex i2c topology asked us to support
> changing bus frequency at run time, for example same device will
> communicate with one slave at 100Kbp/s and another with 400kbp/s and
> maybe also with smae device at different speed (for example an i2c
> mux).
> This is not only for debug.

The above design is fragile to start with. If you have connected peripheral
devices with different speed limitations and you try to access faster one the
slower ones may block and break the bus which will need recovery.

> Can DT overlay support that?

Probably. DT overlay describes the update in the device topology, including
certain device properties.

P.S. Please do not top post.

> On Thu, Oct 1, 2020 at 6:40 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> >
> > On Thu, Oct 1, 2020 at 8:34 AM Tali Perry <tali.perry1@gmail.com> wrote:
> > > On Wed, Sep 30, 2020 at 12:31 PM Andy Shevchenko
> > > <andriy.shevchenko@linux.intel.com> wrote:
> > > >
> > > > On Wed, Sep 30, 2020 at 10:13:42AM +0300, Tali Perry wrote:
> > > > > Systems that can dinamically add and remove slave devices
> > > >
> > > > dynamically
> > > >
> > > > > often need to change the bus speed in runtime.
> > > >
> > > > > This patch exposes the bus frequency to the user.
> > > >
> > > > Expose the bus frequency to the user.
> > > >
> > > > > This feature can also be used for test automation.
> > > >
> > > > In general I think that DT overlays or so should be user rather than this.
> > > > If we allow to change bus speed settings for debugging purposes it might make
> > > > sense to do this on framework level for all drivers which support that (via
> > > > additional callback or so).
> > >
> > > Do you mean adding something like this:
> >
> > Nope. I meant to use DT description for that. I²C core should cope
> > with DT already.
> > I do not understand why you need special nodes for that rather than DT
> > overlay which will change the speed for you.

On Thu, Oct 1, 2020 at 10:41 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Thu, Oct 01, 2020 at 08:13:49PM +0300, Avi Fishman wrote:
> > Hi Andy,
> >
> > Customers using BMC with complex i2c topology asked us to support
> > changing bus frequency at run time, for example same device will
> > communicate with one slave at 100Kbp/s and another with 400kbp/s and
> > maybe also with smae device at different speed (for example an i2c
> > mux).
> > This is not only for debug.
>
> The above design is fragile to start with. If you have connected peripheral
> devices with different speed limitations and you try to access faster one the
> slower ones may block and break the bus which will need recovery.
>

Hi Andy,

To clarify, we are using a single read-only image to support multiple
configurations, so the supported bus rate of the devices are not known
at compile time, but at runtime. We start with 100 kHz, and go 400 kHz
if applicable. FYI, we are using 5.1 kernel, however I don't know much
about DT overlay.

Thx.

-Alex Qiu

Tali indeed pointed our major customers (Alex represent one of them :)
that this feature must be handled carefully since it may break the
communication and they are aware of that. Nevertheless they still want
this feature, they already reviewed this and accepted it (in internal
mails)

So we will appreciate if this will be accepted.

On Thu, Oct 1, 2020 at 9:27 PM Alex Qiu <xqiu@google.com> wrote:
>
> On Thu, Oct 1, 2020 at 10:41 AM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> >
> > On Thu, Oct 01, 2020 at 08:13:49PM +0300, Avi Fishman wrote:
> > > Hi Andy,
> > >
> > > Customers using BMC with complex i2c topology asked us to support
> > > changing bus frequency at run time, for example same device will
> > > communicate with one slave at 100Kbp/s and another with 400kbp/s and
> > > maybe also with smae device at different speed (for example an i2c
> > > mux).
> > > This is not only for debug.
> >
> > The above design is fragile to start with. If you have connected peripheral
> > devices with different speed limitations and you try to access faster one the
> > slower ones may block and break the bus which will need recovery.
> >
>
> Hi Andy,
>
> To clarify, we are using a single read-only image to support multiple
> configurations, so the supported bus rate of the devices are not known
> at compile time, but at runtime. We start with 100 kHz, and go 400 kHz
> if applicable. FYI, we are using 5.1 kernel, however I don't know much
> about DT overlay.
>
> Thx.
>
> -Alex Qiu

On Thu, Oct 1, 2020 at 9:42 PM Avi Fishman <avifishman70@gmail.com> wrote:
>
> Tali indeed pointed our major customers (Alex represent one of them :)
> that this feature must be handled carefully since it may break the
> communication and they are aware of that. Nevertheless they still want
> this feature, they already reviewed this and accepted it (in internal
> mails)
>
> So we will appreciate if this will be accepted.
>
> On Thu, Oct 1, 2020 at 9:27 PM Alex Qiu <xqiu@google.com> wrote:
> >
> > On Thu, Oct 1, 2020 at 10:41 AM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > >
> > > On Thu, Oct 01, 2020 at 08:13:49PM +0300, Avi Fishman wrote:
> > > > Hi Andy,
> > > >
> > > > Customers using BMC with complex i2c topology asked us to support
> > > > changing bus frequency at run time, for example same device will
> > > > communicate with one slave at 100Kbp/s and another with 400kbp/s and
> > > > maybe also with smae device at different speed (for example an i2c
> > > > mux).
> > > > This is not only for debug.
> > >
> > > The above design is fragile to start with. If you have connected peripheral
> > > devices with different speed limitations and you try to access faster one the
> > > slower ones may block and break the bus which will need recovery.
> > >
> >
> > Hi Andy,
> >
> > To clarify, we are using a single read-only image to support multiple
> > configurations, so the supported bus rate of the devices are not known
> > at compile time, but at runtime. We start with 100 kHz, and go 400 kHz
> > if applicable. FYI, we are using 5.1 kernel, however I don't know much
> > about DT overlay.

I see. So, there are following statements:
 - the elaboration is good but I guess needs to be added somewhere in
form of the documentation
 - the internal schedules or so are not crucial for the upstream (it
rather sounds like a bribing the judge)
 - the current approach, if I'm not mistaken, is using debugfs, which
is not ABI and it's good
 - I'm not a maintainer here, but I don't like the approach

Let the maintainer decide.

On Thu, Oct 1, 2020 at 11:51 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> I see. So, there are following statements:
>  - the elaboration is good but I guess needs to be added somewhere in
> form of the documentation
>  - the internal schedules or so are not crucial for the upstream (it
> rather sounds like a bribing the judge)
>  - the current approach, if I'm not mistaken, is using debugfs, which
> is not ABI and it's good
>  - I'm not a maintainer here, but I don't like the approach
>
> Let the maintainer decide.
>
> --
> With Best Regards,
> Andy Shevchenko

Hi Andy,

That makes perfect sense. We may keep it downstream to unblock our own
work if it's not accepted upstream. Thanks for your review!

- Alex Qiu