Re: [PATCH v3] PCI: keystone: Fix pci_ops for AM654x SoC

[Siddharth Vadapalli <s-vadapalli@ti.com>]

Hello Bjorn,

Could you please merge this patch?

On 19/10/23 15:35, Serge Semin wrote:
> On Thu, Oct 19, 2023 at 01:43:30PM +0530, Siddharth Vadapalli wrote:
>> In the process of converting .scan_bus() callbacks to .add_bus(), the
>> ks_pcie_v3_65_scan_bus() function was changed to ks_pcie_v3_65_add_bus().
>> The .scan_bus() method belonged to ks_pcie_host_ops which was specific
>> to controller version 3.65a, while the .add_bus() method had been added
>> to ks_pcie_ops which is shared between the controller versions 3.65a and
>> 4.90a. Neither the older ks_pcie_v3_65_scan_bus() method, nor the newer
>> ks_pcie_v3_65_add_bus() method are applicable to the controller version
>> 4.90a which is present in AM654x SoCs.
>>
>> Thus, fix this by creating ks_pcie_am6_ops for the AM654x SoC which uses DW
>> PCIe IP-core version 4.90a controller and omitting the .add_bus() method
>> which is not applicable to the 4.90a controller. Update ks_pcie_host_init()
>> accordingly in order to set the pci_ops to ks_pcie_am6_ops if the platform
>> is AM654x SoC and to ks_pcie_ops otherwise, by making use of the "is_am6"
>> flag.
>>
>> Fixes: 6ab15b5e7057 ("PCI: dwc: keystone: Convert .scan_bus() callback to use add_bus")
>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> 
> LGTM. Thanks!
> Reviewed-by: Serge Semin <fancer.lancer@gmail.com>
> 
> One more note is further just to draw attention to possible driver
> simplifications.
> 
>> ---
>> Hello,
>>
>> This patch is based on linux-next tagged next-20231018.
>>
>> The v2 of this patch is at:
>> https://lore.kernel.org/r/20231018075038.2740534-1-s-vadapalli@ti.com/
>>
>> Changes since v2:
>> - Implemented Serge's suggestion of adding a new pci_ops structure for
>>   AM654x SoC using DWC PCIe IP-core version 4.90a controller.
>> - Created struct pci_ops ks_pcie_am6_ops for AM654x SoC without the
>>   .add_bus method while retaining other ops from ks_pcie_ops.
>> - Updated ks_pcie_host_init() to set pci_ops to ks_pcie_am6_ops if the
>>   platform is AM654x and to ks_pcie_ops otherwise by making use of the
>>   already existing "is_am6" flag.
>> - Combined the section:
>> 	if (!ks_pcie->is_am6)
>>  		pp->bridge->child_ops = &ks_child_pcie_ops;
>>   into the newly added ELSE condition.
>>
>> The v1 of this patch is at:
>> https://lore.kernel.org/r/20231011123451.34827-1-s-vadapalli@ti.com/
>>
>> While there are a lot of changes since v1 and this patch could have been
>> posted as a v1 patch itself, I decided to post it as the v2 of the patch
>> mentioned above since it aims to address the issue described by the v1
>> patch and is similar in that sense. However, the solution to the issue
>> described in the v1 patch appears to be completely different from what
>> was implemented in the v1 patch. Thus, the commit message and subject of
>> this patch have been modified accordingly.
>>
>> Changes since v1:
>> - Updated patch subject and commit message.
>> - Determined that issue is not with the absence of Link as mentioned in
>>   v1 patch. Even with Link up and endpoint device connected, if
>>   ks_pcie_v3_65_add_bus() is invoked and executed, all reads to the
>>   MSI-X offsets return 0xffffffff when pcieport driver attempts to setup
>>   AER and PME services. The all Fs return value indicates that the MSI-X
>>   configuration is failing even if Endpoint device is connected. This is
>>   because the ks_pcie_v3_65_add_bus() function is not applicable to the
>>   AM654x SoC which uses DW PCIe IP-core version 4.90a.
>>
>> Regards,
>> Siddharth.
>>
>>  drivers/pci/controller/dwc/pci-keystone.c | 13 +++++++++++--
>>  1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
>> index 49aea6ce3e87..66341a0b6c6b 100644
>> --- a/drivers/pci/controller/dwc/pci-keystone.c
>> +++ b/drivers/pci/controller/dwc/pci-keystone.c
>> @@ -487,6 +487,12 @@ static struct pci_ops ks_pcie_ops = {
>>  	.add_bus = ks_pcie_v3_65_add_bus,
>>  };
>>  
>> +static struct pci_ops ks_pcie_am6_ops = {
>> +	.map_bus = dw_pcie_own_conf_map_bus,
>> +	.read = pci_generic_config_read,
>> +	.write = pci_generic_config_write,
>> +};
>> +
>>  /**
>>   * ks_pcie_link_up() - Check if link up
>>   * @pci: A pointer to the dw_pcie structure which holds the DesignWare PCIe host
>> @@ -804,9 +810,12 @@ static int __init ks_pcie_host_init(struct dw_pcie_rp *pp)
>>  	struct keystone_pcie *ks_pcie = to_keystone_pcie(pci);
>>  	int ret;
>>  
>> -	pp->bridge->ops = &ks_pcie_ops;
>> -	if (!ks_pcie->is_am6)
>> +	if (ks_pcie->is_am6) {
>> +		pp->bridge->ops = &ks_pcie_am6_ops;
>> +	} else {
> 
>> +		pp->bridge->ops = &ks_pcie_ops;
>>  		pp->bridge->child_ops = &ks_child_pcie_ops;
> 
> Bjorn, could you please clarify the next suggestion? I'm not that
> fluent in the PCIe core details, but based on the
> pci_host_bridge.child_ops and pci_host_bridge.ops names, the first ops
> will be utilized for the child (non-root) PCIe buses, meanwhile the
> later ones - for the root bus only (see pci_alloc_child_bus()). Right?
> 
> If so then either the pci_is_root_bus() check can be dropped from the
> ks_pcie_v3_65_add_bus() method since the ops it belong to will be
> utilized for the root bus anyway, or the entire ks_child_pcie_ops
> instance can be dropped since the ks_pcie_v3_65_add_bus() method will
> be no-op for the child buses anyway meanwhile ks_child_pcie_ops
> matches to ks_pcie_ops in the rest of the ops. After doing that I
> would have also changed the ks_pcie_v3_65_add_bus name to
> ks_pcie_v3_65_add_root_bus() in anyway. Am I right?
> 
> -Serge(y)
> 
>> +	}
>>  
>>  	ret = ks_pcie_config_legacy_irq(ks_pcie);
>>  	if (ret)
>> -- 
>> 2.34.1
>>

-- 
Regards,
Siddharth.