Re: [PATCH v2 4/6] PCI: brcmstb: add Broadcom STB PCIe host controller driver

From: Florian Fainelli <f.fainelli@gmail.com>
To: Andrew Murray <andrew.murray@arm.com>,
	Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
Cc: maz@kernel.org, linux-kernel@vger.kernel.org,
	Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
	Eric Anholt <eric@anholt.net>, Stefan Wahren <wahrenst@gmx.net>,
	Florian Fainelli <f.fainelli@gmail.com>,
	bcm-kernel-feedback-list@broadcom.com,
	james.quinlan@broadcom.com, mbrugger@suse.com,
	phil@raspberrypi.org, jeremy.linton@arm.com,
	Bjorn Helgaas <bhelgaas@google.com>,
	linux-pci@vger.kernel.org, linux-rpi-kernel@lists.infradead.org,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v2 4/6] PCI: brcmstb: add Broadcom STB PCIe host controller driver
Date: Tue, 19 Nov 2019 10:34:59 -0800	[thread overview]
Message-ID: <8b5ea071-d7a1-ea31-c7fe-3b4585d9cc36@gmail.com> (raw)
In-Reply-To: <20191119162502.GS43905@e119886-lin.cambridge.arm.com>

On 11/19/19 8:25 AM, Andrew Murray wrote:
> On Tue, Nov 12, 2019 at 04:59:23PM +0100, Nicolas Saenz Julienne wrote:
>> From: Jim Quinlan <james.quinlan@broadcom.com>
>>
>> This commit adds the basic Broadcom STB PCIe controller.  Missing is the
>> ability to process MSI. This functionality is added in a subsequent
>> commit.
>>
>> The PCIe block contains an MDIO interface.  This is a local interface
>> only accessible by the PCIe controller.  It cannot be used or shared
>> by any other HW.  As such, the small amount of code for this
>> controller is included in this driver as there is little upside to put
>> it elsewhere.
> 
> This commit message hasn't changed, despite earlier feedback.

Please strip out large parts of the original patch that you are not
quoting for future responses.

[snip]

> 
> I'd rather see use of the pcie_cfg_data structure removed from this series.
> 
> I've seen the comments in the previous thread [1], and I understand that
> the intention is that this driver will eventually be used for other SOCs.
> 
> However this indirection isn't needed *now* and it makes reviewing this
> patch more difficult. If and when a later series is made to cover other
> SOCs - then I'd expect that series to find a way to apply this indirection.

I am not completely sold on the difficulty to review given that the
indirection is in place for only 3 registers which are used in only 3
functions:

brcm_pcie_bridge_sw_init_set()
brcm_pcie_perst_set()
brcm_pcie_map_conf()

but if you think that is a deal breaker, then, okay, let's get rid of it
and we will add it back for other STB SoCs in the future.

> 
> And if that later series is more difficult to review because of the newly
> added indirection, then I'd expect an early patch of that series to apply
> the indirection in a single patch - which would be easy to review.
> 
> The other risk of such premature changes like this is that when you come
> to adding other SOCs, you may then discover that there were shortcomings
> in the way you've approached it here.

2711 is the latest SoC that has actually been supported by this driver,
every other ones that this driver will support in the future has been in
production for years and all the quirks/subtleties are known. This means
that 2711 was added while fitting in the existing abstraction and
Nicholas took out every other chip to leave 2711 only.
-- 
Florian