Re: [PATCH V1] PCI: dwc: Use dev_info for PCIe link down event logging

From: Jon Hunter
Date: Wed Sep 14 2022 - 07:53:12 EST



On 14/09/2022 12:43, Manivannan Sadhasivam wrote:
On Wed, Sep 14, 2022 at 12:25:51PM +0100, Jon Hunter wrote:

On 14/09/2022 12:18, Manivannan Sadhasivam wrote:
On Wed, Sep 14, 2022 at 04:32:10PM +0530, Vidya Sagar wrote:
Agree with Mani.
Not all form factors support PRSNT pin (Ex:- M.2 Key-M form factor) and even
if the slot form factor supports PRSNT, it is not mandatory to have a GPIO
routed to the PRSNT pin of the slot.

Right.

Also, since these are development platforms, we wouldn't want to keep
changing a controller's status in the DT, instead want to know the device's
presence/absence (by way of link up) looking at the log.
In my honest opinion, I don't think the absence of a device in the slot
should be treated as an error.


As I mentioned earlier, timeout can happen due to an issue with PHY layer
also. In those cases, "dev_err()" is relevant.

And I also agree that absence of the device should not be treated as an
error. But my question is, if you know that the slot is going to be
empty always, why cannot you just disable it in dts?

I really don't think that makes sense from a usability perspective. You want
to allow users to connect PCI cards and for them to work without having to
update the DTB. I have plenty of open PCI slots on my PC and I would be a
bit upset if someone told me I need to update the PC firmware each time I
wanted to use a new slot.


My question was, "do you think the slot is going to be empty always".
This can happen with slots exposed through a connector (not a PCIe one) and
users would plug in add-on cards for accessing the slots. In those
cases, the add-on specific devicetree can enable the PCIe instance and
use it.

But from your reply, I can infer that the slot is exposed on a standard
PCIe connector and users would connect a PCIe device any time.

Correct it is exposed via a standard connector. Yes for PCIe slots that are not connected to an on-board device or connector, in that case it would make sense to disable, but this is not the case here.

Jon

--
nvpublic