[PATCH v1] clk: microchip: mpfs: don't reset disabled peripherals

[PATCH v1] clk: microchip: mpfs: don't reset disabled peripherals

[Conor Dooley]

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(-)

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;
 }
-- 
2.35.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH v1] clk: microchip: mpfs: don't reset disabled peripherals

[Conor Dooley]

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(-)

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;
 }
-- 
2.35.1

Re: [PATCH v1] clk: microchip: mpfs: don't reset disabled peripherals

[Andrew Lunn]

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

Re: [PATCH v1] clk: microchip: mpfs: don't reset disabled peripherals

[Andrew Lunn]

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH v1] clk: microchip: mpfs: don't reset disabled peripherals

[Conor.Dooley]

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.

Re: [PATCH v1] clk: microchip: mpfs: don't reset disabled peripherals

[Conor.Dooley]

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.
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv