Re: [PATCH v3 3/3] dt-bindings: mfd: atmel,hlcdc: Convert to DT schema format

[<Dharma.B@microchip.com>]

Hi Conor,
On 24/01/24 10:09 pm, Conor Dooley wrote:
> On Wed, Jan 24, 2024 at 05:18:26AM +0000,Dharma.B@microchip.com  wrote:
>> 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@microchip.com   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@microchip.com   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@microchip.com>
>>>>>>>> ---
>>>>>>>> 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@microchip.com>
>>>>>>>> +  - Alexandre Belloni<alexandre.belloni@bootlin.com>
>>>>>>>> +  - Claudiu Beznea<claudiu.beznea@tuxon.dev>
>>>>>>>> +
>>>>>>>> +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.
> If the lvds pll is an input to the hlcdc, you need to add it here.
>  From your description earlier it does sound like it is an input to
> the hlcdc, but now you are claiming that it is not.

The LVDS PLL serves as an input to both the LCDC and LVDSC, with the 
LVDS_PLL multiplied by 7 for the Pixel clock to the LVDS PHY, and 
LVDS_PLL divided by 7 for the Pixel clock to the LCDC.

I am inclined to believe that appropriately configuring and enabling it 
in the LVDS driver would be the appropriate course of action.

> 
> I don't know your hardware, so I have no idea which of the two is
> correct, but it sounds like the former. Without digging into how this
> works my assumption about the hardware here looks like is that the lvds
> controller is a clock provider,

It's a PLL clock from PMC.

> and that the lvds controller's clock is
> an optional input for the hlcdc.

Again it's a PLL clock from PMC.

Please refer Section 39.3 
https://ww1.microchip.com/downloads/aemDocuments/documents/MPU32/ProductDocuments/DataSheets/SAM9X7-Series-Data-Sheet-DS60001813.pdf

> 
> Can you please explain what provides the lvds pll clock and show an
> example of how you think the devictree would look with "the lvds pll in
> the lvds dt node"?

Sure, Please see the below example

The typical lvds node will look like

                 lvds_controller: lvds-controller@f8060000 {
                         compatible = "microchip,sam9x7-lvds";
                         reg = <0xf8060000 0x100>;
                         interrupts = <56 IRQ_TYPE_LEVEL_HIGH 0>;
                         clocks = <&pmc PMC_TYPE_PERIPHERAL 56>, <&pmc 
PMC_TYPE_CORE PMC_LVDSPLL>;
                         clock-names = "pclk", "lvds_pll_clk";
                         status = "disabled";
                 };

-- 
With Best Regards,
Dharma B.
> 
> Thanks,
> Conor.
>