| Message ID | 1515377863-20358-2-git-send-email-david@lechnology.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Monday 08 January 2018 07:47 AM, David Lechner wrote: > This adds a new binding for the PLL IP blocks in the mach-davinci family > of processors. Currently, only the SYSCLKn and AUXCLK outputs are needed, > but in the future additional child nodes could be added for OBSCLK and > BPDIV. > > Note: Although these PLL controllers are very similar to the TI Keystone > SoCs, we are not re-using those bindings. The Keystone bindings use a > legacy one-node-per-clock binding. Furthermore, the mach-davinici SoCs Not sure what is meant by "legacy one-node-per-clock binding" > have a slightly different PLL register layout and a number of quirks that > can't be handled by the existing bindings, so the keystone bindings could > not be used as-is anyway. Right, I think different register layout between the processors is the main reason for a new driver. This should be sufficient reason IMO. > > Signed-off-by: David Lechner <david@lechnology.com> > --- > .../devicetree/bindings/clock/ti/davinci/pll.txt | 47 ++++++++++++++++++++++ > 1 file changed, 47 insertions(+) > create mode 100644 Documentation/devicetree/bindings/clock/ti/davinci/pll.txt > > diff --git a/Documentation/devicetree/bindings/clock/ti/davinci/pll.txt b/Documentation/devicetree/bindings/clock/ti/davinci/pll.txt > new file mode 100644 > index 0000000..99bf5da > --- /dev/null > +++ b/Documentation/devicetree/bindings/clock/ti/davinci/pll.txt > @@ -0,0 +1,47 @@ > +Binding for TI DaVinci PLL Controllers > + > +The PLL provides clocks to most of the components on the SoC. In addition > +to the PLL itself, this controller also contains bypasses, gates, dividers, > +an multiplexers for various clock signals. > + > +Required properties: > +- compatible: shall be one of: > + - "ti,da850-pll0" for PLL0 on DA850/OMAP-L138/AM18XX > + - "ti,da850-pll1" for PLL1 on DA850/OMAP-L138/AM18XX These PLLs are same IP so they should use the same compatible. You can initialize both PLLs for DA850 based on the same compatible. Thanks, Sekhar
On 01/08/2018 08:00 AM, Sekhar Nori wrote: > On Monday 08 January 2018 07:47 AM, David Lechner wrote: >> This adds a new binding for the PLL IP blocks in the mach-davinci family >> of processors. Currently, only the SYSCLKn and AUXCLK outputs are needed, >> but in the future additional child nodes could be added for OBSCLK and >> BPDIV. >> >> Note: Although these PLL controllers are very similar to the TI Keystone >> SoCs, we are not re-using those bindings. The Keystone bindings use a >> legacy one-node-per-clock binding. Furthermore, the mach-davinici SoCs > > Not sure what is meant by "legacy one-node-per-clock binding" It's a term I picked up from of_clk_detect_critical() * Do not use this function. It exists only for legacy Device Tree * bindings, such as the one-clock-per-node style that are outdated. * Those bindings typically put all clock data into .dts and the Linux * driver has no clock data, thus making it impossible to set this flag * correctly from the driver. Only those drivers may call * of_clk_detect_critical from their setup functions. > >> have a slightly different PLL register layout and a number of quirks that >> can't be handled by the existing bindings, so the keystone bindings could >> not be used as-is anyway. > > Right, I think different register layout between the processors is the > main reason for a new driver. This should be sufficient reason IMO. > >> >> Signed-off-by: David Lechner <david@lechnology.com> >> --- >> .../devicetree/bindings/clock/ti/davinci/pll.txt | 47 ++++++++++++++++++++++ >> 1 file changed, 47 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/clock/ti/davinci/pll.txt >> >> diff --git a/Documentation/devicetree/bindings/clock/ti/davinci/pll.txt b/Documentation/devicetree/bindings/clock/ti/davinci/pll.txt >> new file mode 100644 >> index 0000000..99bf5da >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/clock/ti/davinci/pll.txt >> @@ -0,0 +1,47 @@ >> +Binding for TI DaVinci PLL Controllers >> + >> +The PLL provides clocks to most of the components on the SoC. In addition >> +to the PLL itself, this controller also contains bypasses, gates, dividers, >> +an multiplexers for various clock signals. >> + >> +Required properties: >> +- compatible: shall be one of: >> + - "ti,da850-pll0" for PLL0 on DA850/OMAP-L138/AM18XX >> + - "ti,da850-pll1" for PLL1 on DA850/OMAP-L138/AM18XX > > These PLLs are same IP so they should use the same compatible. You can > initialize both PLLs for DA850 based on the same compatible. > But they are not exactly the same. For example, PLL0 has 7 PLLDIV clocks while PLL1 only has 3. PLL0 has PREDIV while PLL1 does not. PLL0 has certain SYSCLKs that are fixed-ratio but PLL1 does not have any of these. There are even more differences, but these are the ones we are actually using. So, if we use the same compatible, we either have to come up with device tree bindings to describe all of this (yuck) or I suppose we can look at the REVID register to electronically determine exactly what we have. I went with the simpler option of just creating two different compatible strings.
On Monday 08 January 2018 09:59 PM, David Lechner wrote: > On 01/08/2018 08:00 AM, Sekhar Nori wrote: >> On Monday 08 January 2018 07:47 AM, David Lechner wrote: >>> This adds a new binding for the PLL IP blocks in the mach-davinci family >>> of processors. Currently, only the SYSCLKn and AUXCLK outputs are >>> needed, >>> but in the future additional child nodes could be added for OBSCLK and >>> BPDIV. >>> >>> Note: Although these PLL controllers are very similar to the TI Keystone >>> SoCs, we are not re-using those bindings. The Keystone bindings use a >>> legacy one-node-per-clock binding. Furthermore, the mach-davinici SoCs >> >> Not sure what is meant by "legacy one-node-per-clock binding" > > It's a term I picked up from of_clk_detect_critical() > > * Do not use this function. It exists only for legacy Device Tree > * bindings, such as the one-clock-per-node style that are outdated. > * Those bindings typically put all clock data into .dts and the Linux > * driver has no clock data, thus making it impossible to set this flag > * correctly from the driver. Only those drivers may call > * of_clk_detect_critical from their setup functions. Okay, I still don't understand the outdated style. I looked at clocks defined in arch/arm/boot/dts/stih407-clock.dtsi which is the only file that uses clock-critical and don't particularly see anything wrong with the way clocks are defined there. Anyway, I guess we digress. As long as this patch series is not using the "legacy style", we are good :) >>> have a slightly different PLL register layout and a number of quirks >>> that >>> can't be handled by the existing bindings, so the keystone bindings >>> could >>> not be used as-is anyway. >> >> Right, I think different register layout between the processors is the >> main reason for a new driver. This should be sufficient reason IMO. >> >>> >>> Signed-off-by: David Lechner <david@lechnology.com> >>> --- >>> .../devicetree/bindings/clock/ti/davinci/pll.txt | 47 >>> ++++++++++++++++++++++ >>> 1 file changed, 47 insertions(+) >>> create mode 100644 >>> Documentation/devicetree/bindings/clock/ti/davinci/pll.txt >>> >>> diff --git >>> a/Documentation/devicetree/bindings/clock/ti/davinci/pll.txt >>> b/Documentation/devicetree/bindings/clock/ti/davinci/pll.txt >>> new file mode 100644 >>> index 0000000..99bf5da >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/clock/ti/davinci/pll.txt >>> @@ -0,0 +1,47 @@ >>> +Binding for TI DaVinci PLL Controllers >>> + >>> +The PLL provides clocks to most of the components on the SoC. In >>> addition >>> +to the PLL itself, this controller also contains bypasses, gates, >>> dividers, >>> +an multiplexers for various clock signals. >>> + >>> +Required properties: >>> +- compatible: shall be one of: >>> + - "ti,da850-pll0" for PLL0 on DA850/OMAP-L138/AM18XX >>> + - "ti,da850-pll1" for PLL1 on DA850/OMAP-L138/AM18XX >> >> These PLLs are same IP so they should use the same compatible. You can >> initialize both PLLs for DA850 based on the same compatible. >> > > But they are not exactly the same. For example, PLL0 has 7 PLLDIV clocks > while > PLL1 only has 3. PLL0 has PREDIV while PLL1 does not. PLL0 has certain > SYSCLKs > that are fixed-ratio but PLL1 does not have any of these. There are even > more > differences, but these are the ones we are actually using. We need each element of the PLLC to be modeled individually as a clock node. That is, PLL should only model the multiplier, the dividers including post and prediv should be modeled as divider clocks (hopefully being able to use the clk-divider.c library). The sysclks can be fixed-factor-clock type clocks. Without this flexible mechanism, we cannot (at least later) model things like DIV4.5 clock which is the only clock which derives from the output of PLL multiplier before the post divider is applied. Since with DT there are are no retakes, we need to get this right the first time and modifying later will not be an option. > > So, if we use the same compatible, we either have to come up with device > tree > bindings to describe all of this (yuck) or I suppose we can look at the > REVID > register to electronically determine exactly what we have. I went with the > simpler option of just creating two different compatible strings. Thanks, Sekhar
On 01/09/2018 06:35 AM, Sekhar Nori wrote: > On Monday 08 January 2018 09:59 PM, David Lechner wrote: >> On 01/08/2018 08:00 AM, Sekhar Nori wrote: >>> On Monday 08 January 2018 07:47 AM, David Lechner wrote: >>>> This adds a new binding for the PLL IP blocks in the mach-davinci family >>>> of processors. Currently, only the SYSCLKn and AUXCLK outputs are >>>> needed, >>>> but in the future additional child nodes could be added for OBSCLK and >>>> BPDIV. >>>> >>>> Note: Although these PLL controllers are very similar to the TI Keystone >>>> SoCs, we are not re-using those bindings. The Keystone bindings use a >>>> legacy one-node-per-clock binding. Furthermore, the mach-davinici SoCs >>> >>> Not sure what is meant by "legacy one-node-per-clock binding" >> >> It's a term I picked up from of_clk_detect_critical() >> >> * Do not use this function. It exists only for legacy Device Tree >> * bindings, such as the one-clock-per-node style that are outdated. >> * Those bindings typically put all clock data into .dts and the Linux >> * driver has no clock data, thus making it impossible to set this flag >> * correctly from the driver. Only those drivers may call >> * of_clk_detect_critical from their setup functions. > > Okay, I still don't understand the outdated style. I looked at clocks > defined in arch/arm/boot/dts/stih407-clock.dtsi which is the only file > that uses clock-critical and don't particularly see anything wrong with > the way clocks are defined there. > > Anyway, I guess we digress. As long as this patch series is not using > the "legacy style", we are good :) > >>>> have a slightly different PLL register layout and a number of quirks >>>> that >>>> can't be handled by the existing bindings, so the keystone bindings >>>> could >>>> not be used as-is anyway. >>> >>> Right, I think different register layout between the processors is the >>> main reason for a new driver. This should be sufficient reason IMO. >>> >>>> >>>> Signed-off-by: David Lechner <david@lechnology.com> >>>> --- >>>> .../devicetree/bindings/clock/ti/davinci/pll.txt | 47 >>>> ++++++++++++++++++++++ >>>> 1 file changed, 47 insertions(+) >>>> create mode 100644 >>>> Documentation/devicetree/bindings/clock/ti/davinci/pll.txt >>>> >>>> diff --git >>>> a/Documentation/devicetree/bindings/clock/ti/davinci/pll.txt >>>> b/Documentation/devicetree/bindings/clock/ti/davinci/pll.txt >>>> new file mode 100644 >>>> index 0000000..99bf5da >>>> --- /dev/null >>>> +++ b/Documentation/devicetree/bindings/clock/ti/davinci/pll.txt >>>> @@ -0,0 +1,47 @@ >>>> +Binding for TI DaVinci PLL Controllers >>>> + >>>> +The PLL provides clocks to most of the components on the SoC. In >>>> addition >>>> +to the PLL itself, this controller also contains bypasses, gates, >>>> dividers, >>>> +an multiplexers for various clock signals. >>>> + >>>> +Required properties: >>>> +- compatible: shall be one of: >>>> + - "ti,da850-pll0" for PLL0 on DA850/OMAP-L138/AM18XX >>>> + - "ti,da850-pll1" for PLL1 on DA850/OMAP-L138/AM18XX >>> >>> These PLLs are same IP so they should use the same compatible. You can >>> initialize both PLLs for DA850 based on the same compatible. >>> >> >> But they are not exactly the same. For example, PLL0 has 7 PLLDIV clocks >> while >> PLL1 only has 3. PLL0 has PREDIV while PLL1 does not. PLL0 has certain >> SYSCLKs >> that are fixed-ratio but PLL1 does not have any of these. There are even >> more >> differences, but these are the ones we are actually using. > > We need each element of the PLLC to be modeled individually as a clock > node. I gave this a good think while I have been working on this series and I came to the conclusion that we really don't need to do this. These components are all internal to the PLL IP block, so the compatible string is enough to tell us what we have. They only thing we need really in the device tree bindings are the connections that are external to the IP block. > That is, PLL should only model the multiplier, the dividers > including post and prediv should be modeled as divider clocks (hopefully > being able to use the clk-divider.c library). The sysclks can be > fixed-factor-clock type clocks. > > Without this flexible mechanism, we cannot (at least later) model things > like DIV4.5 clock which is the only clock which derives from the output > of PLL multiplier before the post divider is applied. > > Since with DT there are are no retakes, we need to get this right the > first time and modifying later will not be an option. > So, the full device tree binding would look something like this: + + pll0: clock-controller@11000 { + compatible = "ti,da850-pll0"; + reg = <0x11000 0x1000>; + clocks = <&ref_clk>, <&pll1_sysclk 3>, <&pll1_obsclk>; + clock-names = "oscin", pll1_sysclk3", "pll1_osbclk"; + oscin-square-wave; + + pll0_sysclk: sysclk { + #clock-cells = <1>; + }; + + pll0_auxclk: auxclk { + #clock-cells = <0>; + }; + + pll0_div45: div4.5 { + #clock-cells = <0>; + }; + + pll0_obsclk: obsclk { + #clock-cells = <0>; + assigned-clocks = <&pll0_sysclk 1>; + assigned-clock-names = "ocsrc"; + }; + }; There are three clocks coming into the IP block and there are 11 clocks going out (sysclk is 7 clocks). And you can specify the board-specific configuration, like having the "oscin-square-wave" flag when a square wave is used instead of a crystal oscillator and you can assign the multiplexer input that will be used by obsclk. (And, this binding is totally compatible with the binding I have already proposed - although, I see now it would be better to go ahead and add the clocks-names property.) How everything connects together is all implemented in the driver and the driver will be able to know what to do just by looking at the compatible string. Part of my motivation for doing things this way comes from my recent experience with writing some bindings for some LCD panels. These device tree bindings were notorious for trying to be one-size-fits all will lots of properties to try to describe all of the internal workings. And it turns out that just having a new compatible string for each device or variant and pushing all of details of the quirks into the driver is much simpler and cleaner and will make it easier for other projects to reuse the bindings.
On Wednesday 10 January 2018 08:31 AM, David Lechner wrote: > On 01/09/2018 06:35 AM, Sekhar Nori wrote: >> On Monday 08 January 2018 09:59 PM, David Lechner wrote: >>> On 01/08/2018 08:00 AM, Sekhar Nori wrote: >>>> On Monday 08 January 2018 07:47 AM, David Lechner wrote: >>>>> diff --git >>>>> a/Documentation/devicetree/bindings/clock/ti/davinci/pll.txt >>>>> b/Documentation/devicetree/bindings/clock/ti/davinci/pll.txt >>>>> new file mode 100644 >>>>> index 0000000..99bf5da >>>>> --- /dev/null >>>>> +++ b/Documentation/devicetree/bindings/clock/ti/davinci/pll.txt >>>>> @@ -0,0 +1,47 @@ >>>>> +Binding for TI DaVinci PLL Controllers >>>>> + >>>>> +The PLL provides clocks to most of the components on the SoC. In >>>>> addition >>>>> +to the PLL itself, this controller also contains bypasses, gates, >>>>> dividers, >>>>> +an multiplexers for various clock signals. >>>>> + >>>>> +Required properties: >>>>> +- compatible: shall be one of: >>>>> + - "ti,da850-pll0" for PLL0 on DA850/OMAP-L138/AM18XX >>>>> + - "ti,da850-pll1" for PLL1 on DA850/OMAP-L138/AM18XX >>>> >>>> These PLLs are same IP so they should use the same compatible. You can >>>> initialize both PLLs for DA850 based on the same compatible. >>>> >>> >>> But they are not exactly the same. For example, PLL0 has 7 PLLDIV clocks >>> while >>> PLL1 only has 3. PLL0 has PREDIV while PLL1 does not. PLL0 has certain >>> SYSCLKs >>> that are fixed-ratio but PLL1 does not have any of these. There are even >>> more >>> differences, but these are the ones we are actually using. >> >> We need each element of the PLLC to be modeled individually as a clock >> node. > > I gave this a good think while I have been working on this series > and I came to the conclusion that we really don't need to do this. > These components are all internal to the PLL IP block, so the > compatible string is enough to tell us what we have. They only > thing we need really in the device tree bindings are the connections > that are external to the IP block. > > >> That is, PLL should only model the multiplier, the dividers >> including post and prediv should be modeled as divider clocks (hopefully >> being able to use the clk-divider.c library). The sysclks can be >> fixed-factor-clock type clocks. >> >> Without this flexible mechanism, we cannot (at least later) model things >> like DIV4.5 clock which is the only clock which derives from the output >> of PLL multiplier before the post divider is applied. >> >> Since with DT there are are no retakes, we need to get this right the >> first time and modifying later will not be an option. >> > > So, the full device tree binding would look something like this: > > + > + pll0: clock-controller@11000 { > + compatible = "ti,da850-pll0"; > + reg = <0x11000 0x1000>; > + clocks = <&ref_clk>, <&pll1_sysclk 3>, <&pll1_obsclk>; > + clock-names = "oscin", pll1_sysclk3", "pll1_osbclk"; > + oscin-square-wave; > + > + pll0_sysclk: sysclk { > + #clock-cells = <1>; > + }; > + > + pll0_auxclk: auxclk { > + #clock-cells = <0>; > + }; > + > + pll0_div45: div4.5 { > + #clock-cells = <0>; > + }; > + > + pll0_obsclk: obsclk { > + #clock-cells = <0>; > + assigned-clocks = <&pll0_sysclk 1>; > + assigned-clock-names = "ocsrc"; > + }; > + }; Well, I guess this will work as well. And I am probably biased towards the style I mentioned because AM335x and other TI OMAP processors follow that. To make it easy to review that we have all bases covered, can you model the all PLLC0 and PLLC1 (input and output) clocks for the next version? > > There are three clocks coming into the IP block and there are 11 clocks > going out (sysclk is 7 clocks). And you can specify the board-specific > configuration, like having the "oscin-square-wave" flag when a square wave > is used instead of a crystal oscillator and you can assign the multiplexer Ideally the OSCIN vs CLKIN selection should be another clock mux whose output is one of the input clocks to PLL controller. But I can see the difficulty in handling that as the mux itself is controlled by the PLL controller. > input that will be used by obsclk. (And, this binding is totally compatible > with the binding I have already proposed - although, I see now it would > be better to go ahead and add the clocks-names property.) Also, please add the oscin-square-wave to the binding definition too. For the benefit of others reviewing and not familiar with the hardware, the users guide for DA850 is here: http://www.ti.com/lit/ug/spruh77c/spruh77c.pdf and the PLL block diagram is on page 143 (Figure 8-1). Thanks, Sekhar
On Wed, Jan 10, 2018 at 12:52 PM, Sekhar Nori <nsekhar@ti.com> wrote: > On Wednesday 10 January 2018 08:31 AM, David Lechner wrote: >> On 01/09/2018 06:35 AM, Sekhar Nori wrote: >>> On Monday 08 January 2018 09:59 PM, David Lechner wrote: >>>> On 01/08/2018 08:00 AM, Sekhar Nori wrote: >>>>> On Monday 08 January 2018 07:47 AM, David Lechner wrote: > >>>>>> diff --git >>>>>> a/Documentation/devicetree/bindings/clock/ti/davinci/pll.txt >>>>>> b/Documentation/devicetree/bindings/clock/ti/davinci/pll.txt >>>>>> new file mode 100644 >>>>>> index 0000000..99bf5da >>>>>> --- /dev/null >>>>>> +++ b/Documentation/devicetree/bindings/clock/ti/davinci/pll.txt >>>>>> @@ -0,0 +1,47 @@ >>>>>> +Binding for TI DaVinci PLL Controllers >>>>>> + >>>>>> +The PLL provides clocks to most of the components on the SoC. In >>>>>> addition >>>>>> +to the PLL itself, this controller also contains bypasses, gates, >>>>>> dividers, >>>>>> +an multiplexers for various clock signals. >>>>>> + >>>>>> +Required properties: >>>>>> +- compatible: shall be one of: >>>>>> + - "ti,da850-pll0" for PLL0 on DA850/OMAP-L138/AM18XX >>>>>> + - "ti,da850-pll1" for PLL1 on DA850/OMAP-L138/AM18XX >>>>> >>>>> These PLLs are same IP so they should use the same compatible. You can >>>>> initialize both PLLs for DA850 based on the same compatible. >>>>> >>>> >>>> But they are not exactly the same. For example, PLL0 has 7 PLLDIV clocks >>>> while >>>> PLL1 only has 3. PLL0 has PREDIV while PLL1 does not. PLL0 has certain >>>> SYSCLKs >>>> that are fixed-ratio but PLL1 does not have any of these. There are even >>>> more >>>> differences, but these are the ones we are actually using. >>> >>> We need each element of the PLLC to be modeled individually as a clock >>> node. >> >> I gave this a good think while I have been working on this series >> and I came to the conclusion that we really don't need to do this. >> These components are all internal to the PLL IP block, so the >> compatible string is enough to tell us what we have. They only >> thing we need really in the device tree bindings are the connections >> that are external to the IP block. >> >> >>> That is, PLL should only model the multiplier, the dividers >>> including post and prediv should be modeled as divider clocks (hopefully >>> being able to use the clk-divider.c library). The sysclks can be >>> fixed-factor-clock type clocks. >>> >>> Without this flexible mechanism, we cannot (at least later) model things >>> like DIV4.5 clock which is the only clock which derives from the output >>> of PLL multiplier before the post divider is applied. >>> >>> Since with DT there are are no retakes, we need to get this right the >>> first time and modifying later will not be an option. >>> >> >> So, the full device tree binding would look something like this: >> >> + >> + pll0: clock-controller@11000 { >> + compatible = "ti,da850-pll0"; >> + reg = <0x11000 0x1000>; >> + clocks = <&ref_clk>, <&pll1_sysclk 3>, <&pll1_obsclk>; >> + clock-names = "oscin", pll1_sysclk3", "pll1_osbclk"; >> + oscin-square-wave; >> + >> + pll0_sysclk: sysclk { >> + #clock-cells = <1>; >> + }; >> + >> + pll0_auxclk: auxclk { >> + #clock-cells = <0>; >> + }; >> + >> + pll0_div45: div4.5 { >> + #clock-cells = <0>; >> + }; >> + >> + pll0_obsclk: obsclk { >> + #clock-cells = <0>; >> + assigned-clocks = <&pll0_sysclk 1>; >> + assigned-clock-names = "ocsrc"; >> + }; >> + }; > > Well, I guess this will work as well. And I am probably biased towards > the style I mentioned because AM335x and other TI OMAP processors > follow that. > > To make it easy to review that we have all bases covered, can you model > the all PLLC0 and PLLC1 (input and output) clocks for the next version? > >> >> There are three clocks coming into the IP block and there are 11 clocks >> going out (sysclk is 7 clocks). And you can specify the board-specific >> configuration, like having the "oscin-square-wave" flag when a square wave >> is used instead of a crystal oscillator and you can assign the multiplexer > > Ideally the OSCIN vs CLKIN selection should be another clock mux whose > output is one of the input clocks to PLL controller. But I can see the > difficulty in handling that as the mux itself is controlled by the PLL > controller. > >> input that will be used by obsclk. (And, this binding is totally compatible >> with the binding I have already proposed - although, I see now it would >> be better to go ahead and add the clocks-names property.) > > Also, please add the oscin-square-wave to the binding definition too. > > For the benefit of others reviewing and not familiar with the hardware, > the users guide for DA850 is here: > http://www.ti.com/lit/ug/spruh77c/spruh77c.pdf > I am available tomorrow to build and test patches against the da850-evm. I just need to know which version(s) to test. adam > and the PLL block diagram is on page 143 (Figure 8-1). > > Thanks, > Sekhar
On 01/10/2018 04:24 PM, Adam Ford wrote: > > I am available tomorrow to build and test patches against the > da850-evm. I just need to know which version(s) to test. Great. As per the cover letter: You can find a working branch with everything included in the "common-clk-v5" branch of https://github.com/dlech/ev3dev-kernel.git.
On 01/10/2018 12:52 PM, Sekhar Nori wrote: > On Wednesday 10 January 2018 08:31 AM, David Lechner wrote: >> On 01/09/2018 06:35 AM, Sekhar Nori wrote: >>> On Monday 08 January 2018 09:59 PM, David Lechner wrote: >>>> On 01/08/2018 08:00 AM, Sekhar Nori wrote: >>>>> On Monday 08 January 2018 07:47 AM, David Lechner wrote: > >>>>>> diff --git >>>>>> a/Documentation/devicetree/bindings/clock/ti/davinci/pll.txt >>>>>> b/Documentation/devicetree/bindings/clock/ti/davinci/pll.txt >>>>>> new file mode 100644 >>>>>> index 0000000..99bf5da >>>>>> --- /dev/null >>>>>> +++ b/Documentation/devicetree/bindings/clock/ti/davinci/pll.txt >>>>>> @@ -0,0 +1,47 @@ >>>>>> +Binding for TI DaVinci PLL Controllers >>>>>> + >>>>>> +The PLL provides clocks to most of the components on the SoC. In >>>>>> addition >>>>>> +to the PLL itself, this controller also contains bypasses, gates, >>>>>> dividers, >>>>>> +an multiplexers for various clock signals. >>>>>> + >>>>>> +Required properties: >>>>>> +- compatible: shall be one of: >>>>>> + - "ti,da850-pll0" for PLL0 on DA850/OMAP-L138/AM18XX >>>>>> + - "ti,da850-pll1" for PLL1 on DA850/OMAP-L138/AM18XX >>>>> >>>>> These PLLs are same IP so they should use the same compatible. You can >>>>> initialize both PLLs for DA850 based on the same compatible. >>>>> >>>> >>>> But they are not exactly the same. For example, PLL0 has 7 PLLDIV clocks >>>> while >>>> PLL1 only has 3. PLL0 has PREDIV while PLL1 does not. PLL0 has certain >>>> SYSCLKs >>>> that are fixed-ratio but PLL1 does not have any of these. There are even >>>> more >>>> differences, but these are the ones we are actually using. >>> >>> We need each element of the PLLC to be modeled individually as a clock >>> node. >> >> I gave this a good think while I have been working on this series >> and I came to the conclusion that we really don't need to do this. >> These components are all internal to the PLL IP block, so the >> compatible string is enough to tell us what we have. They only >> thing we need really in the device tree bindings are the connections >> that are external to the IP block. >> >> >>> That is, PLL should only model the multiplier, the dividers >>> including post and prediv should be modeled as divider clocks (hopefully >>> being able to use the clk-divider.c library). The sysclks can be >>> fixed-factor-clock type clocks. >>> >>> Without this flexible mechanism, we cannot (at least later) model things >>> like DIV4.5 clock which is the only clock which derives from the output >>> of PLL multiplier before the post divider is applied. >>> >>> Since with DT there are are no retakes, we need to get this right the >>> first time and modifying later will not be an option. >>> >> >> So, the full device tree binding would look something like this: >> >> + >> + pll0: clock-controller@11000 { >> + compatible = "ti,da850-pll0"; >> + reg = <0x11000 0x1000>; >> + clocks = <&ref_clk>, <&pll1_sysclk 3>, <&pll1_obsclk>; >> + clock-names = "oscin", pll1_sysclk3", "pll1_osbclk"; >> + oscin-square-wave; >> + >> + pll0_sysclk: sysclk { >> + #clock-cells = <1>; >> + }; >> + >> + pll0_auxclk: auxclk { >> + #clock-cells = <0>; >> + }; >> + >> + pll0_div45: div4.5 { >> + #clock-cells = <0>; >> + }; >> + >> + pll0_obsclk: obsclk { >> + #clock-cells = <0>; >> + assigned-clocks = <&pll0_sysclk 1>; >> + assigned-clock-names = "ocsrc"; >> + }; >> + }; > > Well, I guess this will work as well. And I am probably biased towards > the style I mentioned because AM335x and other TI OMAP processors > follow that. > > To make it easy to review that we have all bases covered, can you model > the all PLLC0 and PLLC1 (input and output) clocks for the next version? Sure thing. > >> >> There are three clocks coming into the IP block and there are 11 clocks >> going out (sysclk is 7 clocks). And you can specify the board-specific >> configuration, like having the "oscin-square-wave" flag when a square wave >> is used instead of a crystal oscillator and you can assign the multiplexer > > Ideally the OSCIN vs CLKIN selection should be another clock mux whose > output is one of the input clocks to PLL controller. But I can see the > difficulty in handling that as the mux itself is controlled by the PLL > controller. > >> input that will be used by obsclk. (And, this binding is totally compatible >> with the binding I have already proposed - although, I see now it would >> be better to go ahead and add the clocks-names property.) > > Also, please add the oscin-square-wave to the binding definition too. > > For the benefit of others reviewing and not familiar with the hardware, > the users guide for DA850 is here: > http://www.ti.com/lit/ug/spruh77c/spruh77c.pdf > > and the PLL block diagram is on page 143 (Figure 8-1). > > Thanks, > Sekhar >
On Wed, Jan 10, 2018 at 8:50 PM, David Lechner <david@lechnology.com> wrote: > On 01/10/2018 04:24 PM, Adam Ford wrote: >> >> >> I am available tomorrow to build and test patches against the >> da850-evm. I just need to know which version(s) to test. > > > Great. As per the cover letter: > > You can find a working branch with everything included in the > "common-clk-v5" > branch of https://github.com/dlech/ev3dev-kernel.git. I wasn't sure if things had changed after some of the dialog about the bindings and device tree. Here is my log with DEBUG_LL and CONFIG_EARLY_PRINTK set : Starting kernel ... Uncompressing Linux... done, booting the kernel. Booting Linux on physical CPU 0x0 Linux version 4.15.0-rc4-g8564e0f (aford@ubuntu16) (gcc version 7.2.0 (Buildroot 2017.11.1-00021-g7b43660)) #2 PREEMPT Thu Jan 11 06:35:29 CST 2018 CPU: ARM926EJ-S [41069265] revision 5 (ARMv5TEJ), cr=0005317f CPU: VIVT data cache, VIVT instruction cache OF: fdt: Machine model: DA850/AM1808/OMAP-L138 EVM Memory policy: Data cache writethrough cma: Reserved 16 MiB at 0xc2c00000 DaVinci da850/omap-l138 variant 0x0 random: fast init done Built 1 zonelists, mobility grouping on. Total pages: 16256 Kernel command line: console=ttyS2,115200n8 root=PARTUUID= rw rootfstype=ext4 rootwait Dentry cache hash table entries: 8192 (order: 3, 32768 bytes) Inode-cache hash table entries: 4096 (order: 2, 16384 bytes) Memory: 42164K/65536K available (4548K kernel code, 280K rwdata, 1044K rodata, 232K init, 143K bss, 6988K reserved, 16384K cma-reserved) Virtual kernel memory layout: vector : 0xffff0000 - 0xffff1000 ( 4 kB) fixmap : 0xffc00000 - 0xfff00000 (3072 kB) vmalloc : 0xc4800000 - 0xff800000 ( 944 MB) lowmem : 0xc0000000 - 0xc4000000 ( 64 MB) modules : 0xbf000000 - 0xc0000000 ( 16 MB) .text : 0x(ptrval) - 0x(ptrval) (4550 kB) .init : 0x(ptrval) - 0x(ptrval) ( 232 kB) .data : 0x(ptrval) - 0x(ptrval) ( 281 kB) .bss : 0x(ptrval) - 0x(ptrval) ( 144 kB) SLUB: HWalign=32, Order=0-3, MinObjects=0, CPUs=1, Nodes=1 Preemptible hierarchical RCU implementation. Tasks RCU enabled. NR_IRQS: 245 clocksource: timer0_1: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 79635851949 ns sched_clock: 32 bits at 24MHz, resolution 41ns, wraps every 89478484971ns Console: colour dummy device 80x30 Calibrating delay loop... 148.88 BogoMIPS (lpj=744448) pid_max: default: 32768 minimum: 301 Mount-cache hash table entries: 1024 (order: 0, 4096 bytes) Mountpoint-cache hash table entries: 1024 (order: 0, 4096 bytes) CPU: Testing write buffer coherency: ok Setting up static identity map for 0xc0008400 - 0xc0008458 Hierarchical SRCU implementation. devtmpfs: initialized clocksource: jiffies: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 19112604462750000 ns futex hash table entries: 256 (order: -1, 3072 bytes) pinctrl core: initialized pinctrl subsystem NET: Registered protocol family 16 DMA: preallocated 256 KiB pool for atomic coherent allocations cpuidle: using governor menu mux: initialized RTC_ALARM mux: Setting register RTC_ALARM mux: PINMUX0 (0x00000000) = 0x44080000 -> 0x24080000 edma 1c00000.edma: memcpy is disabled edma 1c00000.edma: TI EDMA DMA engine driver edma 1e30000.edma: memcpy is disabled edma 1e30000.edma: TI EDMA DMA engine driver i2c_davinci 1c22000.i2c: could not find pctldev for node /soc@1c00000/pinmux@14120/pinmux_i2c0_pins, deferring probe clocksource: Switched to clocksource timer0_1 NET: Registered protocol family 2 TCP established hash table entries: 1024 (order: 0, 4096 bytes) TCP bind hash table entries: 1024 (order: 0, 4096 bytes) TCP: Hash tables configured (established 1024 bind 1024) UDP hash table entries: 256 (order: 0, 4096 bytes) UDP-Lite hash table entries: 256 (order: 0, 4096 bytes) NET: Registered protocol family 1 RPC: Registered named UNIX socket transport module. RPC: Registered udp transport module. RPC: Registered tcp transport module. RPC: Registered tcp NFSv4.1 backchannel transport module. Initialise system trusted keyrings workingset: timestamp_bits=30 max_order=14 bucket_order=0 Key type asymmetric registered Asymmetric key parser 'x509' registered Block layer SCSI generic (bsg) driver version 0.4 loaded (major 251) io scheduler noop registered (default) io scheduler mq-deadline registered io scheduler kyber registered pinctrl-single 1c14120.pinmux: 160 pins at pa fdfe34a6 size 80 Serial: 8250/16550 driver, 3 ports, IRQ sharing disabled 1c42000.serial: ttyS0 at MMIO 0x1c42000 (irq = 25, base_baud = 9375000) is a TI DA8xx/66AK2x 1d0c000.serial: ttyS1 at MMIO 0x1d0c000 (irq = 53, base_baud = 8250000) is a TI DA8xx/66AK2x 1d0d000.serial: ttyS2 at MMIO 0x1d0d000 (irq = 61, base_baud = 8250000) is a TI DA8xx/66AK2x console [ttyS2] enabled brd: module loaded libphy: Fixed MDIO Bus: probed davinci_mdio 1e24000.mdio: failed to get device clock davinci_mdio: probe of 1e24000.mdio failed with error -2 i2c /dev entries driver davinci_mmc 1c40000.mmc: Using DMA, 4-bit mode NET: Registered protocol family 10 Segment Routing with IPv6 sit: IPv6, IPv4 and MPLS over IPv4 tunneling driver NET: Registered protocol family 17 Loading compiled-in X.509 certificates mmc0: host does not support reading read-only switch, assuming write-enable mmc0: new high speed SDHC card at address b368 mmcblk0: mmc0:b368 00000 3.75 GiB mmcblk0: p1 p2 pca953x 0-0020: 0-0020 supply vcc not found, using dummy regulator pca953x 0-0020: failed reading register pca953x: probe of 0-0020 failed with error -121 console [netcon0] enabled netconsole: network logging started davinci_emac 1e20000.ethernet: incompatible machine/device type for reading mac address hctosys: unable to open rtc device (rtc0) >
On Thursday 11 January 2018 06:15 PM, Adam Ford wrote: > On Wed, Jan 10, 2018 at 8:50 PM, David Lechner <david@lechnology.com> wrote: >> On 01/10/2018 04:24 PM, Adam Ford wrote: >>> >>> >>> I am available tomorrow to build and test patches against the >>> da850-evm. I just need to know which version(s) to test. >> >> >> Great. As per the cover letter: >> >> You can find a working branch with everything included in the >> "common-clk-v5" >> branch of https://github.com/dlech/ev3dev-kernel.git. > > I wasn't sure if things had changed after some of the dialog about the > bindings and device tree. > > Here is my log with DEBUG_LL and CONFIG_EARLY_PRINTK set : > > Starting kernel ... > > Uncompressing Linux... done, booting the kernel. > Booting Linux on physical CPU 0x0 > Linux version 4.15.0-rc4-g8564e0f (aford@ubuntu16) (gcc version 7.2.0 > (Buildroot 2017.11.1-00021-g7b43660)) #2 PREEMPT Thu Jan 11 06:35:29 > CST 2018 > CPU: ARM926EJ-S [41069265] revision 5 (ARMv5TEJ), cr=0005317f > CPU: VIVT data cache, VIVT instruction cache > OF: fdt: Machine model: DA850/AM1808/OMAP-L138 EVM > Memory policy: Data cache writethrough > cma: Reserved 16 MiB at 0xc2c00000 > DaVinci da850/omap-l138 variant 0x0 > random: fast init done > Built 1 zonelists, mobility grouping on. Total pages: 16256 > Kernel command line: console=ttyS2,115200n8 root=PARTUUID= rw Pretty sure an actual UUID is missing here. Thanks, Sekhar
On Thu, Jan 11, 2018 at 9:47 AM, Sekhar Nori <nsekhar@ti.com> wrote: > On Thursday 11 January 2018 06:15 PM, Adam Ford wrote: >> On Wed, Jan 10, 2018 at 8:50 PM, David Lechner <david@lechnology.com> wrote: >>> On 01/10/2018 04:24 PM, Adam Ford wrote: >>>> >>>> >>>> I am available tomorrow to build and test patches against the >>>> da850-evm. I just need to know which version(s) to test. >>> >>> >>> Great. As per the cover letter: >>> >>> You can find a working branch with everything included in the >>> "common-clk-v5" >>> branch of https://github.com/dlech/ev3dev-kernel.git. >> >> I wasn't sure if things had changed after some of the dialog about the >> bindings and device tree. >> >> Here is my log with DEBUG_LL and CONFIG_EARLY_PRINTK set : >> >> Starting kernel ... >> >> Uncompressing Linux... done, booting the kernel. >> Booting Linux on physical CPU 0x0 >> Linux version 4.15.0-rc4-g8564e0f (aford@ubuntu16) (gcc version 7.2.0 >> (Buildroot 2017.11.1-00021-g7b43660)) #2 PREEMPT Thu Jan 11 06:35:29 >> CST 2018 >> CPU: ARM926EJ-S [41069265] revision 5 (ARMv5TEJ), cr=0005317f >> CPU: VIVT data cache, VIVT instruction cache >> OF: fdt: Machine model: DA850/AM1808/OMAP-L138 EVM >> Memory policy: Data cache writethrough >> cma: Reserved 16 MiB at 0xc2c00000 >> DaVinci da850/omap-l138 variant 0x0 >> random: fast init done >> Built 1 zonelists, mobility grouping on. Total pages: 16256 >> Kernel command line: console=ttyS2,115200n8 root=PARTUUID= rw > > Pretty sure an actual UUID is missing here. > When I setup root to be on the /dev/mmcblk0, I get the same result. The kernel still hangs. It doesn't hang using the mainline kernel without this patch series Starting kernel ... Uncompressing Linux... done, booting the kernel. Booting Linux on physical CPU 0x0 Linux version 4.15.0-rc4-g8564e0f (aford@ubuntu16) (gcc version 7.2.0 (Buildroot 2017.11.1-00021-g7b43660)) #2 PREEMPT Thu Jan 11 06:35:29 CST 2018 CPU: ARM926EJ-S [41069265] revision 5 (ARMv5TEJ), cr=0005317f CPU: VIVT data cache, VIVT instruction cache OF: fdt: Machine model: DA850/AM1808/OMAP-L138 EVM Memory policy: Data cache writethrough cma: Reserved 16 MiB at 0xc2c00000 DaVinci da850/omap-l138 variant 0x0 random: fast init done Built 1 zonelists, mobility grouping on. Total pages: 16256 Kernel command line: console=ttyS2,115200n8 root=/dev/mmcblk0p2 rw rootfstype=ext4 rootwait Dentry cache hash table entries: 8192 (order: 3, 32768 bytes) Inode-cache hash table entries: 4096 (order: 2, 16384 bytes) Memory: 42164K/65536K available (4548K kernel code, 280K rwdata, 1044K rodata, 232K init, 143K bss, 6988K reserved, 16384K cma-reserved) Virtual kernel memory layout: vector : 0xffff0000 - 0xffff1000 ( 4 kB) fixmap : 0xffc00000 - 0xfff00000 (3072 kB) vmalloc : 0xc4800000 - 0xff800000 ( 944 MB) lowmem : 0xc0000000 - 0xc4000000 ( 64 MB) modules : 0xbf000000 - 0xc0000000 ( 16 MB) .text : 0x(ptrval) - 0x(ptrval) (4550 kB) .init : 0x(ptrval) - 0x(ptrval) ( 232 kB) .data : 0x(ptrval) - 0x(ptrval) ( 281 kB) .bss : 0x(ptrval) - 0x(ptrval) ( 144 kB) SLUB: HWalign=32, Order=0-3, MinObjects=0, CPUs=1, Nodes=1 Preemptible hierarchical RCU implementation. Tasks RCU enabled. NR_IRQS: 245 clocksource: timer0_1: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 79635851949 ns sched_clock: 32 bits at 24MHz, resolution 41ns, wraps every 89478484971ns Console: colour dummy device 80x30 Calibrating delay loop... 148.88 BogoMIPS (lpj=744448) pid_max: default: 32768 minimum: 301 Mount-cache hash table entries: 1024 (order: 0, 4096 bytes) Mountpoint-cache hash table entries: 1024 (order: 0, 4096 bytes) CPU: Testing write buffer coherency: ok Setting up static identity map for 0xc0008400 - 0xc0008458 Hierarchical SRCU implementation. devtmpfs: initialized clocksource: jiffies: mask: 0xffffffff max_cycles: 0xffffffff, max_idle_ns: 19112604462750000 ns futex hash table entries: 256 (order: -1, 3072 bytes) pinctrl core: initialized pinctrl subsystem NET: Registered protocol family 16 DMA: preallocated 256 KiB pool for atomic coherent allocations cpuidle: using governor menu mux: initialized RTC_ALARM mux: Setting register RTC_ALARM mux: PINMUX0 (0x00000000) = 0x44080000 -> 0x24080000 edma 1c00000.edma: memcpy is disabled edma 1c00000.edma: TI EDMA DMA engine driver edma 1e30000.edma: memcpy is disabled edma 1e30000.edma: TI EDMA DMA engine driver i2c_davinci 1c22000.i2c: could not find pctldev for node /soc@1c00000/pinmux@14120/pinmux_i2c0_pins, deferring probe clocksource: Switched to clocksource timer0_1 NET: Registered protocol family 2 TCP established hash table entries: 1024 (order: 0, 4096 bytes) TCP bind hash table entries: 1024 (order: 0, 4096 bytes) TCP: Hash tables configured (established 1024 bind 1024) UDP hash table entries: 256 (order: 0, 4096 bytes) UDP-Lite hash table entries: 256 (order: 0, 4096 bytes) NET: Registered protocol family 1 RPC: Registered named UNIX socket transport module. RPC: Registered udp transport module. RPC: Registered tcp transport module. RPC: Registered tcp NFSv4.1 backchannel transport module. Initialise system trusted keyrings workingset: timestamp_bits=30 max_order=14 bucket_order=0 Key type asymmetric registered Asymmetric key parser 'x509' registered Block layer SCSI generic (bsg) driver version 0.4 loaded (major 251) io scheduler noop registered (default) io scheduler mq-deadline registered io scheduler kyber registered pinctrl-single 1c14120.pinmux: 160 pins at pa fdfe34a6 size 80 Serial: 8250/16550 driver, 3 ports, IRQ sharing disabled 1c42000.serial: ttyS0 at MMIO 0x1c42000 (irq = 25, base_baud = 9375000) is a TI DA8xx/66AK2x 1d0c000.serial: ttyS1 at MMIO 0x1d0c000 (irq = 53, base_baud = 8250000) is a TI DA8xx/66AK2x 1d0d000.serial: ttyS2 at MMIO 0x1d0d000 (irq = 61, base_baud = 8250000) is a TI DA8xx/66AK2x console [ttyS2] enabled brd: module loaded libphy: Fixed MDIO Bus: probed davinci_mdio 1e24000.mdio: failed to get device clock davinci_mdio: probe of 1e24000.mdio failed with error -2 i2c /dev entries driver davinci_mmc 1c40000.mmc: Using DMA, 4-bit mode NET: Registered protocol family 10 Segment Routing with IPv6 sit: IPv6, IPv4 and MPLS over IPv4 tunneling driver NET: Registered protocol family 17 Loading compiled-in X.509 certificates mmc0: host does not support reading read-only switch, assuming write-enable mmc0: new high speed SDHC card at address b368 mmcblk0: mmc0:b368 00000 3.75 GiB mmcblk0: p1 p2 pca953x 0-0020: 0-0020 supply vcc not found, using dummy regulator pca953x 0-0020: failed reading register pca953x: probe of 0-0020 failed with error -121 console [netcon0] enabled netconsole: network logging started davinci_emac 1e20000.ethernet: incompatible machine/device type for reading mac address hctosys: unable to open rtc device (rtc0) > Thanks, > Sekhar
diff --git a/Documentation/devicetree/bindings/clock/ti/davinci/pll.txt b/Documentation/devicetree/bindings/clock/ti/davinci/pll.txt new file mode 100644 index 0000000..99bf5da --- /dev/null +++ b/Documentation/devicetree/bindings/clock/ti/davinci/pll.txt @@ -0,0 +1,47 @@ +Binding for TI DaVinci PLL Controllers + +The PLL provides clocks to most of the components on the SoC. In addition +to the PLL itself, this controller also contains bypasses, gates, dividers, +an multiplexers for various clock signals. + +Required properties: +- compatible: shall be one of: + - "ti,da850-pll0" for PLL0 on DA850/OMAP-L138/AM18XX + - "ti,da850-pll1" for PLL1 on DA850/OMAP-L138/AM18XX +- reg: physical base address and size of the controller's register area. +- clocks: phandle to the PLL input clock source + +Optional child nodes: + +sysclk + Describes the PLLDIVn divider clocks that provide the SYSCLKn clock + domains. The node name must be "sysclk". Consumers of this node should + use "n" in "SYSCLKn" as the parameter for the clock cell. + + Required properties: + - #clock-cells: must be 1 + +auxclk + Describes the AUXCLK output of the PLL. The node name must be "auxclk". + + Required properties: + - #clock-cells: must be 0 + +Examples: + + pll0: clock-controller@11000 { + compatible = "ti,da850-pll0"; + reg = <0x11000 0x1000>; + clocks = <&ref_clk>; + + pll0_sysclk: sysclk { + #clock-cells = <1>; + }; + + pll0_aux_clk: auxclk { + #clock-cells = <0>; + }; + }; + +Also see: +- Documentation/devicetree/bindings/clock/clock-bindings.txt
This adds a new binding for the PLL IP blocks in the mach-davinci family of processors. Currently, only the SYSCLKn and AUXCLK outputs are needed, but in the future additional child nodes could be added for OBSCLK and BPDIV. Note: Although these PLL controllers are very similar to the TI Keystone SoCs, we are not re-using those bindings. The Keystone bindings use a legacy one-node-per-clock binding. Furthermore, the mach-davinici SoCs have a slightly different PLL register layout and a number of quirks that can't be handled by the existing bindings, so the keystone bindings could not be used as-is anyway. Signed-off-by: David Lechner <david@lechnology.com> --- .../devicetree/bindings/clock/ti/davinci/pll.txt | 47 ++++++++++++++++++++++ 1 file changed, 47 insertions(+) create mode 100644 Documentation/devicetree/bindings/clock/ti/davinci/pll.txt