| Message ID | 1604006877-20092-2-git-send-email-anitha.chrisanthus@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for KeemBay DRM driver | expand |
Hi Anitha. On Thu, Oct 29, 2020 at 02:27:52PM -0700, Anitha Chrisanthus wrote: > This patch adds bindings for Intel KeemBay Display > > v2: review changes from Rob Herring > v3: review changes from Sam Ravnborg (removed mipi dsi entries, and > encoder entry, connect port to dsi) > MSSCAM is part of the display submodule and its used to reset LCD > and MIPI DSI clocks, so its best to be on this device tree. > > Signed-off-by: Anitha Chrisanthus <anitha.chrisanthus@intel.com> > Cc: Sam Ravnborg <sam@ravnborg.org> > Cc: Thomas Zimmermann <tzimmermann@suse.de> > Cc: Daniel Vetter <daniel@ffwll.ch> Looks good - and the split betwwen the display and the mipi<->dsi parts matches the understanding of the HW I have developed. Reviewed-by: Sam Ravnborg <sam@ravnborg.org> > --- > .../bindings/display/intel,keembay-display.yaml | 75 ++++++++++++++++++++++ > 1 file changed, 75 insertions(+) > create mode 100644 Documentation/devicetree/bindings/display/intel,keembay-display.yaml > > diff --git a/Documentation/devicetree/bindings/display/intel,keembay-display.yaml b/Documentation/devicetree/bindings/display/intel,keembay-display.yaml > new file mode 100644 > index 0000000..8a8effe > --- /dev/null > +++ b/Documentation/devicetree/bindings/display/intel,keembay-display.yaml > @@ -0,0 +1,75 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/display/intel,keembay-display.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Devicetree bindings for Intel Keem Bay display controller > + > +maintainers: > + - Anitha Chrisanthus <anitha.chrisanthus@intel.com> > + - Edmond J Dea <edmund.j.dea@intel.com> > + > +properties: > + compatible: > + const: intel,keembay-display > + > + reg: > + items: > + - description: LCD registers range > + - description: Msscam registers range > + > + reg-names: > + items: > + - const: lcd > + - const: msscam > + > + clocks: > + items: > + - description: LCD controller clock > + - description: pll0 clock > + > + clock-names: > + items: > + - const: clk_lcd > + - const: clk_pll0 > + > + interrupts: > + maxItems: 1 > + > + port: > + type: object > + description: Display output node to DSI. > + > +required: > + - compatible > + - reg > + - reg-names > + - clocks > + - clock-names > + - interrupts > + - port > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/interrupt-controller/irq.h> > + #include <dt-bindings/interrupt-controller/arm-gic.h> > + > + display@20930000 { > + compatible = "intel,keembay-display"; > + reg = <0x20930000 0x3000>, > + <0x20910000 0x30>; > + reg-names = "lcd", "msscam"; > + interrupts = <GIC_SPI 33 IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&scmi_clk 0x83>, > + <&scmi_clk 0x0>; > + clock-names = "clk_lcd", "clk_pll0"; > + > + port { > + disp_out: endpoint { > + remote-endpoint = <&dsi_in>; > + }; > + }; > + }; > -- > 2.7.4
Hi, On 29/10/2020 23:20, Sam Ravnborg wrote: > Hi Anitha. > > On Thu, Oct 29, 2020 at 02:27:52PM -0700, Anitha Chrisanthus wrote: >> This patch adds bindings for Intel KeemBay Display >> >> v2: review changes from Rob Herring >> v3: review changes from Sam Ravnborg (removed mipi dsi entries, and >> encoder entry, connect port to dsi) >> MSSCAM is part of the display submodule and its used to reset LCD >> and MIPI DSI clocks, so its best to be on this device tree. >> >> Signed-off-by: Anitha Chrisanthus <anitha.chrisanthus@intel.com> >> Cc: Sam Ravnborg <sam@ravnborg.org> >> Cc: Thomas Zimmermann <tzimmermann@suse.de> >> Cc: Daniel Vetter <daniel@ffwll.ch> > > Looks good - and the split betwwen the display and the mipi<->dsi parts > matches the understanding of the HW I have developed. > > Reviewed-by: Sam Ravnborg <sam@ravnborg.org> > >> --- >> .../bindings/display/intel,keembay-display.yaml | 75 ++++++++++++++++++++++ >> 1 file changed, 75 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/display/intel,keembay-display.yaml >> >> diff --git a/Documentation/devicetree/bindings/display/intel,keembay-display.yaml b/Documentation/devicetree/bindings/display/intel,keembay-display.yaml >> new file mode 100644 >> index 0000000..8a8effe >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/display/intel,keembay-display.yaml >> @@ -0,0 +1,75 @@ >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/display/intel,keembay-display.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Devicetree bindings for Intel Keem Bay display controller >> + >> +maintainers: >> + - Anitha Chrisanthus <anitha.chrisanthus@intel.com> >> + - Edmond J Dea <edmund.j.dea@intel.com> >> + >> +properties: >> + compatible: >> + const: intel,keembay-display >> + >> + reg: >> + items: >> + - description: LCD registers range >> + - description: Msscam registers range >> + Indeed the split is much better, but as you replied on http://lore.kernel.org/r/BY5PR11MB41827DE07436DD0454E24E6E8C0A0@BY5PR11MB4182.namprd11.prod.outlook.com the msscam seems to be shared with the camera subsystem block, if this is the case it should be handled. If it's a shared register block, it could be defined as a "syscon" used by both subsystems. Neil >> + reg-names: >> + items: >> + - const: lcd >> + - const: msscam >> + >> + clocks: >> + items: >> + - description: LCD controller clock >> + - description: pll0 clock >> + >> + clock-names: >> + items: >> + - const: clk_lcd >> + - const: clk_pll0 >> + >> + interrupts: >> + maxItems: 1 >> + >> + port: >> + type: object >> + description: Display output node to DSI. >> + >> +required: >> + - compatible >> + - reg >> + - reg-names >> + - clocks >> + - clock-names >> + - interrupts >> + - port >> + >> +additionalProperties: false >> + >> +examples: >> + - | >> + #include <dt-bindings/interrupt-controller/irq.h> >> + #include <dt-bindings/interrupt-controller/arm-gic.h> >> + >> + display@20930000 { >> + compatible = "intel,keembay-display"; >> + reg = <0x20930000 0x3000>, >> + <0x20910000 0x30>; >> + reg-names = "lcd", "msscam"; >> + interrupts = <GIC_SPI 33 IRQ_TYPE_LEVEL_HIGH>; >> + clocks = <&scmi_clk 0x83>, >> + <&scmi_clk 0x0>; >> + clock-names = "clk_lcd", "clk_pll0"; >> + >> + port { >> + disp_out: endpoint { >> + remote-endpoint = <&dsi_in>; >> + }; >> + }; >> + }; >> -- >> 2.7.4 > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel >
Hi Neil. On Fri, Oct 30, 2020 at 09:31:36AM +0100, Neil Armstrong wrote: > Hi, > > On 29/10/2020 23:20, Sam Ravnborg wrote: > > Hi Anitha. > > > > On Thu, Oct 29, 2020 at 02:27:52PM -0700, Anitha Chrisanthus wrote: > >> This patch adds bindings for Intel KeemBay Display > >> > >> v2: review changes from Rob Herring > >> v3: review changes from Sam Ravnborg (removed mipi dsi entries, and > >> encoder entry, connect port to dsi) > >> MSSCAM is part of the display submodule and its used to reset LCD > >> and MIPI DSI clocks, so its best to be on this device tree. > >> > >> Signed-off-by: Anitha Chrisanthus <anitha.chrisanthus@intel.com> > >> Cc: Sam Ravnborg <sam@ravnborg.org> > >> Cc: Thomas Zimmermann <tzimmermann@suse.de> > >> Cc: Daniel Vetter <daniel@ffwll.ch> > > > > Looks good - and the split betwwen the display and the mipi<->dsi parts > > matches the understanding of the HW I have developed. > > > > Reviewed-by: Sam Ravnborg <sam@ravnborg.org> > > > >> --- > >> .../bindings/display/intel,keembay-display.yaml | 75 ++++++++++++++++++++++ > >> 1 file changed, 75 insertions(+) > >> create mode 100644 Documentation/devicetree/bindings/display/intel,keembay-display.yaml > >> > >> diff --git a/Documentation/devicetree/bindings/display/intel,keembay-display.yaml b/Documentation/devicetree/bindings/display/intel,keembay-display.yaml > >> new file mode 100644 > >> index 0000000..8a8effe > >> --- /dev/null > >> +++ b/Documentation/devicetree/bindings/display/intel,keembay-display.yaml > >> @@ -0,0 +1,75 @@ > >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > >> +%YAML 1.2 > >> +--- > >> +$id: http://devicetree.org/schemas/display/intel,keembay-display.yaml# > >> +$schema: http://devicetree.org/meta-schemas/core.yaml# > >> + > >> +title: Devicetree bindings for Intel Keem Bay display controller > >> + > >> +maintainers: > >> + - Anitha Chrisanthus <anitha.chrisanthus@intel.com> > >> + - Edmond J Dea <edmund.j.dea@intel.com> > >> + > >> +properties: > >> + compatible: > >> + const: intel,keembay-display > >> + > >> + reg: > >> + items: > >> + - description: LCD registers range > >> + - description: Msscam registers range > >> + > > Indeed the split is much better, but as you replied on http://lore.kernel.org/r/BY5PR11MB41827DE07436DD0454E24E6E8C0A0@BY5PR11MB4182.namprd11.prod.outlook.com > the msscam seems to be shared with the camera subsystem block, if this is the case it should be handled. > > If it's a shared register block, it could be defined as a "syscon" used by both subsystems. I think I got it now. msscam is used to enable clocks both for the display driver and the mipi-dsi part. So it should be pulled in to a dedicated node - for example like this: mssscam: msscam@20910000 { compatible = "intel,keembay-msscam", "syscon"; reg = <0x20910000, 0x30>; reg-io-width = <4>; }; And ofc we need a binding file for that. And then we could have code like this in the display driver: regmap *msscam = syscon_regmap_lookup_by_compatible("intel,keembay-msscam"); if (IS_ERR(msscam)) tell-and goto-out; regmap_write(msscam, MSS_LCD_MIPI_CFG, 1); regmap_write(msscam, MSS_LOOPBACK_CFG, 0); And then in the kmb_dsi part: regmap *msscam = syscon_regmap_lookup_by_compatible("intel,keembay-msscam"); if (IS_ERR(msscam)) tell-and goto-out; regmap_write(msscam, MSS_MIPI_CIF_CFG, 1); Did I finally get it? Sam
On Fri, Oct 30, 2020 at 4:15 PM Sam Ravnborg <sam@ravnborg.org> wrote: > > Hi Neil. > > On Fri, Oct 30, 2020 at 09:31:36AM +0100, Neil Armstrong wrote: > > Hi, > > > > On 29/10/2020 23:20, Sam Ravnborg wrote: > > > Hi Anitha. > > > > > > On Thu, Oct 29, 2020 at 02:27:52PM -0700, Anitha Chrisanthus wrote: > > >> This patch adds bindings for Intel KeemBay Display > > >> > > >> v2: review changes from Rob Herring > > >> v3: review changes from Sam Ravnborg (removed mipi dsi entries, and > > >> encoder entry, connect port to dsi) > > >> MSSCAM is part of the display submodule and its used to reset LCD > > >> and MIPI DSI clocks, so its best to be on this device tree. > > >> > > >> Signed-off-by: Anitha Chrisanthus <anitha.chrisanthus@intel.com> > > >> Cc: Sam Ravnborg <sam@ravnborg.org> > > >> Cc: Thomas Zimmermann <tzimmermann@suse.de> > > >> Cc: Daniel Vetter <daniel@ffwll.ch> > > > > > > Looks good - and the split betwwen the display and the mipi<->dsi parts > > > matches the understanding of the HW I have developed. > > > > > > Reviewed-by: Sam Ravnborg <sam@ravnborg.org> > > > > > >> --- > > >> .../bindings/display/intel,keembay-display.yaml | 75 ++++++++++++++++++++++ > > >> 1 file changed, 75 insertions(+) > > >> create mode 100644 Documentation/devicetree/bindings/display/intel,keembay-display.yaml > > >> > > >> diff --git a/Documentation/devicetree/bindings/display/intel,keembay-display.yaml b/Documentation/devicetree/bindings/display/intel,keembay-display.yaml > > >> new file mode 100644 > > >> index 0000000..8a8effe > > >> --- /dev/null > > >> +++ b/Documentation/devicetree/bindings/display/intel,keembay-display.yaml > > >> @@ -0,0 +1,75 @@ > > >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > >> +%YAML 1.2 > > >> +--- > > >> +$id: http://devicetree.org/schemas/display/intel,keembay-display.yaml# > > >> +$schema: http://devicetree.org/meta-schemas/core.yaml# > > >> + > > >> +title: Devicetree bindings for Intel Keem Bay display controller > > >> + > > >> +maintainers: > > >> + - Anitha Chrisanthus <anitha.chrisanthus@intel.com> > > >> + - Edmond J Dea <edmund.j.dea@intel.com> > > >> + > > >> +properties: > > >> + compatible: > > >> + const: intel,keembay-display > > >> + > > >> + reg: > > >> + items: > > >> + - description: LCD registers range > > >> + - description: Msscam registers range > > >> + > > > > Indeed the split is much better, but as you replied on http://lore.kernel.org/r/BY5PR11MB41827DE07436DD0454E24E6E8C0A0@BY5PR11MB4182.namprd11.prod.outlook.com > > the msscam seems to be shared with the camera subsystem block, if this is the case it should be handled. > > > > If it's a shared register block, it could be defined as a "syscon" used by both subsystems. > > I think I got it now. > > msscam is used to enable clocks both for the display driver and the > mipi-dsi part. If just clocks, then the msscam should be a clock provider possibly. If not, then the below looks right. > > So it should be pulled in to a dedicated node - for example like this: > > mssscam: msscam@20910000 { > compatible = "intel,keembay-msscam", "syscon"; > reg = <0x20910000, 0x30>; > reg-io-width = <4>; > }; > > And ofc we need a binding file for that. > > > And then we could have code like this in the display driver: > regmap *msscam = syscon_regmap_lookup_by_compatible("intel,keembay-msscam"); > if (IS_ERR(msscam)) > tell-and goto-out; > > regmap_write(msscam, MSS_LCD_MIPI_CFG, 1); > regmap_write(msscam, MSS_LOOPBACK_CFG, 0); > > And then in the kmb_dsi part: > regmap *msscam = syscon_regmap_lookup_by_compatible("intel,keembay-msscam"); > if (IS_ERR(msscam)) > tell-and goto-out; > > regmap_write(msscam, MSS_MIPI_CIF_CFG, 1); > > > Did I finally get it? > > Sam > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On 02/11/2020 16:16, Rob Herring wrote: > On Fri, Oct 30, 2020 at 4:15 PM Sam Ravnborg <sam@ravnborg.org> wrote: >> >> Hi Neil. >> >> On Fri, Oct 30, 2020 at 09:31:36AM +0100, Neil Armstrong wrote: >>> Hi, >>> >>> On 29/10/2020 23:20, Sam Ravnborg wrote: >>>> Hi Anitha. >>>> >>>> On Thu, Oct 29, 2020 at 02:27:52PM -0700, Anitha Chrisanthus wrote: >>>>> This patch adds bindings for Intel KeemBay Display >>>>> >>>>> v2: review changes from Rob Herring >>>>> v3: review changes from Sam Ravnborg (removed mipi dsi entries, and >>>>> encoder entry, connect port to dsi) >>>>> MSSCAM is part of the display submodule and its used to reset LCD >>>>> and MIPI DSI clocks, so its best to be on this device tree. >>>>> >>>>> Signed-off-by: Anitha Chrisanthus <anitha.chrisanthus@intel.com> >>>>> Cc: Sam Ravnborg <sam@ravnborg.org> >>>>> Cc: Thomas Zimmermann <tzimmermann@suse.de> >>>>> Cc: Daniel Vetter <daniel@ffwll.ch> >>>> >>>> Looks good - and the split betwwen the display and the mipi<->dsi parts >>>> matches the understanding of the HW I have developed. >>>> >>>> Reviewed-by: Sam Ravnborg <sam@ravnborg.org> >>>> >>>>> --- >>>>> .../bindings/display/intel,keembay-display.yaml | 75 ++++++++++++++++++++++ >>>>> 1 file changed, 75 insertions(+) >>>>> create mode 100644 Documentation/devicetree/bindings/display/intel,keembay-display.yaml >>>>> >>>>> diff --git a/Documentation/devicetree/bindings/display/intel,keembay-display.yaml b/Documentation/devicetree/bindings/display/intel,keembay-display.yaml >>>>> new file mode 100644 >>>>> index 0000000..8a8effe >>>>> --- /dev/null >>>>> +++ b/Documentation/devicetree/bindings/display/intel,keembay-display.yaml >>>>> @@ -0,0 +1,75 @@ >>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >>>>> +%YAML 1.2 >>>>> +--- >>>>> +$id: http://devicetree.org/schemas/display/intel,keembay-display.yaml# >>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>>>> + >>>>> +title: Devicetree bindings for Intel Keem Bay display controller >>>>> + >>>>> +maintainers: >>>>> + - Anitha Chrisanthus <anitha.chrisanthus@intel.com> >>>>> + - Edmond J Dea <edmund.j.dea@intel.com> >>>>> + >>>>> +properties: >>>>> + compatible: >>>>> + const: intel,keembay-display >>>>> + >>>>> + reg: >>>>> + items: >>>>> + - description: LCD registers range >>>>> + - description: Msscam registers range >>>>> + >>> >>> Indeed the split is much better, but as you replied on http://lore.kernel.org/r/BY5PR11MB41827DE07436DD0454E24E6E8C0A0@BY5PR11MB4182.namprd11.prod.outlook.com >>> the msscam seems to be shared with the camera subsystem block, if this is the case it should be handled. >>> >>> If it's a shared register block, it could be defined as a "syscon" used by both subsystems. >> >> I think I got it now. >> >> msscam is used to enable clocks both for the display driver and the >> mipi-dsi part. > > If just clocks, then the msscam should be a clock provider possibly. > If not, then the below looks right. I agree > >> >> So it should be pulled in to a dedicated node - for example like this: >> >> mssscam: msscam@20910000 { >> compatible = "intel,keembay-msscam", "syscon"; >> reg = <0x20910000, 0x30>; >> reg-io-width = <4>; >> }; >> >> And ofc we need a binding file for that. >> >> >> And then we could have code like this in the display driver: >> regmap *msscam = syscon_regmap_lookup_by_compatible("intel,keembay-msscam"); >> if (IS_ERR(msscam)) >> tell-and goto-out; It's better to use a phandle in the display node, instead of looking for compatible nodes. >> >> regmap_write(msscam, MSS_LCD_MIPI_CFG, 1); >> regmap_write(msscam, MSS_LOOPBACK_CFG, 0); >> >> And then in the kmb_dsi part: >> regmap *msscam = syscon_regmap_lookup_by_compatible("intel,keembay-msscam"); >> if (IS_ERR(msscam)) >> tell-and goto-out; >> >> regmap_write(msscam, MSS_MIPI_CIF_CFG, 1); >> >> >> Did I finally get it? Yes, this is what I was thinking about, thanks Neil >> >> Sam >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Neil. > >>>>> --- > >>>>> .../bindings/display/intel,keembay-display.yaml | 75 ++++++++++++++++++++++ > >>>>> 1 file changed, 75 insertions(+) > >>>>> create mode 100644 Documentation/devicetree/bindings/display/intel,keembay-display.yaml > >>>>> > >>>>> diff --git a/Documentation/devicetree/bindings/display/intel,keembay-display.yaml b/Documentation/devicetree/bindings/display/intel,keembay-display.yaml > >>>>> new file mode 100644 > >>>>> index 0000000..8a8effe > >>>>> --- /dev/null > >>>>> +++ b/Documentation/devicetree/bindings/display/intel,keembay-display.yaml > >>>>> @@ -0,0 +1,75 @@ > >>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > >>>>> +%YAML 1.2 > >>>>> +--- > >>>>> +$id: http://devicetree.org/schemas/display/intel,keembay-display.yaml# > >>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml# > >>>>> + > >>>>> +title: Devicetree bindings for Intel Keem Bay display controller > >>>>> + > >>>>> +maintainers: > >>>>> + - Anitha Chrisanthus <anitha.chrisanthus@intel.com> > >>>>> + - Edmond J Dea <edmund.j.dea@intel.com> > >>>>> + > >>>>> +properties: > >>>>> + compatible: > >>>>> + const: intel,keembay-display > >>>>> + > >>>>> + reg: > >>>>> + items: > >>>>> + - description: LCD registers range > >>>>> + - description: Msscam registers range > >>>>> + > >>> > >>> Indeed the split is much better, but as you replied on http://lore.kernel.org/r/BY5PR11MB41827DE07436DD0454E24E6E8C0A0@BY5PR11MB4182.namprd11.prod.outlook.com > >>> the msscam seems to be shared with the camera subsystem block, if this is the case it should be handled. > >>> > >>> If it's a shared register block, it could be defined as a "syscon" used by both subsystems. > >> > >> I think I got it now. > >> > >> msscam is used to enable clocks both for the display driver and the > >> mipi-dsi part. > > > > If just clocks, then the msscam should be a clock provider possibly. > > If not, then the below looks right. I am feeling a little clueless here - sorry. Can you help with any example that does this? Everything I looked up in bindings/clock/ had a "#clock-cells" which is not relevant for msscam - or so I think at least. Sam
On Mon, Nov 2, 2020 at 10:38 AM Neil Armstrong <narmstrong@baylibre.com> wrote: > > On 02/11/2020 16:16, Rob Herring wrote: > > On Fri, Oct 30, 2020 at 4:15 PM Sam Ravnborg <sam@ravnborg.org> wrote: > >> > >> Hi Neil. > >> > >> On Fri, Oct 30, 2020 at 09:31:36AM +0100, Neil Armstrong wrote: > >>> Hi, > >>> > >>> On 29/10/2020 23:20, Sam Ravnborg wrote: > >>>> Hi Anitha. > >>>> > >>>> On Thu, Oct 29, 2020 at 02:27:52PM -0700, Anitha Chrisanthus wrote: > >>>>> This patch adds bindings for Intel KeemBay Display > >>>>> > >>>>> v2: review changes from Rob Herring > >>>>> v3: review changes from Sam Ravnborg (removed mipi dsi entries, and > >>>>> encoder entry, connect port to dsi) > >>>>> MSSCAM is part of the display submodule and its used to reset LCD > >>>>> and MIPI DSI clocks, so its best to be on this device tree. > >>>>> > >>>>> Signed-off-by: Anitha Chrisanthus <anitha.chrisanthus@intel.com> > >>>>> Cc: Sam Ravnborg <sam@ravnborg.org> > >>>>> Cc: Thomas Zimmermann <tzimmermann@suse.de> > >>>>> Cc: Daniel Vetter <daniel@ffwll.ch> > >>>> > >>>> Looks good - and the split betwwen the display and the mipi<->dsi parts > >>>> matches the understanding of the HW I have developed. > >>>> > >>>> Reviewed-by: Sam Ravnborg <sam@ravnborg.org> > >>>> > >>>>> --- > >>>>> .../bindings/display/intel,keembay-display.yaml | 75 ++++++++++++++++++++++ > >>>>> 1 file changed, 75 insertions(+) > >>>>> create mode 100644 Documentation/devicetree/bindings/display/intel,keembay-display.yaml > >>>>> > >>>>> diff --git a/Documentation/devicetree/bindings/display/intel,keembay-display.yaml b/Documentation/devicetree/bindings/display/intel,keembay-display.yaml > >>>>> new file mode 100644 > >>>>> index 0000000..8a8effe > >>>>> --- /dev/null > >>>>> +++ b/Documentation/devicetree/bindings/display/intel,keembay-display.yaml > >>>>> @@ -0,0 +1,75 @@ > >>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > >>>>> +%YAML 1.2 > >>>>> +--- > >>>>> +$id: http://devicetree.org/schemas/display/intel,keembay-display.yaml# > >>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml# > >>>>> + > >>>>> +title: Devicetree bindings for Intel Keem Bay display controller > >>>>> + > >>>>> +maintainers: > >>>>> + - Anitha Chrisanthus <anitha.chrisanthus@intel.com> > >>>>> + - Edmond J Dea <edmund.j.dea@intel.com> > >>>>> + > >>>>> +properties: > >>>>> + compatible: > >>>>> + const: intel,keembay-display > >>>>> + > >>>>> + reg: > >>>>> + items: > >>>>> + - description: LCD registers range > >>>>> + - description: Msscam registers range > >>>>> + > >>> > >>> Indeed the split is much better, but as you replied on http://lore.kernel.org/r/BY5PR11MB41827DE07436DD0454E24E6E8C0A0@BY5PR11MB4182.namprd11.prod.outlook.com > >>> the msscam seems to be shared with the camera subsystem block, if this is the case it should be handled. > >>> > >>> If it's a shared register block, it could be defined as a "syscon" used by both subsystems. > >> > >> I think I got it now. > >> > >> msscam is used to enable clocks both for the display driver and the > >> mipi-dsi part. > > > > If just clocks, then the msscam should be a clock provider possibly. > > If not, then the below looks right. > > I agree > > > > >> > >> So it should be pulled in to a dedicated node - for example like this: > >> > >> mssscam: msscam@20910000 { > >> compatible = "intel,keembay-msscam", "syscon"; > >> reg = <0x20910000, 0x30>; > >> reg-io-width = <4>; > >> }; > >> > >> And ofc we need a binding file for that. > >> > >> > >> And then we could have code like this in the display driver: > >> regmap *msscam = syscon_regmap_lookup_by_compatible("intel,keembay-msscam"); > >> if (IS_ERR(msscam)) > >> tell-and goto-out; > > It's better to use a phandle in the display node, instead of looking for compatible nodes. No, you don't need it in DT unless there's more than one instance or other parameters needed like register offsets/masks. The above is actually faster too walking a list rather than a phandle look up (though the phandle cache now may make it faster). Rob
On Mon, Nov 2, 2020 at 12:04 PM Sam Ravnborg <sam@ravnborg.org> wrote: > > Hi Neil. > > > >>>>> --- > > >>>>> .../bindings/display/intel,keembay-display.yaml | 75 ++++++++++++++++++++++ > > >>>>> 1 file changed, 75 insertions(+) > > >>>>> create mode 100644 Documentation/devicetree/bindings/display/intel,keembay-display.yaml > > >>>>> > > >>>>> diff --git a/Documentation/devicetree/bindings/display/intel,keembay-display.yaml b/Documentation/devicetree/bindings/display/intel,keembay-display.yaml > > >>>>> new file mode 100644 > > >>>>> index 0000000..8a8effe > > >>>>> --- /dev/null > > >>>>> +++ b/Documentation/devicetree/bindings/display/intel,keembay-display.yaml > > >>>>> @@ -0,0 +1,75 @@ > > >>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > >>>>> +%YAML 1.2 > > >>>>> +--- > > >>>>> +$id: http://devicetree.org/schemas/display/intel,keembay-display.yaml# > > >>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml# > > >>>>> + > > >>>>> +title: Devicetree bindings for Intel Keem Bay display controller > > >>>>> + > > >>>>> +maintainers: > > >>>>> + - Anitha Chrisanthus <anitha.chrisanthus@intel.com> > > >>>>> + - Edmond J Dea <edmund.j.dea@intel.com> > > >>>>> + > > >>>>> +properties: > > >>>>> + compatible: > > >>>>> + const: intel,keembay-display > > >>>>> + > > >>>>> + reg: > > >>>>> + items: > > >>>>> + - description: LCD registers range > > >>>>> + - description: Msscam registers range > > >>>>> + > > >>> > > >>> Indeed the split is much better, but as you replied on http://lore.kernel.org/r/BY5PR11MB41827DE07436DD0454E24E6E8C0A0@BY5PR11MB4182.namprd11.prod.outlook.com > > >>> the msscam seems to be shared with the camera subsystem block, if this is the case it should be handled. > > >>> > > >>> If it's a shared register block, it could be defined as a "syscon" used by both subsystems. > > >> > > >> I think I got it now. > > >> > > >> msscam is used to enable clocks both for the display driver and the > > >> mipi-dsi part. > > > > > > If just clocks, then the msscam should be a clock provider possibly. > > > If not, then the below looks right. > > I am feeling a little clueless here - sorry. > > Can you help with any example that does this? I'm pretty sure there's some DSI PHYs where they are also a clock provider. > Everything I looked up in bindings/clock/ had a "#clock-cells" which is > not relevant for msscam - or so I think at least. That is precisely what needs to be added to msscam and then there would be another 'clocks' entry here. But it really depends if the register accesses here map to the controls the clock API provides. Rob
On 03/11/2020 03:02, Rob Herring wrote: > On Mon, Nov 2, 2020 at 10:38 AM Neil Armstrong <narmstrong@baylibre.com> wrote: >> >> On 02/11/2020 16:16, Rob Herring wrote: >>> On Fri, Oct 30, 2020 at 4:15 PM Sam Ravnborg <sam@ravnborg.org> wrote: >>>> >>>> Hi Neil. >>>> >>>> On Fri, Oct 30, 2020 at 09:31:36AM +0100, Neil Armstrong wrote: >>>>> Hi, >>>>> >>>>> On 29/10/2020 23:20, Sam Ravnborg wrote: >>>>>> Hi Anitha. >>>>>> >>>>>> On Thu, Oct 29, 2020 at 02:27:52PM -0700, Anitha Chrisanthus wrote: >>>>>>> This patch adds bindings for Intel KeemBay Display >>>>>>> >>>>>>> v2: review changes from Rob Herring >>>>>>> v3: review changes from Sam Ravnborg (removed mipi dsi entries, and >>>>>>> encoder entry, connect port to dsi) >>>>>>> MSSCAM is part of the display submodule and its used to reset LCD >>>>>>> and MIPI DSI clocks, so its best to be on this device tree. >>>>>>> >>>>>>> Signed-off-by: Anitha Chrisanthus <anitha.chrisanthus@intel.com> >>>>>>> Cc: Sam Ravnborg <sam@ravnborg.org> >>>>>>> Cc: Thomas Zimmermann <tzimmermann@suse.de> >>>>>>> Cc: Daniel Vetter <daniel@ffwll.ch> >>>>>> >>>>>> Looks good - and the split betwwen the display and the mipi<->dsi parts >>>>>> matches the understanding of the HW I have developed. >>>>>> >>>>>> Reviewed-by: Sam Ravnborg <sam@ravnborg.org> >>>>>> >>>>>>> --- >>>>>>> .../bindings/display/intel,keembay-display.yaml | 75 ++++++++++++++++++++++ >>>>>>> 1 file changed, 75 insertions(+) >>>>>>> create mode 100644 Documentation/devicetree/bindings/display/intel,keembay-display.yaml >>>>>>> >>>>>>> diff --git a/Documentation/devicetree/bindings/display/intel,keembay-display.yaml b/Documentation/devicetree/bindings/display/intel,keembay-display.yaml >>>>>>> new file mode 100644 >>>>>>> index 0000000..8a8effe >>>>>>> --- /dev/null >>>>>>> +++ b/Documentation/devicetree/bindings/display/intel,keembay-display.yaml >>>>>>> @@ -0,0 +1,75 @@ >>>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >>>>>>> +%YAML 1.2 >>>>>>> +--- >>>>>>> +$id: http://devicetree.org/schemas/display/intel,keembay-display.yaml# >>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>>>>>> + >>>>>>> +title: Devicetree bindings for Intel Keem Bay display controller >>>>>>> + >>>>>>> +maintainers: >>>>>>> + - Anitha Chrisanthus <anitha.chrisanthus@intel.com> >>>>>>> + - Edmond J Dea <edmund.j.dea@intel.com> >>>>>>> + >>>>>>> +properties: >>>>>>> + compatible: >>>>>>> + const: intel,keembay-display >>>>>>> + >>>>>>> + reg: >>>>>>> + items: >>>>>>> + - description: LCD registers range >>>>>>> + - description: Msscam registers range >>>>>>> + >>>>> >>>>> Indeed the split is much better, but as you replied on http://lore.kernel.org/r/BY5PR11MB41827DE07436DD0454E24E6E8C0A0@BY5PR11MB4182.namprd11.prod.outlook.com >>>>> the msscam seems to be shared with the camera subsystem block, if this is the case it should be handled. >>>>> >>>>> If it's a shared register block, it could be defined as a "syscon" used by both subsystems. >>>> >>>> I think I got it now. >>>> >>>> msscam is used to enable clocks both for the display driver and the >>>> mipi-dsi part. >>> >>> If just clocks, then the msscam should be a clock provider possibly. >>> If not, then the below looks right. >> >> I agree >> >>> >>>> >>>> So it should be pulled in to a dedicated node - for example like this: >>>> >>>> mssscam: msscam@20910000 { >>>> compatible = "intel,keembay-msscam", "syscon"; >>>> reg = <0x20910000, 0x30>; >>>> reg-io-width = <4>; >>>> }; >>>> >>>> And ofc we need a binding file for that. >>>> >>>> >>>> And then we could have code like this in the display driver: >>>> regmap *msscam = syscon_regmap_lookup_by_compatible("intel,keembay-msscam"); >>>> if (IS_ERR(msscam)) >>>> tell-and goto-out; >> >> It's better to use a phandle in the display node, instead of looking for compatible nodes. > > No, you don't need it in DT unless there's more than one instance or > other parameters needed like register offsets/masks. The above is > actually faster too walking a list rather than a phandle look up > (though the phandle cache now may make it faster). A phandle makes the dependency explicit, anyway it's only an advice. Neil > > Rob >
On 02/11/2020 19:04, Sam Ravnborg wrote: > Hi Neil. > >>>>>>> --- >>>>>>> .../bindings/display/intel,keembay-display.yaml | 75 ++++++++++++++++++++++ >>>>>>> 1 file changed, 75 insertions(+) >>>>>>> create mode 100644 Documentation/devicetree/bindings/display/intel,keembay-display.yaml >>>>>>> >>>>>>> diff --git a/Documentation/devicetree/bindings/display/intel,keembay-display.yaml b/Documentation/devicetree/bindings/display/intel,keembay-display.yaml >>>>>>> new file mode 100644 >>>>>>> index 0000000..8a8effe >>>>>>> --- /dev/null >>>>>>> +++ b/Documentation/devicetree/bindings/display/intel,keembay-display.yaml >>>>>>> @@ -0,0 +1,75 @@ >>>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) >>>>>>> +%YAML 1.2 >>>>>>> +--- >>>>>>> +$id: http://devicetree.org/schemas/display/intel,keembay-display.yaml# >>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml# >>>>>>> + >>>>>>> +title: Devicetree bindings for Intel Keem Bay display controller >>>>>>> + >>>>>>> +maintainers: >>>>>>> + - Anitha Chrisanthus <anitha.chrisanthus@intel.com> >>>>>>> + - Edmond J Dea <edmund.j.dea@intel.com> >>>>>>> + >>>>>>> +properties: >>>>>>> + compatible: >>>>>>> + const: intel,keembay-display >>>>>>> + >>>>>>> + reg: >>>>>>> + items: >>>>>>> + - description: LCD registers range >>>>>>> + - description: Msscam registers range >>>>>>> + >>>>> >>>>> Indeed the split is much better, but as you replied on http://lore.kernel.org/r/BY5PR11MB41827DE07436DD0454E24E6E8C0A0@BY5PR11MB4182.namprd11.prod.outlook.com >>>>> the msscam seems to be shared with the camera subsystem block, if this is the case it should be handled. >>>>> >>>>> If it's a shared register block, it could be defined as a "syscon" used by both subsystems. >>>> >>>> I think I got it now. >>>> >>>> msscam is used to enable clocks both for the display driver and the >>>> mipi-dsi part. >>> >>> If just clocks, then the msscam should be a clock provider possibly. >>> If not, then the below looks right. > > I am feeling a little clueless here - sorry. > > Can you help with any example that does this? > > Everything I looked up in bindings/clock/ had a "#clock-cells" which is > not relevant for msscam - or so I think at least. Looking at the code make it clear it's not relevant to implement it as clock controller. The syscon is enough. Neil > > > Sam >
Hi Neil, > >>>> > >>>> msscam is used to enable clocks both for the display driver and the > >>>> mipi-dsi part. > >>> > >>> If just clocks, then the msscam should be a clock provider possibly. > >>> If not, then the below looks right. > > > > I am feeling a little clueless here - sorry. > > > > Can you help with any example that does this? > > > > Everything I looked up in bindings/clock/ had a "#clock-cells" which is > > not relevant for msscam - or so I think at least. > > Looking at the code make it clear it's not relevant to implement it as clock controller. > > The syscon is enough. Thanks, I think Anitha is going to send a v11 today with the sysconf change. Sam
diff --git a/Documentation/devicetree/bindings/display/intel,keembay-display.yaml b/Documentation/devicetree/bindings/display/intel,keembay-display.yaml new file mode 100644 index 0000000..8a8effe --- /dev/null +++ b/Documentation/devicetree/bindings/display/intel,keembay-display.yaml @@ -0,0 +1,75 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/display/intel,keembay-display.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Devicetree bindings for Intel Keem Bay display controller + +maintainers: + - Anitha Chrisanthus <anitha.chrisanthus@intel.com> + - Edmond J Dea <edmund.j.dea@intel.com> + +properties: + compatible: + const: intel,keembay-display + + reg: + items: + - description: LCD registers range + - description: Msscam registers range + + reg-names: + items: + - const: lcd + - const: msscam + + clocks: + items: + - description: LCD controller clock + - description: pll0 clock + + clock-names: + items: + - const: clk_lcd + - const: clk_pll0 + + interrupts: + maxItems: 1 + + port: + type: object + description: Display output node to DSI. + +required: + - compatible + - reg + - reg-names + - clocks + - clock-names + - interrupts + - port + +additionalProperties: false + +examples: + - | + #include <dt-bindings/interrupt-controller/irq.h> + #include <dt-bindings/interrupt-controller/arm-gic.h> + + display@20930000 { + compatible = "intel,keembay-display"; + reg = <0x20930000 0x3000>, + <0x20910000 0x30>; + reg-names = "lcd", "msscam"; + interrupts = <GIC_SPI 33 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&scmi_clk 0x83>, + <&scmi_clk 0x0>; + clock-names = "clk_lcd", "clk_pll0"; + + port { + disp_out: endpoint { + remote-endpoint = <&dsi_in>; + }; + }; + };
This patch adds bindings for Intel KeemBay Display v2: review changes from Rob Herring v3: review changes from Sam Ravnborg (removed mipi dsi entries, and encoder entry, connect port to dsi) MSSCAM is part of the display submodule and its used to reset LCD and MIPI DSI clocks, so its best to be on this device tree. Signed-off-by: Anitha Chrisanthus <anitha.chrisanthus@intel.com> Cc: Sam Ravnborg <sam@ravnborg.org> Cc: Thomas Zimmermann <tzimmermann@suse.de> Cc: Daniel Vetter <daniel@ffwll.ch> --- .../bindings/display/intel,keembay-display.yaml | 75 ++++++++++++++++++++++ 1 file changed, 75 insertions(+) create mode 100644 Documentation/devicetree/bindings/display/intel,keembay-display.yaml