Re: [PATCH v3 3/3] clk: sifive: add a driver for the SiFive FU540 PRCI IP block

[Paul Walmsley]

Hi Atish,

On Sat, 27 Apr 2019, Atish Patra wrote:

> On 4/11/19 1:28 AM, Paul Walmsley wrote:
> > Add driver code for the SiFive FU540 PRCI IP block.  This IP block
> > handles reset and clock control for the SiFive FU540 device and
> > implements SoC-level clock tree controls and dividers.

[...]

> > +static const struct of_device_id sifive_fu540_prci_of_match[] = {
> > +	{ .compatible = "sifive,fu540-c000-prci", },
> 
> All the existing unleashed devices have prci clock compatible string as
> "sifive,aloeprci0" or "sifive,ux00prci0". Should it be added to maintain
> backward compatibility?

As you note, just adding the old (unreviewed) compatible string isn't 
enough.

> Even after adding the compatible string (just for my testing purpose), I get
> this while booting.
> 
> [    0.104571] sifive-fu540-prci 10000000.prci: expected only two parent
> clocks, found 1
> [    0.112460] sifive-fu540-prci 10000000.prci: could not register clocks: -22
> [    0.119499] sifive-fu540-prci: probe of 10000000.prci failed with error -22
> 
> Looking at the DT entries, your DT patch has
> 
> +		prci: clock-controller@10000000 {
> +			compatible = "sifive,fu540-c000-prci";
> +			reg = <0x0 0x10000000 0x0 0x1000>;
> +			clocks = <&hfclk>, <&rtcclk>;
> +			#clock-cells = <1>;
> +		};
> 
> 
> while current DT from FSBL
> (https://github.com/sifive/freedom-u540-c000-bootloader/blob/master/fsbl/ux00_fsbl.dts)
> 
> prci: prci@10000000 {
> 			compatible = "sifive,aloeprci0", "sifive,ux00prci0";
> 			reg = <0x0 0x10000000 0x0 0x1000>;
> 			reg-names = "control";
> 			clocks = <&refclk>;
> 			#clock-cells = <1>;
> 		};
> 
> This seems to be the cause of error. It looks like this patch needs a complete
> different DT (your DT patch) than FSBL provides.

That's right.  That old data was completely out of tree and unreviewed.  
It's part of the reason why we're going through the process of posting DT 
data to the kernel and devicetree lists and getting that data reviewed:

https://lore.kernel.org/linux-riscv/20190411084242.4999-1-paul.walmsley@xxxxxxxxxx/

> This means everybody must upgrade the FSBL to use your DT patch in their
> boards once this driver is merged. Is this okay?

People can continue to use the out-of-tree DT data if they want.  They'll 
just have to continue to patch their kernels to add out-of-tree drivers, 
as they do now.

Otherwise, if people want to use the upstream PRCI driver in the upstream 
kernel, then it's necessary to use DT data that aligns with what's in the 
upstream binding documentation.


- Paul