On Wed, Jan 03, 2018 at 02:39:04PM +0800, Honghui Zhang wrote:

On Tue, 2018-01-02 at 10:56 +0000, Lorenzo Pieralisi wrote:

quoted

On Thu, Dec 28, 2017 at 09:39:12AM +0800, Honghui Zhang wrote:

quoted

On Wed, 2017-12-27 at 12:45 -0600, Bjorn Helgaas wrote:

quoted

On Wed, Dec 27, 2017 at 08:59:54AM +0800, honghui.zhang at mediatek.com wrote:

quoted

From: Honghui Zhang <redacted>

quoted

quoted

quoted

quoted

+		/* Set up class code for MT7622 */
+		val = PCI_CLASS_BRIDGE_PCI << 16;
+		writel(val, port->base + PCIE_CONF_CLASS);

1) Your comments mention MT7622 specifically, but this code is run for
both mt2712-pcie and mt7622-pcie.  If this code is safe and necessary
for both mt2712-pcie and mt7622-pcie, please remove the mention of
MT7622.

Hmm, the code snippet added here will only be executed by MT7622, since
MT2712 will not enter this  "if (pcie->base) {"  condition.
Should the mention of MT7622 must be removed in this case?

You should add an explicit way (eg of_device_is_compatible() match for
instance) to apply the quirk just on the platform that requires it.

Checking for "if (pcie->base)" is really not the way to do it.

hi, Lorenzo,
Thanks very much for your advise.
Passing the compatible string or platform data into this function needed
to add some new field in the struct mtk_pcie_port, then I guess both set
it for MT2712 and MT7622 is an easy way, since re-setting those values
for MT2712 is safe.

quoted

quoted

quoted

2) The first comment mentions both "vendor ID and device ID" but you
don't write the device ID.  Since this code applies to both
mt2712-pcie and mt7622-pcie, my guess is that you don't *want* to
write the device ID.  If that's the case, please fix the comment.

My bad, I did not check the comments carefully.
Thanks.

quoted

3) If you only need to set the vendor ID, you're performing a 32-bit
write (writel()) to update a 16-bit value.  Please use writew()
instead.

Ok, thanks, I guess I could use the following code snippet in the next
version:
val = readl(port->base + PCIE_CONF_VENDOR_ID)
val &= ~GENMASK(15, 0);
val |= PCI_VENDOR_ID_MEDIATEK;
writel(val, port->base + PCIE_CONF_VENDOR_ID);

Have you read Bjorn's comment ? Or there is a problem with using
a writew() ?

This control register is a 32bit register, I'm not sure whether the apb
bus support write an 16bit value with 16bit but not 32bit address
alignment. I prefer the more safety old way of writel.

I need to do more test about the writew if the code elegant is more
important.

I will mark this patch as "Changes Requested" waiting for a new
version, thanks.

Lorenzo