| Message ID | 1424901482-3809-3-git-send-email-mw@semihalf.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 25.02.2015 22:58, Marcin Wojtas wrote: > This commit adds the description of the I2S controller to the Marvell > Armada 38x SoC's Device Tree, as well as its pin configuration. > > Signed-off-by: Marcin Wojtas <mw@semihalf.com> > --- > arch/arm/boot/dts/armada-38x.dtsi | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/arch/arm/boot/dts/armada-38x.dtsi b/arch/arm/boot/dts/armada-38x.dtsi > index 1dff30a..fa21cb5 100644 > --- a/arch/arm/boot/dts/armada-38x.dtsi > +++ b/arch/arm/boot/dts/armada-38x.dtsi [...] > @@ -555,6 +561,17 @@ > status = "disabled"; > }; > > + audio_controller: audio-controller@e8000 { > + #sound-dai-cells = <1>; > + compatible = "marvell,armada-380-audio"; > + reg = <0xe8000 0x4000>, <0x18410 0xc>, <0x18204 0x4>; > + reg-names = "i2s_regs", "pll_regs", "soc_ctrl"; Marcin, sorry but NACK. The PLL and SoC ctrl are not even close to the audio controller registers. > + interrupts = <GIC_SPI 75 IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&gateclk 0>; > + clock-names = "internal"; How about providing access to audio PLL by a clock driver and amend the binding to allow for a second more precise PLL clock, e.g. clocks = <&gateclk CLK_AUDIO>, <&pll PLL_AUDIO>; clock-names = "internal", "pll"; we already check for an "extclk" on Dove for the same reason but the name might be misleading here. Also, i2c/spdif muxing option could be handled by 38x's pinctrl driver, we have the same for Dove's internal i2c mux. If you want to use i2s you just add the option to the default pinctrl hog: pinctrl-0 = <&i2s_pins &audio_mux_i2s>; pinctrl-names = "default"; Sebastian > + status = "disabled"; > + }; > + > usb3@f0000 { > compatible = "marvell,armada-380-xhci"; > reg = <0xf0000 0x4000>,<0xf4000 0x4000>; >
Hi Sebastian, Thanks for your input. 2015-02-25 23:21 GMT+01:00 Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>: > On 25.02.2015 22:58, Marcin Wojtas wrote: >> >> This commit adds the description of the I2S controller to the Marvell >> Armada 38x SoC's Device Tree, as well as its pin configuration. >> >> Signed-off-by: Marcin Wojtas <mw@semihalf.com> >> --- >> arch/arm/boot/dts/armada-38x.dtsi | 17 +++++++++++++++++ >> 1 file changed, 17 insertions(+) >> >> diff --git a/arch/arm/boot/dts/armada-38x.dtsi >> b/arch/arm/boot/dts/armada-38x.dtsi >> index 1dff30a..fa21cb5 100644 >> --- a/arch/arm/boot/dts/armada-38x.dtsi >> +++ b/arch/arm/boot/dts/armada-38x.dtsi > > [...] >> >> @@ -555,6 +561,17 @@ >> status = "disabled"; >> }; >> >> + audio_controller: audio-controller@e8000 { >> + #sound-dai-cells = <1>; >> + compatible = "marvell,armada-380-audio"; >> + reg = <0xe8000 0x4000>, <0x18410 0xc>, >> <0x18204 0x4>; >> + reg-names = "i2s_regs", "pll_regs", >> "soc_ctrl"; > > > Marcin, > > sorry but NACK. The PLL and SoC ctrl are not even close to the audio > controller registers. > >> + interrupts = <GIC_SPI 75 >> IRQ_TYPE_LEVEL_HIGH>; >> + clocks = <&gateclk 0>; >> + clock-names = "internal"; > > > How about providing access to audio PLL by a clock driver and amend the > binding to allow for a second more precise PLL clock, e.g. > > clocks = <&gateclk CLK_AUDIO>, <&pll PLL_AUDIO>; > clock-names = "internal", "pll"; > Good idea. Would you suggest some name for the new driver? How about clk-audio.c under drivers/clk/mvebu? > we already check for an "extclk" on Dove for the same reason but the > name might be misleading here. > > Also, i2c/spdif muxing option could be handled by 38x's pinctrl driver, > we have the same for Dove's internal i2c mux. > > If you want to use i2s you just add the option to the default pinctrl > hog: > > pinctrl-0 = <&i2s_pins &audio_mux_i2s>; > pinctrl-names = "default"; > Ok, I'll try to extend pinctrl-armada-38x then. Marcin
On 26.02.2015 01:11, Marcin Wojtas wrote: > 2015-02-25 23:21 GMT+01:00 Sebastian Hesselbarth > <sebastian.hesselbarth@gmail.com>: >> On 25.02.2015 22:58, Marcin Wojtas wrote: [...] >> sorry but NACK. The PLL and SoC ctrl are not even close to the audio >> controller registers. >> >>> + interrupts = <GIC_SPI 75 >>> IRQ_TYPE_LEVEL_HIGH>; >>> + clocks = <&gateclk 0>; >>> + clock-names = "internal"; >> >> >> How about providing access to audio PLL by a clock driver and amend the >> binding to allow for a second more precise PLL clock, e.g. >> >> clocks = <&gateclk CLK_AUDIO>, <&pll PLL_AUDIO>; >> clock-names = "internal", "pll"; >> > > Good idea. Would you suggest some name for the new driver? How about > clk-audio.c under drivers/clk/mvebu? Depends on how much you know about the surrounding registers. If there is more clock related stuff than the audio pll, I'd suggest to have a single driver for everything that isn't core clk or clk gates which are already dealt with in the existing driver. Given that this PLL layout is most likely 38x specific, I'd just put it into drivers/clk/mvebu/armada-38x.c. >> we already check for an "extclk" on Dove for the same reason but the >> name might be misleading here. >> >> Also, i2c/spdif muxing option could be handled by 38x's pinctrl driver, >> we have the same for Dove's internal i2c mux. >> >> If you want to use i2s you just add the option to the default pinctrl >> hog: >> >> pinctrl-0 = <&i2s_pins &audio_mux_i2s>; >> pinctrl-names = "default"; >> > > Ok, I'll try to extend pinctrl-armada-38x then. IMHO, adding a "syscon" compatible to the system-controller node should do the trick. You can reference the regmap from 38x's pinctrl driver and have a specific callback for the i2s/spdif muxing. The reg property range of the system-controller should be extended but that definitely depends on what the you or the Free Electron guys know about the register set. Sebastian
2015-02-26 1:30 GMT+01:00 Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>: > On 26.02.2015 01:11, Marcin Wojtas wrote: >> >> 2015-02-25 23:21 GMT+01:00 Sebastian Hesselbarth >> <sebastian.hesselbarth@gmail.com>: >>> >>> On 25.02.2015 22:58, Marcin Wojtas wrote: > > [...] >>> >>> sorry but NACK. The PLL and SoC ctrl are not even close to the audio >>> controller registers. >>> >>>> + interrupts = <GIC_SPI 75 >>>> IRQ_TYPE_LEVEL_HIGH>; >>>> + clocks = <&gateclk 0>; >>>> + clock-names = "internal"; >>> >>> >>> >>> How about providing access to audio PLL by a clock driver and amend the >>> binding to allow for a second more precise PLL clock, e.g. >>> >>> clocks = <&gateclk CLK_AUDIO>, <&pll PLL_AUDIO>; >>> clock-names = "internal", "pll"; >>> >> >> Good idea. Would you suggest some name for the new driver? How about >> clk-audio.c under drivers/clk/mvebu? > > > Depends on how much you know about the surrounding registers. > > If there is more clock related stuff than the audio pll, I'd > suggest to have a single driver for everything that isn't core clk > or clk gates which are already dealt with in the existing driver. > > Given that this PLL layout is most likely 38x specific, I'd just put it > into drivers/clk/mvebu/armada-38x.c. > Audio PLL setting consist of 3 independent registers. Adjacent to them there are same regs for setting TDM PLL, however I doubt telephony is going to be supported in foreseeable future (if ever). I also have strong suspicion that it looks the same in a39x - Thomas, can you take a look into spec and confirm? >>> we already check for an "extclk" on Dove for the same reason but the >>> name might be misleading here. >>> >>> Also, i2c/spdif muxing option could be handled by 38x's pinctrl driver, >>> we have the same for Dove's internal i2c mux. >>> >>> If you want to use i2s you just add the option to the default pinctrl >>> hog: >>> >>> pinctrl-0 = <&i2s_pins &audio_mux_i2s>; >>> pinctrl-names = "default"; >>> >> >> Ok, I'll try to extend pinctrl-armada-38x then. > > > IMHO, adding a "syscon" compatible to the system-controller node should > do the trick. You can reference the regmap from 38x's pinctrl driver and > have a specific callback for the i2s/spdif muxing. The reg property > range of the system-controller should be extended but that definitely > depends on what the you or the Free Electron guys know about the > register set. > For muxing i2s/spdif (exactly the same pins, they have to be either way set to "audio" by pinctrl driver) changing single bit in "system control 1" register is needed. However currently unused, it is already a part of system-controller@18200 regmap. Anyway the will of using either i2s or spdif has to be explicitly represented in DT. Marcin
Dear Marcin Wojtas, On Thu, 26 Feb 2015 10:05:08 +0100, Marcin Wojtas wrote: > > Given that this PLL layout is most likely 38x specific, I'd just put it > > into drivers/clk/mvebu/armada-38x.c. > > Audio PLL setting consist of 3 independent registers. Adjacent to them > there are same regs for setting TDM PLL, however I doubt telephony is > going to be supported in foreseeable future (if ever). We might be supporting TDM in the future. However, I see that the TDM PLL registers are nicely grouped together, and so are the audio PLL registers. So maybe we can simply have a driver to handle the audio PLL registers for now, and if needed in the future, add another driver for the TDM PLL. > I also have strong suspicion that it looks the same in a39x - Thomas, > can you take a look into spec and confirm? As far as I can see there is no audio support and no TDM support in Armada 39x, and there those TDM PLL and audio PLL registers do not exist. Thomas
Dear Thomas, 2015-02-27 15:03 GMT+01:00 Thomas Petazzoni <thomas.petazzoni@free-electrons.com>: > Dear Marcin Wojtas, > > On Thu, 26 Feb 2015 10:05:08 +0100, Marcin Wojtas wrote: > >> > Given that this PLL layout is most likely 38x specific, I'd just put it >> > into drivers/clk/mvebu/armada-38x.c. >> >> Audio PLL setting consist of 3 independent registers. Adjacent to them >> there are same regs for setting TDM PLL, however I doubt telephony is >> going to be supported in foreseeable future (if ever). > > We might be supporting TDM in the future. However, I see that the TDM > PLL registers are nicely grouped together, and so are the audio PLL > registers. So maybe we can simply have a driver to handle the audio PLL > registers for now, and if needed in the future, add another driver for > the TDM PLL. I've got very fresh experience of adding TDM support for A38x, it definitely would require longer discussion. > >> I also have strong suspicion that it looks the same in a39x - Thomas, >> can you take a look into spec and confirm? > > As far as I can see there is no audio support and no TDM support in > Armada 39x, and there those TDM PLL and audio PLL registers do not > exist. > Given all the facts, do you think that drivers/clk/mvebu/armada-38x.c is a right place for adding this PLL setting support? Best regards, Marcin
Dear Marcin Wojtas, On Fri, 27 Feb 2015 21:22:30 +0100, Marcin Wojtas wrote: > Given all the facts, do you think that drivers/clk/mvebu/armada-38x.c > is a right place for adding this PLL setting support? Yes, sounds right to me. I was thinking that maybe it should be in a separate file armada-38x-pll.c, but that's probably a bit too much for something that is causing to be a simple driver. Have you sorted out how to handle the pin-muxing part of the problem? Best regards, Thomas
Hello Thomas, 2015-02-28 10:58 GMT+01:00 Thomas Petazzoni <thomas.petazzoni@free-electrons.com>: > Dear Marcin Wojtas, > > On Fri, 27 Feb 2015 21:22:30 +0100, Marcin Wojtas wrote: > >> Given all the facts, do you think that drivers/clk/mvebu/armada-38x.c >> is a right place for adding this PLL setting support? > > Yes, sounds right to me. I was thinking that maybe it should be in a > separate file armada-38x-pll.c, but that's probably a bit too much for > something that is causing to be a simple driver. > I've got an update from Marvell - TDM and Audio are available in A39x and it has the same set of PLL registers like A38x. Shouldn't we place PLL configuration code in a separate file like clk-pll.c, or whatever name you suggest? > Have you sorted out how to handle the pin-muxing part of the problem? > I'll come back with this issue after implementing poc. Best regards, Marcin
Dear Marcin Wojtas, On Wed, 4 Mar 2015 13:12:10 +0100, Marcin Wojtas wrote: > I've got an update from Marvell - TDM and Audio are available in A39x > and it has the same set of PLL registers like A38x. Shouldn't we place > PLL configuration code in a separate file like clk-pll.c, or whatever > name you suggest? Yes, seems OK to me. > > Have you sorted out how to handle the pin-muxing part of the problem? > > I'll come back with this issue after implementing poc. Ok, thanks! Thomas
diff --git a/arch/arm/boot/dts/armada-38x.dtsi b/arch/arm/boot/dts/armada-38x.dtsi index 1dff30a..fa21cb5 100644 --- a/arch/arm/boot/dts/armada-38x.dtsi +++ b/arch/arm/boot/dts/armada-38x.dtsi @@ -314,6 +314,12 @@ marvell,pins = "mpp44"; marvell,function = "sata3"; }; + + i2s_pins: i2s_pins { + marvell,pins = "mpp48", "mpp49", "mpp50", + "mpp51", "mpp52", "mpp53"; + marvell,function = "audio"; + }; }; gpio0: gpio@18100 { @@ -555,6 +561,17 @@ status = "disabled"; }; + audio_controller: audio-controller@e8000 { + #sound-dai-cells = <1>; + compatible = "marvell,armada-380-audio"; + reg = <0xe8000 0x4000>, <0x18410 0xc>, <0x18204 0x4>; + reg-names = "i2s_regs", "pll_regs", "soc_ctrl"; + interrupts = <GIC_SPI 75 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&gateclk 0>; + clock-names = "internal"; + status = "disabled"; + }; + usb3@f0000 { compatible = "marvell,armada-380-xhci"; reg = <0xf0000 0x4000>,<0xf4000 0x4000>;
This commit adds the description of the I2S controller to the Marvell Armada 38x SoC's Device Tree, as well as its pin configuration. Signed-off-by: Marcin Wojtas <mw@semihalf.com> --- arch/arm/boot/dts/armada-38x.dtsi | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)