| Message ID | 1461236675-10176-10-git-send-email-peter.griffin@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thursday 21 April 2016 12:04:26 Peter Griffin wrote: > uniperiph-id, version and mode are ST specific bindings and > need the 'st,' prefix. Update the examples, as otherwise copying > them yields a runtime error parsing the DT node. > > Signed-off-by: Peter Griffin <peter.griffin@linaro.org> > Cc: arnaud.pouliquen@st.com > --- > .../devicetree/bindings/sound/st,sti-asoc-card.txt | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/Documentation/devicetree/bindings/sound/st,sti-asoc-card.txt b/Documentation/devicetree/bindings/sound/st,sti-asoc-card.txt > index 028fa1c..ef2e0c6 100644 > --- a/Documentation/devicetree/bindings/sound/st,sti-asoc-card.txt > +++ b/Documentation/devicetree/bindings/sound/st,sti-asoc-card.txt > @@ -67,9 +67,9 @@ Example: > dmas = <&fdma0 4 0 1>; > dai-name = "Uni Player #1 (DAC)"; > dma-names = "tx"; > - uniperiph-id = <2>; > - version = <5>; > - mode = "PCM"; > + st,uniperiph-id = <2>; > + st,version = <5>; > + st,mode = "PCM"; > }; You don't change the binding desciption here, only the example, so they no longer match. What is st,uniperiph-id needed for anyway? It's often an indication that you are doing something wrong if you need this. Arnd
On Thu, Apr 21, 2016 at 12:04:26PM +0100, Peter Griffin wrote: > uniperiph-id, version and mode are ST specific bindings and > need the 'st,' prefix. Update the examples, as otherwise copying > them yields a runtime error parsing the DT node. I'm not sure what connection this or the other ASoC documentation update have to the rest of the series?
Hi Arnd, On Thu, 21 Apr 2016, Arnd Bergmann wrote: > On Thursday 21 April 2016 12:04:26 Peter Griffin wrote: > > uniperiph-id, version and mode are ST specific bindings and > > need the 'st,' prefix. Update the examples, as otherwise copying > > them yields a runtime error parsing the DT node. > > > > Signed-off-by: Peter Griffin <peter.griffin@linaro.org> > > Cc: arnaud.pouliquen@st.com > > --- > > .../devicetree/bindings/sound/st,sti-asoc-card.txt | 14 +++++++------- > > 1 file changed, 7 insertions(+), 7 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/sound/st,sti-asoc-card.txt b/Documentation/devicetree/bindings/sound/st,sti-asoc-card.txt > > index 028fa1c..ef2e0c6 100644 > > --- a/Documentation/devicetree/bindings/sound/st,sti-asoc-card.txt > > +++ b/Documentation/devicetree/bindings/sound/st,sti-asoc-card.txt > > @@ -67,9 +67,9 @@ Example: > > dmas = <&fdma0 4 0 1>; > > dai-name = "Uni Player #1 (DAC)"; > > dma-names = "tx"; > > - uniperiph-id = <2>; > > - version = <5>; > > - mode = "PCM"; > > + st,uniperiph-id = <2>; > > + st,version = <5>; > > + st,mode = "PCM"; > > }; > > You don't change the binding desciption here, only the example, > so they no longer match. Whoops. Will fix that in v4. > > What is st,uniperiph-id needed for anyway? It's often an indication > that you are doing something wrong if you need this. From looking at the code in sound/soc/sti/uniperif_player.c, there is one sysconf register called "Audio glue config" which is shared by all of the uniperif IP instances. This binding is being used to generate a bitoffset into this shared register based on the instance of the IP. I guess the alternative is to have an explosion of compatibles? st,sti-uni-player-1 st,sti-uni-player-2 st,sti-uni-player-3 If not what would you recommend instead? :-) I don't currently have access to the functional spec for this IP block, but I have asked ST to send it to me to see if there is any other way to derive this information (although I suspect there won't be). FYI I didn't actually write or upstream that driver. However fdma is a depedency of the ASoC driver and it is required to get working audio upstream. It's also worth pointing out that this ASoC driver has been merged for a while, so I'm not sure what your opinion is of now changing the DT bindings? Obviously it is currently not used by anyone upstream due to the missing fdma depedency. regards, Peter.
On Tuesday 26 April 2016 11:11:36 Peter Griffin wrote: > Hi Arnd, > > On Thu, 21 Apr 2016, Arnd Bergmann wrote: > > > On Thursday 21 April 2016 12:04:26 Peter Griffin wrote: > > > uniperiph-id, version and mode are ST specific bindings and > > > need the 'st,' prefix. Update the examples, as otherwise copying > > > them yields a runtime error parsing the DT node. > > > > > > Signed-off-by: Peter Griffin <peter.griffin@linaro.org> > > > Cc: arnaud.pouliquen@st.com > > > --- > > > .../devicetree/bindings/sound/st,sti-asoc-card.txt | 14 +++++++------- > > > 1 file changed, 7 insertions(+), 7 deletions(-) > > > > > > diff --git a/Documentation/devicetree/bindings/sound/st,sti-asoc-card.txt b/Documentation/devicetree/bindings/sound/st,sti-asoc-card.txt > > > index 028fa1c..ef2e0c6 100644 > > > --- a/Documentation/devicetree/bindings/sound/st,sti-asoc-card.txt > > > +++ b/Documentation/devicetree/bindings/sound/st,sti-asoc-card.txt > > > @@ -67,9 +67,9 @@ Example: > > > dmas = <&fdma0 4 0 1>; > > > dai-name = "Uni Player #1 (DAC)"; > > > dma-names = "tx"; > > > - uniperiph-id = <2>; > > > - version = <5>; > > > - mode = "PCM"; > > > + st,uniperiph-id = <2>; > > > + st,version = <5>; > > > + st,mode = "PCM"; > > > }; > > > > You don't change the binding desciption here, only the example, > > so they no longer match. > > Whoops. Will fix that in v4. > > > > > What is st,uniperiph-id needed for anyway? It's often an indication > > that you are doing something wrong if you need this. > > From looking at the code in sound/soc/sti/uniperif_player.c, there is > one sysconf register called "Audio glue config" which is shared by all > of the uniperif IP instances. This binding is being used to generate a > bitoffset into this shared register based on the instance of the IP. > > I guess the alternative is to have an explosion of compatibles? > > st,sti-uni-player-1 > st,sti-uni-player-2 > st,sti-uni-player-3 > That would certainly be worse: if the devices are actually identical, they should have the same compatible string. > If not what would you recommend instead? :-) It's still not clear to me what that bit in the syscfg register is for. Given the error message about "sti-audio-clk-glue", I suspect that this is actually a clock controller and that it should be using the clock binding with a separate driver instead of manipulating the regmap directly from the audio driver. Arnd
Hi Mark, On Thu, 21 Apr 2016, Mark Brown wrote: > On Thu, Apr 21, 2016 at 12:04:26PM +0100, Peter Griffin wrote: > > uniperiph-id, version and mode are ST specific bindings and > > need the 'st,' prefix. Update the examples, as otherwise copying > > them yields a runtime error parsing the DT node. > > I'm not sure what connection this or the other ASoC documentation update > have to the rest of the series? The ASoC DT bindings are missing upstream as the ASoC driver depends on the fdma driver. So this series adds the fdma driver, fdma dt bindings, and also the missing ASoC dt bindings. Whilst doing this and getting it working I noticed a few discrepencies in the ASoC dt binding documentation versus the driver so also fixed that. Once this whole series is applied the end result is working audio upstream for STi platforms (as ASoC is the only upstream driver using fdma currently). regards, Peter.
Hi Arnd, On Tue, 26 Apr 2016, Arnd Bergmann wrote: > On Tuesday 26 April 2016 11:11:36 Peter Griffin wrote: > > Hi Arnd, > > > > On Thu, 21 Apr 2016, Arnd Bergmann wrote: > > > > > On Thursday 21 April 2016 12:04:26 Peter Griffin wrote: > > > > uniperiph-id, version and mode are ST specific bindings and > > > > need the 'st,' prefix. Update the examples, as otherwise copying > > > > them yields a runtime error parsing the DT node. > > > > > > > > Signed-off-by: Peter Griffin <peter.griffin@linaro.org> > > > > Cc: arnaud.pouliquen@st.com > > > > --- > > > > .../devicetree/bindings/sound/st,sti-asoc-card.txt | 14 +++++++------- > > > > 1 file changed, 7 insertions(+), 7 deletions(-) > > > > > > > > diff --git a/Documentation/devicetree/bindings/sound/st,sti-asoc-card.txt b/Documentation/devicetree/bindings/sound/st,sti-asoc-card.txt > > > > index 028fa1c..ef2e0c6 100644 > > > > --- a/Documentation/devicetree/bindings/sound/st,sti-asoc-card.txt > > > > +++ b/Documentation/devicetree/bindings/sound/st,sti-asoc-card.txt > > > > @@ -67,9 +67,9 @@ Example: > > > > dmas = <&fdma0 4 0 1>; > > > > dai-name = "Uni Player #1 (DAC)"; > > > > dma-names = "tx"; > > > > - uniperiph-id = <2>; > > > > - version = <5>; > > > > - mode = "PCM"; > > > > + st,uniperiph-id = <2>; > > > > + st,version = <5>; > > > > + st,mode = "PCM"; > > > > }; > > > > > > You don't change the binding desciption here, only the example, > > > so they no longer match. > > > > Whoops. Will fix that in v4. > > > > > > > > What is st,uniperiph-id needed for anyway? It's often an indication > > > that you are doing something wrong if you need this. > > > > From looking at the code in sound/soc/sti/uniperif_player.c, there is > > one sysconf register called "Audio glue config" which is shared by all > > of the uniperif IP instances. This binding is being used to generate a > > bitoffset into this shared register based on the instance of the IP. > > > > I guess the alternative is to have an explosion of compatibles? > > > > st,sti-uni-player-1 > > st,sti-uni-player-2 > > st,sti-uni-player-3 > > > > That would certainly be worse: if the devices are actually identical, > they should have the same compatible string. > > > If not what would you recommend instead? :-) > > It's still not clear to me what that bit in the syscfg register > is for. Given the error message about "sti-audio-clk-glue", > I suspect that this is actually a clock controller and that > it should be using the clock binding with a separate driver > instead of manipulating the regmap directly from the audio driver. Luckily I do have the datasheet for the audio-glue sysconf register. It says: - [11:8] PCM_CLK_SEL: Selects the frequency synthesizer clock or the external PCM clock for each channel. The driver only ever sets this to 1 which selects the frequency synthesizer clock. So the bitfield of the register which the driver is using (PCM_CLK_SEL) is a clock mux. Peter.
On Tuesday 26 April 2016 12:15:32 Peter Griffin wrote: > > > > > If not what would you recommend instead? > > > > It's still not clear to me what that bit in the syscfg register > > is for. Given the error message about "sti-audio-clk-glue", > > I suspect that this is actually a clock controller and that > > it should be using the clock binding with a separate driver > > instead of manipulating the regmap directly from the audio driver. > > Luckily I do have the datasheet for the audio-glue sysconf register. > > It says: - > > [11:8] PCM_CLK_SEL: Selects the frequency synthesizer clock or the external > PCM clock for each channel. > > The driver only ever sets this to 1 which selects the frequency synthesizer > clock. So the bitfield of the register which the driver is using (PCM_CLK_SEL) > is a clock mux. Ok, that sounds like it could be either a really simple clock driver with just a few lines, or integrated into an existing clock driver if you already have one for this syscon node. Arnd
On 04/26/2016 01:02 PM, Peter Griffin wrote: > Hi Mark, > > On Thu, 21 Apr 2016, Mark Brown wrote: > >> On Thu, Apr 21, 2016 at 12:04:26PM +0100, Peter Griffin wrote: >>> uniperiph-id, version and mode are ST specific bindings and >>> need the 'st,' prefix. Update the examples, as otherwise copying >>> them yields a runtime error parsing the DT node. >> >> I'm not sure what connection this or the other ASoC documentation update >> have to the rest of the series? > > The ASoC DT bindings are missing upstream as the ASoC driver depends on the > fdma driver. So this series adds the fdma driver, fdma dt bindings, > and also the missing ASoC dt bindings. > > Whilst doing this and getting it working I noticed a few discrepencies > in the ASoC dt binding documentation versus the driver so also fixed that. > > Once this whole series is applied the end result is working > audio upstream for STi platforms (as ASoC is the only upstream driver > using fdma currently). Hi Peter, I also noticed the error in bindings, i'm planning to correct it with DTS upstream for STI ASoC after FDMA has been accepted. So if you estimate that it is simplier for FDMA upstream, you can drop ASoC patches in you next version. Regards, Arnaud
hello Arnd, peter, On 04/26/2016 01:44 PM, Arnd Bergmann wrote: > On Tuesday 26 April 2016 12:15:32 Peter Griffin wrote: >>> >>>> If not what would you recommend instead? >>> >>> It's still not clear to me what that bit in the syscfg register >>> is for. Given the error message about "sti-audio-clk-glue", >>> I suspect that this is actually a clock controller and that >>> it should be using the clock binding with a separate driver >>> instead of manipulating the regmap directly from the audio driver. >> >> Luckily I do have the datasheet for the audio-glue sysconf register. >> >> It says: - >> >> [11:8] PCM_CLK_SEL: Selects the frequency synthesizer clock or the external >> PCM clock for each channel. >> >> The driver only ever sets this to 1 which selects the frequency synthesizer >> clock. So the bitfield of the register which the driver is using (PCM_CLK_SEL) >> is a clock mux. > > Ok, that sounds like it could be either a really simple clock driver > with just a few lines, or integrated into an existing clock driver > if you already have one for this syscon node. > > Arnd > FYI, Name of this glue is related to the register name. But it does not concern only clock... This glue register is used to : - select clock source ( clock framework or external clock from GPIO) => one bit field per IP instance (player->clk_sel) - select uniperiph player IP instance for PCM out. (http://www.spinics.net/lists/alsa-devel/msg49034.html) Regards Arnaud
On Wednesday 04 May 2016 09:52:19 Arnaud Pouliquen wrote: > hello Arnd, peter, > > On 04/26/2016 01:44 PM, Arnd Bergmann wrote: > > On Tuesday 26 April 2016 12:15:32 Peter Griffin wrote: > >>> > >>>> If not what would you recommend instead? > >>> > >>> It's still not clear to me what that bit in the syscfg register > >>> is for. Given the error message about "sti-audio-clk-glue", > >>> I suspect that this is actually a clock controller and that > >>> it should be using the clock binding with a separate driver > >>> instead of manipulating the regmap directly from the audio driver. > >> > >> Luckily I do have the datasheet for the audio-glue sysconf register. > >> > >> It says: - > >> > >> [11:8] PCM_CLK_SEL: Selects the frequency synthesizer clock or the external > >> PCM clock for each channel. > >> > >> The driver only ever sets this to 1 which selects the frequency synthesizer > >> clock. So the bitfield of the register which the driver is using (PCM_CLK_SEL) > >> is a clock mux. > > > > Ok, that sounds like it could be either a really simple clock driver > > with just a few lines, or integrated into an existing clock driver > > if you already have one for this syscon node. > > > > Arnd > > > FYI, Name of this glue is related to the register name. But it does not > concern only clock... > This glue register is used to : > - select clock source ( clock framework or external clock from GPIO) > => one bit field per IP instance (player->clk_sel) > - select uniperiph player IP instance for PCM out. > (http://www.spinics.net/lists/alsa-devel/msg49034.html) Ok, I see. This is of course again the STi platform being a bit different from everyone else, and whatever we do to hide it won't give us a nice abstraction. Having just a clock driver for the register won't do the job here as you say, so I guess the original patch was already the least awkward way to handle it. Arnd
diff --git a/Documentation/devicetree/bindings/sound/st,sti-asoc-card.txt b/Documentation/devicetree/bindings/sound/st,sti-asoc-card.txt index 028fa1c..ef2e0c6 100644 --- a/Documentation/devicetree/bindings/sound/st,sti-asoc-card.txt +++ b/Documentation/devicetree/bindings/sound/st,sti-asoc-card.txt @@ -67,9 +67,9 @@ Example: dmas = <&fdma0 4 0 1>; dai-name = "Uni Player #1 (DAC)"; dma-names = "tx"; - uniperiph-id = <2>; - version = <5>; - mode = "PCM"; + st,uniperiph-id = <2>; + st,version = <5>; + st,mode = "PCM"; }; sti_uni_player3: sti-uni-player@3 { @@ -83,9 +83,9 @@ Example: dmas = <&fdma0 7 0 1>; dma-names = "tx"; dai-name = "Uni Player #1 (PIO)"; - uniperiph-id = <3>; - version = <5>; - mode = "SPDIF"; + st,uniperiph-id = <3>; + st,version = <5>; + st,mode = "SPDIF"; }; sti_uni_reader1: sti-uni-reader@1 { @@ -98,7 +98,7 @@ Example: dmas = <&fdma0 6 0 1>; dma-names = "rx"; dai-name = "Uni Reader #1 (HDMI RX)"; - version = <3>; + st,version = <3>; }; 2) sti-sas-codec: internal audio codec IPs driver
uniperiph-id, version and mode are ST specific bindings and need the 'st,' prefix. Update the examples, as otherwise copying them yields a runtime error parsing the DT node. Signed-off-by: Peter Griffin <peter.griffin@linaro.org> Cc: arnaud.pouliquen@st.com --- .../devicetree/bindings/sound/st,sti-asoc-card.txt | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)