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

[<Dharma.B@microchip.com>]

Hi Conor,

On 25/01/24 1:57 pm, Conor Dooley - M52691 wrote:
> 
>>> 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
> 
> Then it should be an input to both the LCDC and LVDSC in the devicetree.

For the LVDSC to operate, the presence of the LVDS PLL is crucial. However, in the case of the LCDC, LVDS PLL is not essential for its operation unless LVDS interface is used and when it is used lvds driver will take care of preparing and enabling the LVDS PLL.

Consequently, it seems that there might not be any significant actions we can take within the LCD driver regarding the LVDS PLL.

If there are no intentions to utilize it within the driver, is it necessary to explicitly designate it as an input in the device tree?

If yes, I will update the bindings with optional LVDS PLL clock.

clock-names:
  items:
    - const: periph_clk
    - const: sys_clk
    - const: slow_clk
    - const: lvds_pll  # Optional clock


> 
>> with the
>> LVDS_PLL multiplied by 7 for the Pixel clock to the LVDS PHY, and
> 
> Are you sure? The diagram doesn't show a multiplier, the 7x comment
> there seems to be showing relations?

Sorry, 
LVDS PLL = (PCK * 7) goes to LVDSC PHY
PCK = (LVDS PLL / 7) goes to LCDC

> 
>> 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.
> 
> We're talking about bindings here, not drivers, but I would imagine that
> if two peripherals are using the same clock then both of them should be
> getting a reference to and enabling that clock so that the clock
> framework can correctly track the users.
> 
>>> 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
> 
> It is not the same exact clock as you pointed out above though, so the
> by 7 divider should be modelled.

Modelled in mfd binding? If possible, could you please provide an example for better clarity? Thank you.

> 
>>> 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";
>>                   };
> 
> In isolation, this looks fine.
> 
> Cheers,
> Conor.
-- 
With Best Regards,
Dharma B.