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. >>> 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 "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? > 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 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. > 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? 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. 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/ls1021a.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. And it seems to me, that "ipg" is something different than the "osc" clock you're citing from the manual. 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#L1043 [2] https://elixir.bootlin.com/linux/latest/source/arch/arm/boot/dts/imx25.dtsi#L118 >>> +- 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 |
Attachment:
signature.asc
Description: OpenPGP digital signature
