* Two separate i2c transfers @ 2020-05-14 12:41 Adamski, Krzysztof (Nokia - PL/Wrocław) 2020-05-14 14:50 ` Wolfram Sang 0 siblings, 1 reply; 14+ messages in thread From: Adamski, Krzysztof (Nokia - PL/Wrocław) @ 2020-05-14 12:41 UTC (permalink / raw) To: Wolfram Sang; +Cc: linux-i2c Hi, I have a problem that I think cannot be currently easily addressed by I2C framework in the kernel and I'm seeking for an advice on how to approach this. I have an I2C device that can be accessed from two I2C masters connected to I2C bus master selector channels. Both masters must do such a sequence before performing long operation: 1. Read status register. 2. If busy flag is not set - write to a register to start operation (this sets busy flag), otherwise exit. but we cannot call "start operation" if busy flag is already set. We can solve this problem by keeping our channel selected by BMS between operation 1 and 2 but that is not possible via i2c framework right now as i2c_mux_master_xfer() will always do select(); i2c_transfer(); deselect() and we need to do two separate calls to i2c_transfer() (as we have to skip the 2nd one if busy flag is returned by the first one). There is no way to pass flags to the I2C transfer currently, we can only pass flags to individual messages. A dirty solution would be to introduce a flag like I2C_M_NO_DESELECT that would be checked in the first (or last) message passed to i2c_mux_master_xfer() (it would be ignored by normal i2c adapters) before calling deselect(). I'm afraid, however, that we might need something similar for select as I imagine some i2c muxes might return NAK if you try to select some channel while other is already selected. But maybe there is some better way of handling that you could suggest? I imagine this is not a really common scenario but also think we might not be the only ones with such a need technically this is achievable - the only problem is we cannot express this in current code. Best regards, Krzysztof Adamski ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Two separate i2c transfers 2020-05-14 12:41 Two separate i2c transfers Adamski, Krzysztof (Nokia - PL/Wrocław) @ 2020-05-14 14:50 ` Wolfram Sang 2020-05-15 7:04 ` Adamski, Krzysztof (Nokia - PL/Wrocław) 0 siblings, 1 reply; 14+ messages in thread From: Wolfram Sang @ 2020-05-14 14:50 UTC (permalink / raw) To: Adamski, Krzysztof (Nokia - PL/Wrocław), Peter Rosin; +Cc: linux-i2c [-- Attachment #1: Type: text/plain, Size: 791 bytes --] Hi Krzysztof, On Thu, May 14, 2020 at 02:41:17PM +0200, Adamski, Krzysztof (Nokia - PL/Wrocław) wrote: Adding Peter as the mux maintainer to CC. > I have a problem that I think cannot be currently easily addressed by I2C framework in the kernel and I'm seeking for an > advice on how to approach this. I have an I2C device that can be accessed from two I2C masters connected to I2C bus > master selector channels. Both masters must do such a sequence before performing long operation: I need a diagram of that setup. What is the BMS? A chip? Some software? Can you draw a graph and give names of chips etc...? And, of course, why on earth do you need to access the same chip from two masters within one Linux? :) (That's how I understood it) Regards, Wolfram [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Two separate i2c transfers 2020-05-14 14:50 ` Wolfram Sang @ 2020-05-15 7:04 ` Adamski, Krzysztof (Nokia - PL/Wrocław) 2020-05-15 7:53 ` Wolfram Sang 2020-05-15 8:02 ` Peter Rosin 0 siblings, 2 replies; 14+ messages in thread From: Adamski, Krzysztof (Nokia - PL/Wrocław) @ 2020-05-15 7:04 UTC (permalink / raw) To: Wolfram Sang, Peter Rosin; +Cc: linux-i2c Hi Wolfram, Peter, Thank you for the quick response. W dniu 14.05.2020 o 16:50, Wolfram Sang pisze: > Hi Krzysztof, > > On Thu, May 14, 2020 at 02:41:17PM +0200, Adamski, Krzysztof (Nokia - PL/Wrocław) wrote: > > Adding Peter as the mux maintainer to CC. > >> I have a problem that I think cannot be currently easily addressed by I2C framework in the kernel and I'm seeking for an >> advice on how to approach this. I have an I2C device that can be accessed from two I2C masters connected to I2C bus >> master selector channels. Both masters must do such a sequence before performing long operation: > > I need a diagram of that setup. What is the BMS? A chip? Some software? > Can you draw a graph and give names of chips etc...? > > And, of course, why on earth do you need to access the same chip from > two masters within one Linux? :) (That's how I understood it) > A diagram might be good indeed as it seems to be hard to explain. I don't use two masters from one Linux, both masters are own two separate, independent systems and each of them needs access to the target device. BMS is a commonly available chip - PCA9641 2-channel I2C-bus master arbiter but the problem is not related to any specific arbitrator device. The target device is also a physical commonly available chip and it is designed in a way that the operation we are performing might not be interrupted by other transfers except reading status register. The arbitrator device is there to temporary block access from the other system if the first one is doing its transactions so it gives us a possibility to do a mutex lock on the bus. Here's my try to draw this, hopefully this clears things up: +---------------+ | Target device | +---------------+ | +-----+-----|----+------+ | | Upstream | | | +----------+ | | Bus Master Arbiter | | PCA9641 | | +---+ +---+ | | |Ch0| |Ch1| | +----+---+---+---+------+ | | +--------------------------+ | | +--------------------------+ | System 1 +------------+| | | |+------------+ System 2 | | | I2C Master |+---+ +---+| I2C Master | | | +------------+| |+------------+ | | +-----+ | | +-----+ | | | CPU | | | | CPU | | | +-----+ | | +-----+ | +--------------------------+ +--------------------------+ I was thinking that maybe a lock like this could be expressed by i2c_lock_bus with some special flag that would make sure no deselect is called in i2c_mux_master_xfer() (and would be ignored if our parent is not an arbiter)? We already have the I2C_MUX_ARBITRATOR flag and the i2c_mux_core does have an arbitrator bool so it is just a matter of allowing to do this kind of deeper lock on the bus. Best regards, Krzysztof Adamski ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Two separate i2c transfers 2020-05-15 7:04 ` Adamski, Krzysztof (Nokia - PL/Wrocław) @ 2020-05-15 7:53 ` Wolfram Sang 2020-05-15 8:51 ` Adamski, Krzysztof (Nokia - PL/Wrocław) 2020-05-15 8:02 ` Peter Rosin 1 sibling, 1 reply; 14+ messages in thread From: Wolfram Sang @ 2020-05-15 7:53 UTC (permalink / raw) To: Adamski, Krzysztof (Nokia - PL/Wrocław); +Cc: Peter Rosin, linux-i2c [-- Attachment #1: Type: text/plain, Size: 2963 bytes --] > A diagram might be good indeed as it seems to be hard to explain. I don't use two masters from one Linux, both masters > are own two separate, independent systems and each of them needs access to the target device. BMS is a commonly > available chip - PCA9641 2-channel I2C-bus master arbiter but the problem is not related to any specific arbitrator > device. The target device is also a physical commonly available chip and it is designed in a way that the operation we > are performing might not be interrupted by other transfers except reading status register. The arbitrator device is > there to temporary block access from the other system if the first one is doing its transactions so it gives us a > possibility to do a mutex lock on the bus. > > Here's my try to draw this, hopefully this clears things up: > > +---------------+ > | Target device | > +---------------+ > | > +-----+-----|----+------+ > | | Upstream | | > | +----------+ | > | Bus Master Arbiter | > | PCA9641 | > | +---+ +---+ | > | |Ch0| |Ch1| | > +----+---+---+---+------+ > | | > +--------------------------+ | | +--------------------------+ > | System 1 +------------+| | | |+------------+ System 2 | > | | I2C Master |+---+ +---+| I2C Master | | > | +------------+| |+------------+ | > | +-----+ | | +-----+ | > | | CPU | | | | CPU | | > | +-----+ | | +-----+ | > +--------------------------+ +--------------------------+ > > I was thinking that maybe a lock like this could be expressed by i2c_lock_bus with some special flag that would make > sure no deselect is called in i2c_mux_master_xfer() (and would be ignored if our parent is not an arbiter)? We already > have the I2C_MUX_ARBITRATOR flag and the i2c_mux_core does have an arbitrator bool so it is just a matter of allowing to > do this kind of deeper lock on the bus. Thanks for this explanation. Much more clear to me now. However, Peter might have way more insight than me because he was already working on PCA9641 driver a year ago and I don't know how the arbitration was designed there. http://patchwork.ozlabs.org/project/linux-i2c/list/?series=95793 (Do you use this driver or a custom one?) I'd think that the PCA9641 driver should return -EBUSY if another master is active, so we'd have the lock on that level. But I may be totally missing some detail here. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Two separate i2c transfers 2020-05-15 7:53 ` Wolfram Sang @ 2020-05-15 8:51 ` Adamski, Krzysztof (Nokia - PL/Wrocław) 2020-05-15 9:20 ` Wolfram Sang 0 siblings, 1 reply; 14+ messages in thread From: Adamski, Krzysztof (Nokia - PL/Wrocław) @ 2020-05-15 8:51 UTC (permalink / raw) To: Wolfram Sang; +Cc: Peter Rosin, linux-i2c Hi Wolfram, W dniu 15.05.2020 o 09:53, Wolfram Sang pisze: > > http://patchwork.ozlabs.org/project/linux-i2c/list/?series=95793 > > (Do you use this driver or a custom one?) Unfortunatly not right now. For historical reasons we have a custom one but I would like to move to this upstream one in near future. > > I'd think that the PCA9641 driver should return -EBUSY if another master > is active, so we'd have the lock on that level. But I may be totally > missing some detail here. That is a good point however right now the logic in i2c-mux.c is like this: ret = muxc->select(muxc, priv->chan_id); if (ret >= 0) ret = i2c_transfer(parent, msgs, num); if (muxc->deselect) muxc->deselect(muxc, priv->chan_id); So this does not really solve my problem - even if we do get the lock by calling select(), we will simply release it as soon as our i2c_transfer() is finished (and will let the other master take it, breaking atomicity of the two operations I need to perform on target device) and this is exactly what I need to avoid. BTW the deselect is called regardless of the return code of select() - even if it returns an error. I'm not sure if this might cause some problems (unbalanced numer of select/deselect operation) but I find it a little odd. Is this deliberate? Best regards, Krzysztof Adamski ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Two separate i2c transfers 2020-05-15 8:51 ` Adamski, Krzysztof (Nokia - PL/Wrocław) @ 2020-05-15 9:20 ` Wolfram Sang 2020-05-15 9:24 ` Peter Rosin 2020-05-15 9:46 ` Adamski, Krzysztof (Nokia - PL/Wrocław) 0 siblings, 2 replies; 14+ messages in thread From: Wolfram Sang @ 2020-05-15 9:20 UTC (permalink / raw) To: Adamski, Krzysztof (Nokia - PL/Wrocław); +Cc: Peter Rosin, linux-i2c [-- Attachment #1: Type: text/plain, Size: 544 bytes --] > So this does not really solve my problem - even if we do get the lock > by calling select(), we will simply release it as soon as our > i2c_transfer() is finished (and will let the other master take it, > breaking atomicity of the two operations I need to perform on target > device) and this is exactly what I need to avoid. I was assuming that once you have the lock from the arbitrator you can be sure that the other master is not active and, thus, you maybe don't need to read the status register at all? But I am probably wrong here. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Two separate i2c transfers 2020-05-15 9:20 ` Wolfram Sang @ 2020-05-15 9:24 ` Peter Rosin 2020-05-15 9:31 ` Wolfram Sang 2020-05-15 9:46 ` Adamski, Krzysztof (Nokia - PL/Wrocław) 1 sibling, 1 reply; 14+ messages in thread From: Peter Rosin @ 2020-05-15 9:24 UTC (permalink / raw) To: Wolfram Sang, Adamski, Krzysztof (Nokia - PL/Wrocław); +Cc: linux-i2c On 2020-05-15 11:20, Wolfram Sang wrote: > >> So this does not really solve my problem - even if we do get the lock >> by calling select(), we will simply release it as soon as our >> i2c_transfer() is finished (and will let the other master take it, >> breaking atomicity of the two operations I need to perform on target >> device) and this is exactly what I need to avoid. > > I was assuming that once you have the lock from the arbitrator you can > be sure that the other master is not active and, thus, you maybe don't > need to read the status register at all? But I am probably wrong here. You are right in that when you have the lock from the arbtrator, you lock the other master(s) out, but the issue here is that the mux code releases the arbitrator lock after each xfer. Krzysztof needs to do two xfers with the arbitrator lock held over both so that another master cannot sneak in and change the world view. Cheers, Peter ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Two separate i2c transfers 2020-05-15 9:24 ` Peter Rosin @ 2020-05-15 9:31 ` Wolfram Sang 0 siblings, 0 replies; 14+ messages in thread From: Wolfram Sang @ 2020-05-15 9:31 UTC (permalink / raw) To: Peter Rosin; +Cc: Adamski, Krzysztof (Nokia - PL/Wrocław), linux-i2c [-- Attachment #1: Type: text/plain, Size: 930 bytes --] > > I was assuming that once you have the lock from the arbitrator you can > > be sure that the other master is not active and, thus, you maybe don't > > need to read the status register at all? But I am probably wrong here. > > You are right in that when you have the lock from the arbtrator, you > lock the other master(s) out, but the issue here is that the mux code > releases the arbitrator lock after each xfer. Krzysztof needs to do > two xfers with the arbitrator lock held over both so that another > master cannot sneak in and change the world view. Yes. My probably wrong assumption was that the first part of the transfer (reading a status reg) was only to check if another master has started the device and is running some operation. So, if we could avoid that this happens by proper locking of the arbitrator, you could either skip the first part of the transaction or just send both in one go. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Two separate i2c transfers 2020-05-15 9:20 ` Wolfram Sang 2020-05-15 9:24 ` Peter Rosin @ 2020-05-15 9:46 ` Adamski, Krzysztof (Nokia - PL/Wrocław) 1 sibling, 0 replies; 14+ messages in thread From: Adamski, Krzysztof (Nokia - PL/Wrocław) @ 2020-05-15 9:46 UTC (permalink / raw) To: Wolfram Sang; +Cc: Peter Rosin, linux-i2c Hi Wolfram W dniu 15.05.2020 o 11:20, Wolfram Sang pisze: > >> So this does not really solve my problem - even if we do get the lock >> by calling select(), we will simply release it as soon as our >> i2c_transfer() is finished (and will let the other master take it, >> breaking atomicity of the two operations I need to perform on target >> device) and this is exactly what I need to avoid. > > I was assuming that once you have the lock from the arbitrator you can > be sure that the other master is not active and, thus, you maybe don't > need to read the status register at all? But I am probably wrong here. > The problem here is that the i2c-mux framework will only take lock for one transfer i2c and will always drop this lock after this one transfer. We can send multiple messages in one transaction and keep the lock for the whole time but what I actually needs is to send multiple transactions, not just messages. We are programming flash memory of an target device and we have to poll for some status bits before we can send more data, for example. So there are two possibilities: 1. I could lock the arbitrator for the whole time the programming is done. This way I could indeed skip reading status register. 2. We could lock the arbitrator, read the status register of the target device to check if programming is already running, if it is not, then start it (which will set the bit in status register). Those to operations have to be atomic so I have to hold the lock between the two transactions. None of the two options can be done currently as mux layer will call deselect (releasing the lock) after each transaction and we need more than one transaction (as described above). We need to use the 2nd option, however, as this lets 2nd system still talk to some other devices on the downstream bus while the 1st one is performing the flashing of the target device for example while we're waiting for target device to be ready for next page of data) but this is not that relevant in this discussion. Best regards, Krzysztof Adamski ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Two separate i2c transfers 2020-05-15 7:04 ` Adamski, Krzysztof (Nokia - PL/Wrocław) 2020-05-15 7:53 ` Wolfram Sang @ 2020-05-15 8:02 ` Peter Rosin 2020-05-15 8:36 ` Adamski, Krzysztof (Nokia - PL/Wrocław) 1 sibling, 1 reply; 14+ messages in thread From: Peter Rosin @ 2020-05-15 8:02 UTC (permalink / raw) To: Adamski, Krzysztof (Nokia - PL/Wrocław), Wolfram Sang; +Cc: linux-i2c On 2020-05-15 09:04, Adamski, Krzysztof (Nokia - PL/Wrocław) wrote: > Hi Wolfram, Peter, > > Thank you for the quick response. > > W dniu 14.05.2020 o 16:50, Wolfram Sang pisze: >> Hi Krzysztof, >> >> On Thu, May 14, 2020 at 02:41:17PM +0200, Adamski, Krzysztof (Nokia - PL/Wrocław) wrote: >> >> Adding Peter as the mux maintainer to CC. >> >>> I have a problem that I think cannot be currently easily addressed by I2C framework in the kernel and I'm seeking for an >>> advice on how to approach this. I have an I2C device that can be accessed from two I2C masters connected to I2C bus >>> master selector channels. Both masters must do such a sequence before performing long operation: >> >> I need a diagram of that setup. What is the BMS? A chip? Some software? >> Can you draw a graph and give names of chips etc...? >> >> And, of course, why on earth do you need to access the same chip from >> two masters within one Linux? :) (That's how I understood it) >> *snip* useful layout. > I was thinking that maybe a lock like this could be expressed by i2c_lock_bus with some special flag that would make > sure no deselect is called in i2c_mux_master_xfer() (and would be ignored if our parent is not an arbiter)? We already > have the I2C_MUX_ARBITRATOR flag and the i2c_mux_core does have an arbitrator bool so it is just a matter of allowing to > do this kind of deeper lock on the bus. Yes, that definitely makes it clearer. So, what you need is something more complex than an I2C xfer between select/deselect. Your proposal to add a flag to not do the deselect should probably have a matching flag to not do the select on the following xfer. Otherwise that second xfer is likely to do useless work. This should probably also be two independent flags, so that you can add intermediate xfers that neither select nor deselect. But you also need an explicit deselect mechanism for use if there is a problem half way through... But, I think all that exposes the too much to the users, and while it is certainly possible (most things are), I not a huge fan of it. Maintainers of other subsystems will not know the rules, and drivers are, over time, bound to start using these facilities in half-baked ways. I would rather have some generic I2C mechanism to request more than a single xfer as a unit, such that the muxes/arbs/gates can then lock the mux/arb/gate, do the select, feed the unit to the parent adapter and finally deselect and unlock the mux/arb/gate. With the locking and selecting centralized instead of spread out to all users. This helps avoid bugs where things are not cleaned up on error paths etc. How that "I2C unit" should be constructed is another matter. The cool thing of the day seems to be eBPF programs, but I don't know if that fits here? Note that there are other users needing more complex "units" than xfers. Muxes themselves could use it, and there are a number of drivers that lock the adapter manually, do a couple of unlocked xfers, and then unlock the adapter. All that could probably be cleaned up a bit with a generic "I2C unit" facility. Cheers, Peter ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Two separate i2c transfers 2020-05-15 8:02 ` Peter Rosin @ 2020-05-15 8:36 ` Adamski, Krzysztof (Nokia - PL/Wrocław) 2020-05-15 21:19 ` Peter Rosin 0 siblings, 1 reply; 14+ messages in thread From: Adamski, Krzysztof (Nokia - PL/Wrocław) @ 2020-05-15 8:36 UTC (permalink / raw) To: Peter Rosin, Wolfram Sang; +Cc: linux-i2c Hi Peter, W dniu 15.05.2020 o 10:02, Peter Rosin pisze: > > > On 2020-05-15 09:04, Adamski, Krzysztof (Nokia - PL/Wrocław) wrote: >> Hi Wolfram, Peter, >> >> Thank you for the quick response. >> >> W dniu 14.05.2020 o 16:50, Wolfram Sang pisze: >>> Hi Krzysztof, >>> >>> On Thu, May 14, 2020 at 02:41:17PM +0200, Adamski, Krzysztof (Nokia - PL/Wrocław) wrote: >>> >>> Adding Peter as the mux maintainer to CC. >>> >>>> I have a problem that I think cannot be currently easily addressed by I2C framework in the kernel and I'm seeking for an >>>> advice on how to approach this. I have an I2C device that can be accessed from two I2C masters connected to I2C bus >>>> master selector channels. Both masters must do such a sequence before performing long operation: >>> >>> I need a diagram of that setup. What is the BMS? A chip? Some software? >>> Can you draw a graph and give names of chips etc...? >>> >>> And, of course, why on earth do you need to access the same chip from >>> two masters within one Linux? :) (That's how I understood it) >>> > > *snip* useful layout. > >> I was thinking that maybe a lock like this could be expressed by i2c_lock_bus with some special flag that would make >> sure no deselect is called in i2c_mux_master_xfer() (and would be ignored if our parent is not an arbiter)? We already >> have the I2C_MUX_ARBITRATOR flag and the i2c_mux_core does have an arbitrator bool so it is just a matter of allowing to >> do this kind of deeper lock on the bus. > > Yes, that definitely makes it clearer. So, what you need is something > more complex than an I2C xfer between select/deselect. Your proposal > to add a flag to not do the deselect should probably have a matching > flag to not do the select on the following xfer. Otherwise that second > xfer is likely to do useless work. This should probably also be two > independent flags, so that you can add intermediate xfers that neither > select nor deselect. But you also need an explicit deselect mechanism > for use if there is a problem half way through... > > But, I think all that exposes the too much to the users, and while it > is certainly possible (most things are), I not a huge fan of it. > Maintainers of other subsystems will not know the rules, and drivers > are, over time, bound to start using these facilities in half-baked > ways. > > I would rather have some generic I2C mechanism to request more than > a single xfer as a unit, such that the muxes/arbs/gates can then lock > the mux/arb/gate, do the select, feed the unit to the parent adapter > and finally deselect and unlock the mux/arb/gate. With the locking > and selecting centralized instead of spread out to all users. This > helps avoid bugs where things are not cleaned up on error paths etc. > Hmm.. but isn't such a "unit" expressed right now by using i2c_lock_bus()? If you want to do more transfers and do not want to be disturbed by anyone on local system, you do: i2c_lock_bus(client->adapter, I2C_LOCK_ROOT_ADAPTER); __i2c_transfer(client->adapter, ...); some_logic __ice_transfer(client->adapter, ...); i2c_unlock_bus(client->adapter, I2C_LOCK_ROOT_ADAPTER); right? So my idea would be to extend it to "outside of local system" if we have an arbiter as a parent. To not change existing semantics, that might require additional flag (so such a "deeper lock" would be opt-in): i2c_lock_bus(client->adapter, I2C_LOCK_ROOT_ADAPTER | I2C_LOCK_ARBITRATOR); __i2c_transfer(client->adapter, ...) some_logic __ice_transfer(client->adapter, ...) i2c_unlock_bus(client->adapter, I2C_LOCK_ROOT_ADAPTER | I2C_LOCK_ARBITRATOR); The I2C_LOCK_ARBITER flag would only be relevant for i2c_mux_lock_bus() that could do: if (muxc->arbitrator && flags & I2C_LOCK_ARBITRATOR) { priv->muxc->select(muxc, priv->chan_id); priv->muxc->arbitrator_selected = true; } and unlock could do the opposite: if (muxc->arbitrator && flags & I2C_LOCK_ARBITRATOR) { priv->muxc->select(muxc, priv->chan_id); priv->muxc->arbitrator_selected = true; } Now we would also need to change the xfer functions in i2c-mux.c to check for muxc->arbitrator_selected - if it is set, it would skip doing both select and deselect as this "flag" would mean that selecting/deselecting is done on lock/unlock and not on transfer level. Of course we could also skip the check for muxc->arbitrator if you think such a lock could make sense for other types of muxes as well.. Would that really be confusing / un-intutive ? Best regards, ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Two separate i2c transfers 2020-05-15 8:36 ` Adamski, Krzysztof (Nokia - PL/Wrocław) @ 2020-05-15 21:19 ` Peter Rosin 2020-05-17 21:32 ` Peter Rosin 0 siblings, 1 reply; 14+ messages in thread From: Peter Rosin @ 2020-05-15 21:19 UTC (permalink / raw) To: Adamski, Krzysztof (Nokia - PL/Wrocław), Wolfram Sang; +Cc: linux-i2c On 2020-05-15 10:36, Adamski, Krzysztof (Nokia - PL/Wrocław) wrote: > Hi Peter, > > W dniu 15.05.2020 o 10:02, Peter Rosin pisze: >> >> >> On 2020-05-15 09:04, Adamski, Krzysztof (Nokia - PL/Wrocław) wrote: >>> Hi Wolfram, Peter, >>> >>> Thank you for the quick response. >>> >>> W dniu 14.05.2020 o 16:50, Wolfram Sang pisze: >>>> Hi Krzysztof, >>>> >>>> On Thu, May 14, 2020 at 02:41:17PM +0200, Adamski, Krzysztof (Nokia - PL/Wrocław) wrote: >>>> >>>> Adding Peter as the mux maintainer to CC. >>>> >>>>> I have a problem that I think cannot be currently easily addressed by I2C framework in the kernel and I'm seeking for an >>>>> advice on how to approach this. I have an I2C device that can be accessed from two I2C masters connected to I2C bus >>>>> master selector channels. Both masters must do such a sequence before performing long operation: >>>> >>>> I need a diagram of that setup. What is the BMS? A chip? Some software? >>>> Can you draw a graph and give names of chips etc...? >>>> >>>> And, of course, why on earth do you need to access the same chip from >>>> two masters within one Linux? :) (That's how I understood it) >>>> >> >> *snip* useful layout. >> >>> I was thinking that maybe a lock like this could be expressed by i2c_lock_bus with some special flag that would make >>> sure no deselect is called in i2c_mux_master_xfer() (and would be ignored if our parent is not an arbiter)? We already >>> have the I2C_MUX_ARBITRATOR flag and the i2c_mux_core does have an arbitrator bool so it is just a matter of allowing to >>> do this kind of deeper lock on the bus. >> >> Yes, that definitely makes it clearer. So, what you need is something >> more complex than an I2C xfer between select/deselect. Your proposal >> to add a flag to not do the deselect should probably have a matching >> flag to not do the select on the following xfer. Otherwise that second >> xfer is likely to do useless work. This should probably also be two >> independent flags, so that you can add intermediate xfers that neither >> select nor deselect. But you also need an explicit deselect mechanism >> for use if there is a problem half way through... >> >> But, I think all that exposes the too much to the users, and while it >> is certainly possible (most things are), I not a huge fan of it. >> Maintainers of other subsystems will not know the rules, and drivers >> are, over time, bound to start using these facilities in half-baked >> ways. >> >> I would rather have some generic I2C mechanism to request more than >> a single xfer as a unit, such that the muxes/arbs/gates can then lock >> the mux/arb/gate, do the select, feed the unit to the parent adapter >> and finally deselect and unlock the mux/arb/gate. With the locking >> and selecting centralized instead of spread out to all users. This >> helps avoid bugs where things are not cleaned up on error paths etc. >> > > Hmm.. but isn't such a "unit" expressed right now by using i2c_lock_bus()? If you want to do more transfers and do not > want to be disturbed by anyone on local system, you do: > > i2c_lock_bus(client->adapter, I2C_LOCK_ROOT_ADAPTER); > __i2c_transfer(client->adapter, ...); > some_logic > __ice_transfer(client->adapter, ...); > i2c_unlock_bus(client->adapter, I2C_LOCK_ROOT_ADAPTER); > > right? So my idea would be to extend it to "outside of local system" if we have an arbiter as a parent. To not change > existing semantics, that might require additional flag (so such a "deeper lock" would be opt-in): Ok, now I see what you mean, and I realize that you were talking about a new flag for i2c_bus_lock() all along. Brain-fart on my part. Sorry. Yes, by adding the flag to i2c_bus_(un)lock() you get something that is much closer to existing semantics. > i2c_lock_bus(client->adapter, I2C_LOCK_ROOT_ADAPTER | I2C_LOCK_ARBITRATOR); > __i2c_transfer(client->adapter, ...) > some_logic > __ice_transfer(client->adapter, ...) > i2c_unlock_bus(client->adapter, I2C_LOCK_ROOT_ADAPTER | I2C_LOCK_ARBITRATOR); > > > The I2C_LOCK_ARBITER flag would only be relevant for i2c_mux_lock_bus() that could do: This is not true, you would have to do it in i2c_parent_(un)lock_bus too. The I2C client drivers should not need to know (or care) if the muxes are parent-locked or mux-locked. > if (muxc->arbitrator && flags & I2C_LOCK_ARBITRATOR) { > priv->muxc->select(muxc, priv->chan_id); > priv->muxc->arbitrator_selected = true; > } > > and unlock could do the opposite: > if (muxc->arbitrator && flags & I2C_LOCK_ARBITRATOR) { > priv->muxc->select(muxc, priv->chan_id); > priv->muxc->arbitrator_selected = true; (unselect and false, but I get it) > } > > Now we would also need to change the xfer functions in i2c-mux.c to check for muxc->arbitrator_selected - if it is set, > it would skip doing both select and deselect as this "flag" would mean that selecting/deselecting is done on lock/unlock > and not on transfer level. Of course we could also skip the check for muxc->arbitrator if you think such a lock could > make sense for other types of muxes as well.. I think it may well make sense, so skip the check and name the name of the flag can be just 'selected'. > Would that really be confusing / un-intutive ? No, I guess not. It's pretty much on-par with the existing I2C locking facilities. Hmmm, on second thought, maybe the select/deselect should be part of lock/unlock unconditionally? The big problem with that is that "locking" may fail. And, I now realize that this is the big issue with the whole approach. You need to add a return value to i2c_lock_operations.lock_bus, and that feels like a *big* change. Maybe not technically, the there will be a signal that the function may fail. I.e. every client will, with that return value, get pressure to check and act on locking failures when it is in fact only needed in this new situation. I guess you could do the select/deselect thing conditionally, like you suggested, but make it an error to try to use it with i2c_lock_bus(), and only allow it from i2c_trylock_bus() which already has a return value. But with that, I feel that you need to somehow separate the cases when you want to actually wait for the lock and when you want to give up immediately if the lock is taken. I don't know if I'm making sense. I'm typing as I'm thinking if anyone hadn't noticed... Cheers, Peter ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Two separate i2c transfers 2020-05-15 21:19 ` Peter Rosin @ 2020-05-17 21:32 ` Peter Rosin 2020-05-19 12:59 ` Adamski, Krzysztof (Nokia - PL/Wrocław) 0 siblings, 1 reply; 14+ messages in thread From: Peter Rosin @ 2020-05-17 21:32 UTC (permalink / raw) To: Adamski, Krzysztof (Nokia - PL/Wrocław), Wolfram Sang; +Cc: linux-i2c On 2020-05-15 23:19, Peter Rosin wrote: > > > On 2020-05-15 10:36, Adamski, Krzysztof (Nokia - PL/Wrocław) wrote: >> Hi Peter, >> >> W dniu 15.05.2020 o 10:02, Peter Rosin pisze: >>> >>> >>> On 2020-05-15 09:04, Adamski, Krzysztof (Nokia - PL/Wrocław) wrote: >>>> Hi Wolfram, Peter, >>>> >>>> Thank you for the quick response. >>>> >>>> W dniu 14.05.2020 o 16:50, Wolfram Sang pisze: >>>>> Hi Krzysztof, >>>>> >>>>> On Thu, May 14, 2020 at 02:41:17PM +0200, Adamski, Krzysztof (Nokia - PL/Wrocław) wrote: >>>>> >>>>> Adding Peter as the mux maintainer to CC. >>>>> >>>>>> I have a problem that I think cannot be currently easily addressed by I2C framework in the kernel and I'm seeking for an >>>>>> advice on how to approach this. I have an I2C device that can be accessed from two I2C masters connected to I2C bus >>>>>> master selector channels. Both masters must do such a sequence before performing long operation: >>>>> >>>>> I need a diagram of that setup. What is the BMS? A chip? Some software? >>>>> Can you draw a graph and give names of chips etc...? >>>>> >>>>> And, of course, why on earth do you need to access the same chip from >>>>> two masters within one Linux? :) (That's how I understood it) >>>>> >>> >>> *snip* useful layout. >>> >>>> I was thinking that maybe a lock like this could be expressed by i2c_lock_bus with some special flag that would make >>>> sure no deselect is called in i2c_mux_master_xfer() (and would be ignored if our parent is not an arbiter)? We already >>>> have the I2C_MUX_ARBITRATOR flag and the i2c_mux_core does have an arbitrator bool so it is just a matter of allowing to >>>> do this kind of deeper lock on the bus. >>> >>> Yes, that definitely makes it clearer. So, what you need is something >>> more complex than an I2C xfer between select/deselect. Your proposal >>> to add a flag to not do the deselect should probably have a matching >>> flag to not do the select on the following xfer. Otherwise that second >>> xfer is likely to do useless work. This should probably also be two >>> independent flags, so that you can add intermediate xfers that neither >>> select nor deselect. But you also need an explicit deselect mechanism >>> for use if there is a problem half way through... >>> >>> But, I think all that exposes the too much to the users, and while it >>> is certainly possible (most things are), I not a huge fan of it. >>> Maintainers of other subsystems will not know the rules, and drivers >>> are, over time, bound to start using these facilities in half-baked >>> ways. >>> >>> I would rather have some generic I2C mechanism to request more than >>> a single xfer as a unit, such that the muxes/arbs/gates can then lock >>> the mux/arb/gate, do the select, feed the unit to the parent adapter >>> and finally deselect and unlock the mux/arb/gate. With the locking >>> and selecting centralized instead of spread out to all users. This >>> helps avoid bugs where things are not cleaned up on error paths etc. >>> >> >> Hmm.. but isn't such a "unit" expressed right now by using i2c_lock_bus()? If you want to do more transfers and do not >> want to be disturbed by anyone on local system, you do: >> >> i2c_lock_bus(client->adapter, I2C_LOCK_ROOT_ADAPTER); >> __i2c_transfer(client->adapter, ...); >> some_logic >> __ice_transfer(client->adapter, ...); >> i2c_unlock_bus(client->adapter, I2C_LOCK_ROOT_ADAPTER); >> >> right? So my idea would be to extend it to "outside of local system" if we have an arbiter as a parent. To not change >> existing semantics, that might require additional flag (so such a "deeper lock" would be opt-in): > > Ok, now I see what you mean, and I realize that you were talking > about a new flag for i2c_bus_lock() all along. Brain-fart on my > part. Sorry. > > Yes, by adding the flag to i2c_bus_(un)lock() you get something that > is much closer to existing semantics. > >> i2c_lock_bus(client->adapter, I2C_LOCK_ROOT_ADAPTER | I2C_LOCK_ARBITRATOR); >> __i2c_transfer(client->adapter, ...) >> some_logic >> __ice_transfer(client->adapter, ...) >> i2c_unlock_bus(client->adapter, I2C_LOCK_ROOT_ADAPTER | I2C_LOCK_ARBITRATOR); >> >> >> The I2C_LOCK_ARBITER flag would only be relevant for i2c_mux_lock_bus() that could do: > > This is not true, you would have to do it in i2c_parent_(un)lock_bus > too. The I2C client drivers should not need to know (or care) if the > muxes are parent-locked or mux-locked. > >> if (muxc->arbitrator && flags & I2C_LOCK_ARBITRATOR) { >> priv->muxc->select(muxc, priv->chan_id); >> priv->muxc->arbitrator_selected = true; >> } >> >> and unlock could do the opposite: >> if (muxc->arbitrator && flags & I2C_LOCK_ARBITRATOR) { >> priv->muxc->select(muxc, priv->chan_id); >> priv->muxc->arbitrator_selected = true; > > (unselect and false, but I get it) > >> } >> >> Now we would also need to change the xfer functions in i2c-mux.c to check for muxc->arbitrator_selected - if it is set, >> it would skip doing both select and deselect as this "flag" would mean that selecting/deselecting is done on lock/unlock >> and not on transfer level. Of course we could also skip the check for muxc->arbitrator if you think such a lock could >> make sense for other types of muxes as well.. > > I think it may well make sense, so skip the check and name the > name of the flag can be just 'selected'. > >> Would that really be confusing / un-intutive ? > > No, I guess not. It's pretty much on-par with the existing I2C locking > facilities. > > Hmmm, on second thought, maybe the select/deselect should be part of > lock/unlock unconditionally? The big problem with that is that > "locking" may fail. And, I now realize that this is the big issue > with the whole approach. You need to add a return value to > i2c_lock_operations.lock_bus, and that feels like a *big* change. > Maybe not technically, the there will be a signal that the function > may fail. I.e. every client will, with that return value, get pressure > to check and act on locking failures when it is in fact only needed in > this new situation. > > I guess you could do the select/deselect thing conditionally, like > you suggested, but make it an error to try to use it with > i2c_lock_bus(), and only allow it from i2c_trylock_bus() which > already has a return value. But with that, I feel that you need to > somehow separate the cases when you want to actually wait for the lock > and when you want to give up immediately if the lock is taken. > > I don't know if I'm making sense. I'm typing as I'm thinking if > anyone hadn't noticed... The return value could be added to i2c_lock_operations.lock_bus, but not be exposed when called through i2c_lock_bus(). That wouldn't bee too bad I guess. Then a separate set of functions could be added for lock+select that do expose the return value (and adds the new flag of course). I.e. i2c_lock_select_bus, i2c_trylock_select_bus and t2c_deselect_unlock_bus. Or something like that. However, I have the feeling that *most* client drivers that do their own i2c_lock_adapter/__i2c_transfer/.../i2c_unlock_adapter really would like to also keep arbitrators locked during the operation to prevent not only the local master from clobbering the chip status. Hmm. New idea. Maybe the arbitrator ->select should remain with the xfer as it is today, but then you set a flag that the arbitrator is selected, and delay the deselect until the unlock (instead of doing it last in the xfer). You would of course need to check the flag before doing the select in the xfer, so that follow-up xfers don't redo the select. Something like this (untested, written in the mail client): static int __i2c_mux_master_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) { struct i2c_mux_priv *priv = adap->algo_data; struct i2c_mux_core *muxc = priv->muxc; struct i2c_adapter *parent = muxc->parent; /* Switch to the right mux port and perform the transfer. */ if (!muxc->selected) { int ret = muxc->select(muxc, priv->chan_id); muxc->selected = true; if (ret < 0) return ret; } return __i2c_transfer(parent, msgs, num); } static void i2c_parent_unlock_bus(struct i2c_adapter *adapter, unsigned int flags) { struct i2c_mux_priv *priv = adapter->algo_data; struct i2c_mux_core *muxc = priv->muxc; struct i2c_adapter *parent = muxc->parent; if (muxc->selected && muxc->deselect) muxc->deselect(muxc, priv->chan_id); muxc->selected = false; i2c_unlock_bus(parent, flags); rt_mutex_unlock(&parent->mux_lock); } (i2c_mux_master_xfer(), __i2c_mux_smbus_xfer(), i2c_mux_smbus_xfer() and i2c_mux_unlock_bus() need corresponding changes) One more thing to keep in mind. Some arbs/muxes/gates auto-deselects after the Nth xfer (always the 1st?), so arb/mux/gate drivers must probably have some way to opt out of whatever we do to support "I2C units" across arbs/muxes/gates. Maybe that property mostly applies to gates? Anyway, the above is not a complete solution. Cheers, Peter ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Two separate i2c transfers 2020-05-17 21:32 ` Peter Rosin @ 2020-05-19 12:59 ` Adamski, Krzysztof (Nokia - PL/Wrocław) 0 siblings, 0 replies; 14+ messages in thread From: Adamski, Krzysztof (Nokia - PL/Wrocław) @ 2020-05-19 12:59 UTC (permalink / raw) To: Peter Rosin, Wolfram Sang; +Cc: linux-i2c Hi Peter, Wofram, W dniu 17.05.2020 o 23:32, Peter Rosin pisze: > On 2020-05-15 23:19, Peter Rosin wrote: > > Ok, now I see what you mean, and I realize that you were talking > about a new flag for i2c_bus_lock() all along. Brain-fart on my > part. Sorry. > > Yes, by adding the flag to i2c_bus_(un)lock() you get something that > is much closer to existing semantics. > >>> i2c_lock_bus(client->adapter, I2C_LOCK_ROOT_ADAPTER | I2C_LOCK_ARBITRATOR); >>> __i2c_transfer(client->adapter, ...) >>> some_logic >>> __ice_transfer(client->adapter, ...) >>> i2c_unlock_bus(client->adapter, I2C_LOCK_ROOT_ADAPTER | I2C_LOCK_ARBITRATOR); >>> >>> >>> The I2C_LOCK_ARBITER flag would only be relevant for i2c_mux_lock_bus() that could do: >> This is not true, you would have to do it in i2c_parent_(un)lock_bus >> too. The I2C client drivers should not need to know (or care) if the >> muxes are parent-locked or mux-locked. Yes, I agree. That was what I was thinking about - I meant that it is only relevant to callbacks in i2c-mux.c >> Hmmm, on second thought, maybe the select/deselect should be part of >> lock/unlock unconditionally? The big problem with that is that >> "locking" may fail. And, I now realize that this is the big issue >> with the whole approach. You need to add a return value to >> i2c_lock_operations.lock_bus, and that feels like a *big* change. >> Maybe not technically, the there will be a signal that the function >> may fail. I.e. every client will, with that return value, get pressure >> to check and act on locking failures when it is in fact only needed in >> this new situation. This is indeed a bigger problem I didn't think about earlier. But at I guess we agree that having such a possibility is useful, we just have to find out the best way to implement this. >> >> I guess you could do the select/deselect thing conditionally, like >> you suggested, but make it an error to try to use it with >> i2c_lock_bus(), and only allow it from i2c_trylock_bus() which >> already has a return value. But with that, I feel that you need to >> somehow separate the cases when you want to actually wait for the lock >> and when you want to give up immediately if the lock is taken. But the problem is that currently the select callback does not have a "trylock" semantics - it tries for quite some time before giving up. We would need to introduce some kind of flag to distinguish between the two. But the caller then would have to most likely implement his own timeout loop which is not elegant solution either. > The return value could be added to i2c_lock_operations.lock_bus, but not > be exposed when called through i2c_lock_bus(). That wouldn't bee too bad > I guess. Then a separate set of functions could be added for lock+select > that do expose the return value (and adds the new flag of course). I.e. > i2c_lock_select_bus, i2c_trylock_select_bus and t2c_deselect_unlock_bus. > Or something like that. That would be ok for me but Wolfram would have to comment, I guess. > However, I have the feeling that *most* client drivers that do their > own i2c_lock_adapter/__i2c_transfer/.../i2c_unlock_adapter really would > like to also keep arbitrators locked during the operation to prevent not > only the local master from clobbering the chip status. I'm not sure how useful this is for generic case (if it's "most" or only "some") but as you pointed out, it is not a simple extension of a lock as now the client would have to deal with the fail possibility so I think such a lock should still be considered an opt-in. For this, a new API is good idea. > Hmm. New idea. Maybe the arbitrator ->select should remain with the xfer > as it is today, but then you set a flag that the arbitrator is selected, > and delay the deselect until the unlock (instead of doing it last in the > xfer). You would of course need to check the flag before doing the select > in the xfer, so that follow-up xfers don't redo the select. > > Something like this (untested, written in the mail client): > > static int __i2c_mux_master_xfer(struct i2c_adapter *adap, > struct i2c_msg msgs[], int num) > { > struct i2c_mux_priv *priv = adap->algo_data; > struct i2c_mux_core *muxc = priv->muxc; > struct i2c_adapter *parent = muxc->parent; > > /* Switch to the right mux port and perform the transfer. */ > > if (!muxc->selected) { > int ret = muxc->select(muxc, priv->chan_id); > muxc->selected = true; > if (ret < 0) > return ret; > } > > return __i2c_transfer(parent, msgs, num); > } > > static void i2c_parent_unlock_bus(struct i2c_adapter *adapter, > unsigned int flags) > { > struct i2c_mux_priv *priv = adapter->algo_data; > struct i2c_mux_core *muxc = priv->muxc; > struct i2c_adapter *parent = muxc->parent; > > if (muxc->selected && muxc->deselect) > muxc->deselect(muxc, priv->chan_id); > muxc->selected = false; > i2c_unlock_bus(parent, flags); > rt_mutex_unlock(&parent->mux_lock); > } > > (i2c_mux_master_xfer(), __i2c_mux_smbus_xfer(), i2c_mux_smbus_xfer() and > i2c_mux_unlock_bus() need corresponding changes) Something like this was my initial idea. But I'm not that sure if such a "deeper lock" should indeed be default option for all callers. Could that introduce some regressions? Especially if we somehow get deselected automatically by the device (like noted below). > One more thing to keep in mind. Some arbs/muxes/gates auto-deselects after > the Nth xfer (always the 1st?), so arb/mux/gate drivers must probably have > some way to opt out of whatever we do to support "I2C units" across > arbs/muxes/gates. Maybe that property mostly applies to gates? Anyway, the > above is not a complete solution. Also there is a possibility that a mux could be deselected after some idle time as well. I think the PCA9461 does support something like that but I would have to check. Maybe a hybrid approach would be best, then? The user could provide a flag to i2c_bus_lock() to opt-in for a deeper lock (to avoid possible regressions and changed behavior) but the actual select would be called in __i2c_mux_mater_xfer and deselect in unlock_bus as you suggested above (but only if the flag was provided to i2c_bus_{un,}lock(). I'm not sure if this is better than introducing i2c_{un,}lock_select_bus() as suggested previously, however. Wolfram, what are your thoughts? I think an additional point of view would be useful here. Krzysztof ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2020-05-19 12:59 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-05-14 12:41 Two separate i2c transfers Adamski, Krzysztof (Nokia - PL/Wrocław) 2020-05-14 14:50 ` Wolfram Sang 2020-05-15 7:04 ` Adamski, Krzysztof (Nokia - PL/Wrocław) 2020-05-15 7:53 ` Wolfram Sang 2020-05-15 8:51 ` Adamski, Krzysztof (Nokia - PL/Wrocław) 2020-05-15 9:20 ` Wolfram Sang 2020-05-15 9:24 ` Peter Rosin 2020-05-15 9:31 ` Wolfram Sang 2020-05-15 9:46 ` Adamski, Krzysztof (Nokia - PL/Wrocław) 2020-05-15 8:02 ` Peter Rosin 2020-05-15 8:36 ` Adamski, Krzysztof (Nokia - PL/Wrocław) 2020-05-15 21:19 ` Peter Rosin 2020-05-17 21:32 ` Peter Rosin 2020-05-19 12:59 ` Adamski, Krzysztof (Nokia - PL/Wrocław)
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).