| Message ID | 20190411214422.2267-2-peron.clem@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Allwinner H6 SPDIF support | expand |
Hi, On Thu, Apr 11, 2019 at 11:44:19PM +0200, Clément Péron wrote: > Allwinner H6 has a SPDIF controller with an increase of the fifo > size and the possibility to output the master clock. > However it's still compatible with the already existing driver for > Allwiner H3. > > Add this compatible in the device-tree bindings documentation. How was this tested? After looking at the datasheet, it looks like it's still significantly different. In particular: - The MCLK divider isn't at the same offset - The bits to flush the FIFO aren't at the same offset - The FIFO trigger levels aren't at the same offset - The FIFO status register layout is almost entirely different So it doesn't really look compatible to me Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Hi, On Mon, 15 Apr 2019 at 10:04, Maxime Ripard <maxime.ripard@bootlin.com> wrote: > > Hi, > > On Thu, Apr 11, 2019 at 11:44:19PM +0200, Clément Péron wrote: > > Allwinner H6 has a SPDIF controller with an increase of the fifo > > size and the possibility to output the master clock. > > However it's still compatible with the already existing driver for > > Allwiner H3. > > > > Add this compatible in the device-tree bindings documentation. > > How was this tested? Using Alsa utils on a Beelink GS1 and I check correct output sound on my home cinema. > > After looking at the datasheet, it looks like it's still significantly > different. In particular: > > - The MCLK divider isn't at the same offset We don't use the MCLK > - The bits to flush the FIFO aren't at the same offset > - The FIFO trigger levels aren't at the same offset > - The FIFO status register layout is almost entirely different We use DMA not FIFO Clement > > So it doesn't really look compatible to me > > Maxime > > -- > Maxime Ripard, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com
On Mon, 15 Apr 2019 at 10:09, Clément Péron <peron.clem@gmail.com> wrote: > > Hi, > > On Mon, 15 Apr 2019 at 10:04, Maxime Ripard <maxime.ripard@bootlin.com> wrote: > > > > Hi, > > > > On Thu, Apr 11, 2019 at 11:44:19PM +0200, Clément Péron wrote: > > > Allwinner H6 has a SPDIF controller with an increase of the fifo > > > size and the possibility to output the master clock. > > > However it's still compatible with the already existing driver for > > > Allwiner H3. > > > > > > Add this compatible in the device-tree bindings documentation. > > > > How was this tested? > > Using Alsa utils on a Beelink GS1 and I check correct output sound on > my home cinema. > > > > After looking at the datasheet, it looks like it's still significantly > > different. In particular: > > > > - The MCLK divider isn't at the same offset > We don't use the MCLK > > - The bits to flush the FIFO aren't at the same offset > > - The FIFO trigger levels aren't at the same offset > > - The FIFO status register layout is almost entirely different > We use DMA not FIFO And also only SPDIF Tx has been tested. > > Clement > > > > So it doesn't really look compatible to me > > > > Maxime > > > > -- > > Maxime Ripard, Bootlin > > Embedded Linux and Kernel engineering > > https://bootlin.com
On Mon, Apr 15, 2019 at 10:09:11AM +0200, Clément Péron wrote: > Hi, > > On Mon, 15 Apr 2019 at 10:04, Maxime Ripard <maxime.ripard@bootlin.com> wrote: > > > > Hi, > > > > On Thu, Apr 11, 2019 at 11:44:19PM +0200, Clément Péron wrote: > > > Allwinner H6 has a SPDIF controller with an increase of the fifo > > > size and the possibility to output the master clock. > > > However it's still compatible with the already existing driver for > > > Allwiner H3. > > > > > > Add this compatible in the device-tree bindings documentation. > > > > How was this tested? > > Using Alsa utils on a Beelink GS1 and I check correct output sound on > my home cinema. > > > After looking at the datasheet, it looks like it's still significantly > > different. In particular: > > > > - The MCLK divider isn't at the same offset > > We don't use the MCLK > > > - The bits to flush the FIFO aren't at the same offset > > - The FIFO trigger levels aren't at the same offset > > - The FIFO status register layout is almost entirely different > > We use DMA not FIFO Well, DMA still uses the FIFOs. And even then, you're kind of missing the point. The device tree's compatible is about whether a particular device can be used as another one. How you are using it in the driver is completely irrelevant. Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On Mon, 15 Apr 2019 at 10:18, Maxime Ripard <maxime.ripard@bootlin.com> wrote: > > On Mon, Apr 15, 2019 at 10:09:11AM +0200, Clément Péron wrote: > > Hi, > > > > On Mon, 15 Apr 2019 at 10:04, Maxime Ripard <maxime.ripard@bootlin.com> wrote: > > > > > > Hi, > > > > > > On Thu, Apr 11, 2019 at 11:44:19PM +0200, Clément Péron wrote: > > > > Allwinner H6 has a SPDIF controller with an increase of the fifo > > > > size and the possibility to output the master clock. > > > > However it's still compatible with the already existing driver for > > > > Allwiner H3. > > > > > > > > Add this compatible in the device-tree bindings documentation. > > > > > > How was this tested? > > > > Using Alsa utils on a Beelink GS1 and I check correct output sound on > > my home cinema. > > > > > After looking at the datasheet, it looks like it's still significantly > > > different. In particular: > > > > > > - The MCLK divider isn't at the same offset > > > > We don't use the MCLK > > > > > - The bits to flush the FIFO aren't at the same offset > > > - The FIFO trigger levels aren't at the same offset > > > - The FIFO status register layout is almost entirely different > > > > We use DMA not FIFO > > Well, DMA still uses the FIFOs. And even then, you're kind of missing > the point. The device tree's compatible is about whether a particular > device can be used as another one. How you are using it in the driver > is completely irrelevant. Sorry I meant " We use DMA not PIO". I didn't change the driver and the SPDIF is working what's irrelevant in declaring the driver as compatible ? If some feature are missing they can be introduce later as there is a special binding for H6 no ? Clement > > Maxime > > -- > Maxime Ripard, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com
On Mon, Apr 15, 2019 at 10:30:38AM +0200, Clément Péron wrote: > On Mon, 15 Apr 2019 at 10:18, Maxime Ripard <maxime.ripard@bootlin.com> wrote: > > > > On Mon, Apr 15, 2019 at 10:09:11AM +0200, Clément Péron wrote: > > > Hi, > > > > > > On Mon, 15 Apr 2019 at 10:04, Maxime Ripard <maxime.ripard@bootlin.com> wrote: > > > > > > > > Hi, > > > > > > > > On Thu, Apr 11, 2019 at 11:44:19PM +0200, Clément Péron wrote: > > > > > Allwinner H6 has a SPDIF controller with an increase of the fifo > > > > > size and the possibility to output the master clock. > > > > > However it's still compatible with the already existing driver for > > > > > Allwiner H3. > > > > > > > > > > Add this compatible in the device-tree bindings documentation. > > > > > > > > How was this tested? > > > > > > Using Alsa utils on a Beelink GS1 and I check correct output sound on > > > my home cinema. > > > > > > > After looking at the datasheet, it looks like it's still significantly > > > > different. In particular: > > > > > > > > - The MCLK divider isn't at the same offset > > > > > > We don't use the MCLK > > > > > > > - The bits to flush the FIFO aren't at the same offset > > > > - The FIFO trigger levels aren't at the same offset > > > > - The FIFO status register layout is almost entirely different > > > > > > We use DMA not FIFO > > > > Well, DMA still uses the FIFOs. And even then, you're kind of missing > > the point. The device tree's compatible is about whether a particular > > device can be used as another one. How you are using it in the driver > > is completely irrelevant. > > Sorry I meant " We use DMA not PIO". I'm still not quite sure what you mean. Both DMA and PIO would be using the FIFOs. > I didn't change the driver and the SPDIF is working what's irrelevant > in declaring the driver as compatible ? It's working by accident. This won't work for example: https://elixir.bootlin.com/linux/latest/source/sound/soc/sunxi/sun4i-spdif.c#L181 > If some feature are missing they can be introduce later as there is a > special binding for H6 no ? Again, that's irrelevant. The only question you need to ask yourself is whether, if you take an imaginary source code that would support all the H3 SPDIF features and run it on an H6, it works just as well. The examples above that it's not the case, so you can't claim that the device is compatible. Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Hi Clement, On Mon, Apr 15, 2019 at 10:30:38AM +0200, Clément Péron wrote: > On Mon, 15 Apr 2019 at 10:18, Maxime Ripard <maxime.ripard@bootlin.com> wrote: > > > > On Mon, Apr 15, 2019 at 10:09:11AM +0200, Clément Péron wrote: > > > Hi, > > > > > > On Mon, 15 Apr 2019 at 10:04, Maxime Ripard <maxime.ripard@bootlin.com> wrote: > > > > > > > > Hi, > > > > > > > > On Thu, Apr 11, 2019 at 11:44:19PM +0200, Clément Péron wrote: > > > > > Allwinner H6 has a SPDIF controller with an increase of the fifo > > > > > size and the possibility to output the master clock. > > > > > However it's still compatible with the already existing driver for > > > > > Allwiner H3. > > > > > > > > > > Add this compatible in the device-tree bindings documentation. > > > > > > > > How was this tested? > > > > > > Using Alsa utils on a Beelink GS1 and I check correct output sound on > > > my home cinema. > > > > > > > After looking at the datasheet, it looks like it's still significantly > > > > different. In particular: > > > > > > > > - The MCLK divider isn't at the same offset > > > > > > We don't use the MCLK > > > > > > > - The bits to flush the FIFO aren't at the same offset > > > > - The FIFO trigger levels aren't at the same offset > > > > - The FIFO status register layout is almost entirely different > > > > > > We use DMA not FIFO > > > > Well, DMA still uses the FIFOs. And even then, you're kind of missing > > the point. The device tree's compatible is about whether a particular > > device can be used as another one. How you are using it in the driver > > is completely irrelevant. > > Sorry I meant " We use DMA not PIO". > > I didn't change the driver and the SPDIF is working what's irrelevant > in declaring the driver as compatible ? > > If some feature are missing they can be introduce later as there is a > special binding for H6 no ? You have to drop fallback to allwinner,sun8i-h3-spdif at the very least: + #sound-dai-cells = <0>; + compatible = "allwinner,sun50i-h6-spdif", + "allwinner,sun8i-h3-spdif"; + reg = <0x05093000 0x400>; If the H6 hardware doesn't just implement some optional superset of what H3 does. regards, o. > Clement > > > > > Maxime > > > > -- > > Maxime Ripard, Bootlin > > Embedded Linux and Kernel engineering > > https://bootlin.com > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hi, On Mon, 15 Apr 2019 at 16:51, Maxime Ripard <maxime.ripard@bootlin.com> wrote: > > On Mon, Apr 15, 2019 at 10:30:38AM +0200, Clément Péron wrote: > > On Mon, 15 Apr 2019 at 10:18, Maxime Ripard <maxime.ripard@bootlin.com> wrote: > > > > > > On Mon, Apr 15, 2019 at 10:09:11AM +0200, Clément Péron wrote: > > > > Hi, > > > > > > > > On Mon, 15 Apr 2019 at 10:04, Maxime Ripard <maxime.ripard@bootlin.com> wrote: > > > > > > > > > > Hi, > > > > > > > > > > On Thu, Apr 11, 2019 at 11:44:19PM +0200, Clément Péron wrote: > > > > > > Allwinner H6 has a SPDIF controller with an increase of the fifo > > > > > > size and the possibility to output the master clock. > > > > > > However it's still compatible with the already existing driver for > > > > > > Allwiner H3. > > > > > > > > > > > > Add this compatible in the device-tree bindings documentation. > > > > > > > > > > How was this tested? > > > > > > > > Using Alsa utils on a Beelink GS1 and I check correct output sound on > > > > my home cinema. > > > > > > > > > After looking at the datasheet, it looks like it's still significantly > > > > > different. In particular: > > > > > > > > > > - The MCLK divider isn't at the same offset > > > > > > > > We don't use the MCLK > > > > > > > > > - The bits to flush the FIFO aren't at the same offset > > > > > - The FIFO trigger levels aren't at the same offset > > > > > - The FIFO status register layout is almost entirely different > > > > > > > > We use DMA not FIFO > > > > > > Well, DMA still uses the FIFOs. And even then, you're kind of missing > > > the point. The device tree's compatible is about whether a particular > > > device can be used as another one. How you are using it in the driver > > > is completely irrelevant. > > > > Sorry I meant " We use DMA not PIO". > > I'm still not quite sure what you mean. Both DMA and PIO would be > using the FIFOs. Just correcting myself but yes we use FIFO with the DMA. > > > I didn't change the driver and the SPDIF is working what's irrelevant > > in declaring the driver as compatible ? > > It's working by accident. > > This won't work for example: > https://elixir.bootlin.com/linux/latest/source/sound/soc/sunxi/sun4i-spdif.c#L181 > > > If some feature are missing they can be introduce later as there is a > > special binding for H6 no ? > > Again, that's irrelevant. The only question you need to ask yourself > is whether, if you take an imaginary source code that would support > all the H3 SPDIF features and run it on an H6, it works just as well. > > The examples above that it's not the case, so you can't claim that the > device is compatible. Is it ok if I fix the TX Fifo flush bit and remove the compatible with H3 ? (Looking at the driver this is the only bit used that is incompatible with H6) Or should I send a new driver with the correct mapping ? Thanks Clément > > Maxime > > -- > Maxime Ripard, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com
On Mon, Apr 15, 2019 at 06:30:10PM +0200, Clément Péron wrote: > > > I didn't change the driver and the SPDIF is working what's irrelevant > > > in declaring the driver as compatible ? > > > > It's working by accident. > > > > This won't work for example: > > https://elixir.bootlin.com/linux/latest/source/sound/soc/sunxi/sun4i-spdif.c#L181 > > > > > If some feature are missing they can be introduce later as there is a > > > special binding for H6 no ? > > > > Again, that's irrelevant. The only question you need to ask yourself > > is whether, if you take an imaginary source code that would support > > all the H3 SPDIF features and run it on an H6, it works just as well. > > > > The examples above that it's not the case, so you can't claim that the > > device is compatible. > > Is it ok if I fix the TX Fifo flush bit and remove the compatible with H3 ? > (Looking at the driver this is the only bit used that is incompatible with H6) Sure > Or should I send a new driver with the correct mapping ? No, we definitely don't want to duplicate a driver just for a single bit offset that changes. Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Hi, On Tue, 16 Apr 2019 at 09:34, Maxime Ripard <maxime.ripard@bootlin.com> wrote: > > On Mon, Apr 15, 2019 at 06:30:10PM +0200, Clément Péron wrote: > > > > I didn't change the driver and the SPDIF is working what's irrelevant > > > > in declaring the driver as compatible ? > > > > > > It's working by accident. > > > > > > This won't work for example: > > > https://elixir.bootlin.com/linux/latest/source/sound/soc/sunxi/sun4i-spdif.c#L181 > > > > > > > If some feature are missing they can be introduce later as there is a > > > > special binding for H6 no ? > > > > > > Again, that's irrelevant. The only question you need to ask yourself > > > is whether, if you take an imaginary source code that would support > > > all the H3 SPDIF features and run it on an H6, it works just as well. > > > > > > The examples above that it's not the case, so you can't claim that the > > > device is compatible. > > > > Is it ok if I fix the TX Fifo flush bit and remove the compatible with H3 ? > > (Looking at the driver this is the only bit used that is incompatible with H6) > > Sure > > > Or should I send a new driver with the correct mapping ? > > No, we definitely don't want to duplicate a driver just for a single > bit offset that changes. Thanks for the advice, I will try to submit a new version with this fix. Clement > > Maxime > > -- > Maxime Ripard, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com
diff --git a/Documentation/devicetree/bindings/sound/sunxi,sun4i-spdif.txt b/Documentation/devicetree/bindings/sound/sunxi,sun4i-spdif.txt index 0c64a209c2e9..e33180354dce 100644 --- a/Documentation/devicetree/bindings/sound/sunxi,sun4i-spdif.txt +++ b/Documentation/devicetree/bindings/sound/sunxi,sun4i-spdif.txt @@ -7,10 +7,11 @@ For now only playback is supported. Required properties: - - compatible : should be one of the following: + - compatible : Should be one of the following: - "allwinner,sun4i-a10-spdif": for the Allwinner A10 SoC - "allwinner,sun6i-a31-spdif": for the Allwinner A31 SoC - "allwinner,sun8i-h3-spdif": for the Allwinner H3 SoC + - "allwinner,sun50i-h6-spdif", "allwinner,sun8i-h3-spdif": for H6 SoC - reg : Offset and length of the register set for the device.
Allwinner H6 has a SPDIF controller with an increase of the fifo size and the possibility to output the master clock. However it's still compatible with the already existing driver for Allwiner H3. Add this compatible in the device-tree bindings documentation. Signed-off-by: Clément Péron <peron.clem@gmail.com> --- Documentation/devicetree/bindings/sound/sunxi,sun4i-spdif.txt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)