| Message ID | 20220408131352.3421559-1-conor.dooley@microchip.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1] clk: microchip: mpfs: don't reset disabled peripherals | expand |
On Fri, Apr 08, 2022 at 01:13:53PM +0000, Conor Dooley wrote: > The current clock driver for PolarFire SoC puts the hardware behind > "periph" clocks into reset if their clock is disabled. CONFIG_PM was > recently added to the riscv defconfig and exposed issues caused by this > behaviour, where the Cadence GEM was being put into reset between its > bringup & the PHY bringup: > > https://lore.kernel.org/linux-riscv/9f4b057d-1985-5fd3-65c0-f944161c7792@microchip.com/ > > Fix this by removing the reset enable/disable code from the driver & > rely (for now) on the bootloader bringing peripherals out of reset > during boot. Maybe you should keep the clock enable -> disable reset part, and only remove the clock disable -> assert reset part. You are making the assumption that the bootloader disables reset on everything, when in fact it could only disable resets on peripherals it needs, and it needs the ethernet for TPTP boot. What is your long term fix? It seems like you need to add a reset controller. The macb already seems to support that: macb_main.c: /* Fully reset GEM controller at hardware level using zynqmp-reset driver, macb_main.c: ret = device_reset_optional(&pdev->dev); So once you have it, it should be easy to wire up for this peripheral. Once you have them all using the reset controller, you can then remove all the reset code from the clock driver. Andrew
On 08/04/2022 13:56, Andrew Lunn wrote: > On Fri, Apr 08, 2022 at 01:13:53PM +0000, Conor Dooley wrote: >> The current clock driver for PolarFire SoC puts the hardware behind >> "periph" clocks into reset if their clock is disabled. CONFIG_PM was >> recently added to the riscv defconfig and exposed issues caused by this >> behaviour, where the Cadence GEM was being put into reset between its >> bringup & the PHY bringup: >> >> https://lore.kernel.org/linux-riscv/9f4b057d-1985-5fd3-65c0-f944161c7792@microchip.com/ >> >> Fix this by removing the reset enable/disable code from the driver & >> rely (for now) on the bootloader bringing peripherals out of reset >> during boot. > > Maybe you should keep the clock enable -> disable reset part, and only > remove the clock disable -> assert reset part. You are making the > assumption that the bootloader disables reset on everything, when in > fact it could only disable resets on peripherals it needs, and it > needs the ethernet for TPTP boot. Yeah, I made that assumption because everything supported by mainline currently are taken out of reset by the first stage bootloader. I will double check & make sure that this is extended to all peripherals. > What is your long term fix? It seems like you need to add a reset > controller. The macb already seems to support that: > > macb_main.c: /* Fully reset GEM controller at hardware level using zynqmp-reset driver, > macb_main.c: ret = device_reset_optional(&pdev->dev); > > So once you have it, it should be easy to wire up for this > peripheral. Once you have them all using the reset controller, you can > then remove all the reset code from the clock driver. Yeah, adding a reset controller is the plan. Thanks, Conor.
diff --git a/drivers/clk/microchip/clk-mpfs.c b/drivers/clk/microchip/clk-mpfs.c index aa1561b773d6..4ddc7e7c9766 100644 --- a/drivers/clk/microchip/clk-mpfs.c +++ b/drivers/clk/microchip/clk-mpfs.c @@ -13,7 +13,6 @@ /* address offset of control registers */ #define REG_CLOCK_CONFIG_CR 0x08u #define REG_SUBBLK_CLOCK_CR 0x84u -#define REG_SUBBLK_RESET_CR 0x88u struct mpfs_clock_data { void __iomem *base; @@ -177,10 +176,6 @@ static int mpfs_periph_clk_enable(struct clk_hw *hw) spin_lock_irqsave(&mpfs_clk_lock, flags); - reg = readl_relaxed(base_addr + REG_SUBBLK_RESET_CR); - val = reg & ~(1u << periph->shift); - writel_relaxed(val, base_addr + REG_SUBBLK_RESET_CR); - reg = readl_relaxed(base_addr + REG_SUBBLK_CLOCK_CR); val = reg | (1u << periph->shift); writel_relaxed(val, base_addr + REG_SUBBLK_CLOCK_CR); @@ -200,10 +195,6 @@ static void mpfs_periph_clk_disable(struct clk_hw *hw) spin_lock_irqsave(&mpfs_clk_lock, flags); - reg = readl_relaxed(base_addr + REG_SUBBLK_RESET_CR); - val = reg | (1u << periph->shift); - writel_relaxed(val, base_addr + REG_SUBBLK_RESET_CR); - reg = readl_relaxed(base_addr + REG_SUBBLK_CLOCK_CR); val = reg & ~(1u << periph->shift); writel_relaxed(val, base_addr + REG_SUBBLK_CLOCK_CR); @@ -218,12 +209,9 @@ static int mpfs_periph_clk_is_enabled(struct clk_hw *hw) void __iomem *base_addr = periph_hw->sys_base; u32 reg; - reg = readl_relaxed(base_addr + REG_SUBBLK_RESET_CR); - if ((reg & (1u << periph->shift)) == 0u) { - reg = readl_relaxed(base_addr + REG_SUBBLK_CLOCK_CR); - if (reg & (1u << periph->shift)) - return 1; - } + reg = readl_relaxed(base_addr + REG_SUBBLK_CLOCK_CR); + if (reg & (1u << periph->shift)) + return 1; return 0; }
The current clock driver for PolarFire SoC puts the hardware behind "periph" clocks into reset if their clock is disabled. CONFIG_PM was recently added to the riscv defconfig and exposed issues caused by this behaviour, where the Cadence GEM was being put into reset between its bringup & the PHY bringup: https://lore.kernel.org/linux-riscv/9f4b057d-1985-5fd3-65c0-f944161c7792@microchip.com/ Fix this by removing the reset enable/disable code from the driver & rely (for now) on the bootloader bringing peripherals out of reset during boot. Fixes: 635e5e73370e ("clk: microchip: Add driver for Microchip PolarFire SoC") Reviewed-by: Daire McNamara <daire.mcnamara@microchip.com> Signed-off-by: Conor Dooley <conor.dooley@microchip.com> --- drivers/clk/microchip/clk-mpfs.c | 18 +++--------------- 1 file changed, 3 insertions(+), 15 deletions(-)