Re: [PATCH] i3c: make sure the PID is set before registering the device

["Przemysław Gaj" <pgaj@cadence.com>]

Hi Vitor,

The 12/10/2019 15:23, Vitor Soares wrote:
> 
> Hi Przemysław,
> 
> From: Przemysław Gaj <pgaj@cadence.com>
> Date: Tue, Dec 10, 2019 at 14:42:33
> 
> > Hi Vitor,
> > 
> > The 12/10/2019 14:28, Vitor Soares wrote:
> > > 
> > > Hi Boris,
> > > 
> > > From: Boris Brezillon <boris.brezillon@collabora.com>
> > > Date: Mon, Dec 09, 2019 at 12:26:16
> > > 
> > > > On Mon, 9 Dec 2019 12:20:03 +0000
> > > > Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> > > > 
> > > > > Hi Boris,
> > > > > 
> > > > > From: Boris Brezillon <boris.brezillon@collabora.com>
> > > > > Date: Mon, Dec 09, 2019 at 09:47:11
> > > > > 
> > > > > > On Sun, 8 Dec 2019 15:18:34 +0100
> > > > > > Przemysław Gaj <pgaj@cadence.com> wrote:
> > > > > >   
> > > > > > > From: Przemyslaw Gaj <pgaj@cadence.com>
> > > > > > > 
> > > > > > > Provisioned ID (PID) is the value with which device drivers are
> > > > > > > matched. I check the value before registering the device.
> > > > > > >   
> > > > > > 
> > > > > > Can this situation (having a device with a DA but without a valid PID)
> > > > > > happen right now or is this something you need to support secondary
> > > > > > masters who receive device DA (through DEFSVLS) without being able to
> > > > > > query extra information until they own the bus?  
> > > > > 
> > > > > This is the use case where a device fails the 
> > > > > i3c_master_pre_assign_dyn_addr() and I already mention it on [1].
> > > > > I still think the best way is to detach/free the devices that fails 
> > > > > during i3c_master_pre_assign_dyn_addr().
> > > > 
> > > > And I disagree (I gave my arguments already, so won't repeat them
> > > > here).
> > > 
> > > Sorry Boris, did you give? I ask you several times why to keep non DA 
> > > devices attached to the bus, yet you wasn't able to give me a technical 
> > > reason. Even from device model you should kept them.
> > 
> > I think we should keep them because framework should still have the
> > information about all the devices. We already discussed that, right?
> > Especially, when we have to deal with group addresses soon, it's better
> > to keep them.
> 
> Could you please point me where do we need it? We still have the 
> boardinfo that hold DT information which is what we need.

As MIPI 1.1 intruduces group address feature, I think it's better to
keep the devices on the list as the information about that device may be
needed when we manage devices under that group. I know the device is not
addressed that time but when it joins, we'll have the description
(possibly with group virtual I3C address) so we can assign it easily.

> 
> If a device doesn't have DA, why do we need it? By definition the i3c dev 
> desc is the discovered devices, if it doesn't have an DA isn't discovered 
> yet.

Yes, I know. MIPI spec defines that we should not define slave list
without DA but having devices or not in the subsystem is our internal
implementation and I think we can keep them and not serve them on the bus.
At least for now, when some big features are missing.

> 
> > 
> > > Honestly, I'm starting ask myself if this is something against because 
> > > every time that I'm trying to improve something I just see difficulty 
> > > from your side.
> > 
> > Don't forget that my patches are accepted after 6th, 7th versions. I
> > think that it just should work like that. This makes things better :)
> 
> I don't have issue in send multiple versions of my patches as long as 
> they are improved. In this case,  I don't agree with Boris's requirements 
> because in my tests I still have issues.
> Please make the exercise and check what needs to be done when a device 
> fails in i3c_master_pre_assign_dyn_addr() so everything goes ok and what 
> it is the advantage of kept i3c_dev_desc from those devices.

I forced the error of SETDASA and then joined the bus. Dupplicate was
found and that's it. What kind of issue you have? I can debug it on my
end and see if anything new is discovered.

It this problem is related to lack of slots in the HW, you can reuse
already allocated slot in ->attach_i3c_dev() and that should solve
the problem, right?

> 
> And still I have issue on my side. 
> 
> Did you test the previous patch? Did you find any issue? If so, you 
> wouldn't need the fix the i3c_master_register_new_i3c_devs() which is the 
> last instance of registering an i3c device.

I know, it's more like I'm preparing this for the next stage, mastership
takeover. I can introdue this also when introducing that patchset but I
want to keep mastership as simple as possible. I hope you understand
that. This should be beneficial for all of us.

> I would ask you to verify in v1.1 spec the CCCs table and verify if fix 
> the i3c_master_register_new_i3c_devs() is correct according what is 
> there.

Could you be more specific here? When you look into MIPI spec, you'll
see that we can have 'partialy discovered' devices by lack of information
in DEFSLVS.

> 
> > 
> > > 
> > > > Can we move on please?
> > > 
> > > I think you should start learning to listen the other and have the 
> > > greatness to accept the others experience and opinions. With your 
> > > experience you should already learn that.
> > > The previous patch makes all the sense yet your response was - I am not 
> > > convinced. Could you please give a proper justification? At least test 
> > > what I spend time to do?
> > > 
> > > > Can you send a new version that does what
> > > > I suggest, or should I ask Przemek to do it?
> > > 
> > > Does the ethics approve such attitude? Don't you see with kind of 
> > > attitude you are just putting people away of this subsystem?
> > > 
> > > > 
> > > > _______________________________________________
> > > > linux-i3c mailing list
> > > > linux-i3c@lists.infradead.org
> > > > https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.infradead.org_mailman_listinfo_linux-2Di3c&d=DwIGaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=qVuU64u9x77Y0Kd0PhDK_lpxFgg6PK9PateHwjb_DY0&m=cXyPCHO2vhcyD3aN-LnqOqzodbB9g-utJXeWOQRkJmk&s=odrtYhb-IaK-dfTzi1p7TwH2YJcnZ3RSdNNrKJjXFdM&e= 
> > > 
> > > Best regards,
> > > Vitor Soares
> > > 
> > 
> > -- 
> > -- 
> > Regards,
> > Przemek
> 
> Honestly, I think it is not right you being involved in this especially 
> by the companies we represent.

Oh, we are part of the community here, let's not speak about the
companies :)

> As I already said before I here to contribute and if I think my solution 
> is beneficial for everyone and doesn't have a negative impact on others 
> drivers, I can't accept unjustifiable requirements.



> 
> Best regards,
> Vitor Soares

-- 
-- 
Regards,
Przemek

_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c