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