| Message ID | 1502748417-26417-3-git-send-email-lori.hikichi@broadcom.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Aug 14, 2017 at 03:06:50PM -0700, Lori Hikichi wrote:
> Allow each audio port to select which clock (if any) it wants to use.
Why is this in DT for the port and not either using standard clock
bindings to configure the clock tree or allowing the machine driver to
pick?
On 8/15/2017 10:14 AM, Mark Brown wrote: > On Mon, Aug 14, 2017 at 03:06:50PM -0700, Lori Hikichi wrote: >> Allow each audio port to select which clock (if any) it wants to use. > Why is this in DT for the port and not either using standard clock > bindings to configure the clock tree or allowing the machine driver to > pick? The previous version of the driver essentially had a clock mapping that could not be changed. This is fine for 99% of our use cases. If we need to change the mapping, then we need to modify the audio port's clock mux. Creating a clock for these muxes was going to be messy. There is a mux per audio port and the registers used to program the muxes are staggered throughout the io space used by the audio driver. Additionally, these registers have bits that are controlled by the audio driver. Had I used syscon to access these registers this would have resulted in a very fragmented io space, complicating the audio drivers access this space. I have put the mux assignment in DT because the assignment is a static property and did not need run time programmability from the machine driver. Lori.
On Tue, Aug 15, 2017 at 12:29:44PM -0700, Lori Hikichi wrote: > I have put the mux assignment in DT because the assignment is a > static property and did not need run time programmability from the > machine driver. Why is this a static property, what prevents something wanting to change things at runtime? You might be running with simpler setups now but perhaps you'll run into a more complex use case later?
On 8/16/2017 3:59 AM, Mark Brown wrote: > On Tue, Aug 15, 2017 at 12:29:44PM -0700, Lori Hikichi wrote: > >> I have put the mux assignment in DT because the assignment is a >> static property and did not need run time programmability from the >> machine driver. > Why is this a static property, what prevents something wanting to change > things at runtime? You might be running with simpler setups now but > perhaps you'll run into a more complex use case later? The short answer is I have analyzed the possible use cases for Cygnus' audio block, and nothing should need to change the assignments at runtime. The longer explanation follows. The clocking configuration is this. There is one pll with its output run through 3 post dividers. The audio ports can select one of these outputs. There are only 5 possible consumers of these 3 clocks. The 3 i2s/tdm ports, 1 spdif port, and an exceptional case of another "non-audio" IP block. For the i2s/tdm ports this clock is the MCLK. By far the most common usage case for Cygnus is a configuration which uses only the three i2s/tdm ports. In this case each port is assigned a clock. Each clock has the same capabilities so there would never be a reason change the static mapping. Now for the case when the "non-audio block" uses one of these clocks. In this situation we will only need one i2s port because this configuration of the chip is not audio intensive. When the system is designed we know if this non-audio block will be in use, it is not a runtime configurable thing. Again, a static mapping is fine. The only situation which could get more complex is with SPDIF. First off, the SPDIF port is not actively used in any current configuration and I do not think there are any plans for it to be used. But, we are talking about possible future configurations. The only limitation the current static scheme would introduce is if SPDIF is active along with all 3 i2s ports. Additionally, all 3 of the i2s ports would need to be in master mode (slave mode would free up a clock for SPDIF). In this case, two of the clock consumers would need to share a clock. In this situation I envisioned that both consumers would agree on a fixed rate and work within those limitation. For example, the ports would choose 24.576 MHz as their mclk and be limited to the the frame rates that could be derived from that clock. At the time it did not seem necessary to make addition driver changes to support a use case that will very likely never arise. As it turns out, we are working on a new version of this audio block. The clocking configuration for this new version is more complex and I am already in the process of creating clock bindings for all this this. I am hopeful that the driver for this new version will be applicable to Cygnus. Lori.
On Wed, Aug 16, 2017 at 12:39:42PM -0700, Lori Hikichi wrote: > By far the most common usage case for Cygnus is a configuration which > uses only the three i2s/tdm ports. In this case each port is assigned > a clock. Each clock has the same capabilities so there would never be > a reason change the static mapping. The usual reason would be to bring things into sync. > Now for the case when the "non-audio block" uses one of these clocks. > In this situation we will only need one i2s port because this > configuration of the chip is not audio intensive. When the system > is designed we know if this non-audio block will be in use, it is not > a runtime configurable thing. Again, a static mapping is fine. Is this limitation when the other block is in use a physical limitation or is it just a case of not seeing the use case. > At the time it did not seem necessary to make addition driver changes to > support a use case that will very likely never arise. As it turns out, > we are working on a new version of this audio block. The clocking > configuration for this new version is more complex and I am already in > the process of creating clock bindings for all this this. I am hopeful > that the driver for this new version will be applicable to Cygnus. If the clocking is more complex that seems like even more reason to not fix this in the binding, and possibly to do as I think I suggested earlier and use the common clock bindings to manage this rather than doing something custom and driver specific.
On 8/22/2017 9:07 AM, Mark Brown wrote: > On Wed, Aug 16, 2017 at 12:39:42PM -0700, Lori Hikichi wrote: > >> By far the most common usage case for Cygnus is a configuration which >> uses only the three i2s/tdm ports. In this case each port is assigned >> a clock. Each clock has the same capabilities so there would never be >> a reason change the static mapping. > The usual reason would be to bring things into sync. > >> Now for the case when the "non-audio block" uses one of these clocks. >> In this situation we will only need one i2s port because this >> configuration of the chip is not audio intensive. When the system >> is designed we know if this non-audio block will be in use, it is not >> a runtime configurable thing. Again, a static mapping is fine. > Is this limitation when the other block is in use a physical limitation > or is it just a case of not seeing the use case. > >> At the time it did not seem necessary to make addition driver changes to >> support a use case that will very likely never arise. As it turns out, >> we are working on a new version of this audio block. The clocking >> configuration for this new version is more complex and I am already in >> the process of creating clock bindings for all this this. I am hopeful >> that the driver for this new version will be applicable to Cygnus. > If the clocking is more complex that seems like even more reason to not > fix this in the binding, and possibly to do as I think I suggested > earlier and use the common clock bindings to manage this rather than > doing something custom and driver specific. Ok, I will create the necessary clocks to allow this to work. This will likely impact a couple of the other patches in the series, but most should be applicable. What would be the best way to have some of the other patches in this series reviewed? Should I just drop this series and resubmit new individual patches for review.
On Wed, Sep 06, 2017 at 06:45:22PM -0700, Lori Hikichi wrote: > This will likely impact a couple of the other patches in the series, but > most should be applicable. What would be the best way to have some of the > other patches in this series reviewed? Should I just drop this series and > resubmit new individual patches for review. Resubmit the ones that can be applied as is please, that'll avoid me making a mistake about which ones have the dependency.
diff --git a/Documentation/devicetree/bindings/sound/brcm,cygnus-audio.txt b/Documentation/devicetree/bindings/sound/brcm,cygnus-audio.txt index b139e66..2ef2f2c 100644 --- a/Documentation/devicetree/bindings/sound/brcm,cygnus-audio.txt +++ b/Documentation/devicetree/bindings/sound/brcm,cygnus-audio.txt @@ -9,19 +9,28 @@ Required properties: Valid names are "aud" and "i2s_in". "aud" contains a set of DMA, I2S_OUT and SPDIF registers. "i2s_in" contains a set of I2S_IN registers. - - clocks: PLL and leaf clocks used by audio ports - - assigned-clocks: PLL and leaf clocks - - assigned-clock-parents: parent clocks of the assigned clocks - (usually the PLL) - - assigned-clock-rates: List of clock frequencies of the - assigned clocks - - clock-names: names of 3 leaf clocks used by audio ports - Valid names are "ch0_audio", "ch1_audio", "ch2_audio" - interrupts: audio DMA interrupt number +Optional properties: + - assigned-clocks: only valid choice is audiopll + - assigned-clock-rates: clock frequency for audiopll +If none of the ports need an internal master clock then there no need to +initialize the pll clock. + + SSP Subnode properties: -- reg: The index of ssp port interface to use - Valid value are 0, 1, 2, or 3 (for spdif) +Required: + - reg: The index of ssp port interface to use + Valid value are 0, 1, 2, or 3 (for spdif) +Optional: + - clocks: clock used by audio port + one of the audiopll outputs (see brcm,iproc-clocks.txt). + - clock-names: Must be "ssp_clk" + - brcm,ssp-clk-mux = Needed if a clock is named and used. This value is + used to program the mux within the audio driver which selects + the incoming clock. Here is the mapping. + audio_pll output 0 = 0, output 1 = 1, and output 2 = 2 + Example: cygnus_audio: audio@180ae000 { @@ -30,38 +39,49 @@ Example: #size-cells = <0>; reg = <0x180ae000 0xafd>, <0x180aec00 0x1f8>; reg-names = "aud", "i2s_in"; - clocks = <&audiopll BCM_CYGNUS_AUDIOPLL_CH0>, - <&audiopll BCM_CYGNUS_AUDIOPLL_CH1>, - <&audiopll BCM_CYGNUS_AUDIOPLL_CH2>; - assigned-clocks = <&audiopll BCM_CYGNUS_AUDIOPLL>, - <&audiopll BCM_CYGNUS_AUDIOPLL_CH0>, - <&audiopll BCM_CYGNUS_AUDIOPLL_CH1>, - <&audiopll BCM_CYGNUS_AUDIOPLL_CH2>; - assigned-clock-parents = <&audiopll BCM_CYGNUS_AUDIOPLL>; - assigned-clock-rates = <1769470191>, - <0>, - <0>, - <0>; - clock-names = "ch0_audio", "ch1_audio", "ch2_audio"; + + assigned-clocks = <&audiopll BCM_CYGNUS_AUDIOPLL>; + assigned-clock-rates = <1376255989>; + interrupts = <GIC_SPI 143 IRQ_TYPE_LEVEL_HIGH>; ssp0: ssp_port@0 { reg = <0>; + + clocks = <&audiopll BCM_CYGNUS_AUDIOPLL_CH0>; + clock-names = "ssp_clk"; + brcm,ssp-clk-mux = <0>; + status = "okay"; }; ssp1: ssp_port@1 { reg = <1>; - status = "disabled"; + + clocks = <&audiopll BCM_CYGNUS_AUDIOPLL_CH1>; + clock-names = "ssp_clk"; + brcm,ssp-clk-mux = <1>; + + status = "okay"; }; ssp2: ssp_port@2 { reg = <2>; - status = "disabled"; + + clocks = <&audiopll BCM_CYGNUS_AUDIOPLL_CH2>; + clock-names = "ssp_clk"; + brcm,ssp-clk-mux = <2>; + + status = "okay"; }; spdif: spdif_port@3 { reg = <3>; + + clocks = <&audiopll BCM_CYGNUS_AUDIOPLL_CH2>; + clock-names = "ssp_clk"; + brcm,ssp-clk-mux = <2>; + status = "disabled"; }; };
Allow each audio port to select which clock (if any) it wants to use. Signed-off-by: Lori Hikichi <lori.hikichi@broadcom.com> --- .../bindings/sound/brcm,cygnus-audio.txt | 70 ++++++++++++++-------- 1 file changed, 45 insertions(+), 25 deletions(-)