i3c_master_add_i3c_dev_locked() accept PID, BCR, DCR

[pgaj@cadence.com (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