linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v1] i2c: npcm7xx: Support changing bus speed using debugfs.
@ 2020-09-30  7:13 Tali Perry
  2020-09-30  9:31 ` Andy Shevchenko
  0 siblings, 1 reply; 10+ messages in thread
From: Tali Perry @ 2020-09-30  7:13 UTC (permalink / raw)
  To: wsa, andriy.shevchenko, xqiu, kunyi, benjaminfair, avifishman70,
	joel, tmaimon77
  Cc: linux-i2c, openbmc, linux-kernel, Tali Perry

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(+)

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);
+
+	if (ret)
+		return -EAGAIN;
+
+	return 0;
+}
+
+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;
 }

base-commit: 06d56c38d7d411c162e4d406a9864bed32e30e61
-- 
2.22.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v1] i2c: npcm7xx: Support changing bus speed using debugfs.
  2020-09-30  7:13 [PATCH v1] i2c: npcm7xx: Support changing bus speed using debugfs Tali Perry
@ 2020-09-30  9:31 ` Andy Shevchenko
  2020-10-01  5:32   ` Tali Perry
  0 siblings, 1 reply; 10+ messages in thread
From: Andy Shevchenko @ 2020-09-30  9:31 UTC (permalink / raw)
  To: Tali Perry
  Cc: wsa, xqiu, kunyi, benjaminfair, avifishman70, joel, tmaimon77,
	linux-i2c, openbmc, linux-kernel

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;
>  }

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v1] i2c: npcm7xx: Support changing bus speed using debugfs.
  2020-09-30  9:31 ` Andy Shevchenko
@ 2020-10-01  5:32   ` Tali Perry
  2020-10-01 15:40     ` Andy Shevchenko
  0 siblings, 1 reply; 10+ messages in thread
From: Tali Perry @ 2020-10-01  5:32 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Wolfram Sang, Alex Qiu, Kun Yi, Benjamin Fair, avifishman70,
	joel, Tomer Maimon, Linux I2C, OpenBMC Maillist,
	Linux Kernel Mailing List

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
>
>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v1] i2c: npcm7xx: Support changing bus speed using debugfs.
  2020-10-01  5:32   ` Tali Perry
@ 2020-10-01 15:40     ` Andy Shevchenko
  2020-10-01 17:13       ` Avi Fishman
  0 siblings, 1 reply; 10+ messages in thread
From: Andy Shevchenko @ 2020-10-01 15:40 UTC (permalink / raw)
  To: Tali Perry
  Cc: Andy Shevchenko, Wolfram Sang, Alex Qiu, Kun Yi, Benjamin Fair,
	Avi Fishman, Joel Stanley, Tomer Maimon, Linux I2C,
	OpenBMC Maillist, Linux Kernel Mailing List

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v1] i2c: npcm7xx: Support changing bus speed using debugfs.
  2020-10-01 15:40     ` Andy Shevchenko
@ 2020-10-01 17:13       ` Avi Fishman
  2020-10-01 17:40         ` Andy Shevchenko
  0 siblings, 1 reply; 10+ messages in thread
From: Avi Fishman @ 2020-10-01 17:13 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Tali Perry, Andy Shevchenko, Wolfram Sang, Alex Qiu, Kun Yi,
	Benjamin Fair, Joel Stanley, Tomer Maimon, Linux I2C,
	OpenBMC Maillist, Linux Kernel Mailing List

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



-- 
Regards,
Avi

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v1] i2c: npcm7xx: Support changing bus speed using debugfs.
  2020-10-01 17:13       ` Avi Fishman
@ 2020-10-01 17:40         ` Andy Shevchenko
  2020-10-01 18:27           ` Alex Qiu
  0 siblings, 1 reply; 10+ messages in thread
From: Andy Shevchenko @ 2020-10-01 17:40 UTC (permalink / raw)
  To: Avi Fishman
  Cc: Tali Perry, Wolfram Sang, Alex Qiu, Kun Yi, Benjamin Fair,
	Joel Stanley, Tomer Maimon, Linux I2C, OpenBMC Maillist,
	Linux Kernel Mailing List

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.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v1] i2c: npcm7xx: Support changing bus speed using debugfs.
  2020-10-01 17:40         ` Andy Shevchenko
@ 2020-10-01 18:27           ` Alex Qiu
  2020-10-01 18:41             ` Avi Fishman
  0 siblings, 1 reply; 10+ messages in thread
From: Alex Qiu @ 2020-10-01 18:27 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Avi Fishman, Tali Perry, Wolfram Sang, Kun Yi, Benjamin Fair,
	Joel Stanley, Tomer Maimon, Linux I2C, OpenBMC Maillist,
	Linux Kernel Mailing List

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v1] i2c: npcm7xx: Support changing bus speed using debugfs.
  2020-10-01 18:27           ` Alex Qiu
@ 2020-10-01 18:41             ` Avi Fishman
  2020-10-01 18:51               ` Andy Shevchenko
  0 siblings, 1 reply; 10+ messages in thread
From: Avi Fishman @ 2020-10-01 18:41 UTC (permalink / raw)
  To: Alex Qiu
  Cc: Andy Shevchenko, Tali Perry, Wolfram Sang, Kun Yi, Benjamin Fair,
	Joel Stanley, Tomer Maimon, Linux I2C, OpenBMC Maillist,
	Linux Kernel Mailing List

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



-- 
Regards,
Avi

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v1] i2c: npcm7xx: Support changing bus speed using debugfs.
  2020-10-01 18:41             ` Avi Fishman
@ 2020-10-01 18:51               ` Andy Shevchenko
  2020-10-01 21:11                 ` Alex Qiu
  0 siblings, 1 reply; 10+ messages in thread
From: Andy Shevchenko @ 2020-10-01 18:51 UTC (permalink / raw)
  To: Avi Fishman
  Cc: Alex Qiu, Tali Perry, Wolfram Sang, Kun Yi, Benjamin Fair,
	Joel Stanley, Tomer Maimon, Linux I2C, OpenBMC Maillist,
	Linux Kernel Mailing List

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.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v1] i2c: npcm7xx: Support changing bus speed using debugfs.
  2020-10-01 18:51               ` Andy Shevchenko
@ 2020-10-01 21:11                 ` Alex Qiu
  0 siblings, 0 replies; 10+ messages in thread
From: Alex Qiu @ 2020-10-01 21:11 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Avi Fishman, Tali Perry, Wolfram Sang, Kun Yi, Benjamin Fair,
	Joel Stanley, Tomer Maimon, Linux I2C, OpenBMC Maillist,
	Linux Kernel Mailing List

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2020-10-01 21:11 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-30  7:13 [PATCH v1] i2c: npcm7xx: Support changing bus speed using debugfs Tali Perry
2020-09-30  9:31 ` Andy Shevchenko
2020-10-01  5:32   ` Tali Perry
2020-10-01 15:40     ` Andy Shevchenko
2020-10-01 17:13       ` Avi Fishman
2020-10-01 17:40         ` Andy Shevchenko
2020-10-01 18:27           ` Alex Qiu
2020-10-01 18:41             ` Avi Fishman
2020-10-01 18:51               ` Andy Shevchenko
2020-10-01 21:11                 ` Alex Qiu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).