| Message ID | 105a3ac1653e9ae658056a5ec9ddc2a084a61669.1567437955.git.vitor.soares@synopsys.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | i3c: remove device if failed on pre_assign_dyn_addr() | expand |
On Tue, 3 Sep 2019 12:35:50 +0200 Vitor Soares <Vitor.Soares@synopsys.com> wrote: > On pre_assing_dyn_addr() the devices that fail: > i3c_master_setdasa_locked() > i3c_master_reattach_i3c_dev() > i3c_master_retrieve_dev_info() > > are kept in memory and master->bus.devs list. This makes the i3c devices > without a dynamic address are sent on DEFSLVS CCC command. Fix this by > detaching and freeing the devices that fail on pre_assign_dyn_addr(). > > Signed-off-by: Vitor Soares <vitor.soares@synopsys.com> > --- > Changes in v2: > - Move out detach/free the i3c_dev_desc from pre_assign_dyn_addr() So, you decided to ignore my comment about leaving the i3c_dev_desc allocated and skipping entries that don't have a dynamic address when forging the DEFSLVS frame instead of doing this allocate/free/re-allocate dance, and more importantly, you didn't even bother explaining why. I'm not willing to accept this patch unless you come up with solid reasons.
Hi Boris, From: Boris Brezillon <boris.brezillon@collabora.com> Date: Tue, Sep 03, 2019 at 11:52:37 > On Tue, 3 Sep 2019 12:35:50 +0200 > Vitor Soares <Vitor.Soares@synopsys.com> wrote: > > > On pre_assing_dyn_addr() the devices that fail: > > i3c_master_setdasa_locked() > > i3c_master_reattach_i3c_dev() > > i3c_master_retrieve_dev_info() > > > > are kept in memory and master->bus.devs list. This makes the i3c devices > > without a dynamic address are sent on DEFSLVS CCC command. Fix this by > > detaching and freeing the devices that fail on pre_assign_dyn_addr(). > > > > Signed-off-by: Vitor Soares <vitor.soares@synopsys.com> > > --- > > Changes in v2: > > - Move out detach/free the i3c_dev_desc from pre_assign_dyn_addr() > > So, you decided to ignore my comment about leaving the i3c_dev_desc > allocated and skipping entries that don't have a dynamic address when > forging the DEFSLVS frame instead of doing this > allocate/free/re-allocate dance, and more importantly, you didn't even > bother explaining why. > > I'm not willing to accept this patch unless you come up with solid > reasons. I think I already give a strong reason for my decision. Let say that my controller only has space for 4 devices and one of them is offline during pre_assign_dyn_addr() and hence it fails. When device tries to do the HJ I won't be able to do the enumeration because there is no space left. Anyway, you are right and I need to add this to commit message. BTW, It is not clear to me why should we keep non addressed devices allocated when we have i3c_dev_boardinfo list with ->of_node information and what is the problem with allocate/free/re-allocate dance? Best regards, Vitor Soares
Hi Vitor, I'm sorry for the delay. The 09/03/2019 12:35, Vitor Soares wrote: > EXTERNAL MAIL > > > On pre_assing_dyn_addr() the devices that fail: > i3c_master_setdasa_locked() > i3c_master_reattach_i3c_dev() > i3c_master_retrieve_dev_info() > > are kept in memory and master->bus.devs list. This makes the i3c devices > without a dynamic address are sent on DEFSLVS CCC command. Fix this by > detaching and freeing the devices that fail on pre_assign_dyn_addr(). What is the problem with sending devices without dynamic address in DEFSLVS? Shouldn't we still inform rest of the devices about that? About fact that device with SA without DA exists on the bus? I think that this is limitation for us. Espacially we have SA and DA fields in DEFSLVS frame so we are able to distinguish devices with and without Dynamic Address. > > Signed-off-by: Vitor Soares <vitor.soares@synopsys.com> > --- > Changes in v2: > - Move out detach/free the i3c_dev_desc from pre_assign_dyn_addr() > - Convert i3c_master_pre_assign_dyn_addr() to return an int > > drivers/i3c/master.c | 22 +++++++++++++++------- > 1 file changed, 15 insertions(+), 7 deletions(-) > > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c > index 5f4bd52..586e34f 100644 > --- a/drivers/i3c/master.c > +++ b/drivers/i3c/master.c > @@ -1426,19 +1426,19 @@ static void i3c_master_detach_i2c_dev(struct i2c_dev_desc *dev) > master->ops->detach_i2c_dev(dev); > } > > -static void i3c_master_pre_assign_dyn_addr(struct i3c_dev_desc *dev) > +static int i3c_master_pre_assign_dyn_addr(struct i3c_dev_desc *dev) > { > struct i3c_master_controller *master = i3c_dev_get_master(dev); > int ret; > > if (!dev->boardinfo || !dev->boardinfo->init_dyn_addr || > !dev->boardinfo->static_addr) > - return; > + return 0; > > ret = i3c_master_setdasa_locked(master, dev->info.static_addr, > dev->boardinfo->init_dyn_addr); > if (ret) > - return; > + return ret; > > dev->info.dyn_addr = dev->boardinfo->init_dyn_addr; > ret = i3c_master_reattach_i3c_dev(dev, 0); > @@ -1449,10 +1449,12 @@ static void i3c_master_pre_assign_dyn_addr(struct i3c_dev_desc *dev) > if (ret) > goto err_rstdaa; > > - return; > + return 0; > > err_rstdaa: > i3c_master_rstdaa_locked(master, dev->boardinfo->init_dyn_addr); > + > + return ret; > } > > static void > @@ -1647,7 +1649,7 @@ static int i3c_master_bus_init(struct i3c_master_controller *master) > enum i3c_addr_slot_status status; > struct i2c_dev_boardinfo *i2cboardinfo; > struct i3c_dev_boardinfo *i3cboardinfo; > - struct i3c_dev_desc *i3cdev; > + struct i3c_dev_desc *i3cdev, *i3ctmp; > struct i2c_dev_desc *i2cdev; > int ret; > > @@ -1746,8 +1748,14 @@ static int i3c_master_bus_init(struct i3c_master_controller *master) > * Pre-assign dynamic address and retrieve device information if > * needed. > */ > - i3c_bus_for_each_i3cdev(&master->bus, i3cdev) > - i3c_master_pre_assign_dyn_addr(i3cdev); > + list_for_each_entry_safe(i3cdev, i3ctmp, &master->bus.devs.i3c, > + common.node) { > + ret = i3c_master_pre_assign_dyn_addr(i3cdev); > + if (ret) { > + i3c_master_detach_i3c_dev(i3cdev); > + i3c_master_free_i3c_dev(i3cdev); > + } > + } > > ret = i3c_master_do_daa(master); > if (ret) > -- > 2.7.4 >
From: Przemyslaw Gaj <pgaj@cadence.com> Date: Tue, Sep 03, 2019 at 12:13:57 > Hi Vitor, > > I'm sorry for the delay. > > The 09/03/2019 12:35, Vitor Soares wrote: > > EXTERNAL MAIL > > > > > > On pre_assing_dyn_addr() the devices that fail: > > i3c_master_setdasa_locked() > > i3c_master_reattach_i3c_dev() > > i3c_master_retrieve_dev_info() > > > > are kept in memory and master->bus.devs list. This makes the i3c devices > > without a dynamic address are sent on DEFSLVS CCC command. Fix this by > > detaching and freeing the devices that fail on pre_assign_dyn_addr(). > > What is the problem with sending devices without dynamic address in DEFSLVS? How do you address them? For the DA field in DEFSLVS frame, the spec says: "shall contain the current value of the Slave's assigned 7-bit Dynamic address". If the device doesn't have the dynamic address assigned yet why send it? > Shouldn't we still inform rest of the devices about that? About fact that > device with SA without DA exists on the bus? I would like to understand what is the use case for this? On I3C spec table "I3C Devices Roles vs Responsibilities", Secondary Master is not responsible to do Dynamic Address Assignment but it is optional to do Hot-Join Dynamic Address Assignment. > > I think that this is limitation for us. Espacially we have SA and DA fields in > DEFSLVS frame so we are able to distinguish devices with and without Dynamic > Address. I would say the reason behind is regarding how to distinguish i2c from i3c devices. > > > > > Signed-off-by: Vitor Soares <vitor.soares@synopsys.com> > > --- > > Changes in v2: > > - Move out detach/free the i3c_dev_desc from pre_assign_dyn_addr() > > - Convert i3c_master_pre_assign_dyn_addr() to return an int > > > > drivers/i3c/master.c | 22 +++++++++++++++------- > > 1 file changed, 15 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c > > index 5f4bd52..586e34f 100644 > > --- a/drivers/i3c/master.c > > +++ b/drivers/i3c/master.c > > @@ -1426,19 +1426,19 @@ static void i3c_master_detach_i2c_dev(struct i2c_dev_desc *dev) > > master->ops->detach_i2c_dev(dev); > > } > > > > -static void i3c_master_pre_assign_dyn_addr(struct i3c_dev_desc *dev) > > +static int i3c_master_pre_assign_dyn_addr(struct i3c_dev_desc *dev) > > { > > struct i3c_master_controller *master = i3c_dev_get_master(dev); > > int ret; > > > > if (!dev->boardinfo || !dev->boardinfo->init_dyn_addr || > > !dev->boardinfo->static_addr) > > - return; > > + return 0; > > > > ret = i3c_master_setdasa_locked(master, dev->info.static_addr, > > dev->boardinfo->init_dyn_addr); > > if (ret) > > - return; > > + return ret; > > > > dev->info.dyn_addr = dev->boardinfo->init_dyn_addr; > > ret = i3c_master_reattach_i3c_dev(dev, 0); > > @@ -1449,10 +1449,12 @@ static void i3c_master_pre_assign_dyn_addr(struct i3c_dev_desc *dev) > > if (ret) > > goto err_rstdaa; > > > > - return; > > + return 0; > > > > err_rstdaa: > > i3c_master_rstdaa_locked(master, dev->boardinfo->init_dyn_addr); > > + > > + return ret; > > } > > > > static void > > @@ -1647,7 +1649,7 @@ static int i3c_master_bus_init(struct i3c_master_controller *master) > > enum i3c_addr_slot_status status; > > struct i2c_dev_boardinfo *i2cboardinfo; > > struct i3c_dev_boardinfo *i3cboardinfo; > > - struct i3c_dev_desc *i3cdev; > > + struct i3c_dev_desc *i3cdev, *i3ctmp; > > struct i2c_dev_desc *i2cdev; > > int ret; > > > > @@ -1746,8 +1748,14 @@ static int i3c_master_bus_init(struct i3c_master_controller *master) > > * Pre-assign dynamic address and retrieve device information if > > * needed. > > */ > > - i3c_bus_for_each_i3cdev(&master->bus, i3cdev) > > - i3c_master_pre_assign_dyn_addr(i3cdev); > > + list_for_each_entry_safe(i3cdev, i3ctmp, &master->bus.devs.i3c, > > + common.node) { > > + ret = i3c_master_pre_assign_dyn_addr(i3cdev); > > + if (ret) { > > + i3c_master_detach_i3c_dev(i3cdev); > > + i3c_master_free_i3c_dev(i3cdev); > > + } > > + } > > > > ret = i3c_master_do_daa(master); > > if (ret) > > -- > > 2.7.4 > > > > -- > -- > Przemyslaw Gaj Best regards, Vitor Soares
The 09/03/2019 11:57, Vitor Soares wrote: > EXTERNAL MAIL > > > From: Przemyslaw Gaj <pgaj@cadence.com> > Date: Tue, Sep 03, 2019 at 12:13:57 > > > Hi Vitor, > > > > I'm sorry for the delay. > > > > The 09/03/2019 12:35, Vitor Soares wrote: > > > EXTERNAL MAIL > > > > > > > > > On pre_assing_dyn_addr() the devices that fail: > > > i3c_master_setdasa_locked() > > > i3c_master_reattach_i3c_dev() > > > i3c_master_retrieve_dev_info() > > > > > > are kept in memory and master->bus.devs list. This makes the i3c devices > > > without a dynamic address are sent on DEFSLVS CCC command. Fix this by > > > detaching and freeing the devices that fail on pre_assign_dyn_addr(). > > > > What is the problem with sending devices without dynamic address in DEFSLVS? > > How do you address them? > For the DA field in DEFSLVS frame, the spec says: "shall contain the > current value of the Slave's assigned 7-bit Dynamic address". If the > device doesn't have the dynamic address assigned yet why send it? > What stops us from operating with this device in I2C mode using his SA? > > Shouldn't we still inform rest of the devices about that? About fact that > > device with SA without DA exists on the bus? > > I would like to understand what is the use case for this? Look above. I2C mode still should be possible IMO. Just consider the case when you really need some information from that device, main master can assign dynamic address later but you can make some transfers. I know our framework does not support that case but secondary master can be different system which should know that this device exists and don't have DA yet, so I2C mode is available. > > On I3C spec table "I3C Devices Roles vs Responsibilities", Secondary > Master is not responsible to do Dynamic Address Assignment but it is > optional to do Hot-Join Dynamic Address Assignment. > Yes, we discussed that few times during the review of Mastership patch series. > > > > I think that this is limitation for us. Espacially we have SA and DA fields in > > DEFSLVS frame so we are able to distinguish devices with and without Dynamic > > Address. > > I would say the reason behind is regarding how to distinguish i2c from > i3c devices. > > > > > > > > > Signed-off-by: Vitor Soares <vitor.soares@synopsys.com> > > > --- > > > Changes in v2: > > > - Move out detach/free the i3c_dev_desc from pre_assign_dyn_addr() > > > - Convert i3c_master_pre_assign_dyn_addr() to return an int > > > > > > drivers/i3c/master.c | 22 +++++++++++++++------- > > > 1 file changed, 15 insertions(+), 7 deletions(-) > > > > > > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c > > > index 5f4bd52..586e34f 100644 > > > --- a/drivers/i3c/master.c > > > +++ b/drivers/i3c/master.c > > > @@ -1426,19 +1426,19 @@ static void i3c_master_detach_i2c_dev(struct i2c_dev_desc *dev) > > > master->ops->detach_i2c_dev(dev); > > > } > > > > > > -static void i3c_master_pre_assign_dyn_addr(struct i3c_dev_desc *dev) > > > +static int i3c_master_pre_assign_dyn_addr(struct i3c_dev_desc *dev) > > > { > > > struct i3c_master_controller *master = i3c_dev_get_master(dev); > > > int ret; > > > > > > if (!dev->boardinfo || !dev->boardinfo->init_dyn_addr || > > > !dev->boardinfo->static_addr) > > > - return; > > > + return 0; > > > > > > ret = i3c_master_setdasa_locked(master, dev->info.static_addr, > > > dev->boardinfo->init_dyn_addr); > > > if (ret) > > > - return; > > > + return ret; > > > > > > dev->info.dyn_addr = dev->boardinfo->init_dyn_addr; > > > ret = i3c_master_reattach_i3c_dev(dev, 0); > > > @@ -1449,10 +1449,12 @@ static void i3c_master_pre_assign_dyn_addr(struct i3c_dev_desc *dev) > > > if (ret) > > > goto err_rstdaa; > > > > > > - return; > > > + return 0; > > > > > > err_rstdaa: > > > i3c_master_rstdaa_locked(master, dev->boardinfo->init_dyn_addr); > > > + > > > + return ret; > > > } > > > > > > static void > > > @@ -1647,7 +1649,7 @@ static int i3c_master_bus_init(struct i3c_master_controller *master) > > > enum i3c_addr_slot_status status; > > > struct i2c_dev_boardinfo *i2cboardinfo; > > > struct i3c_dev_boardinfo *i3cboardinfo; > > > - struct i3c_dev_desc *i3cdev; > > > + struct i3c_dev_desc *i3cdev, *i3ctmp; > > > struct i2c_dev_desc *i2cdev; > > > int ret; > > > > > > @@ -1746,8 +1748,14 @@ static int i3c_master_bus_init(struct i3c_master_controller *master) > > > * Pre-assign dynamic address and retrieve device information if > > > * needed. > > > */ > > > - i3c_bus_for_each_i3cdev(&master->bus, i3cdev) > > > - i3c_master_pre_assign_dyn_addr(i3cdev); > > > + list_for_each_entry_safe(i3cdev, i3ctmp, &master->bus.devs.i3c, > > > + common.node) { > > > + ret = i3c_master_pre_assign_dyn_addr(i3cdev); > > > + if (ret) { > > > + i3c_master_detach_i3c_dev(i3cdev); > > > + i3c_master_free_i3c_dev(i3cdev); > > > + } > > > + } > > > > > > ret = i3c_master_do_daa(master); > > > if (ret) > > > -- > > > 2.7.4 > > > > > > > -- > > -- > > Przemyslaw Gaj > > Best regards, > Vitor Soares
From: Przemyslaw Gaj <pgaj@cadence.com> Date: Wed, Sep 04, 2019 at 05:47:18 > The 09/03/2019 11:57, Vitor Soares wrote: > > EXTERNAL MAIL > > > > > > From: Przemyslaw Gaj <pgaj@cadence.com> > > Date: Tue, Sep 03, 2019 at 12:13:57 > > > > > Hi Vitor, > > > > > > I'm sorry for the delay. > > > > > > The 09/03/2019 12:35, Vitor Soares wrote: > > > > EXTERNAL MAIL > > > > > > > > > > > > On pre_assing_dyn_addr() the devices that fail: > > > > i3c_master_setdasa_locked() > > > > i3c_master_reattach_i3c_dev() > > > > i3c_master_retrieve_dev_info() > > > > > > > > are kept in memory and master->bus.devs list. This makes the i3c devices > > > > without a dynamic address are sent on DEFSLVS CCC command. Fix this by > > > > detaching and freeing the devices that fail on pre_assign_dyn_addr(). > > > > > > What is the problem with sending devices without dynamic address in DEFSLVS? > > > > How do you address them? > > For the DA field in DEFSLVS frame, the spec says: "shall contain the > > current value of the Slave's assigned 7-bit Dynamic address". If the > > device doesn't have the dynamic address assigned yet why send it? > > > > What stops us from operating with this device in I2C mode using his SA? So, send it as an I2C device as spec says. > > > > Shouldn't we still inform rest of the devices about that? About fact that > > > device with SA without DA exists on the bus? > > > > I would like to understand what is the use case for this? > > Look above. I2C mode still should be possible IMO. Just consider the case when > you really need some information from that device, main master can assign > dynamic address later but you can make some transfers. It is true, but again it is a I2C device and not I3C device. It won't reply to I3C commands until has a DA. How will you know that the DA sent in DEFSLVS is not assigned yet? > I know our framework > does not support that case but secondary master can be different system which > should know that this device exists and don't have DA yet, so I2C mode is > available. If the HJ happen in secondary master side, there is a change to describe in DT what should be DA. Please check 2nd patch. > > > > > On I3C spec table "I3C Devices Roles vs Responsibilities", Secondary > > Master is not responsible to do Dynamic Address Assignment but it is > > optional to do Hot-Join Dynamic Address Assignment. > > > > Yes, we discussed that few times during the review of Mastership patch series. Hence, based on that table, secondary master won't do DAA just because he want, It needs a HJ to trigger DAA. Sorry, but for me based on what spec says this use case is not feasible. Best regards, Vitor Soares
diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c index 5f4bd52..586e34f 100644 --- a/drivers/i3c/master.c +++ b/drivers/i3c/master.c @@ -1426,19 +1426,19 @@ static void i3c_master_detach_i2c_dev(struct i2c_dev_desc *dev) master->ops->detach_i2c_dev(dev); } -static void i3c_master_pre_assign_dyn_addr(struct i3c_dev_desc *dev) +static int i3c_master_pre_assign_dyn_addr(struct i3c_dev_desc *dev) { struct i3c_master_controller *master = i3c_dev_get_master(dev); int ret; if (!dev->boardinfo || !dev->boardinfo->init_dyn_addr || !dev->boardinfo->static_addr) - return; + return 0; ret = i3c_master_setdasa_locked(master, dev->info.static_addr, dev->boardinfo->init_dyn_addr); if (ret) - return; + return ret; dev->info.dyn_addr = dev->boardinfo->init_dyn_addr; ret = i3c_master_reattach_i3c_dev(dev, 0); @@ -1449,10 +1449,12 @@ static void i3c_master_pre_assign_dyn_addr(struct i3c_dev_desc *dev) if (ret) goto err_rstdaa; - return; + return 0; err_rstdaa: i3c_master_rstdaa_locked(master, dev->boardinfo->init_dyn_addr); + + return ret; } static void @@ -1647,7 +1649,7 @@ static int i3c_master_bus_init(struct i3c_master_controller *master) enum i3c_addr_slot_status status; struct i2c_dev_boardinfo *i2cboardinfo; struct i3c_dev_boardinfo *i3cboardinfo; - struct i3c_dev_desc *i3cdev; + struct i3c_dev_desc *i3cdev, *i3ctmp; struct i2c_dev_desc *i2cdev; int ret; @@ -1746,8 +1748,14 @@ static int i3c_master_bus_init(struct i3c_master_controller *master) * Pre-assign dynamic address and retrieve device information if * needed. */ - i3c_bus_for_each_i3cdev(&master->bus, i3cdev) - i3c_master_pre_assign_dyn_addr(i3cdev); + list_for_each_entry_safe(i3cdev, i3ctmp, &master->bus.devs.i3c, + common.node) { + ret = i3c_master_pre_assign_dyn_addr(i3cdev); + if (ret) { + i3c_master_detach_i3c_dev(i3cdev); + i3c_master_free_i3c_dev(i3cdev); + } + } ret = i3c_master_do_daa(master); if (ret)
On pre_assing_dyn_addr() the devices that fail: i3c_master_setdasa_locked() i3c_master_reattach_i3c_dev() i3c_master_retrieve_dev_info() are kept in memory and master->bus.devs list. This makes the i3c devices without a dynamic address are sent on DEFSLVS CCC command. Fix this by detaching and freeing the devices that fail on pre_assign_dyn_addr(). Signed-off-by: Vitor Soares <vitor.soares@synopsys.com> --- Changes in v2: - Move out detach/free the i3c_dev_desc from pre_assign_dyn_addr() - Convert i3c_master_pre_assign_dyn_addr() to return an int drivers/i3c/master.c | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-)