| Message ID | 1368897139-25485-5-git-send-email-sebastian.hesselbarth@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, 18 May 2013 19:12:19 +0200 Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> wrote: > The RFC sent by Russell King was missing an include for tda998x. This > is just a compatible clone to remember Russell to add that later. > > Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> > --- > Cc: Russell King <linux@arm.linux.org.uk> > Cc: linux-arm-kernel@lists.infradead.org > Cc: dri-devel@lists.freedesktop.org > Cc: Jason Cooper <jason@lakedaemon.net> > Cc: Jean-Francois Moine <moinejf@free.fr> > --- > include/drm/i2c/tda998x.h | 23 +++++++++++++++++++++++ > 1 file changed, 23 insertions(+) > create mode 100644 include/drm/i2c/tda998x.h > > diff --git a/include/drm/i2c/tda998x.h b/include/drm/i2c/tda998x.h > new file mode 100644 > index 0000000..41f799f > --- /dev/null > +++ b/include/drm/i2c/tda998x.h > @@ -0,0 +1,23 @@ > +#ifndef __TDA998X_H__ > +#define __TDA998X_H__ > + > +enum tda998x_audio_format { > + AFMT_I2S, > + AFMT_SPDIF, > +}; > + > +struct tda998x_encoder_params { > + int audio_cfg; > + int audio_clk_cfg; > + enum tda998x_audio_format audio_format; > + int audio_sample_rate; > + char audio_frame[6]; > + int swap_a, mirr_a; > + int swap_b, mirr_b; > + int swap_c, mirr_c; > + int swap_d, mirr_d; > + int swap_e, mirr_e; > + int swap_f, mirr_f; > +}; > + > +#endif These parameters should not be there. It seems to me that the DT is the right place.
On 05/18/2013 07:46 PM, Jean-Francois Moine wrote: > On Sat, 18 May 2013 19:12:19 +0200 > Sebastian Hesselbarth<sebastian.hesselbarth@gmail.com> wrote: > >> The RFC sent by Russell King was missing an include for tda998x. This >> is just a compatible clone to remember Russell to add that later. >> >> Signed-off-by: Sebastian Hesselbarth<sebastian.hesselbarth@gmail.com> ... > These parameters should not be there. It seems to me that the DT is the > right place. True, but if you just read the description above: "RFC sent by Russell King was missing an include for tda998x". You want to test the RFC, you need that include. Sebastian
On Sat, 18 May 2013 14:23:19 -0400 Rob Clark <robdclark@gmail.com> wrote: > > These parameters should not be there. It seems to me that the DT is the > > right place. > > You might not want to directly have a hard DT dependency in tda998x, > as the encoder could be used on non-DT platforms. Although a DT to > encoder-params helper might be a nice idea for platforms which do have > DT. If I correctly understand: - Russell does not use any DT, so his drm driver should be declared in some cubox-setup code in mach-dove/ - this code should also declare the tda998x - the drm driver contains/passes parameters to the tda998x As the connection Dove LCD <-> tda998x is Cubox specific, the question is: why are'nt the tda998x parameters in the cubox-setup code?
On 05/18/2013 08:58 PM, Jean-Francois Moine wrote: > On Sat, 18 May 2013 14:23:19 -0400 > Rob Clark<robdclark@gmail.com> wrote: > >>> These parameters should not be there. It seems to me that the DT is the >>> right place. >> >> You might not want to directly have a hard DT dependency in tda998x, >> as the encoder could be used on non-DT platforms. Although a DT to >> encoder-params helper might be a nice idea for platforms which do have >> DT. > > If I correctly understand: > > - Russell does not use any DT, so his drm driver should be declared in > some cubox-setup code in mach-dove/ No. The _device_ is declared in some cubox-setup but the _driver_ goes into drivers/gpu/drm. Reading vendor provided kernel code may be misleading as they often just put all stuff in arch/arm/mach-something. > - this code should also declare the tda998x The device for tda998x yes, but not the driver. Anyway, Russel decided to have tda998x probed by his drm_driver. > - the drm driver contains/passes parameters to the tda998x > > As the connection Dove LCD<-> tda998x is Cubox specific, the question > is: why are'nt the tda998x parameters in the cubox-setup code? The connection of Dove LCD and tda998x is _not_ Cubox specific, it is also on the D2Plug. To be precise, even "Dove LCD" is not Dove specific as you can find the very same controller on other Marvell SoCs with little differences. So in the end, we will have a DT node for the HW controllers found in Dove SoCs, a node for TDA998x, and a node for the video card, i.e. _how_ lcd controllers, external encoders, clocks, maybe audio, ... are hooked up on that specific board. There is so much to take care of like pixel format on lcd pins driving an external encoder (_not_ only tda998x), what gpio pin is connected to TDA interrupt line, one or two lcds, ... The corresponding drivers _will_ take care of it .. but in the future. All I try to make sure is that driver architecture does not prevent us from e.g. having two lcds plus dcon later on. Or allows to reuse dove-drm on pxa where only one lcd but no dcon is available. Sebastian
On Sat, May 18, 2013 at 09:30:09PM +0200, Sebastian Hesselbarth wrote: > The device for tda998x yes, but not the driver. Anyway, Russel decided > to have tda998x probed by his drm_driver. For the simple reason that _that_ is how DRM slave encoders work. Sometimes, reading the code of the subsystem you're using is well worth the effort. If Jean-Francois would like to read drm_encoder_slave.c, then it will be found that in order to use the TDA998x driver, which is itself a DRM slave encoder, you must use drm_i2c_encoder_init(). In order to use that, you must provide the I2C adapter structure, and a board info structure. If you don't want to do that, your options are: (a) you don't use the existing TDA998x DRM slave encoder, and instead write your own TDA998x driver, which will likely be justifyable rejected, or (b) you propose a new DRM interface to allow DRM components to be registered independently, without reference to a core drm_device structure. > The connection of Dove LCD and tda998x is _not_ Cubox specific, it is > also on the D2Plug. To be precise, even "Dove LCD" is not Dove specific > as you can find the very same controller on other Marvell SoCs with > little differences. Well, to spoil the argument a little, actually, the interconnection between the two is in no way "standardized". There's many different ways to wire the two chips together and have it work - because the TDA998x chips have a set of input muxes and swaps which allow you to connect the red, green, blue high/low nibbles in various ways and still have a correctly working system. The TDA998x connectivity is _highly_ configuable. So, just because one board connects LCD_D0 (red bit 0) to a particular pin on the TDA998x does not mean that another board does it that way too. So Jean-Francois is quite correct that this data needs to be provided by the board in some manner. The question is - how to do that sensibly. One possible stop-gap solution is to provide a default set which just happens to match the cubox, and allow OF to override it. :) > There is so much to take care of like pixel format on lcd pins driving > an external encoder (_not_ only tda998x), what gpio pin is connected to > TDA interrupt line, one or two lcds, ... Luckily, drivers/gpu/drm/i2c/tda998x.c does not make use of the IRQ signal at present - it's fairly basic and it currently operates by polling. Eventually, this could change of course. :) I think people need to keep a sense of perspective here: this is all entirely "new" stuff which is still being actively developed. It is not fully polished. We've not had a true open source TDA998x driver before 3.9 (that's when it was introduced.) It has teething problems at the moment, but I'm working with the authors to resolve these issues. I'm also still working on the DRM driver. For example, I've been playing with the RGB888 cursor support today, which seems to be suffering from a one pixel error in the hotspot location. I've not got to the bottom of it, but that kind of error _is_ important to understand and resolve, because it means that things like drawing programmes become unusable. What I'm starting to suspect is a bug in the X server causing this and not either my DRM driver or Xorg driver.
On 05/18/2013 10:26 PM, Russell King - ARM Linux wrote: > On Sat, May 18, 2013 at 09:30:09PM +0200, Sebastian Hesselbarth wrote: >> The device for tda998x yes, but not the driver. Anyway, Russel decided >> to have tda998x probed by his drm_driver. > > For the simple reason that _that_ is how DRM slave encoders work. > Sometimes, reading the code of the subsystem you're using is well > worth the effort. I agree and add that the probing itself doesn't prevent you from using DT for tda driver at all. You can still have an marvell,external-slave property pointing to the phandle of tda node. With that you get the adapter and i2c slave address for what is currently called dove_tda19989.c and may become e.g. dove_ext_i2c.c. In tda998x_drv you find the node and get all properties for input config or interrupt gpio. I have done that in the drivers before, but DT node parsing here is _added_ to the driver as it can be used on other non-DT platforms as well. >> The connection of Dove LCD and tda998x is _not_ Cubox specific, it is >> also on the D2Plug. To be precise, even "Dove LCD" is not Dove specific >> as you can find the very same controller on other Marvell SoCs with >> little differences. > > Well, to spoil the argument a little, actually, the interconnection > between the two is in no way "standardized". There's many different > ways to wire the two chips together and have it work - because the > TDA998x chips have a set of input muxes and swaps which allow you to > connect the red, green, blue high/low nibbles in various ways and > still have a correctly working system. The TDA998x connectivity is > _highly_ configuable. > > So, just because one board connects LCD_D0 (red bit 0) to a particular > pin on the TDA998x does not mean that another board does it that way > too. > > So Jean-Francois is quite correct that this data needs to be provided > by the board in some manner. The question is - how to do that sensibly. > > One possible stop-gap solution is to provide a default set which just > happens to match the cubox, and allow OF to override it. :) While I agree, Rob may have a different view on that for tda998x ;) >> There is so much to take care of like pixel format on lcd pins driving >> an external encoder (_not_ only tda998x), what gpio pin is connected to >> TDA interrupt line, one or two lcds, ... > > Luckily, drivers/gpu/drm/i2c/tda998x.c does not make use of the IRQ > signal at present - it's fairly basic and it currently operates by > polling. Eventually, this could change of course. :) Again, that is in the driver Jean-Francois has available. Make sure irq handler runs in a separate thread from get_edid and hpd and you will be interrupted on hpd. Having said, that should finally lead to the slave encoder setting .connector_type and .polled as this is where you know it. Sebastian
On Sat, 18 May 2013 21:30:09 +0200 Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> wrote: > So in the end, we will have a DT node for the HW controllers found > in Dove SoCs, a node for TDA998x, and a node for the video card, i.e. > _how_ lcd controllers, external encoders, clocks, maybe audio, ... > are hooked up on that specific board. Here is my dove-cubox.dts. What is wrong with it? /dts-v1/; /include/ "dove.dtsi" / { model = "SolidRun CuBox"; compatible = "solidrun,cubox", "marvell,dove"; memory { device_type = "memory"; reg = <0x00000000 0x40000000>; }; chosen { bootargs = "console=ttyS0,115200n8 earlyprintk"; }; leds { compatible = "gpio-leds"; pinctrl-0 = <&pmx_gpio_18>; pinctrl-names = "default"; power { label = "Power"; gpios = <&gpio0 18 1>; linux,default-trigger = "default-on"; }; }; regulators { compatible = "simple-bus"; #address-cells = <1>; #size-cells = <0>; usb_power: regulator@1 { compatible = "regulator-fixed"; reg = <1>; regulator-name = "USB Power"; regulator-min-microvolt = <5000000>; regulator-max-microvolt = <5000000>; enable-active-high; regulator-always-on; regulator-boot-on; gpio = <&gpio0 1 0>; }; }; clocks { /* 25MHz reference crystal */ ref25: oscillator { compatible = "fixed-clock"; #clock-cells = <0>; clock-frequency = <25000000>; }; lcdclk: fixed-clock { compatible = "fixed-clock"; #clock-cells = <0>; clock-frequency = <400000000>; }; }; audio { compatible = "marvell,kirkwood-spdif-audio"; id = <1>; }; video { compatible = "marvell,dove-video"; }; }; &uart0 { status = "okay"; }; &sata0 { status = "okay"; }; &i2c0 { status = "okay"; clock-frequency = <100000>; si5351: clock-generator { compatible = "silabs,si5351a-msop"; reg = <0x60>; #address-cells = <1>; #size-cells = <0>; #clock-cells = <1>; /* connect xtal input to 25MHz reference */ clocks = <&ref25>; /* connect xtal input as source of pll0 and pll1 */ silabs,pll-source = <0 0>, <1 0>; clkout0 { reg = <0>; silabs,drive-strength = <8>; silabs,multisynth-source = <0>; silabs,clock-source = <0>; silabs,pll-master; }; clkout1 { reg = <1>; silabs,drive-strength = <8>; silabs,multisynth-source = <1>; silabs,clock-source = <0>; silabs,pll-master; }; clkout2 { reg = <2>; silabs,multisynth-source = <1>; silabs,clock-source = <0>; }; }; tda998x: hdmi-encoder { compatible = "nxp,tda998x"; reg = <0x70>; interrupt-parent = <&gpio0>; interrupts = <27 2>; /* falling edge */ }; }; &sdio0 { status = "okay"; /* sdio0 card detect is connected to wrong pin on CuBox */ cd-gpios = <&gpio0 12 1>; }; &spi0 { status = "okay"; /* spi0.0: 4M Flash Winbond W25Q32BV */ spi-flash@0 { compatible = "st,w25q32"; spi-max-frequency = <20000000>; reg = <0>; }; }; &pinctrl { pinctrl-0 = <&pmx_gpio_1 &pmx_gpio_12 &pmx_gpio_13 &pmx_gpio_camera>; pinctrl-names = "default"; pmx_gpio_1: pmx-gpio-1 { marvell,pins = "mpp1"; marvell,function = "gpio"; }; pmx_gpio_12: pmx-gpio-12 { marvell,pins = "mpp12"; marvell,function = "gpio"; }; /* kirkwood i2s */ pmx_gpio_13: pmx-gpio-13 { marvell,pins = "mpp13"; marvell,function = "gpio"; }; pmx_gpio_18: pmx-gpio-18 { marvell,pins = "mpp18"; marvell,function = "gpio"; }; /* nxp HDMI irq on pin 27 */ pmx_gpio_camera: pmx-gpio-camera { marvell,pins = "mpp_camera"; marvell,function = "gpio"; }; }; &mdio { status = "okay"; }; ðernet { status = "okay"; }; &lcd0 { status = "okay"; clocks = <&core_clk 3>, <0>, <&lcdclk>, <&si5351 0>; marvell,port-type = <11>; /* HDMIA */ marvell,external-encoder = <&tda998x>; }; &i2s1 { status = "okay"; }; /* --- test (not cubox) ---- * &dcon { status = "okay"; }; &lcd1 { status = "okay"; clocks = <&core_clk 3>, <0>, <&lcdclk>, <0>; marvell,port-type = <1>; display-timings { mode { hactive = <1920>; vactive = <1080>; hfront-porch = <88>; hsync-len = <44>; hback-porch = <148>; vfront-porch = <4>; vsync-len = <5>; vback-porch = <36>; clock = <148500>; }; }; }; * ---- */
On 05/19/2013 08:01 AM, Jean-Francois Moine wrote: > On Sat, 18 May 2013 21:30:09 +0200 > Sebastian Hesselbarth<sebastian.hesselbarth@gmail.com> wrote: >> So in the end, we will have a DT node for the HW controllers found >> in Dove SoCs, a node for TDA998x, and a node for the video card, i.e. >> _how_ lcd controllers, external encoders, clocks, maybe audio, ... >> are hooked up on that specific board. > > Here is my dove-cubox.dts. What is wrong with it? Oh come on, seriously? There is _nothing_ wrong with it, it already reflects what I told you would be required for generic Dove SoC board video. > video { > compatible = "marvell,dove-video"; > }; As you may have noticed, the RFC node I sent yesterday. Is in _no_ way different from a functional point of view. I added a video-memory property, I renamed the compatible string. And I moved clocks property to the video card *but* as I said in the RFC description: "This adds a video card node required for rmk's dove_drm driver. Reg property matches reserved memory region (currently 16M at top of memory), clocks property should carry extclk0 for now." It is a node *required for rmk's dove_drm driver* for you to _test_ that very driver. > &lcd0 { > status = "okay"; > clocks =<&core_clk 3>,<0>,<&lcdclk>,<&si5351 0>; > marvell,port-type =<11>; /* HDMIA */ > marvell,external-encoder =<&tda998x>; > }; Again, there is no DT support in *rmk's driver*! I didn't add those to the RFC node. I provided a DT node to *test* rmk driver. While you were busy with complaining about things we already answered, I did test the driver on both Cubox and D2Plug. Now I can start with looking into rmk's driver and _suggest_ to fix this or that. > /* --- test (not cubox) ---- * > &dcon { status = "okay"; }; > > &lcd1 { > status = "okay"; > clocks =<&core_clk 3>,<0>,<&lcdclk>,<0>; > marvell,port-type =<1>; > display-timings { > mode { > hactive =<1920>; > vactive =<1080>; > hfront-porch =<88>; > hsync-len =<44>; > hback-porch =<148>; > vfront-porch =<4>; > vsync-len =<5>; > vback-porch =<36>; > clock =<148500>; > }; > }; I would be surprised if, lcd1 will ever be capable of driving 1080p60 on a *VGA port*! Seriously, start _reading_ what we say. I want all those features I already told you for your driver, in mainline driver too. All I told you was to prevent you from doing dirty little Cubox specific hacks that I would have to remove for e.g. D2Plug. *But* if you ask me if we should take Russell's or your driver as a basis, the answer is Russell's. Colon. Sebastian
On Sun, 19 May 2013 10:30:00 +0200 Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> wrote: > > /* --- test (not cubox) ---- * > > &dcon { status = "okay"; }; > > > > &lcd1 { > > status = "okay"; > > clocks =<&core_clk 3>,<0>,<&lcdclk>,<0>; > > marvell,port-type =<1>; > > display-timings { > > mode { > > hactive =<1920>; > > vactive =<1080>; > > hfront-porch =<88>; > > hsync-len =<44>; > > hback-porch =<148>; > > vfront-porch =<4>; > > vsync-len =<5>; > > vback-porch =<36>; > > clock =<148500>; > > }; > > }; > > I would be surprised if, lcd1 will ever be capable of driving > 1080p60 on a *VGA port*! As you may see, this sequence is preceded by an open comment: "test". Now, as the port B of the display controller is only VGA (this is checked in my driver), as there is no VGA connector on the Cubox, as there is no VGA DAC code in my driver, and as I had to test the display controller, I: - defined the port B as VGA, - set the timings of the mode I use for my HDMI display. This does not mean the example must be used in the real word. It is just a working example for test purpose. > Seriously, start _reading_ what we say. I want all those > features I already told you for your driver, in mainline driver > too. All I told you was to prevent you from doing dirty little > Cubox specific hacks that I would have to remove for e.g. D2Plug. There is _NO_ Cubox specific stuff in my driver (I don't even use any cubox-setup as Russell) and it should work without any change (except the DT) in your D2plug. Remember, all my drm driver work is in http://moinejf.free.fr/cubox/ as a big kernel patch (I will add the I2C: mv64xxx which work fine - thanks Russell). > *But* if you ask me if we should take Russell's or your driver > as a basis, the answer is Russell's. Colon. OK. Do what you want. My driver works fine enough for my usage: software development. For video, my ISP gives us for free a Atom based multimedia player with a Blue-Ray reader, and I have a AMD64 double core on my family TV set for internet video (it will be a long time till there will be VP8 decoding with vMeta). The only interest I see in the Cubox is its low power consumption. Now, I will switch back to my favorite long-distance development... See you.
diff --git a/include/drm/i2c/tda998x.h b/include/drm/i2c/tda998x.h new file mode 100644 index 0000000..41f799f --- /dev/null +++ b/include/drm/i2c/tda998x.h @@ -0,0 +1,23 @@ +#ifndef __TDA998X_H__ +#define __TDA998X_H__ + +enum tda998x_audio_format { + AFMT_I2S, + AFMT_SPDIF, +}; + +struct tda998x_encoder_params { + int audio_cfg; + int audio_clk_cfg; + enum tda998x_audio_format audio_format; + int audio_sample_rate; + char audio_frame[6]; + int swap_a, mirr_a; + int swap_b, mirr_b; + int swap_c, mirr_c; + int swap_d, mirr_d; + int swap_e, mirr_e; + int swap_f, mirr_f; +}; + +#endif
The RFC sent by Russell King was missing an include for tda998x. This is just a compatible clone to remember Russell to add that later. Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> --- Cc: Russell King <linux@arm.linux.org.uk> Cc: linux-arm-kernel@lists.infradead.org Cc: dri-devel@lists.freedesktop.org Cc: Jason Cooper <jason@lakedaemon.net> Cc: Jean-Francois Moine <moinejf@free.fr> --- include/drm/i2c/tda998x.h | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) create mode 100644 include/drm/i2c/tda998x.h