| Message ID | 1439427380-2436-2-git-send-email-eric@anholt.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 08/12/2015 06:56 PM, Eric Anholt wrote: > Signed-off-by: Eric Anholt <eric@anholt.net> This one definitely needs a patch description, since someone might not know what a VC4 is, and "git log" won't show the text from the binding doc itself. I'd suggest adding the initial paragraph of the binding doc as the patch description, or more. > diff --git a/Documentation/devicetree/bindings/gpu/brcm,bcm-vc4.txt b/Documentation/devicetree/bindings/gpu/brcm,bcm-vc4.txt > +Required properties for VC4: > +- compatible: Should be "brcm,vc4" > +- crtcs: List of references to pixelvalve scanout engines s/references to/phandles of/ would be more typical DT language. > +- hvss: List of references to HVS video scalers > +- encoders: List of references to output encoders (HDMI, SDTV) Would it make sense to make all those nodes child node of the vc4 object. That way, there's no need to have these lists of objects; they can be automatically built up as the DT is enumerated. I know that e.g. the NVIDIA Tegra host1x binding works this way, and I think it may have been inspired by other similar cases. Of course, this is only appropriate if the HW modules really are logically children of the VC4 HW module. Perhaps they aren't. If they aren't though, I wonder what this "vc4" module actually represents in HW? > +Required properties for HDMI > +- compatible: Should be "brcm,vc4-hdmi" > +- reg: Physical base address and length of the two register ranges > + ("HDMI" and "HD") I'd add "in that order" right before ")". > +Example: > +/ { > + soc { Minor nit: Examples often don't include any nodes "above" the nodes whose bindings are being documented.
Stephen Warren <swarren@wwwdotorg.org> writes: > On 08/12/2015 06:56 PM, Eric Anholt wrote: >> Signed-off-by: Eric Anholt <eric@anholt.net> > > This one definitely needs a patch description, since someone might not > know what a VC4 is, and "git log" won't show the text from the binding > doc itself. I'd suggest adding the initial paragraph of the binding doc > as the patch description, or more. > >> diff --git a/Documentation/devicetree/bindings/gpu/brcm,bcm-vc4.txt b/Documentation/devicetree/bindings/gpu/brcm,bcm-vc4.txt >> +- hvss: List of references to HVS video scalers >> +- encoders: List of references to output encoders (HDMI, SDTV) > > Would it make sense to make all those nodes child node of the vc4 > object. That way, there's no need to have these lists of objects; they > can be automatically built up as the DT is enumerated. I know that e.g. > the NVIDIA Tegra host1x binding works this way, and I think it may have > been inspired by other similar cases. I've looked at tegra, and the component system used by msm appears to be nicer than it. To follow tegra's model, it looks like I need to build this extra bus thing corresponding to host1x that is effectively the drivers/base/component.c code, so that I can get at vc4's structure from the component drivers. > Of course, this is only appropriate if the HW modules really are > logically children of the VC4 HW module. Perhaps they aren't. If they > aren't though, I wonder what this "vc4" module actually represents in HW? It's the subsystem, same as we use a subsystem node for msm, sti, rockchip, imx, and exynos. This appears to be the common model of how the collection of graphics-related components is represented in the DT.
On Mon, Aug 17, 2015 at 1:30 PM, Eric Anholt <eric@anholt.net> wrote: > Stephen Warren <swarren@wwwdotorg.org> writes: > >> On 08/12/2015 06:56 PM, Eric Anholt wrote: >>> Signed-off-by: Eric Anholt <eric@anholt.net> >> >> This one definitely needs a patch description, since someone might not >> know what a VC4 is, and "git log" won't show the text from the binding >> doc itself. I'd suggest adding the initial paragraph of the binding doc >> as the patch description, or more. >> >>> diff --git a/Documentation/devicetree/bindings/gpu/brcm,bcm-vc4.txt b/Documentation/devicetree/bindings/gpu/brcm,bcm-vc4.txt > >>> +- hvss: List of references to HVS video scalers >>> +- encoders: List of references to output encoders (HDMI, SDTV) >> >> Would it make sense to make all those nodes child node of the vc4 >> object. That way, there's no need to have these lists of objects; they >> can be automatically built up as the DT is enumerated. I know that e.g. >> the NVIDIA Tegra host1x binding works this way, and I think it may have >> been inspired by other similar cases. > > I've looked at tegra, and the component system used by msm appears to be > nicer than it. To follow tegra's model, it looks like I need to build > this extra bus thing corresponding to host1x that is effectively the > drivers/base/component.c code, so that I can get at vc4's structure from > the component drivers. > >> Of course, this is only appropriate if the HW modules really are >> logically children of the VC4 HW module. Perhaps they aren't. If they >> aren't though, I wonder what this "vc4" module actually represents in HW? > > It's the subsystem, same as we use a subsystem node for msm, sti, > rockchip, imx, and exynos. This appears to be the common model of how > the collection of graphics-related components is represented in the DT. I think most of these bindings are wrong. They are grouped together because that is what DRM wants not because that reflects the h/w. So convince me this is one block, not that it is what other people do. Rob
On Wed, Aug 12, 2015 at 7:56 PM, Eric Anholt <eric@anholt.net> wrote: > Signed-off-by: Eric Anholt <eric@anholt.net> > --- > .../devicetree/bindings/gpu/brcm,bcm-vc4.txt | 83 ++++++++++++++++++++++ > 1 file changed, 83 insertions(+) > create mode 100644 Documentation/devicetree/bindings/gpu/brcm,bcm-vc4.txt > > diff --git a/Documentation/devicetree/bindings/gpu/brcm,bcm-vc4.txt b/Documentation/devicetree/bindings/gpu/brcm,bcm-vc4.txt > new file mode 100644 > index 0000000..2b13e61 > --- /dev/null > +++ b/Documentation/devicetree/bindings/gpu/brcm,bcm-vc4.txt > @@ -0,0 +1,83 @@ > +Broadcom VC4 GPU > + > +The VC4 device present on the Raspberry Pi includes a display system > +with HDMI output and the HVS scaler for compositing display planes. > + > +Required properties for VC4: > +- compatible: Should be "brcm,vc4" > +- crtcs: List of references to pixelvalve scanout engines > +- hvss: List of references to HVS video scalers > +- encoders: List of references to output encoders (HDMI, SDTV) Creating these links is what the OF graph binding is for. Please use it. Plus this is a DRMism in the binding. > + > +Required properties for Pixel Valve: > +- compatible: Should be "brcm,vc4-pixelvalve" There's only one version of IP and all chips have the same bugs? You should have chip names in the compatible strings. > +- reg: Physical base address and length of the PV's registers > +- interrupts: The interrupt number > + See bindings/interrupt-controller/brcm,bcm2835-armctrl-ic.txt > + > +Required properties for HVS: > +- compatible: Should be "brcm,vc4-hvs" > +- reg: Physical base address and length of the HVS's registers > +- interrupts: The interrupt number > + See bindings/interrupt-controller/brcm,bcm2835-armctrl-ic.txt > + > +Required properties for HDMI > +- compatible: Should be "brcm,vc4-hdmi" > +- reg: Physical base address and length of the two register ranges > + ("HDMI" and "HD") > +- interrupts: The interrupt numbers > + See bindings/interrupt-controller/brcm,bcm2835-armctrl-ic.txt > +- ddc: phandle of the I2C controller used for DDC EDID probing > +- crtc: phandle to the pixelvalve CRTC the HDMI encoder is attached to Same comment about OF graph. > + > +Optional properties for HDMI: > +- hpd-gpio: The GPIO pin for HDMI hotplug detect (if it doesn't appear > + as an interrupt/status bit in the HDMI controller > + itself). See bindings/pinctrl/brcm,bcm2835-gpio.txt Use the hdmi-connector binding. This doesn't belong here. The clue is your comment in parenthesis. Rob
On Mon, Aug 24, 2015 at 9:47 AM, Rob Herring <robherring2@gmail.com> wrote: > On Mon, Aug 17, 2015 at 1:30 PM, Eric Anholt <eric@anholt.net> wrote: >> Stephen Warren <swarren@wwwdotorg.org> writes: >> >>> On 08/12/2015 06:56 PM, Eric Anholt wrote: >>>> Signed-off-by: Eric Anholt <eric@anholt.net> >>> >>> This one definitely needs a patch description, since someone might not >>> know what a VC4 is, and "git log" won't show the text from the binding >>> doc itself. I'd suggest adding the initial paragraph of the binding doc >>> as the patch description, or more. >>> >>>> diff --git a/Documentation/devicetree/bindings/gpu/brcm,bcm-vc4.txt b/Documentation/devicetree/bindings/gpu/brcm,bcm-vc4.txt >> >>>> +- hvss: List of references to HVS video scalers >>>> +- encoders: List of references to output encoders (HDMI, SDTV) >>> >>> Would it make sense to make all those nodes child node of the vc4 >>> object. That way, there's no need to have these lists of objects; they >>> can be automatically built up as the DT is enumerated. I know that e.g. >>> the NVIDIA Tegra host1x binding works this way, and I think it may have >>> been inspired by other similar cases. >> >> I've looked at tegra, and the component system used by msm appears to be >> nicer than it. To follow tegra's model, it looks like I need to build >> this extra bus thing corresponding to host1x that is effectively the >> drivers/base/component.c code, so that I can get at vc4's structure from >> the component drivers. >> >>> Of course, this is only appropriate if the HW modules really are >>> logically children of the VC4 HW module. Perhaps they aren't. If they >>> aren't though, I wonder what this "vc4" module actually represents in HW? >> >> It's the subsystem, same as we use a subsystem node for msm, sti, >> rockchip, imx, and exynos. This appears to be the common model of how >> the collection of graphics-related components is represented in the DT. > > I think most of these bindings are wrong. They are grouped together > because that is what DRM wants not because that reflects the h/w. So > convince me this is one block, not that it is what other people do. I think, when it comes to more complex driver subsystems (like drm in particular) we have a bit of mismatch between how things look from the "pure hw ignoring sw" perspective, and the "how sw and in particular userspace expects things" perspective. Maybe it is less a problem in other subsystems, where bindings map to things that are only visible in the kernel, or well defined devices like uart or sata controller. But when given the choice, I'm going to err on the side of not confusing userspace and the large software stack that sits above drm/kms, over dt purity. Maybe it would be nice to have a sort of dt overlay that adds the bits needed to tie together hw blocks that should be assembled into a single logical device for linux and userspace (but maybe not some other hypothetical operating system). But so far that doesn't exist. All we have is a hammer (devicetree), everything looks like a nail. End result is we end up adding some things in the bindings which aren't purely about the hw. Until someone invents a screwdriver, I'm not sure what else to do. In the end, other hypothetical OS is free to ignore those extra fields in the bindings if it doesn't need them. So meh? BR, -R > Rob > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Tue, Aug 25, 2015 at 3:42 PM, Rob Clark <robdclark@gmail.com> wrote: > On Mon, Aug 24, 2015 at 9:47 AM, Rob Herring <robherring2@gmail.com> wrote: >> On Mon, Aug 17, 2015 at 1:30 PM, Eric Anholt <eric@anholt.net> wrote: >>> Stephen Warren <swarren@wwwdotorg.org> writes: >>> >>>> On 08/12/2015 06:56 PM, Eric Anholt wrote: >>>>> Signed-off-by: Eric Anholt <eric@anholt.net> >>>> >>>> This one definitely needs a patch description, since someone might not >>>> know what a VC4 is, and "git log" won't show the text from the binding >>>> doc itself. I'd suggest adding the initial paragraph of the binding doc >>>> as the patch description, or more. >>>> >>>>> diff --git a/Documentation/devicetree/bindings/gpu/brcm,bcm-vc4.txt b/Documentation/devicetree/bindings/gpu/brcm,bcm-vc4.txt >>> >>>>> +- hvss: List of references to HVS video scalers >>>>> +- encoders: List of references to output encoders (HDMI, SDTV) >>>> >>>> Would it make sense to make all those nodes child node of the vc4 >>>> object. That way, there's no need to have these lists of objects; they >>>> can be automatically built up as the DT is enumerated. I know that e.g. >>>> the NVIDIA Tegra host1x binding works this way, and I think it may have >>>> been inspired by other similar cases. >>> >>> I've looked at tegra, and the component system used by msm appears to be >>> nicer than it. To follow tegra's model, it looks like I need to build >>> this extra bus thing corresponding to host1x that is effectively the >>> drivers/base/component.c code, so that I can get at vc4's structure from >>> the component drivers. >>> >>>> Of course, this is only appropriate if the HW modules really are >>>> logically children of the VC4 HW module. Perhaps they aren't. If they >>>> aren't though, I wonder what this "vc4" module actually represents in HW? >>> >>> It's the subsystem, same as we use a subsystem node for msm, sti, >>> rockchip, imx, and exynos. This appears to be the common model of how >>> the collection of graphics-related components is represented in the DT. >> >> I think most of these bindings are wrong. They are grouped together >> because that is what DRM wants not because that reflects the h/w. So >> convince me this is one block, not that it is what other people do. > > I think, when it comes to more complex driver subsystems (like drm in > particular) we have a bit of mismatch between how things look from the > "pure hw ignoring sw" perspective, and the "how sw and in particular > userspace expects things" perspective. Maybe it is less a problem in > other subsystems, where bindings map to things that are only visible > in the kernel, or well defined devices like uart or sata controller. > But when given the choice, I'm going to err on the side of not > confusing userspace and the large software stack that sits above > drm/kms, over dt purity. I wasn't implying that this should get exposed to userspace as components. V4L2 has gone that route with media controller and sub-devs. Perhaps that is needed for DRM, perhaps not. For the moment, I definitely agree the kernel should hide most/all of those details, but I don't think that means DT has to hide the details or know what components are handled by a single driver. My point was that on the DT side we have a mixture of OF graph usage, parent-child nodes or custom phandles (this case) to describe the relationships between h/w components. That's not necessarily wrong, but we should have some rules around how certain relationships are described. Then in the drivers we have a mixture of deferred probe, component API, and custom inter-module APIs to control init order. We then have a mixture of all those which leads to very few if any drivers having the same overall structure that could be shared. Should we mandate using the component API for h/w that is discrete blocks? Should we throw out the component API for something else? Can we tie the graph parsing and component API together with common code? > Maybe it would be nice to have a sort of dt overlay that adds the bits > needed to tie together hw blocks that should be assembled into a > single logical device for linux and userspace (but maybe not some > other hypothetical operating system). But so far that doesn't exist. OF graph is supposed to do this. OF graph is a double edged sword. It is very flexible, but then each platform can do something different. We need to have some level of requirements around how the OF graph is used. As an example, any system with an HDMI connector should have an "hdmi-connector" compatible node or encoder/bridge chips/blocks must have certain ports defined. > All we have is a hammer (devicetree), everything looks like a nail. > End result is we end up adding some things in the bindings which > aren't purely about the hw. Until someone invents a screwdriver, I'm > not sure what else to do. In the end, other hypothetical OS is free > to ignore those extra fields in the bindings if it doesn't need them. > So meh? We really want to err on the side of fewer bindings, not more as once used they are an ABI. Rob
On Fri, Aug 14, 2015 at 10:38:54PM -0600, Stephen Warren wrote: > On 08/12/2015 06:56 PM, Eric Anholt wrote: > > Signed-off-by: Eric Anholt <eric@anholt.net> > > This one definitely needs a patch description, since someone might not > know what a VC4 is, and "git log" won't show the text from the binding > doc itself. I'd suggest adding the initial paragraph of the binding doc > as the patch description, or more. > > > diff --git a/Documentation/devicetree/bindings/gpu/brcm,bcm-vc4.txt b/Documentation/devicetree/bindings/gpu/brcm,bcm-vc4.txt > > > +Required properties for VC4: > > +- compatible: Should be "brcm,vc4" > > +- crtcs: List of references to pixelvalve scanout engines > > s/references to/phandles of/ would be more typical DT language. > > > +- hvss: List of references to HVS video scalers > > +- encoders: List of references to output encoders (HDMI, SDTV) > > Would it make sense to make all those nodes child node of the vc4 > object. That way, there's no need to have these lists of objects; they > can be automatically built up as the DT is enumerated. I know that e.g. > the NVIDIA Tegra host1x binding works this way, and I think it may have > been inspired by other similar cases. Actually the host1x binding was the first of its kind. Unfortunately for the purposes of this discussion (but fortunately otherwise) Tegra is the odd-ball it seems. host1x is indeed a physical parent of all the devices pertaining to the DRM driver, so the DT description is accurate from a hardware point of view while at the same time giving us a top-level device that we can bind against. Now for most other cases it seems like the central piece that they are missing is this top-level device, hence why the "virtual DRM subsystem device" is instantiated. I tried to argue in the past that it wasn't a proper description and proposed alternatives, but I was always pretty much the only one with this viewpoint, so my comments ended up being ignored. Technically there is nothing that would prevent other drivers from doing without the lists of phandles. On Tegra, again this might be special for this particular hardware, we've never had a need to describe these kinds of relationships. Each display controller can essentially drive each of the outputs, which we deal with elegantly by setting the .possible_crtcs mask of the encoders. Also, to pull together all devices that are needed to make up the DRM device, we use a list of compatible strings in the driver to find these devices. Then as each of them registers with the host1x bus we wait for the subdevice list to become empty and ->probe() the component host1x device. Note that while this predates component/master, this is all very similar in principle (Russell and I did have some discussions about this back at the time, but I'm not sure how much, if anything, he took as inspiration from the host1x infrastructure). After component/master was merged I did try to convert Tegra DRM to use it. Things looked pretty good, but ended up not working because each componentized device must have a unique master device. This poses a problem because on Tegra we needed the top- level (i.e. master) device to be shared among multiple drivers. I posted patches at some point to try and fix remedy the situation but wasn't able to elicit any reactions, and since I had something that was working did not pursue this any further. Thierry
On Tue, Aug 25, 2015 at 04:42:18PM -0400, Rob Clark wrote: > On Mon, Aug 24, 2015 at 9:47 AM, Rob Herring <robherring2@gmail.com> wrote: > > On Mon, Aug 17, 2015 at 1:30 PM, Eric Anholt <eric@anholt.net> wrote: > >> Stephen Warren <swarren@wwwdotorg.org> writes: > >> > >>> On 08/12/2015 06:56 PM, Eric Anholt wrote: > >>>> Signed-off-by: Eric Anholt <eric@anholt.net> > >>> > >>> This one definitely needs a patch description, since someone might not > >>> know what a VC4 is, and "git log" won't show the text from the binding > >>> doc itself. I'd suggest adding the initial paragraph of the binding doc > >>> as the patch description, or more. > >>> > >>>> diff --git a/Documentation/devicetree/bindings/gpu/brcm,bcm-vc4.txt b/Documentation/devicetree/bindings/gpu/brcm,bcm-vc4.txt > >> > >>>> +- hvss: List of references to HVS video scalers > >>>> +- encoders: List of references to output encoders (HDMI, SDTV) > >>> > >>> Would it make sense to make all those nodes child node of the vc4 > >>> object. That way, there's no need to have these lists of objects; they > >>> can be automatically built up as the DT is enumerated. I know that e.g. > >>> the NVIDIA Tegra host1x binding works this way, and I think it may have > >>> been inspired by other similar cases. > >> > >> I've looked at tegra, and the component system used by msm appears to be > >> nicer than it. To follow tegra's model, it looks like I need to build > >> this extra bus thing corresponding to host1x that is effectively the > >> drivers/base/component.c code, so that I can get at vc4's structure from > >> the component drivers. > >> > >>> Of course, this is only appropriate if the HW modules really are > >>> logically children of the VC4 HW module. Perhaps they aren't. If they > >>> aren't though, I wonder what this "vc4" module actually represents in HW? > >> > >> It's the subsystem, same as we use a subsystem node for msm, sti, > >> rockchip, imx, and exynos. This appears to be the common model of how > >> the collection of graphics-related components is represented in the DT. > > > > I think most of these bindings are wrong. They are grouped together > > because that is what DRM wants not because that reflects the h/w. So > > convince me this is one block, not that it is what other people do. > > I think, when it comes to more complex driver subsystems (like drm in > particular) we have a bit of mismatch between how things look from the > "pure hw ignoring sw" perspective, and the "how sw and in particular > userspace expects things" perspective. Maybe it is less a problem in > other subsystems, where bindings map to things that are only visible > in the kernel, or well defined devices like uart or sata controller. > But when given the choice, I'm going to err on the side of not > confusing userspace and the large software stack that sits above > drm/kms, over dt purity. > > Maybe it would be nice to have a sort of dt overlay that adds the bits > needed to tie together hw blocks that should be assembled into a > single logical device for linux and userspace (but maybe not some > other hypothetical operating system). But so far that doesn't exist. > All we have is a hammer (devicetree), everything looks like a nail. > End result is we end up adding some things in the bindings which > aren't purely about the hw. Until someone invents a screwdriver, I'm > not sure what else to do. In the end, other hypothetical OS is free > to ignore those extra fields in the bindings if it doesn't need them. > So meh? I thought we agreed a while back that these kind of "pull everything for the logical device together" dt nodes which just have piles of phandles are totally accepted? At least that's the point behind the component helpers, and Eric even suggested to create dt-specific component helpers to cut down a bit on the usual boilerplate. dt maintainers are also fine with this approach afaik. From my understanding tegra with the host1x bus really is the odd one out and not the norm. Given that and with the hope that we'll eventually see a dt-enabled component functions to standardize this even more the overall concept is Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> Cheers, Daniel
On Wed, Aug 26, 2015 at 01:52:29PM +0200, Daniel Vetter wrote: > On Tue, Aug 25, 2015 at 04:42:18PM -0400, Rob Clark wrote: > > On Mon, Aug 24, 2015 at 9:47 AM, Rob Herring <robherring2@gmail.com> wrote: > > > On Mon, Aug 17, 2015 at 1:30 PM, Eric Anholt <eric@anholt.net> wrote: > > >> Stephen Warren <swarren@wwwdotorg.org> writes: > > >> > > >>> On 08/12/2015 06:56 PM, Eric Anholt wrote: > > >>>> Signed-off-by: Eric Anholt <eric@anholt.net> > > >>> > > >>> This one definitely needs a patch description, since someone might not > > >>> know what a VC4 is, and "git log" won't show the text from the binding > > >>> doc itself. I'd suggest adding the initial paragraph of the binding doc > > >>> as the patch description, or more. > > >>> > > >>>> diff --git a/Documentation/devicetree/bindings/gpu/brcm,bcm-vc4.txt b/Documentation/devicetree/bindings/gpu/brcm,bcm-vc4.txt > > >> > > >>>> +- hvss: List of references to HVS video scalers > > >>>> +- encoders: List of references to output encoders (HDMI, SDTV) > > >>> > > >>> Would it make sense to make all those nodes child node of the vc4 > > >>> object. That way, there's no need to have these lists of objects; they > > >>> can be automatically built up as the DT is enumerated. I know that e.g. > > >>> the NVIDIA Tegra host1x binding works this way, and I think it may have > > >>> been inspired by other similar cases. > > >> > > >> I've looked at tegra, and the component system used by msm appears to be > > >> nicer than it. To follow tegra's model, it looks like I need to build > > >> this extra bus thing corresponding to host1x that is effectively the > > >> drivers/base/component.c code, so that I can get at vc4's structure from > > >> the component drivers. > > >> > > >>> Of course, this is only appropriate if the HW modules really are > > >>> logically children of the VC4 HW module. Perhaps they aren't. If they > > >>> aren't though, I wonder what this "vc4" module actually represents in HW? > > >> > > >> It's the subsystem, same as we use a subsystem node for msm, sti, > > >> rockchip, imx, and exynos. This appears to be the common model of how > > >> the collection of graphics-related components is represented in the DT. > > > > > > I think most of these bindings are wrong. They are grouped together > > > because that is what DRM wants not because that reflects the h/w. So > > > convince me this is one block, not that it is what other people do. > > > > I think, when it comes to more complex driver subsystems (like drm in > > particular) we have a bit of mismatch between how things look from the > > "pure hw ignoring sw" perspective, and the "how sw and in particular > > userspace expects things" perspective. Maybe it is less a problem in > > other subsystems, where bindings map to things that are only visible > > in the kernel, or well defined devices like uart or sata controller. > > But when given the choice, I'm going to err on the side of not > > confusing userspace and the large software stack that sits above > > drm/kms, over dt purity. > > > > Maybe it would be nice to have a sort of dt overlay that adds the bits > > needed to tie together hw blocks that should be assembled into a > > single logical device for linux and userspace (but maybe not some > > other hypothetical operating system). But so far that doesn't exist. > > All we have is a hammer (devicetree), everything looks like a nail. > > End result is we end up adding some things in the bindings which > > aren't purely about the hw. Until someone invents a screwdriver, I'm > > not sure what else to do. In the end, other hypothetical OS is free > > to ignore those extra fields in the bindings if it doesn't need them. > > So meh? > > I thought we agreed a while back that these kind of "pull everything for > the logical device together" dt nodes which just have piles of phandles > are totally accepted? At least that's the point behind the component > helpers, and Eric even suggested to create dt-specific component helpers > to cut down a bit on the usual boilerplate. dt maintainers are also fine > with this approach afaik. From my understanding tegra with the host1x bus > really is the odd one out and not the norm. I agree that in many aspects Tegra is somewhat special. But the same principles that the host1x infrastructure uses could be implemented in a similar way for other DRM drivers. You can easily collect information about subdevices by walking the device tree and matching on known compatible strings. And you can also instantiate the top-level device from driver code rather than have it in DT. It should still be possible to make this work without an artificial device node in DT. The component and master infrastructure is largely orthogonal to that, and as far as I remember the only blocker is the need for a top-level device. I wonder if perhaps this could be made to work by binding the master to the top- level SoC device. Obviously adding the node in DT is easier, but to my knowledge easy has never been a good excuse for mangling DT. Perhaps that's different these days... Thierry
On Wed, Aug 26, 2015 at 7:09 AM, Thierry Reding <thierry.reding@gmail.com> wrote: > On Wed, Aug 26, 2015 at 01:52:29PM +0200, Daniel Vetter wrote: >> On Tue, Aug 25, 2015 at 04:42:18PM -0400, Rob Clark wrote: >> > On Mon, Aug 24, 2015 at 9:47 AM, Rob Herring <robherring2@gmail.com> wrote: >> > > On Mon, Aug 17, 2015 at 1:30 PM, Eric Anholt <eric@anholt.net> wrote: >> > >> Stephen Warren <swarren@wwwdotorg.org> writes: >> > >> >> > >>> On 08/12/2015 06:56 PM, Eric Anholt wrote: >> > >>>> Signed-off-by: Eric Anholt <eric@anholt.net> >> > >>> >> > >>> This one definitely needs a patch description, since someone might not >> > >>> know what a VC4 is, and "git log" won't show the text from the binding >> > >>> doc itself. I'd suggest adding the initial paragraph of the binding doc >> > >>> as the patch description, or more. >> > >>> >> > >>>> diff --git a/Documentation/devicetree/bindings/gpu/brcm,bcm-vc4.txt b/Documentation/devicetree/bindings/gpu/brcm,bcm-vc4.txt >> > >> >> > >>>> +- hvss: List of references to HVS video scalers >> > >>>> +- encoders: List of references to output encoders (HDMI, SDTV) >> > >>> >> > >>> Would it make sense to make all those nodes child node of the vc4 >> > >>> object. That way, there's no need to have these lists of objects; they >> > >>> can be automatically built up as the DT is enumerated. I know that e.g. >> > >>> the NVIDIA Tegra host1x binding works this way, and I think it may have >> > >>> been inspired by other similar cases. >> > >> >> > >> I've looked at tegra, and the component system used by msm appears to be >> > >> nicer than it. To follow tegra's model, it looks like I need to build >> > >> this extra bus thing corresponding to host1x that is effectively the >> > >> drivers/base/component.c code, so that I can get at vc4's structure from >> > >> the component drivers. >> > >> >> > >>> Of course, this is only appropriate if the HW modules really are >> > >>> logically children of the VC4 HW module. Perhaps they aren't. If they >> > >>> aren't though, I wonder what this "vc4" module actually represents in HW? >> > >> >> > >> It's the subsystem, same as we use a subsystem node for msm, sti, >> > >> rockchip, imx, and exynos. This appears to be the common model of how >> > >> the collection of graphics-related components is represented in the DT. >> > > >> > > I think most of these bindings are wrong. They are grouped together >> > > because that is what DRM wants not because that reflects the h/w. So >> > > convince me this is one block, not that it is what other people do. >> > >> > I think, when it comes to more complex driver subsystems (like drm in >> > particular) we have a bit of mismatch between how things look from the >> > "pure hw ignoring sw" perspective, and the "how sw and in particular >> > userspace expects things" perspective. Maybe it is less a problem in >> > other subsystems, where bindings map to things that are only visible >> > in the kernel, or well defined devices like uart or sata controller. >> > But when given the choice, I'm going to err on the side of not >> > confusing userspace and the large software stack that sits above >> > drm/kms, over dt purity. >> > >> > Maybe it would be nice to have a sort of dt overlay that adds the bits >> > needed to tie together hw blocks that should be assembled into a >> > single logical device for linux and userspace (but maybe not some >> > other hypothetical operating system). But so far that doesn't exist. >> > All we have is a hammer (devicetree), everything looks like a nail. >> > End result is we end up adding some things in the bindings which >> > aren't purely about the hw. Until someone invents a screwdriver, I'm >> > not sure what else to do. In the end, other hypothetical OS is free >> > to ignore those extra fields in the bindings if it doesn't need them. >> > So meh? >> >> I thought we agreed a while back that these kind of "pull everything for >> the logical device together" dt nodes which just have piles of phandles >> are totally accepted? At least that's the point behind the component >> helpers, and Eric even suggested to create dt-specific component helpers >> to cut down a bit on the usual boilerplate. dt maintainers are also fine >> with this approach afaik. From my understanding tegra with the host1x bus >> really is the odd one out and not the norm. > > I agree that in many aspects Tegra is somewhat special. But the same > principles that the host1x infrastructure uses could be implemented in a > similar way for other DRM drivers. You can easily collect information > about subdevices by walking the device tree and matching on known > compatible strings. And you can also instantiate the top-level device > from driver code rather than have it in DT. It should still be possible > to make this work without an artificial device node in DT. The component > and master infrastructure is largely orthogonal to that, and as far as I > remember the only blocker is the need for a top-level device. I wonder > if perhaps this could be made to work by binding the master to the top- > level SoC device. > > Obviously adding the node in DT is easier, but to my knowledge easy has > never been a good excuse for mangling DT. Perhaps that's different these > days... I agree we should avoid the virtual node if possible. It is certainly possible as I started out with one and removed it. At least in my case, it essentially required the drm_device and crtc to be a single driver rather than 2 components. However, I'm more concerned that we are consistent from platform to platform where it makes sense than whether we have a somewhat questionable node or not. Rob
On 27 August 2015 at 00:30, Rob Herring <robherring2@gmail.com> wrote: > On Wed, Aug 26, 2015 at 7:09 AM, Thierry Reding > <thierry.reding@gmail.com> wrote: >> On Wed, Aug 26, 2015 at 01:52:29PM +0200, Daniel Vetter wrote: >>> On Tue, Aug 25, 2015 at 04:42:18PM -0400, Rob Clark wrote: >>> > On Mon, Aug 24, 2015 at 9:47 AM, Rob Herring <robherring2@gmail.com> wrote: >>> > > On Mon, Aug 17, 2015 at 1:30 PM, Eric Anholt <eric@anholt.net> wrote: >>> > >> Stephen Warren <swarren@wwwdotorg.org> writes: >>> > >> >>> > >>> On 08/12/2015 06:56 PM, Eric Anholt wrote: >>> > >>>> Signed-off-by: Eric Anholt <eric@anholt.net> >>> > >>> >>> > >>> This one definitely needs a patch description, since someone might not >>> > >>> know what a VC4 is, and "git log" won't show the text from the binding >>> > >>> doc itself. I'd suggest adding the initial paragraph of the binding doc >>> > >>> as the patch description, or more. >>> > >>> >>> > >>>> diff --git a/Documentation/devicetree/bindings/gpu/brcm,bcm-vc4.txt b/Documentation/devicetree/bindings/gpu/brcm,bcm-vc4.txt >>> > >> >>> > >>>> +- hvss: List of references to HVS video scalers >>> > >>>> +- encoders: List of references to output encoders (HDMI, SDTV) >>> > >>> >>> > >>> Would it make sense to make all those nodes child node of the vc4 >>> > >>> object. That way, there's no need to have these lists of objects; they >>> > >>> can be automatically built up as the DT is enumerated. I know that e.g. >>> > >>> the NVIDIA Tegra host1x binding works this way, and I think it may have >>> > >>> been inspired by other similar cases. >>> > >> >>> > >> I've looked at tegra, and the component system used by msm appears to be >>> > >> nicer than it. To follow tegra's model, it looks like I need to build >>> > >> this extra bus thing corresponding to host1x that is effectively the >>> > >> drivers/base/component.c code, so that I can get at vc4's structure from >>> > >> the component drivers. >>> > >> >>> > >>> Of course, this is only appropriate if the HW modules really are >>> > >>> logically children of the VC4 HW module. Perhaps they aren't. If they >>> > >>> aren't though, I wonder what this "vc4" module actually represents in HW? >>> > >> >>> > >> It's the subsystem, same as we use a subsystem node for msm, sti, >>> > >> rockchip, imx, and exynos. This appears to be the common model of how >>> > >> the collection of graphics-related components is represented in the DT. >>> > > >>> > > I think most of these bindings are wrong. They are grouped together >>> > > because that is what DRM wants not because that reflects the h/w. So >>> > > convince me this is one block, not that it is what other people do. >>> > >>> > I think, when it comes to more complex driver subsystems (like drm in >>> > particular) we have a bit of mismatch between how things look from the >>> > "pure hw ignoring sw" perspective, and the "how sw and in particular >>> > userspace expects things" perspective. Maybe it is less a problem in >>> > other subsystems, where bindings map to things that are only visible >>> > in the kernel, or well defined devices like uart or sata controller. >>> > But when given the choice, I'm going to err on the side of not >>> > confusing userspace and the large software stack that sits above >>> > drm/kms, over dt purity. >>> > >>> > Maybe it would be nice to have a sort of dt overlay that adds the bits >>> > needed to tie together hw blocks that should be assembled into a >>> > single logical device for linux and userspace (but maybe not some >>> > other hypothetical operating system). But so far that doesn't exist. >>> > All we have is a hammer (devicetree), everything looks like a nail. >>> > End result is we end up adding some things in the bindings which >>> > aren't purely about the hw. Until someone invents a screwdriver, I'm >>> > not sure what else to do. In the end, other hypothetical OS is free >>> > to ignore those extra fields in the bindings if it doesn't need them. >>> > So meh? >>> >>> I thought we agreed a while back that these kind of "pull everything for >>> the logical device together" dt nodes which just have piles of phandles >>> are totally accepted? At least that's the point behind the component >>> helpers, and Eric even suggested to create dt-specific component helpers >>> to cut down a bit on the usual boilerplate. dt maintainers are also fine >>> with this approach afaik. From my understanding tegra with the host1x bus >>> really is the odd one out and not the norm. >> >> I agree that in many aspects Tegra is somewhat special. But the same >> principles that the host1x infrastructure uses could be implemented in a >> similar way for other DRM drivers. You can easily collect information >> about subdevices by walking the device tree and matching on known >> compatible strings. And you can also instantiate the top-level device >> from driver code rather than have it in DT. It should still be possible >> to make this work without an artificial device node in DT. The component >> and master infrastructure is largely orthogonal to that, and as far as I >> remember the only blocker is the need for a top-level device. I wonder >> if perhaps this could be made to work by binding the master to the top- >> level SoC device. >> >> Obviously adding the node in DT is easier, but to my knowledge easy has >> never been a good excuse for mangling DT. Perhaps that's different these >> days... > > I agree we should avoid the virtual node if possible. It is certainly > possible as I started out with one and removed it. At least in my > case, it essentially required the drm_device and crtc to be a single > driver rather than 2 components. However, I'm more concerned that we > are consistent from platform to platform where it makes sense than > whether we have a somewhat questionable node or not. > http://lists.freedesktop.org/archives/dri-devel/2013-July/041159.html So can we at least have some continuity of decision making, bikeshedding this every time we submit a driver isn't giving me any hope going forward. Dave.
On Wed, Aug 26, 2015 at 3:59 PM, Dave Airlie <airlied@gmail.com> wrote: > On 27 August 2015 at 00:30, Rob Herring <robherring2@gmail.com> wrote: >> On Wed, Aug 26, 2015 at 7:09 AM, Thierry Reding >> <thierry.reding@gmail.com> wrote: >>> On Wed, Aug 26, 2015 at 01:52:29PM +0200, Daniel Vetter wrote: >>>> On Tue, Aug 25, 2015 at 04:42:18PM -0400, Rob Clark wrote: >>>> > On Mon, Aug 24, 2015 at 9:47 AM, Rob Herring <robherring2@gmail.com> wrote: >>>> > > On Mon, Aug 17, 2015 at 1:30 PM, Eric Anholt <eric@anholt.net> wrote: >>>> > >> Stephen Warren <swarren@wwwdotorg.org> writes: >>>> > >> >>>> > >>> On 08/12/2015 06:56 PM, Eric Anholt wrote: >>>> > >>>> Signed-off-by: Eric Anholt <eric@anholt.net> [...] >>>> > >>>> +- hvss: List of references to HVS video scalers >>>> > >>>> +- encoders: List of references to output encoders (HDMI, SDTV) [...] >>>> > >>> Of course, this is only appropriate if the HW modules really are >>>> > >>> logically children of the VC4 HW module. Perhaps they aren't. If they >>>> > >>> aren't though, I wonder what this "vc4" module actually represents in HW? >>>> > >> >>>> > >> It's the subsystem, same as we use a subsystem node for msm, sti, >>>> > >> rockchip, imx, and exynos. This appears to be the common model of how >>>> > >> the collection of graphics-related components is represented in the DT. >>>> > > >>>> > > I think most of these bindings are wrong. They are grouped together >>>> > > because that is what DRM wants not because that reflects the h/w. So >>>> > > convince me this is one block, not that it is what other people do. >>>> > >>>> > I think, when it comes to more complex driver subsystems (like drm in >>>> > particular) we have a bit of mismatch between how things look from the >>>> > "pure hw ignoring sw" perspective, and the "how sw and in particular >>>> > userspace expects things" perspective. Maybe it is less a problem in >>>> > other subsystems, where bindings map to things that are only visible >>>> > in the kernel, or well defined devices like uart or sata controller. >>>> > But when given the choice, I'm going to err on the side of not >>>> > confusing userspace and the large software stack that sits above >>>> > drm/kms, over dt purity. >>>> > >>>> > Maybe it would be nice to have a sort of dt overlay that adds the bits >>>> > needed to tie together hw blocks that should be assembled into a >>>> > single logical device for linux and userspace (but maybe not some >>>> > other hypothetical operating system). But so far that doesn't exist. >>>> > All we have is a hammer (devicetree), everything looks like a nail. >>>> > End result is we end up adding some things in the bindings which >>>> > aren't purely about the hw. Until someone invents a screwdriver, I'm >>>> > not sure what else to do. In the end, other hypothetical OS is free >>>> > to ignore those extra fields in the bindings if it doesn't need them. >>>> > So meh? >>>> >>>> I thought we agreed a while back that these kind of "pull everything for >>>> the logical device together" dt nodes which just have piles of phandles >>>> are totally accepted? At least that's the point behind the component >>>> helpers, and Eric even suggested to create dt-specific component helpers >>>> to cut down a bit on the usual boilerplate. dt maintainers are also fine >>>> with this approach afaik. From my understanding tegra with the host1x bus >>>> really is the odd one out and not the norm. >>> >>> I agree that in many aspects Tegra is somewhat special. But the same >>> principles that the host1x infrastructure uses could be implemented in a >>> similar way for other DRM drivers. You can easily collect information >>> about subdevices by walking the device tree and matching on known >>> compatible strings. And you can also instantiate the top-level device >>> from driver code rather than have it in DT. It should still be possible >>> to make this work without an artificial device node in DT. The component >>> and master infrastructure is largely orthogonal to that, and as far as I >>> remember the only blocker is the need for a top-level device. I wonder >>> if perhaps this could be made to work by binding the master to the top- >>> level SoC device. >>> >>> Obviously adding the node in DT is easier, but to my knowledge easy has >>> never been a good excuse for mangling DT. Perhaps that's different these >>> days... >> >> I agree we should avoid the virtual node if possible. It is certainly >> possible as I started out with one and removed it. At least in my >> case, it essentially required the drm_device and crtc to be a single >> driver rather than 2 components. However, I'm more concerned that we >> are consistent from platform to platform where it makes sense than >> whether we have a somewhat questionable node or not. >> > http://lists.freedesktop.org/archives/dri-devel/2013-July/041159.html > > So can we at least have some continuity of decision making, > bikeshedding this every time we submit a driver isn't giving me any > hope going forward. As I said, whether we have a virtual node or not is a minor part of having some consistency in bindings and was the only part Grant discussed. We have though diverged from the specific problems with this binding which is that it invents yet another way to describe the relationship between components. The use of DRM component names in the binding properties is the first clue. As to what is the "right" way, well if that is known or documented I've seen no evidence. Defining what that is and having the infrastructure in place to support it is what I'm interested in. Rob
diff --git a/Documentation/devicetree/bindings/gpu/brcm,bcm-vc4.txt b/Documentation/devicetree/bindings/gpu/brcm,bcm-vc4.txt new file mode 100644 index 0000000..2b13e61 --- /dev/null +++ b/Documentation/devicetree/bindings/gpu/brcm,bcm-vc4.txt @@ -0,0 +1,83 @@ +Broadcom VC4 GPU + +The VC4 device present on the Raspberry Pi includes a display system +with HDMI output and the HVS scaler for compositing display planes. + +Required properties for VC4: +- compatible: Should be "brcm,vc4" +- crtcs: List of references to pixelvalve scanout engines +- hvss: List of references to HVS video scalers +- encoders: List of references to output encoders (HDMI, SDTV) + +Required properties for Pixel Valve: +- compatible: Should be "brcm,vc4-pixelvalve" +- reg: Physical base address and length of the PV's registers +- interrupts: The interrupt number + See bindings/interrupt-controller/brcm,bcm2835-armctrl-ic.txt + +Required properties for HVS: +- compatible: Should be "brcm,vc4-hvs" +- reg: Physical base address and length of the HVS's registers +- interrupts: The interrupt number + See bindings/interrupt-controller/brcm,bcm2835-armctrl-ic.txt + +Required properties for HDMI +- compatible: Should be "brcm,vc4-hdmi" +- reg: Physical base address and length of the two register ranges + ("HDMI" and "HD") +- interrupts: The interrupt numbers + See bindings/interrupt-controller/brcm,bcm2835-armctrl-ic.txt +- ddc: phandle of the I2C controller used for DDC EDID probing +- crtc: phandle to the pixelvalve CRTC the HDMI encoder is attached to + +Optional properties for HDMI: +- hpd-gpio: The GPIO pin for HDMI hotplug detect (if it doesn't appear + as an interrupt/status bit in the HDMI controller + itself). See bindings/pinctrl/brcm,bcm2835-gpio.txt + +Example: +/ { + soc { + pv0: brcm,vc4-pixelvalve@7e206000 { + compatible = "brcm,vc4-pixelvalve"; + reg = <0x7e206000 0x100>; + interrupts = <2 13>; /* pwa2 */ + }; + + pv1: brcm,vc4-pixelvalve@7e207000 { + compatible = "brcm,vc4-pixelvalve"; + reg = <0x7e207000 0x100>; + interrupts = <2 14>; /* pwa1 */ + }; + + pv2: brcm,vc4-pixelvalve@7e807000 { + compatible = "brcm,vc4-pixelvalve"; + reg = <0x7e807000 0x100>; + interrupts = <2 10>; /* pixelvalve */ + }; + + hvs: brcm,hvs@7e400000 { + compatible = "brcm,vc4-hvs"; + reg = <0x7e400000 0x6000>; + interrupts = <2 1>; + }; + + hdmi: brcm,vc4-hdmi@7e902000 { + compatible = "brcm,vc4-hdmi"; + reg = <0x7e902000 0x600>, + <0x7e808000 0x100>; + interrupts = <2 8>, <2 9>; + ddc = <&i2c2>; + hpd-gpio = <&gpio 46 GPIO_ACTIVE_HIGH>; + crtc = <&pv2>; + }; + + vc4: vc4@0x7e4c0000 { + compatible = "brcm,vc4"; + + crtcs = <&pv0>, <&pv1>, <&pv2>; + encoders = <&hdmi>; + hvss = <&hvs>; + }; + }; +};
Signed-off-by: Eric Anholt <eric@anholt.net> --- .../devicetree/bindings/gpu/brcm,bcm-vc4.txt | 83 ++++++++++++++++++++++ 1 file changed, 83 insertions(+) create mode 100644 Documentation/devicetree/bindings/gpu/brcm,bcm-vc4.txt