| Message ID | E1VSwYO-0000iT-CU@rmk-PC.arm.linux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, 06 Oct 2013 23:11:56 +0100 Russell King <rmk+kernel@arm.linux.org.uk> wrote: > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> > --- > drivers/gpu/drm/armada/Kconfig | 9 +++++++ > drivers/gpu/drm/armada/armada_drv.c | 42 +++++++++++++++++++++++++++++++++++ > 2 files changed, 51 insertions(+), 0 deletions(-) > > diff --git a/drivers/gpu/drm/armada/Kconfig b/drivers/gpu/drm/armada/Kconfig > index c7a0a94..87e62dd 100644 > --- a/drivers/gpu/drm/armada/Kconfig > +++ b/drivers/gpu/drm/armada/Kconfig > @@ -13,3 +13,12 @@ config DRM_ARMADA > This driver provides no built-in acceleration; acceleration is > performed by other IP found on the SoC. This driver provides > kernel mode setting and buffer management to userspace. > + > +config DRM_ARMADA_TDA1998X > + bool "Support TDA1998X HDMI output" > + depends on DRM_ARMADA != n > + depends on I2C && DRM_I2C_NXP_TDA998X = y > + default y > + help > + Support the TDA1998x HDMI output device found on the Solid-Run > + CuBox. It seems we are going backwards: as the Armada based boards will soon move to full DT (mvebu), you are making an exception for the Cubox, so that there should be Cubox specific kernels. I don't like that... > diff --git a/drivers/gpu/drm/armada/armada_drv.c b/drivers/gpu/drm/armada/armada_drv.c > index db62f1b..69517cf 100644 > --- a/drivers/gpu/drm/armada/armada_drv.c > +++ b/drivers/gpu/drm/armada/armada_drv.c > @@ -16,6 +16,42 @@ > #include <drm/armada_drm.h> > #include "armada_ioctlP.h" > > +#ifdef CONFIG_DRM_ARMADA_TDA1998X > +#include <drm/i2c/tda998x.h> > +#include "armada_slave.h" > + > +static struct tda998x_encoder_params params = { > + /* With 0x24, there is no translation between vp_out and int_vp > + FB LCD out Pins VIP Int Vp > + R:23:16 R:7:0 VPC7:0 7:0 7:0[R] > + G:15:8 G:15:8 VPB7:0 23:16 23:16[G] > + B:7:0 B:23:16 VPA7:0 15:8 15:8[B] > + */ > + .swap_a = 2, > + .swap_b = 3, > + .swap_c = 4, > + .swap_d = 5, > + .swap_e = 0, > + .swap_f = 1, I still don't agree. You don't need to invert R <-> B for the Cubox at the tda998x level: this may be done setting as it should be the CFG_GRA_SWAPRB flag of the lcd register LCD_SPU_DMA_CTRL0. > + .audio_cfg = BIT(2), > + .audio_frame[1] = 1, > + .audio_format = AFMT_SPDIF, > + .audio_sample_rate = 44100, These values are rather mysterious! Looking at the tda998x driver, I found that: - audio_cfg can be 0x03 for i2s input and 0x04 for spdif input - audio_frame[1] is the (channel count - 1) - audio_format and audio_sample_rate are hardcoding the audio input to spdif and the sample rate to 44.1kHz I don't think that these parameters are needed: - the tda998x must be prepared to get either i2s or spdif as the audio input at any time. The choice of this input results from the audio subsystem constraints (codec) found at audio streaming start time. - the channel count is always 2 for spdif. Do we need to accept four channels for i2s? - audio on hdmi works fine for me at any input rate setting the tda998x sample rate to 96 kHz. Anyway, in the actual tda998x driver, this audio_sample_rate value is just used to set an approximate value of ACR. But this value is not used because an optimal value is computed by the hardware thanks to the flag AIP_CNTRL_0_ACR_MAN! [snip] The remaining patch is Cubox specific.
On Mon, Oct 07, 2013 at 11:18:07AM +0200, Jean-Francois Moine wrote: > On Sun, 06 Oct 2013 23:11:56 +0100 > Russell King <rmk+kernel@arm.linux.org.uk> wrote: > > > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> > > --- > > drivers/gpu/drm/armada/Kconfig | 9 +++++++ > > drivers/gpu/drm/armada/armada_drv.c | 42 +++++++++++++++++++++++++++++++++++ > > 2 files changed, 51 insertions(+), 0 deletions(-) > > > > diff --git a/drivers/gpu/drm/armada/Kconfig b/drivers/gpu/drm/armada/Kconfig > > index c7a0a94..87e62dd 100644 > > --- a/drivers/gpu/drm/armada/Kconfig > > +++ b/drivers/gpu/drm/armada/Kconfig > > @@ -13,3 +13,12 @@ config DRM_ARMADA > > This driver provides no built-in acceleration; acceleration is > > performed by other IP found on the SoC. This driver provides > > kernel mode setting and buffer management to userspace. > > + > > +config DRM_ARMADA_TDA1998X > > + bool "Support TDA1998X HDMI output" > > + depends on DRM_ARMADA != n > > + depends on I2C && DRM_I2C_NXP_TDA998X = y > > + default y > > + help > > + Support the TDA1998x HDMI output device found on the Solid-Run > > + CuBox. > > It seems we are going backwards: as the Armada based boards will soon > move to full DT (mvebu), you are making an exception for the Cubox, so > that there should be Cubox specific kernels. I don't like that... *Ignored*. You know why. > > diff --git a/drivers/gpu/drm/armada/armada_drv.c b/drivers/gpu/drm/armada/armada_drv.c > > index db62f1b..69517cf 100644 > > --- a/drivers/gpu/drm/armada/armada_drv.c > > +++ b/drivers/gpu/drm/armada/armada_drv.c > > @@ -16,6 +16,42 @@ > > #include <drm/armada_drm.h> > > #include "armada_ioctlP.h" > > > > +#ifdef CONFIG_DRM_ARMADA_TDA1998X > > +#include <drm/i2c/tda998x.h> > > +#include "armada_slave.h" > > + > > +static struct tda998x_encoder_params params = { > > + /* With 0x24, there is no translation between vp_out and int_vp > > + FB LCD out Pins VIP Int Vp > > + R:23:16 R:7:0 VPC7:0 7:0 7:0[R] > > + G:15:8 G:15:8 VPB7:0 23:16 23:16[G] > > + B:7:0 B:23:16 VPA7:0 15:8 15:8[B] > > + */ > > + .swap_a = 2, > > + .swap_b = 3, > > + .swap_c = 4, > > + .swap_d = 5, > > + .swap_e = 0, > > + .swap_f = 1, > > I still don't agree. You don't need to invert R <-> B for the Cubox at > the tda998x level: this may be done setting as it should be the > CFG_GRA_SWAPRB flag of the lcd register LCD_SPU_DMA_CTRL0. You are totally and utterly wrong there. We need R and B presented on their correct lanes to the TDA998x so that the Armadas YUV->RGB conversion works. Setting CFG_GRA_SWAPRB does not swap the YUV output to match, neither does setting any of the other bits. CFG_GRA_SWAPRB is all about the _graphics_ _framebuffer_ format, it's got nothing to do at all with how the output is wired. > > + .audio_cfg = BIT(2), > > + .audio_frame[1] = 1, > > + .audio_format = AFMT_SPDIF, > > + .audio_sample_rate = 44100, > > These values are rather mysterious! Also I'm going to ignore this comment, because quite honestly, I think this is worthless. You haven't investigated how the TDA998x actually gets setup by Rabeeh.
On Mon, 7 Oct 2013 10:44:04 +0100 Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > On Mon, Oct 07, 2013 at 11:18:07AM +0200, Jean-Francois Moine wrote: [snip] > > It seems we are going backwards: as the Armada based boards will soon > > move to full DT (mvebu), you are making an exception for the Cubox, so > > that there should be Cubox specific kernels. I don't like that... > > *Ignored*. You know why. Sorry. I don't see why. May you explain again? > > > diff --git a/drivers/gpu/drm/armada/armada_drv.c b/drivers/gpu/drm/armada/armada_drv.c [snip] > > > +static struct tda998x_encoder_params params = { > > > + /* With 0x24, there is no translation between vp_out and int_vp > > > + FB LCD out Pins VIP Int Vp > > > + R:23:16 R:7:0 VPC7:0 7:0 7:0[R] > > > + G:15:8 G:15:8 VPB7:0 23:16 23:16[G] > > > + B:7:0 B:23:16 VPA7:0 15:8 15:8[B] > > > + */ > > > + .swap_a = 2, > > > + .swap_b = 3, > > > + .swap_c = 4, > > > + .swap_d = 5, > > > + .swap_e = 0, > > > + .swap_f = 1, > > > > I still don't agree. You don't need to invert R <-> B for the Cubox at > > the tda998x level: this may be done setting as it should be the > > CFG_GRA_SWAPRB flag of the lcd register LCD_SPU_DMA_CTRL0. > > You are totally and utterly wrong there. We need R and B presented on > their correct lanes to the TDA998x so that the Armadas YUV->RGB > conversion works. Setting CFG_GRA_SWAPRB does not swap the YUV output > to match, neither does setting any of the other bits. > > CFG_GRA_SWAPRB is all about the _graphics_ _framebuffer_ format, it's got > nothing to do at all with how the output is wired. Then, may you explain why you must swap R/B on simple RGB output? From your [PATCH 2/5] DRM: Armada: Add Armada DRM driver: + FMT(RGB888, 888PACK, CFG_SWAPRB); + FMT(BGR888, 888PACK, 0); + FMT(YUYV, 422PACK, CFG_YUV2RGB | CFG_SWAPYU | CFG_SWAPUV); > > > + .audio_cfg = BIT(2), > > > + .audio_frame[1] = 1, > > > + .audio_format = AFMT_SPDIF, > > > + .audio_sample_rate = 44100, > > > > These values are rather mysterious! > > Also I'm going to ignore this comment, because quite honestly, I think > this is worthless. You haven't investigated how the TDA998x actually > gets setup by Rabeeh. Rabeeh did the most he could to have a working Cubox. He used bad written drivers and he had not the time to think about how the drivers could be enhanced. Here is a small story about i2s/spdif: once, I set the tda998x to use the spdif input, and at this time, I was using the dummy codec. This codec accepts the format 32_LE, as does the audio device, but the output cannot go to spdif. Result: no hdmi sound. Should we restrict the hdmi transmitter to spdif (thus 'no 32 bits stream') or to i2s (thus 'no compressed stream') only, or accept both i2s and spdif and allow a full range of supported audio streams?
On Mon, Oct 07, 2013 at 12:48:20PM +0200, Jean-Francois Moine wrote: > On Mon, 7 Oct 2013 10:44:04 +0100 > Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > > > On Mon, Oct 07, 2013 at 11:18:07AM +0200, Jean-Francois Moine wrote: > [snip] > > > It seems we are going backwards: as the Armada based boards will soon > > > move to full DT (mvebu), you are making an exception for the Cubox, so > > > that there should be Cubox specific kernels. I don't like that... > > > > *Ignored*. You know why. > > Sorry. I don't see why. May you explain again? I don't run DT because DT lacks most of the features I require on the cubox. Therefore I can't develop for DT. Simple. > > > > diff --git a/drivers/gpu/drm/armada/armada_drv.c b/drivers/gpu/drm/armada/armada_drv.c > [snip] > > > > +static struct tda998x_encoder_params params = { > > > > + /* With 0x24, there is no translation between vp_out and int_vp > > > > + FB LCD out Pins VIP Int Vp > > > > + R:23:16 R:7:0 VPC7:0 7:0 7:0[R] > > > > + G:15:8 G:15:8 VPB7:0 23:16 23:16[G] > > > > + B:7:0 B:23:16 VPA7:0 15:8 15:8[B] > > > > + */ > > > > + .swap_a = 2, > > > > + .swap_b = 3, > > > > + .swap_c = 4, > > > > + .swap_d = 5, > > > > + .swap_e = 0, > > > > + .swap_f = 1, > > > > > > I still don't agree. You don't need to invert R <-> B for the Cubox at > > > the tda998x level: this may be done setting as it should be the > > > CFG_GRA_SWAPRB flag of the lcd register LCD_SPU_DMA_CTRL0. > > > > You are totally and utterly wrong there. We need R and B presented on > > their correct lanes to the TDA998x so that the Armadas YUV->RGB > > conversion works. Setting CFG_GRA_SWAPRB does not swap the YUV output > > to match, neither does setting any of the other bits. > > > > CFG_GRA_SWAPRB is all about the _graphics_ _framebuffer_ format, it's got > > nothing to do at all with how the output is wired. > > Then, may you explain why you must swap R/B on simple RGB output? > > From your [PATCH 2/5] DRM: Armada: Add Armada DRM driver: > > + FMT(RGB888, 888PACK, CFG_SWAPRB); > + FMT(BGR888, 888PACK, 0); I think I explained above. This is to do with the *graphics* *framebuffer* *format* in *memory*. This has nothing to do with how the hardware is wired up. With no swapping, the 888 format is defined by the documentation to be: [7:0] = red [15:8] = green [23:16] = blue Now, if you look this up in the drm_fourcc.h header file, this corresponds with this entry: #define DRM_FORMAT_BGR888 fourcc_code('B', 'G', '2', '4') /* [23:0] B:G:R little endian */ Hence, this is the BGR888 format with no swapping. Therefore, my second line is correct. With CFG_SWAPRB set, the format in hardware becomes: [7:0] = blue [15:8] = green [23:16] = red Again, if you look this up in the drm_fourcc.h header, you get: #define DRM_FORMAT_RGB888 fourcc_code('R', 'G', '2', '4') /* [23:0] R:G:B little endian */ So, the two entries you quote above are 100% correct. > + FMT(YUYV, 422PACK, CFG_YUV2RGB | CFG_SWAPYU | CFG_SWAPUV); Where is the RGB swap there? Again, this is to do with swapping the format to match the hardware. Please read the documentation on the framebuffer formats. The native 422PACK format is: [7:0] = U [15:8] = Y0 [23:16] = V [31:24] = Y1 and we need to set both CFG_SWAPYU and CFG_SWAPUV to get this to be: [7:0] = V [15:8] = Y1 [23:16] = U [31:24] = Y0 which is what the YUYV format wants. > Rabeeh did the most he could to have a working Cubox. He used bad > written drivers and he had not the time to think about how the drivers > could be enhanced. What does I2S give us in terms of "enhancement" which SPDIF does not, remembering that audio is transmitted over HDMI in a format which closely resembles SPDIF (but with a 28-bit subframe size.) > Here is a small story about i2s/spdif: once, I set the tda998x to use > the spdif input, and at this time, I was using the dummy codec. > This codec accepts the format 32_LE, as does the audio device, but the > output cannot go to spdif. Result: no hdmi sound. How ASoC works in this regard is that the capabilities are the _union_ of the codec and the cpu DAIs. The SPDIF transmitter codec supports 16, 20 and 24 bits per sample but not 32. Quite simply, that's because the SPDIF format does not allow 32-bits per sample. Therefore, 32_LE is not available for use with audio output, and userspace must convert down to something which the hardware does support. "Because I2S can support 32-bit audio" is a poor reason, because you can't pass 32-bit audio via HDMI due to a subframe being limited to 28 bits - up to 24 bits for the sample and 4 bits of control/status information.
On 10/07/2013 01:09 PM, Russell King - ARM Linux wrote: > On Mon, Oct 07, 2013 at 12:48:20PM +0200, Jean-Francois Moine wrote: >> On Mon, 7 Oct 2013 10:44:04 +0100 >> Rabeeh did the most he could to have a working Cubox. He used bad >> written drivers and he had not the time to think about how the drivers >> could be enhanced. > > What does I2S give us in terms of "enhancement" which SPDIF does not, > remembering that audio is transmitted over HDMI in a format which closely > resembles SPDIF (but with a 28-bit subframe size.) With respect to what _most_ users want, we should use SPDIF and ignore I2S. Consumer audio fits well into SPDIF and we get pass-through, which we loose on I2S. I2S _can_ support more than two channels, but only if you wire up more DATA lines. Those are not available on Dove, so its I2S is limited to two channel audio. The CuBox is simply not designed for HBR or multi-channel PCM, it is a _consumer_ settop box replacement. Sebastian
On Mon, 7 Oct 2013 12:09:02 +0100 Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > > Here is a small story about i2s/spdif: once, I set the tda998x to use > > the spdif input, and at this time, I was using the dummy codec. > > This codec accepts the format 32_LE, as does the audio device, but the > > output cannot go to spdif. Result: no hdmi sound. > > How ASoC works in this regard is that the capabilities are the _union_ of > the codec and the cpu DAIs. > > The SPDIF transmitter codec supports 16, 20 and 24 bits per sample but not > 32. Quite simply, that's because the SPDIF format does not allow 32-bits > per sample. Therefore, 32_LE is not available for use with audio output, > and userspace must convert down to something which the hardware does > support. > > "Because I2S can support 32-bit audio" is a poor reason, because you can't > pass 32-bit audio via HDMI due to a subframe being limited to 28 bits - up > to 24 bits for the sample and 4 bits of control/status information. OK. So, to resume, if both i2s and spdif are furnished by the audio subsystem, the tda998x must use spdif only (with the spdif codec). Then, the declaration of the tda998x in the DT will include an optional audio property, say 'audio-input' with the values "i2s" or "spdif" which corresponds to your parameter 'audio_format'. Also, from your video swap explanation, the DT will contain some video property(ies) TBD. The remaining question is: do we need more audio parameters? - the audio_cfg value is defined by i2s/spdif (3 or 4) - the audio_frame[1] value is always 1 (2 channels) - audio_sample_rate is useless
On Mon, Oct 07, 2013 at 02:03:27PM +0200, Jean-Francois Moine wrote: > On Mon, 7 Oct 2013 12:09:02 +0100 > Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > > > > Here is a small story about i2s/spdif: once, I set the tda998x to use > > > the spdif input, and at this time, I was using the dummy codec. > > > This codec accepts the format 32_LE, as does the audio device, but the > > > output cannot go to spdif. Result: no hdmi sound. > > > > How ASoC works in this regard is that the capabilities are the _union_ of > > the codec and the cpu DAIs. > > > > The SPDIF transmitter codec supports 16, 20 and 24 bits per sample but not > > 32. Quite simply, that's because the SPDIF format does not allow 32-bits > > per sample. Therefore, 32_LE is not available for use with audio output, > > and userspace must convert down to something which the hardware does > > support. > > > > "Because I2S can support 32-bit audio" is a poor reason, because you can't > > pass 32-bit audio via HDMI due to a subframe being limited to 28 bits - up > > to 24 bits for the sample and 4 bits of control/status information. > > OK. So, to resume, if both i2s and spdif are furnished by the audio > subsystem, the tda998x must use spdif only (with the spdif codec). This is going off-topic because the TDA998x bindings are a subject for the TDA998x driver, not the Armada driver. This data structure merely exists to provide the required data for the TDA998x in absence of DT information. To do the job properly - which is what should be done _whenever_ DT bindings are being considered - you have to look at the hardware with an overview of its capabilities, never at the implementation. So, the TDA998x family needs to be looked at in isolation, and its DT bindings need to be considered on its own merit. The TDA998x can accept a SPDIF signal on a single pin (on AP6), with or without a clock (on AP5), or it can accept I2S: I2S clock in APCLK, I2S word select on AP0, and up to four I2S streams on AP1..4 and auxillary ("internal test") data in AP7. AP7 should therefore not be used. The TDA19988 is slightly different, because it doesn't have soo many AP pins. Here, SPDIF can be supplied on either AP1 or AP2 with the optional clock on AP3. I2S is similar to the above but only AP1 and AP2 accept streams. I think the I2S is a special case - I'm not aware of anything which outputs _four_ synchronised I2S data streams with a common word select. Nevertheless, it's something that needs to be considered when creating DT bindings. So, we have some quite different capabilities depending on the device. For I2S, we can get away with this: tda998x { compatible = "nxp,tda19988", "nxp,tda998x"; i2s-source = <&i2s_source N>; }; where N is the number of coherent I2S streams supplied. For the TDA998x, N can be 1 to 4. For TDA19988, N can be 1 or 2 but no more. For SPDIF, the situation is more complex. The TDA19988 could in theory have two streams connected, and one of those streams can be selected. So, maybe: tda998x { compatible = "nxp,tda998x"; spdif-source = <&spdif_source>; spdif-has-mclk; }; and for the 19988: tda19988 { compatible = "nxp,tda19988"; spdif0-source = <&spdif_source0>; spdif0-has-mclk; spdif1-source = <&spdif_source1>; spdif1-has-mclk; }; > Also, from your video swap explanation, the DT will contain some video > property(ies) TBD. The TDA998x needs some properties to define how it is connected to the rest of the hardware - which pins are connected to what in terms of the colour channel information. As I hope I've made clear to you, this has no bearing on the settings in the Dove for the SWAPRB bits. This is purely only to do with the TDA998x. So, this also needs to be presented as properties for the TDA998x driver. > The remaining question is: do we need more audio parameters? > > - the audio_cfg value is defined by i2s/spdif (3 or 4) This should be discovered from the DT binding information, because which AP pins get used depends on the signals being supplied to it. > - the audio_frame[1] value is always 1 (2 channels) > - audio_sample_rate is useless audio_sample_rate comes in if we wish to use a fixed sample rate and have coherent video and audio clocks derived from a common source. That allows fixed CTS and N values to be used based on the audio sample rate being supplied as the clocks have a fixed known relationship (and aren't going to drift.) However, you are right that this should not be specified in DT - it should come from the audio subsystem. However, that's something which is a very very long way away from being possible (if ever) with Linux since I don't see the DPCM issues ever getting resolved.
On Mon, Oct 7, 2013 at 7:09 AM, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > On Mon, Oct 07, 2013 at 12:48:20PM +0200, Jean-Francois Moine wrote: >> On Mon, 7 Oct 2013 10:44:04 +0100 >> Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: >> >> > On Mon, Oct 07, 2013 at 11:18:07AM +0200, Jean-Francois Moine wrote: >> [snip] >> > > It seems we are going backwards: as the Armada based boards will soon >> > > move to full DT (mvebu), you are making an exception for the Cubox, so >> > > that there should be Cubox specific kernels. I don't like that... >> > >> > *Ignored*. You know why. >> >> Sorry. I don't see why. May you explain again? > > I don't run DT because DT lacks most of the features I require on the > cubox. Therefore I can't develop for DT. Simple. Jean-François, just as an aside, I really don't think code that can be shared, like tda998x, should encode a DT requirement.. there are plenty of platforms that don't use DT (arm isn't everything, and last I heard aarch64 was going to be ACPI). Beyond that, it is a driver decision whether or not to support only-DT or DT + other.. and as long as there is a common board which can use the driver but which is not DT, there is probably a compelling reason to still support the non-DT case. BR, -R
On Mon, Oct 07, 2013 at 01:29:30PM +0200, Sebastian Hesselbarth wrote: > I2S _can_ support more than two channels, but only if you wire up more > DATA lines. Those are not available on Dove, so its I2S is limited to > two channel audio. A lot of devices implement extra channels with TDM rather than with extra wires - you get all the left channels and all the right channels grouped together (like with DSP/PCM modes except the channels are ordered differently). I'm never sure that's actually useful, though, since it's not something that just works with most hosts that don't understand it in hardware and most of those can do DSP/PCM anyway.
On 10/07/2013 05:53 PM, Mark Brown wrote: > On Mon, Oct 07, 2013 at 01:29:30PM +0200, Sebastian Hesselbarth wrote: > >> I2S _can_ support more than two channels, but only if you wire up more >> DATA lines. Those are not available on Dove, so its I2S is limited to >> two channel audio. > > A lot of devices implement extra channels with TDM rather than with > extra wires - you get all the left channels and all the right channels > grouped together (like with DSP/PCM modes except the channels are > ordered differently). I'm never sure that's actually useful, though, > since it's not something that just works with most hosts that don't > understand it in hardware and most of those can do DSP/PCM anyway. True, I didn't mention TDM modes squeezing multiple channels into one L/R frame. Neither Dove nor TDA998x support this, but thanks for the clarification.
On Mon, Oct 07, 2013 at 06:08:12PM +0200, Sebastian Hesselbarth wrote: > True, I didn't mention TDM modes squeezing multiple channels into one > L/R frame. Neither Dove nor TDA998x support this, but thanks for the > clarification. Depending on how the DMA and so on is done you can sometimes still do it on the CPU side by lying to the CPU about the data format (eg, tell it that two 16 bit samples are actually a 32 bit one) though it gets messy for I2S since you need to be able to gather all the left samples and all the right samples. Though given that your CODEC doesn't support it it's still all moot anyway as you say.
On Mon, 7 Oct 2013 10:59:39 -0400 Rob Clark <robdclark@gmail.com> wrote: > Jean-François, just as an aside, I really don't think code that can be > shared, like tda998x, should encode a DT requirement.. there are > plenty of platforms that don't use DT (arm isn't everything, and last > I heard aarch64 was going to be ACPI). > > Beyond that, it is a driver decision whether or not to support only-DT > or DT + other.. and as long as there is a common board which can use > the driver but which is not DT, there is probably a compelling reason > to still support the non-DT case. Rob, The Cubox is an open platform, and I use it just like a desktop PC. When its required drivers will be in the mainline, I will do the same as I do with PCs: I will not recompile a specific kernel each time there are kernel bugs or security issues. Instead, I will just upgrade my system from my distributor (Debian), and, in the packages, there will be a generic mvebu kernel as there is already one for Marvell Armada 370/xp, Freescale iMX5x/iMX6 (linux-image-3.10-3-armmp). But, for that, all the Cubox specific stuff must be described in a DT.
On Tue, Oct 08, 2013 at 11:19:13AM +0200, Jean-Francois Moine wrote: > The Cubox is an open platform, and I use it just like a desktop PC. > When its required drivers will be in the mainline, I will do the same > as I do with PCs: I will not recompile a specific kernel each time > there are kernel bugs or security issues. Instead, I will just upgrade > my system from my distributor (Debian), and, in the packages, there > will be a generic mvebu kernel as there is already one for Marvell > Armada 370/xp, Freescale iMX5x/iMX6 (linux-image-3.10-3-armmp). > But, for that, all the Cubox specific stuff must be described in a DT. Which scenario is better: 1. To have something in mainline which is capable of driving the hardware, but may need some additional work to make it useful for DT based setups. or 2. To have nothing. Now, you may prefer to have nothing, but personally, I prefer there to be forward progress. Forward progress means getting some kind of DRM driver into mainline. Yes, it may not work with DT setups yet, but - as per the discussions that have happened _endlessly_ on this topic, it's something that can be resolved at a later date. We _still_ haven't worked out how to properly deal with the TDA998x driver (or indeed any DRM based outputs) in a DT based setup, and all the time that problem exists, it won't be possible to write a proper stable DT binding for this. So please, get off your hobby horse about this and allow us to make some modicum of progress.
On Tue, Oct 8, 2013 at 5:19 AM, Jean-Francois Moine <moinejf@free.fr> wrote: > On Mon, 7 Oct 2013 10:59:39 -0400 > Rob Clark <robdclark@gmail.com> wrote: > >> Jean-François, just as an aside, I really don't think code that can be >> shared, like tda998x, should encode a DT requirement.. there are >> plenty of platforms that don't use DT (arm isn't everything, and last >> I heard aarch64 was going to be ACPI). >> >> Beyond that, it is a driver decision whether or not to support only-DT >> or DT + other.. and as long as there is a common board which can use >> the driver but which is not DT, there is probably a compelling reason >> to still support the non-DT case. > > Rob, > > The Cubox is an open platform, and I use it just like a desktop PC. > When its required drivers will be in the mainline, I will do the same > as I do with PCs: I will not recompile a specific kernel each time > there are kernel bugs or security issues. Instead, I will just upgrade > my system from my distributor (Debian), and, in the packages, there > will be a generic mvebu kernel as there is already one for Marvell > Armada 370/xp, Freescale iMX5x/iMX6 (linux-image-3.10-3-armmp). > But, for that, all the Cubox specific stuff must be described in a DT. sure, I'm not saying *not* to support DT (send patches if need be). I'm just saying that it should be ok to also support non-DT case and that shared components (like tda998x) should also still work in non-DT case. BR, -R > -- > Ken ar c'hentañ | ** Breizh ha Linux atav! ** > Jef | http://moinejf.free.fr/
On Tue, 8 Oct 2013 10:49:39 +0100 Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > On Tue, Oct 08, 2013 at 11:19:13AM +0200, Jean-Francois Moine wrote: > > The Cubox is an open platform, and I use it just like a desktop PC. > > When its required drivers will be in the mainline, I will do the same > > as I do with PCs: I will not recompile a specific kernel each time > > there are kernel bugs or security issues. Instead, I will just upgrade > > my system from my distributor (Debian), and, in the packages, there > > will be a generic mvebu kernel as there is already one for Marvell > > Armada 370/xp, Freescale iMX5x/iMX6 (linux-image-3.10-3-armmp). > > But, for that, all the Cubox specific stuff must be described in a DT. > > Which scenario is better: > > 1. To have something in mainline which is capable of driving the hardware, > but may need some additional work to make it useful for DT based setups. > > or > > 2. To have nothing. > > Now, you may prefer to have nothing, but personally, I prefer there to be > forward progress. Forward progress means getting some kind of DRM driver > into mainline. Yes, it may not work with DT setups yet, but - as per > the discussions that have happened _endlessly_ on this topic, it's > something that can be resolved at a later date. > > We _still_ haven't worked out how to properly deal with the TDA998x > driver (or indeed any DRM based outputs) in a DT based setup, and all > the time that problem exists, it won't be possible to write a proper > stable DT binding for this. > > So please, get off your hobby horse about this and allow us to make some > modicum of progress. You forgot: 3. To have all patches ready for submission and have a working DT driven Cubox kernel. I will submit a patch to add DT to the tda998x driver as soon as I have checked the new audio properties we talked about yesterday. Normally, this should have no impact on your Armada drm driver, and, yes, I will add DT to your driver as soon as it will be accepted (sorry to not ack it now: I had no time yet to have a look at it).
On Tue, Oct 8, 2013 at 11:34 AM, Jean-Francois Moine <moinejf@free.fr> wrote: > On Tue, 8 Oct 2013 10:49:39 +0100 > Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > >> On Tue, Oct 08, 2013 at 11:19:13AM +0200, Jean-Francois Moine wrote: >> > The Cubox is an open platform, and I use it just like a desktop PC. >> > When its required drivers will be in the mainline, I will do the same >> > as I do with PCs: I will not recompile a specific kernel each time >> > there are kernel bugs or security issues. Instead, I will just upgrade >> > my system from my distributor (Debian), and, in the packages, there >> > will be a generic mvebu kernel as there is already one for Marvell >> > Armada 370/xp, Freescale iMX5x/iMX6 (linux-image-3.10-3-armmp). >> > But, for that, all the Cubox specific stuff must be described in a DT. >> >> Which scenario is better: >> >> 1. To have something in mainline which is capable of driving the hardware, >> but may need some additional work to make it useful for DT based setups. >> >> or >> >> 2. To have nothing. >> >> Now, you may prefer to have nothing, but personally, I prefer there to be >> forward progress. Forward progress means getting some kind of DRM driver >> into mainline. Yes, it may not work with DT setups yet, but - as per >> the discussions that have happened _endlessly_ on this topic, it's >> something that can be resolved at a later date. >> >> We _still_ haven't worked out how to properly deal with the TDA998x >> driver (or indeed any DRM based outputs) in a DT based setup, and all >> the time that problem exists, it won't be possible to write a proper >> stable DT binding for this. >> >> So please, get off your hobby horse about this and allow us to make some >> modicum of progress. > > You forgot: > > 3. To have all patches ready for submission and have a working DT driven > Cubox kernel. No need to block this patch on DT support for all platforms that can use this driver. I don't see an issue with removing non-DT support later if it is no longer needed, or adding DT support in a future patch for platforms which can use it. So this patch looks fine to me. Signed-off-by: Rob Clark <robdclark@gmail.com > I will submit a patch to add DT to the tda998x driver as soon as I have > checked the new audio properties we talked about yesterday. > > Normally, this should have no impact on your Armada drm driver, and, > yes, I will add DT to your driver as soon as it will be accepted > (sorry to not ack it now: I had no time yet to have a look at it). > > -- > Ken ar c'hentañ | ** Breizh ha Linux atav! ** > Jef | http://moinejf.free.fr/
diff --git a/drivers/gpu/drm/armada/Kconfig b/drivers/gpu/drm/armada/Kconfig index c7a0a94..87e62dd 100644 --- a/drivers/gpu/drm/armada/Kconfig +++ b/drivers/gpu/drm/armada/Kconfig @@ -13,3 +13,12 @@ config DRM_ARMADA This driver provides no built-in acceleration; acceleration is performed by other IP found on the SoC. This driver provides kernel mode setting and buffer management to userspace. + +config DRM_ARMADA_TDA1998X + bool "Support TDA1998X HDMI output" + depends on DRM_ARMADA != n + depends on I2C && DRM_I2C_NXP_TDA998X = y + default y + help + Support the TDA1998x HDMI output device found on the Solid-Run + CuBox. diff --git a/drivers/gpu/drm/armada/armada_drv.c b/drivers/gpu/drm/armada/armada_drv.c index db62f1b..69517cf 100644 --- a/drivers/gpu/drm/armada/armada_drv.c +++ b/drivers/gpu/drm/armada/armada_drv.c @@ -16,6 +16,42 @@ #include <drm/armada_drm.h> #include "armada_ioctlP.h" +#ifdef CONFIG_DRM_ARMADA_TDA1998X +#include <drm/i2c/tda998x.h> +#include "armada_slave.h" + +static struct tda998x_encoder_params params = { + /* With 0x24, there is no translation between vp_out and int_vp + FB LCD out Pins VIP Int Vp + R:23:16 R:7:0 VPC7:0 7:0 7:0[R] + G:15:8 G:15:8 VPB7:0 23:16 23:16[G] + B:7:0 B:23:16 VPA7:0 15:8 15:8[B] + */ + .swap_a = 2, + .swap_b = 3, + .swap_c = 4, + .swap_d = 5, + .swap_e = 0, + .swap_f = 1, + .audio_cfg = BIT(2), + .audio_frame[1] = 1, + .audio_format = AFMT_SPDIF, + .audio_sample_rate = 44100, +}; + +static const struct armada_drm_slave_config tda19988_config = { + .i2c_adapter_id = 0, + .crtcs = 1 << 0, /* Only LCD0 at the moment */ + .polled = DRM_CONNECTOR_POLL_CONNECT | DRM_CONNECTOR_POLL_DISCONNECT, + .interlace_allowed = true, + .info = { + .type = "tda998x", + .addr = 0x70, + .platform_data = ¶ms, + }, +}; +#endif + static void armada_drm_unref_work(struct work_struct *work) { struct armada_private *priv = @@ -134,6 +170,12 @@ static int armada_drm_load(struct drm_device *dev, unsigned long flags) goto err_kms; } +#ifdef CONFIG_DRM_ARMADA_TDA1998X + ret = armada_drm_connector_slave_create(dev, &tda19988_config); + if (ret) + goto err_kms; +#endif + ret = drm_vblank_init(dev, n); if (ret) goto err_kms;
Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> --- drivers/gpu/drm/armada/Kconfig | 9 +++++++ drivers/gpu/drm/armada/armada_drv.c | 42 +++++++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 0 deletions(-)