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

[Atish Patra <atish.patra@wdc.com>]

On 4/29/19 11:20 PM, Paul Walmsley wrote:
> 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@sifive.com/
> 
>> 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.
> 

There were some concerns about the breaking the existing setup in the past.

> 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.
> 

Personally, it makes sense to me. I am okay with upgrading FSBL to 
update the DT once the patches are in mainline. In fact, I used to do 
that for topology patch series. This will help to add any new DT entry 
in future as well.

However, if SiFive can share a prebuilt FSBL image for everybody to 
upgrade, that would be very helpful.

Regards,
Atish
> 
> - Paul
>