* Re: address translation for PCIe-to-localbus bridge
@ 2013-11-06 18:03 ` Thomas Petazzoni
0 siblings, 0 replies; 44+ messages in thread
From: Thomas Petazzoni @ 2013-11-06 18:03 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Gerlando Falauto,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
Thierry Reding, Ezequiel Garcia, Lior Amsalem,
Gregory Clément
Dear Jason Gunthorpe,
(Adding a bunch of mvebu people in Cc)
On Wed, 6 Nov 2013 10:36:49 -0700, Jason Gunthorpe wrote:
> Thomas: There is one buglet here that I haven't had time to do
> anything about. Notice the DT is listing the PEX memory window in its
> ranges. I've done this for two reasons
> - The bootloader sets this address range up, so it is correct to
> include in the DT
The fact that the bootloader sets this MBus window is more-or-less
irrelevant because when the mvebu-mbus driver is initialized, it
completely clears *all* existing MBus windows:
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/bus/mvebu-mbus.c#n722
Therefore, when the kernel starts, what the bootloader may have set up
in terms of MBus windows is irrelevant.
> - The address translation machinery requires it, otherwise we can't
> translate addreses of the non-PCI sub devices (eg gpio3)
Right.
> The latter is a kernel issue. As we discussed when mbus was first put
> together something needs to make the ranges consistent with the actual
> mapping so that address translation works. IIRC people objected to
> actually changing the ranges at runtime, so the alternate mechanism of
> hooking the address translation seems necessary?
I indeed remember some objections, but I'm not sure what they were
precisely. Maybe we didn't had a precise use case back at the time, to
really make people objecting realize what the problem was?
On the other hand, I think the of_*() API is quite limited when it
comes to updating the DT. If I remember correctly, you can update some
nodes, but you can never reclaim the memory that was used for the
previous value of the node. So any change to the in-memory DT
representation is basically a memory leak for the entire lifetime of
the system (of course, I might be wrong on this, I haven't dived into
all the hardcore details of DT manipulation in the kernel).
I'm not sure what would be the alternate mechanism to hook into the
address translation. of_translate_one(), where all the translation
through ranges takes place is really tied to the DT only, adding
another mechanism to hook some custom address translation in there
seems a bit weird, no?
> Unfortunately if you add the ranges then the mbus driver throws a
> warning that it is trying to overwrite existing windows, but otherwise
> things work OK.
Yes, because at boot time the mvebu-mbus driver will set up windows for
the statically defined ranges (the one you've written explicitly in the
DT), and then later one when the PCIe driver will initialize, it will
enumerate devices, realize that it needs a PCIe memory window, and ask
the mvebu-mbus driver to create, which will fail because an overlapping
window already exists.
However, it just works by pure luck: nothing guarantees you that the
PCIe 0 memory window will start at 0xe0000000. Of course, since you
have only one PCIe interface enabled, you have a guarantee from one
boot to another. But in the general case, if you have several PCIe
interfaces, on which you may plug/unplug different devices, you have no
guarantee a priori as to what will be the base address of the PCIe
memory window for a given PCIe interface.
Best regards,
Thomas
--
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 44+ messages in thread
* address translation for PCIe-to-localbus bridge
2013-11-06 18:03 ` Thomas Petazzoni
@ 2013-11-06 18:24 ` Jason Gunthorpe
-1 siblings, 0 replies; 44+ messages in thread
From: Jason Gunthorpe @ 2013-11-06 18:24 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Nov 06, 2013 at 07:03:08PM +0100, Thomas Petazzoni wrote:
> Dear Jason Gunthorpe,
>
> (Adding a bunch of mvebu people in Cc)
>
> On Wed, 6 Nov 2013 10:36:49 -0700, Jason Gunthorpe wrote:
>
> > Thomas: There is one buglet here that I haven't had time to do
> > anything about. Notice the DT is listing the PEX memory window in its
> > ranges. I've done this for two reasons
> > - The bootloader sets this address range up, so it is correct to
> > include in the DT
>
> The fact that the bootloader sets this MBus window is more-or-less
> irrelevant because when the mvebu-mbus driver is initialized, it
> completely clears *all* existing MBus windows:
Yes, but it is not irrelevent. In my case this same DT is supporting
3.10 and 3.12 kernels - 3.10 doesn't even have a MBUS driver or
dynamic PEX driver and it relies upon the ranges entry matching the
static bootloader configuration to work properly.
So the 'ranges matching bootloader' does have a use case.
> I indeed remember some objections, but I'm not sure what they were
> precisely. Maybe we didn't had a precise use case back at the time, to
> really make people objecting realize what the problem was?
That is about where I am at too..
> On the other hand, I think the of_*() API is quite limited when it
> comes to updating the DT. If I remember correctly, you can update some
> nodes, but you can never reclaim the memory that was used for the
> previous value of the node. So any change to the in-memory DT
> representation is basically a memory leak for the entire lifetime of
> the system (of course, I might be wrong on this, I haven't dived into
> all the hardcore details of DT manipulation in the kernel).
I'm not clear on that either, but it seems plausible..
> I'm not sure what would be the alternate mechanism to hook into the
> address translation. of_translate_one(), where all the translation
> through ranges takes place is really tied to the DT only, adding
> another mechanism to hook some custom address translation in there
> seems a bit weird, no?
I agree, some kind of callback scheme would really be needed.. Sort of
like the xlate callback we have for IRQs?
So the mbus would register an address xlate for its node that is
called instead of ranges parsing. For the example in my last message
the FPGA driver would register an xlate that made addresses relative
to its own BAR0 address.
> > Unfortunately if you add the ranges then the mbus driver throws a
> > warning that it is trying to overwrite existing windows, but otherwise
> > things work OK.
>
> Yes, because at boot time the mvebu-mbus driver will set up windows for
> the statically defined ranges (the one you've written explicitly in the
> DT), and then later one when the PCIe driver will initialize, it will
> enumerate devices, realize that it needs a PCIe memory window, and ask
> the mvebu-mbus driver to create, which will fail because an overlapping
> window already exists.
Exactly.
> However, it just works by pure luck: nothing guarantees you that the
> PCIe 0 memory window will start at 0xe0000000. Of course, since you
Right, the general case is totally borked here - the dynamic changes
to the address map by MBUS and PCI are not being reflected to the
of_address machinery, be they from MBUS or from PCI BAR assignment.
However, address assignment is very predicatble, so if you have a
constrained system it can work.
In the normal DT use case (eg on a SPARC or something) the DT itself
would be self-consistent with the addresses assigned from the firmware
and the PCI machinery should try to respect the bootloader addresses
if they are workable during address assignment.
So it isn't as dire as 'pure luck' :)
Jason
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: address translation for PCIe-to-localbus bridge
@ 2013-11-06 18:24 ` Jason Gunthorpe
0 siblings, 0 replies; 44+ messages in thread
From: Jason Gunthorpe @ 2013-11-06 18:24 UTC (permalink / raw)
To: Thomas Petazzoni
Cc: Gerlando Falauto,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
Thierry Reding, Ezequiel Garcia, Lior Amsalem, Gregory Cl??ment
On Wed, Nov 06, 2013 at 07:03:08PM +0100, Thomas Petazzoni wrote:
> Dear Jason Gunthorpe,
>
> (Adding a bunch of mvebu people in Cc)
>
> On Wed, 6 Nov 2013 10:36:49 -0700, Jason Gunthorpe wrote:
>
> > Thomas: There is one buglet here that I haven't had time to do
> > anything about. Notice the DT is listing the PEX memory window in its
> > ranges. I've done this for two reasons
> > - The bootloader sets this address range up, so it is correct to
> > include in the DT
>
> The fact that the bootloader sets this MBus window is more-or-less
> irrelevant because when the mvebu-mbus driver is initialized, it
> completely clears *all* existing MBus windows:
Yes, but it is not irrelevent. In my case this same DT is supporting
3.10 and 3.12 kernels - 3.10 doesn't even have a MBUS driver or
dynamic PEX driver and it relies upon the ranges entry matching the
static bootloader configuration to work properly.
So the 'ranges matching bootloader' does have a use case.
> I indeed remember some objections, but I'm not sure what they were
> precisely. Maybe we didn't had a precise use case back at the time, to
> really make people objecting realize what the problem was?
That is about where I am at too..
> On the other hand, I think the of_*() API is quite limited when it
> comes to updating the DT. If I remember correctly, you can update some
> nodes, but you can never reclaim the memory that was used for the
> previous value of the node. So any change to the in-memory DT
> representation is basically a memory leak for the entire lifetime of
> the system (of course, I might be wrong on this, I haven't dived into
> all the hardcore details of DT manipulation in the kernel).
I'm not clear on that either, but it seems plausible..
> I'm not sure what would be the alternate mechanism to hook into the
> address translation. of_translate_one(), where all the translation
> through ranges takes place is really tied to the DT only, adding
> another mechanism to hook some custom address translation in there
> seems a bit weird, no?
I agree, some kind of callback scheme would really be needed.. Sort of
like the xlate callback we have for IRQs?
So the mbus would register an address xlate for its node that is
called instead of ranges parsing. For the example in my last message
the FPGA driver would register an xlate that made addresses relative
to its own BAR0 address.
> > Unfortunately if you add the ranges then the mbus driver throws a
> > warning that it is trying to overwrite existing windows, but otherwise
> > things work OK.
>
> Yes, because at boot time the mvebu-mbus driver will set up windows for
> the statically defined ranges (the one you've written explicitly in the
> DT), and then later one when the PCIe driver will initialize, it will
> enumerate devices, realize that it needs a PCIe memory window, and ask
> the mvebu-mbus driver to create, which will fail because an overlapping
> window already exists.
Exactly.
> However, it just works by pure luck: nothing guarantees you that the
> PCIe 0 memory window will start at 0xe0000000. Of course, since you
Right, the general case is totally borked here - the dynamic changes
to the address map by MBUS and PCI are not being reflected to the
of_address machinery, be they from MBUS or from PCI BAR assignment.
However, address assignment is very predicatble, so if you have a
constrained system it can work.
In the normal DT use case (eg on a SPARC or something) the DT itself
would be self-consistent with the addresses assigned from the firmware
and the PCI machinery should try to respect the bootloader addresses
if they are workable during address assignment.
So it isn't as dire as 'pure luck' :)
Jason
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 44+ messages in thread
* address translation for PCIe-to-localbus bridge
@ 2013-11-06 19:00 ` Thomas Petazzoni
0 siblings, 0 replies; 44+ messages in thread
From: Thomas Petazzoni @ 2013-11-06 19:00 UTC (permalink / raw)
To: linux-arm-kernel
Dear Jason Gunthorpe,
On Wed, 6 Nov 2013 11:24:57 -0700, Jason Gunthorpe wrote:
> > The fact that the bootloader sets this MBus window is more-or-less
> > irrelevant because when the mvebu-mbus driver is initialized, it
> > completely clears *all* existing MBus windows:
>
> Yes, but it is not irrelevent. In my case this same DT is supporting
> 3.10 and 3.12 kernels - 3.10 doesn't even have a MBUS driver or
> dynamic PEX driver and it relies upon the ranges entry matching the
> static bootloader configuration to work properly.
Ok, I understand.
> > I indeed remember some objections, but I'm not sure what they were
> > precisely. Maybe we didn't had a precise use case back at the time, to
> > really make people objecting realize what the problem was?
>
> That is about where I am at too..
So maybe we should revive this direction then?
> > On the other hand, I think the of_*() API is quite limited when it
> > comes to updating the DT. If I remember correctly, you can update some
> > nodes, but you can never reclaim the memory that was used for the
> > previous value of the node. So any change to the in-memory DT
> > representation is basically a memory leak for the entire lifetime of
> > the system (of course, I might be wrong on this, I haven't dived into
> > all the hardcore details of DT manipulation in the kernel).
>
> I'm not clear on that either, but it seems plausible..
>
> > I'm not sure what would be the alternate mechanism to hook into the
> > address translation. of_translate_one(), where all the translation
> > through ranges takes place is really tied to the DT only, adding
> > another mechanism to hook some custom address translation in there
> > seems a bit weird, no?
>
> I agree, some kind of callback scheme would really be needed.. Sort of
> like the xlate callback we have for IRQs?
>
> So the mbus would register an address xlate for its node that is
> called instead of ranges parsing. For the example in my last message
> the FPGA driver would register an xlate that made addresses relative
> to its own BAR0 address.
Interesting. I admit I never had a look how the IRQ ->xlate() stuff was
hooked into the OF code, maybe it's a good starting point. This
discussion is on the DT mailing list, so maybe some DT person will
agree to let us know what they think the good approach is?
> > However, it just works by pure luck: nothing guarantees you that the
> > PCIe 0 memory window will start at 0xe0000000. Of course, since you
>
> Right, the general case is totally borked here - the dynamic changes
> to the address map by MBUS and PCI are not being reflected to the
> of_address machinery, be they from MBUS or from PCI BAR assignment.
Right.
> However, address assignment is very predicatble, so if you have a
> constrained system it can work.
Indeed.
> In the normal DT use case (eg on a SPARC or something) the DT itself
> would be self-consistent with the addresses assigned from the firmware
> and the PCI machinery should try to respect the bootloader addresses
> if they are workable during address assignment.
So ARM isn't a "normal DT use case" ? :-)
Thomas
--
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: address translation for PCIe-to-localbus bridge
@ 2013-11-06 19:00 ` Thomas Petazzoni
0 siblings, 0 replies; 44+ messages in thread
From: Thomas Petazzoni @ 2013-11-06 19:00 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Gerlando Falauto,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
Thierry Reding, Ezequiel Garcia, Lior Amsalem, Gregory Cl??ment
Dear Jason Gunthorpe,
On Wed, 6 Nov 2013 11:24:57 -0700, Jason Gunthorpe wrote:
> > The fact that the bootloader sets this MBus window is more-or-less
> > irrelevant because when the mvebu-mbus driver is initialized, it
> > completely clears *all* existing MBus windows:
>
> Yes, but it is not irrelevent. In my case this same DT is supporting
> 3.10 and 3.12 kernels - 3.10 doesn't even have a MBUS driver or
> dynamic PEX driver and it relies upon the ranges entry matching the
> static bootloader configuration to work properly.
Ok, I understand.
> > I indeed remember some objections, but I'm not sure what they were
> > precisely. Maybe we didn't had a precise use case back at the time, to
> > really make people objecting realize what the problem was?
>
> That is about where I am at too..
So maybe we should revive this direction then?
> > On the other hand, I think the of_*() API is quite limited when it
> > comes to updating the DT. If I remember correctly, you can update some
> > nodes, but you can never reclaim the memory that was used for the
> > previous value of the node. So any change to the in-memory DT
> > representation is basically a memory leak for the entire lifetime of
> > the system (of course, I might be wrong on this, I haven't dived into
> > all the hardcore details of DT manipulation in the kernel).
>
> I'm not clear on that either, but it seems plausible..
>
> > I'm not sure what would be the alternate mechanism to hook into the
> > address translation. of_translate_one(), where all the translation
> > through ranges takes place is really tied to the DT only, adding
> > another mechanism to hook some custom address translation in there
> > seems a bit weird, no?
>
> I agree, some kind of callback scheme would really be needed.. Sort of
> like the xlate callback we have for IRQs?
>
> So the mbus would register an address xlate for its node that is
> called instead of ranges parsing. For the example in my last message
> the FPGA driver would register an xlate that made addresses relative
> to its own BAR0 address.
Interesting. I admit I never had a look how the IRQ ->xlate() stuff was
hooked into the OF code, maybe it's a good starting point. This
discussion is on the DT mailing list, so maybe some DT person will
agree to let us know what they think the good approach is?
> > However, it just works by pure luck: nothing guarantees you that the
> > PCIe 0 memory window will start at 0xe0000000. Of course, since you
>
> Right, the general case is totally borked here - the dynamic changes
> to the address map by MBUS and PCI are not being reflected to the
> of_address machinery, be they from MBUS or from PCI BAR assignment.
Right.
> However, address assignment is very predicatble, so if you have a
> constrained system it can work.
Indeed.
> In the normal DT use case (eg on a SPARC or something) the DT itself
> would be self-consistent with the addresses assigned from the firmware
> and the PCI machinery should try to respect the bootloader addresses
> if they are workable during address assignment.
So ARM isn't a "normal DT use case" ? :-)
Thomas
--
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 44+ messages in thread
* address translation for PCIe-to-localbus bridge
@ 2013-11-11 15:50 ` Grant Likely
0 siblings, 0 replies; 44+ messages in thread
From: Grant Likely @ 2013-11-11 15:50 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, 6 Nov 2013 11:24:57 -0700, Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote:
> On Wed, Nov 06, 2013 at 07:03:08PM +0100, Thomas Petazzoni wrote:
> > Dear Jason Gunthorpe,
> >
> > (Adding a bunch of mvebu people in Cc)
> >
> > On Wed, 6 Nov 2013 10:36:49 -0700, Jason Gunthorpe wrote:
> >
> > > Thomas: There is one buglet here that I haven't had time to do
> > > anything about. Notice the DT is listing the PEX memory window in its
> > > ranges. I've done this for two reasons
> > > - The bootloader sets this address range up, so it is correct to
> > > include in the DT
> >
> > The fact that the bootloader sets this MBus window is more-or-less
> > irrelevant because when the mvebu-mbus driver is initialized, it
> > completely clears *all* existing MBus windows:
>
> Yes, but it is not irrelevent. In my case this same DT is supporting
> 3.10 and 3.12 kernels - 3.10 doesn't even have a MBUS driver or
> dynamic PEX driver and it relies upon the ranges entry matching the
> static bootloader configuration to work properly.
>
> So the 'ranges matching bootloader' does have a use case.
>
> > I indeed remember some objections, but I'm not sure what they were
> > precisely. Maybe we didn't had a precise use case back at the time, to
> > really make people objecting realize what the problem was?
>
> That is about where I am at too..
>
> > On the other hand, I think the of_*() API is quite limited when it
> > comes to updating the DT. If I remember correctly, you can update some
> > nodes, but you can never reclaim the memory that was used for the
> > previous value of the node. So any change to the in-memory DT
> > representation is basically a memory leak for the entire lifetime of
> > the system (of course, I might be wrong on this, I haven't dived into
> > all the hardcore details of DT manipulation in the kernel).
>
> I'm not clear on that either, but it seems plausible..
>
> > I'm not sure what would be the alternate mechanism to hook into the
> > address translation. of_translate_one(), where all the translation
> > through ranges takes place is really tied to the DT only, adding
> > another mechanism to hook some custom address translation in there
> > seems a bit weird, no?
>
> I agree, some kind of callback scheme would really be needed.. Sort of
> like the xlate callback we have for IRQs?
>
> So the mbus would register an address xlate for its node that is
> called instead of ranges parsing. For the example in my last message
> the FPGA driver would register an xlate that made addresses relative
> to its own BAR0 address.
There are already bus-specific transations available. Take a look at
struct of_bus in drivers/of/address.c
g.
> > However, it just works by pure luck: nothing guarantees you that the
> > PCIe 0 memory window will start at 0xe0000000. Of course, since you
>
> Right, the general case is totally borked here - the dynamic changes
> to the address map by MBUS and PCI are not being reflected to the
> of_address machinery, be they from MBUS or from PCI BAR assignment.
>
> However, address assignment is very predicatble, so if you have a
> constrained system it can work.
Yes, the problem is that the DT probably can't have an accurate set of
ranges for the devices under the PCIe bus since they are dynamically
assigned. It would be really nice to wire this up properly so that the
DT states the layout of stuff behind the bar, but leaves it to the OS to
assign PCI regions.
However, if firmware enumarates PCI and wants that to be passed through
to the OS, then the DT really should be updated... even in that case
though Linux should be able to pick up the PCI settings from the BAR
registers and use those for dynamics translation.
g.
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: address translation for PCIe-to-localbus bridge
@ 2013-11-11 15:50 ` Grant Likely
0 siblings, 0 replies; 44+ messages in thread
From: Grant Likely @ 2013-11-11 15:50 UTC (permalink / raw)
To: Jason Gunthorpe, Thomas Petazzoni
Cc: Lior Amsalem, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Thierry Reding, Gerlando Falauto, Ezequiel Garcia,
Gregory Cl??ment,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
On Wed, 6 Nov 2013 11:24:57 -0700, Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> wrote:
> On Wed, Nov 06, 2013 at 07:03:08PM +0100, Thomas Petazzoni wrote:
> > Dear Jason Gunthorpe,
> >
> > (Adding a bunch of mvebu people in Cc)
> >
> > On Wed, 6 Nov 2013 10:36:49 -0700, Jason Gunthorpe wrote:
> >
> > > Thomas: There is one buglet here that I haven't had time to do
> > > anything about. Notice the DT is listing the PEX memory window in its
> > > ranges. I've done this for two reasons
> > > - The bootloader sets this address range up, so it is correct to
> > > include in the DT
> >
> > The fact that the bootloader sets this MBus window is more-or-less
> > irrelevant because when the mvebu-mbus driver is initialized, it
> > completely clears *all* existing MBus windows:
>
> Yes, but it is not irrelevent. In my case this same DT is supporting
> 3.10 and 3.12 kernels - 3.10 doesn't even have a MBUS driver or
> dynamic PEX driver and it relies upon the ranges entry matching the
> static bootloader configuration to work properly.
>
> So the 'ranges matching bootloader' does have a use case.
>
> > I indeed remember some objections, but I'm not sure what they were
> > precisely. Maybe we didn't had a precise use case back at the time, to
> > really make people objecting realize what the problem was?
>
> That is about where I am at too..
>
> > On the other hand, I think the of_*() API is quite limited when it
> > comes to updating the DT. If I remember correctly, you can update some
> > nodes, but you can never reclaim the memory that was used for the
> > previous value of the node. So any change to the in-memory DT
> > representation is basically a memory leak for the entire lifetime of
> > the system (of course, I might be wrong on this, I haven't dived into
> > all the hardcore details of DT manipulation in the kernel).
>
> I'm not clear on that either, but it seems plausible..
>
> > I'm not sure what would be the alternate mechanism to hook into the
> > address translation. of_translate_one(), where all the translation
> > through ranges takes place is really tied to the DT only, adding
> > another mechanism to hook some custom address translation in there
> > seems a bit weird, no?
>
> I agree, some kind of callback scheme would really be needed.. Sort of
> like the xlate callback we have for IRQs?
>
> So the mbus would register an address xlate for its node that is
> called instead of ranges parsing. For the example in my last message
> the FPGA driver would register an xlate that made addresses relative
> to its own BAR0 address.
There are already bus-specific transations available. Take a look at
struct of_bus in drivers/of/address.c
g.
> > However, it just works by pure luck: nothing guarantees you that the
> > PCIe 0 memory window will start at 0xe0000000. Of course, since you
>
> Right, the general case is totally borked here - the dynamic changes
> to the address map by MBUS and PCI are not being reflected to the
> of_address machinery, be they from MBUS or from PCI BAR assignment.
>
> However, address assignment is very predicatble, so if you have a
> constrained system it can work.
Yes, the problem is that the DT probably can't have an accurate set of
ranges for the devices under the PCIe bus since they are dynamically
assigned. It would be really nice to wire this up properly so that the
DT states the layout of stuff behind the bar, but leaves it to the OS to
assign PCI regions.
However, if firmware enumarates PCI and wants that to be passed through
to the OS, then the DT really should be updated... even in that case
though Linux should be able to pick up the PCI settings from the BAR
registers and use those for dynamics translation.
g.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 44+ messages in thread
* address translation for PCIe-to-localbus bridge
@ 2013-11-12 7:05 ` Thomas Petazzoni
0 siblings, 0 replies; 44+ messages in thread
From: Thomas Petazzoni @ 2013-11-12 7:05 UTC (permalink / raw)
To: linux-arm-kernel
Dear Grant Likely,
On Mon, 11 Nov 2013 15:50:50 +0000, Grant Likely wrote:
> > So the mbus would register an address xlate for its node that is
> > called instead of ranges parsing. For the example in my last message
> > the FPGA driver would register an xlate that made addresses relative
> > to its own BAR0 address.
>
> There are already bus-specific transations available. Take a look at
> struct of_bus in drivers/of/address.c
Hum, right, but unless I'm wrong the of_busses[] array of struct of_bus
is fixed in drivers/of/address.c, and as it is, there is no way for a
specific bus driver to provide its own struct of_bus. So that would
need to be extended, right?
Thomas
--
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: address translation for PCIe-to-localbus bridge
@ 2013-11-12 7:05 ` Thomas Petazzoni
0 siblings, 0 replies; 44+ messages in thread
From: Thomas Petazzoni @ 2013-11-12 7:05 UTC (permalink / raw)
To: Grant Likely
Cc: Jason Gunthorpe, Lior Amsalem,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Thierry Reding, Gerlando Falauto, Ezequiel Garcia,
Gregory Cl??ment,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
Dear Grant Likely,
On Mon, 11 Nov 2013 15:50:50 +0000, Grant Likely wrote:
> > So the mbus would register an address xlate for its node that is
> > called instead of ranges parsing. For the example in my last message
> > the FPGA driver would register an xlate that made addresses relative
> > to its own BAR0 address.
>
> There are already bus-specific transations available. Take a look at
> struct of_bus in drivers/of/address.c
Hum, right, but unless I'm wrong the of_busses[] array of struct of_bus
is fixed in drivers/of/address.c, and as it is, there is no way for a
specific bus driver to provide its own struct of_bus. So that would
need to be extended, right?
Thomas
--
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 44+ messages in thread
* address translation for PCIe-to-localbus bridge
2013-11-12 7:05 ` Thomas Petazzoni
@ 2013-11-12 8:11 ` Gerlando Falauto
-1 siblings, 0 replies; 44+ messages in thread
From: Gerlando Falauto @ 2013-11-12 8:11 UTC (permalink / raw)
To: linux-arm-kernel
On 11/12/2013 08:05 AM, Thomas Petazzoni wrote:
> Dear Grant Likely,
>
> On Mon, 11 Nov 2013 15:50:50 +0000, Grant Likely wrote:
>
>>> So the mbus would register an address xlate for its node that is
>>> called instead of ranges parsing. For the example in my last message
>>> the FPGA driver would register an xlate that made addresses relative
>>> to its own BAR0 address.
>>
>> There are already bus-specific transations available. Take a look at
>> struct of_bus in drivers/of/address.c
>
> Hum, right, but unless I'm wrong the of_busses[] array of struct of_bus
> is fixed in drivers/of/address.c, and as it is, there is no way for a
> specific bus driver to provide its own struct of_bus.
That was exactly my understanding as well, and that's where I was
expecting some trickery to happen.
> So that would need to be extended, right?
The other approach I was foreseeing was to implement a way of
dynamically updating the DT when the PCI subsystem enumerates the
devices and assigns memory areas (namely, I would expect the ranges
property to reflect that). But this would imply a standard way of
defining the ranges property (I would expect something like <bar#
start_addr length>, with an arguable number of cells). And I could
not find any such definition in the PCI bus binding document, so I'm
probably completely off-track here. Aren't I?
After all, we should expect a driver to behave (and expect) the same of
the DT, regardless of whether enumeration was performed by firmware or
by the OS itself. Or am I wrong on this too?
Thanks guys!
Gerlando
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: address translation for PCIe-to-localbus bridge
@ 2013-11-12 8:11 ` Gerlando Falauto
0 siblings, 0 replies; 44+ messages in thread
From: Gerlando Falauto @ 2013-11-12 8:11 UTC (permalink / raw)
To: Thomas Petazzoni
Cc: Grant Likely, Jason Gunthorpe, Lior Amsalem,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Thierry Reding, Ezequiel Garcia, Gregory Cl??ment,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
On 11/12/2013 08:05 AM, Thomas Petazzoni wrote:
> Dear Grant Likely,
>
> On Mon, 11 Nov 2013 15:50:50 +0000, Grant Likely wrote:
>
>>> So the mbus would register an address xlate for its node that is
>>> called instead of ranges parsing. For the example in my last message
>>> the FPGA driver would register an xlate that made addresses relative
>>> to its own BAR0 address.
>>
>> There are already bus-specific transations available. Take a look at
>> struct of_bus in drivers/of/address.c
>
> Hum, right, but unless I'm wrong the of_busses[] array of struct of_bus
> is fixed in drivers/of/address.c, and as it is, there is no way for a
> specific bus driver to provide its own struct of_bus.
That was exactly my understanding as well, and that's where I was
expecting some trickery to happen.
> So that would need to be extended, right?
The other approach I was foreseeing was to implement a way of
dynamically updating the DT when the PCI subsystem enumerates the
devices and assigns memory areas (namely, I would expect the ranges
property to reflect that). But this would imply a standard way of
defining the ranges property (I would expect something like <bar#
start_addr length>, with an arguable number of cells). And I could
not find any such definition in the PCI bus binding document, so I'm
probably completely off-track here. Aren't I?
After all, we should expect a driver to behave (and expect) the same of
the DT, regardless of whether enumeration was performed by firmware or
by the OS itself. Or am I wrong on this too?
Thanks guys!
Gerlando
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 44+ messages in thread
* address translation for PCIe-to-localbus bridge
@ 2013-11-12 8:16 ` Thomas Petazzoni
0 siblings, 0 replies; 44+ messages in thread
From: Thomas Petazzoni @ 2013-11-12 8:16 UTC (permalink / raw)
To: linux-arm-kernel
Dear Gerlando Falauto,
On Tue, 12 Nov 2013 09:11:33 +0100, Gerlando Falauto wrote:
> > Hum, right, but unless I'm wrong the of_busses[] array of struct of_bus
> > is fixed in drivers/of/address.c, and as it is, there is no way for a
> > specific bus driver to provide its own struct of_bus.
>
> That was exactly my understanding as well, and that's where I was
> expecting some trickery to happen.
>
> > So that would need to be extended, right?
>
> The other approach I was foreseeing was to implement a way of
> dynamically updating the DT when the PCI subsystem enumerates the
> devices and assigns memory areas (namely, I would expect the ranges
> property to reflect that). But this would imply a standard way of
> defining the ranges property (I would expect something like <bar#
> start_addr length>, with an arguable number of cells). And I could
> not find any such definition in the PCI bus binding document, so I'm
> probably completely off-track here. Aren't I?
>
> After all, we should expect a driver to behave (and expect) the same of
> the DT, regardless of whether enumeration was performed by firmware or
> by the OS itself. Or am I wrong on this too?
Well, in the context of the mvebu platforms (including Kirkwood), the
problem is not so much the ranges in the pcie-controller node, but the
ranges in the main soc { ... } node which encloses the description of
the MBus. It is those ranges that need to be updated when a new window
is created, or a window is removed. So the problem is not PCI related,
but MBus related in this case, no?
Best regards,
Thomas
--
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: address translation for PCIe-to-localbus bridge
@ 2013-11-12 8:16 ` Thomas Petazzoni
0 siblings, 0 replies; 44+ messages in thread
From: Thomas Petazzoni @ 2013-11-12 8:16 UTC (permalink / raw)
To: Gerlando Falauto
Cc: Grant Likely, Jason Gunthorpe, Lior Amsalem,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Thierry Reding, Ezequiel Garcia, Gregory Cl??ment,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
Dear Gerlando Falauto,
On Tue, 12 Nov 2013 09:11:33 +0100, Gerlando Falauto wrote:
> > Hum, right, but unless I'm wrong the of_busses[] array of struct of_bus
> > is fixed in drivers/of/address.c, and as it is, there is no way for a
> > specific bus driver to provide its own struct of_bus.
>
> That was exactly my understanding as well, and that's where I was
> expecting some trickery to happen.
>
> > So that would need to be extended, right?
>
> The other approach I was foreseeing was to implement a way of
> dynamically updating the DT when the PCI subsystem enumerates the
> devices and assigns memory areas (namely, I would expect the ranges
> property to reflect that). But this would imply a standard way of
> defining the ranges property (I would expect something like <bar#
> start_addr length>, with an arguable number of cells). And I could
> not find any such definition in the PCI bus binding document, so I'm
> probably completely off-track here. Aren't I?
>
> After all, we should expect a driver to behave (and expect) the same of
> the DT, regardless of whether enumeration was performed by firmware or
> by the OS itself. Or am I wrong on this too?
Well, in the context of the mvebu platforms (including Kirkwood), the
problem is not so much the ranges in the pcie-controller node, but the
ranges in the main soc { ... } node which encloses the description of
the MBus. It is those ranges that need to be updated when a new window
is created, or a window is removed. So the problem is not PCI related,
but MBus related in this case, no?
Best regards,
Thomas
--
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 44+ messages in thread* address translation for PCIe-to-localbus bridge
2013-11-12 8:16 ` Thomas Petazzoni
@ 2013-11-12 8:26 ` Gerlando Falauto
-1 siblings, 0 replies; 44+ messages in thread
From: Gerlando Falauto @ 2013-11-12 8:26 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
On 11/12/2013 09:16 AM, Thomas Petazzoni wrote:
> Dear Gerlando Falauto,
>
> On Tue, 12 Nov 2013 09:11:33 +0100, Gerlando Falauto wrote:
>
>>> Hum, right, but unless I'm wrong the of_busses[] array of struct of_bus
>>> is fixed in drivers/of/address.c, and as it is, there is no way for a
>>> specific bus driver to provide its own struct of_bus.
>>
>> That was exactly my understanding as well, and that's where I was
>> expecting some trickery to happen.
>>
>>> So that would need to be extended, right?
>>
>> The other approach I was foreseeing was to implement a way of
>> dynamically updating the DT when the PCI subsystem enumerates the
>> devices and assigns memory areas (namely, I would expect the ranges
>> property to reflect that). But this would imply a standard way of
>> defining the ranges property (I would expect something like <bar#
>> start_addr length>, with an arguable number of cells). And I could
>> not find any such definition in the PCI bus binding document, so I'm
>> probably completely off-track here. Aren't I?
>>
>> After all, we should expect a driver to behave (and expect) the same of
>> the DT, regardless of whether enumeration was performed by firmware or
>> by the OS itself. Or am I wrong on this too?
>
> Well, in the context of the mvebu platforms (including Kirkwood), the
> problem is not so much the ranges in the pcie-controller node, but the
> ranges in the main soc { ... } node which encloses the description of
> the MBus. It is those ranges that need to be updated when a new window
> is created, or a window is removed. So the problem is not PCI related,
> but MBus related in this case, no?
I was actually referring to ranges within the PCI *device* (~leaf node).
So not thinking about mvebu, but rather about general PCI devices.
I see drivers/of/address.c implements some:
extern const __be32 *of_get_pci_address(struct device_node *dev, int bar_no,
u64 *size, unsigned int *flags);
extern int of_pci_address_to_resource(struct device_node *dev, int bar,
struct resource *r);
Which are not hooked to any ranges (of_bus) mechanisms nor any DT-aware
PCI device driver and that's where I got completely lost.
But I'm probably looking at it from a wrong perspective.
Thanks,
Gerlando
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: address translation for PCIe-to-localbus bridge
@ 2013-11-12 8:26 ` Gerlando Falauto
0 siblings, 0 replies; 44+ messages in thread
From: Gerlando Falauto @ 2013-11-12 8:26 UTC (permalink / raw)
To: Thomas Petazzoni
Cc: Grant Likely, Jason Gunthorpe, Lior Amsalem,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Thierry Reding, Ezequiel Garcia, Gregory Cl??ment,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
Hi,
On 11/12/2013 09:16 AM, Thomas Petazzoni wrote:
> Dear Gerlando Falauto,
>
> On Tue, 12 Nov 2013 09:11:33 +0100, Gerlando Falauto wrote:
>
>>> Hum, right, but unless I'm wrong the of_busses[] array of struct of_bus
>>> is fixed in drivers/of/address.c, and as it is, there is no way for a
>>> specific bus driver to provide its own struct of_bus.
>>
>> That was exactly my understanding as well, and that's where I was
>> expecting some trickery to happen.
>>
>>> So that would need to be extended, right?
>>
>> The other approach I was foreseeing was to implement a way of
>> dynamically updating the DT when the PCI subsystem enumerates the
>> devices and assigns memory areas (namely, I would expect the ranges
>> property to reflect that). But this would imply a standard way of
>> defining the ranges property (I would expect something like <bar#
>> start_addr length>, with an arguable number of cells). And I could
>> not find any such definition in the PCI bus binding document, so I'm
>> probably completely off-track here. Aren't I?
>>
>> After all, we should expect a driver to behave (and expect) the same of
>> the DT, regardless of whether enumeration was performed by firmware or
>> by the OS itself. Or am I wrong on this too?
>
> Well, in the context of the mvebu platforms (including Kirkwood), the
> problem is not so much the ranges in the pcie-controller node, but the
> ranges in the main soc { ... } node which encloses the description of
> the MBus. It is those ranges that need to be updated when a new window
> is created, or a window is removed. So the problem is not PCI related,
> but MBus related in this case, no?
I was actually referring to ranges within the PCI *device* (~leaf node).
So not thinking about mvebu, but rather about general PCI devices.
I see drivers/of/address.c implements some:
extern const __be32 *of_get_pci_address(struct device_node *dev, int bar_no,
u64 *size, unsigned int *flags);
extern int of_pci_address_to_resource(struct device_node *dev, int bar,
struct resource *r);
Which are not hooked to any ranges (of_bus) mechanisms nor any DT-aware
PCI device driver and that's where I got completely lost.
But I'm probably looking at it from a wrong perspective.
Thanks,
Gerlando
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 44+ messages in thread
* address translation for PCIe-to-localbus bridge
@ 2013-11-13 6:26 ` Grant Likely
0 siblings, 0 replies; 44+ messages in thread
From: Grant Likely @ 2013-11-13 6:26 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, 12 Nov 2013 09:11:33 +0100, Gerlando Falauto <gerlando.falauto@keymile.com> wrote:
> On 11/12/2013 08:05 AM, Thomas Petazzoni wrote:
> > Dear Grant Likely,
> >
> > On Mon, 11 Nov 2013 15:50:50 +0000, Grant Likely wrote:
> >
> >>> So the mbus would register an address xlate for its node that is
> >>> called instead of ranges parsing. For the example in my last message
> >>> the FPGA driver would register an xlate that made addresses relative
> >>> to its own BAR0 address.
> >>
> >> There are already bus-specific transations available. Take a look at
> >> struct of_bus in drivers/of/address.c
> >
> > Hum, right, but unless I'm wrong the of_busses[] array of struct of_bus
> > is fixed in drivers/of/address.c, and as it is, there is no way for a
> > specific bus driver to provide its own struct of_bus.
>
> That was exactly my understanding as well, and that's where I was
> expecting some trickery to happen.
>
> > So that would need to be extended, right?
>
> The other approach I was foreseeing was to implement a way of
> dynamically updating the DT when the PCI subsystem enumerates the
> devices and assigns memory areas (namely, I would expect the ranges
> property to reflect that). But this would imply a standard way of
> defining the ranges property (I would expect something like <bar#
> start_addr length>, with an arguable number of cells). And I could
> not find any such definition in the PCI bus binding document, so I'm
> probably completely off-track here. Aren't I?
The ranges property is pretty well defined already. The format is
<parent-addr-specifier> <child-addr-specifier> <size-specifier>, and
then use assigned-addresses for each BAR to encode the address it is
assigned to in the PCI address space.
Ranges provides the windows into the parent bus, and assigned-address
is used to map the child into the parent. Using it should make the
of_bus translations work without changes.
Grep the source tree for assigned-addresses to get examples.
> After all, we should expect a driver to behave (and expect) the same of
> the DT, regardless of whether enumeration was performed by firmware or
> by the OS itself. Or am I wrong on this too?
Yes, that is a reasonable expectation and it is a limitation of the
current implementation. We've got two choices here. Modify the DT at
boot time, or extend the DT translation code to ask the PCI device for
BAR settings. I'm not going to make a call on which is better until I
see some prototype code.
g.
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: address translation for PCIe-to-localbus bridge
@ 2013-11-13 6:26 ` Grant Likely
0 siblings, 0 replies; 44+ messages in thread
From: Grant Likely @ 2013-11-13 6:26 UTC (permalink / raw)
To: Gerlando Falauto, Thomas Petazzoni
Cc: Jason Gunthorpe, Lior Amsalem,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Thierry Reding, Ezequiel Garcia, Gregory Cl??ment,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
On Tue, 12 Nov 2013 09:11:33 +0100, Gerlando Falauto <gerlando.falauto-SkAbAL50j+5BDgjK7y7TUQ@public.gmane.org> wrote:
> On 11/12/2013 08:05 AM, Thomas Petazzoni wrote:
> > Dear Grant Likely,
> >
> > On Mon, 11 Nov 2013 15:50:50 +0000, Grant Likely wrote:
> >
> >>> So the mbus would register an address xlate for its node that is
> >>> called instead of ranges parsing. For the example in my last message
> >>> the FPGA driver would register an xlate that made addresses relative
> >>> to its own BAR0 address.
> >>
> >> There are already bus-specific transations available. Take a look at
> >> struct of_bus in drivers/of/address.c
> >
> > Hum, right, but unless I'm wrong the of_busses[] array of struct of_bus
> > is fixed in drivers/of/address.c, and as it is, there is no way for a
> > specific bus driver to provide its own struct of_bus.
>
> That was exactly my understanding as well, and that's where I was
> expecting some trickery to happen.
>
> > So that would need to be extended, right?
>
> The other approach I was foreseeing was to implement a way of
> dynamically updating the DT when the PCI subsystem enumerates the
> devices and assigns memory areas (namely, I would expect the ranges
> property to reflect that). But this would imply a standard way of
> defining the ranges property (I would expect something like <bar#
> start_addr length>, with an arguable number of cells). And I could
> not find any such definition in the PCI bus binding document, so I'm
> probably completely off-track here. Aren't I?
The ranges property is pretty well defined already. The format is
<parent-addr-specifier> <child-addr-specifier> <size-specifier>, and
then use assigned-addresses for each BAR to encode the address it is
assigned to in the PCI address space.
Ranges provides the windows into the parent bus, and assigned-address
is used to map the child into the parent. Using it should make the
of_bus translations work without changes.
Grep the source tree for assigned-addresses to get examples.
> After all, we should expect a driver to behave (and expect) the same of
> the DT, regardless of whether enumeration was performed by firmware or
> by the OS itself. Or am I wrong on this too?
Yes, that is a reasonable expectation and it is a limitation of the
current implementation. We've got two choices here. Modify the DT at
boot time, or extend the DT translation code to ask the PCI device for
BAR settings. I'm not going to make a call on which is better until I
see some prototype code.
g.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 44+ messages in thread
* address translation for PCIe-to-localbus bridge
2013-11-12 7:05 ` Thomas Petazzoni
@ 2013-11-12 8:51 ` Grant Likely
-1 siblings, 0 replies; 44+ messages in thread
From: Grant Likely @ 2013-11-12 8:51 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, 12 Nov 2013 08:05:12 +0100, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote:
> Dear Grant Likely,
>
> On Mon, 11 Nov 2013 15:50:50 +0000, Grant Likely wrote:
>
> > > So the mbus would register an address xlate for its node that is
> > > called instead of ranges parsing. For the example in my last message
> > > the FPGA driver would register an xlate that made addresses relative
> > > to its own BAR0 address.
> >
> > There are already bus-specific transations available. Take a look at
> > struct of_bus in drivers/of/address.c
>
> Hum, right, but unless I'm wrong the of_busses[] array of struct of_bus
> is fixed in drivers/of/address.c, and as it is, there is no way for a
> specific bus driver to provide its own struct of_bus. So that would
> need to be extended, right?
Yes... or embed what you need in drivers/of/address.c for the time
being.
g.
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: address translation for PCIe-to-localbus bridge
@ 2013-11-12 8:51 ` Grant Likely
0 siblings, 0 replies; 44+ messages in thread
From: Grant Likely @ 2013-11-12 8:51 UTC (permalink / raw)
To: Thomas Petazzoni
Cc: Jason Gunthorpe, Lior Amsalem,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Thierry Reding, Gerlando Falauto, Ezequiel Garcia,
Gregory Cl??ment,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
On Tue, 12 Nov 2013 08:05:12 +0100, Thomas Petazzoni <thomas.petazzoni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> Dear Grant Likely,
>
> On Mon, 11 Nov 2013 15:50:50 +0000, Grant Likely wrote:
>
> > > So the mbus would register an address xlate for its node that is
> > > called instead of ranges parsing. For the example in my last message
> > > the FPGA driver would register an xlate that made addresses relative
> > > to its own BAR0 address.
> >
> > There are already bus-specific transations available. Take a look at
> > struct of_bus in drivers/of/address.c
>
> Hum, right, but unless I'm wrong the of_busses[] array of struct of_bus
> is fixed in drivers/of/address.c, and as it is, there is no way for a
> specific bus driver to provide its own struct of_bus. So that would
> need to be extended, right?
Yes... or embed what you need in drivers/of/address.c for the time
being.
g.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 44+ messages in thread
* address translation for PCIe-to-localbus bridge
2013-11-06 18:03 ` Thomas Petazzoni
@ 2013-11-06 18:33 ` Pantelis Antoniou
-1 siblings, 0 replies; 44+ messages in thread
From: Pantelis Antoniou @ 2013-11-06 18:33 UTC (permalink / raw)
To: linux-arm-kernel
Hi Thomas,
On Nov 6, 2013, at 8:03 PM, Thomas Petazzoni wrote:
> Dear Jason Gunthorpe,
>
[snip]
>
> Thomas
> --
> Thomas Petazzoni, CTO, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com
I don't know if that's really revevant, but this sounds very similar to the way that the GPMC driver
works (badly) on OMAPs.
In the old days we used to rely on the bootloader setting up things like chip selects, address windows
and what nots, and the kernel being very careful not messing with them.
So the drivers needed nothing more to work than being pointed the address windows that their device
happened to be configured by the bootloader.
Now that with the trend in bootloaders being really minimal, that task falls to the kernel.
Using board files one can get away with it using some per-board specific initialization.
On DT this fall downs completely, and you get abominations like the OMAP GPMC bindings...
Observe:
> status = "okay";
>
> #address-cells = <2>;
> #size-cells = <1>;
>
> pinctrl-names = "default";
> pinctrl-0 = <&gpmc_pins>;
>
> /* chip select ranges */
> ranges = <0 0 0x08000000 0x10000000>, /* bootloader has this enabled */
> <1 0 0x18000000 0x08000000>,
> <2 0 0x20000000 0x08000000>,
> <3 0 0x28000000 0x08000000>,
> <4 0 0x30000000 0x08000000>,
> <5 0 0x38000000 0x04000000>,
> <6 0 0x3c000000 0x04000000>;
>
> /*
> * This is complete and utter cr*p
> * It doesn't belong here, but we don't
> * have a memory controller abstraction.
> * So we muddle along...
> */
> camera {
> compatible = "cssp-camera";
> status = "okay";
> pinctrl-names = "default";
> pinctrl-0 = <&cssp_camera_pins>;
>
> reg = <1 0 0x01000000>; /* CS1 */
>
> bank-width = <2>; /* GPMC_CONFIG1_DEVICESIZE(1) */
>
> gpmc,burst-read; /* GPMC_CONFIG1_READMULTIPLE_SUPP */
> gpmc,sync-read; /* GPMC_CONFIG1_READTYPE_SYNC */
> gpmc,sync-write; /* GPMC_CONFIG1_WRITETYPE_SYNC */
> gpmc,clk-activation-ns = <20>; /* GPMC_CONFIG1_CLKACTIVATIONTIME(2) */
> gpmc,burst-length = <16>; /* GPMC_CONFIG1_PAGE_LEN(2) */
> gpmc,mux-add-data = <2>; /* GPMC_CONFIG1_MUXTYPE(2) */
>
> gpmc,sync-clk-ps = <20000>; /* CONFIG2 */
>
> gpmc,cs-on-ns = <1>;
> gpmc,cs-rd-off-ns = <160>;
> gpmc,cs-wr-off-ns = <310>;
>
> gpmc,adv-on-ns = <0>; /* CONFIG3 */
> gpmc,adv-rd-off-ns = <40>;
> gpmc,adv-wr-off-ns = <40>;
>
> gpmc,we-on-ns = <60>; /* CONFIG4 */
> gpmc,we-off-ns = <310>;
> gpmc,oe-on-ns = <60>;
> gpmc,oe-off-ns = <160>;
>
> gpmc,page-burst-access-ns = <20>; /* CONFIG 5 */
> gpmc,access-ns = <140>;
> gpmc,rd-cycle-ns = <160>;
> gpmc,wr-cycle-ns = <310>;
>
> gpmc,wr-access-ns = <100>; /* CONFIG 6 */
> gpmc,wr-data-mux-bus-ns = <60>;
>
> gpmc,bus-turnaround-ns = <40>; /* CONFIG6:3:0 = 4 */
> gpmc,cycle2cycle-samecsen; /* CONFIG6:7 = 1 */
> gpmc,cycle2cycle-delay-ns = <40>; /* CONFIG6:11:8 = 4 */
>
> /* not using dma engine yet, but we can get the channel number here */
> dmas = <&edma 20>;
> dma-names = "cssp";
>
> /* clocks */
> cssp,camera-clk-name = "adjustable_clkout2_ck";
> cssp,camera-clk-rate = <32000000>;
>
> /* reset */
> reset-gpio = <&gpio1 4 0>;
>
> /* orientation; -> MT9T112_FLAG_VFLIP */
> orientation-gpio = <&gpio1 30 0>;
>
> /* reset controller (for the black) */
> reset = <&rstctl 0 0>;
> reset-names = "eMMC_RSTn-CAM3";
>
> /* camera sensor */
> cssp,sensor {
> i2c-adapter = <&i2c2>;
>
> /* need it to stop the whinning */
> #address-cells = <1>;
> #size-cells = <0>;
>
> /* fake i2c device node */
> m59t112 {
> compatible = "aptina,mt9t112";
> reg = <0x3c>;
>
> /* m, n, p1-7 */
> flags = <0>;
> pll-divider = <24 1 0 7 0 10 14 7 0>;
> };
> };
>
> };
>
>
This is a fragment from a camera instantiation that happens to be connected to the GPMC interface.
The GPMC driver has to be hacked to support _any_ possible device it might be connected, and
there is a abundance of GPMC configuration properties in an unrelated device driver.
I could be wrong and jumped in the wrong thread, but I believe what we really need is a generic GPMC
framework, similar to the way pinctrl works.
So instead of doing the above we could just add a property that contains a reference to the required GPMC
configuration.
i.e.
gpmc {
compatible = "ti,gpmc";
camera_gpmc_config {
bank-width = <2>; /* GPMC_CONFIG1_DEVICESIZE(1) */
gpmc,burst-read; /* GPMC_CONFIG1_READMULTIPLE_SUPP */
gpmc,sync-read; /* GPMC_CONFIG1_READTYPE_SYNC */
gpmc,sync-write; /* GPMC_CONFIG1_WRITETYPE_SYNC */
gpmc,clk-activation-ns = <20>; /* GPMC_CONFIG1_CLKACTIVATIONTIME(2) */
gpmc,burst-length = <16>; /* GPMC_CONFIG1_PAGE_LEN(2) */
gpmc,mux-add-data = <2>; /* GPMC_CONFIG1_MUXTYPE(2) */
};
}
camera {
compatible = "cssp-camera";
reg = <0 0x01000000>; /* CS1 */
gpmc-config = <&camera_gpmc_config 1> /* CS1 */
...
};
Regards
-- Pantelis
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: address translation for PCIe-to-localbus bridge
@ 2013-11-06 18:33 ` Pantelis Antoniou
0 siblings, 0 replies; 44+ messages in thread
From: Pantelis Antoniou @ 2013-11-06 18:33 UTC (permalink / raw)
To: Thomas Petazzoni
Cc: Jason Gunthorpe, Gerlando Falauto,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
Thierry Reding, Ezequiel Garcia, Lior Amsalem,
Gregory Clément
Hi Thomas,
On Nov 6, 2013, at 8:03 PM, Thomas Petazzoni wrote:
> Dear Jason Gunthorpe,
>
[snip]
>
> Thomas
> --
> Thomas Petazzoni, CTO, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com
I don't know if that's really revevant, but this sounds very similar to the way that the GPMC driver
works (badly) on OMAPs.
In the old days we used to rely on the bootloader setting up things like chip selects, address windows
and what nots, and the kernel being very careful not messing with them.
So the drivers needed nothing more to work than being pointed the address windows that their device
happened to be configured by the bootloader.
Now that with the trend in bootloaders being really minimal, that task falls to the kernel.
Using board files one can get away with it using some per-board specific initialization.
On DT this fall downs completely, and you get abominations like the OMAP GPMC bindings...
Observe:
> status = "okay";
>
> #address-cells = <2>;
> #size-cells = <1>;
>
> pinctrl-names = "default";
> pinctrl-0 = <&gpmc_pins>;
>
> /* chip select ranges */
> ranges = <0 0 0x08000000 0x10000000>, /* bootloader has this enabled */
> <1 0 0x18000000 0x08000000>,
> <2 0 0x20000000 0x08000000>,
> <3 0 0x28000000 0x08000000>,
> <4 0 0x30000000 0x08000000>,
> <5 0 0x38000000 0x04000000>,
> <6 0 0x3c000000 0x04000000>;
>
> /*
> * This is complete and utter cr*p
> * It doesn't belong here, but we don't
> * have a memory controller abstraction.
> * So we muddle along...
> */
> camera {
> compatible = "cssp-camera";
> status = "okay";
> pinctrl-names = "default";
> pinctrl-0 = <&cssp_camera_pins>;
>
> reg = <1 0 0x01000000>; /* CS1 */
>
> bank-width = <2>; /* GPMC_CONFIG1_DEVICESIZE(1) */
>
> gpmc,burst-read; /* GPMC_CONFIG1_READMULTIPLE_SUPP */
> gpmc,sync-read; /* GPMC_CONFIG1_READTYPE_SYNC */
> gpmc,sync-write; /* GPMC_CONFIG1_WRITETYPE_SYNC */
> gpmc,clk-activation-ns = <20>; /* GPMC_CONFIG1_CLKACTIVATIONTIME(2) */
> gpmc,burst-length = <16>; /* GPMC_CONFIG1_PAGE_LEN(2) */
> gpmc,mux-add-data = <2>; /* GPMC_CONFIG1_MUXTYPE(2) */
>
> gpmc,sync-clk-ps = <20000>; /* CONFIG2 */
>
> gpmc,cs-on-ns = <1>;
> gpmc,cs-rd-off-ns = <160>;
> gpmc,cs-wr-off-ns = <310>;
>
> gpmc,adv-on-ns = <0>; /* CONFIG3 */
> gpmc,adv-rd-off-ns = <40>;
> gpmc,adv-wr-off-ns = <40>;
>
> gpmc,we-on-ns = <60>; /* CONFIG4 */
> gpmc,we-off-ns = <310>;
> gpmc,oe-on-ns = <60>;
> gpmc,oe-off-ns = <160>;
>
> gpmc,page-burst-access-ns = <20>; /* CONFIG 5 */
> gpmc,access-ns = <140>;
> gpmc,rd-cycle-ns = <160>;
> gpmc,wr-cycle-ns = <310>;
>
> gpmc,wr-access-ns = <100>; /* CONFIG 6 */
> gpmc,wr-data-mux-bus-ns = <60>;
>
> gpmc,bus-turnaround-ns = <40>; /* CONFIG6:3:0 = 4 */
> gpmc,cycle2cycle-samecsen; /* CONFIG6:7 = 1 */
> gpmc,cycle2cycle-delay-ns = <40>; /* CONFIG6:11:8 = 4 */
>
> /* not using dma engine yet, but we can get the channel number here */
> dmas = <&edma 20>;
> dma-names = "cssp";
>
> /* clocks */
> cssp,camera-clk-name = "adjustable_clkout2_ck";
> cssp,camera-clk-rate = <32000000>;
>
> /* reset */
> reset-gpio = <&gpio1 4 0>;
>
> /* orientation; -> MT9T112_FLAG_VFLIP */
> orientation-gpio = <&gpio1 30 0>;
>
> /* reset controller (for the black) */
> reset = <&rstctl 0 0>;
> reset-names = "eMMC_RSTn-CAM3";
>
> /* camera sensor */
> cssp,sensor {
> i2c-adapter = <&i2c2>;
>
> /* need it to stop the whinning */
> #address-cells = <1>;
> #size-cells = <0>;
>
> /* fake i2c device node */
> m59t112 {
> compatible = "aptina,mt9t112";
> reg = <0x3c>;
>
> /* m, n, p1-7 */
> flags = <0>;
> pll-divider = <24 1 0 7 0 10 14 7 0>;
> };
> };
>
> };
>
>
This is a fragment from a camera instantiation that happens to be connected to the GPMC interface.
The GPMC driver has to be hacked to support _any_ possible device it might be connected, and
there is a abundance of GPMC configuration properties in an unrelated device driver.
I could be wrong and jumped in the wrong thread, but I believe what we really need is a generic GPMC
framework, similar to the way pinctrl works.
So instead of doing the above we could just add a property that contains a reference to the required GPMC
configuration.
i.e.
gpmc {
compatible = "ti,gpmc";
camera_gpmc_config {
bank-width = <2>; /* GPMC_CONFIG1_DEVICESIZE(1) */
gpmc,burst-read; /* GPMC_CONFIG1_READMULTIPLE_SUPP */
gpmc,sync-read; /* GPMC_CONFIG1_READTYPE_SYNC */
gpmc,sync-write; /* GPMC_CONFIG1_WRITETYPE_SYNC */
gpmc,clk-activation-ns = <20>; /* GPMC_CONFIG1_CLKACTIVATIONTIME(2) */
gpmc,burst-length = <16>; /* GPMC_CONFIG1_PAGE_LEN(2) */
gpmc,mux-add-data = <2>; /* GPMC_CONFIG1_MUXTYPE(2) */
};
}
camera {
compatible = "cssp-camera";
reg = <0 0x01000000>; /* CS1 */
gpmc-config = <&camera_gpmc_config 1> /* CS1 */
...
};
Regards
-- Pantelis
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 44+ messages in thread* address translation for PCIe-to-localbus bridge
@ 2013-11-06 18:56 ` Jason Gunthorpe
0 siblings, 0 replies; 44+ messages in thread
From: Jason Gunthorpe @ 2013-11-06 18:56 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Nov 06, 2013 at 08:33:39PM +0200, Pantelis Antoniou wrote:
> On DT this fall downs completely, and you get abominations like the
> OMAP GPMC bindings...
This looks similar to Ezequiel's expansion bus bindings for MVEBU.
Maybe you can clarify the issue you see?
The general pattern I expect is something like this:
some_bus_bridge {
compatible = 'bridge bus driver';
ranges <[CS#] 0 [CS window base] [size]>;
chip-select,1 {
< All Parameters to configure timing, etc, for this
chip select>
ranges = <0 1 0 [size]>;
my device {
compatble = 'camera'
reg = <0 [size]>
}
}
The way it works is the DT machinery instantiates a some_bus_bridge.
The bridge driver goes through and parses the ranges, it sets up the
address mappings as necessary to create a window for each CS. If this
address assignment is dynamic then it needs to make the 'ranges'
correct. (MVEBU land does this in the mbus driver)
The bridge driver parses each child and configures the bus CS timing
parameters. (this is done in the external memory driver, IIRC)
The bridge driver constructs a struct device for each child in every
chip-select (the 'my device' node). Normal OF address translation
makes this work fine.
Your camera driver attaches to the 'my device' node, not to the chip
select node. The chip select node is used only by the bus bridge
driver to setup that specific chip select.
The example you have looks close to that except for a few details:
> Observe:
>
> > status = "okay";
> >
> > #address-cells = <2>;
> > #size-cells = <1>;
> >
> > pinctrl-names = "default";
> > pinctrl-0 = <&gpmc_pins>;
> >
> > /* chip select ranges */
> > ranges = <0 0 0x08000000 0x10000000>, /* bootloader has this enabled */
> > <1 0 0x18000000 0x08000000>,
> > <2 0 0x20000000 0x08000000>,
> > <3 0 0x28000000 0x08000000>,
> > <4 0 0x30000000 0x08000000>,
> > <5 0 0x38000000 0x04000000>,
> > <6 0 0x3c000000 0x04000000>;
OK, seems reasonable, setting up chip select windows, giving them base
addresess and sizes.
> > camera {
> > compatible = "cssp-camera";
> > status = "okay";
> > pinctrl-names = "default";
> > pinctrl-0 = <&cssp_camera_pins>;
> >
> > reg = <1 0 0x01000000>; /* CS1 */
Here we want to hoist bus specific stuff out of 'camera' and into
'cs1@', so:
No reg at this point.
> > bank-width = <2>; /* GPMC_CONFIG1_DEVICESIZE(1) */
> > gpmc,burst-read; /* GPMC_CONFIG1_READMULTIPLE_SUPP */
[...]
Timing parameters seem reasonable.
> >
> > /* not using dma engine yet, but we can get the channel number here */
> > dmas = <&edma 20>;
> > dma-names = "cssp";
DMA's/gpios, etc probably belong in the child. Maybe clocks belong in
the CS? Depends on the bridge driver..
> > /* clocks */
> > cssp,camera-clk-name = "adjustable_clkout2_ck";
> > cssp,camera-clk-rate = <32000000>;
> >
> > /* reset */
> > reset-gpio = <&gpio1 4 0>;
> >
> > /* orientation; -> MT9T112_FLAG_VFLIP */
> > orientation-gpio = <&gpio1 30 0>;
> >
> > /* reset controller (for the black) */
> > reset = <&rstctl 0 0>;
> > reset-names = "eMMC_RSTn-CAM3";
Missing:
ranges = <0 1 0 0x01000000>;
Missing:
camera at 0 {
reg = <0 0x01000000>;
Put DMA's/etc here.
> > /* camera sensor */
> > cssp,sensor {
> > i2c-adapter = <&i2c2>;
Yikes!
If there is an i2c component then a phandle to the component itself is
way better:
i2c-component = <&i2c_camera>
i2c2: ...
{
i2c_camera: m59t112 {
compatible = "aptina,mt9t112";
reg = <0x3c>;
}
}
Sprinkle address-cells/size-cells as necessary.
> gpmc {
> compatible = "ti,gpmc";
>
> camera_gpmc_config {
>
> bank-width = <2>; /* GPMC_CONFIG1_DEVICESIZE(1) */
>
> gpmc,burst-read; /* GPMC_CONFIG1_READMULTIPLE_SUPP */
> gpmc,sync-read; /* GPMC_CONFIG1_READTYPE_SYNC */
> gpmc,sync-write; /* GPMC_CONFIG1_WRITETYPE_SYNC */
> gpmc,clk-activation-ns = <20>; /* GPMC_CONFIG1_CLKACTIVATIONTIME(2) */
> gpmc,burst-length = <16>; /* GPMC_CONFIG1_PAGE_LEN(2) */
> gpmc,mux-add-data = <2>; /* GPMC_CONFIG1_MUXTYPE(2) */
> };
> }
Similar idea, but you have thrown away the nesting.
DT should reflect the address translation hierarchy of the system, so
'ti,gpmc' does address translation, the things it translates for
should be its children.
Jason
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: address translation for PCIe-to-localbus bridge
@ 2013-11-06 18:56 ` Jason Gunthorpe
0 siblings, 0 replies; 44+ messages in thread
From: Jason Gunthorpe @ 2013-11-06 18:56 UTC (permalink / raw)
To: Pantelis Antoniou
Cc: Thomas Petazzoni, Gerlando Falauto,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
Thierry Reding, Ezequiel Garcia, Lior Amsalem, Gregory Cl?ment
On Wed, Nov 06, 2013 at 08:33:39PM +0200, Pantelis Antoniou wrote:
> On DT this fall downs completely, and you get abominations like the
> OMAP GPMC bindings...
This looks similar to Ezequiel's expansion bus bindings for MVEBU.
Maybe you can clarify the issue you see?
The general pattern I expect is something like this:
some_bus_bridge {
compatible = 'bridge bus driver';
ranges <[CS#] 0 [CS window base] [size]>;
chip-select,1 {
< All Parameters to configure timing, etc, for this
chip select>
ranges = <0 1 0 [size]>;
my device {
compatble = 'camera'
reg = <0 [size]>
}
}
The way it works is the DT machinery instantiates a some_bus_bridge.
The bridge driver goes through and parses the ranges, it sets up the
address mappings as necessary to create a window for each CS. If this
address assignment is dynamic then it needs to make the 'ranges'
correct. (MVEBU land does this in the mbus driver)
The bridge driver parses each child and configures the bus CS timing
parameters. (this is done in the external memory driver, IIRC)
The bridge driver constructs a struct device for each child in every
chip-select (the 'my device' node). Normal OF address translation
makes this work fine.
Your camera driver attaches to the 'my device' node, not to the chip
select node. The chip select node is used only by the bus bridge
driver to setup that specific chip select.
The example you have looks close to that except for a few details:
> Observe:
>
> > status = "okay";
> >
> > #address-cells = <2>;
> > #size-cells = <1>;
> >
> > pinctrl-names = "default";
> > pinctrl-0 = <&gpmc_pins>;
> >
> > /* chip select ranges */
> > ranges = <0 0 0x08000000 0x10000000>, /* bootloader has this enabled */
> > <1 0 0x18000000 0x08000000>,
> > <2 0 0x20000000 0x08000000>,
> > <3 0 0x28000000 0x08000000>,
> > <4 0 0x30000000 0x08000000>,
> > <5 0 0x38000000 0x04000000>,
> > <6 0 0x3c000000 0x04000000>;
OK, seems reasonable, setting up chip select windows, giving them base
addresess and sizes.
> > camera {
> > compatible = "cssp-camera";
> > status = "okay";
> > pinctrl-names = "default";
> > pinctrl-0 = <&cssp_camera_pins>;
> >
> > reg = <1 0 0x01000000>; /* CS1 */
Here we want to hoist bus specific stuff out of 'camera' and into
'cs1@', so:
No reg at this point.
> > bank-width = <2>; /* GPMC_CONFIG1_DEVICESIZE(1) */
> > gpmc,burst-read; /* GPMC_CONFIG1_READMULTIPLE_SUPP */
[...]
Timing parameters seem reasonable.
> >
> > /* not using dma engine yet, but we can get the channel number here */
> > dmas = <&edma 20>;
> > dma-names = "cssp";
DMA's/gpios, etc probably belong in the child. Maybe clocks belong in
the CS? Depends on the bridge driver..
> > /* clocks */
> > cssp,camera-clk-name = "adjustable_clkout2_ck";
> > cssp,camera-clk-rate = <32000000>;
> >
> > /* reset */
> > reset-gpio = <&gpio1 4 0>;
> >
> > /* orientation; -> MT9T112_FLAG_VFLIP */
> > orientation-gpio = <&gpio1 30 0>;
> >
> > /* reset controller (for the black) */
> > reset = <&rstctl 0 0>;
> > reset-names = "eMMC_RSTn-CAM3";
Missing:
ranges = <0 1 0 0x01000000>;
Missing:
camera@0 {
reg = <0 0x01000000>;
Put DMA's/etc here.
> > /* camera sensor */
> > cssp,sensor {
> > i2c-adapter = <&i2c2>;
Yikes!
If there is an i2c component then a phandle to the component itself is
way better:
i2c-component = <&i2c_camera>
i2c2: ...
{
i2c_camera: m59t112 {
compatible = "aptina,mt9t112";
reg = <0x3c>;
}
}
Sprinkle address-cells/size-cells as necessary.
> gpmc {
> compatible = "ti,gpmc";
>
> camera_gpmc_config {
>
> bank-width = <2>; /* GPMC_CONFIG1_DEVICESIZE(1) */
>
> gpmc,burst-read; /* GPMC_CONFIG1_READMULTIPLE_SUPP */
> gpmc,sync-read; /* GPMC_CONFIG1_READTYPE_SYNC */
> gpmc,sync-write; /* GPMC_CONFIG1_WRITETYPE_SYNC */
> gpmc,clk-activation-ns = <20>; /* GPMC_CONFIG1_CLKACTIVATIONTIME(2) */
> gpmc,burst-length = <16>; /* GPMC_CONFIG1_PAGE_LEN(2) */
> gpmc,mux-add-data = <2>; /* GPMC_CONFIG1_MUXTYPE(2) */
> };
> }
Similar idea, but you have thrown away the nesting.
DT should reflect the address translation hierarchy of the system, so
'ti,gpmc' does address translation, the things it translates for
should be its children.
Jason
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 44+ messages in thread* address translation for PCIe-to-localbus bridge
@ 2013-11-06 19:16 ` Pantelis Antoniou
0 siblings, 0 replies; 44+ messages in thread
From: Pantelis Antoniou @ 2013-11-06 19:16 UTC (permalink / raw)
To: linux-arm-kernel
Hi Jason,
On Nov 6, 2013, at 8:56 PM, Jason Gunthorpe wrote:
> On Wed, Nov 06, 2013 at 08:33:39PM +0200, Pantelis Antoniou wrote:
>
>> On DT this fall downs completely, and you get abominations like the
>> OMAP GPMC bindings...
>
> This looks similar to Ezequiel's expansion bus bindings for MVEBU.
>
> Maybe you can clarify the issue you see?
>
> The general pattern I expect is something like this:
>
> some_bus_bridge {
> compatible = 'bridge bus driver';
>
> ranges <[CS#] 0 [CS window base] [size]>;
>
> chip-select,1 {
> < All Parameters to configure timing, etc, for this
> chip select>
> ranges = <0 1 0 [size]>;
>
> my device {
> compatble = 'camera'
> reg = <0 [size]>
> }
> }
>
> The way it works is the DT machinery instantiates a some_bus_bridge.
>
> The bridge driver goes through and parses the ranges, it sets up the
> address mappings as necessary to create a window for each CS. If this
> address assignment is dynamic then it needs to make the 'ranges'
> correct. (MVEBU land does this in the mbus driver)
>
> The bridge driver parses each child and configures the bus CS timing
> parameters. (this is done in the external memory driver, IIRC)
>
> The bridge driver constructs a struct device for each child in every
> chip-select (the 'my device' node). Normal OF address translation
> makes this work fine.
>
> Your camera driver attaches to the 'my device' node, not to the chip
> select node. The chip select node is used only by the bus bridge
> driver to setup that specific chip select.
>
No. I get the point about the bridge driver, but that's not what I want.
I agree that the bridge driver configures the chip selects, address ranges
and timings, but it shouldn't do that at the time the bridge driver is
instantiated.
That would mean that all configuration is fixed at boot time.
> The example you have looks close to that except for a few details:
>
>> Observe:
>>
>>> status = "okay";
>>>
>>> #address-cells = <2>;
>>> #size-cells = <1>;
>>>
>>> pinctrl-names = "default";
>>> pinctrl-0 = <&gpmc_pins>;
>>>
>>> /* chip select ranges */
>>> ranges = <0 0 0x08000000 0x10000000>, /* bootloader has this enabled */
>>> <1 0 0x18000000 0x08000000>,
>>> <2 0 0x20000000 0x08000000>,
>>> <3 0 0x28000000 0x08000000>,
>>> <4 0 0x30000000 0x08000000>,
>>> <5 0 0x38000000 0x04000000>,
>>> <6 0 0x3c000000 0x04000000>;
>
> OK, seems reasonable, setting up chip select windows, giving them base
> addresess and sizes.
>
>>> camera {
>>> compatible = "cssp-camera";
>>> status = "okay";
>>> pinctrl-names = "default";
>>> pinctrl-0 = <&cssp_camera_pins>;
>>>
>>> reg = <1 0 0x01000000>; /* CS1 */
>
> Here we want to hoist bus specific stuff out of 'camera' and into
> 'cs1@', so:
>
> No reg at this point.
>
>>> bank-width = <2>; /* GPMC_CONFIG1_DEVICESIZE(1) */
>>> gpmc,burst-read; /* GPMC_CONFIG1_READMULTIPLE_SUPP */
> [...]
>
> Timing parameters seem reasonable.
>
>>>
>>> /* not using dma engine yet, but we can get the channel number here */
>>> dmas = <&edma 20>;
>>> dma-names = "cssp";
>
> DMA's/gpios, etc probably belong in the child. Maybe clocks belong in
> the CS? Depends on the bridge driver..
>
>>> /* clocks */
>>> cssp,camera-clk-name = "adjustable_clkout2_ck";
>>> cssp,camera-clk-rate = <32000000>;
>>>
>>> /* reset */
>>> reset-gpio = <&gpio1 4 0>;
>>>
>>> /* orientation; -> MT9T112_FLAG_VFLIP */
>>> orientation-gpio = <&gpio1 30 0>;
>>>
>>> /* reset controller (for the black) */
>>> reset = <&rstctl 0 0>;
>>> reset-names = "eMMC_RSTn-CAM3";
>
> Missing:
>
> ranges = <0 1 0 0x01000000>;
>
> Missing:
>
> camera at 0 {
> reg = <0 0x01000000>;
>
> Put DMA's/etc here.
>
>>> /* camera sensor */
>>> cssp,sensor {
>>> i2c-adapter = <&i2c2>;
>
> Yikes!
>
> If there is an i2c component then a phandle to the component itself is
> way better:
>
> i2c-component = <&i2c_camera>
>
> i2c2: ...
> {
> i2c_camera: m59t112 {
> compatible = "aptina,mt9t112";
> reg = <0x3c>;
> }
> }
>
> Sprinkle address-cells/size-cells as necessary.
>
>> gpmc {
>> compatible = "ti,gpmc";
>>
>> camera_gpmc_config {
>>
>> bank-width = <2>; /* GPMC_CONFIG1_DEVICESIZE(1) */
>>
>> gpmc,burst-read; /* GPMC_CONFIG1_READMULTIPLE_SUPP */
>> gpmc,sync-read; /* GPMC_CONFIG1_READTYPE_SYNC */
>> gpmc,sync-write; /* GPMC_CONFIG1_WRITETYPE_SYNC */
>> gpmc,clk-activation-ns = <20>; /* GPMC_CONFIG1_CLKACTIVATIONTIME(2) */
>> gpmc,burst-length = <16>; /* GPMC_CONFIG1_PAGE_LEN(2) */
>> gpmc,mux-add-data = <2>; /* GPMC_CONFIG1_MUXTYPE(2) */
>> };
>> }
>
> Similar idea, but you have thrown away the nesting.
>
The bindings presented was just for illustration, I just wanted to present the idea :)
> DT should reflect the address translation hierarchy of the system, so
> 'ti,gpmc' does address translation, the things it translates for
> should be its children.
>
That's where we disagree. I don't want the drivers to be children of the bridge.
All the drivers that are controlled by a chip select and/or bus access method are
written without ever dealing with the bridge; they just expect the configuration to be
valid and need to only know the address range they reside.
As you pointed the i2c component, which was a child of the device node, it is better to become a
phandle reference.
That's what I'm proposing for the bridge configuration as well, so that the configuration is applied
only when a device requests it. This is important for hot-plugging, and or when using stuff like
device overlays.
> Jason
Regards
-- Pantelis
^ permalink raw reply [flat|nested] 44+ messages in thread* Re: address translation for PCIe-to-localbus bridge
@ 2013-11-06 19:16 ` Pantelis Antoniou
0 siblings, 0 replies; 44+ messages in thread
From: Pantelis Antoniou @ 2013-11-06 19:16 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Thomas Petazzoni, Gerlando Falauto,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
Thierry Reding, Ezequiel Garcia, Lior Amsalem, Gregory Cl?ment
Hi Jason,
On Nov 6, 2013, at 8:56 PM, Jason Gunthorpe wrote:
> On Wed, Nov 06, 2013 at 08:33:39PM +0200, Pantelis Antoniou wrote:
>
>> On DT this fall downs completely, and you get abominations like the
>> OMAP GPMC bindings...
>
> This looks similar to Ezequiel's expansion bus bindings for MVEBU.
>
> Maybe you can clarify the issue you see?
>
> The general pattern I expect is something like this:
>
> some_bus_bridge {
> compatible = 'bridge bus driver';
>
> ranges <[CS#] 0 [CS window base] [size]>;
>
> chip-select,1 {
> < All Parameters to configure timing, etc, for this
> chip select>
> ranges = <0 1 0 [size]>;
>
> my device {
> compatble = 'camera'
> reg = <0 [size]>
> }
> }
>
> The way it works is the DT machinery instantiates a some_bus_bridge.
>
> The bridge driver goes through and parses the ranges, it sets up the
> address mappings as necessary to create a window for each CS. If this
> address assignment is dynamic then it needs to make the 'ranges'
> correct. (MVEBU land does this in the mbus driver)
>
> The bridge driver parses each child and configures the bus CS timing
> parameters. (this is done in the external memory driver, IIRC)
>
> The bridge driver constructs a struct device for each child in every
> chip-select (the 'my device' node). Normal OF address translation
> makes this work fine.
>
> Your camera driver attaches to the 'my device' node, not to the chip
> select node. The chip select node is used only by the bus bridge
> driver to setup that specific chip select.
>
No. I get the point about the bridge driver, but that's not what I want.
I agree that the bridge driver configures the chip selects, address ranges
and timings, but it shouldn't do that at the time the bridge driver is
instantiated.
That would mean that all configuration is fixed at boot time.
> The example you have looks close to that except for a few details:
>
>> Observe:
>>
>>> status = "okay";
>>>
>>> #address-cells = <2>;
>>> #size-cells = <1>;
>>>
>>> pinctrl-names = "default";
>>> pinctrl-0 = <&gpmc_pins>;
>>>
>>> /* chip select ranges */
>>> ranges = <0 0 0x08000000 0x10000000>, /* bootloader has this enabled */
>>> <1 0 0x18000000 0x08000000>,
>>> <2 0 0x20000000 0x08000000>,
>>> <3 0 0x28000000 0x08000000>,
>>> <4 0 0x30000000 0x08000000>,
>>> <5 0 0x38000000 0x04000000>,
>>> <6 0 0x3c000000 0x04000000>;
>
> OK, seems reasonable, setting up chip select windows, giving them base
> addresess and sizes.
>
>>> camera {
>>> compatible = "cssp-camera";
>>> status = "okay";
>>> pinctrl-names = "default";
>>> pinctrl-0 = <&cssp_camera_pins>;
>>>
>>> reg = <1 0 0x01000000>; /* CS1 */
>
> Here we want to hoist bus specific stuff out of 'camera' and into
> 'cs1@', so:
>
> No reg at this point.
>
>>> bank-width = <2>; /* GPMC_CONFIG1_DEVICESIZE(1) */
>>> gpmc,burst-read; /* GPMC_CONFIG1_READMULTIPLE_SUPP */
> [...]
>
> Timing parameters seem reasonable.
>
>>>
>>> /* not using dma engine yet, but we can get the channel number here */
>>> dmas = <&edma 20>;
>>> dma-names = "cssp";
>
> DMA's/gpios, etc probably belong in the child. Maybe clocks belong in
> the CS? Depends on the bridge driver..
>
>>> /* clocks */
>>> cssp,camera-clk-name = "adjustable_clkout2_ck";
>>> cssp,camera-clk-rate = <32000000>;
>>>
>>> /* reset */
>>> reset-gpio = <&gpio1 4 0>;
>>>
>>> /* orientation; -> MT9T112_FLAG_VFLIP */
>>> orientation-gpio = <&gpio1 30 0>;
>>>
>>> /* reset controller (for the black) */
>>> reset = <&rstctl 0 0>;
>>> reset-names = "eMMC_RSTn-CAM3";
>
> Missing:
>
> ranges = <0 1 0 0x01000000>;
>
> Missing:
>
> camera@0 {
> reg = <0 0x01000000>;
>
> Put DMA's/etc here.
>
>>> /* camera sensor */
>>> cssp,sensor {
>>> i2c-adapter = <&i2c2>;
>
> Yikes!
>
> If there is an i2c component then a phandle to the component itself is
> way better:
>
> i2c-component = <&i2c_camera>
>
> i2c2: ...
> {
> i2c_camera: m59t112 {
> compatible = "aptina,mt9t112";
> reg = <0x3c>;
> }
> }
>
> Sprinkle address-cells/size-cells as necessary.
>
>> gpmc {
>> compatible = "ti,gpmc";
>>
>> camera_gpmc_config {
>>
>> bank-width = <2>; /* GPMC_CONFIG1_DEVICESIZE(1) */
>>
>> gpmc,burst-read; /* GPMC_CONFIG1_READMULTIPLE_SUPP */
>> gpmc,sync-read; /* GPMC_CONFIG1_READTYPE_SYNC */
>> gpmc,sync-write; /* GPMC_CONFIG1_WRITETYPE_SYNC */
>> gpmc,clk-activation-ns = <20>; /* GPMC_CONFIG1_CLKACTIVATIONTIME(2) */
>> gpmc,burst-length = <16>; /* GPMC_CONFIG1_PAGE_LEN(2) */
>> gpmc,mux-add-data = <2>; /* GPMC_CONFIG1_MUXTYPE(2) */
>> };
>> }
>
> Similar idea, but you have thrown away the nesting.
>
The bindings presented was just for illustration, I just wanted to present the idea :)
> DT should reflect the address translation hierarchy of the system, so
> 'ti,gpmc' does address translation, the things it translates for
> should be its children.
>
That's where we disagree. I don't want the drivers to be children of the bridge.
All the drivers that are controlled by a chip select and/or bus access method are
written without ever dealing with the bridge; they just expect the configuration to be
valid and need to only know the address range they reside.
As you pointed the i2c component, which was a child of the device node, it is better to become a
phandle reference.
That's what I'm proposing for the bridge configuration as well, so that the configuration is applied
only when a device requests it. This is important for hot-plugging, and or when using stuff like
device overlays.
> Jason
Regards
-- Pantelis
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 44+ messages in thread* address translation for PCIe-to-localbus bridge
@ 2013-11-06 19:50 ` Jason Gunthorpe
0 siblings, 0 replies; 44+ messages in thread
From: Jason Gunthorpe @ 2013-11-06 19:50 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Nov 06, 2013 at 09:16:13PM +0200, Pantelis Antoniou wrote:
> > Your camera driver attaches to the 'my device' node, not to the chip
> > select node. The chip select node is used only by the bus bridge
> > driver to setup that specific chip select.
>
> No. I get the point about the bridge driver, but that's not what I want.
>
> I agree that the bridge driver configures the chip selects, address ranges
> and timings, but it shouldn't do that at the time the bridge driver is
> instantiated.
But that is normal Linux behavior. The bus driver controls the bus,
and it takes control when it is instantiated.
> That would mean that all configuration is fixed at boot time.
No, it means at boot time the configuration data in the DT is
programmed into the hardware at boot time.
If you hotplug the environment later during run time then you have to
notify the bridge driver and have it make whatever changes. This is no
different than PCI which requires notifying the PCI layer to have it
reprogram bridge devices.
This is why the nodes must be under the bridge, only the bridge knows
when it is safe to create struct device's for the children.
> All the drivers that are controlled by a chip select and/or bus
> access method are written without ever dealing with the bridge; they
> just expect the configuration to be valid and need to only know the
> address range they reside.
That is exactly what my example is showing. The driver has no idea
about the bridge. Ezequiel's MVEBU version does this today, IIRC, he
has the standard every day memory mapped NOR driver working as a node
under the bridge, with full configurability, and no changes to the NOR
driver.
> As you pointed the i2c component, which was a child of the device
> node, it is better to become a phandle reference.
Again, the DT parent/child reflects bus ownership/address
translation.
It is incorrect to put an i2c device below any node other than the i2c
bus it is attached to.
> That's what I'm proposing for the bridge configuration as well, so
> that the configuration is applied only when a device requests
> it. This is important for hot-plugging, and or when using stuff like
> device overlays.
If there is no children to a chip select then the bridge can leave it
turned off and keep address space deallocated.
The bridge driver simply has to be involved in the hot plug process,
which is again normal in Linux.
Device tree overlays provide a mechanism to create new chip select
nodes under the bridge, I don't see the problem you do :)
Jason
^ permalink raw reply [flat|nested] 44+ messages in thread
* Re: address translation for PCIe-to-localbus bridge
@ 2013-11-06 19:50 ` Jason Gunthorpe
0 siblings, 0 replies; 44+ messages in thread
From: Jason Gunthorpe @ 2013-11-06 19:50 UTC (permalink / raw)
To: Pantelis Antoniou
Cc: Thomas Petazzoni, Gerlando Falauto,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
Thierry Reding, Ezequiel Garcia, Lior Amsalem, Gregory Cl?ment
On Wed, Nov 06, 2013 at 09:16:13PM +0200, Pantelis Antoniou wrote:
> > Your camera driver attaches to the 'my device' node, not to the chip
> > select node. The chip select node is used only by the bus bridge
> > driver to setup that specific chip select.
>
> No. I get the point about the bridge driver, but that's not what I want.
>
> I agree that the bridge driver configures the chip selects, address ranges
> and timings, but it shouldn't do that at the time the bridge driver is
> instantiated.
But that is normal Linux behavior. The bus driver controls the bus,
and it takes control when it is instantiated.
> That would mean that all configuration is fixed at boot time.
No, it means at boot time the configuration data in the DT is
programmed into the hardware at boot time.
If you hotplug the environment later during run time then you have to
notify the bridge driver and have it make whatever changes. This is no
different than PCI which requires notifying the PCI layer to have it
reprogram bridge devices.
This is why the nodes must be under the bridge, only the bridge knows
when it is safe to create struct device's for the children.
> All the drivers that are controlled by a chip select and/or bus
> access method are written without ever dealing with the bridge; they
> just expect the configuration to be valid and need to only know the
> address range they reside.
That is exactly what my example is showing. The driver has no idea
about the bridge. Ezequiel's MVEBU version does this today, IIRC, he
has the standard every day memory mapped NOR driver working as a node
under the bridge, with full configurability, and no changes to the NOR
driver.
> As you pointed the i2c component, which was a child of the device
> node, it is better to become a phandle reference.
Again, the DT parent/child reflects bus ownership/address
translation.
It is incorrect to put an i2c device below any node other than the i2c
bus it is attached to.
> That's what I'm proposing for the bridge configuration as well, so
> that the configuration is applied only when a device requests
> it. This is important for hot-plugging, and or when using stuff like
> device overlays.
If there is no children to a chip select then the bridge can leave it
turned off and keep address space deallocated.
The bridge driver simply has to be involved in the hot plug process,
which is again normal in Linux.
Device tree overlays provide a mechanism to create new chip select
nodes under the bridge, I don't see the problem you do :)
Jason
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 44+ messages in thread