> -----Original Message----- > From: Marc Kleine-Budde [mailto:mkl@xxxxxxxxxxxxxx] > Sent: Wednesday, November 28, 2018 7:27 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/28/18 7:34 AM, Pankaj Bansal wrote: > >>> --- 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. > > On some SoCs with flexcan IP core you can select the input clock for the CAN > waveform generation on others not...more details below. You are right. I checked that in I.MX6Q the CLKSRC is not implemented, so we cannot select the input clock. > > > >>> 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/devicetre > > e/bindings/clock/clock-bindings.txt#L99 > > "osc" is the external oscillator on most imx SoCs, following the clock binding > example you mentioned. > > I was not involved creating the imx SoC clock trees, but I think the clock were > named after the clocks names from the SoCs datasheet. > > This is the relevant part of the clock tree on a imx6: > > > enable prepare protect duty > > clock count count count rate accuracy phase cycle > > ----------------------------------------------------------------------------------------------- > --- > > osc 6 6 0 24000000 0 0 50000 > > pll3 1 1 0 480000000 0 0 50000 > > pll3_bypass 1 1 0 480000000 0 0 50000 > > pll3_usb_otg 4 4 0 480000000 0 0 50000 > > pll3_60m 1 1 0 60000000 0 0 50000 > > can_root 1 1 0 30000000 0 0 50000 > > can2_serial 0 0 0 30000000 0 0 50000 > > can1_serial 1 1 0 30000000 0 0 50000 > > pll2 1 1 0 528000000 0 0 50000 > > pll2_bypass 1 1 0 528000000 0 0 50000 > > pll2_bus 1 1 0 528000000 0 0 50000 > > cko1_sel 0 0 0 528000000 0 0 50000 > > periph_pre 1 1 0 396000000 0 0 50000 > > periph 4 4 0 396000000 0 0 50000 > > ahb 8 9 0 132000000 0 0 50000 > > ipg 6 8 0 66000000 0 0 50000 > > can2_ipg 0 0 0 66000000 0 0 50000 > > can1_ipg 1 1 0 66000000 0 0 50000 > > In addition the mx6qdl DT: > > clocks = <&clks IMX6QDL_CLK_CAN1_IPG>, > > <&clks IMX6QDL_CLK_CAN1_SERIAL>; > > clock-names = "ipg", "per"; > > > This means both can clocks are driven by the oscillator clock. While one goes > over pll2 the other over pll3. The canX_serial is used generate the timing for the > CAN wave form. The 24 MHz from the osc never reach the CAN core on the > mx6. > > In the mx6 I've found the following table of the flexcan core's external > signals: > > > Clock name Clock Root Description > > ipg_clk ipg_clk_root Peripheral clock > > ipg_clk_chi ipg_clk_root CHI clock > > ipg_clk_pe can_clk_root Protocol Engine clock > > ipg_clk_pe_nogate can_clk_root Protocol Engine clock (no gating) > > ipg_clk_s ipg_clk_root Peripheral access clock > > mem_ram_CLK ipg_clk_root RAM clock > > AFAICS the other datasheets only describe the rx/tx external signals. > > > Second, it's the peripheral clock that is required to access the register space. > > Refer : 17.5.9 Clock domains and restrictions > > 17.5.9 of which data sheet? Is it publicly available? It's mentioned in both LX2160A/LS1021A data sheet. The link is https://www.nxp.com/webapp/Download?colCode=LS1021ARM&Parent_nodeId=1378142598972722203474&Parent_pageType=product I think you need NXP account to download it. > > > 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. > > Is this a generic flexcan IP-Core datssheet? Does it take into account how the IP- > Core is integrated into SoCs (with publicly available datasheets)? The terminology "DOES" vary between different SOCs and flexcan IP-core datasheet. Here is what I found out: • The FLEXCAN module is divided into two blocks. Controller host interface ("CHI") and Protocol Engine ("PE") • Both these blocks require clock. • CHI is responsible for registers read write including MB read/write. While PE is responsible for Transfer/receive data on CAN bus. • The clocks feeding to these two blocks can be synchronous (i.e. same clock) or asynchronous (i.e. different clocks). • Selection is made in the CLK_SRC bit (bit 13) of Control 1 Register. • if CLK_SRC bit (bit 13) = 1, then CHI clock is used for PE (synchronous), and PE clock is not used. • If this bit is not implemented in SOC (or CLK_SRC bit (bit 13) = 0), then SOC only supports asynchronous clocking for FLEXCAN. • Either of the clock can be generated by any of the clock source. It can be separate oscillator or peripheral clock shared with other modules. • When the two clocks are asynchronous, then following restrictions apply to PE clock. PE clock must be less than CHI clock. • If low jitter is required on CAN bus dedicated oscillator can be used to provide PE clock, but it must be less than CHI 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. > > The reality is more complex. > > The only oscillator clock we have in the imx6 SoC is the 24 MHz (see clock tree > above). First the oscillator with 24 MHz is Pll'ed up to 480 MHz then divided back > to 60 MHz and finally 30 MHz. > > > 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). > > CLKSRC is bit 13? > > Looking at various datasheets: > > mx28: 0=crystal oscillator clock, 1=peripheral clock > hardwired to crystal oscillator clock > IIRC this is 24 MHz > mx25: 0=oscillator clock (24.576 MHz), 1=bus clock (66.5 MHz) > mx35: 0=oscillator clock (24.576 MHz), 1=bus clock (66.5 MHz) > mx53: marked as reserved, but documented: > 0=crystal oscillator (24.576 MHz), 1=bus clock (66.5 MHz) > mx6: marked as reserved, no further documentation > vf610: 0=oscillator clock, 1=peripheral clock > > Looking at the names of the clocks on the imx6 (see table above), it only > mentions the ipg_clk* and the mem_ram_CLK and in addition with the CLKSRC > documentation I think the mx6 doesn't even have a > (connected/functional) oscillator clock input to the flexcan core. As you mentioned the i.MX6 has two clocks, both obtained from same oscillator with different clock multipliers/dividers. One of the clocks (the higher freq) feeds the CHI block and the other (low frequency) feeds PE block. > > > 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". > > So you say "per" is essential and that one that's called "ipg" on the > mx6 should be called "osc". And that "osc" has to be less than "per" if "osc" is > used. And "osc" is optional if not used. > > Correct? As I explained above, In I.MX6 case both are essential, because it doesn't have CLKSRC implemented. But in LS1021A/LX2160A etc, yes the separate PE clock is non-essential (for linux driver), as we use CHI clock to feed the PE block (using CLKSRC) > > Looking at the mx6: > > ipg = 66 MHz > per = 30 MHz > > This means: > As "ipg" is 66 MHz and "per" is 30 MHz, "ipg" cannot be in use, as it's supposed > to be less than "per". And as it's optional if not used, I can switch it off in the > driver. As I explained above, you can't switch off any clock in i.MX6, as both are required. > > Let's see, I don't use the "ipg" clock any more, get a second reference to the > "per" clock instead: > > > diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c > > index d12b2f7d2834..c867a251d6c2 100644 > > --- a/drivers/net/can/flexcan.c > > +++ b/drivers/net/can/flexcan.c > > @@ -1501,7 +1501,7 @@ static int flexcan_probe(struct platform_device > *pdev) > > "clock-frequency", &clock_freq); > > > > if (!clock_freq) { > > - clk_ipg = devm_clk_get(&pdev->dev, "ipg"); > > + clk_ipg = devm_clk_get(&pdev->dev, "per"); > > if (IS_ERR(clk_ipg)) { > > dev_err(&pdev->dev, "no ipg clock defined\n"); > > return PTR_ERR(clk_ipg); > > Which results in: > > > [183288.488630] flexcan 2090000.flexcan: 2090000.flexcan supply > > xceiver not found, using dummy regulator [183288.513518] flexcan > > 2090000.flexcan: Linked as a consumer to regulator.0 [183288.521049] > > flexcan 2090000.flexcan: registering netdev failed [183288.535237] > > flexcan 2090000.flexcan: Dropping the link to regulator.0 > > [183288.546384] flexcan: probe of 2090000.flexcan failed with error > > -110 > > The driver doesn't load anymore. > > It's the chip_disable that fails: > > https://elixir.bootlin.com/linux/latest/source/drivers/net/can/flexcan.c#L1200 > > > 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/ls102 > > 1a.dtsi#L762 > > https://elixir.bootlin.com/linux/latest/source/arch/arm/boot/dts/imx35 > > .dtsi#L304 > > With my mx6 test it shows that we need both "per" and "ipg" clock on the imx6. Yes, we need both clocks in i.MX6. > And it seems to me, that "ipg" is something different than the "osc" clock you're > citing from the manual. As per above explanation, we have two clocks one for CHI and another for PE. I think their names should also be like this, so that we don't get confused by various terminologies used in different SOCs. > > Why provide the mx35 and the ls1021a (and the mx28[1] and mx25[2]) one > hardware clock as two different clocks to the driver? This way, we don't need > any special error handling for optional clocks in the driver. > > [1] > https://elixir.bootlin.com/linux/latest/source/arch/arm/boot/dts/imx28.dtsi#L1 > 043 > [2] > https://elixir.bootlin.com/linux/latest/source/arch/arm/boot/dts/imx25.dtsi#L1 > 18 I cannot speak for the i.MX35, i.MX28, i.MX25. But for LX2160A the CHI and PE clocks are different. The PE clock is connected to "sysclk". I will check for LS1021A, but I am sure that it would also be connected to "sysclk". The Layerscape series SOCs usually follow same design. > > >>> +- 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. > > Nope, see the mx6 example. > > >>> + - "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>" > > ok. > > 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 |
