| Message ID | 20200422091512.950-2-tomi.valkeinen@ti.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 76921f15acc0758e7e6a0f84bf9c082b8240184b |
| Headers | show |
| Series | [1/3] arm64: dts: ti: am654: Add DSS node | expand |
On 22/04/2020 12:15, Tomi Valkeinen wrote: > Add DSS node for J721E SoC. Subject should drop .dtsi, I can fix that locally though. Got a question below. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> > --- > arch/arm64/boot/dts/ti/k3-j721e-main.dtsi | 57 +++++++++++++++++++++++ > 1 file changed, 57 insertions(+) > > diff --git a/arch/arm64/boot/dts/ti/k3-j721e-main.dtsi b/arch/arm64/boot/dts/ti/k3-j721e-main.dtsi > index 0b9d14b838a1..21c362042ecf 100644 > --- a/arch/arm64/boot/dts/ti/k3-j721e-main.dtsi > +++ b/arch/arm64/boot/dts/ti/k3-j721e-main.dtsi > @@ -736,6 +736,63 @@ > }; > }; > > + dss: dss@04a00000 { > + compatible = "ti,j721e-dss"; > + reg = > + <0x00 0x04a00000 0x00 0x10000>, /* common_m */ > + <0x00 0x04a10000 0x00 0x10000>, /* common_s0*/ > + <0x00 0x04b00000 0x00 0x10000>, /* common_s1*/ > + <0x00 0x04b10000 0x00 0x10000>, /* common_s2*/ > + > + <0x00 0x04a20000 0x00 0x10000>, /* vidl1 */ > + <0x00 0x04a30000 0x00 0x10000>, /* vidl2 */ > + <0x00 0x04a50000 0x00 0x10000>, /* vid1 */ > + <0x00 0x04a60000 0x00 0x10000>, /* vid2 */ > + > + <0x00 0x04a70000 0x00 0x10000>, /* ovr1 */ > + <0x00 0x04a90000 0x00 0x10000>, /* ovr2 */ > + <0x00 0x04ab0000 0x00 0x10000>, /* ovr3 */ > + <0x00 0x04ad0000 0x00 0x10000>, /* ovr4 */ > + > + <0x00 0x04a80000 0x00 0x10000>, /* vp1 */ > + <0x00 0x04aa0000 0x00 0x10000>, /* vp2 */ > + <0x00 0x04ac0000 0x00 0x10000>, /* vp3 */ > + <0x00 0x04ae0000 0x00 0x10000>, /* vp4 */ > + <0x00 0x04af0000 0x00 0x10000>; /* wb */ > + > + reg-names = "common_m", "common_s0", > + "common_s1", "common_s2", > + "vidl1", "vidl2","vid1","vid2", > + "ovr1", "ovr2", "ovr3", "ovr4", > + "vp1", "vp2", "vp3", "vp4", > + "wb"; > + > + clocks = <&k3_clks 152 0>, > + <&k3_clks 152 1>, > + <&k3_clks 152 4>, > + <&k3_clks 152 9>, > + <&k3_clks 152 13>; > + clock-names = "fck", "vp1", "vp2", "vp3", "vp4"; > + > + power-domains = <&k3_pds 152 TI_SCI_PD_EXCLUSIVE>; > + > + interrupts = <GIC_SPI 602 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 603 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 604 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 605 IRQ_TYPE_LEVEL_HIGH>; > + interrupt-names = "common_m", > + "common_s0", > + "common_s1", > + "common_s2"; > + > + status = "disabled"; Again, why disabled by default? -Tero > + > + dss_ports: ports { > + #address-cells = <1>; > + #size-cells = <0>; > + }; > + }; > + > mcasp0: mcasp@2b00000 { > compatible = "ti,am33xx-mcasp-audio"; > reg = <0x0 0x02b00000 0x0 0x2000>, > -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On 27/04/2020 13:09, Tero Kristo wrote: >> + status = "disabled"; > > Again, why disabled by default? > tidss device is not functional without a defined video-port. The driver is not implemented in a way that it would handle a broken configuration gracefully. Best regards, Jyri
On 27/04/2020 13:37, Jyri Sarha wrote: > On 27/04/2020 13:09, Tero Kristo wrote: >>> + status = "disabled"; >> >> Again, why disabled by default? >> > > tidss device is not functional without a defined video-port. The driver > is not implemented in a way that it would handle a broken configuration > gracefully. What/where/when is the video-port going to be defined then? Is this going to be done in an overlay? -Tero -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On 27/04/2020 13:41, Tero Kristo wrote: > On 27/04/2020 13:37, Jyri Sarha wrote: >> On 27/04/2020 13:09, Tero Kristo wrote: >>>> + status = "disabled"; >>> >>> Again, why disabled by default? >>> >> >> tidss device is not functional without a defined video-port. The driver >> is not implemented in a way that it would handle a broken configuration >> gracefully. > > What/where/when is the video-port going to be defined then? Is this > going to be done in an overlay? > Yes. It should be defined in the board specific dts or dtso file, where the video-connector or -panel is. BR, Jyri
On 27/04/2020 13:37, Jyri Sarha wrote: > On 27/04/2020 13:09, Tero Kristo wrote: >>> + status = "disabled"; >> >> Again, why disabled by default? >> > > tidss device is not functional without a defined video-port. The driver > is not implemented in a way that it would handle a broken configuration > gracefully. Then we need to fix it. The driver should handle the case where there are no ports defined just fine. Tomi
On 27/04/2020 13:51, Tomi Valkeinen wrote: > On 27/04/2020 13:37, Jyri Sarha wrote: >> On 27/04/2020 13:09, Tero Kristo wrote: >>>> + status = "disabled"; >>> >>> Again, why disabled by default? >>> >> >> tidss device is not functional without a defined video-port. The driver >> is not implemented in a way that it would handle a broken configuration >> gracefully. > > Then we need to fix it. The driver should handle the case where there > are no ports defined just fine. > Just by reading the code, I would say that currently the probe would fail with returned -ENOMEM after calling drm_vblank_init() with zero CRTCs. So should the probe fail gracefully and silently, or should we try to register a DRM device with no CRTCs? Is that even possible? BR, Jyri
On 27/04/2020 14:10, Jyri Sarha wrote: > On 27/04/2020 13:51, Tomi Valkeinen wrote: >> On 27/04/2020 13:37, Jyri Sarha wrote: >>> On 27/04/2020 13:09, Tero Kristo wrote: >>>>> + status = "disabled"; >>>> >>>> Again, why disabled by default? >>>> >>> >>> tidss device is not functional without a defined video-port. The driver >>> is not implemented in a way that it would handle a broken configuration >>> gracefully. >> >> Then we need to fix it. The driver should handle the case where there >> are no ports defined just fine. >> > > Just by reading the code, I would say that currently the probe would > fail with returned -ENOMEM after calling drm_vblank_init() with zero CRTCs. > > So should the probe fail gracefully and silently, or should we try to > register a DRM device with no CRTCs? Is that even possible? My first thought is that the driver should exit probe silently with ENODEV if there are no outputs defined (but, of course, with EPROBE_DEFER if there are outputs which haven't been probed yet). It gets a bit more complex if we ever support writeback, as that can be used as mem-to-mem without any displays, but I think we can ignore that for now. Tomi
On 27/04/2020 14:15, Tomi Valkeinen wrote: > On 27/04/2020 14:10, Jyri Sarha wrote: >> On 27/04/2020 13:51, Tomi Valkeinen wrote: >>> On 27/04/2020 13:37, Jyri Sarha wrote: >>>> On 27/04/2020 13:09, Tero Kristo wrote: >>>>>> + status = "disabled"; >>>>> >>>>> Again, why disabled by default? >>>>> >>>> >>>> tidss device is not functional without a defined video-port. The driver >>>> is not implemented in a way that it would handle a broken configuration >>>> gracefully. >>> >>> Then we need to fix it. The driver should handle the case where there >>> are no ports defined just fine. >>> >> >> Just by reading the code, I would say that currently the probe would >> fail with returned -ENOMEM after calling drm_vblank_init() with zero CRTCs. >> >> So should the probe fail gracefully and silently, or should we try to >> register a DRM device with no CRTCs? Is that even possible? > > My first thought is that the driver should exit probe silently with ENODEV if there are no outputs > defined (but, of course, with EPROBE_DEFER if there are outputs which haven't been probed yet). > > It gets a bit more complex if we ever support writeback, as that can be used as mem-to-mem without > any displays, but I think we can ignore that for now. In any case, that's not the reason for status = "disabled", so that discussion is not related to these patches as such. The reason to have DSS disabled is just to prevent pointless driver probing. When a board dts or a DT overlay adds a display, the DSS DT node has to be modified anyway to add the DT graph and the panel/bridge data. So one can as well add the single line of "status = enabled" there. Tomi
On 27/04/2020 14:37, Tomi Valkeinen wrote: > On 27/04/2020 14:15, Tomi Valkeinen wrote: >> On 27/04/2020 14:10, Jyri Sarha wrote: >>> On 27/04/2020 13:51, Tomi Valkeinen wrote: >>>> On 27/04/2020 13:37, Jyri Sarha wrote: >>>>> On 27/04/2020 13:09, Tero Kristo wrote: >>>>>>> + status = "disabled"; >>>>>> >>>>>> Again, why disabled by default? >>>>>> >>>>> >>>>> tidss device is not functional without a defined video-port. The >>>>> driver >>>>> is not implemented in a way that it would handle a broken >>>>> configuration >>>>> gracefully. >>>> >>>> Then we need to fix it. The driver should handle the case where there >>>> are no ports defined just fine. >>>> >>> >>> Just by reading the code, I would say that currently the probe would >>> fail with returned -ENOMEM after calling drm_vblank_init() with zero >>> CRTCs. >>> >>> So should the probe fail gracefully and silently, or should we try to >>> register a DRM device with no CRTCs? Is that even possible? >> >> My first thought is that the driver should exit probe silently with >> ENODEV if there are no outputs defined (but, of course, with >> EPROBE_DEFER if there are outputs which haven't been probed yet). >> >> It gets a bit more complex if we ever support writeback, as that can >> be used as mem-to-mem without any displays, but I think we can ignore >> that for now. > > In any case, that's not the reason for status = "disabled", so that > discussion is not related to these patches as such. > > The reason to have DSS disabled is just to prevent pointless driver > probing. When a board dts or a DT overlay adds a display, the DSS DT > node has to be modified anyway to add the DT graph and the panel/bridge > data. So one can as well add the single line of "status = enabled" there. Ok, thanks for the explanation, queued all three patches towards 5.8 based on that. -Tero -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
diff --git a/arch/arm64/boot/dts/ti/k3-j721e-main.dtsi b/arch/arm64/boot/dts/ti/k3-j721e-main.dtsi index 0b9d14b838a1..21c362042ecf 100644 --- a/arch/arm64/boot/dts/ti/k3-j721e-main.dtsi +++ b/arch/arm64/boot/dts/ti/k3-j721e-main.dtsi @@ -736,6 +736,63 @@ }; }; + dss: dss@04a00000 { + compatible = "ti,j721e-dss"; + reg = + <0x00 0x04a00000 0x00 0x10000>, /* common_m */ + <0x00 0x04a10000 0x00 0x10000>, /* common_s0*/ + <0x00 0x04b00000 0x00 0x10000>, /* common_s1*/ + <0x00 0x04b10000 0x00 0x10000>, /* common_s2*/ + + <0x00 0x04a20000 0x00 0x10000>, /* vidl1 */ + <0x00 0x04a30000 0x00 0x10000>, /* vidl2 */ + <0x00 0x04a50000 0x00 0x10000>, /* vid1 */ + <0x00 0x04a60000 0x00 0x10000>, /* vid2 */ + + <0x00 0x04a70000 0x00 0x10000>, /* ovr1 */ + <0x00 0x04a90000 0x00 0x10000>, /* ovr2 */ + <0x00 0x04ab0000 0x00 0x10000>, /* ovr3 */ + <0x00 0x04ad0000 0x00 0x10000>, /* ovr4 */ + + <0x00 0x04a80000 0x00 0x10000>, /* vp1 */ + <0x00 0x04aa0000 0x00 0x10000>, /* vp2 */ + <0x00 0x04ac0000 0x00 0x10000>, /* vp3 */ + <0x00 0x04ae0000 0x00 0x10000>, /* vp4 */ + <0x00 0x04af0000 0x00 0x10000>; /* wb */ + + reg-names = "common_m", "common_s0", + "common_s1", "common_s2", + "vidl1", "vidl2","vid1","vid2", + "ovr1", "ovr2", "ovr3", "ovr4", + "vp1", "vp2", "vp3", "vp4", + "wb"; + + clocks = <&k3_clks 152 0>, + <&k3_clks 152 1>, + <&k3_clks 152 4>, + <&k3_clks 152 9>, + <&k3_clks 152 13>; + clock-names = "fck", "vp1", "vp2", "vp3", "vp4"; + + power-domains = <&k3_pds 152 TI_SCI_PD_EXCLUSIVE>; + + interrupts = <GIC_SPI 602 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 603 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 604 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 605 IRQ_TYPE_LEVEL_HIGH>; + interrupt-names = "common_m", + "common_s0", + "common_s1", + "common_s2"; + + status = "disabled"; + + dss_ports: ports { + #address-cells = <1>; + #size-cells = <0>; + }; + }; + mcasp0: mcasp@2b00000 { compatible = "ti,am33xx-mcasp-audio"; reg = <0x0 0x02b00000 0x0 0x2000>,
Add DSS node for J721E SoC. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> --- arch/arm64/boot/dts/ti/k3-j721e-main.dtsi | 57 +++++++++++++++++++++++ 1 file changed, 57 insertions(+)