Re: [PATCH v3 3/6] PCI: cadence: Add header support for PCIe HPA controller

Hans Zhang
2025-04-12T15:19:19Z

On 2025/4/12 04:31, Rob Herring wrote:
>> diff --git a/drivers/pci/controller/cadence/pcie-cadence-plat.c b/drivers/pci/controller/cadence/pcie-cadence-plat.c
>> index 0456845dabb9..b24176d4df1f 100644
>> --- a/drivers/pci/controller/cadence/pcie-cadence-plat.c
>> +++ b/drivers/pci/controller/cadence/pcie-cadence-plat.c
>> @@ -24,6 +24,15 @@ struct cdns_plat_pcie {
>>
>> struct cdns_plat_pcie_of_data {
>> bool is_rc;
>> + bool is_hpa;
>
> These can be bitfields (e.g. "is_rc: 1").
>

Hi Rob,

Thanks your for reply. Will change.

>> + 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;
>> };
>>
>> static const struct of_device_id cdns_plat_pcie_of_match[];
>> @@ -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;
>> + rc->pcie.is_rc = data->is_rc;
>> +
>> + /* 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;
>
> Why not just store the match data ptr instead of having 2 copies of the
> information?

Will change.

>
>> +
>> cdns_plat_pcie->pcie = &rc->pcie;
>>
>> ret = cdns_pcie_init_phy(dev, cdns_plat_pcie->pcie);
>> @@ -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;
>> + ep->pcie.is_rc = data->is_rc;
>> +
>> + /* Store all the register bank offset */
>> + ep->pcie.cdns_pcie_reg_offsets.ip_reg_bank_off = data->ip_reg_bank_off;
>> + ep->pcie.cdns_pcie_reg_offsets.ip_cfg_ctrl_reg_off = data->ip_cfg_ctrl_reg_off;
>> + ep->pcie.cdns_pcie_reg_offsets.axi_mstr_common_off = data->axi_mstr_common_off;
>> + ep->pcie.cdns_pcie_reg_offsets.axi_master_off = data->axi_master_off;
>> + ep->pcie.cdns_pcie_reg_offsets.axi_slave_off = data->axi_slave_off;
>> + ep->pcie.cdns_pcie_reg_offsets.axi_hls_off = data->axi_hls_off;
>> + ep->pcie.cdns_pcie_reg_offsets.axi_ras_off = data->axi_ras_off;
>> + ep->pcie.cdns_pcie_reg_offsets.axi_dti_off = data->axi_dti_off;
>> +
>> cdns_plat_pcie->pcie = &ep->pcie;
>>
>> ret = cdns_pcie_init_phy(dev, cdns_plat_pcie->pcie);
>> @@ -150,10 +185,54 @@ static void cdns_plat_pcie_shutdown(struct platform_device *pdev)
>>
>> 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,
>
> You can omit anything initialized to 0.

Will change.

Best regards,
Hans