| Message ID | 1441229148-12095-1-git-send-email-dianders@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Sep 2, 2015 at 4:25 PM, Douglas Anderson <dianders@chromium.org> wrote: > The ddc-i2c-bus property was missing from the veyron dtsi file since > downstream the ddc-i2c-bus was still being specified in rk3288.dtsi and > nobody noticed when the veyron dtsi was sent upstream. Add it. > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > Note: I noticed that this was wrong but I don't currently have > graphics up and running on upstream on veyron. Posting this anyway > since it's pretty clear that it's needed. If someone else wants to > try it out that'd be nice, otherwise I'll put it on my list to figure > out how to get myself setup for graphics upstream. ;) Based on your other patch, this is temporary, right? I've been looking at DRM a lot lately. I think specifying the i2c bus in the hdmi chip or IP block node is wrong. If the I2C host is separate from the HDMI block, then it's only connection is to the HDMI connector. So the I2C host to the connector relationship is what the DT should describe. HPD gpio is similar. Now if the HDMI bridge controls DDC and HPD directly, then we don't need to describe those connections. Rob > > arch/arm/boot/dts/rk3288-veyron.dtsi | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/arch/arm/boot/dts/rk3288-veyron.dtsi b/arch/arm/boot/dts/rk3288-veyron.dtsi > index 2fa7a0d..275c78c 100644 > --- a/arch/arm/boot/dts/rk3288-veyron.dtsi > +++ b/arch/arm/boot/dts/rk3288-veyron.dtsi > @@ -158,6 +158,7 @@ > }; > > &hdmi { > + ddc-i2c-bus = <&i2c5>; > status = "okay"; > }; > > -- > 2.5.0.457.gab17608 >
Rob, On Wed, Sep 2, 2015 at 5:13 PM, Rob Herring <robherring2@gmail.com> wrote: > On Wed, Sep 2, 2015 at 4:25 PM, Douglas Anderson <dianders@chromium.org> wrote: >> The ddc-i2c-bus property was missing from the veyron dtsi file since >> downstream the ddc-i2c-bus was still being specified in rk3288.dtsi and >> nobody noticed when the veyron dtsi was sent upstream. Add it. >> >> Signed-off-by: Douglas Anderson <dianders@chromium.org> >> --- >> Note: I noticed that this was wrong but I don't currently have >> graphics up and running on upstream on veyron. Posting this anyway >> since it's pretty clear that it's needed. If someone else wants to >> try it out that'd be nice, otherwise I'll put it on my list to figure >> out how to get myself setup for graphics upstream. ;) > > Based on your other patch, this is temporary, right? Yes, though since I'm not personally working on the other patch series upstream I can't say for how long the "temporary" is.. I mostly posted the 2nd patch because it was clearly correct to add some pinmuxing states and could land any time, so I thought I'd be helpful. You're right that in the Chrome OS tree I turned right around and effectively removed the "ddc-i2c-bus", but having it land first adds a much better logical progression (make it the same as everyone else and _then_ change it). It also provides a revert path if something goes wrong. :) > I've been looking at DRM a lot lately. I think specifying the i2c bus > in the hdmi chip or IP block node is wrong. If the I2C host is > separate from the HDMI block, then it's only connection is to the HDMI > connector. So the I2C host to the connector relationship is what the > DT should describe. HPD gpio is similar. Now if the HDMI bridge > controls DDC and HPD directly, then we don't need to describe those > connections. I will say that I know very very little about DRM. Mostly I just visit it when there's some bug I'm running into that I can't find a better suited owner for. ;) I'm not sure I followed your whole paragraph. Could you give a fragment of DTS for how you'd imagine this ought to work? Also: the patch I submitted does match the current bindings if I understand it right. ...as is typical with device tree, if we want to change the bindings we've got to have a really good reason because we'd either need to figure out how to deal with existing DTBs in the field that need to run with newer kernels (if those exist). -Doug
On Wed, Sep 02, 2015 at 07:13:24PM -0500, Rob Herring wrote: > On Wed, Sep 2, 2015 at 4:25 PM, Douglas Anderson <dianders@chromium.org> wrote: > > The ddc-i2c-bus property was missing from the veyron dtsi file since > > downstream the ddc-i2c-bus was still being specified in rk3288.dtsi and > > nobody noticed when the veyron dtsi was sent upstream. Add it. > > > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > > --- > > Note: I noticed that this was wrong but I don't currently have > > graphics up and running on upstream on veyron. Posting this anyway > > since it's pretty clear that it's needed. If someone else wants to > > try it out that'd be nice, otherwise I'll put it on my list to figure > > out how to get myself setup for graphics upstream. ;) > > Based on your other patch, this is temporary, right? > > I've been looking at DRM a lot lately. I think specifying the i2c bus > in the hdmi chip or IP block node is wrong. If the I2C host is > separate from the HDMI block, then it's only connection is to the HDMI > connector. So the I2C host to the connector relationship is what the > DT should describe. HPD gpio is similar. Now if the HDMI bridge > controls DDC and HPD directly, then we don't need to describe those > connections. Except... we don't generally model connectors under DRM as a general rule. (The fbdev video connector stuff happened without very much publicity afaics.) It's not always appropriate to split it out from the bridge in any case. Consider something like a TDA998x where the TDA998x itself takes care of reading the DDC bus, and doesn't provide an I2C-like interface. If you try and split that into "bridge" or "encoder" and "connector" you end up having to invent a new kind of I2C thing which isn't an I2C adapter, or somehow squeezing an I2C adapter which isn't into the I2C layer. The TDA998x provides an interface to read a block of EDID at a time. It always does the page register access. You don't get to read it byte wise. It doesn't fit into I2C as an adapter at all.
On Thu, Sep 3, 2015 at 3:22 AM, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > On Wed, Sep 02, 2015 at 07:13:24PM -0500, Rob Herring wrote: >> On Wed, Sep 2, 2015 at 4:25 PM, Douglas Anderson <dianders@chromium.org> wrote: >> > The ddc-i2c-bus property was missing from the veyron dtsi file since >> > downstream the ddc-i2c-bus was still being specified in rk3288.dtsi and >> > nobody noticed when the veyron dtsi was sent upstream. Add it. >> > >> > Signed-off-by: Douglas Anderson <dianders@chromium.org> >> > --- >> > Note: I noticed that this was wrong but I don't currently have >> > graphics up and running on upstream on veyron. Posting this anyway >> > since it's pretty clear that it's needed. If someone else wants to >> > try it out that'd be nice, otherwise I'll put it on my list to figure >> > out how to get myself setup for graphics upstream. ;) >> >> Based on your other patch, this is temporary, right? >> >> I've been looking at DRM a lot lately. I think specifying the i2c bus >> in the hdmi chip or IP block node is wrong. If the I2C host is >> separate from the HDMI block, then it's only connection is to the HDMI >> connector. So the I2C host to the connector relationship is what the >> DT should describe. HPD gpio is similar. Now if the HDMI bridge >> controls DDC and HPD directly, then we don't need to describe those >> connections. > > Except... we don't generally model connectors under DRM as a general > rule. (The fbdev video connector stuff happened without very much > publicity afaics.) True. We have a binding with no users in the kernel. I think we should move in the direction of using it though. After all, it should not be a question of fbdev vs. DRM support for the binding. Connectors are certainly a concept within DRM, but they are very closely tied to encoder/bridge drivers ATM. We should be able to separate them to handle the common cases, but we have to have some consistency across DT bindings to do that. I don't yet have more concrete proposals though. > It's not always appropriate to split it out from the bridge in any > case. Consider something like a TDA998x where the TDA998x itself > takes care of reading the DDC bus, and doesn't provide an I2C-like > interface. If you try and split that into "bridge" or "encoder" and > "connector" you end up having to invent a new kind of I2C thing which > isn't an I2C adapter, or somehow squeezing an I2C adapter which isn't > into the I2C layer. Yes, that is fairly common (ADV75xx is same), and we would not describe an I2C bus in DT in that case. Same with HPD directly handled vs. a GPIO line. That is no different than what Doug has said: ddc-i2c-bus is present if using the SOC's I2C host and absent if using the HDMI block's DDC functionality. I'm only questioning the location of the property. Even if the bridge handles DDC and HPD, there is also regulator control for the connector power supply. That's most likely never going to be part of the bridge. > The TDA998x provides an interface to read a block of EDID at a time. > It always does the page register access. You don't get to read it > byte wise. It doesn't fit into I2C as an adapter at all. Agreed. Rob
On Thu, Sep 03, 2015 at 09:46:38AM -0500, Rob Herring wrote: > Yes, that is fairly common (ADV75xx is same), and we would not > describe an I2C bus in DT in that case. Same with HPD directly handled > vs. a GPIO line. That is no different than what Doug has said: > ddc-i2c-bus is present if using the SOC's I2C host and absent if using > the HDMI block's DDC functionality. I'm only questioning the location > of the property. No, I don't think that's what Doug wants. Doug wants the bridge's internal I2C host to be exposed, so he can number it through a DT alias.
On Thu, Sep 3, 2015 at 10:18 AM, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > On Thu, Sep 03, 2015 at 09:46:38AM -0500, Rob Herring wrote: >> Yes, that is fairly common (ADV75xx is same), and we would not >> describe an I2C bus in DT in that case. Same with HPD directly handled >> vs. a GPIO line. That is no different than what Doug has said: >> ddc-i2c-bus is present if using the SOC's I2C host and absent if using >> the HDMI block's DDC functionality. I'm only questioning the location >> of the property. > > No, I don't think that's what Doug wants. Doug wants the bridge's > internal I2C host to be exposed, so he can number it through a DT > alias. See his earlier reply and other patch[1] which states once the dw_hdmi built-in I2C controller support is added in mainline, then this property is not needed. For now, the SOC's general purpose I2C controller is used. Rob [1] https://lkml.org/lkml/2015/9/2/571
Hi, On Thu, Sep 3, 2015 at 8:18 AM, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > On Thu, Sep 03, 2015 at 09:46:38AM -0500, Rob Herring wrote: >> Yes, that is fairly common (ADV75xx is same), and we would not >> describe an I2C bus in DT in that case. Same with HPD directly handled >> vs. a GPIO line. That is no different than what Doug has said: >> ddc-i2c-bus is present if using the SOC's I2C host and absent if using >> the HDMI block's DDC functionality. I'm only questioning the location >> of the property. > > No, I don't think that's what Doug wants. Doug wants the bridge's > internal I2C host to be exposed, so he can number it through a DT > alias. Actually, my primary concern is that it _doesn't_ conflict with the DT aliases for busses I care about. If I didn't provide it with a number the i2c subsystem was assigning 0 which was conflicting with the real i2c bus 0. If the bus was simply not exposed that would have solved my problem just fine. ...but if it is being exposed, we need a way to give it a number. That being said, my (uninformed) opinion is that if the hardware does provide enough functionality to expose an i2c bus it's convenient to expose it, even if it's under a DEBUG config option. That allows you to use standard i2c tools during bringup or to debug strange problems. Obviously since there is hardware that doesn't expose a full i2c bus then the abstraction should handle that and let a driver just provide the data directly. -Doug
Hi, On Thu, Sep 3, 2015 at 8:46 AM, Rob Herring <robherring2@gmail.com> wrote: > On Thu, Sep 3, 2015 at 10:18 AM, Russell King - ARM Linux > <linux@arm.linux.org.uk> wrote: >> On Thu, Sep 03, 2015 at 09:46:38AM -0500, Rob Herring wrote: >>> Yes, that is fairly common (ADV75xx is same), and we would not >>> describe an I2C bus in DT in that case. Same with HPD directly handled >>> vs. a GPIO line. That is no different than what Doug has said: >>> ddc-i2c-bus is present if using the SOC's I2C host and absent if using >>> the HDMI block's DDC functionality. I'm only questioning the location >>> of the property. >> >> No, I don't think that's what Doug wants. Doug wants the bridge's >> internal I2C host to be exposed, so he can number it through a DT >> alias. > > See his earlier reply and other patch[1] which states once the dw_hdmi > built-in I2C controller support is added in mainline, then this > property is not needed. For now, the SOC's general purpose I2C > controller is used. > > Rob > > [1] https://lkml.org/lkml/2015/9/2/571 Hmmm, I think we're getting all mixed up here. To summarize: 1. On rk3288 you can mux the same pins on the SoC to _either_ be controlled by a generic rk3288 i2c controller (i2c5) or controlled by the dw_hdmi's i2c block. 2. At the moment, there's no code in mainline to handle the dw_hdmi's i2c block. 3. Right now there _is_ code in mainline to handle specifying "ddc-i2c-bus" and have it point to the generic rk3288 i2c controller. 4. So in mainline if you want to read an EDID, you've got to configure the pinmux as "i2c5" and add a "ddc-i2c-bus" reference to the HDMI section of the device tree. That's what most rk3288 boards do and (as far as I understand) matches existing bindings. The only reason veyron didn't have this reference was due to a small oversight when sending the DTS file upstream. 5. There are apparently benefits to using the builtin i2c controller in dw_hdmi. There's an outstanding patch add code to support the dw_hdmi's i2c block. 6. Once you start using the dw_hdmi's i2c block with the currently posted patch against mainline (to do this you not only need the patch but you need to remove the ddc-i2c-bus property, set the pinmux, and disable i2c5) then you'll see a bonafide i2c bus exposed to Linux. In my case this stole i2c0 away from the builtin SoC I2C bus and caused the SoC I2C bus to fail to probe. Doh. 7. I was trying to solve #6 by adding an "of_node" to the i2c bus which allowed me to give it a (non-conflicting) bus ID. -Doug
Am Donnerstag, den 03.09.2015, 09:04 -0700 schrieb Doug Anderson: > Hi, > > On Thu, Sep 3, 2015 at 8:46 AM, Rob Herring <robherring2@gmail.com> wrote: > > On Thu, Sep 3, 2015 at 10:18 AM, Russell King - ARM Linux > > <linux@arm.linux.org.uk> wrote: > >> On Thu, Sep 03, 2015 at 09:46:38AM -0500, Rob Herring wrote: > >>> Yes, that is fairly common (ADV75xx is same), and we would not > >>> describe an I2C bus in DT in that case. Same with HPD directly handled > >>> vs. a GPIO line. That is no different than what Doug has said: > >>> ddc-i2c-bus is present if using the SOC's I2C host and absent if using > >>> the HDMI block's DDC functionality. I'm only questioning the location > >>> of the property. > >> > >> No, I don't think that's what Doug wants. Doug wants the bridge's > >> internal I2C host to be exposed, so he can number it through a DT > >> alias. > > > > See his earlier reply and other patch[1] which states once the dw_hdmi > > built-in I2C controller support is added in mainline, then this > > property is not needed. For now, the SOC's general purpose I2C > > controller is used. > > > > Rob > > > > [1] https://lkml.org/lkml/2015/9/2/571 > > Hmmm, I think we're getting all mixed up here. To summarize: > > 1. On rk3288 you can mux the same pins on the SoC to _either_ be > controlled by a generic rk3288 i2c controller (i2c5) or controlled by > the dw_hdmi's i2c block. > > 2. At the moment, there's no code in mainline to handle the dw_hdmi's i2c block. > > 3. Right now there _is_ code in mainline to handle specifying > "ddc-i2c-bus" and have it point to the generic rk3288 i2c controller. > > 4. So in mainline if you want to read an EDID, you've got to configure > the pinmux as "i2c5" and add a "ddc-i2c-bus" reference to the HDMI > section of the device tree. That's what most rk3288 boards do and (as > far as I understand) matches existing bindings. The only reason > veyron didn't have this reference was due to a small oversight when > sending the DTS file upstream. > > 5. There are apparently benefits to using the builtin i2c controller > in dw_hdmi. There's an outstanding patch add code to support the > dw_hdmi's i2c block. > > 6. Once you start using the dw_hdmi's i2c block with the currently > posted patch against mainline (to do this you not only need the patch > but you need to remove the ddc-i2c-bus property, set the pinmux, and > disable i2c5) then you'll see a bonafide i2c bus exposed to Linux. In > my case this stole i2c0 away from the builtin SoC I2C bus and caused > the SoC I2C bus to fail to probe. Doh. > This shouldn't happen. I don't know if the patches landed yet, but I know Wolfram (i2c maintainer) had patches to reserve the range of bus numbers that are fixed by alias nodes and don't hand out those numbers to adapters with a dynamic bus number allocation. > 7. I was trying to solve #6 by adding an "of_node" to the i2c bus > which allowed me to give it a (non-conflicting) bus ID. > This should not be needed. Regards, Lucas
Lucas, On Thu, Sep 3, 2015 at 9:13 AM, Lucas Stach <l.stach@pengutronix.de> wrote: >> 6. Once you start using the dw_hdmi's i2c block with the currently >> posted patch against mainline (to do this you not only need the patch >> but you need to remove the ddc-i2c-bus property, set the pinmux, and >> disable i2c5) then you'll see a bonafide i2c bus exposed to Linux. In >> my case this stole i2c0 away from the builtin SoC I2C bus and caused >> the SoC I2C bus to fail to probe. Doh. >> > This shouldn't happen. I don't know if the patches landed yet, but I > know Wolfram (i2c maintainer) had patches to reserve the range of bus > numbers that are fixed by alias nodes and don't hand out those numbers > to adapters with a dynamic bus number allocation. > >> 7. I was trying to solve #6 by adding an "of_node" to the i2c bus >> which allowed me to give it a (non-conflicting) bus ID. >> > This should not be needed. Ah. It's very possible that the issue is due to the fact that I was testing this on an old kernel (3.14) that's missing that feature. OK, if that's fixed then at least the actual bug (making i2c0 fail to load) is not a problem. It'd still be nice to be able to assign a bus number to this bus just to make reading of the logs easier, but it's not a functionality requirement. -Doug
Hi, On Wed, Sep 2, 2015 at 2:25 PM, Douglas Anderson <dianders@chromium.org> wrote: > The ddc-i2c-bus property was missing from the veyron dtsi file since > downstream the ddc-i2c-bus was still being specified in rk3288.dtsi and > nobody noticed when the veyron dtsi was sent upstream. Add it. > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > Note: I noticed that this was wrong but I don't currently have > graphics up and running on upstream on veyron. Posting this anyway > since it's pretty clear that it's needed. If someone else wants to > try it out that'd be nice, otherwise I'll put it on my list to figure > out how to get myself setup for graphics upstream. ;) OK, I've used Heiko's "somewhat stable" branch to test this against something very close to mainline. Without my patch I can't read the HDMI EDID. With my patch I can. :) -Doug
Am Donnerstag, 3. September 2015, 14:00:26 schrieb Doug Anderson: > Hi, > > On Wed, Sep 2, 2015 at 2:25 PM, Douglas Anderson <dianders@chromium.org> wrote: > > The ddc-i2c-bus property was missing from the veyron dtsi file since > > downstream the ddc-i2c-bus was still being specified in rk3288.dtsi and > > nobody noticed when the veyron dtsi was sent upstream. Add it. > > > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > > --- > > Note: I noticed that this was wrong but I don't currently have > > graphics up and running on upstream on veyron. Posting this anyway > > since it's pretty clear that it's needed. If someone else wants to > > try it out that'd be nice, otherwise I'll put it on my list to figure > > out how to get myself setup for graphics upstream. ;) > > OK, I've used Heiko's "somewhat stable" branch to test this against > something very close to mainline. for people playing along, that is [0]. Essentially bleeding edge mainline + some backports from other maintainer trees +the last bits in the works, like the eDP support. > Without my patch I can't read the HDMI EDID. With my patch I can. :) The change follows the current dt-binding in the kernel for the (rockchip- variant of the) dw_hdmi, so if nobody keeps protesting loudly, I'd like to pick this up to get the hdmi on Veyron devices going. And I guess we'll remove this again (also in the other boards), once the dw_hdmi-internal i2c stuff is included - so this should be temporary as Rob hoped. Heiko [0] https://github.com/mmind/linux-rockchip/tree/v4.2-rockchip-somewhat-stable
On Thu, Sep 03, 2015 at 02:00:26PM -0700, Doug Anderson wrote: > On Wed, Sep 2, 2015 at 2:25 PM, Douglas Anderson <dianders@chromium.org> wrote: > > The ddc-i2c-bus property was missing from the veyron dtsi file since > > downstream the ddc-i2c-bus was still being specified in rk3288.dtsi and > > nobody noticed when the veyron dtsi was sent upstream. Add it. > > > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > > --- > > Note: I noticed that this was wrong but I don't currently have > > graphics up and running on upstream on veyron. Posting this anyway > > since it's pretty clear that it's needed. If someone else wants to > > try it out that'd be nice, otherwise I'll put it on my list to figure > > out how to get myself setup for graphics upstream. ;) > > OK, I've used Heiko's "somewhat stable" branch to test this against > something very close to mainline. > > Without my patch I can't read the HDMI EDID. With my patch I can. :) Same for veyron-jaq: Tested-by: Brian Norris <briannorris@chromium.org>
Am Mittwoch, 2. September 2015, 14:25:48 schrieb Douglas Anderson: > The ddc-i2c-bus property was missing from the veyron dtsi file since > downstream the ddc-i2c-bus was still being specified in rk3288.dtsi and > nobody noticed when the veyron dtsi was sent upstream. Add it. > > Signed-off-by: Douglas Anderson <dianders@chromium.org> I've applied that to a dts-fixes branch for 4.3 with Brian's Tested-by tag. As discussed this is temporary-only anyway till the internal i2c stuff of the dw_hdmi works. Heiko
diff --git a/arch/arm/boot/dts/rk3288-veyron.dtsi b/arch/arm/boot/dts/rk3288-veyron.dtsi index 2fa7a0d..275c78c 100644 --- a/arch/arm/boot/dts/rk3288-veyron.dtsi +++ b/arch/arm/boot/dts/rk3288-veyron.dtsi @@ -158,6 +158,7 @@ }; &hdmi { + ddc-i2c-bus = <&i2c5>; status = "okay"; };
The ddc-i2c-bus property was missing from the veyron dtsi file since downstream the ddc-i2c-bus was still being specified in rk3288.dtsi and nobody noticed when the veyron dtsi was sent upstream. Add it. Signed-off-by: Douglas Anderson <dianders@chromium.org> --- Note: I noticed that this was wrong but I don't currently have graphics up and running on upstream on veyron. Posting this anyway since it's pretty clear that it's needed. If someone else wants to try it out that'd be nice, otherwise I'll put it on my list to figure out how to get myself setup for graphics upstream. ;) arch/arm/boot/dts/rk3288-veyron.dtsi | 1 + 1 file changed, 1 insertion(+)