Message-ID: <20241119101121.t4kaaeuvj37scmxm@thinkpad>
Date: Tue, 19 Nov 2024 15:41:21 +0530
From: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
To: Frank Li <Frank.li@....com>
Cc: Rob Herring <robh@...nel.org>, Saravana Kannan <saravanak@...gle.com>,
	Jingoo Han <jingoohan1@...il.com>,
	Lorenzo Pieralisi <lpieralisi@...nel.org>,
	Krzysztof Wilczyński <kw@...ux.com>,
	Bjorn Helgaas <bhelgaas@...gle.com>,
	Richard Zhu <hongxing.zhu@....com>,
	Lucas Stach <l.stach@...gutronix.de>,
	Shawn Guo <shawnguo@...nel.org>,
	Sascha Hauer <s.hauer@...gutronix.de>,
	Pengutronix Kernel Team <kernel@...gutronix.de>,
	Fabio Estevam <festevam@...il.com>, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org, imx@...ts.linux.dev
Subject: Re: [PATCH v7 2/7] PCI: dwc: Using parent_bus_addr in of_range to
 eliminate cpu_addr_fixup()

On Fri, Nov 15, 2024 at 02:18:38PM -0500, Frank Li wrote:

[...]

> > > +		if (pci->using_dtbus_info) {
> > > +			index = of_property_match_string(np, "reg-names", "config");
> > > +			if (index < 0)
> > > +				return -EINVAL;
> > > +			/*
> > > +			 * Retrieve the parent bus address of PCI config space.
> > > +			 * If the parent bus ranges in the device tree provide
> > > +			 * the correct address conversion information, set
> > > +			 * 'using_dtbus_info' to true, The 'cpu_addr_fixup()'
> > > +			 * can be eliminated.
> > > +			 */
> >
> > Nobody will switch to 'ranges' property if you mention it in comments. We
> > usually add dev_warn_once() to print a warning for broken DT so that the users
> > will try to fix it. You can use below diff (as a separate patch ofc):
> >
> > ```
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> > index 6d6cbc8b5b2c..d1e5395386fe 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > @@ -844,6 +844,9 @@ void dw_pcie_iatu_detect(struct dw_pcie *pci)
> >                  dw_pcie_cap_is(pci, IATU_UNROLL) ? "T" : "F",
> >                  pci->num_ob_windows, pci->num_ib_windows,
> >                  pci->region_align / SZ_1K, (pci->region_limit + 1) / SZ_1G);
> > +
> > +       if (pci->ops && pci->ops->cpu_addr_fixup)
> > +               dev_warn_once(pci->dev, "Broken \"ranges\" property detected. Please fix DT!\n");
> 
> How about "Detect use old method "cpu_addr_fixup()", it should correct DT's
> 'ranges' and remove cpu_addr_fixup()?
> 

Hmm, yeah makes sense:

	/*
	 * If the parent 'ranges' property in DT correctly describes the address
	 * translation, cpu_addr_fixup() callback is not needed.
	 */
	dev_warn_once(pci->dev, "cpu_addr_fixup() usage detected. Please fix DT!\n";

But then the drivers need to be smart enough to detect the valid parent 'ranges'
property and then only use the callback. Because, callback has to be present to
support older DTs.

> >  }
> >
> >  static u32 dw_pcie_readl_dma(struct dw_pcie *pci, u32 reg)
> > ```
> >
> > > +			of_property_read_reg(np, index, &pp->cfg0_base, NULL);
> >
> > Can you explain what is going on here?
> 
> Because dwc use reg-name 'config' to get config space,
> of_property_read_reg() will get untranslate address 'parent' bus address.
> <0x8ff00000 0x80000> at example address.
> 
> cfg0_base is used to set outbound ATU.
> 

Ok, please add a comment like this:

	/* Get the untranslated 'config' address */

Same for other usage of of_property_read_reg().

> >
> > > +		}
> > > +
> > >  		pp->va_cfg0_base = devm_pci_remap_cfg_resource(dev, res);
> > >  		if (IS_ERR(pp->va_cfg0_base))
> > >  			return PTR_ERR(pp->va_cfg0_base);
> > > @@ -462,6 +505,9 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
> > >  		pp->io_base = pci_pio_to_address(win->res->start);
> > >  	}
> > >
> > > +	if (dw_pcie_get_untranslate_addr(pci, pp->io_bus_addr, &pp->io_base))
> > > +		return -ENODEV;
> >
> > Use actual return value here and below.
> >
> > > +
> > >  	/* Set default bus ops */
> > >  	bridge->ops = &dw_pcie_ops;
> > >  	bridge->child_ops = &dw_child_pcie_ops;
> > > @@ -722,6 +768,8 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
> > >
> > >  	i = 0;
> > >  	resource_list_for_each_entry(entry, &pp->bridge->windows) {
> > > +		resource_size_t parent_bus_addr;
> > > +
> > >  		if (resource_type(entry->res) != IORESOURCE_MEM)
> > >  			continue;
> > >
> > > @@ -730,9 +778,14 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
> > >
> > >  		atu.index = i;
> > >  		atu.type = PCIE_ATU_TYPE_MEM;
> > > -		atu.cpu_addr = entry->res->start;
> > > +		parent_bus_addr = entry->res->start;
> > >  		atu.pci_addr = entry->res->start - entry->offset;
> > >
> > > +		if (dw_pcie_get_untranslate_addr(pci, entry->res->start, &parent_bus_addr))
> > > +			return -EINVAL;
> > > +
> > > +		atu.cpu_addr = parent_bus_addr;
> > > +
> > >  		/* Adjust iATU size if MSG TLP region was allocated before */
> > >  		if (pp->msg_res && pp->msg_res->parent == entry->res)
> > >  			atu.size = resource_size(entry->res) -
> > > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> > > index 347ab74ac35aa..f8067393ad35a 100644
> > > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > > @@ -463,6 +463,14 @@ struct dw_pcie {
> > >  	struct reset_control_bulk_data	core_rsts[DW_PCIE_NUM_CORE_RSTS];
> > >  	struct gpio_desc		*pe_rst;
> > >  	bool			suspended;
> > > +	/*
> > > +	 * Use device tree 'ranges' property of bus node instead using
> > > +	 * cpu_addr_fixup(). Some old platform dts 'ranges' in bus node may not
> > > +	 * reflect real hardware's behavior. In case break these platform back
> > > +	 * compatibility, add below flags. Set it true if dts already correct
> > > +	 * indicate bus fabric address convert.
> >
> > 	/*
> > 	 * This flag indicates that the vendor driver uses devicetree 'ranges'
> > 	 * property to allow iATU to use the Intermediate Address (IA) for
> > 	 * outbound mapping. Using this flag also avoids the usage of
> > 	 * 'cpu_addr_fixup' callback implementation in the driver.
> > 	 */
> >
> > > +	 */
> > > +	bool			using_dtbus_info;
> >
> > 'use_dt_ranges'?
> 
> It will be confused because pcie node alreadys use ranges, just parent bus
> 's ranges is wrong.
> 
> 'use_dtbus_ranges' ?
> 

There is nothing called 'dtbus'. How about, "use_parent_dt_ranges"?

- Mani

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

Powered by blists - more mailing lists