From: Paul Gortmaker <hidden> Date: 2012-04-02 23:48:25
commit 9f786d033d025ab7d2c4d1b959aa81d935eb9e19
"arm/PCI: get rid of device resource fixups"
causes this failure on the versatile:
arch/arm/mach-versatile/pci.c: In function 'pci_versatile_setup_resources':
arch/arm/mach-versatile/pci.c:221: error: 'sys' undeclared (first use in this function)
because the versatile wasn't passing in the full struct pci_sys_data
but only the resource sub-field. Change it to pass in the full
struct so that sys will be in scope.
Reported-by: Bruce Ashfield <redacted>
Signed-off-by: Paul Gortmaker <redacted>
Hi Paul.
On Mon, Apr 2, 2012 at 8:48 PM, Paul Gortmaker
[off-list ref] wrote:
commit 9f786d033d025ab7d2c4d1b959aa81d935eb9e19
? ?"arm/PCI: get rid of device resource fixups"
causes this failure on the versatile:
arch/arm/mach-versatile/pci.c: In function 'pci_versatile_setup_resources':
arch/arm/mach-versatile/pci.c:221: error: 'sys' undeclared (first use in this function)
because the versatile wasn't passing in the full struct pci_sys_data
but only the resource sub-field. ?Change it to pass in the full
struct so that sys will be in scope.
Reported-by: Bruce Ashfield <redacted>
Signed-off-by: Paul Gortmaker <redacted>
From: Paul Gortmaker <hidden> Date: 2012-04-03 01:01:40
On Mon, Apr 2, 2012 at 8:03 PM, Fabio Estevam [off-list ref] wrote:
Hi Paul.
On Mon, Apr 2, 2012 at 8:48 PM, Paul Gortmaker
[off-list ref] wrote:
quoted
commit 9f786d033d025ab7d2c4d1b959aa81d935eb9e19
? ?"arm/PCI: get rid of device resource fixups"
causes this failure on the versatile:
arch/arm/mach-versatile/pci.c: In function 'pci_versatile_setup_resources':
arch/arm/mach-versatile/pci.c:221: error: 'sys' undeclared (first use in this function)
because the versatile wasn't passing in the full struct pci_sys_data
but only the resource sub-field. ?Change it to pass in the full
struct so that sys will be in scope.
Reported-by: Bruce Ashfield <redacted>
Signed-off-by: Paul Gortmaker <redacted>
I see. Well, there weren't any follow ups to it but I would say that
passing both sys->resources and sys both side-by-side at the same
time doesn't really make sense. I think just passing in sys by itself
as I did is a cleaner way to go, wouldn't you agree?
Thanks,
Paul.
On Mon, Apr 2, 2012 at 10:01 PM, Paul Gortmaker
[off-list ref] wrote:
I see. ?Well, there weren't any follow ups to it but I would say that
passing both sys->resources and sys both side-by-side at the same
time doesn't really make sense. ?I think just passing in sys by itself
as I did is a cleaner way to go, wouldn't you agree?
From: Olof Johansson <hidden> Date: 2012-04-03 04:58:25
On Mon, Apr 02, 2012 at 07:48:25PM -0400, Paul Gortmaker wrote:
commit 9f786d033d025ab7d2c4d1b959aa81d935eb9e19
"arm/PCI: get rid of device resource fixups"
causes this failure on the versatile:
arch/arm/mach-versatile/pci.c: In function 'pci_versatile_setup_resources':
arch/arm/mach-versatile/pci.c:221: error: 'sys' undeclared (first use in this function)
because the versatile wasn't passing in the full struct pci_sys_data
but only the resource sub-field. Change it to pass in the full
struct so that sys will be in scope.
Reported-by: Bruce Ashfield <redacted>
Signed-off-by: Paul Gortmaker <redacted>
On Mon, Apr 2, 2012 at 10:01 PM, Paul Gortmaker
[off-list ref] wrote:
quoted
I see. Well, there weren't any follow ups to it but I would say that
passing both sys->resources and sys both side-by-side at the same
time doesn't really make sense. I think just passing in sys by itself
as I did is a cleaner way to go, wouldn't you agree?
Yes, I agree.
But that doesn't work: struct pci_sys_data is ARM specific, while
pci_add_resource_offset is common code.
I think it would be much more sensible here to just use pci_add_resource(),
which is the same as the offsets are all zero for versatile.
Arnd
From: Paul Gortmaker <hidden> Date: 2012-04-03 13:47:13
[adding Bjorn Helgaas to CC; I overlooked doing that earlier]
On 12-04-03 04:19 AM, Arnd Bergmann wrote:
On Tuesday 03 April 2012, Fabio Estevam wrote:
quoted
On Mon, Apr 2, 2012 at 10:01 PM, Paul Gortmaker
[off-list ref] wrote:
quoted
I see. Well, there weren't any follow ups to it but I would say that
passing both sys->resources and sys both side-by-side at the same
time doesn't really make sense. I think just passing in sys by itself
as I did is a cleaner way to go, wouldn't you agree?
Yes, I agree.
But that doesn't work: struct pci_sys_data is ARM specific, while
pci_add_resource_offset is common code.
I'm not sure I'm following you here. I'm not feeding the
struct pci_sys_data to pci_add_resource_offset. Instead it
is getting subfields of it, consistent with the rest of the
original commit from Bjorn.
- pci_add_resource_offset(resources, &io_mem, sys->io_offset);
- pci_add_resource_offset(resources, &non_mem, sys->mem_offset);
- pci_add_resource_offset(resources, &pre_mem, sys->mem_offset);
+ pci_add_resource_offset(&sys->resources, &io_mem, sys->io_offset);
+ pci_add_resource_offset(&sys->resources, &non_mem, sys->mem_offset);
+ pci_add_resource_offset(&sys->resources, &pre_mem, sys->mem_offset);
As a side note, the fact that struct pci_sys_data is ARM specific
does make one think it would be nice if the name somehow did
reflect that... wonder if it is worth changing. Anyway, that is
a separate topic.
Thanks,
Paul.
--
I think it would be much more sensible here to just use pci_add_resource(),
which is the same as the offsets are all zero for versatile.
Arnd
I'm not sure I'm following you here. I'm not feeding the
struct pci_sys_data to pci_add_resource_offset. Instead it
is getting subfields of it, consistent with the rest of the
original commit from Bjorn.
Sorry, my fault, I thought the suggestion was to pass sys into
pci_add_resource_offset.
However, I would still prefer just reverting the versatile
part of Bjorn's patch because versatile uses a zero offset.
I think we should only pass the offset in cases where it's
actually required because there are multiple buses or they
have interesting mappings to the physical address space.
Those seem to be the minority on ARM anyway.
Arnd
On Tue, Apr 3, 2012 at 8:26 AM, Arnd Bergmann [off-list ref] wrote:
On Tuesday 03 April 2012, Paul Gortmaker wrote:
quoted
I'm not sure I'm following you here. I'm not feeding the
struct pci_sys_data to pci_add_resource_offset. ?Instead it
is getting subfields of it, consistent with the rest of the
original commit from Bjorn.
Sorry, my fault, I thought the suggestion was to pass sys into
pci_add_resource_offset.
However, I would still prefer just reverting the versatile
part of Bjorn's patch because versatile uses a zero offset.
I think we should only pass the offset in cases where it's
actually required because there are multiple buses or they
have interesting mappings to the physical address space.
I used pci_add_resource_offset() on *all* arm platforms because the
offsets are defined in struct pci_sys_data, which is not specific to
any platform type, and I thought it was easier and more future-proof
to pay attention to those offsets.
I was also thinking that the actual pci_add_resource_offset() calls
and the request_resource() or allocate_resource() calls usually found
near them in the arm .setup() functions are really not
platform-specific, so it might make sense to float them up into
something like pcibios_init_hw() someday.
But if you prefer to use the offset only for platforms that actually
assign non-zero values to the pci_sys_data offsets, that's fine with
me.
Since this only affects arm, do you want to handle the fix entirely in
the arm tree? If you want me to push something through PCI, I can do
that, too. Sorry for the breakage.
Bjorn
From: Paul Gortmaker <hidden> Date: 2012-04-03 19:09:36
On 12-04-03 12:44 PM, Bjorn Helgaas wrote:
[...]
Since this only affects arm, do you want to handle the fix entirely in
the arm tree? If you want me to push something through PCI, I can do
that, too. Sorry for the breakage.
Olof already applied my patch to fixes, so I don't
think there is any requirement for immediate action.
You and Arnd can work out whether you want to use
offsets or not on the versatile at your leisure.
Thanks,
Paul.
Since this only affects arm, do you want to handle the fix entirely in
the arm tree? If you want me to push something through PCI, I can do
that, too. Sorry for the breakage.
Olof already applied my patch to fixes, so I don't
think there is any requirement for immediate action.
Yes, and this is the important part for now, making this look nice can
wait. My preference would be to get rid of the io_offset and mem_offset
in as many places as possible in the long run, but we don't have to do
that for v3.4.
You and Arnd can work out whether you want to use
offsets or not on the versatile at your leisure.
I have a patch set for versatile PCI that I should have sent a long
time ago. When I resubmit that, I'll take care of this, too.
Arnd
From: Russell King - ARM Linux <hidden> Date: 2012-04-04 09:07:54
On Tue, Apr 03, 2012 at 09:47:13AM -0400, Paul Gortmaker wrote:
As a side note, the fact that struct pci_sys_data is ARM specific
does make one think it would be nice if the name somehow did
reflect that... wonder if it is worth changing. Anyway, that is
a separate topic.
And what's wrong with the existing name when it's limited to only
ARM specific files?
From: Russell King - ARM Linux <hidden> Date: 2012-04-04 09:08:44
On Tue, Apr 03, 2012 at 02:26:22PM +0000, Arnd Bergmann wrote:
On Tuesday 03 April 2012, Paul Gortmaker wrote:
quoted
I'm not sure I'm following you here. I'm not feeding the
struct pci_sys_data to pci_add_resource_offset. Instead it
is getting subfields of it, consistent with the rest of the
original commit from Bjorn.
Sorry, my fault, I thought the suggestion was to pass sys into
pci_add_resource_offset.
However, I would still prefer just reverting the versatile
part of Bjorn's patch because versatile uses a zero offset.
I think we should only pass the offset in cases where it's
actually required because there are multiple buses or they
have interesting mappings to the physical address space.
From: Paul Gortmaker <hidden> Date: 2012-04-04 13:53:08
On 12-04-04 05:07 AM, Russell King - ARM Linux wrote:
On Tue, Apr 03, 2012 at 09:47:13AM -0400, Paul Gortmaker wrote:
quoted
As a side note, the fact that struct pci_sys_data is ARM specific
does make one think it would be nice if the name somehow did
reflect that... wonder if it is worth changing. Anyway, that is
a separate topic.
And what's wrong with the existing name when it's limited to only
ARM specific files?
Just in the context of Arnd's original comment, he thought I
screwed up by feeding an ARM specific struct into generic
PCI code, because I wasn't aware it was ARM specific.
That wasn't the case, but he's right that something like that
could happen A minor nit, sure, but if it was something like
pci_arm_data, the name itself would convey it was arm specific,
even in reduced context scenarios (like grep output etc).
P.
From: Russell King - ARM Linux <hidden> Date: 2012-04-04 14:26:16
On Wed, Apr 04, 2012 at 09:53:08AM -0400, Paul Gortmaker wrote:
On 12-04-04 05:07 AM, Russell King - ARM Linux wrote:
quoted
On Tue, Apr 03, 2012 at 09:47:13AM -0400, Paul Gortmaker wrote:
quoted
As a side note, the fact that struct pci_sys_data is ARM specific
does make one think it would be nice if the name somehow did
reflect that... wonder if it is worth changing. Anyway, that is
a separate topic.
And what's wrong with the existing name when it's limited to only
ARM specific files?
Just in the context of Arnd's original comment, he thought I
screwed up by feeding an ARM specific struct into generic
PCI code, because I wasn't aware it was ARM specific.
That wasn't the case, but he's right that something like that
could happen A minor nit, sure, but if it was something like
pci_arm_data, the name itself would convey it was arm specific,
even in reduced context scenarios (like grep output etc).
If the struct ends up being fed into non-ARM code, then non-ARM code
isn't going to be able to dereference it because there isn't a definition
for it outside of arch/arm/.
The risk which exists is that generic code invents its own pci_sys_data.
At that point, we end up with two definitions for the struct, and the
compiler will error out.
We really don't need the churn of changing this. It ain't broken, so
don't try to fix it. Moreover, you'll only end up with conflicts to
deal with if the ixp2xxx stuff is removed (and other people will have
similar problems if they're dealing with code in this area too.)