On 2015-03-06 16:32, Bill Pringlemeir wrote: > On 6 Mar 2015, ste...@agner.ch wrote: > >> On 2015-03-06 07:15, Sascha Hauer wrote: >>> Hi Stefan, > >>> On Thu, Mar 05, 2015 at 12:10:20AM +0100, Stefan Agner wrote: >>>> + >>>> +static int vf610_nfc_probe_dt(struct device *dev, struct >>>> vf610_nfc_config *cfg) >>>> +{ >>>> + struct device_node *np = dev->of_node; >>>> + int buswidth; >>>> + u32 clkrate; >>>> + >>>> + if (!np) >>>> + return 1; >>>> + >>>> + cfg->flash_bbt = of_get_nand_on_flash_bbt(np); >>>> + >>>> + if (!of_property_read_u32(np, "clock-frequency", &clkrate)) >>>> + cfg->clkrate = clkrate; > >>> Normally the clock-frequency property tells the driver at which >>> frequency the device actually is running, not to tell the driver at >>> which frequency the device *should* run. It's strange to use the >>> value of the clock-frequency property as input to >>> clk_set_rate(). Maybe the assigned clock binding is more appropriate >>> here, see Documentation/devicetree/bindings/clock/clock-bindings.txt. > >> What we try to do here is to specify the hardware limitations. There >> seem to be some hardware restrictions when it comes to clock >> frequencies. There has been a rather long discussion over at >> Freescales community about it: >> https://community.freescale.com/thread/317074 > >> Not sure if this is the right way to specify the supported >> frequencies, or should we create a custom property for this, something >> like fsl,max-nfc-frequency = <33000000>? > > On most SOC's with this controller, the input clock to the controller > affects the NAND flash timing and other factors; so you will want to set > it based on the board/NAND stuffed. The clock is for synchronous logic > in the controller and affects many properties. > > I guess Sascha's point is, the board's DT should just have some > '&nfc_clk' node and not have this part of the driver? Either way works. > However, this clock is very important to get the driver to function. It > seem better for a user/porter to have this info in the node. I guess > you need to be trained to look at every node in the sub-tree for a > device. I think the other way might be better or a sub-system > maintainer. I looked at 'i2c' and other node which have a > 'clock-frequency' parameter. > > In the Documentation/devicetree/bindings/clock/clock-bindings.txt, > > uart@a000 { > compatible = "fsl,imx-uart"; > reg = <0xa000 0x1000>; > interrupts = <33>; > clocks = <&osc 0>, <&pll 1>; > clock-names = "baud", "register"; > }; > > Here, this uart clock may affect the maximum baud rate supported by the > device. For this controller (vf610_nfc), the clock is like setting the > 'baud rate'; it affect the NAND memory cycles. There is not really any > 'wait state' type logic in the controller register set that would allow > the driver to work with a 'given clock' rate. For certain a board > should set this clock for the NAND chips they wish to support. > > What would the board file look like to use clock node? > > [generic] > nfc: nand@400e0000 { > compatible = "fsl,vf610-nfc"; > reg = <0x400e0000 0x4000>; > clocks = <&clks VF610_CLK_NFC>; > clock-names = "nfc_clk"; > status = "disabled"; > }; > > [board] > > &nfc { > nand-bus-width = <16>; > nand-ecc-mode = "hw"; > nand-on-flash-bbt; > nand-ecc-strength = <24>; > nand-ecc-step-size = <2048>; > pinctrl-names = "default"; > pinctrl-0 = <&pinctrl_nfc_1>; > status = "okay"; > &nfc_clk { frequency = <33000000>} /* Like this? */
There is a property called "clock-frequency", but this is really for static clocks (e.g. external oscillators). I don't think this is the right approach. > }; > > I don't know how to do the 'Like this?' part. I don't think the > 'clock-bindings.txt' explains it. I see this is better as the the > driver needs no 'clock handling' code. It is definitely a little more > obtuse for the users. There are drivers which use the clock-frequency property to actually specify maximum operating frequencies: Documentation/devicetree/bindings/i2c/i2c-efm32.txt: - clock-frequency : maximal I2C bus clock frequency in Hz. Documentation/devicetree/bindings/media/samsung-fimc.txt:- clock-frequency: maximum FIMC local clock (LCLK) frequency; In the MMC subsystem there is a property called max-frequency, the SPI subsystem uses the property spi-max-frequency to specify maximum operating frequencies of SPI slaves. How about just max-frequency? Or with vendor prefix, e.g. fsl,max-frequency...? > > [snip] > >>> Does this driver work without device tree or not? Currently the >>> driver bails out when device tree support is enabled but no device >>> node is given. When device tree support is disabled in the kernel >>> though the driver happily continues here. > >> Hm, I never tried using this Driver without DT. > > [snip] > > I also didn't test this. The driver was ported from Linux versions > where DT did not exist. It is used in some OpenWRT/68k/coldfire > (patched) kernels and I wanted it to be useful for them. However, we > could probably remove the 'platform support'. Other people are using > this on PPC platforms and they will also have dt/of. > > Currently the platform control has no way to 'pass data', so the driver > only works with whatever defaults it has (or that is my belief). For > instance, those OpenWRT kernels have a 'machine file' which will set the > 'clock-frequency' and other parameters. We could remove the platform > support completely if it is misleading. I guess the KConfig would need > a 'depends on CONFIG_OF'. > > Thanks for the review, > Bill Pringlemeir. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/