Hi Conor & All, On 22/01/24 2:46 pm, Conor Dooley wrote: > On Mon, Jan 22, 2024 at 03:38:41AM +0000,Dharma.B@xxxxxxxxxxxxx wrote: >> Hi Conor, >> On 19/01/24 5:33 pm, Conor Dooley - M52691 wrote: >>> On Fri, Jan 19, 2024 at 03:32:49AM +0000,Dharma.B@xxxxxxxxxxxxx wrote: >>>> On 18/01/24 9:10 pm, Conor Dooley wrote: >>>>> On Thu, Jan 18, 2024 at 02:56:12PM +0530, Dharma Balasubiramani wrote: >>>>>> Convert the atmel,hlcdc binding to DT schema format. >>>>>> >>>>>> Adjust the clock-names property to clarify that the LCD controller expects >>>>>> one of these clocks (either sys_clk or lvds_pll_clk to be present but not >>>>>> both) along with the slow_clk and periph_clk. This alignment with the actual >>>>>> hardware requirements will enable accurate device tree configuration for >>>>>> systems using the HLCDC IP. >>>>>> >>>>>> Signed-off-by: Dharma Balasubiramani<dharma.b@xxxxxxxxxxxxx> >>>>>> --- >>>>>> changelog >>>>>> v2 -> v3 >>>>>> - Rename hlcdc-display-controller and hlcdc-pwm to generic names. >>>>>> - Modify the description by removing the unwanted comments and '|'. >>>>>> - Modify clock-names simpler. >>>>>> v1 -> v2 >>>>>> - Remove the explicit copyrights. >>>>>> - Modify title (not include words like binding/driver). >>>>>> - Modify description actually describing the hardware and not the driver. >>>>>> - Add details of lvds_pll addition in commit message. >>>>>> - Ref endpoint and not endpoint-base. >>>>>> - Fix coding style. >>>>>> ... >>>>>> .../devicetree/bindings/mfd/atmel,hlcdc.yaml | 97 +++++++++++++++++++ >>>>>> .../devicetree/bindings/mfd/atmel-hlcdc.txt | 56 ----------- >>>>>> 2 files changed, 97 insertions(+), 56 deletions(-) >>>>>> create mode 100644 Documentation/devicetree/bindings/mfd/atmel,hlcdc.yaml >>>>>> delete mode 100644 Documentation/devicetree/bindings/mfd/atmel-hlcdc.txt >>>>>> >>>>>> diff --git a/Documentation/devicetree/bindings/mfd/atmel,hlcdc.yaml b/Documentation/devicetree/bindings/mfd/atmel,hlcdc.yaml >>>>>> new file mode 100644 >>>>>> index 000000000000..eccc998ac42c >>>>>> --- /dev/null >>>>>> +++ b/Documentation/devicetree/bindings/mfd/atmel,hlcdc.yaml >>>>>> @@ -0,0 +1,97 @@ >>>>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) >>>>>> +%YAML 1.2 >>>>>> +--- >>>>>> +$id:http://devicetree.org/schemas/mfd/atmel,hlcdc.yaml# >>>>>> +$schema:http://devicetree.org/meta-schemas/core.yaml# >>>>>> + >>>>>> +title: Atmel's HLCD Controller >>>>>> + >>>>>> +maintainers: >>>>>> + - Nicolas Ferre<nicolas.ferre@xxxxxxxxxxxxx> >>>>>> + - Alexandre Belloni<alexandre.belloni@xxxxxxxxxxx> >>>>>> + - Claudiu Beznea<claudiu.beznea@xxxxxxxxx> >>>>>> + >>>>>> +description: >>>>>> + The Atmel HLCDC (HLCD Controller) IP available on Atmel SoCs exposes two >>>>>> + subdevices, a PWM chip and a Display Controller. >>>>>> + >>>>>> +properties: >>>>>> + compatible: >>>>>> + enum: >>>>>> + - atmel,at91sam9n12-hlcdc >>>>>> + - atmel,at91sam9x5-hlcdc >>>>>> + - atmel,sama5d2-hlcdc >>>>>> + - atmel,sama5d3-hlcdc >>>>>> + - atmel,sama5d4-hlcdc >>>>>> + - microchip,sam9x60-hlcdc >>>>>> + - microchip,sam9x75-xlcdc >>>>>> + >>>>>> + reg: >>>>>> + maxItems: 1 >>>>>> + >>>>>> + interrupts: >>>>>> + maxItems: 1 >>>>>> + >>>>>> + clocks: >>>>>> + maxItems: 3 >>>>> Hmm, one thing I probably should have said on the previous version, but >>>>> I missed somehow: It would be good to add an items list to the clocks >>>>> property here to explain what the 3 clocks are/are used for - especially >>>>> since there is additional complexity being added here to use either the >>>>> sys or lvds clocks. >>>> May I inquire if this approach is likely to be effective? >>>> >>>> clocks: >>>> items: >>>> - description: peripheral clock >>>> - description: generic clock or lvds pll clock >>>> Once the LVDS PLL is enabled, the pixel clock is used as the >>>> clock for LCDC, so its GCLK is no longer needed. >>>> - description: slow clock >>>> maxItems: 3 >>> Hmm that sounds very suspect to me. "Once the lvdspll is enabled the >>> generic clock is no longer needed" sounds like both clocks can be provided >>> to the IP on different pins and their provision is not mutually >>> exclusive, just that the IP will only actually use one at a time. If >>> that is the case, then this patch is nott correct and the binding should >>> allow for 4 clocks, with both the generic clock and the lvds pll being >>> present in the DT at the same time. >>> >>> I vaguely recall internal discussion about this problem some time back >>> but the details all escape me. >> Let's delve deeper into the clock configuration for LCDC_PCK. >> >> Considering the flexibility of the design, it appears that both clocks, >> sys_clk (generic clock) and lvds_pll_clk, can indeed be provided to the >> IP simultaneously. The crucial aspect, however, is that the IP will >> utilize only one of these clocks at any given time. This aligns with the >> specific requirements of the application, where the choice of clock >> depends on whether the LVDS interface or MIPI/DSI is in use. > If both clocks can physically be provided to the IP then both of them > should be in the dt. The hcldc appears to me to be a part of the SoC and > the clock routing to the IP is likely fixed. > >> To ensure proper configuration of the pixel clock period, we need to >> distinctly identify which clocks are being utilized. For instance, in >> the LVDS interface scenario, the lvds_pll_clk is essential, resulting in >> LCDC_PCK being set to the source clock. Conversely, in the MIPI/DSI >> case, the LCDC GCLK is required, leading to LCDC_PCK being defined as >> source clock/CLKDIV+2. >> >> Considering the potential coexistence of sys_clk and lvds_pll_clk in the >> Device Tree (DT), we may need to introduce an additional flag in the DT. >> This flag could serve as a clear indicator of whether the LVDS interface >> or MIPI/DSI is being employed. As we discussed to drop this flag and >> just have any one of the clocks I believe that this approach provides a >> sensible and scalable solution, allowing for a comprehensive >> representation of the clocking configuration. > This is probably a question for the folks on the DRM or media side of > things, but is it not possible to determine based on the endpoint what > protocol is required? > I know that on the media side of things there's an endpoint property > that can be used to specific the bus-type - is there an equivalent > property for DRM stuff? Yes, it can be done. I will have the lvds pll in the lvds DT node. I will just convert the existing text binding to yaml without this additonal lvds pll clock. -- Thanks, Dharma B. > > Cheers, > Conor.
