| Message ID | 1413838448-29464-5-git-send-email-cernekee@gmail.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Monday 20 October 2014 13:54:03 Kevin Cernekee wrote: > +- clock-names: The appropriate output name in the referenced clock node. > + > + uart0: serial@14e00520 { > + compatible = "brcm,bcm6345-uart"; > + reg = <0x14e00520 0x18>; > + interrupt-parent = <&periph_intc>; > + interrupts = <2>; > + clocks = <&periph_clk>; > + clock-names = "periph"; > + }; > + > + clocks { > + periph_clk: periph_clk@0 { > + compatible = "fixed-clock"; > + #clock-cells = <0>; > + clock-frequency = <54000000>; > + clock-output-names = "periph"; > + }; > + }; In this example, the clock output name of the clock provider is the same as the clock input of the consumer, that is almost always a bug and would not be a good example at all. I assume the output name is correct and the input is not. If you have access to the HDL source of the bcm6345-uart, please check if the input has a proper name, otherwise just call it "uart" or remove the clock-names property completely. In the documentation enough, you must document the specific name of the clock that is supposed to be used by the uart driver. Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday 20 October 2014 23:20:08 Arnd Bergmann wrote: > > In this example, the clock output name of the clock provider is > the same as the clock input of the consumer, that is almost always > a bug and would not be a good example at all. > > Ah, found the bug: the MIPS code is written to ignore the device and just look up a global clock name: struct clk *clk_get(struct device *dev, const char *id) { if (!strcmp(id, "enet0")) return &clk_enet0; if (!strcmp(id, "enet1")) return &clk_enet1; if (!strcmp(id, "enetsw")) return &clk_enetsw; if (!strcmp(id, "ephy")) return &clk_ephy; if (!strcmp(id, "usbh")) return &clk_usbh; if (!strcmp(id, "usbd")) return &clk_usbd; if (!strcmp(id, "spi")) return &clk_spi; if (!strcmp(id, "hsspi")) return &clk_hsspi; if (!strcmp(id, "xtm")) return &clk_xtm; if (!strcmp(id, "periph")) return &clk_periph; if ((BCMCPU_IS_3368() || BCMCPU_IS_6358()) && !strcmp(id, "pcm")) return &clk_pcm; if ((BCMCPU_IS_6362() || BCMCPU_IS_6368()) && !strcmp(id, "ipsec")) return &clk_ipsec; if ((BCMCPU_IS_6328() || BCMCPU_IS_6362()) && !strcmp(id, "pcie")) return &clk_pcie; return ERR_PTR(-ENOENT); } This should be changed to use the drivers/clk/clkdev.c lookup code if you want to share drivers between architectures. In particular, the "enet0"/"enet1" clock name makes no sense -- the clock input name should be independent of the instance, aside from the question of which output of the provider it is wired up to. Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday 20 October 2014 15:53:53 Florian Fainelli wrote: > > For now, I suppose that s simple fix could be to use an anonymous clock > request when probed via DT. This code you quote dates from 2008 when > there was no clkdev in the kernel at all. So something like this would > probably do it for now: > > diff --git a/drivers/tty/serial/bcm63xx_uart.c > b/drivers/tty/serial/bcm63xx_uart.c > index e0b87d507670..1b914b85dd31 100644 > --- a/drivers/tty/serial/bcm63xx_uart.c > +++ b/drivers/tty/serial/bcm63xx_uart.c > @@ -819,7 +819,7 @@ static int bcm_uart_probe(struct platform_device *pdev) > if (!res_irq) > return -ENODEV; > > - clk = clk_get(&pdev->dev, "periph"); > + clk = clk_get(&pdev->dev, pdev->dev.of_node ? NULL : "periph"); > if (IS_ERR(clk)) > return -ENODEV; > > Yes, that would work. Just make sure the same bug doesn't creep in for other drivers you are converting. Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/serial/bcm63xx-uart.txt b/Documentation/devicetree/bindings/serial/bcm63xx-uart.txt new file mode 100644 index 0000000..b01c76a --- /dev/null +++ b/Documentation/devicetree/bindings/serial/bcm63xx-uart.txt @@ -0,0 +1,34 @@ +* BCM63xx UART + +Required properties: + +- compatible: "brcm,bcm6345-uart" + +- reg: The base address of the UART register bank. + +- interrupts: A single interrupt specifier. + +- clocks: Clock driving the hardware; used to figure out the baud rate + divisor. + +- clock-names: The appropriate output name in the referenced clock node. + +Example: + + uart0: serial@14e00520 { + compatible = "brcm,bcm6345-uart"; + reg = <0x14e00520 0x18>; + interrupt-parent = <&periph_intc>; + interrupts = <2>; + clocks = <&periph_clk>; + clock-names = "periph"; + }; + + clocks { + periph_clk: periph_clk@0 { + compatible = "fixed-clock"; + #clock-cells = <0>; + clock-frequency = <54000000>; + clock-output-names = "periph"; + }; + };
This squashes a checkpatch warning on my new bcm3384 dts submission. Signed-off-by: Kevin Cernekee <cernekee@gmail.com> --- .../devicetree/bindings/serial/bcm63xx-uart.txt | 34 ++++++++++++++++++++++ 1 file changed, 34 insertions(+) create mode 100644 Documentation/devicetree/bindings/serial/bcm63xx-uart.txt