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

[Hans Zhang <hans.zhang@cixtech.com>]

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