| Message ID | 20200502100628.24809-17-pragnesh.patel@sifive.com |
|---|---|
| State | Superseded |
| Delegated to: | Andes |
| Headers | show |
| Series | RISC-V SiFive FU540 support SPL | expand |
On Sat, May 2, 2020 at 6:08 PM Pragnesh Patel <pragnesh.patel@sifive.com> wrote: > > The cpu clock is probably already enabled if we are executing code (though > we could be executing from a different core). This patch prevents the cpu > clock or its parents from being disabled. > > Signed-off-by: Sean Anderson <seanga2@gmail.com> > Signed-off-by: Pragnesh Patel <pragnesh.patel@sifive.com> > Reviewed-by: Bin Meng <bmeng.cn@gmail.com> > --- > drivers/cpu/riscv_cpu.c | 33 +++++++++++++++++++++++++++++++++ > 1 file changed, 33 insertions(+) > Tested-by: Bin Meng <bmeng.cn@gmail.com>
On 5/2/20 6:06 AM, Pragnesh Patel wrote: > The cpu clock is probably already enabled if we are executing code (though > we could be executing from a different core). This patch prevents the cpu > clock or its parents from being disabled. > > Signed-off-by: Sean Anderson <seanga2@gmail.com> If you make substantial changes can you please make a note of it in the commit? I did not sign off on *this* code. > Signed-off-by: Pragnesh Patel <pragnesh.patel@sifive.com> > Reviewed-by: Bin Meng <bmeng.cn@gmail.com> > --- > drivers/cpu/riscv_cpu.c | 33 +++++++++++++++++++++++++++++++++ > 1 file changed, 33 insertions(+) > > diff --git a/drivers/cpu/riscv_cpu.c b/drivers/cpu/riscv_cpu.c > index 28ad0aa30f..8ebe0341fd 100644 > --- a/drivers/cpu/riscv_cpu.c > +++ b/drivers/cpu/riscv_cpu.c > @@ -9,6 +9,7 @@ > #include <errno.h> > #include <dm/device-internal.h> > #include <dm/lists.h> > +#include <clk.h> > > DECLARE_GLOBAL_DATA_PTR; > > @@ -100,6 +101,37 @@ static int riscv_cpu_bind(struct udevice *dev) > return 0; > } > > +static int riscv_cpu_probe(struct udevice *dev) > +{ > + int ret = 0; > + u32 clock = 0; > + struct clk clk; > + > + /* Get a clock if it exists */ > + ret = clk_get_by_index(dev, 0, &clk); > + if (ret) > + return 0; > + > + ret = dev_read_u32(dev, "clock-frequency", &clock); Ok, so as I understand it, your goal for your changes this patch is to have a way to set the cpu frequency on startup. However, I think the cpu-frequency property is not the correct way to go about this when we have a clock from the device tree. In this case, one should use the assigned-clock* properties to have the frequency set automatically when clk_get_by_index is called. There is no need to add this functionality here. With the previous patch in the series you pulled this from [1], one could easily do something like cpus { assigned-clocks = <&foo FOO_CPU>; assigned-clock-frequency = <100000000>; cpu@0 { /* ... */ clocks = <&foo FOO_CPU>; /* ... */ }; }; which would use existing code to assign the frequency. [1] https://patchwork.ozlabs.org/project/uboot/patch/20200423023320.1380090-18-seanga2@gmail.com/ > + if (ret) { > + debug("clock-frequency not found in dt %d\n", ret); This should not be an error. You also need to check for ENOSYS and ENOTSUPP like below. > + return ret; > + } else { > + ret = clk_set_rate(&clk, clock); > + if (ret < 0) { > + debug("Could not set CPU clock\n"); Neither should this be. > + return ret; > + } > + } > + > + ret = clk_enable(&clk); > + clk_free(&clk); > + if (ret == -ENOSYS || ret == -ENOTSUPP) All clk_ calls can return ENOSYS and ENOTSUPP. These are returned when the underlying clock does not support the operation. However, they are not appropriate errors to return from this function. > + return 0; > + else > + return ret; > +} > + > static const struct cpu_ops riscv_cpu_ops = { > .get_desc = riscv_cpu_get_desc, > .get_info = riscv_cpu_get_info, > @@ -116,6 +148,7 @@ U_BOOT_DRIVER(riscv_cpu) = { > .id = UCLASS_CPU, > .of_match = riscv_cpu_ids, > .bind = riscv_cpu_bind, > + .probe = riscv_cpu_probe, > .ops = &riscv_cpu_ops, > .flags = DM_FLAG_PRE_RELOC, > }; > --Sean
Hi Sean, >-----Original Message----- >From: Sean Anderson <seanga2@gmail.com> >Sent: 02 May 2020 23:46 >To: Pragnesh Patel <pragnesh.patel@sifive.com>; u-boot@lists.denx.de >Cc: atish.patra@wdc.com; palmerdabbelt@google.com; >bmeng.cn@gmail.com; Paul Walmsley <paul.walmsley@sifive.com>; >jagan@amarulasolutions.com; Troy Benjegerdes ><troy.benjegerdes@sifive.com>; anup.patel@wdc.com; Sagar Kadam ><sagar.kadam@sifive.com>; rick@andestech.com; Lukas Auer ><lukas.auer@aisec.fraunhofer.de> >Subject: Re: [PATCH v7 16/22] riscv: Enable cpu clock if it is present > >[External Email] Do not click links or attachments unless you recognize the >sender and know the content is safe > >On 5/2/20 6:06 AM, Pragnesh Patel wrote: >> The cpu clock is probably already enabled if we are executing code >> (though we could be executing from a different core). This patch >> prevents the cpu clock or its parents from being disabled. >> >> Signed-off-by: Sean Anderson <seanga2@gmail.com> > >If you make substantial changes can you please make a note of it in the >commit? I did not sign off on *this* code. This patch is copied from your v9 series [1] and I made some changes, so the idea is to give credit to everyone who contributed. [1] https://patchwork.ozlabs.org/project/uboot/patch/20200503023550.326791-19-seanga2@gmail.com/ > >> Signed-off-by: Pragnesh Patel <pragnesh.patel@sifive.com> >> Reviewed-by: Bin Meng <bmeng.cn@gmail.com> >> --- >> drivers/cpu/riscv_cpu.c | 33 +++++++++++++++++++++++++++++++++ >> 1 file changed, 33 insertions(+) >> >> diff --git a/drivers/cpu/riscv_cpu.c b/drivers/cpu/riscv_cpu.c index >> 28ad0aa30f..8ebe0341fd 100644 >> --- a/drivers/cpu/riscv_cpu.c >> +++ b/drivers/cpu/riscv_cpu.c >> @@ -9,6 +9,7 @@ >> #include <errno.h> >> #include <dm/device-internal.h> >> #include <dm/lists.h> >> +#include <clk.h> >> >> DECLARE_GLOBAL_DATA_PTR; >> >> @@ -100,6 +101,37 @@ static int riscv_cpu_bind(struct udevice *dev) >> return 0; >> } >> >> +static int riscv_cpu_probe(struct udevice *dev) { >> + int ret = 0; >> + u32 clock = 0; >> + struct clk clk; >> + >> + /* Get a clock if it exists */ >> + ret = clk_get_by_index(dev, 0, &clk); >> + if (ret) >> + return 0; >> + >> + ret = dev_read_u32(dev, "clock-frequency", &clock); > >Ok, so as I understand it, your goal for your changes this patch is to have a >way to set the cpu frequency on startup. However, I think the cpu-frequency >property is not the correct way to go about this when we have a clock from >the device tree. In this case, one should use the >assigned-clock* properties to have the frequency set automatically when >clk_get_by_index is called. There is no need to add this functionality here. > >With the previous patch in the series you pulled this from [1], one could easily >do something like > >cpus { > assigned-clocks = <&foo FOO_CPU>; > assigned-clock-frequency = <100000000>; > cpu@0 { > /* ... */ > clocks = <&foo FOO_CPU>; > /* ... */ > }; >}; > >which would use existing code to assign the frequency. > >[1] >https://patchwork.ozlabs.org/project/uboot/patch/20200423023320.1380090 >-18-seanga2@gmail.com/ Thanks for pointing this out to me, I will give it a try with your patch [1] and drop this patch if not necessary. [1] https://patchwork.ozlabs.org/project/uboot/patch/20200503023550.326791-19-seanga2@gmail.com/ > >> + if (ret) { >> + debug("clock-frequency not found in dt %d\n", ret); > >This should not be an error. You also need to check for ENOSYS and >ENOTSUPP like below. Thanks for the review, will take care in future. > >> + return ret; >> + } else { >> + ret = clk_set_rate(&clk, clock); >> + if (ret < 0) { >> + debug("Could not set CPU clock\n"); > >Neither should this be. will take care in future. > >> + return ret; >> + } >> + } >> + >> + ret = clk_enable(&clk); >> + clk_free(&clk); >> + if (ret == -ENOSYS || ret == -ENOTSUPP) > >All clk_ calls can return ENOSYS and ENOTSUPP. These are returned when the >underlying clock does not support the operation. However, they are not >appropriate errors to return from this function. Thanks for the explanation, will take care in future. > >> + return 0; >> + else >> + return ret; >> +} >> + >> static const struct cpu_ops riscv_cpu_ops = { >> .get_desc = riscv_cpu_get_desc, >> .get_info = riscv_cpu_get_info, >> @@ -116,6 +148,7 @@ U_BOOT_DRIVER(riscv_cpu) = { >> .id = UCLASS_CPU, >> .of_match = riscv_cpu_ids, >> .bind = riscv_cpu_bind, >> + .probe = riscv_cpu_probe, >> .ops = &riscv_cpu_ops, >> .flags = DM_FLAG_PRE_RELOC, >> }; >> > >--Sean
On 5/3/20 3:12 AM, Pragnesh Patel wrote: > Hi Sean, > >> -----Original Message----- >> From: Sean Anderson <seanga2@gmail.com> >> Sent: 02 May 2020 23:46 >> To: Pragnesh Patel <pragnesh.patel@sifive.com>; u-boot@lists.denx.de >> Cc: atish.patra@wdc.com; palmerdabbelt@google.com; >> bmeng.cn@gmail.com; Paul Walmsley <paul.walmsley@sifive.com>; >> jagan@amarulasolutions.com; Troy Benjegerdes >> <troy.benjegerdes@sifive.com>; anup.patel@wdc.com; Sagar Kadam >> <sagar.kadam@sifive.com>; rick@andestech.com; Lukas Auer >> <lukas.auer@aisec.fraunhofer.de> >> Subject: Re: [PATCH v7 16/22] riscv: Enable cpu clock if it is present >> >> [External Email] Do not click links or attachments unless you recognize the >> sender and know the content is safe >> >> On 5/2/20 6:06 AM, Pragnesh Patel wrote: >>> The cpu clock is probably already enabled if we are executing code >>> (though we could be executing from a different core). This patch >>> prevents the cpu clock or its parents from being disabled. >>> >>> Signed-off-by: Sean Anderson <seanga2@gmail.com> >> >> If you make substantial changes can you please make a note of it in the >> commit? I did not sign off on *this* code. > > This patch is copied from your v9 series [1] and I made some changes, so the idea is to > give credit to everyone who contributed. That's fine, just please make a note when you make substantial changes. For example, you could write Signed-off-by: Sean Anderson <seanga2@gmail.com> Reviewed-by: Bin Meng <bmeng.cn@gmail.com> [set clock frequency from clock-frequency property] Signed-off-by: Pragnesh Patel <pragnesh.patel@sifive.com> so it's more clear what changes have been made since this patch was posted last. --Sean
Hi Sean, >-----Original Message----- >From: Sean Anderson <seanga2@gmail.com> >Sent: 03 May 2020 22:47 >To: Pragnesh Patel <pragnesh.patel@sifive.com>; u-boot@lists.denx.de >Cc: atish.patra@wdc.com; palmerdabbelt@google.com; >bmeng.cn@gmail.com; Paul Walmsley <paul.walmsley@sifive.com>; >jagan@amarulasolutions.com; Troy Benjegerdes ><troy.benjegerdes@sifive.com>; anup.patel@wdc.com; Sagar Kadam ><sagar.kadam@sifive.com>; rick@andestech.com; Lukas Auer ><lukas.auer@aisec.fraunhofer.de> >Subject: Re: [PATCH v7 16/22] riscv: Enable cpu clock if it is present > >[External Email] Do not click links or attachments unless you recognize the >sender and know the content is safe > >On 5/3/20 3:12 AM, Pragnesh Patel wrote: >> Hi Sean, >> >>> -----Original Message----- >>> From: Sean Anderson <seanga2@gmail.com> >>> Sent: 02 May 2020 23:46 >>> To: Pragnesh Patel <pragnesh.patel@sifive.com>; u-boot@lists.denx.de >>> Cc: atish.patra@wdc.com; palmerdabbelt@google.com; >>> bmeng.cn@gmail.com; Paul Walmsley <paul.walmsley@sifive.com>; >>> jagan@amarulasolutions.com; Troy Benjegerdes >>> <troy.benjegerdes@sifive.com>; anup.patel@wdc.com; Sagar Kadam >>> <sagar.kadam@sifive.com>; rick@andestech.com; Lukas Auer >>> <lukas.auer@aisec.fraunhofer.de> >>> Subject: Re: [PATCH v7 16/22] riscv: Enable cpu clock if it is >>> present >>> >>> [External Email] Do not click links or attachments unless you >>> recognize the sender and know the content is safe >>> >>> On 5/2/20 6:06 AM, Pragnesh Patel wrote: >>>> The cpu clock is probably already enabled if we are executing code >>>> (though we could be executing from a different core). This patch >>>> prevents the cpu clock or its parents from being disabled. >>>> >>>> Signed-off-by: Sean Anderson <seanga2@gmail.com> >>> >>> If you make substantial changes can you please make a note of it in >>> the commit? I did not sign off on *this* code. >> >> This patch is copied from your v9 series [1] and I made some changes, >> so the idea is to give credit to everyone who contributed. > >That's fine, just please make a note when you make substantial changes. >For example, you could write > >Signed-off-by: Sean Anderson <seanga2@gmail.com> >Reviewed-by: Bin Meng <bmeng.cn@gmail.com> [set clock frequency from >clock-frequency property] >Signed-off-by: Pragnesh Patel <pragnesh.patel@sifive.com> > >so it's more clear what changes have been made since this patch was posted >last. Will take care in future. > >--Sean
diff --git a/drivers/cpu/riscv_cpu.c b/drivers/cpu/riscv_cpu.c index 28ad0aa30f..8ebe0341fd 100644 --- a/drivers/cpu/riscv_cpu.c +++ b/drivers/cpu/riscv_cpu.c @@ -9,6 +9,7 @@ #include <errno.h> #include <dm/device-internal.h> #include <dm/lists.h> +#include <clk.h> DECLARE_GLOBAL_DATA_PTR; @@ -100,6 +101,37 @@ static int riscv_cpu_bind(struct udevice *dev) return 0; } +static int riscv_cpu_probe(struct udevice *dev) +{ + int ret = 0; + u32 clock = 0; + struct clk clk; + + /* Get a clock if it exists */ + ret = clk_get_by_index(dev, 0, &clk); + if (ret) + return 0; + + ret = dev_read_u32(dev, "clock-frequency", &clock); + if (ret) { + debug("clock-frequency not found in dt %d\n", ret); + return ret; + } else { + ret = clk_set_rate(&clk, clock); + if (ret < 0) { + debug("Could not set CPU clock\n"); + return ret; + } + } + + ret = clk_enable(&clk); + clk_free(&clk); + if (ret == -ENOSYS || ret == -ENOTSUPP) + return 0; + else + return ret; +} + static const struct cpu_ops riscv_cpu_ops = { .get_desc = riscv_cpu_get_desc, .get_info = riscv_cpu_get_info, @@ -116,6 +148,7 @@ U_BOOT_DRIVER(riscv_cpu) = { .id = UCLASS_CPU, .of_match = riscv_cpu_ids, .bind = riscv_cpu_bind, + .probe = riscv_cpu_probe, .ops = &riscv_cpu_ops, .flags = DM_FLAG_PRE_RELOC, };