> -----Original Message----- > From: Marc Kleine-Budde [mailto:mkl@xxxxxxxxxxxxxx] > Sent: Friday, November 23, 2018 8:57 PM > To: Pankaj Bansal <pankaj.bansal@xxxxxxx>; Wolfgang Grandegger > <wg@xxxxxxxxxxxxxx>; Shawn Guo <shawnguo@xxxxxxxxxx>; Leo Li > <leoyang.li@xxxxxxx> > Cc: linux-can@xxxxxxxxxxxxxxx > Subject: Re: [PATCH v2] Documentation: can: flexcan: Add flexcan clocks' > information > > On 11/12/18 9:10 AM, Pankaj Bansal wrote: > > The property clock-frequency is optional for device tree probe. When > > it's absent, the flexcan driver will try to get the frequency from clk > > system by calling clk_get_rate. > > > > But, the clk_get_rate requires that clocks and clock-names properties > > to be present in the flexcan node. > > Document the usage of these properties. > > > > Signed-off-by: Pankaj Bansal <pankaj.bansal@xxxxxxx> > > --- > > > > Notes: > > V2: > > - Added notes that mentioning clocks' phandles is preferred method > > over mentioning clock-frequency > > > > .../bindings/net/can/fsl-flexcan.txt | 37 ++++++++++++++++-- > > 1 file changed, 34 insertions(+), 3 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt > > b/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt > > index bfc0c433654f..e1f9cc17d818 100644 > > --- a/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt > > +++ b/Documentation/devicetree/bindings/net/can/fsl-flexcan.txt > > @@ -12,10 +12,29 @@ Required properties: > > - reg : Offset and length of the register set for this device > > - interrupts : Interrupt tuple for this device > > > > -Optional properties: > > +The input clock is the one fed to the prescaler to generate the > > +serial clock > > Actually the "per" clock is used as the input for the pre-scaler to generate the > serial clock to generate the waveform: Yes I see that in code : https://elixir.bootlin.com/linux/latest/source/drivers/net/can/flexcan.c#L1204 Technically hardware provides the option to select either oscillator clock or peripheral clock for flexcan. But, in s/w we have chosen to always use peripheral clock. As device tree is s/w agnostic, I think this information Should be put. > > > clock_freq = clk_get_rate(clk_per); > > > > +(Sclock). Serial Clock (Sclock), whose period defines the 'time quantum' > > +used to compose the CAN waveform. The input clock information can be > > +given in two formats > > + > > On modern fsl/nxp ARM cores you actually need both clocks, the ipg and the per > clock. IIRC the ipg is needed to access the register space, while the per is used to > generate the waveform. First of all I think "ipg" is a misnomer. The oscillator clock should be named as "osc" https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/clock/clock-bindings.txt#L99 Second, it's the peripheral clock that is required to access the register space. Refer : 17.5.9 Clock domains and restrictions The FlexCAN module has two clock domains asynchronous to each other: • The Bus Domain feeds the Control Host Interface (CHI) submodule and is derived from the peripheral clock. • The Oscillator Domain feeds the CAN Protocol Engine (PE) submodule and is derived directly from a crystal oscillator clock, so that very low jitter performance can be achieved on the CAN bus. When CAN_CTRL1[CLKSRC] bit is set, synchronous operation occurs because both domains are connected to the peripheral clock (creating a 1:1 ratio between the peripheral and oscillator clocks). When the two domains are connected to clocks with different frequencies and/or phases, there are restrictions on the frequency relationship between the two clock domains. In the case of asynchronous operation, the Bus Domain clock frequency must always be greater than the Oscillator Domain clock frequency. NOTE Asynchronous operation with a 1:1 ratio between peripheral and oscillator clocks is not allowed. So in nutshell peripheral clock or "per" is the essential clock, while oscillator clock or "osc" is optional. Also if "osc" is to be used, it has to be less than "per". And as per above the information in existing dts files is wrong (they report osc = per) : https://elixir.bootlin.com/linux/latest/source/arch/arm/boot/dts/ls1021a.dtsi#L762 https://elixir.bootlin.com/linux/latest/source/arch/arm/boot/dts/imx35.dtsi#L304 > > > +- clocks: phandle to the clocks feeding the SPI controller. Two can be given: > > "SPI controller" should be CAN controller I suppose. Thanks for pointing out. I will fix > > "Two _must_ be given", for this method of configuration. As I explained above, per is essential while osc is optional. > > > + - "ipg": Oscillator Clock > > + - "per": Peripheral Clock > > +- clock-names: Must contain the clock names described just above Note > > +: This method is preferred method. > > + > > + or > > + > > +- clock-frequency : The oscillator/peripheral clock frequency > > + driving the flexcan device. > > +Note : Only on platforms where you don't have your clocks in the Device > Tree, > > + use "clock-frequency" (as a fallback). > > > > -- clock-frequency : The oscillator frequency driving the flexcan > > device > > +The oscillator clock should be selected whenever a tight tolerance > > +(up to 0.1%) is required in the CAN bus timing. The crystal > > +oscillator clock has better jitter performance than the peripheral clock. > > Using the clock-frequency attribute is only useful if the kernel doesn't support > the generic clock framework for your SoC. Then it's up to someone else (i.e. not > the flexcan driver) to turn on _all_ needed clocks for the CAN core to work, > select whatever clock is preferred to be the input clock for the pre-scaler and > add exactly that clock's frequency to the "clock-frequency" attribute in the > device tree. I agree with you. I will write this same line that you have written here in Documentation. > > > > > +Optional properties: > > - xceiver-supply: Regulator that powers the CAN transceiver > > > > - big-endian: This means the registers of FlexCAN controller are big endian. > > @@ -25,11 +44,23 @@ Optional properties: > > endian. > > > > Example: > > + can0: can@2180000 { > > + compatible = "fsl,lx2160ar1-flexcan"; > > + reg = <0x0 0x2180000 0x0 0x10000>; > > + interrupts = <GIC_SPI 21 IRQ_TYPE_LEVEL_HIGH>; > > + clocks = <&clockgen 4 7>, <&clockgen 4 7>; // preferred > method This line is wrong. I will change it to correct line "<&sysclk>, <&clockgen 4 7>" > > + clock-names = "ipg", "per"; > > + status = "disabled"; > > + }; > > > > can@1c000 { > > compatible = "fsl,p1010-flexcan"; > > reg = <0x1c000 0x1000>; > > interrupts = <48 0x2>; > > interrupt-parent = <&mpic>; > > - clock-frequency = <200000000>; // filled in by bootloader > > + clock-frequency = <200000000>; // fallback method if no clocks > > + // in device tree. can be filled > > + // by bootloader > > + big-endian; > > }; > > + > > > > Marc > > -- > Pengutronix e.K. | Marc Kleine-Budde | > Industrial Linux Solutions | Phone: +49-231-2826-924 | > Vertretung West/Dortmund | Fax: +49-5121-206917-5555 | > Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
