Re: [PATCH 1/6] powerpc/fsl-pci: Unify pci/pcie initialization code

[Scott Wood <scottwood@freescale.com>]

On 07/24/2012 09:35 PM, Jia Hongtao-B38951 wrote:
> 
> 
>> -----Original Message-----
>> From: Wood Scott-B07421
>> Sent: Wednesday, July 25, 2012 2:43 AM
>> To: Jia Hongtao-B38951
>> Cc: linuxppc-dev@lists.ozlabs.org; galak@kernel.crashing.org; Wood Scott-
>> B07421; Li Yang-R58472
>> Subject: Re: [PATCH 1/6] powerpc/fsl-pci: Unify pci/pcie initialization
>> code
>>
>> On 07/24/2012 05:20 AM, Jia Hongtao wrote:
>>> We unified the Freescale pci/pcie initialization by changing the
>> fsl_pci
>>> to a platform driver.
>>>
>>> In previous version pci/pcie initialization is in platform code which
>>> Initialize pci bridge base on EP/RC or host/agent settings.
>>
>> The previous version of what?  This patch, or the PCI code?  What
>> changed in this patch since the last time you sent it, and where is the
>> version number?
>>
>>> +#if defined(CONFIG_FSL_SOC_BOOKE) || defined(CONFIG_PPC_86xx)
>>> +static const struct of_device_id pci_ids[] = {
>>> +	{ .compatible = "fsl,mpc8540-pci", },
>>> +	{ .compatible = "fsl,mpc8548-pcie", },
>>> +	{ .compatible = "fsl,mpc8641-pcie", },
>>> +	{ .compatible = "fsl,p1022-pcie", },
>>> +	{ .compatible = "fsl,p1010-pcie", },
>>> +	{ .compatible = "fsl,p1023-pcie", },
>>> +	{ .compatible = "fsl,p4080-pcie", },
>>> +	{ .compatible = "fsl,qoriq-pcie-v2.3", },
>>> +	{ .compatible = "fsl,qoriq-pcie-v2.2", },
>>> +	{},
>>> +};
>>
>> Again, please base this on the latest tree, which has my PCI patches.
>> This table already exists in this file.  And you're still missing
>> fsl,mpc8610-pci.
> 
> Sorry fsl,mpc8610-pci will be added.

To what?  The table is already there in Linus's tree, with
fsl,mpc8610-pci.  You don't need to add it again.

>> It's too late for swiotlb here.  Again, please don't break something in
>> one patch and then fix it in a later patch.  Use "git rebase -i" to edit
>> your patchset into a reviewable, bisectable form.
>>
>> -Scott
> 
> Yes, bisectable requirement is sort of reasonable. 
> 
> But I check the SubmittingPatches Doc and it says "If one patch depends on
> another patch in order for a change to be complete, that is OK. Simply
> note 'this patch depends on patch X' in your patch description". In my
> opinion swiotlb is a whole functional patch so I separate them. Maybe
> I should add depends description in the next patch.

That's not what that means.  What it means is that if someone else has
already posted a patch, and your patch is supposed to go on top of that
patch, you should mention that.

> About all this patch set Leo and I insist to make it as a platform driver
> which is architectural better. I didn't base this patch set on the latest
> tree and it's unapplicable just because I want to show the whole idea of
> this patchset. If the idea is ok for upstream I will rebase the patch set.

If that's the case, you should label it as an [RFC PATCH] (stands for
Request For Comments), and mention under the --- line any known issues,
such as that it doesn't apply to the current tree.

But it would be a lot easier to comment on it if it were based on the
current code, rather than having to speculate what you'd do when you rebase.

-Scott