Hi!
Wolfram, question for you below!
On 2021-01-12 00:24, Vadim Pasternak wrote:
> Hi Peter,
>
>> -----Original Message-----
>> From: Peter Rosin <peda@xxxxxxxxxx>
>> Sent: Monday, January 11, 2021 11:23 PM
>> To: Vadim Pasternak <vadimp@xxxxxxxxxx>; wsa@xxxxxxxxxxxxx
>> Cc: linux-i2c@xxxxxxxxxxxxxxx
>> 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:
>>> Hi Peter,
>>>
>>>> -----Original Message-----
>>>> From: Peter Rosin <peda@xxxxxxxxxx>
>>>> Sent: Friday, January 08, 2021 10:02 AM
>>>> To: Vadim Pasternak <vadimp@xxxxxxxxxx>; wsa@xxxxxxxxxxxxx
>>>> Cc: linux-i2c@xxxxxxxxxxxxxxx
>>>> 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:
>>>>> Hi Peter,
>>>>>
>>>>> Thank you very much for review.
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Peter Rosin <peda@xxxxxxxxxx>
>>>>>> Sent: Thursday, January 07, 2021 12:04 PM
>>>>>> To: Vadim Pasternak <vadimp@xxxxxxxxxx>; wsa@xxxxxxxxxxxxx
>>>>>> Cc: linux-i2c@xxxxxxxxxxxxxxx
>>>>>> 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:
>>>>>>> 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@xxxxxxxxxx>
>>>>>>> Reviewed-by: Michael Shych <michaelsh@xxxxxxxxxx>
>>>>>>> ---
>>>>>>> drivers/i2c/muxes/i2c-mux-mlxcpld.c | 57
>>>>>> +++++++++++++++++++++++++++--------
>>>>>>> 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.
>>>>>> 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.
>>>>>>
>>>>>>> };
>>>>>>>
>>>>>>> /* 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.
>>>>>
>>>>> 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,
>>>>> 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].
>>>>> 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
>>>> l=3 [25-dc-04]
>>>>> 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
>>>> l=1 [88]
>>>>> cat-17623 [004] .... 1152583.811281: i2c_read: i2c-1 #1 a=062
>> f=0001
>>>> l=2
>>>>> cat-17623 [004] .... 1152583.811700: i2c_reply: i2c-1 #1 a=062
>> f=0001
>>>> l=2 [2f-f0]
>>>>> 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
>>>> l=3 [25-dc-00]
>>>>>
>>>>> 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.
>>>> - 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.
>> 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.
