Linux-i3c Archive on lore.kernel.org
 help / color / Atom feed
From: Vitor Soares <Vitor.Soares@synopsys.com>
To: "Przemysław Gaj" <pgaj@cadence.com>
Cc: "linux-i3c@lists.infradead.org" <linux-i3c@lists.infradead.org>,
	Boris Brezillon <boris.brezillon@collabora.com>,
	"rafalc@cadence.com" <rafalc@cadence.com>,
	"bbrezillon@kernel.org" <bbrezillon@kernel.org>
Subject: RE: [PATCH] i3c: make sure the PID is set before registering the device
Date: Wed, 11 Dec 2019 12:21:33 +0000
Message-ID: <CH2PR12MB4216AECB558C90894AEDD7E0AE5A0@CH2PR12MB4216.namprd12.prod.outlook.com> (raw)
In-Reply-To: <20191211093316.GB10780@global.cadence.com>

Hi Przemysław,

From: Przemysław Gaj <pgaj@cadence.com>
Date: Wed, Dec 11, 2019 at 09:33:18

> 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.

Although you present this use case, in practice you won't need the 
i3c_dev_desc allocated because I have introduced 
i3c_master_init_i3c_dev_boardinfo() in this patch series.
if you check the i3c_master_add_i3c_dev_locked() this i3c_dev_desc that 
we are keeping unnecessarily is detach and free later.
So I wonder why do we need to wait and waste resources until there? 

> 
> > 
> > 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.

Sorry, you are only presenting an assumption of something that I already 
presented facts that is problematic and a solution to fix it.

> 
> > 
> > > 
> > > > 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.

Please check the table and you will realize that the fix isn't 
compatible.
Sorry but for now this is all. 

Best regards,
Vitor Soares


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

      reply index

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-12-08 14:18 Przemysław Gaj
2019-12-09  9:47 ` Boris Brezillon
2019-12-09 12:20   ` Vitor Soares
2019-12-09 12:26     ` Boris Brezillon
2019-12-10 14:28       ` Vitor Soares
2019-12-10 14:42         ` Przemysław Gaj
2019-12-10 15:23           ` Vitor Soares
2019-12-11  9:33             ` Przemysław Gaj
2019-12-11 12:21               ` Vitor Soares [this message]

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CH2PR12MB4216AECB558C90894AEDD7E0AE5A0@CH2PR12MB4216.namprd12.prod.outlook.com \
    --to=vitor.soares@synopsys.com \
    --cc=bbrezillon@kernel.org \
    --cc=boris.brezillon@collabora.com \
    --cc=linux-i3c@lists.infradead.org \
    --cc=pgaj@cadence.com \
    --cc=rafalc@cadence.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-i3c Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-i3c/0 linux-i3c/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-i3c linux-i3c/ https://lore.kernel.org/linux-i3c \
		linux-i3c@lists.infradead.org
	public-inbox-index linux-i3c

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-i3c


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git