Re: [PATCH 2/7] PCI: cadence: Add header support for PCIe next generation controllers

[Bjorn Helgaas]

On Thu, Mar 27, 2025 at 11:26:21AM +0000, Manikandan Karunakaran Pillai wrote:
> Add the required definitions for register addresses and register bits
> for the next generation Cadence PCIe controllers - High performance
> rchitecture(HPA) controllers. Define register access functions for
> SoC platforms with different base address

"Next generation" is not really meaningful since there will probably
be a next-next generation, and "high performance architecture" is
probably not much better because the next-next generation will
presumably be "higher than high performance."

I would just use the codename or marketing name and omit "next
generation."  Maybe that's "HPA" and we can look forward to another
superlative name for the next generation after this :)

s/High performance/High Performance/
s/rchitecture/Architecture/

Add period at end of sentence.

> +#define CDNS_PCIE_HPA_LM_EP_FUNC_BAR_CFG(bar, fn) \
> +	(((bar) < BAR_3) ? CDNS_PCIE_HPA_LM_EP_FUNC_BAR_CFG0(fn) : \
> +			CDNS_PCIE_HPA_LM_EP_FUNC_BAR_CFG1(fn))
> +#define CDNS_PCIE_HPA_LM_EP_FUNC_BAR_CFG0(pfn) (0x4000 * (pfn))
> +#define CDNS_PCIE_HPA_LM_EP_FUNC_BAR_CFG1(pfn) ((0x4000 * (pfn)) + 0x04)
> +#define CDNS_PCIE_HPA_LM_EP_VFUNC_BAR_CFG(bar, fn) \
> +	(((bar) < BAR_3) ? CDNS_PCIE_HPA_LM_EP_VFUNC_BAR_CFG0(fn) : \
> +			CDNS_PCIE_HPA_LM_EP_VFUNC_BAR_CFG1(fn))
> +#define CDNS_PCIE_HPA_LM_EP_VFUNC_BAR_CFG0(vfn) ((0x4000 * (vfn)) + 0x08)
> +#define CDNS_PCIE_HPA_LM_EP_VFUNC_BAR_CFG1(vfn) ((0x4000 * (vfn)) + 0x0C)
> +#define CDNS_PCIE_HPA_LM_EP_FUNC_BAR_CFG_BAR_APERTURE_MASK(f) \
> +	(GENMASK(9, 4) << ((f) * 10))
> +#define CDNS_PCIE_HPA_LM_EP_FUNC_BAR_CFG_BAR_APERTURE(b, a) \
> +	(((a) << (4 + ((b) * 10))) & (CDNS_PCIE_HPA_LM_EP_FUNC_BAR_CFG_BAR_APERTURE_MASK(b)))
> +#define CDNS_PCIE_HPA_LM_EP_FUNC_BAR_CFG_BAR_CTRL_MASK(f) \
> +	(GENMASK(3, 0) << ((f) * 10))
> +#define CDNS_PCIE_HPA_LM_EP_FUNC_BAR_CFG_BAR_CTRL(b, c) \
> +	(((c) << ((b) * 10)) & (CDNS_PCIE_HPA_LM_EP_FUNC_BAR_CFG_BAR_CTRL_MASK(b)))

Wow, these names are ... loooong.  This would be more readable if they
could be abbreviated a bit.  "PCIE" could be dropped with no loss.
There are probably other words that could be dropped too.

>  struct cdns_pcie_ops {
>  	int	(*start_link)(struct cdns_pcie *pcie);
>  	void	(*stop_link)(struct cdns_pcie *pcie);
>  	bool	(*link_up)(struct cdns_pcie *pcie);
>  	u64     (*cpu_addr_fixup)(struct cdns_pcie *pcie, u64 cpu_addr);
> +	int	(*pcie_host_init_root_port)(struct cdns_pcie_rc *rc);
> +	int	(*pcie_host_bar_ib_config)(struct cdns_pcie_rc *rc,
> +					   enum cdns_pcie_rp_bar bar,
> +					   u64 cpu_addr, u64 size,
> +					   unsigned long flags);
> +	int	(*pcie_host_init_address_translation)(struct cdns_pcie_rc *rc);
> +	void	(*pcie_detect_quiet_min_delay_set)(struct cdns_pcie *pcie);
> +	void	(*pcie_set_outbound_region)(struct cdns_pcie *pcie, u8 busnr, u8 fn,
> +					    u32 r, bool is_io, u64 cpu_addr,
> +					    u64 pci_addr, size_t size);
> +	void	(*pcie_set_outbound_region_for_normal_msg)(struct cdns_pcie *pcie,
> +							   u8 busnr, u8 fn, u32 r,
> +							   u64 cpu_addr);
> +	void	(*pcie_reset_outbound_region)(struct cdns_pcie *pcie, u32 r);

Also some pretty long names here that don't fit the style of the
existing members (none of the others have the "pcie_" prefix).

> + * struct cdns_pcie_reg_offset - Register bank offset for a platform
> + * @ip_reg_bank_off - ip register bank start offset
> + * @iP_cfg_ctrl_reg_off - ip config contrl register start offset

s/@iP_cfg_ctrl_reg_off/@ip_cfg_ctrl_reg_off/

"scripts/kernel-doc -none <file>" should find errors like this for you.

s/contrl/control/

> + * @axi_mstr_common_off - AXI master common register start
> + * @axi_slave_off - AXI skave offset start

s/skave/slave/

> +struct cdns_pcie_reg_offset {
> +	u32  ip_reg_bank_off;
> +	u32  ip_cfg_ctrl_reg_off;
> +	u32  axi_mstr_common_off;
> +	u32  axi_slave_off;
> +	u32  axi_master_off;
> +	u32  axi_hls_off;
> +	u32  axi_ras_off;
> +	u32  axi_dti_off;
>  };
>  
>  /**
> @@ -305,10 +551,12 @@ struct cdns_pcie {
>  	struct resource		*mem_res;
>  	struct device		*dev;
>  	bool			is_rc;
> +	bool			is_hpa;
>  	int			phy_count;
>  	struct phy		**phy;
>  	struct device_link	**link;
>  	const struct cdns_pcie_ops *ops;
> +	struct cdns_pcie_reg_offset cdns_pcie_reg_offsets;

Why does struct cdns_pcie need to contain an entire struct
cdns_pcie_reg_offset instead of just a pointer to it?

> +static inline u32 cdns_reg_bank_to_off(struct cdns_pcie *pcie, enum cdns_pcie_reg_bank bank)
> +{
> +	u32 offset;
> +
> +	switch (bank) {
> +	case REG_BANK_IP_REG:
> +		offset = pcie->cdns_pcie_reg_offsets.ip_reg_bank_off;

It's a little hard to untangle this without being able to apply the
series, but normally we would add the struct cdns_pcie_reg_offset
definition, the inclusion in struct cdns_pcie, this use of it, and the
setting of it in the same patch.

>  #ifdef CONFIG_PCIE_CADENCE_EP
> @@ -556,7 +909,10 @@ static inline int cdns_pcie_ep_setup(struct cdns_pcie_ep *ep)
>  	return 0;
>  }
>  #endif
> -

Probably spurious change?  Looks like we would want the blank line
here.

> +bool cdns_pcie_linkup(struct cdns_pcie *pcie);
> +bool cdns_pcie_hpa_linkup(struct cdns_pcie *pcie);
> +int cdns_pcie_hpa_startlink(struct cdns_pcie *pcie);
> +void cdns_pcie_hpa_stop_link(struct cdns_pcie *pcie);
>  void cdns_pcie_detect_quiet_min_delay_set(struct cdns_pcie *pcie);