i3c_master_add_i3c_dev_locked() accept PID, BCR, DCR

i3c_master_add_i3c_dev_locked() accept PID, BCR, DCR

[vitor]

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

i3c_master_add_i3c_dev_locked() accept PID, BCR, DCR

[Boris Brezillon]

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

i3c_master_add_i3c_dev_locked() accept PID, BCR, DCR

[Boris Brezillon]

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.

i3c_master_add_i3c_dev_locked() accept PID, BCR, DCR

[vitor]

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

i3c_master_add_i3c_dev_locked() accept PID, BCR, DCR

[Boris Brezillon]

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?

i3c_master_add_i3c_dev_locked() accept PID, BCR, DCR

[vitor]

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.

i3c_master_add_i3c_dev_locked() accept PID, BCR, DCR

[Boris Brezillon]

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.

i3c_master_add_i3c_dev_locked() accept PID, BCR, DCR

[Boris Brezillon]

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.

i3c_master_add_i3c_dev_locked() accept PID, BCR, DCR

[vitor]

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.

i3c_master_add_i3c_dev_locked() accept PID, BCR, DCR

[Przemyslaw Gaj]

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=
    
    

i3c_master_add_i3c_dev_locked() accept PID, BCR, DCR

[Boris Brezillon]

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

i3c_master_add_i3c_dev_locked() accept PID, BCR, DCR

[Przemyslaw Gaj]

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

i3c_master_add_i3c_dev_locked() accept PID, BCR, DCR

[Boris Brezillon]

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.

i3c_master_add_i3c_dev_locked() accept PID, BCR, DCR

[Przemyslaw Gaj]

?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

i3c_master_add_i3c_dev_locked() accept PID, BCR, DCR

[Boris Brezillon]

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.