Re: [PATCH 3/7] PCI: cadence: Add platform related architecture and register information

[Bjorn Helgaas <helgaas@kernel.org>]

On Thu, Mar 27, 2025 at 11:39:43AM +0000, Manikandan Karunakaran Pillai wrote:
> Add the register bank offsets for different platforms and update the global
> platform data - platform architecture, EP or RP configuration and the
> correct values of register offsets for different register banks during the
> platform probe

Add period.

> @@ -72,6 +81,19 @@ static int cdns_plat_pcie_probe(struct platform_device *pdev)
>  		rc = pci_host_bridge_priv(bridge);
>  		rc->pcie.dev = dev;
>  		rc->pcie.ops = &cdns_plat_ops;
> +		rc->pcie.is_hpa = data->is_hpa;
> +		/*

Add a blank line between the code and the start of the comment.

> +		 * Store all the register bank offsets
> +		 */
> +		rc->pcie.cdns_pcie_reg_offsets.ip_reg_bank_off = data->ip_reg_bank_off;
> +		rc->pcie.cdns_pcie_reg_offsets.ip_cfg_ctrl_reg_off = data->ip_cfg_ctrl_reg_off;
> +		rc->pcie.cdns_pcie_reg_offsets.axi_mstr_common_off = data->axi_mstr_common_off;
> +		rc->pcie.cdns_pcie_reg_offsets.axi_master_off = data->axi_master_off;
> +		rc->pcie.cdns_pcie_reg_offsets.axi_slave_off = data->axi_slave_off;
> +		rc->pcie.cdns_pcie_reg_offsets.axi_hls_off = data->axi_hls_off;
> +		rc->pcie.cdns_pcie_reg_offsets.axi_ras_off = data->axi_ras_off;
> +		rc->pcie.cdns_pcie_reg_offsets.axi_dti_off = data->axi_dti_off;

But what's the point of copying all these offsets?  Can't you just
keep the offsets in a couple separate structures and save the pointer?

It looks like cdns_plat_pcie_host_of_data and
cdns_plat_pcie_ep_of_data could use the same pointer, as could
cdns_plat_pcie_hpa_host_of_data and cdns_plat_pcie_hpa_ep_of_data.

> @@ -99,6 +121,19 @@ static int cdns_plat_pcie_probe(struct platform_device *pdev)
>  
>  		ep->pcie.dev = dev;
>  		ep->pcie.ops = &cdns_plat_ops;
> +		ep->pcie.is_hpa = data->is_hpa;
> +		/*

Add blank line again.

> +		 * Store all the register bank offset

>  static const struct cdns_plat_pcie_of_data cdns_plat_pcie_host_of_data = {
>  	.is_rc = true,
> +	.is_hpa = false,
> +	.ip_reg_bank_off = 0x0,
> +	.ip_cfg_ctrl_reg_off = 0x0,
> +	.axi_mstr_common_off = 0x0,
> +	.axi_slave_off = 0x0,
> +	.axi_master_off = 0x0,
> +	.axi_hls_off = 0x0,
> +	.axi_ras_off = 0x0,
> +	.axi_dti_off = 0x0,

In my opinion, you should only initialize fields that are non-false
and non-zero.  Then the differences are more obvious.

Bjorn