>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 :)
>
"HPA" will be used from the next patch series
>s/High performance/High Performance/
>s/rchitecture/Architecture/
>
>Add period at end of sentence.
>
OK
>> +#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.
>
The names are in sync with the hardware register specification and also with the existing
code for legacy Cadence PCIe controller. Hence would like to retain them for readability.
>> 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).
"pcie" is removed from the function names to be in sync with other function pointer naming
>
>> + * 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.
kernel-doc --none is run on all the files for the next patch series
>
>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?
The cdns_pci_reg_offset is declared only in this global store for further usage by the driver. There is only
Struct defined for a PCIe controller and it made sense to define it inside this global context struct for controllers
>
>> +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.
>
Ok
>> #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.
>
Ok
>> +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);
