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);
