diff mbox

[01/11] ARM64 / PCI: introduce struct pci_controller for ACPI

Message ID 1432644564-24746-2-git-send-email-hanjun.guo@linaro.org
State Changes Requested
Headers show

Commit Message

Hanjun Guo May 26, 2015, 12:49 p.m. UTC 
ARM64 ACPI based PCI host bridge init needs a arch dependent
struct pci_controller to accommodate common PCI host bridge
code which is introduced later, or it will lead to compile
errors on ARM64.

Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
Tested-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
CC: Arnd Bergmann <arnd@arndb.de>
CC: Catalin Marinas <catalin.marinas@arm.com>
CC: Liviu Dudau <Liviu.Dudau@arm.com>
CC: Lorenzo Pieralisi <Lorenzo.Pieralisi@arm.com>
CC: Will Deacon <will.deacon@arm.com>
---
 arch/arm64/include/asm/pci.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

Liviu Dudau May 26, 2015, 4:58 p.m. UTC | #1 
On Tue, May 26, 2015 at 01:49:14PM +0100, Hanjun Guo wrote:
> ARM64 ACPI based PCI host bridge init needs a arch dependent
> struct pci_controller to accommodate common PCI host bridge
> code which is introduced later, or it will lead to compile
> errors on ARM64.

Hi Hanjun,

Two questions: why don't you introduce this patch next to the
one that is going to make use of it (or even merge it there)?
Second, why is the whole struct pci_controller not surrounded
by #ifdef CONFIG_ACPI as you are implying that this is needed
only for ACPI?

Btw, looking through the whole series I'm not (yet) convinced
that this is needed at all.

Best regards,
Liviu

> 
> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
> Tested-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
> CC: Arnd Bergmann <arnd@arndb.de>
> CC: Catalin Marinas <catalin.marinas@arm.com>
> CC: Liviu Dudau <Liviu.Dudau@arm.com>
> CC: Lorenzo Pieralisi <Lorenzo.Pieralisi@arm.com>
> CC: Will Deacon <will.deacon@arm.com>
> ---
>  arch/arm64/include/asm/pci.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/pci.h b/arch/arm64/include/asm/pci.h
> index b008a72..7088495 100644
> --- a/arch/arm64/include/asm/pci.h
> +++ b/arch/arm64/include/asm/pci.h
> @@ -10,6 +10,16 @@
>  #include <asm-generic/pci-bridge.h>
>  #include <asm-generic/pci-dma-compat.h>
>  
> +struct acpi_device;
> +
> +struct pci_controller {
> +#ifdef CONFIG_ACPI
> +	struct acpi_device *companion;	/* ACPI companion device */
> +#endif
> +	int		segment;	/* PCI domain */
> +	int		node;		/* NUMA node */
> +};
> +
>  #define PCIBIOS_MIN_IO		0x1000
>  #define PCIBIOS_MIN_MEM		0
>  
> -- 
> 1.9.1
>
Jiang Liu May 26, 2015, 5:20 p.m. UTC | #2 
On 2015/5/27 0:58, Liviu Dudau wrote:
> On Tue, May 26, 2015 at 01:49:14PM +0100, Hanjun Guo wrote:
>> ARM64 ACPI based PCI host bridge init needs a arch dependent
>> struct pci_controller to accommodate common PCI host bridge
>> code which is introduced later, or it will lead to compile
>> errors on ARM64.
> 
> Hi Hanjun,
> 
> Two questions: why don't you introduce this patch next to the
> one that is going to make use of it (or even merge it there)?
> Second, why is the whole struct pci_controller not surrounded
> by #ifdef CONFIG_ACPI as you are implying that this is needed
> only for ACPI?
> 
> Btw, looking through the whole series I'm not (yet) convinced
> that this is needed at all.
Hi Liviu,
	This structure is required by the requested patch set
at http://patchwork.ozlabs.org/patch/472249/, which consolidates
the common code to support PCI host bridge into ACPI core.
Thanks!
Gerry

> 
> Best regards,
> Liviu
> 
>>
>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
>> Tested-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
>> CC: Arnd Bergmann <arnd@arndb.de>
>> CC: Catalin Marinas <catalin.marinas@arm.com>
>> CC: Liviu Dudau <Liviu.Dudau@arm.com>
>> CC: Lorenzo Pieralisi <Lorenzo.Pieralisi@arm.com>
>> CC: Will Deacon <will.deacon@arm.com>
>> ---
>>  arch/arm64/include/asm/pci.h | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/arch/arm64/include/asm/pci.h b/arch/arm64/include/asm/pci.h
>> index b008a72..7088495 100644
>> --- a/arch/arm64/include/asm/pci.h
>> +++ b/arch/arm64/include/asm/pci.h
>> @@ -10,6 +10,16 @@
>>  #include <asm-generic/pci-bridge.h>
>>  #include <asm-generic/pci-dma-compat.h>
>>  
>> +struct acpi_device;
>> +
>> +struct pci_controller {
>> +#ifdef CONFIG_ACPI
>> +	struct acpi_device *companion;	/* ACPI companion device */
>> +#endif
>> +	int		segment;	/* PCI domain */
>> +	int		node;		/* NUMA node */
>> +};
>> +
>>  #define PCIBIOS_MIN_IO		0x1000
>>  #define PCIBIOS_MIN_MEM		0
>>  
>> -- 
>> 1.9.1
>>
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hanjun Guo May 27, 2015, 8:21 a.m. UTC | #3 
Hi Liviu,

On 2015年05月27日 01:20, Jiang Liu wrote:
> On 2015/5/27 0:58, Liviu Dudau wrote:
>> On Tue, May 26, 2015 at 01:49:14PM +0100, Hanjun Guo wrote:
>>> ARM64 ACPI based PCI host bridge init needs a arch dependent
>>> struct pci_controller to accommodate common PCI host bridge
>>> code which is introduced later, or it will lead to compile
>>> errors on ARM64.
>>
>> Hi Hanjun,
>>
>> Two questions: why don't you introduce this patch next to the
>> one that is going to make use of it (or even merge it there)?

this is because of this patch is needed by Jiang Liu's patch set
to fix the compile error on ARM64, I'd rather do that, but It's
better to let Jiang Liu's patch goes in, and then this one, that's
why I prepared a single patch for the struct. (I mentioned it
in the cover letter)

>> Second, why is the whole struct pci_controller not surrounded
>> by #ifdef CONFIG_ACPI as you are implying that this is needed
>> only for ACPI?

I hope it can be reused, since the NUMA node and segment (domain)
is both needed for DT and ACPI, if it's not the case foe now, I
can surrounded them all by #ifdef CONFIG_ACPI.

>>
>> Btw, looking through the whole series I'm not (yet) convinced
>> that this is needed at all.
> Hi Liviu,
> 	This structure is required by the requested patch set
> at http://patchwork.ozlabs.org/patch/472249/, which consolidates
> the common code to support PCI host bridge into ACPI core.

Jiang, thanks for the explanation :)

Thanks
Hanjun
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Liviu Dudau May 27, 2015, 9:47 a.m. UTC | #4 
On Tue, May 26, 2015 at 06:20:40PM +0100, Jiang Liu wrote:
> On 2015/5/27 0:58, Liviu Dudau wrote:
> > On Tue, May 26, 2015 at 01:49:14PM +0100, Hanjun Guo wrote:
> >> ARM64 ACPI based PCI host bridge init needs a arch dependent
> >> struct pci_controller to accommodate common PCI host bridge
> >> code which is introduced later, or it will lead to compile
> >> errors on ARM64.
> > 
> > Hi Hanjun,
> > 
> > Two questions: why don't you introduce this patch next to the
> > one that is going to make use of it (or even merge it there)?
> > Second, why is the whole struct pci_controller not surrounded
> > by #ifdef CONFIG_ACPI as you are implying that this is needed
> > only for ACPI?
> > 
> > Btw, looking through the whole series I'm not (yet) convinced
> > that this is needed at all.
> Hi Liviu,
> 	This structure is required by the requested patch set
> at http://patchwork.ozlabs.org/patch/472249/, which consolidates
> the common code to support PCI host bridge into ACPI core.
> Thanks!
> Gerry

Hi Jiang,

Thanks for pointing me on the right answer, I've missed that series!
Probably not the best place to comment on that series here, but I
wonder why did you not made the pci_controller structure available
in a more generic header file that can be included so that arches
don't have to redefine the structure every time. After all, you are
trying to consolidate things.

Oh, and pci_controller name throws a lot of false negatives, maybe
a more specific one (acpi_pci_controller?) would make things clear?

Best regards,
Liviu

> 
> > 
> > Best regards,
> > Liviu
> > 
> >>
> >> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
> >> Tested-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
> >> CC: Arnd Bergmann <arnd@arndb.de>
> >> CC: Catalin Marinas <catalin.marinas@arm.com>
> >> CC: Liviu Dudau <Liviu.Dudau@arm.com>
> >> CC: Lorenzo Pieralisi <Lorenzo.Pieralisi@arm.com>
> >> CC: Will Deacon <will.deacon@arm.com>
> >> ---
> >>  arch/arm64/include/asm/pci.h | 10 ++++++++++
> >>  1 file changed, 10 insertions(+)
> >>
> >> diff --git a/arch/arm64/include/asm/pci.h b/arch/arm64/include/asm/pci.h
> >> index b008a72..7088495 100644
> >> --- a/arch/arm64/include/asm/pci.h
> >> +++ b/arch/arm64/include/asm/pci.h
> >> @@ -10,6 +10,16 @@
> >>  #include <asm-generic/pci-bridge.h>
> >>  #include <asm-generic/pci-dma-compat.h>
> >>  
> >> +struct acpi_device;
> >> +
> >> +struct pci_controller {
> >> +#ifdef CONFIG_ACPI
> >> +	struct acpi_device *companion;	/* ACPI companion device */
> >> +#endif
> >> +	int		segment;	/* PCI domain */
> >> +	int		node;		/* NUMA node */
> >> +};
> >> +
> >>  #define PCIBIOS_MIN_IO		0x1000
> >>  #define PCIBIOS_MIN_MEM		0
> >>  
> >> -- 
> >> 1.9.1
> >>
> > 
>
Jiang Liu May 27, 2015, 11:29 a.m. UTC | #5 
On 2015/5/27 17:47, Liviu Dudau wrote:
> On Tue, May 26, 2015 at 06:20:40PM +0100, Jiang Liu wrote:
>> On 2015/5/27 0:58, Liviu Dudau wrote:
>>> On Tue, May 26, 2015 at 01:49:14PM +0100, Hanjun Guo wrote:
>>>> ARM64 ACPI based PCI host bridge init needs a arch dependent
>>>> struct pci_controller to accommodate common PCI host bridge
>>>> code which is introduced later, or it will lead to compile
>>>> errors on ARM64.
>>>
>>> Hi Hanjun,
>>>
>>> Two questions: why don't you introduce this patch next to the
>>> one that is going to make use of it (or even merge it there)?
>>> Second, why is the whole struct pci_controller not surrounded
>>> by #ifdef CONFIG_ACPI as you are implying that this is needed
>>> only for ACPI?
>>>
>>> Btw, looking through the whole series I'm not (yet) convinced
>>> that this is needed at all.
>> Hi Liviu,
>> 	This structure is required by the requested patch set
>> at http://patchwork.ozlabs.org/patch/472249/, which consolidates
>> the common code to support PCI host bridge into ACPI core.
>> Thanks!
>> Gerry
> 
> Hi Jiang,
> 
> Thanks for pointing me on the right answer, I've missed that series!
> Probably not the best place to comment on that series here, but I
> wonder why did you not made the pci_controller structure available
> in a more generic header file that can be included so that arches
> don't have to redefine the structure every time. After all, you are
> trying to consolidate things.
> 
> Oh, and pci_controller name throws a lot of false negatives, maybe
> a more specific one (acpi_pci_controller?) would make things clear?
Hi Liviu,
	It's a trade-off. Once I tried to rename it too, but gave up
later. There are several reasons to keep it as is:
1) Several architectures define pci_controller to support PCI
   root bus.
2) struct pci_controller is a generic concept, I guess, and ACPI code
   extends pci_controller to host some ACPI specific data on IA64 and
   x86.
3) It will cause big code changes if we rename pci_controller to
   something else.
Thanks!
Gerry

> 
> Best regards,
> Liviu
> 
>>
>>>
>>> Best regards,
>>> Liviu
>>>
>>>>
>>>> Signed-off-by: Hanjun Guo <hanjun.guo@linaro.org>
>>>> Tested-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
>>>> CC: Arnd Bergmann <arnd@arndb.de>
>>>> CC: Catalin Marinas <catalin.marinas@arm.com>
>>>> CC: Liviu Dudau <Liviu.Dudau@arm.com>
>>>> CC: Lorenzo Pieralisi <Lorenzo.Pieralisi@arm.com>
>>>> CC: Will Deacon <will.deacon@arm.com>
>>>> ---
>>>>  arch/arm64/include/asm/pci.h | 10 ++++++++++
>>>>  1 file changed, 10 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/include/asm/pci.h b/arch/arm64/include/asm/pci.h
>>>> index b008a72..7088495 100644
>>>> --- a/arch/arm64/include/asm/pci.h
>>>> +++ b/arch/arm64/include/asm/pci.h
>>>> @@ -10,6 +10,16 @@
>>>>  #include <asm-generic/pci-bridge.h>
>>>>  #include <asm-generic/pci-dma-compat.h>
>>>>  
>>>> +struct acpi_device;
>>>> +
>>>> +struct pci_controller {
>>>> +#ifdef CONFIG_ACPI
>>>> +	struct acpi_device *companion;	/* ACPI companion device */
>>>> +#endif
>>>> +	int		segment;	/* PCI domain */
>>>> +	int		node;		/* NUMA node */
>>>> +};
>>>> +
>>>>  #define PCIBIOS_MIN_IO		0x1000
>>>>  #define PCIBIOS_MIN_MEM		0
>>>>  
>>>> -- 
>>>> 1.9.1
>>>>
>>>
>>
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ganapatrao Kulkarni Sept. 7, 2015, 4:14 a.m. UTC | #6 
Hi Hanjun,

On Wed, May 27, 2015 at 1:51 PM, Hanjun Guo <hanjun.guo@linaro.org> wrote:
> Hi Liviu,
>
> On 2015年05月27日 01:20, Jiang Liu wrote:
>>
>> On 2015/5/27 0:58, Liviu Dudau wrote:
>>>
>>> On Tue, May 26, 2015 at 01:49:14PM +0100, Hanjun Guo wrote:
>>>>
>>>> ARM64 ACPI based PCI host bridge init needs a arch dependent
>>>> struct pci_controller to accommodate common PCI host bridge
>>>> code which is introduced later, or it will lead to compile
>>>> errors on ARM64.
>>>
>>>
>>> Hi Hanjun,
>>>
>>> Two questions: why don't you introduce this patch next to the
>>> one that is going to make use of it (or even merge it there)?
>
>
> this is because of this patch is needed by Jiang Liu's patch set
> to fix the compile error on ARM64, I'd rather do that, but It's
> better to let Jiang Liu's patch goes in, and then this one, that's
> why I prepared a single patch for the struct. (I mentioned it
> in the cover letter)
>
>>> Second, why is the whole struct pci_controller not surrounded
>>> by #ifdef CONFIG_ACPI as you are implying that this is needed
>>> only for ACPI?
>
>
> I hope it can be reused, since the NUMA node and segment (domain)
> is both needed for DT and ACPI, if it's not the case foe now, I
> can surrounded them all by #ifdef CONFIG_ACPI.
we can make use of this structure to hold pci to numa node
mapping(pcibus_to_node).
can you please pull node member out of  CONFIG_ACPI ifdef.
or you can put only acpi_device under ifdef.
>
>>>
>>> Btw, looking through the whole series I'm not (yet) convinced
>>> that this is needed at all.
>>
>> Hi Liviu,
>>         This structure is required by the requested patch set
>> at http://patchwork.ozlabs.org/patch/472249/, which consolidates
>> the common code to support PCI host bridge into ACPI core.
>
>
> Jiang, thanks for the explanation :)
>
> Thanks
> Hanjun

thanks
Ganapat
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lorenzo Pieralisi Sept. 7, 2015, 8:45 a.m. UTC | #7 
On Mon, Sep 07, 2015 at 05:14:22AM +0100, Ganapatrao Kulkarni wrote:
> Hi Hanjun,
> 
> On Wed, May 27, 2015 at 1:51 PM, Hanjun Guo <hanjun.guo@linaro.org> wrote:
> > Hi Liviu,
> >
> > On 2015???05???27??? 01:20, Jiang Liu wrote:
> >>
> >> On 2015/5/27 0:58, Liviu Dudau wrote:
> >>>
> >>> On Tue, May 26, 2015 at 01:49:14PM +0100, Hanjun Guo wrote:
> >>>>
> >>>> ARM64 ACPI based PCI host bridge init needs a arch dependent
> >>>> struct pci_controller to accommodate common PCI host bridge
> >>>> code which is introduced later, or it will lead to compile
> >>>> errors on ARM64.
> >>>
> >>>
> >>> Hi Hanjun,
> >>>
> >>> Two questions: why don't you introduce this patch next to the
> >>> one that is going to make use of it (or even merge it there)?
> >
> >
> > this is because of this patch is needed by Jiang Liu's patch set
> > to fix the compile error on ARM64, I'd rather do that, but It's
> > better to let Jiang Liu's patch goes in, and then this one, that's
> > why I prepared a single patch for the struct. (I mentioned it
> > in the cover letter)
> >
> >>> Second, why is the whole struct pci_controller not surrounded
> >>> by #ifdef CONFIG_ACPI as you are implying that this is needed
> >>> only for ACPI?
> >
> >
> > I hope it can be reused, since the NUMA node and segment (domain)
> > is both needed for DT and ACPI, if it's not the case foe now, I
> > can surrounded them all by #ifdef CONFIG_ACPI.
> we can make use of this structure to hold pci to numa node
> mapping(pcibus_to_node).
> can you please pull node member out of  CONFIG_ACPI ifdef.
> or you can put only acpi_device under ifdef.

That struct disappeared in the latest series:

https://lkml.org/lkml/2015/6/8/443

we have to have a common way to handle the NUMA info in DT and ACPI
so we should still find a solution that can be shared between the two,
it is yet another thing to take into account for PCI ACPI on arm64.

Thanks,
Lorenzo
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hanjun Guo Sept. 8, 2015, 1:35 p.m. UTC | #8 
On 09/07/2015 04:45 PM, Lorenzo Pieralisi wrote:
> On Mon, Sep 07, 2015 at 05:14:22AM +0100, Ganapatrao Kulkarni wrote:
>> Hi Hanjun,
>>
>> On Wed, May 27, 2015 at 1:51 PM, Hanjun Guo <hanjun.guo@linaro.org> wrote:
>>> Hi Liviu,
>>>
>>> On 2015???05???27??? 01:20, Jiang Liu wrote:
>>>>
>>>> On 2015/5/27 0:58, Liviu Dudau wrote:
>>>>>
>>>>> On Tue, May 26, 2015 at 01:49:14PM +0100, Hanjun Guo wrote:
>>>>>>
>>>>>> ARM64 ACPI based PCI host bridge init needs a arch dependent
>>>>>> struct pci_controller to accommodate common PCI host bridge
>>>>>> code which is introduced later, or it will lead to compile
>>>>>> errors on ARM64.
>>>>>
>>>>>
>>>>> Hi Hanjun,
>>>>>
>>>>> Two questions: why don't you introduce this patch next to the
>>>>> one that is going to make use of it (or even merge it there)?
>>>
>>>
>>> this is because of this patch is needed by Jiang Liu's patch set
>>> to fix the compile error on ARM64, I'd rather do that, but It's
>>> better to let Jiang Liu's patch goes in, and then this one, that's
>>> why I prepared a single patch for the struct. (I mentioned it
>>> in the cover letter)
>>>
>>>>> Second, why is the whole struct pci_controller not surrounded
>>>>> by #ifdef CONFIG_ACPI as you are implying that this is needed
>>>>> only for ACPI?
>>>
>>>
>>> I hope it can be reused, since the NUMA node and segment (domain)
>>> is both needed for DT and ACPI, if it's not the case foe now, I
>>> can surrounded them all by #ifdef CONFIG_ACPI.
>> we can make use of this structure to hold pci to numa node
>> mapping(pcibus_to_node).
>> can you please pull node member out of  CONFIG_ACPI ifdef.
>> or you can put only acpi_device under ifdef.
>
> That struct disappeared in the latest series:
>
> https://lkml.org/lkml/2015/6/8/443

Yes, I think that is the right direction going.

>
> we have to have a common way to handle the NUMA info in DT and ACPI
> so we should still find a solution that can be shared between the two,
> it is yet another thing to take into account for PCI ACPI on arm64.

Agreed, we can take that into account when finished the basic
support.

Thanks
Hanjun
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/arch/arm64/include/asm/pci.h b/arch/arm64/include/asm/pci.h
index b008a72..7088495 100644
--- a/arch/arm64/include/asm/pci.h
+++ b/arch/arm64/include/asm/pci.h
@@ -10,6 +10,16 @@ 
 #include <asm-generic/pci-bridge.h>
 #include <asm-generic/pci-dma-compat.h>
 
+struct acpi_device;
+
+struct pci_controller {
+#ifdef CONFIG_ACPI
+	struct acpi_device *companion;	/* ACPI companion device */
+#endif
+	int		segment;	/* PCI domain */
+	int		node;		/* NUMA node */
+};
+
 #define PCIBIOS_MIN_IO		0x1000
 #define PCIBIOS_MIN_MEM		0