diff mbox

[v2,1/9] dmaengine: st_fdma: Add STMicroelectronics FDMA DT binding documentation

Message ID 1441980871-24475-2-git-send-email-peter.griffin@linaro.org
State Under Review, archived
Headers show

Commit Message

Peter Griffin Sept. 11, 2015, 2:14 p.m. UTC
This patch adds the DT binding documentation for the FDMA constroller
found on STi based chipsets from STMicroelectronics.

Signed-off-by: Ludovic Barre <ludovic.barre@st.com>
Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
---
 Documentation/devicetree/bindings/dma/st_fdma.txt | 78 +++++++++++++++++++++++
 1 file changed, 78 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/dma/st_fdma.txt

Comments

Arnd Bergmann Sept. 11, 2015, 8:36 p.m. UTC | #1
On Friday 11 September 2015 15:14:23 Peter Griffin wrote:
> +- st,fdma-id	: Must contain fdma controller number

What for?
> +Example:
> +
> +	fdma1: fdma-app@8e40000 {

The name should be "dma-controller", not "fdma-app".

> +Example:
> +
> +	snd_uni_player2: snd-uni-player@2 {
> +		compatible = "st,snd_uni_player";

Maybe change the compatible string to "st,snd-uni-player"?

The string you use here would not be acceptable for a binding.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Peter Griffin Sept. 12, 2015, 12:07 p.m. UTC | #2
Hi Arnd,

On Fri, 11 Sep 2015, Arnd Bergmann wrote:

> On Friday 11 September 2015 15:14:23 Peter Griffin wrote:
> > +- st,fdma-id	: Must contain fdma controller number
> 
> What for?

It is used by the driver to generate a unique firmware name.
Basically we need to know which controller instance we
are as each controller has a different firmware which needs
to be loaded.

Rob did say that having a index type property is undesirable
over here, see my reply at the bottom
http://www.spinics.net/lists/devicetree/msg92529.html.

However I can't think of any other useful properties we could add
to derive this information.

> > +Example:
> > +
> > +	fdma1: fdma-app@8e40000 {
> 
> The name should be "dma-controller", not "fdma-app".

Ok will fix in v3

> 
> > +Example:
> > +
> > +	snd_uni_player2: snd-uni-player@2 {
> > +		compatible = "st,snd_uni_player";
> 
> Maybe change the compatible string to "st,snd-uni-player"?

Just checking the upstream driver which got merged for v4.3
and the compatible has changed to "st,sti-uni-player" so I
can update the DT doc example to use this in v3.

> 
> The string you use here would not be acceptable for a binding.

Presumably it is the use of "underscores" in the compatible which
is not allowed? Is that correct?

regards,

Peter.


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lee Jones Sept. 14, 2015, 8:19 a.m. UTC | #3
On Sat, 12 Sep 2015, Peter Griffin wrote:

> Hi Arnd,
> 
> On Fri, 11 Sep 2015, Arnd Bergmann wrote:
> 
> > On Friday 11 September 2015 15:14:23 Peter Griffin wrote:
> > > +- st,fdma-id	: Must contain fdma controller number
> > 
> > What for?
> 
> It is used by the driver to generate a unique firmware name.
> Basically we need to know which controller instance we
> are as each controller has a different firmware which needs
> to be loaded.
> 
> Rob did say that having a index type property is undesirable
> over here, see my reply at the bottom
> http://www.spinics.net/lists/devicetree/msg92529.html.
> 
> However I can't think of any other useful properties we could add
> to derive this information.

I wouldn't use a property at all.  Why not use the compatible string?

Who chooses the naming scheme of the firmware binary?

Is there any reason they can't be:

  fdma_STiH407_audio.elf
  fdma_STiH407_app.elf
  fdma_STiH407_free_running.elf

Then you can have a different compatible for each:

  compatible = "st,stih407-fdma-mpe31-audio";
  compatible = "st,stih407-fdma-mpe31-app";
  compatible = "st,stih407-fdma-mpe31-free-running";

[...]

> > > +Example:
> > > +
> > > +	snd_uni_player2: snd-uni-player@2 {
> > > +		compatible = "st,snd_uni_player";
> > 
> > Maybe change the compatible string to "st,snd-uni-player"?
> 
> Just checking the upstream driver which got merged for v4.3
> and the compatible has changed to "st,sti-uni-player" so I
> can update the DT doc example to use this in v3.
> 
> > 
> > The string you use here would not be acceptable for a binding.
> 
> Presumably it is the use of "underscores" in the compatible which
> is not allowed? Is that correct?

Yes.
Peter Griffin Sept. 29, 2015, 10:04 a.m. UTC | #4
Hi Lee,

On Mon, 14 Sep 2015, Lee Jones wrote:

> > On Fri, 11 Sep 2015, Arnd Bergmann wrote:
> > 
> > > On Friday 11 September 2015 15:14:23 Peter Griffin wrote:
> > > > +- st,fdma-id	: Must contain fdma controller number
> > > 
> > > What for?
> > 
> > It is used by the driver to generate a unique firmware name.
> > Basically we need to know which controller instance we
> > are as each controller has a different firmware which needs
> > to be loaded.
> > 
> > Rob did say that having a index type property is undesirable
> > over here, see my reply at the bottom
> > http://www.spinics.net/lists/devicetree/msg92529.html.
> > 
> > However I can't think of any other useful properties we could add
> > to derive this information.
> 
> I wouldn't use a property at all.  Why not use the compatible string?
> 
> Who chooses the naming scheme of the firmware binary?

ST

> 
> Is there any reason they can't be:
> 
>   fdma_STiH407_audio.elf
>   fdma_STiH407_app.elf
>   fdma_STiH407_free_running.elf

Not sure, we could easily rename them locally. Getting ST to change
the firmware names in the stlinux distro might be harder.
My personal preference is to leave the firmware names "as is", unless
there is a real show stopper reason where we *have* to change them e.g.
we can't support all STi platforms with the same userspace because
the firmware filenames aren't unique enough.

> 
> Then you can have a different compatible for each:
> 
>   compatible = "st,stih407-fdma-mpe31-audio";
>   compatible = "st,stih407-fdma-mpe31-app";
>   compatible = "st,stih407-fdma-mpe31-free-running";

I think if you took the approach of only using the compatible
you would still need to use the fdma instance number
otherwise you end up with the same problems discussed in the 
"Firmware filename in DT" thread [1] e.g.

   compatible = "st,stih407-fdma-mpe31-0";
   compatible = "st,stih407-fdma-mpe31-1";
   compatible = "st,stih407-fdma-mpe31-2";

Specifically the problem is related to

"The hardware is identical, and different firmware is used to apply
   it in different ways."

Which is the case with fdma. By encoding the "way you wish to apply it" into the
compatible string, it causes problems if you want to change for example fdma0
to do some other function other than audio.

You then require a DT update, (when the hardware hasn't changed, just the
firmware) which is the same problem as using the filename directly in DT.

Therefore I believe it is important that the DT binding does *not* encode the
way the hardware is to be applied into the binding in *any* way, and defers this
 decision to the driver.
That is the rationale / reasoning behind choosing the fdma instance number.

Assuming you agree with my arguments above, then the choice becomes between 
having a fdma instance DT property, or having lots of compatibles where the only
difference is the appending of the instance number. I think out of the two I prefer
my original approach.

Any thoughts from the DT folks?

regards,

Peter.

[1] http://comments.gmane.org/gmane.linux.drivers.devicetree/133782
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann Sept. 29, 2015, 11:17 a.m. UTC | #5
On Tuesday 29 September 2015 11:04:40 Peter Griffin wrote:
> 
> "The hardware is identical, and different firmware is used to apply
>    it in different ways."
> 
> Which is the case with fdma. By encoding the "way you wish to apply it" into the
> compatible string, it causes problems if you want to change for example fdma0
> to do some other function other than audio.
> 
> You then require a DT update, (when the hardware hasn't changed, just the
> firmware) which is the same problem as using the filename directly in DT.
> 
> Therefore I believe it is important that the DT binding does *not* encode the
> way the hardware is to be applied into the binding in *any* way, and defers this
>  decision to the driver.
> That is the rationale / reasoning behind choosing the fdma instance number.
> 
> Assuming you agree with my arguments above, then the choice becomes between 
> having a fdma instance DT property, or having lots of compatibles where the only
> difference is the appending of the instance number. I think out of the two I prefer
> my original approach.
> 
> Any thoughts from the DT folks?

To me both approaches sound wrong: basing the firmware name on the instance
number requires that each instance is always used in the same way, which
is not guaranteed to be the case, and you correctly describe the problem with
using the compatible string for the firmware name if the driver for the FDMA
does not actually care what firmware is being used here.

Whatever code makes the decision as to how the FDMA is used should also
decide on the name of the firmware file. You can have a mapping in the
driver that contains the file names that are used by ST's distro, possibly
with a fallback mechanism that tries more meaningful names first.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Peter Griffin Sept. 29, 2015, 12:11 p.m. UTC | #6
Hi Arnd,

On Tue, 29 Sep 2015, Arnd Bergmann wrote:

> On Tuesday 29 September 2015 11:04:40 Peter Griffin wrote:
> > 
> > "The hardware is identical, and different firmware is used to apply
> >    it in different ways."
> > 
> > Which is the case with fdma. By encoding the "way you wish to apply it" into the
> > compatible string, it causes problems if you want to change for example fdma0
> > to do some other function other than audio.
> > 
> > You then require a DT update, (when the hardware hasn't changed, just the
> > firmware) which is the same problem as using the filename directly in DT.
> > 
> > Therefore I believe it is important that the DT binding does *not* encode the
> > way the hardware is to be applied into the binding in *any* way, and defers this
> >  decision to the driver.
> > That is the rationale / reasoning behind choosing the fdma instance number.
> > 
> > Assuming you agree with my arguments above, then the choice becomes between 
> > having a fdma instance DT property, or having lots of compatibles where the only
> > difference is the appending of the instance number. I think out of the two I prefer
> > my original approach.
> > 
> > Any thoughts from the DT folks?
> 
> To me both approaches sound wrong: basing the firmware name on the instance
> number requires that each instance is always used in the same way, which
> is not guaranteed to be the case,

Does it? I didn't think it did.

Using the instance number as a DT property defers the decision over what firmware to
load to the driver, which can choose whatever firmware name it wishes.

e.g. in v4.3 it could load xyz.elf, in v4.4 it could choose abc.elf. The DT will remain
unchanged, but the use of that fdma instance has changed.

We currently only have one firmware for each instance with the "use" compiled into it.
If in the future we had two firmwares with different "uses" for the same instance some extra
logic would be required in the driver to make a decision on which firmware to load.

> and you correctly describe the problem with
> using the compatible string for the firmware name if the driver for the FDMA
> does not actually care what firmware is being used here.
> 
> Whatever code makes the decision as to how the FDMA is used should also
> decide on the name of the firmware file.

The code which makes this decision currently is the st_fdma.c driver. However it does
need to know which fdma controller it is operating on to make this decision correctly.

Apart from passing the fdma instance number in DT, how else can we determine which
controller we are?

I guess we could infer it by having a table in the driver containing the base addresses
of the controllers for a given SoC, and match that against what DT passes us in the
reg property. But that seems ugly, and is encoding the same information in two
different places.

I'm open to suggestions if there is a better way to do this.

regards,

Peter.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann Sept. 29, 2015, 12:30 p.m. UTC | #7
On Tuesday 29 September 2015 13:11:55 Peter Griffin wrote:
> Hi Arnd,
> 
> On Tue, 29 Sep 2015, Arnd Bergmann wrote:
> 
> > On Tuesday 29 September 2015 11:04:40 Peter Griffin wrote:
> > > 
> > > "The hardware is identical, and different firmware is used to apply
> > >    it in different ways."
> > > 
> > > Which is the case with fdma. By encoding the "way you wish to apply it" into the
> > > compatible string, it causes problems if you want to change for example fdma0
> > > to do some other function other than audio.
> > > 
> > > You then require a DT update, (when the hardware hasn't changed, just the
> > > firmware) which is the same problem as using the filename directly in DT.
> > > 
> > > Therefore I believe it is important that the DT binding does *not* encode the
> > > way the hardware is to be applied into the binding in *any* way, and defers this
> > >  decision to the driver.
> > > That is the rationale / reasoning behind choosing the fdma instance number.
> > > 
> > > Assuming you agree with my arguments above, then the choice becomes between 
> > > having a fdma instance DT property, or having lots of compatibles where the only
> > > difference is the appending of the instance number. I think out of the two I prefer
> > > my original approach.
> > > 
> > > Any thoughts from the DT folks?
> > 
> > To me both approaches sound wrong: basing the firmware name on the instance
> > number requires that each instance is always used in the same way, which
> > is not guaranteed to be the case,
> 
> Does it? I didn't think it did.
> 
> Using the instance number as a DT property defers the decision over what firmware to
> load to the driver, which can choose whatever firmware name it wishes.
> 
> e.g. in v4.3 it could load xyz.elf, in v4.4 it could choose abc.elf. The DT will remain
> unchanged, but the use of that fdma instance has changed.
> 
> We currently only have one firmware for each instance with the "use" compiled into it.
> If in the future we had two firmwares with different "uses" for the same instance some extra
> logic would be required in the driver to make a decision on which firmware to load.

Ok, I probably need some more background about what the firmware on this
device does, and what it could do with a different firmware. Could you
elaborate?

> > and you correctly describe the problem with
> > using the compatible string for the firmware name if the driver for the FDMA
> > does not actually care what firmware is being used here.
> > 
> > Whatever code makes the decision as to how the FDMA is used should also
> > decide on the name of the firmware file.
> 
> The code which makes this decision currently is the st_fdma.c driver. However it does
> need to know which fdma controller it is operating on to make this decision correctly.
> 
> Apart from passing the fdma instance number in DT, how else can we determine which
> controller we are?
> 
> I guess we could infer it by having a table in the driver containing the base addresses
> of the controllers for a given SoC, and match that against what DT passes us in the
> reg property. But that seems ugly, and is encoding the same information in two
> different places.
> 
> I'm open to suggestions if there is a better way to do this.

Using the address would be the same thing, that doesn't change the
fundamental logic. Can you explain why it matters which instance
a firmware is used on for this driver?

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Peter Griffin Sept. 29, 2015, 1:42 p.m. UTC | #8
Hi Arnd,

On Tue, 29 Sep 2015, Arnd Bergmann wrote:

> On Tuesday 29 September 2015 13:11:55 Peter Griffin wrote:
> > Hi Arnd,
> > 
> > On Tue, 29 Sep 2015, Arnd Bergmann wrote:
> > 
> > > On Tuesday 29 September 2015 11:04:40 Peter Griffin wrote:
> > > > 
> > > > "The hardware is identical, and different firmware is used to apply
> > > >    it in different ways."
> > > > 
> > > > Which is the case with fdma. By encoding the "way you wish to apply it" into the
> > > > compatible string, it causes problems if you want to change for example fdma0
> > > > to do some other function other than audio.
> > > > 
> > > > You then require a DT update, (when the hardware hasn't changed, just the
> > > > firmware) which is the same problem as using the filename directly in DT.
> > > > 
> > > > Therefore I believe it is important that the DT binding does *not* encode the
> > > > way the hardware is to be applied into the binding in *any* way, and defers this
> > > >  decision to the driver.
> > > > That is the rationale / reasoning behind choosing the fdma instance number.
> > > > 
> > > > Assuming you agree with my arguments above, then the choice becomes between 
> > > > having a fdma instance DT property, or having lots of compatibles where the only
> > > > difference is the appending of the instance number. I think out of the two I prefer
> > > > my original approach.
> > > > 
> > > > Any thoughts from the DT folks?
> > > 
> > > To me both approaches sound wrong: basing the firmware name on the instance
> > > number requires that each instance is always used in the same way, which
> > > is not guaranteed to be the case,
> > 
> > Does it? I didn't think it did.
> > 
> > Using the instance number as a DT property defers the decision over what firmware to
> > load to the driver, which can choose whatever firmware name it wishes.
> > 
> > e.g. in v4.3 it could load xyz.elf, in v4.4 it could choose abc.elf. The DT will remain
> > unchanged, but the use of that fdma instance has changed.
> > 
> > We currently only have one firmware for each instance with the "use" compiled into it.
> > If in the future we had two firmwares with different "uses" for the same instance some extra
> > logic would be required in the driver to make a decision on which firmware to load.
> 
> Ok, I probably need some more background about what the firmware on this
> device does, and what it could do with a different firmware. Could you
> elaborate?

So the fdma hw is a dma engine based around a xp70 slim core. It supports: -
* block memory move between 2 system locations
* paced transfer from system memory to paced peripheral
* paced transfer from a paced peripheral to system memory

I believe each firmware for each instance supports those 3 things.

However the xp70 also has some ancilary HW to facilitate Start Code Detection.
It is this feature which I believe would require a different firmware if we wanted to make
use of it. Looking at the functional spec each xp70 also
has 16 gp output signals which we could also control from the firmware. Whether
these are actually connected to anything useful inside the SoC I don't know.

> > > and you correctly describe the problem with
> > > using the compatible string for the firmware name if the driver for the FDMA
> > > does not actually care what firmware is being used here.
> > > 
> > > Whatever code makes the decision as to how the FDMA is used should also
> > > decide on the name of the firmware file.
> > 
> > The code which makes this decision currently is the st_fdma.c driver. However it does
> > need to know which fdma controller it is operating on to make this decision correctly.
> > 
> > Apart from passing the fdma instance number in DT, how else can we determine which
> > controller we are?
> > 
> > I guess we could infer it by having a table in the driver containing the base addresses
> > of the controllers for a given SoC, and match that against what DT passes us in the
> > reg property. But that seems ugly, and is encoding the same information in two
> > different places.
> > 
> > I'm open to suggestions if there is a better way to do this.
> 
> Using the address would be the same thing, that doesn't change the
> fundamental logic. Can you explain why it matters which instance
> a firmware is used on for this driver?

The reason we care, is that each instance has its own firmware file.

I just did a hexdump on the 3 different firmwares and compared them. Although the majority
of the binary is the same, there are various bytes which change at several different offsets
in the firmware file depending on the instance.

I don't have a xp70 toolchain or know enough about the cpu architecture to analyze what exactly
the firmware is doing at these locations.

regards,

Peter.


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann Sept. 29, 2015, 2:15 p.m. UTC | #9
On Tuesday 29 September 2015 14:42:15 Peter Griffin wrote:
> On Tue, 29 Sep 2015, Arnd Bergmann wrote:
> > On Tuesday 29 September 2015 13:11:55 Peter Griffin wrote:
> > > Does it? I didn't think it did.
> > > 
> > > Using the instance number as a DT property defers the decision over what firmware to
> > > load to the driver, which can choose whatever firmware name it wishes.
> > > 
> > > e.g. in v4.3 it could load xyz.elf, in v4.4 it could choose abc.elf. The DT will remain
> > > unchanged, but the use of that fdma instance has changed.
> > > 
> > > We currently only have one firmware for each instance with the "use" compiled into it.
> > > If in the future we had two firmwares with different "uses" for the same instance some extra
> > > logic would be required in the driver to make a decision on which firmware to load.
> > 
> > Ok, I probably need some more background about what the firmware on this
> > device does, and what it could do with a different firmware. Could you
> > elaborate?
> 
> So the fdma hw is a dma engine based around a xp70 slim core. It supports: -
> * block memory move between 2 system locations
> * paced transfer from system memory to paced peripheral
> * paced transfer from a paced peripheral to system memory
> 
> I believe each firmware for each instance supports those 3 things.

Ok

> However the xp70 also has some ancilary HW to facilitate Start Code Detection.
> It is this feature which I believe would require a different firmware if we wanted to make
> use of it. Looking at the functional spec each xp70 also
> has 16 gp output signals which we could also control from the firmware. Whether
> these are actually connected to anything useful inside the SoC I don't know.

I still don't understand what Start Code Detection is here.

> > > > and you correctly describe the problem with
> > > > using the compatible string for the firmware name if the driver for the FDMA
> > > > does not actually care what firmware is being used here.
> > > > 
> > > > Whatever code makes the decision as to how the FDMA is used should also
> > > > decide on the name of the firmware file.
> > > 
> > > The code which makes this decision currently is the st_fdma.c driver. However it does
> > > need to know which fdma controller it is operating on to make this decision correctly.
> > > 
> > > Apart from passing the fdma instance number in DT, how else can we determine which
> > > controller we are?
> > > 
> > > I guess we could infer it by having a table in the driver containing the base addresses
> > > of the controllers for a given SoC, and match that against what DT passes us in the
> > > reg property. But that seems ugly, and is encoding the same information in two
> > > different places.
> > > 
> > > I'm open to suggestions if there is a better way to do this.
> > 
> > Using the address would be the same thing, that doesn't change the
> > fundamental logic. Can you explain why it matters which instance
> > a firmware is used on for this driver?
> 
> The reason we care, is that each instance has its own firmware file.
> 
> I just did a hexdump on the 3 different firmwares and compared them. Although the majority
> of the binary is the same, there are various bytes which change at several different offsets
> in the firmware file depending on the instance.
> 
> I don't have a xp70 toolchain or know enough about the cpu architecture to analyze what exactly
> the firmware is doing at these locations.

This sounds like we should indeed treat them as different things: We don't
know what the difference is, but we know that each of them needs a different
firmware, and presumably if you get a new SoC variant, it will in turn
need three new ones.

In this case, I'd say using the compatible string to identify them is best,
using whatever the SoC documentation uses to describe them. You can use the
of_device_id data fields to look up the firmware name then.

If the output signals end up being connected to something we want to
control through the firmware, that also makes sense for a new compatible
string, as the driver needs to know about the feature in order to
communicate with the firmware, and the DT needs to be able to describe
the pins (e.g. by making the node act as a GPIO controller) in a way that
requires a different string, too.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Peter Griffin Oct. 13, 2015, 11:18 a.m. UTC | #10
Hi Arnd,

On Tue, 29 Sep 2015, Arnd Bergmann wrote:

> On Tuesday 29 September 2015 14:42:15 Peter Griffin wrote:
> > On Tue, 29 Sep 2015, Arnd Bergmann wrote:
> > > On Tuesday 29 September 2015 13:11:55 Peter Griffin wrote:
> > > > Does it? I didn't think it did.
> > > > 
> > > > Using the instance number as a DT property defers the decision over what firmware to
> > > > load to the driver, which can choose whatever firmware name it wishes.
> > > > 
> > > > e.g. in v4.3 it could load xyz.elf, in v4.4 it could choose abc.elf. The DT will remain
> > > > unchanged, but the use of that fdma instance has changed.
> > > > 
> > > > We currently only have one firmware for each instance with the "use" compiled into it.
> > > > If in the future we had two firmwares with different "uses" for the same instance some extra
> > > > logic would be required in the driver to make a decision on which firmware to load.
> > > 
> > > Ok, I probably need some more background about what the firmware on this
> > > device does, and what it could do with a different firmware. Could you
> > > elaborate?
> > 
> > So the fdma hw is a dma engine based around a xp70 slim core. It supports: -
> > * block memory move between 2 system locations
> > * paced transfer from system memory to paced peripheral
> > * paced transfer from a paced peripheral to system memory
> > 
> > I believe each firmware for each instance supports those 3 things.
> 
> Ok
> 
> > However the xp70 also has some ancilary HW to facilitate Start Code Detection.
> > It is this feature which I believe would require a different firmware if we wanted to make
> > use of it. Looking at the functional spec each xp70 also
> > has 16 gp output signals which we could also control from the firmware. Whether
> > these are actually connected to anything useful inside the SoC I don't know.
> 
> I still don't understand what Start Code Detection is here.

I believe the "Start Code Detection" feature is referring to the 4 byte start code
found in packetized elementary stream (PES). So it is a A/V optimisation for the
DMA controller.

> 
> > > > > and you correctly describe the problem with
> > > > > using the compatible string for the firmware name if the driver for the FDMA
> > > > > does not actually care what firmware is being used here.
> > > > > 
> > > > > Whatever code makes the decision as to how the FDMA is used should also
> > > > > decide on the name of the firmware file.
> > > > 
> > > > The code which makes this decision currently is the st_fdma.c driver. However it does
> > > > need to know which fdma controller it is operating on to make this decision correctly.
> > > > 
> > > > Apart from passing the fdma instance number in DT, how else can we determine which
> > > > controller we are?
> > > > 
> > > > I guess we could infer it by having a table in the driver containing the base addresses
> > > > of the controllers for a given SoC, and match that against what DT passes us in the
> > > > reg property. But that seems ugly, and is encoding the same information in two
> > > > different places.
> > > > 
> > > > I'm open to suggestions if there is a better way to do this.
> > > 
> > > Using the address would be the same thing, that doesn't change the
> > > fundamental logic. Can you explain why it matters which instance
> > > a firmware is used on for this driver?
> > 
> > The reason we care, is that each instance has its own firmware file.
> > 
> > I just did a hexdump on the 3 different firmwares and compared them. Although the majority
> > of the binary is the same, there are various bytes which change at several different offsets
> > in the firmware file depending on the instance.
> > 
> > I don't have a xp70 toolchain or know enough about the cpu architecture to analyze what exactly
> > the firmware is doing at these locations.
> 
> This sounds like we should indeed treat them as different things: We don't
> know what the difference is, but we know that each of them needs a different
> firmware, and presumably if you get a new SoC variant, it will in turn
> need three new ones.

Yes I believe so.

> 
> In this case, I'd say using the compatible string to identify them is best,
> using whatever the SoC documentation uses to describe them. You can use the
> of_device_id data fields to look up the firmware name then.

Ok, I  will implement like this in the next version.

> 
> If the output signals end up being connected to something we want to
> control through the firmware, that also makes sense for a new compatible
> string, as the driver needs to know about the feature in order to
> communicate with the firmware, and the DT needs to be able to describe
> the pins (e.g. by making the node act as a GPIO controller) in a way that
> requires a different string, too.

Yes I agree, thanks for your review and comments :)

regards,

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

Patch

diff --git a/Documentation/devicetree/bindings/dma/st_fdma.txt b/Documentation/devicetree/bindings/dma/st_fdma.txt
new file mode 100644
index 0000000..c24b8d7
--- /dev/null
+++ b/Documentation/devicetree/bindings/dma/st_fdma.txt
@@ -0,0 +1,78 @@ 
+* STMicroelectronics Flexible Direct Memory Access Device Tree bindings
+
+The FDMA is a general-purpose direct memory access controller capable of
+supporting 16 independent DMA channels. It accepts up to 32 DMA requests.
+The FDMA is based on a Slim processor which require a firmware.
+
+* FDMA Controller
+
+Required properties:
+- compatible	: Should be "st,stih407-fdma-mpe31"
+- reg		: Should contain DMA registers location and length
+- interrupts	: Should contain one interrupt shared by all channels
+- dma-channels	: Number of channels supported by the controller
+- #dma-cells	: Must be <3>. See DMA client section below
+- st,fdma-id	: Must contain fdma controller number
+- clocks	: Must contain an entry for each name in clock-names
+- clock-names	: Must contain "fdma_slim, fdma_hi, fdma_low, fdma_ic" entries
+See: Documentation/devicetree/bindings/clock/clock-bindings.txt
+
+
+Example:
+
+	fdma1: fdma-app@8e40000 {
+		compatible = "st,stih407-fdma-mpe31";
+		reg = <0x8e40000 0x20000>;
+		interrupts = <GIC_SPI 7 IRQ_TYPE_NONE>;
+		dma-channels = <16>;
+		#dma-cells = <3>;
+		st,fdma-id = <0>;
+		clocks = <&CLK_S_C0_FLEXGEN CLK_FDMA>,
+			 <&CLK_S_C0_FLEXGEN CLK_TX_ICN_DMU>,
+			 <&CLK_S_C0_FLEXGEN CLK_TX_ICN_DMU>,
+			 <&CLK_S_C0_FLEXGEN CLK_EXT2F_A9>;
+		clock-names = "fdma_slim",
+			      "fdma_hi",
+			      "fdma_low",
+			      "fdma_ic";
+		};
+
+* DMA client
+
+Required properties:
+- dmas: Comma separated list of dma channel requests
+- dma-names: Names of the aforementioned requested channels
+
+Each dmas request consists of 4 cells:
+1. A phandle pointing to the FDMA controller
+2. The request line number
+3. A 32bit mask specifying (see include/linux/platform_data/dma-st-fdma.h)
+ -bit 2-0: Holdoff value, dreq will be masked for
+	0x0: 0-0.5us
+	0x1: 0.5-1us
+	0x2: 1-1.5us
+ -bit 17: data swap
+	0x0: disabled
+	0x1: enabled
+ -bit 21: Increment Address
+	0x0: no address increment between transfers
+	0x1: increment address between transfers
+ -bit 22: 2 STBus Initiator Coprocessor interface
+	0x0: high priority port
+	0x1: low priority port
+4. transfers type
+ 0 free running
+ 1 paced
+
+Example:
+
+	snd_uni_player2: snd-uni-player@2 {
+		compatible = "st,snd_uni_player";
+		status = "okay";
+		reg = <0x8D82000  0x158>;
+		interrupts = <GIC_SPI 86 IRQ_TYPE_NONE>;
+		version = <5>;
+		dmas = <&fdma0 4 0 1>;
+		dma-names = "tx";
+		description = "Uni Player #2 (DAC)";
+	};