lkml.org 
[lkml]   [2019]   [Apr]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v3 3/3] clk: sifive: add a driver for the SiFive FU540 PRCI IP block
From
Date
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
>

\
 
 \ /
  Last update: 2019-04-30 09:02    [W:0.058 / U:7.496 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site