Hi!

Wolfram, question for you below!

On 2021-01-12 00:24, Vadim Pasternak wrote:

Hi Peter,

quoted

-----Original Message-----
From: Peter Rosin <redacted>
Sent: Monday, January 11, 2021 11:23 PM
To: Vadim Pasternak <vadimp@nvidia.com>; wsa@the-dreams.de
Cc: linux-i2c@vger.kernel.org
Subject: Re: [Re-send: PATCH i2c-next 5/6] i2c: mux: mlxcpld: Extend driver to
support word address space devices

Hi!

On 2021-01-11 19:11, Vadim Pasternak wrote:

quoted

Hi Peter,

quoted

-----Original Message-----
From: Peter Rosin <redacted>
Sent: Friday, January 08, 2021 10:02 AM
To: Vadim Pasternak <vadimp@nvidia.com>; wsa@the-dreams.de
Cc: linux-i2c@vger.kernel.org
Subject: Re: [Re-send: PATCH i2c-next 5/6] i2c: mux: mlxcpld: Extend
driver to support word address space devices

Hi!

On 2021-01-07 21:43, Vadim Pasternak wrote:

quoted

Hi Peter,

Thank you very much for review.

quoted

-----Original Message-----
From: Peter Rosin <redacted>
Sent: Thursday, January 07, 2021 12:04 PM
To: Vadim Pasternak <vadimp@nvidia.com>; wsa@the-dreams.de
Cc: linux-i2c@vger.kernel.org
Subject: Re: [Re-send: PATCH i2c-next 5/6] i2c: mux: mlxcpld:
Extend driver to support word address space devices

Hi!

Again, sorry for the late review.

On 2020-11-18 15:44, Vadim Pasternak wrote:

quoted

Extend driver to allow I2C routing control through CPLD devices
with word address space. Till now only CPLD devices with byte
address space have been supported.

Signed-off-by: Vadim Pasternak <vadimp@nvidia.com>
Reviewed-by: Michael Shych <michaelsh@nvidia.com>
---
 drivers/i2c/muxes/i2c-mux-mlxcpld.c   | 57

+++++++++++++++++++++++++++--------

quoted

 include/linux/platform_data/mlxcpld.h |  2 ++
 2 files changed, 47 insertions(+), 12 deletions(-)

diff --git a/drivers/i2c/muxes/i2c-mux-mlxcpld.c

b/drivers/i2c/muxes/i2c-mux-mlxcpld.c
index 6bb8caecf8e8..c76180919fc3 100644

--- a/drivers/i2c/muxes/i2c-mux-mlxcpld.c
+++ b/drivers/i2c/muxes/i2c-mux-mlxcpld.c

@@ -21,11 +21,13 @@
  * @last_chan - last register value
  * @client - I2C device client
  * @pdata: platform data
+ * @sel_buf: I2C message buffer for mux select 16 bits
+ transactions
  */
 struct mlxcpld_mux {
 	u8 last_chan;
 	struct i2c_client *client;
 	struct mlxcpld_mux_plat_data pdata;
+	u8 sel_buf[3];

I think it's a mistake to have this buffer here. I'd rather create
a buffer on the stack in mlxcpld_mux_reg_write() and fill it with
values for

every xfer.

quoted

quoted

Sure, I bet there are external locks that prevent any clobbering of
the buffer, but it's so small that it really can be on the stack.

quoted

 };

 /* MUX logic description.

@@ -60,26 +62,42 @@ struct mlxcpld_mux {
  * for this as they will try to lock adapter a second time.
  */
 static int mlxcpld_mux_reg_write(struct i2c_adapter *adap,
-				 struct mlxcpld_mux *mux, u8 val)
+				 struct mlxcpld_mux *mux, int chan)
 {
 	struct i2c_client *client = mux->client;
-	union i2c_smbus_data data = { .byte = val };
-
-	return __i2c_smbus_xfer(adap, client->addr, client->flags,
-				I2C_SMBUS_WRITE, mux-

pdata.sel_reg_addr,
-				I2C_SMBUS_BYTE_DATA, &data);
+	union i2c_smbus_data data;
+	struct i2c_msg msg;
+
+	switch (mux->pdata.reg_size) {
+	case 1:
+		data.byte = (chan < 0) ? 0 : chan;
+		return __i2c_smbus_xfer(adap, client->addr, client->flags,
+					I2C_SMBUS_WRITE,
+					mux->pdata.sel_reg_addr,
+					I2C_SMBUS_BYTE_DATA, &data);
+	case 2:
+		mux->sel_buf[mux->pdata.reg_size] = (chan < 0) ? 0 :
+						    mux-
pdata.adap_ids[chan];

I get the feeling that you are desperatly trying to get some
specific numbering in user space.

The adapter id is one thing.
The mux channel is one thing.
The value in the register is one thing.

Often, it can make things easier with an easy mapping between the
latter two, but you program the system global I2C adapter id into
the channel selection register of the mux. That is problematic. Just don't.

OK, I will explain what I am trying to get.
This is not something related to the user space.

I want to access some device, located on a line card, which is replaceable.
This is for modular system, which can be equipped with the different
type of line cards.

I have mux selector register in line card CPLD, which is located at
some offset in CPLD register space, f.e. 0x25dc. On other systems it
could

different offset.

quoted

For this line card type in pdata.adap_ids[] channels mapping looks like:
{
	0x04, 0x05, 0x06, 0x07, 0x08, 0x10, 0x20, 0x21, 0x22, 0x23, 0x40, 0x41,
	0x42, 0x43, 0x44, 0x45, 0x46, 0x47, 0x48, 0x49, 0x4a, 0x4b, 0x4c, 0x4d,
	0x4e, 0x4f
};
Ids from 0x01 - 0x0f are used for access to devices like  voltage
regulators,

hotswap,

quoted

	EEPROMs, iio, etcetera.
Ids from 0x10 are used for FPGAs.
Ids from 0x20 are used for gearboxes.
Ids from 0x40 are used for QSFP.
On other line card type it could be different device tree, but it
still will follow the same convention.

CPLD is connected to some upper adapter at address 0x32, and device
on line card is connected to adapter = base_nr * slot +

pdata.adap_ids[channel].

quoted

quoted

quoted

For example, base_nr is 100, slot, at which line card is inserted
is 1, channel 0 will be be configured for adapter 104.

And access will be as below:
cat /sys/bus/i2c/devices/104-0062/hwmon/hwmon5/in1_input
11750

             cat-17623   [004] .... 1152583.810824: i2c_write: i2c-1 #0 a=032

f=0000

quoted

quoted

l=3 [25-dc-04]

quoted

             cat-17623   [004] .... 1152583.811276: i2c_result: i2c-1 n=1 ret=1
             cat-17623   [004] .... 1152583.811281: i2c_write: i2c-1 #0 a=062

f=0000

quoted

quoted

l=1 [88]

quoted

             cat-17623   [004] .... 1152583.811281: i2c_read: i2c-1 #1 a=062

f=0001

quoted

quoted

l=2

quoted

             cat-17623   [004] .... 1152583.811700: i2c_reply: i2c-1 #1 a=062

f=0001

quoted

quoted

l=2 [2f-f0]

quoted

             cat-17623   [004] .... 1152583.811700: i2c_result: i2c-1 n=2 ret=2
             cat-17623   [004] .... 1152583.811704: i2c_write: i2c-1 #0 a=032

f=0000

quoted

quoted

l=3 [25-dc-00]

quoted

When the same line card is inserted for example at slot 3, the
adapter, to which this device is connected will be 304.

Yes, I think I get it. You are however not introducing base_nr until
6/6, so at this point the code makes no sense. But even after 6/6
with base_nr in place, I suggest the following:

- The adap_ids array is for forceing the system global adapter id.
Leave this variable
  alone and let it continue to do what it does, and only that. Or...
- Since you stated somewhere that there are no users of this drivers,
I'd be happy to just
  see the adap_ids variable deleted, i.e. I see no need to force the adapter

id.

quoted

quoted

- Instead of reusing adap_ids, intruduce a new "channel" array and
fill it with the same
  values that you provide in adap_ids, and then have the driver feed
them to the 3rd arg
  of i2c_mux_add_adapter(), i.e. chan_id.

Would that work for you? Or do you somehow depend on predictable
adapter ids?

I can drop adap_id[]s, use chan_ids[] instead and modify loop for
adding Adapters like:
	for (num = 0; num < pdata->num_adaps; num++) {;
		err = i2c_mux_add_adapter(muxc, pdata->base_nr + num,
					  pdata->chan_ids[num], 0);
		if (err)
			goto virt_reg_failed;
	}

In such way I can keep convention for adapters numbering for card in
slot 'n', nrs will be:
Form 100 * n + 1 - for voltage  regulators, hotswap, EEPROMs, iio, ...
From 100 *n + 16 - for FPGAs.
From 100 * n + 32 - for gearboxes.
From 100 * n + 64 - for QSFP.

Would it be OK?

What convention are you talking about? What makes it interesting to force
specific adapter IDs? I just don't see the point. I would do

		err = i2c_mux_add_adapter(muxc, 0, pdata->chan_ids[num],
0);

and let the adapted ID be whatever the I2C core makes up.

What's wrong with that?

The motivation it to provide support for new modular systems which
could be equipped with the different types of replaceable line cards
and management board.

The line cards could be of different types and could have different
I2C topolgy:
- Line cards with 16x100Gbe QSFP28 Ethernet ports.
- Line cards with 8x200Gbe QSFP28 Ethernet ports.
- Line cards with 4x400Gbe QSFP-DD Ethernet ports.
- Smart cards equipped with Nvidia ARM CPU for offloading and for fast
  access to the storage (EBoF).
- Fabric InfiniBand cards for inter-connection.

The first version of modular system will be equipped with 8 slots.

With no enforcement, for example, it could be the next bus assignments:
- if system is booted with empty slot number one, and with line card at slot
  number 2, i2c devices i2c-{n1} - i2c-{n2} will be created for line card at the slot 2.
- if system is booted with line cards at slot 1 and at slot 2, devices i2c-{n1} - i2c-{n2}
  this time will be associated with line card at slot 1, while i2c-{n2+1} - i2c-{n2*2} will
  be associated with line card at slot 2.
- line cards could are removed and then re-inserted in some random order, and it'll
  also could change bus indexes for line card inserted at the same slot.   

It'll make a big challenge for any user application, which wants to use /dev/i2c-{x}.

With enforcement I can avoid this situation.
So, for fixed system it would be fine to have base_nr equal zero, but for modular
I'd like to have base_nr = f(slot), f.e. 100 * slot.

My confusion stems from your response to my note "I get the feeling that you are
desperatly trying to get some specific numbering in user space", when you said:

	OK, I will explain what I am trying to get.
	This is not something related to the user space.

So, you had me confused. :-)

Wolfram, is there a better way to get something stable for user space to
interact with? Is there maybe a way to do this with aliases or something?
Setting up an ad-hoc scheme for forcing the adapter IDs feels a bit outdated.

quoted

Trying to force specific adapater IDs risks failure whenever any of those IDs
happen to be taken, and you have no way of preallocating some range the I2C
core should not use. The only way to do what you do is to select some high
enough ID range and hope for the best. But what if someone else does the
same thing? It's just a slippery slope. So, why?

Yes, it could happen with fixed system, but in this case I expect base_nr to be
zero and IDs will be allocated just from free pool.
But for modular system availability of specific IDs could be granted.

With your latest suggested code, setting base_nr to zero will not trigger
allocation from the free pool. Well, the first channel would be, but
that's ... not practical.

Cheers,
Peter

Otherwise I'll have IDs reordering, each time when some devices are removed/
inserted.