Re: [PATCH v5 09/12] PCI: brcmstb: Refactor for chips with many regular inbound windows

[Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>]

On Wed, Aug 07, 2024 at 07:16:44AM -0700, Florian Fainelli wrote:
> 
> 
> On 8/7/2024 7:04 AM, Manivannan Sadhasivam wrote:
> > On Wed, Jul 31, 2024 at 06:28:23PM -0400, Jim Quinlan wrote:
> > > Provide support for new chips with multiple inbound windows while
> > > keeping the legacy support for the older chips.
> > > 
> > > In existing chips there are three inbound windows with fixed purposes: the
> > > first was for mapping SoC internal registers, the second was for memory,
> > > and the third was for memory but with the endian swapped.  Typically, only
> > > one window was used.
> > > 
> > > Complicating the inbound window usage was the fact that the PCIe HW would
> > > do a baroque internal mapping of system memory, and concatenate the regions
> > > of multiple memory controllers.
> > > 
> > > Newer chips such as the 7712 and Cable Modem SOCs take a step forward and
> > > drop the internal mapping while providing for multiple inbound windows.
> > > This works in concert with the dma-ranges property, where each provided
> > > range becomes an inbound window.
> > > 
> > > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com>
> > > ---
> 
> [snip]
> 
> > > +static void set_inbound_win_registers(struct brcm_pcie *pcie,
> > > +				      const struct inbound_win *inbound_wins,
> > > +				      int num_inbound_wins)
> > > +{
> > > +	void __iomem *base = pcie->base;
> > > +	int i;
> > > +
> > > +	for (i = 1; i <= num_inbound_wins; i++) {
> > > +		u64 pci_offset = inbound_wins[i].pci_offset;
> > > +		u64 cpu_addr = inbound_wins[i].cpu_addr;
> > > +		u64 size = inbound_wins[i].size;
> > > +		u32 reg_offset = brcm_bar_reg_offset(i);
> > > +		u32 tmp = lower_32_bits(pci_offset);
> > > +
> > > +		u32p_replace_bits(&tmp, brcm_pcie_encode_ibar_size(size),
> > > +				  PCIE_MISC_RC_BAR1_CONFIG_LO_SIZE_MASK);
> > > +
> > > +		/* Write low */
> > > +		writel(tmp, base + reg_offset);
> > 
> > Can you use writel_relaxed() instead? Here and below. I don't see a necessity to
> > use the barrier that comes with non-relaxed version of writel.
> 
> Out of curiosity what is the reasoning here for asking to use
> writel_relaxed(), this is not a hot path, this is a configuration path
> anyway. I am not certain clear on the implication of using writel_relaxed()
> on systems like 7712/2712 where the busing is different from the other STB
> chips.

It is the general recommendation (although not documented). If the code path do
not need ordering/barrier, then there is no need for non-relaxed variants.

Btw, if the register accesses are to the same domain (like PCIe), then certainly
you do not need barrier as writes to the same domain are ordered.

Problem with readl/writel is that the drivers started using non-relaxed variants
as if it is a norm and completely ignored the relaxed variants.

- Mani

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