| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <1a689627-851b-4929-aaf8-3dcddb57af6b@gmail.com>
Date: Wed, 28 Aug 2024 01:14:04 -0700
From: Bo Gan <ganboing@...il.com>
To: Conor Dooley <conor.dooley@...rochip.com>
Cc: linux-clk@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-riscv@...ts.infradead.org, Pragnesh.patel@...ive.com,
aou@...s.berkeley.edu, erik.danie@...ive.com, hes@...ive.com,
mturquette@...libre.com, palmer@...belt.com, paul.walmsley@...ive.com,
sboyd@...nel.org, schwab@...ux-m68k.org, zong.li@...ive.com
Subject: Re: [PATCH 3/3] clk: sifive: prci: Add release_reset hooks for
gemgxlpll/cltxpll
Hi Conor,
Thanks for replying so quickly. See inline.
On 8/28/24 00:58, Conor Dooley wrote:
> On Tue, Aug 27, 2024 at 11:55:20PM -0700, Bo Gan wrote:
>> This patch adds the release_reset hook interface to __prci_wrpll_data.
>> During clock enablement, the function (if present) will be called after PLL
>> registers are configured. It aligns the logic to the driver in u-boot. When
>> there's a previous bootloader stage, such as u-boot, it usually enables the
>> gemgxlpll clock when trying to PXE/network boot. The kernel boots fine, but
>> we should not depend on it being our previous stage, and the logic within:
>>
>> a. We (linux) can get directly invoked by firmware (OpenSBI).
>> b. U-boot doesn't necessarily have to initialize ethernet and enable the
>> clock (when not enabled in CONFIG).
>>
>> When the kernel is the first to initialize gemgxlpll, it must also release
>> the corresponding reset. Otherwise the chip will just hang during macb
>> initialization, and even external JTAG debugger will lose control over the
>> risc-v debug module. (Observed with my Sifive Unmatched Rev.B board)
>>
>> The patch took the dt-bindings and logics directly from u-boot with some
>> additional modifications:
>> - Use __prci_writel after __prci_readl to have barrier semantic. U-boot
>> has the strong version of readl/writel, but linux has the relaxed ones.
>> - Use pd->reset.rcdev.ops to access the reset regs.
>> - Split reset bindings for FU540/FU740 and use them directly, instead of
>> looking it up through reset-names.
>
> The macb driver already supports using a reset at boot time (see zynq and
> mpfs) if hooked up in the devicetree, why doesn't that work for you in
> this situation?
>
> Thanks,
> Conor.
>
That a good idea. I never tried it. It's probably a cleaner solution, and I'll
try it some later time. I used the same logic in u-boot partially because the
way how sifive people coded this up. Specifically, PROCMONCFG is set between
the releases of 2 resets. I assume there's some dependency between those regs.
If reset is triggered from the macb driver side, there's really no proper way
to set PROCMONCFG. Also from the FU740 manual, I can't find any mentioning
about PROCMONCFG. If it's not needed, surely it's better to just let macb do
the resetting. Perhaps I should wait for Sifive folks to fill in the gap.
Thanks!
Bo
<Removed Yash Shah and Pragnesh Patel. Looks like they left Sifive>
>>
>> Signed-off-by: Bo Gan <ganboing@...il.com>
>> ---
>> drivers/clk/sifive/fu540-prci.h | 16 ++++++++++++++++
>> drivers/clk/sifive/fu740-prci.h | 31 +++++++++++++++++++++++++++++++
>> drivers/clk/sifive/sifive-prci.c | 23 +++++++++++++++++++++++
>> drivers/clk/sifive/sifive-prci.h | 8 ++++++++
>> 4 files changed, 78 insertions(+)
>>
>> diff --git a/drivers/clk/sifive/fu540-prci.h b/drivers/clk/sifive/fu540-prci.h
>> index e0173324f3c5..9d2ca18f47a4 100644
>> --- a/drivers/clk/sifive/fu540-prci.h
>> +++ b/drivers/clk/sifive/fu540-prci.h
>> @@ -23,9 +23,24 @@
>> #include <linux/module.h>
>>
>> #include <dt-bindings/clock/sifive-fu540-prci.h>
>> +#include <dt-bindings/reset/sifive-fu540-prci.h>
>>
>> #include "sifive-prci.h"
>>
>> +/**
>> + * sifive_fu540_prci_ethernet_release_reset() - Release ethernet reset
>> + * @pd: struct __prci_data * for the PRCI containing the Ethernet CLK mux reg
>> + *
>> + */
>> +static void sifive_fu540_prci_ethernet_release_reset(struct __prci_data *pd)
>> +{
>> + /* Release GEMGXL reset */
>> + pd->reset.rcdev.ops->deassert(&pd->reset.rcdev, FU540_PRCI_RST_GEMGXL_N);
>> +
>> + /* Procmon => core clock */
>> + sifive_prci_set_procmoncfg(pd, PRCI_PROCMONCFG_CORE_CLOCK_MASK);
>> +}
>> +
>> /* PRCI integration data for each WRPLL instance */
>>
>> static struct __prci_wrpll_data sifive_fu540_prci_corepll_data = {
>> @@ -43,6 +58,7 @@ static struct __prci_wrpll_data sifive_fu540_prci_ddrpll_data = {
>> static struct __prci_wrpll_data sifive_fu540_prci_gemgxlpll_data = {
>> .cfg0_offs = PRCI_GEMGXLPLLCFG0_OFFSET,
>> .cfg1_offs = PRCI_GEMGXLPLLCFG1_OFFSET,
>> + .release_reset = sifive_fu540_prci_ethernet_release_reset,
>> };
>>
>> /* Linux clock framework integration */
>> diff --git a/drivers/clk/sifive/fu740-prci.h b/drivers/clk/sifive/fu740-prci.h
>> index f31cd30fc395..dd0f54277a99 100644
>> --- a/drivers/clk/sifive/fu740-prci.h
>> +++ b/drivers/clk/sifive/fu740-prci.h
>> @@ -10,9 +10,38 @@
>> #include <linux/module.h>
>>
>> #include <dt-bindings/clock/sifive-fu740-prci.h>
>> +#include <dt-bindings/reset/sifive-fu740-prci.h>
>>
>> #include "sifive-prci.h"
>>
>> +/**
>> + * sifive_fu740_prci_ethernet_release_reset() - Release ethernet reset
>> + * @pd: struct __prci_data * for the PRCI containing the Ethernet CLK mux reg
>> + *
>> + */
>> +static void sifive_fu740_prci_ethernet_release_reset(struct __prci_data *pd)
>> +{
>> + /* Release GEMGXL reset */
>> + pd->reset.rcdev.ops->deassert(&pd->reset.rcdev, FU740_PRCI_RST_GEMGXL_N);
>> +
>> + /* Procmon => core clock */
>> + sifive_prci_set_procmoncfg(pd, PRCI_PROCMONCFG_CORE_CLOCK_MASK);
>> +
>> + /* Release Chiplink reset */
>> + pd->reset.rcdev.ops->deassert(&pd->reset.rcdev, FU740_PRCI_RST_CLTX_N);
>> +}
>> +
>> +/**
>> + * sifive_fu740_prci_cltx_release_reset() - Release cltx reset
>> + * @pd: struct __prci_data * for the PRCI containing the Ethernet CLK mux reg
>> + *
>> + */
>> +static void sifive_fu740_prci_cltx_release_reset(struct __prci_data *pd)
>> +{
>> + /* Release CLTX reset */
>> + pd->reset.rcdev.ops->deassert(&pd->reset.rcdev, FU740_PRCI_RST_CLTX_N);
>> +}
>> +
>> /* PRCI integration data for each WRPLL instance */
>>
>> static struct __prci_wrpll_data sifive_fu740_prci_corepll_data = {
>> @@ -30,6 +59,7 @@ static struct __prci_wrpll_data sifive_fu740_prci_ddrpll_data = {
>> static struct __prci_wrpll_data sifive_fu740_prci_gemgxlpll_data = {
>> .cfg0_offs = PRCI_GEMGXLPLLCFG0_OFFSET,
>> .cfg1_offs = PRCI_GEMGXLPLLCFG1_OFFSET,
>> + .release_reset = sifive_fu740_prci_ethernet_release_reset,
>> };
>>
>> static struct __prci_wrpll_data sifive_fu740_prci_dvfscorepll_data = {
>> @@ -49,6 +79,7 @@ static struct __prci_wrpll_data sifive_fu740_prci_hfpclkpll_data = {
>> static struct __prci_wrpll_data sifive_fu740_prci_cltxpll_data = {
>> .cfg0_offs = PRCI_CLTXPLLCFG0_OFFSET,
>> .cfg1_offs = PRCI_CLTXPLLCFG1_OFFSET,
>> + .release_reset = sifive_fu740_prci_cltx_release_reset,
>> };
>>
>> /* Linux clock framework integration */
>> diff --git a/drivers/clk/sifive/sifive-prci.c b/drivers/clk/sifive/sifive-prci.c
>> index caba0400f8a2..ae8055a84466 100644
>> --- a/drivers/clk/sifive/sifive-prci.c
>> +++ b/drivers/clk/sifive/sifive-prci.c
>> @@ -249,6 +249,9 @@ int sifive_prci_clock_enable(struct clk_hw *hw)
>> if (pwd->disable_bypass)
>> pwd->disable_bypass(pd);
>>
>> + if (pwd->release_reset)
>> + pwd->release_reset(pd);
>> +
>> return 0;
>> }
>>
>> @@ -448,6 +451,26 @@ void sifive_prci_hfpclkpllsel_use_hfpclkpll(struct __prci_data *pd)
>> r = __prci_readl(pd, PRCI_HFPCLKPLLSEL_OFFSET); /* barrier */
>> }
>>
>> +/*
>> + * PROCMONCFG
>> + */
>> +
>> +/**
>> + * sifive_prci_set_procmoncfg() - set PROCMONCFG
>> + * @pd: struct __prci_data * PRCI context
>> + * @val: u32 value to write to PROCMONCFG register
>> + *
>> + * Set the PROCMONCFG register to @val
>> + *
>> + * Context: Any context. Caller must prevent concurrent changes to the
>> + * PROCMONCFG_OFFSET register.
>> + */
>> +void sifive_prci_set_procmoncfg(struct __prci_data *pd, u32 val)
>> +{
>> + __prci_writel(val, PRCI_PROCMONCFG_OFFSET, pd);
>> + __prci_readl(pd, PRCI_PROCMONCFG_OFFSET); /* barrier */
>> +}
>> +
>> /* PCIE AUX clock APIs for enable, disable. */
>> int sifive_prci_pcie_aux_clock_is_enabled(struct clk_hw *hw)
>> {
>> diff --git a/drivers/clk/sifive/sifive-prci.h b/drivers/clk/sifive/sifive-prci.h
>> index 91658a88af4e..825a0aef9fd5 100644
>> --- a/drivers/clk/sifive/sifive-prci.h
>> +++ b/drivers/clk/sifive/sifive-prci.h
>> @@ -210,6 +210,9 @@
>>
>> /* PROCMONCFG */
>> #define PRCI_PROCMONCFG_OFFSET 0xf0
>> +#define PRCI_PROCMONCFG_CORE_CLOCK_SHIFT 24
>> +#define PRCI_PROCMONCFG_CORE_CLOCK_MASK \
>> + (0x1 << PRCI_PROCMONCFG_CORE_CLOCK_SHIFT)
>>
>> /*
>> * Private structures
>> @@ -235,6 +238,7 @@ struct __prci_data {
>> * @disable_bypass: fn ptr to code to not bypass the WRPLL (or NULL)
>> * @cfg0_offs: WRPLL CFG0 register offset (in bytes) from the PRCI base address
>> * @cfg1_offs: WRPLL CFG1 register offset (in bytes) from the PRCI base address
>> + * @release_reset: fn ptr to code to release clock reset
>> *
>> * @enable_bypass and @disable_bypass are used for WRPLL instances
>> * that contain a separate external glitchless clock mux downstream
>> @@ -246,6 +250,7 @@ struct __prci_wrpll_data {
>> void (*disable_bypass)(struct __prci_data *pd);
>> u8 cfg0_offs;
>> u8 cfg1_offs;
>> + void (*release_reset)(struct __prci_data *pd);
>> };
>>
>> /**
>> @@ -290,6 +295,9 @@ void sifive_prci_corepllsel_use_corepll(struct __prci_data *pd);
>> void sifive_prci_hfpclkpllsel_use_hfclk(struct __prci_data *pd);
>> void sifive_prci_hfpclkpllsel_use_hfpclkpll(struct __prci_data *pd);
>>
>> +/* PROCMONCFG */
>> +void sifive_prci_set_procmoncfg(struct __prci_data *pd, u32 val);
>> +
>> /* Linux clock framework integration */
>> long sifive_prci_wrpll_round_rate(struct clk_hw *hw, unsigned long rate,
>> unsigned long *parent_rate);
>> --
>> 2.34.1
>>
Powered by blists - more mailing lists