linux-kernel - Re: [PATCH v3 2/4] PCI: qcom: Move host bridge 'phy' and 'reset' pointers to struct qcom_pcie

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <nhvnnjviqmz2gqgjlqjvnlrvyj7brvjeep4wchhhfxy7pgkbbz@ejqcopawboqp>
Date: Mon, 15 Sep 2025 18:31:32 +0530
From: Manivannan Sadhasivam <mani@...nel.org>
To: Bjorn Helgaas <helgaas@...nel.org>
Cc: manivannan.sadhasivam@....qualcomm.com, 
	Lorenzo Pieralisi <lpieralisi@...nel.org>, Krzysztof Wilczyński <kwilczynski@...nel.org>, 
	Rob Herring <robh@...nel.org>, Bjorn Helgaas <bhelgaas@...gle.com>, 
	Bartosz Golaszewski <brgl@...ev.pl>, Saravana Kannan <saravanak@...gle.com>, 
	linux-pci@...r.kernel.org, linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org, 
	devicetree@...r.kernel.org, Krishna Chaitanya Chundru <krishna.chundru@....qualcomm.com>, 
	Brian Norris <briannorris@...omium.org>
Subject: Re: [PATCH v3 2/4] PCI: qcom: Move host bridge 'phy' and 'reset'
 pointers to struct qcom_pcie_port

On Fri, Sep 12, 2025 at 06:23:48PM GMT, Bjorn Helgaas wrote:
> On Fri, Sep 12, 2025 at 02:05:02PM +0530, Manivannan Sadhasivam via B4 Relay wrote:
> > From: Manivannan Sadhasivam <manivannan.sadhasivam@....qualcomm.com>
> > 
> > DT binding allows specifying 'phy' and 'reset' properties in both host
> > bridge and Root Port nodes, though specifying in the host bridge node is
> > marked as deprecated. Still, the pcie-qcom driver should support both
> > combinations for maintaining the DT backwards compatibility. For this
> > purpose, the driver is holding the relevant pointers of these properties in
> > two structs: struct qcom_pcie_port and struct qcom_pcie.
> > 
> > However, this causes confusion and increases the driver complexity. Hence,
> > move the pointers from struct qcom_pcie to struct qcom_pcie_port. As a
> > result, even if these properties are specified in the host bridge node,
> > the pointers will be stored in struct qcom_pcie_port as if the properties
> > are specified in a single Root Port node. This logic simplifies the driver
> > a lot.
> 
> > @@ -297,11 +295,8 @@ static void qcom_perst_assert(struct qcom_pcie *pcie, bool assert)
> >  	struct qcom_pcie_port *port;
> >  	int val = assert ? 1 : 0;
> >  
> > -	if (list_empty(&pcie->ports))
> > -		gpiod_set_value_cansleep(pcie->reset, val);
> > -	else
> > -		list_for_each_entry(port, &pcie->ports, list)
> > -			gpiod_set_value_cansleep(port->reset, val);
> > +	list_for_each_entry(port, &pcie->ports, list)
> > +		gpiod_set_value_cansleep(port->reset, val);
> 
> This is so much nicer, thanks for doing this!
> 
> >  static int qcom_pcie_parse_legacy_binding(struct qcom_pcie *pcie)
> >  {
> >  	struct device *dev = pcie->pci->dev;
> > +	struct qcom_pcie_port *port;
> > +	struct gpio_desc *reset;
> > +	struct phy *phy;
> >  	int ret;
> >  
> > -	pcie->phy = devm_phy_optional_get(dev, "pciephy");
> > -	if (IS_ERR(pcie->phy))
> > -		return PTR_ERR(pcie->phy);
> > +	phy = devm_phy_optional_get(dev, "pciephy");
> > +	if (IS_ERR(phy))
> > +		return PTR_ERR(phy);
> 
> Seems like it would be easier to integrate this fallback into
> qcom_pcie_parse_port() instead if separating it into
> qcom_pcie_parse_legacy_binding().
> 
> What if you did something like this in qcom_pcie_parse_port():
> 
>   qcom_pcie_parse_port
>   {
>       reset = devm_fwnode_gpiod_get(dev, of_fwnode_handle(node),
> 				    "reset",  GPIOD_OUT_HIGH, "PERST#");
>       if (IS_ERR(reset)) {
> 	  reset = devm_gpiod_get_optional(dev, "perst", GPIOD_OUT_HIGH);
> 	  if (IS_ERR(reset))
> 	      return PTR_ERR(reset);
>       }
>       ...
> 
> Then you could share all the port kzalloc and port list management.
> 
> Could do the same with the PHY stuff.
> 

Yeah, we could do something like this, but this will unnecessarily do a lookup
for 'perst-gpios' for every bridge node where there will only be 'reset-gpios'.
It is not a big deal though, since this code is only called in the probe path,
but I find the qcom_pcie_parse_legacy_binding() function to be visually
appealing as it separates legacy binding handling from the Root Port binding
and also makes it easy to drop the support for it in the future.

- Mani

-- 
மணிவண்ணன் சதாசிவம்