* i3c_master_add_i3c_dev_locked() accept PID, BCR, DCR
@ 2018-12-10 19:21 vitor
2018-12-10 19:43 ` Boris Brezillon
0 siblings, 1 reply; 15+ messages in thread
From: vitor @ 2018-12-10 19:21 UTC (permalink / raw)
To: linux-i3c
Hi,
The function bellow doesn't accept the PID, BCR and DCR captured
during the ENTDAA.
int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master,
?? ??? ??? ??? ?? u8 addr)
{
?? ?struct i3c_device_info info = { .dyn_addr = addr };
I would like to change it to received those parameters. Something like this:
int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master,
?? ??? ??? ??? ?? u64 pid, u8 bcr, u8 dcr, u8 addr)
{
?? ?struct i3c_device_info info = { .pid = pid,
?? ??? ??? ??? ??? ?.bcr = bcr,
?? ??? ??? ??? ??? ?.dcr = dcr,
?? ??? ??? ??? ??? ?.dyn_addr = addr };
Do you agree?
Best regards,
Vitor Soares
^ permalink raw reply [flat|nested] 15+ messages in thread* i3c_master_add_i3c_dev_locked() accept PID, BCR, DCR 2018-12-10 19:21 i3c_master_add_i3c_dev_locked() accept PID, BCR, DCR vitor @ 2018-12-10 19:43 ` Boris Brezillon 2018-12-10 20:14 ` Boris Brezillon 0 siblings, 1 reply; 15+ messages in thread From: Boris Brezillon @ 2018-12-10 19:43 UTC (permalink / raw) To: linux-i3c Hi Vitor, Can you please keep Cc-ing me when you send I3C patches. Sending it only to the ML is not enough, maintainers should be Cc-ed too. On Mon, 10 Dec 2018 19:21:37 +0000 vitor <vitor.soares@synopsys.com> wrote: > Hi, > > The function bellow doesn't accept the PID, BCR and DCR captured > during the ENTDAA. > > int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master, > ?? ??? ??? ??? ?? u8 addr) > { > ?? ?struct i3c_device_info info = { .dyn_addr = addr }; > > > I would like to change it to received those parameters. Something like this: > > int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master, > ?? ??? ??? ??? ?? u64 pid, u8 bcr, u8 dcr, u8 addr) > { > ?? ?struct i3c_device_info info = { .pid = pid, > ?? ??? ??? ??? ??? ?.bcr = bcr, > ?? ??? ??? ??? ??? ?.dcr = dcr, > ?? ??? ??? ??? ??? ?.dyn_addr = addr }; > > Do you agree? I need a reason, like for every other requests you made. Why do you need it? Do you want to check the value returned by the GETBCR, GETDCR and GETPID operations? Do you want to skip those operations in the DAA case, and if you do, why? How will that work for the non-DAA case (SETDASA)? I think I already mentioned it, but please remember that any changes you suggest should be explained so that we can have a constructive discussion. Regards, Boris ^ permalink raw reply [flat|nested] 15+ messages in thread
* i3c_master_add_i3c_dev_locked() accept PID, BCR, DCR 2018-12-10 19:43 ` Boris Brezillon @ 2018-12-10 20:14 ` Boris Brezillon 2018-12-11 11:54 ` vitor 0 siblings, 1 reply; 15+ messages in thread From: Boris Brezillon @ 2018-12-10 20:14 UTC (permalink / raw) To: linux-i3c On Mon, 10 Dec 2018 20:43:30 +0100 Boris Brezillon <b.brezillon.dev@gmail.com> wrote: > Hi Vitor, > > Can you please keep Cc-ing me when you send I3C patches. Sending it > only to the ML is not enough, maintainers should be Cc-ed too. > > On Mon, 10 Dec 2018 19:21:37 +0000 > vitor <vitor.soares@synopsys.com> wrote: > > > Hi, > > > > The function bellow doesn't accept the PID, BCR and DCR captured > > during the ENTDAA. > > > > int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master, > > ?? ??? ??? ??? ?? u8 addr) > > { > > ?? ?struct i3c_device_info info = { .dyn_addr = addr }; > > > > > > I would like to change it to received those parameters. Something like this: > > > > int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master, > > ?? ??? ??? ??? ?? u64 pid, u8 bcr, u8 dcr, u8 addr) > > { > > ?? ?struct i3c_device_info info = { .pid = pid, > > ?? ??? ??? ??? ??? ?.bcr = bcr, > > ?? ??? ??? ??? ??? ?.dcr = dcr, > > ?? ??? ??? ??? ??? ?.dyn_addr = addr }; > > > > Do you agree? > > I need a reason, like for every other requests you made. > > Why do you need it? Do you want to check the value returned by the > GETBCR, GETDCR and GETPID operations? Do you want to skip those > operations in the DAA case, and if you do, why? How will that work for > the non-DAA case (SETDASA)? For the record, I initially decided to not pass pid, bcr and dcr to i3c_master_add_i3c_dev_locked() to have a single function working for both DAA and SETDASA cases. Given how fast it is to send 3 CCC commands per device (and the limited number of devices per bus) I didn't think it was useful to optimize it. If you think otherwise, I'd like to hear your arguments. ^ permalink raw reply [flat|nested] 15+ messages in thread
* i3c_master_add_i3c_dev_locked() accept PID, BCR, DCR 2018-12-10 20:14 ` Boris Brezillon @ 2018-12-11 11:54 ` vitor 2018-12-11 11:58 ` Boris Brezillon 0 siblings, 1 reply; 15+ messages in thread From: vitor @ 2018-12-11 11:54 UTC (permalink / raw) To: linux-i3c Hi Boris, On 10/12/18 20:14, Boris Brezillon wrote: > On Mon, 10 Dec 2018 20:43:30 +0100 > Boris Brezillon <b.brezillon.dev@gmail.com> wrote: > >> Hi Vitor, >> >> Can you please keep Cc-ing me when you send I3C patches. Sending it >> only to the ML is not enough, maintainers should be Cc-ed too. Sorry, I will do that in the future. >> >> On Mon, 10 Dec 2018 19:21:37 +0000 >> vitor <vitor.soares@synopsys.com> wrote: >> >>> Hi, >>> >>> The function bellow doesn't accept the PID, BCR and DCR captured >>> during the ENTDAA. >>> >>> int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master, >>> ?? ??? ??? ??? ?? u8 addr) >>> { >>> ?? ?struct i3c_device_info info = { .dyn_addr = addr }; >>> >>> >>> I would like to change it to received those parameters. Something like this: >>> >>> int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master, >>> ?? ??? ??? ??? ?? u64 pid, u8 bcr, u8 dcr, u8 addr) >>> { >>> ?? ?struct i3c_device_info info = { .pid = pid, >>> ?? ??? ??? ??? ??? ?.bcr = bcr, >>> ?? ??? ??? ??? ??? ?.dcr = dcr, >>> ?? ??? ??? ??? ??? ?.dyn_addr = addr }; >>> >>> Do you agree? >> I need a reason, like for every other requests you made. >> >> Why do you need it? Do you want to check the value returned by the >> GETBCR, GETDCR and GETPID operations? Do you want to skip those >> operations in the DAA case, and if you do, why? How will that work for >> the non-DAA case (SETDASA)? > For the record, I initially decided to not pass pid, bcr and dcr to > i3c_master_add_i3c_dev_locked() to have a single function working for > both DAA and SETDASA cases. Given how fast it is to send 3 CCC commands > per device (and the limited number of devices per bus) I didn't think it > was useful to optimize it. If you think otherwise, I'd like to hear your > arguments. I just want to skip GETBCR/DCR/PID operations since those data are already available in the controller. There's advantages but they only have relevance depending of use case. Best regards, Vitor Soares ^ permalink raw reply [flat|nested] 15+ messages in thread
* i3c_master_add_i3c_dev_locked() accept PID, BCR, DCR 2018-12-11 11:54 ` vitor @ 2018-12-11 11:58 ` Boris Brezillon 2018-12-11 12:07 ` vitor 0 siblings, 1 reply; 15+ messages in thread From: Boris Brezillon @ 2018-12-11 11:58 UTC (permalink / raw) To: linux-i3c On Tue, 11 Dec 2018 11:54:22 +0000 vitor <vitor.soares@synopsys.com> wrote: > >>> The function bellow doesn't accept the PID, BCR and DCR captured > >>> during the ENTDAA. > >>> > >>> int i3c_master_add_i3c_dev_locked(struct i3c_master_controller > >>> *master, u8 addr) > >>> { > >>> ?? ?struct i3c_device_info info = { .dyn_addr = addr }; > >>> > >>> > >>> I would like to change it to received those parameters. Something > >>> like this: > >>> > >>> int i3c_master_add_i3c_dev_locked(struct i3c_master_controller > >>> *master, u64 pid, u8 bcr, u8 dcr, u8 addr) > >>> { > >>> ?? ?struct i3c_device_info info = { .pid = pid, > >>> ?? ??? ??? ??? ??? ?.bcr = bcr, > >>> ?? ??? ??? ??? ??? ?.dcr = dcr, > >>> ?? ??? ??? ??? ??? ?.dyn_addr = addr }; > >>> > >>> Do you agree? > >> I need a reason, like for every other requests you made. > >> > >> Why do you need it? Do you want to check the value returned by the > >> GETBCR, GETDCR and GETPID operations? Do you want to skip those > >> operations in the DAA case, and if you do, why? How will that work > >> for the non-DAA case (SETDASA)? > > For the record, I initially decided to not pass pid, bcr and dcr to > > i3c_master_add_i3c_dev_locked() to have a single function working > > for both DAA and SETDASA cases. Given how fast it is to send 3 CCC > > commands per device (and the limited number of devices per bus) I > > didn't think it was useful to optimize it. If you think otherwise, > > I'd like to hear your arguments. > > I just want to skip GETBCR/DCR/PID operations since those data are > already available in the controller. There's advantages but they only > have relevance depending of use case. Can you detail those use cases please? ^ permalink raw reply [flat|nested] 15+ messages in thread
* i3c_master_add_i3c_dev_locked() accept PID, BCR, DCR 2018-12-11 11:58 ` Boris Brezillon @ 2018-12-11 12:07 ` vitor 2018-12-11 12:21 ` Boris Brezillon 0 siblings, 1 reply; 15+ messages in thread From: vitor @ 2018-12-11 12:07 UTC (permalink / raw) To: linux-i3c On 11/12/18 11:58, Boris Brezillon wrote: >> I just want to skip GETBCR/DCR/PID operations since those data are >> already available in the controller. There's advantages but they only >> have relevance depending of use case. > Can you detail those use cases please? If the bus has a lot of devices and you want to decrease the initialization time this could be the first step. ^ permalink raw reply [flat|nested] 15+ messages in thread
* i3c_master_add_i3c_dev_locked() accept PID, BCR, DCR 2018-12-11 12:07 ` vitor @ 2018-12-11 12:21 ` Boris Brezillon 2018-12-11 12:25 ` Boris Brezillon 0 siblings, 1 reply; 15+ messages in thread From: Boris Brezillon @ 2018-12-11 12:21 UTC (permalink / raw) To: linux-i3c On Tue, 11 Dec 2018 12:07:16 +0000 vitor <vitor.soares@synopsys.com> wrote: > On 11/12/18 11:58, Boris Brezillon wrote: > >> I just want to skip GETBCR/DCR/PID operations since those data are > >> already available in the controller. There's advantages but they > >> only have relevance depending of use case. > > Can you detail those use cases please? > > If the bus has a lot of devices and you want to decrease the > initialization time this could be the first step. > Did you quantify the boot-time/perf improvement? The clk should be running at 12.5MHz, GETPID takes 9bytes and GETBCR/DCR take 4 bytes each. Let's add one byte per command to account for the start/stop bits. That makes a total of 20bytes per device, with a maximum of 11 devices per bus: max_time_to_query_pid_bcr_dcr_per_bus = 20 * 10 / 12500000 = 17.6 usec. Let's assume we have a huge overhead caused by the OS (and all the scheduling involved), and make it 100usec. So we're talking about saving a maximum of ~100 usec per bus. Sorry, but I think you'll have plenty of other things to optimize before this becomes a boot-time issue. ^ permalink raw reply [flat|nested] 15+ messages in thread
* i3c_master_add_i3c_dev_locked() accept PID, BCR, DCR 2018-12-11 12:21 ` Boris Brezillon @ 2018-12-11 12:25 ` Boris Brezillon 2018-12-11 15:47 ` vitor 0 siblings, 1 reply; 15+ messages in thread From: Boris Brezillon @ 2018-12-11 12:25 UTC (permalink / raw) To: linux-i3c On Tue, 11 Dec 2018 13:21:03 +0100 Boris Brezillon <bbrezillon@kernel.org> wrote: > On Tue, 11 Dec 2018 12:07:16 +0000 > vitor <vitor.soares@synopsys.com> wrote: > > > On 11/12/18 11:58, Boris Brezillon wrote: > > >> I just want to skip GETBCR/DCR/PID operations since those data are > > >> already available in the controller. There's advantages but they > > >> only have relevance depending of use case. > > > Can you detail those use cases please? > > > > If the bus has a lot of devices and you want to decrease the > > initialization time this could be the first step. > > > > Did you quantify the boot-time/perf improvement? The clk should be > running at 12.5MHz, GETPID takes 9bytes and GETBCR/DCR take 4 bytes > each. Let's add one byte per command to account for the start/stop > bits. That makes a total of 20bytes per device, with a maximum of 11 > devices per bus: > > max_time_to_query_pid_bcr_dcr_per_bus = 20 * 10 / 12500000 = 17.6 usec. Sorry, small mistake, I counted bytes instead of bits. max_time_to_query_pid_bcr_dcr_per_bus = 20 * 8 * 11 / 12500000 = 141usec. > Let's assume we have a huge overhead caused by the OS (and all the > scheduling involved), and make it 100usec. And let's make it 500usec with the OS overhead, which is still not particularly worrisome in term of boot-time. ^ permalink raw reply [flat|nested] 15+ messages in thread
* i3c_master_add_i3c_dev_locked() accept PID, BCR, DCR 2018-12-11 12:25 ` Boris Brezillon @ 2018-12-11 15:47 ` vitor 2018-12-12 6:06 ` Przemyslaw Gaj 0 siblings, 1 reply; 15+ messages in thread From: vitor @ 2018-12-11 15:47 UTC (permalink / raw) To: linux-i3c On 11/12/18 12:25, Boris Brezillon wrote: > On Tue, 11 Dec 2018 13:21:03 +0100 > Boris Brezillon <bbrezillon@kernel.org> wrote: > >> On Tue, 11 Dec 2018 12:07:16 +0000 >> vitor <vitor.soares@synopsys.com> wrote: >> >>> On 11/12/18 11:58, Boris Brezillon wrote: >>>>> I just want to skip GETBCR/DCR/PID operations since those data are >>>>> already available in the controller. There's advantages but they >>>>> only have relevance depending of use case. >>>> Can you detail those use cases please? >>> If the bus has a lot of devices and you want to decrease the >>> initialization time this could be the first step. >>> >> Did you quantify the boot-time/perf improvement? The clk should be >> running at 12.5MHz, GETPID takes 9bytes and GETBCR/DCR take 4 bytes >> each. Let's add one byte per command to account for the start/stop >> bits. That makes a total of 20bytes per device, with a maximum of 11 >> devices per bus: >> >> max_time_to_query_pid_bcr_dcr_per_bus = 20 * 10 / 12500000 = 17.6 usec. > Sorry, small mistake, I counted bytes instead of bits. > > max_time_to_query_pid_bcr_dcr_per_bus = 20 * 8 * 11 / 12500000 = 141usec. In fact there is missing other things like, S time, Sr time, stop time, stall time in t-bit and OD speed for the addresses... I measured in my setup with a scope and the additional time to get that information is around 110 us per device and with only one device the bus takes around 550 us for the initialization. >> Let's assume we have a huge overhead caused by the OS (and all the >> scheduling involved), and make it 100usec. > And let's make it 500usec with the OS overhead, which is still not > particularly worrisome in term of boot-time. Ok, it can be done in the future. Thanks. ^ permalink raw reply [flat|nested] 15+ messages in thread
* i3c_master_add_i3c_dev_locked() accept PID, BCR, DCR 2018-12-11 15:47 ` vitor @ 2018-12-12 6:06 ` Przemyslaw Gaj 2018-12-12 8:21 ` Boris Brezillon 0 siblings, 1 reply; 15+ messages in thread From: Przemyslaw Gaj @ 2018-12-12 6:06 UTC (permalink / raw) To: linux-i3c Hi Boris, ?On 12/11/18, 4:48 PM, "linux-i3c on behalf of vitor" <linux-i3c-bounces at lists.infradead.org on behalf of vitor.soares@synopsys.com> wrote: EXTERNAL MAIL On 11/12/18 12:25, Boris Brezillon wrote: > On Tue, 11 Dec 2018 13:21:03 +0100 > Boris Brezillon <bbrezillon@kernel.org> wrote: > >> On Tue, 11 Dec 2018 12:07:16 +0000 >> vitor <vitor.soares@synopsys.com> wrote: >> >>> On 11/12/18 11:58, Boris Brezillon wrote: >>>>> I just want to skip GETBCR/DCR/PID operations since those data are >>>>> already available in the controller. There's advantages but they >>>>> only have relevance depending of use case. >>>> Can you detail those use cases please? >>> If the bus has a lot of devices and you want to decrease the >>> initialization time this could be the first step. >>> >> Did you quantify the boot-time/perf improvement? The clk should be >> running at 12.5MHz, GETPID takes 9bytes and GETBCR/DCR take 4 bytes >> each. Let's add one byte per command to account for the start/stop >> bits. That makes a total of 20bytes per device, with a maximum of 11 >> devices per bus: >> >> max_time_to_query_pid_bcr_dcr_per_bus = 20 * 10 / 12500000 = 17.6 usec. > Sorry, small mistake, I counted bytes instead of bits. > > max_time_to_query_pid_bcr_dcr_per_bus = 20 * 8 * 11 / 12500000 = 141usec. In fact there is missing other things like, S time, Sr time, stop time, stall time in t-bit and OD speed for the addresses... I measured in my setup with a scope and the additional time to get that information is around 110 us per device and with only one device the bus takes around 550 us for the initialization. So, for now, we are transferring the same information two times over the bus, during DAA procedure and then manually. Three commands per device GETBCR/DCR/PID need six register accesses to write (in our controller). Don't you think it is more significant overhead? I'm just wondering. >> Let's assume we have a huge overhead caused by the OS (and all the >> scheduling involved), and make it 100usec. > And let's make it 500usec with the OS overhead, which is still not > particularly worrisome in term of boot-time. Ok, it can be done in the future. I was thinking also how to skip this in case of mastership request where device information is received in DEFSLVS command, secondary master can't do DAA. Mastership takeover can be faster without need to get BCR/DCR from devices again. Thanks. Thanks, Przemek _______________________________________________ linux-i3c mailing list linux-i3c at lists.infradead.org https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.infradead.org_mailman_listinfo_linux-2Di3c&d=DwICAg&c=aUq983L2pue2FqKFoP6PGHMJQyoJ7kl3s3GZ-_haXqY&r=CMnAfM_OfpqcWZRfiqcRWw&m=trNUbFnImlZoS093pWZERer4mHsTchT6X8dOa6-jfgw&s=uCZJLvo-KNMKIPKv9Tia18uSSdtzAFTMgSumhmAYRzk&e= ^ permalink raw reply [flat|nested] 15+ messages in thread
* i3c_master_add_i3c_dev_locked() accept PID, BCR, DCR 2018-12-12 6:06 ` Przemyslaw Gaj @ 2018-12-12 8:21 ` Boris Brezillon 2018-12-12 8:37 ` Przemyslaw Gaj 0 siblings, 1 reply; 15+ messages in thread From: Boris Brezillon @ 2018-12-12 8:21 UTC (permalink / raw) To: linux-i3c Hi Przemek, On Wed, 12 Dec 2018 06:06:18 +0000 Przemyslaw Gaj <pgaj@cadence.com> wrote: > > I measured in my setup with a scope and the additional time to get that information is around 110 us per device and with only one device the bus takes around 550 us for the initialization. > > So, for now, we are transferring the same information two times over the bus, during DAA procedure and then manually. Three commands per device GETBCR/DCR/PID need six register accesses to write (in our controller). Don't you think it is more significant overhead? I'm just wondering. Not really, and I think it's actually more than 3 reg accesses per device (you also have to clear the interrupts and read the RX FIFO). My point is, this should only happen once at init (DAA) time and once in a while if devices are joining the bus afterwards (Hot-Join requests). So, this is not really a hot path, hence my decision to choose simplicity over perfs: have a single function for the DAA (where PID, BCR and DCR are available) and SETDASA (where these have to be retrieved using GETPID, GETDCR, GETBCR) cases. Nothing is set in stone, and if one of you comes with a patch that keep things simple while allowing one to skip the GET{PID,BCR,DCR} commands, I'll consider merging it. But, to be honest, I don't think this a priority. We still don't have support for DDR and mastership handover, which in my opinion are much more valuable than optimizing the bus-discovery path. > > > >> Let's assume we have a huge overhead caused by the OS (and all the > > >> scheduling involved), and make it 100usec. > > > And let's make it 500usec with the OS overhead, which is still not > > > particularly worrisome in term of boot-time. > > > > Ok, it can be done in the future. > > I was thinking also how to skip this in case of mastership request where device information is received in DEFSLVS command, secondary master can't do DAA. Okay, so this is actually one reason we'd want to add devices without triggering transfers on the bus. The thing is, we need more than just PID, BCR and DCR, and those information are not passed in the DEFSLVS frame (MXDS, MRL, MWL, HDRCAP). IIRC, we started discussing that with Arnd, Vitor and you and we listed 2 options: 1/ force the secondary master to take bus ownership after it has received DEFSLVS so that it can retrieve the missing bits and finally expose devices to its users 2/ bind partially discovered devices to drivers and retrieve the device information on-demand (when i3c_device_get_info() is called). If we go for option #2, we'll need a separate function that does more than just skipping GETPID, GETDCR and GETBCR. Of course #2 is more elegant in that it doesn't force a mastership handover until someone really wants to access a device through the secondary master, but #1 is definitely simpler to implement. > Mastership takeover can be faster without need to get BCR/DCR from devices again. Again, this should only happen once, not every time the master changes, simply because devices on the bus will stay the same. So I don't think optimization is a good argument. Regards, Boris ^ permalink raw reply [flat|nested] 15+ messages in thread
* i3c_master_add_i3c_dev_locked() accept PID, BCR, DCR 2018-12-12 8:21 ` Boris Brezillon @ 2018-12-12 8:37 ` Przemyslaw Gaj 2018-12-12 8:52 ` Boris Brezillon 0 siblings, 1 reply; 15+ messages in thread From: Przemyslaw Gaj @ 2018-12-12 8:37 UTC (permalink / raw) To: linux-i3c Hi Boris, ?On 12/12/18, 9:21 AM, "Boris Brezillon" <bbrezillon@kernel.org> wrote: EXTERNAL MAIL Hi Przemek, On Wed, 12 Dec 2018 06:06:18 +0000 Przemyslaw Gaj <pgaj@cadence.com> wrote: > > I measured in my setup with a scope and the additional time to get that information is around 110 us per device and with only one device the bus takes around 550 us for the initialization. > > So, for now, we are transferring the same information two times over the bus, during DAA procedure and then manually. Three commands per device GETBCR/DCR/PID need six register accesses to write (in our controller). Don't you think it is more significant overhead? I'm just wondering. Not really, and I think it's actually more than 3 reg accesses per device (you also have to clear the interrupts and read the RX FIFO). My point is, this should only happen once at init (DAA) time and once in a while if devices are joining the bus afterwards (Hot-Join requests). So, this is not really a hot path, hence my decision to choose simplicity over perfs: have a single function for the DAA (where PID, BCR and DCR are available) and SETDASA (where these have to be retrieved using GETPID, GETDCR, GETBCR) cases. Nothing is set in stone, and if one of you comes with a patch that keep things simple while allowing one to skip the GET{PID,BCR,DCR} commands, I'll consider merging it. But, to be honest, I don't think this a priority. We still don't have support for DDR and mastership handover, which in my opinion are much more valuable than optimizing the bus-discovery path. Yes, I agree we can leave it for later. I'm composing patch containing mastership and HRD mode. I'll release it soon. > > > >> Let's assume we have a huge overhead caused by the OS (and all the > > >> scheduling involved), and make it 100usec. > > > And let's make it 500usec with the OS overhead, which is still not > > > particularly worrisome in term of boot-time. > > > > Ok, it can be done in the future. > > I was thinking also how to skip this in case of mastership request where device information is received in DEFSLVS command, secondary master can't do DAA. Okay, so this is actually one reason we'd want to add devices without triggering transfers on the bus. The thing is, we need more than just PID, BCR and DCR, and those information are not passed in the DEFSLVS frame (MXDS, MRL, MWL, HDRCAP). IIRC, we started discussing that with Arnd, Vitor and you and we listed 2 options: 1/ force the secondary master to take bus ownership after it has received DEFSLVS so that it can retrieve the missing bits and finally expose devices to its users 2/ bind partially discovered devices to drivers and retrieve the device information on-demand (when i3c_device_get_info() is called). If we go for option #2, we'll need a separate function that does more than just skipping GETPID, GETDCR and GETBCR. Of course #2 is more elegant in that it doesn't force a mastership handover until someone really wants to access a device through the secondary master, but #1 is definitely simpler to implement. Ok, I see. Regarding #2, device is matched by PID, how do you want to handle this? I think we still need to use GETPID before registering the device. Can you see different path? > Mastership takeover can be faster without need to get BCR/DCR from devices again. Again, this should only happen once, not every time the master changes, simply because devices on the bus will stay the same. So I don't think optimization is a good argument. Indeed. Of course, devices does not have to be the same, after Hot-Join for example. But this is different story :) Regards, Boris Thanks, Przemek ^ permalink raw reply [flat|nested] 15+ messages in thread
* i3c_master_add_i3c_dev_locked() accept PID, BCR, DCR 2018-12-12 8:37 ` Przemyslaw Gaj @ 2018-12-12 8:52 ` Boris Brezillon 2018-12-12 9:02 ` Przemyslaw Gaj 0 siblings, 1 reply; 15+ messages in thread From: Boris Brezillon @ 2018-12-12 8:52 UTC (permalink / raw) To: linux-i3c On Wed, 12 Dec 2018 08:37:18 +0000 Przemyslaw Gaj <pgaj@cadence.com> wrote: > Hi Boris, > > ?On 12/12/18, 9:21 AM, "Boris Brezillon" <bbrezillon@kernel.org> wrote: > > EXTERNAL MAIL > > > Hi Przemek, > > On Wed, 12 Dec 2018 06:06:18 +0000 > Przemyslaw Gaj <pgaj@cadence.com> wrote: > > > > > I measured in my setup with a scope and the additional time to get that information is around 110 us per device and with only one device the bus takes around 550 us for the initialization. > > > > So, for now, we are transferring the same information two times over the bus, during DAA procedure and then manually. Three commands per device GETBCR/DCR/PID need six register accesses to write (in our controller). Don't you think it is more significant overhead? I'm just wondering. > > Not really, and I think it's actually more than 3 reg accesses per > device (you also have to clear the interrupts and read the RX FIFO). My > point is, this should only happen once at init (DAA) time and once in a > while if devices are joining the bus afterwards (Hot-Join requests). So, > this is not really a hot path, hence my decision to choose simplicity > over perfs: have a single function for the DAA (where PID, BCR and > DCR are available) and SETDASA (where these have to be retrieved using > GETPID, GETDCR, GETBCR) cases. > > Nothing is set in stone, and if one of you comes with a patch that > keep things simple while allowing one to skip the GET{PID,BCR,DCR} > commands, I'll consider merging it. But, to be honest, I don't think > this a priority. We still don't have support for DDR and mastership > handover, which in my opinion are much more valuable than optimizing > the bus-discovery path. > > Yes, I agree we can leave it for later. I'm composing patch containing mastership > and HRD mode. I'll release it soon. Please send those 2 series separately, as they are completely independent. > > > > > > > >> Let's assume we have a huge overhead caused by the OS (and all the > > > > >> scheduling involved), and make it 100usec. > > > > > And let's make it 500usec with the OS overhead, which is still not > > > > > particularly worrisome in term of boot-time. > > > > > > > > Ok, it can be done in the future. > > > > I was thinking also how to skip this in case of mastership request where device information is received in DEFSLVS command, secondary master can't do DAA. > > Okay, so this is actually one reason we'd want to add devices without > triggering transfers on the bus. The thing is, we need more than just > PID, BCR and DCR, and those information are not passed in the DEFSLVS > frame (MXDS, MRL, MWL, HDRCAP). IIRC, we started discussing that with > Arnd, Vitor and you and we listed 2 options: > > 1/ force the secondary master to take bus ownership after it has > received DEFSLVS so that it can retrieve the missing bits and finally > expose devices to its users > 2/ bind partially discovered devices to drivers and retrieve the device > information on-demand (when i3c_device_get_info() is called). > > If we go for option #2, we'll need a separate function that does more > than just skipping GETPID, GETDCR and GETBCR. Of course #2 is more > elegant in that it doesn't force a mastership handover until someone > really wants to access a device through the secondary master, but #1 is > definitely simpler to implement. > > Ok, I see. Regarding #2, device is matched by PID, how do you want to handle this? As I said, if we go for #2, we'll need a way to pass the PID, BCR and DCR we received from the DEFSLVS frame. So yes, that's actually one case where passing this info to i3c_master_add_i3c_dev_locked() makes sense. But, you'll also have to modify this function to skip i3c_master_retrieve_dev_info(). So maybe it's just simpler to have a separate function for this case (i3c_master_defslvs_add_dev_locked()?) > I think we still need to use GETPID before registering the device. Can you see different path? We need the PID for sure, but it's passed in DEFSLVS, so we should be good. > > > Mastership takeover can be faster without need to get BCR/DCR from devices again. > > Again, this should only happen once, not every time the master changes, > simply because devices on the bus will stay the same. So I don't think > optimization is a good argument. > > Indeed. Of course, devices does not have to be the same, after Hot-Join for example. > But this is different story :) Yes, HJ will trigger DAA which will trigger new DEFSLVS which might in turn trigger a mastership handover if we go for option #1. But I expect Hot-Join events to happen rarely, not every millisecond. So again, not really something we should try to optimize until it becomes the bottleneck for someone. ^ permalink raw reply [flat|nested] 15+ messages in thread
* i3c_master_add_i3c_dev_locked() accept PID, BCR, DCR 2018-12-12 8:52 ` Boris Brezillon @ 2018-12-12 9:02 ` Przemyslaw Gaj 2018-12-12 9:09 ` Boris Brezillon 0 siblings, 1 reply; 15+ messages in thread From: Przemyslaw Gaj @ 2018-12-12 9:02 UTC (permalink / raw) To: linux-i3c ?On 12/12/18, 9:52 AM, "Boris Brezillon" <bbrezillon@kernel.org> wrote: EXTERNAL MAIL On Wed, 12 Dec 2018 08:37:18 +0000 Przemyslaw Gaj <pgaj@cadence.com> wrote: > Hi Boris, > > On 12/12/18, 9:21 AM, "Boris Brezillon" <bbrezillon@kernel.org> wrote: > > EXTERNAL MAIL > > > Hi Przemek, > > On Wed, 12 Dec 2018 06:06:18 +0000 > Przemyslaw Gaj <pgaj@cadence.com> wrote: > > > > > I measured in my setup with a scope and the additional time to get that information is around 110 us per device and with only one device the bus takes around 550 us for the initialization. > > > > So, for now, we are transferring the same information two times over the bus, during DAA procedure and then manually. Three commands per device GETBCR/DCR/PID need six register accesses to write (in our controller). Don't you think it is more significant overhead? I'm just wondering. > > Not really, and I think it's actually more than 3 reg accesses per > device (you also have to clear the interrupts and read the RX FIFO). My > point is, this should only happen once at init (DAA) time and once in a > while if devices are joining the bus afterwards (Hot-Join requests). So, > this is not really a hot path, hence my decision to choose simplicity > over perfs: have a single function for the DAA (where PID, BCR and > DCR are available) and SETDASA (where these have to be retrieved using > GETPID, GETDCR, GETBCR) cases. > > Nothing is set in stone, and if one of you comes with a patch that > keep things simple while allowing one to skip the GET{PID,BCR,DCR} > commands, I'll consider merging it. But, to be honest, I don't think > this a priority. We still don't have support for DDR and mastership > handover, which in my opinion are much more valuable than optimizing > the bus-discovery path. > > Yes, I agree we can leave it for later. I'm composing patch containing mastership > and HRD mode. I'll release it soon. Please send those 2 series separately, as they are completely independent. Ok. > > > > > > > >> Let's assume we have a huge overhead caused by the OS (and all the > > > > >> scheduling involved), and make it 100usec. > > > > > And let's make it 500usec with the OS overhead, which is still not > > > > > particularly worrisome in term of boot-time. > > > > > > > > Ok, it can be done in the future. > > > > I was thinking also how to skip this in case of mastership request where device information is received in DEFSLVS command, secondary master can't do DAA. > > Okay, so this is actually one reason we'd want to add devices without > triggering transfers on the bus. The thing is, we need more than just > PID, BCR and DCR, and those information are not passed in the DEFSLVS > frame (MXDS, MRL, MWL, HDRCAP). IIRC, we started discussing that with > Arnd, Vitor and you and we listed 2 options: > > 1/ force the secondary master to take bus ownership after it has > received DEFSLVS so that it can retrieve the missing bits and finally > expose devices to its users > 2/ bind partially discovered devices to drivers and retrieve the device > information on-demand (when i3c_device_get_info() is called). > > If we go for option #2, we'll need a separate function that does more > than just skipping GETPID, GETDCR and GETBCR. Of course #2 is more > elegant in that it doesn't force a mastership handover until someone > really wants to access a device through the secondary master, but #1 is > definitely simpler to implement. > > Ok, I see. Regarding #2, device is matched by PID, how do you want to handle this? As I said, if we go for #2, we'll need a way to pass the PID, BCR and DCR we received from the DEFSLVS frame. So yes, that's actually one case where passing this info to i3c_master_add_i3c_dev_locked() makes sense. But, you'll also have to modify this function to skip i3c_master_retrieve_dev_info(). So maybe it's just simpler to have a separate function for this case (i3c_master_defslvs_add_dev_locked()?) > I think we still need to use GETPID before registering the device. Can you see different path? We need the PID for sure, but it's passed in DEFSLVS, so we should be good. As far as I remember it's not. Only BCR, DCR, DA and SA. > > > Mastership takeover can be faster without need to get BCR/DCR from devices again. > > Again, this should only happen once, not every time the master changes, > simply because devices on the bus will stay the same. So I don't think > optimization is a good argument. > > Indeed. Of course, devices does not have to be the same, after Hot-Join for example. > But this is different story :) Yes, HJ will trigger DAA which will trigger new DEFSLVS which might in turn trigger a mastership handover if we go for option #1. But I expect Hot-Join events to happen rarely, not every millisecond. So again, not really something we should try to optimize until it becomes the bottleneck for someone. I agree. Przemek ^ permalink raw reply [flat|nested] 15+ messages in thread
* i3c_master_add_i3c_dev_locked() accept PID, BCR, DCR 2018-12-12 9:02 ` Przemyslaw Gaj @ 2018-12-12 9:09 ` Boris Brezillon 0 siblings, 0 replies; 15+ messages in thread From: Boris Brezillon @ 2018-12-12 9:09 UTC (permalink / raw) To: linux-i3c On Wed, 12 Dec 2018 09:02:17 +0000 Przemyslaw Gaj <pgaj@cadence.com> wrote: > > > > Okay, so this is actually one reason we'd want to add devices without > > triggering transfers on the bus. The thing is, we need more than just > > PID, BCR and DCR, and those information are not passed in the DEFSLVS > > frame (MXDS, MRL, MWL, HDRCAP). IIRC, we started discussing that with > > Arnd, Vitor and you and we listed 2 options: > > > > 1/ force the secondary master to take bus ownership after it has > > received DEFSLVS so that it can retrieve the missing bits and finally > > expose devices to its users > > 2/ bind partially discovered devices to drivers and retrieve the device > > information on-demand (when i3c_device_get_info() is called). > > > > If we go for option #2, we'll need a separate function that does more > > than just skipping GETPID, GETDCR and GETBCR. Of course #2 is more > > elegant in that it doesn't force a mastership handover until someone > > really wants to access a device through the secondary master, but #1 is > > definitely simpler to implement. > > > > Ok, I see. Regarding #2, device is matched by PID, how do you want to handle this? > > As I said, if we go for #2, we'll need a way to pass the PID, BCR and > DCR we received from the DEFSLVS frame. So yes, that's actually one > case where passing this info to i3c_master_add_i3c_dev_locked() makes > sense. But, you'll also have to modify this function to skip > i3c_master_retrieve_dev_info(). So maybe it's just simpler to have a > separate function for this case (i3c_master_defslvs_add_dev_locked()?) > > > I think we still need to use GETPID before registering the device. Can you see different path? > > We need the PID for sure, but it's passed in DEFSLVS, so we should be > good. > > As far as I remember it's not. Only BCR, DCR, DA and SA. Oh, I thought it was. So option #2 is not even possible, which means we don't have a choice, we must implement solution #1. ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2018-12-12 9:09 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-12-10 19:21 i3c_master_add_i3c_dev_locked() accept PID, BCR, DCR vitor 2018-12-10 19:43 ` Boris Brezillon 2018-12-10 20:14 ` Boris Brezillon 2018-12-11 11:54 ` vitor 2018-12-11 11:58 ` Boris Brezillon 2018-12-11 12:07 ` vitor 2018-12-11 12:21 ` Boris Brezillon 2018-12-11 12:25 ` Boris Brezillon 2018-12-11 15:47 ` vitor 2018-12-12 6:06 ` Przemyslaw Gaj 2018-12-12 8:21 ` Boris Brezillon 2018-12-12 8:37 ` Przemyslaw Gaj 2018-12-12 8:52 ` Boris Brezillon 2018-12-12 9:02 ` Przemyslaw Gaj 2018-12-12 9:09 ` Boris Brezillon
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).