Hi Conor, On 24/01/24 10:09 pm, Conor Dooley wrote: > On Wed, Jan 24, 2024 at 05:18:26AM +0000,Dharma.B@xxxxxxxxxxxxx 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@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. > 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. >
