Re: [PATCH] PCI: iproc: move quirks to driver

[Ray Jui <ray.jui@broadcom.com>]

On 12/11/19 2:34 PM, Bjorn Helgaas wrote:
> On Wed, Dec 11, 2019 at 05:45:11PM +0000, Wei Liu wrote:
>> The quirks were originally enclosed by ifdef. That made the quirks not
>> to be applied when respective drivers were compiled as modules.
>>
>> Move the quirks to driver code to fix the issue.
>>
>> Signed-off-by: Wei Liu <wei.liu@kernel.org>
> 
> This straddles the core and native driver boundary, so I applied it to
> pci/misc for v5.6.  Thanks, I think this is a great solution!  It's
> always nice when we can encapsulate device-specific things in a
> driver.
> 

Opps! I was going to review and comment and you are quick, :)

I was going to say, I think it's better to keep this quirk in 
"pcie-iproc.c" instead of "pcie-iproc-platform.c".

The quirk is specific to certain PCIe devices under iProc (activated 
based on device ID), but should not be tied to a specific bus 
architecture (i.e., platform vs BCMA).

Thanks,

Ray

>> ---
>>   drivers/pci/controller/pcie-iproc-platform.c | 24 ++++++++++++++++++
>>   drivers/pci/quirks.c                         | 26 --------------------
>>   2 files changed, 24 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/pci/controller/pcie-iproc-platform.c b/drivers/pci/controller/pcie-iproc-platform.c
>> index ff0a81a632a1..4e6f7cdd9a25 100644
>> --- a/drivers/pci/controller/pcie-iproc-platform.c
>> +++ b/drivers/pci/controller/pcie-iproc-platform.c
>> @@ -19,6 +19,30 @@
>>   #include "../pci.h"
>>   #include "pcie-iproc.h"
>>   
>> +static void quirk_paxc_bridge(struct pci_dev *pdev)
>> +{
>> +	/*
>> +	 * The PCI config space is shared with the PAXC root port and the first
>> +	 * Ethernet device.  So, we need to workaround this by telling the PCI
>> +	 * code that the bridge is not an Ethernet device.
>> +	 */
>> +	if (pdev->hdr_type == PCI_HEADER_TYPE_BRIDGE)
>> +		pdev->class = PCI_CLASS_BRIDGE_PCI << 8;
>> +
>> +	/*
>> +	 * MPSS is not being set properly (as it is currently 0).  This is
>> +	 * because that area of the PCI config space is hard coded to zero, and
>> +	 * is not modifiable by firmware.  Set this to 2 (e.g., 512 byte MPS)
>> +	 * so that the MPS can be set to the real max value.
>> +	 */
>> +	pdev->pcie_mpss = 2;
>> +}
>> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_BROADCOM, 0x16cd, quirk_paxc_bridge);
>> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_BROADCOM, 0x16f0, quirk_paxc_bridge);
>> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_BROADCOM, 0xd750, quirk_paxc_bridge);
>> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_BROADCOM, 0xd802, quirk_paxc_bridge);
>> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_BROADCOM, 0xd804, quirk_paxc_bridge);
>> +
>>   static const struct of_device_id iproc_pcie_of_match_table[] = {
>>   	{
>>   		.compatible = "brcm,iproc-pcie",
>> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
>> index 4937a088d7d8..202837ed68c9 100644
>> --- a/drivers/pci/quirks.c
>> +++ b/drivers/pci/quirks.c
>> @@ -2381,32 +2381,6 @@ DECLARE_PCI_FIXUP_ENABLE(PCI_VENDOR_ID_BROADCOM,
>>   			 PCI_DEVICE_ID_TIGON3_5719,
>>   			 quirk_brcm_5719_limit_mrrs);
>>   
>> -#ifdef CONFIG_PCIE_IPROC_PLATFORM
>> -static void quirk_paxc_bridge(struct pci_dev *pdev)
>> -{
>> -	/*
>> -	 * The PCI config space is shared with the PAXC root port and the first
>> -	 * Ethernet device.  So, we need to workaround this by telling the PCI
>> -	 * code that the bridge is not an Ethernet device.
>> -	 */
>> -	if (pdev->hdr_type == PCI_HEADER_TYPE_BRIDGE)
>> -		pdev->class = PCI_CLASS_BRIDGE_PCI << 8;
>> -
>> -	/*
>> -	 * MPSS is not being set properly (as it is currently 0).  This is
>> -	 * because that area of the PCI config space is hard coded to zero, and
>> -	 * is not modifiable by firmware.  Set this to 2 (e.g., 512 byte MPS)
>> -	 * so that the MPS can be set to the real max value.
>> -	 */
>> -	pdev->pcie_mpss = 2;
>> -}
>> -DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_BROADCOM, 0x16cd, quirk_paxc_bridge);
>> -DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_BROADCOM, 0x16f0, quirk_paxc_bridge);
>> -DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_BROADCOM, 0xd750, quirk_paxc_bridge);
>> -DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_BROADCOM, 0xd802, quirk_paxc_bridge);
>> -DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_BROADCOM, 0xd804, quirk_paxc_bridge);
>> -#endif
>> -
>>   /*
>>    * Originally in EDAC sources for i82875P: Intel tells BIOS developers to
>>    * hide device 6 which configures the overflow device access containing the
>> -- 
>> 2.20.1
>>