diff mbox series

[v2,3/3] ARM: dts: aspeed: add reset properties into MDIO nodes

Message ID 20220321095648.4760-4-dylan_hung@aspeedtech.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series Add reset deassertion for Aspeed MDIO | expand

Checks

Context Check Description
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers success CCed 6 of 6 maintainers
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 28 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/tree_selection success Guessing tree name failed - patch did not apply

Commit Message

Dylan Hung March 21, 2022, 9:56 a.m. UTC 
Add reset control properties into MDIO nodes.  The 4 MDIO controllers in
AST2600 SOC share one reset control bit SCU50[3].

Signed-off-by: Dylan Hung <dylan_hung@aspeedtech.com>
Cc: stable@vger.kernel.org
---
 arch/arm/boot/dts/aspeed-g6.dtsi | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Krzysztof Kozlowski March 21, 2022, 3:53 p.m. UTC | #1 
On 21/03/2022 10:56, Dylan Hung wrote:
> Add reset control properties into MDIO nodes.  The 4 MDIO controllers in
> AST2600 SOC share one reset control bit SCU50[3].
> 
> Signed-off-by: Dylan Hung <dylan_hung@aspeedtech.com>
> Cc: stable@vger.kernel.org

Please describe the bug being fixed. See stable-kernel-rules.

Best regards,
Krzysztof
Dylan Hung March 22, 2022, 2:32 a.m. UTC | #2 
> -----Original Message-----
> From: Krzysztof Kozlowski [mailto:krzk@kernel.org]
> Sent: 2022年3月21日 11:53 PM
> To: Dylan Hung <dylan_hung@aspeedtech.com>; robh+dt@kernel.org;
> joel@jms.id.au; andrew@aj.id.au; andrew@lunn.ch; hkallweit1@gmail.com;
> linux@armlinux.org.uk; davem@davemloft.net; kuba@kernel.org;
> pabeni@redhat.com; p.zabel@pengutronix.de; devicetree@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org; linux-aspeed@lists.ozlabs.org;
> linux-kernel@vger.kernel.org; netdev@vger.kernel.org
> Cc: BMC-SW <BMC-SW@aspeedtech.com>; stable@vger.kernel.org
> Subject: Re: [PATCH v2 3/3] ARM: dts: aspeed: add reset properties into MDIO
> nodes
> 
> On 21/03/2022 10:56, Dylan Hung wrote:
> > Add reset control properties into MDIO nodes.  The 4 MDIO controllers in
> > AST2600 SOC share one reset control bit SCU50[3].
> >
> > Signed-off-by: Dylan Hung <dylan_hung@aspeedtech.com>
> > Cc: stable@vger.kernel.org
> 
> Please describe the bug being fixed. See stable-kernel-rules.

Thank you for your comment.
The reset deassertion of the MDIO device was usually done by the bootloader (u-boot).
However, one of our clients uses proprietary bootloader and doesn't deassert the MDIO
reset so failed to access the HW in kernel driver.  The reset deassertion is missing in the
kernel driver since it was created, should I add a BugFix for the first commit of this driver?
Or would it be better if I remove " Cc: stable@vger.kernel.org"?

> 
> Best regards,
> Krzysztof
Andrew Lunn March 22, 2022, 3 a.m. UTC | #3 
On Tue, Mar 22, 2022 at 02:32:13AM +0000, Dylan Hung wrote:
> > -----Original Message-----
> > From: Krzysztof Kozlowski [mailto:krzk@kernel.org]
> > Sent: 2022年3月21日 11:53 PM
> > To: Dylan Hung <dylan_hung@aspeedtech.com>; robh+dt@kernel.org;
> > joel@jms.id.au; andrew@aj.id.au; andrew@lunn.ch; hkallweit1@gmail.com;
> > linux@armlinux.org.uk; davem@davemloft.net; kuba@kernel.org;
> > pabeni@redhat.com; p.zabel@pengutronix.de; devicetree@vger.kernel.org;
> > linux-arm-kernel@lists.infradead.org; linux-aspeed@lists.ozlabs.org;
> > linux-kernel@vger.kernel.org; netdev@vger.kernel.org
> > Cc: BMC-SW <BMC-SW@aspeedtech.com>; stable@vger.kernel.org
> > Subject: Re: [PATCH v2 3/3] ARM: dts: aspeed: add reset properties into MDIO
> > nodes
> > 
> > On 21/03/2022 10:56, Dylan Hung wrote:
> > > Add reset control properties into MDIO nodes.  The 4 MDIO controllers in
> > > AST2600 SOC share one reset control bit SCU50[3].
> > >
> > > Signed-off-by: Dylan Hung <dylan_hung@aspeedtech.com>
> > > Cc: stable@vger.kernel.org
> > 
> > Please describe the bug being fixed. See stable-kernel-rules.
> 
> Thank you for your comment.
> The reset deassertion of the MDIO device was usually done by the bootloader (u-boot).
> However, one of our clients uses proprietary bootloader and doesn't deassert the MDIO
> reset so failed to access the HW in kernel driver.

So are you saying mainline u-boot releases the reset?

> The reset deassertion is missing in the
> kernel driver since it was created, should I add a BugFix for the first commit of this driver?

Yes, that is normal. Ideally the kernel should not depend on u-boot,
because often people want to use other bootloaders, e.g. barebox. You
should also consider kexec, where one kernel hands over to another
kernel, without the bootloader being involved. In such a situation,
you ideally want to assert and deassert the reset just to clean away
any state the old kernel left around.

But please do note, that the reset is optional, since you need to be
able to work with old DT blobs which don't have the reset property in
them.

	Andrew
Dylan Hung March 22, 2022, 3:22 a.m. UTC | #4 
> -----Original Message-----
> From: Andrew Lunn [mailto:andrew@lunn.ch]
> Sent: 2022年3月22日 11:01 AM
> To: Dylan Hung <dylan_hung@aspeedtech.com>
> Cc: Krzysztof Kozlowski <krzk@kernel.org>; robh+dt@kernel.org;
> joel@jms.id.au; andrew@aj.id.au; hkallweit1@gmail.com;
> linux@armlinux.org.uk; davem@davemloft.net; kuba@kernel.org;
> pabeni@redhat.com; p.zabel@pengutronix.de; devicetree@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org; linux-aspeed@lists.ozlabs.org;
> linux-kernel@vger.kernel.org; netdev@vger.kernel.org; BMC-SW
> <BMC-SW@aspeedtech.com>; stable@vger.kernel.org
> Subject: Re: [PATCH v2 3/3] ARM: dts: aspeed: add reset properties into MDIO
> nodes
> 
> On Tue, Mar 22, 2022 at 02:32:13AM +0000, Dylan Hung wrote:
> > > -----Original Message-----
> > > From: Krzysztof Kozlowski [mailto:krzk@kernel.org]
> > > Sent: 2022年3月21日 11:53 PM
> > > To: Dylan Hung <dylan_hung@aspeedtech.com>; robh+dt@kernel.org;
> > > joel@jms.id.au; andrew@aj.id.au; andrew@lunn.ch;
> > > hkallweit1@gmail.com; linux@armlinux.org.uk; davem@davemloft.net;
> > > kuba@kernel.org; pabeni@redhat.com; p.zabel@pengutronix.de;
> > > devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> > > linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org;
> > > netdev@vger.kernel.org
> > > Cc: BMC-SW <BMC-SW@aspeedtech.com>; stable@vger.kernel.org
> > > Subject: Re: [PATCH v2 3/3] ARM: dts: aspeed: add reset properties
> > > into MDIO nodes
> > >
> > > On 21/03/2022 10:56, Dylan Hung wrote:
> > > > Add reset control properties into MDIO nodes.  The 4 MDIO
> > > > controllers in
> > > > AST2600 SOC share one reset control bit SCU50[3].
> > > >
> > > > Signed-off-by: Dylan Hung <dylan_hung@aspeedtech.com>
> > > > Cc: stable@vger.kernel.org
> > >
> > > Please describe the bug being fixed. See stable-kernel-rules.
> >
> > Thank you for your comment.
> > The reset deassertion of the MDIO device was usually done by the
> bootloader (u-boot).
> > However, one of our clients uses proprietary bootloader and doesn't
> > deassert the MDIO reset so failed to access the HW in kernel driver.
> 
> So are you saying mainline u-boot releases the reset?
> 
Yes, if the mdio devices are used in u-boot.

> > The reset deassertion is missing in the kernel driver since it was
> > created, should I add a BugFix for the first commit of this driver?
> 
> Yes, that is normal. Ideally the kernel should not depend on u-boot, because
> often people want to use other bootloaders, e.g. barebox. You should also
> consider kexec, where one kernel hands over to another kernel, without the
> bootloader being involved. In such a situation, you ideally want to assert and
> deassert the reset just to clean away any state the old kernel left around.
> 
> But please do note, that the reset is optional, since you need to be able to
> work with old DT blobs which don't have the reset property in them.
> 

Thank you. I will let the reset property be optional and modify the error-checking
in the driver accordingly in V3.


> 	Andrew
> 
>
Krzysztof Kozlowski March 22, 2022, 8:40 a.m. UTC | #5 
On 22/03/2022 03:32, Dylan Hung wrote:
>> -----Original Message-----
>> From: Krzysztof Kozlowski [mailto:krzk@kernel.org]
>> Sent: 2022年3月21日 11:53 PM
>> To: Dylan Hung <dylan_hung@aspeedtech.com>; robh+dt@kernel.org;
>> joel@jms.id.au; andrew@aj.id.au; andrew@lunn.ch; hkallweit1@gmail.com;
>> linux@armlinux.org.uk; davem@davemloft.net; kuba@kernel.org;
>> pabeni@redhat.com; p.zabel@pengutronix.de; devicetree@vger.kernel.org;
>> linux-arm-kernel@lists.infradead.org; linux-aspeed@lists.ozlabs.org;
>> linux-kernel@vger.kernel.org; netdev@vger.kernel.org
>> Cc: BMC-SW <BMC-SW@aspeedtech.com>; stable@vger.kernel.org
>> Subject: Re: [PATCH v2 3/3] ARM: dts: aspeed: add reset properties into MDIO
>> nodes
>>
>> On 21/03/2022 10:56, Dylan Hung wrote:
>>> Add reset control properties into MDIO nodes.  The 4 MDIO controllers in
>>> AST2600 SOC share one reset control bit SCU50[3].
>>>
>>> Signed-off-by: Dylan Hung <dylan_hung@aspeedtech.com>
>>> Cc: stable@vger.kernel.org
>>
>> Please describe the bug being fixed. See stable-kernel-rules.
> 
> Thank you for your comment.
> The reset deassertion of the MDIO device was usually done by the bootloader (u-boot).
> However, one of our clients uses proprietary bootloader and doesn't deassert the MDIO
> reset so failed to access the HW in kernel driver.  The reset deassertion is missing in the
> kernel driver since it was created, should I add a BugFix for the first commit of this driver?
> Or would it be better if I remove " Cc: stable@vger.kernel.org"?

This rather looks like a missing feature, not a bug. Anyway any
description must be in commit message.


Best regards,
Krzysztof
Dylan Hung March 22, 2022, 8:44 a.m. UTC | #6 
> -----Original Message-----
> From: Krzysztof Kozlowski [mailto:krzk@kernel.org]
> Sent: 2022年3月22日 4:40 PM
> To: Dylan Hung <dylan_hung@aspeedtech.com>; robh+dt@kernel.org;
> joel@jms.id.au; andrew@aj.id.au; andrew@lunn.ch; hkallweit1@gmail.com;
> linux@armlinux.org.uk; davem@davemloft.net; kuba@kernel.org;
> pabeni@redhat.com; p.zabel@pengutronix.de; devicetree@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org; linux-aspeed@lists.ozlabs.org;
> linux-kernel@vger.kernel.org; netdev@vger.kernel.org
> Cc: BMC-SW <BMC-SW@aspeedtech.com>; stable@vger.kernel.org
> Subject: Re: [PATCH v2 3/3] ARM: dts: aspeed: add reset properties into MDIO
> nodes
> 
> On 22/03/2022 03:32, Dylan Hung wrote:
> >> -----Original Message-----
> >> From: Krzysztof Kozlowski [mailto:krzk@kernel.org]
> >> Sent: 2022年3月21日 11:53 PM
> >> To: Dylan Hung <dylan_hung@aspeedtech.com>; robh+dt@kernel.org;
> >> joel@jms.id.au; andrew@aj.id.au; andrew@lunn.ch;
> >> hkallweit1@gmail.com; linux@armlinux.org.uk; davem@davemloft.net;
> >> kuba@kernel.org; pabeni@redhat.com; p.zabel@pengutronix.de;
> >> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> >> linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org;
> >> netdev@vger.kernel.org
> >> Cc: BMC-SW <BMC-SW@aspeedtech.com>; stable@vger.kernel.org
> >> Subject: Re: [PATCH v2 3/3] ARM: dts: aspeed: add reset properties
> >> into MDIO nodes
> >>
> >> On 21/03/2022 10:56, Dylan Hung wrote:
> >>> Add reset control properties into MDIO nodes.  The 4 MDIO
> >>> controllers in
> >>> AST2600 SOC share one reset control bit SCU50[3].
> >>>
> >>> Signed-off-by: Dylan Hung <dylan_hung@aspeedtech.com>
> >>> Cc: stable@vger.kernel.org
> >>
> >> Please describe the bug being fixed. See stable-kernel-rules.
> >
> > Thank you for your comment.
> > The reset deassertion of the MDIO device was usually done by the
> bootloader (u-boot).
> > However, one of our clients uses proprietary bootloader and doesn't
> > deassert the MDIO reset so failed to access the HW in kernel driver.
> > The reset deassertion is missing in the kernel driver since it was created,
> should I add a BugFix for the first commit of this driver?
> > Or would it be better if I remove " Cc: stable@vger.kernel.org"?
> 
> This rather looks like a missing feature, not a bug. Anyway any description
> must be in commit message.

Thank you. I will remove " Cc: stable@vger.kernel.org" and add more description
in commit message in V3.

> 
> 
> Best regards,
> Krzysztof
Philipp Zabel March 22, 2022, 9:08 a.m. UTC | #7 
On Di, 2022-03-22 at 03:22 +0000, Dylan Hung wrote:
> > -----Original Message-----
> > From: Andrew Lunn [mailto:andrew@lunn.ch]
> > Sent: 2022年3月22日 11:01 AM
> > To: Dylan Hung <dylan_hung@aspeedtech.com>
> > Cc: Krzysztof Kozlowski <krzk@kernel.org>; robh+dt@kernel.org;
> > joel@jms.id.au; andrew@aj.id.au; hkallweit1@gmail.com;
> > linux@armlinux.org.uk; davem@davemloft.net; kuba@kernel.org;
> > pabeni@redhat.com; p.zabel@pengutronix.de; 
> > devicetree@vger.kernel.org;
> > linux-arm-kernel@lists.infradead.org;
> > linux-aspeed@lists.ozlabs.org;
> > linux-kernel@vger.kernel.org; netdev@vger.kernel.org; BMC-SW
> > <BMC-SW@aspeedtech.com>; stable@vger.kernel.org
> > Subject: Re: [PATCH v2 3/3] ARM: dts: aspeed: add reset properties
> > into MDIO
> > nodes
> > 
> > On Tue, Mar 22, 2022 at 02:32:13AM +0000, Dylan Hung wrote:
> > > > -----Original Message-----
> > > > From: Krzysztof Kozlowski [mailto:krzk@kernel.org]
> > > > Sent: 2022年3月21日 11:53 PM
> > > > To: Dylan Hung <dylan_hung@aspeedtech.com>; robh+dt@kernel.org;
> > > > joel@jms.id.au; andrew@aj.id.au; andrew@lunn.ch;
> > > > hkallweit1@gmail.com; linux@armlinux.org.uk; 
> > > > davem@davemloft.net;
> > > > kuba@kernel.org; pabeni@redhat.com; p.zabel@pengutronix.de;
> > > > devicetree@vger.kernel.org; 
> > > > linux-arm-kernel@lists.infradead.org;
> > > > linux-aspeed@lists.ozlabs.org; linux-kernel@vger.kernel.org;
> > > > netdev@vger.kernel.org
> > > > Cc: BMC-SW <BMC-SW@aspeedtech.com>; stable@vger.kernel.org
> > > > Subject: Re: [PATCH v2 3/3] ARM: dts: aspeed: add reset
> > > > properties
> > > > into MDIO nodes
> > > > 
> > > > On 21/03/2022 10:56, Dylan Hung wrote:
> > > > > Add reset control properties into MDIO nodes.  The 4 MDIO
> > > > > controllers in
> > > > > AST2600 SOC share one reset control bit SCU50[3].
> > > > > 
> > > > > Signed-off-by: Dylan Hung <dylan_hung@aspeedtech.com>
> > > > > Cc: stable@vger.kernel.org
> > > > 
> > > > Please describe the bug being fixed. See stable-kernel-rules.
> > > 
> > > Thank you for your comment.
> > > The reset deassertion of the MDIO device was usually done by the
> > bootloader (u-boot).
> > > However, one of our clients uses proprietary bootloader and
> > > doesn't
> > > deassert the MDIO reset so failed to access the HW in kernel
> > > driver.
> > 
> > So are you saying mainline u-boot releases the reset?
> > 
> Yes, if the mdio devices are used in u-boot.
> 
> > > The reset deassertion is missing in the kernel driver since it
> > > was
> > > created, should I add a BugFix for the first commit of this
> > > driver?
> > 
> > Yes, that is normal. Ideally the kernel should not depend on u-
> > boot, because
> > often people want to use other bootloaders, e.g. barebox. You
> > should also
> > consider kexec, where one kernel hands over to another kernel,
> > without the
> > bootloader being involved. In such a situation, you ideally want to
> > assert and
> > deassert the reset just to clean away any state the old kernel left
> > around.
> > 
> > But please do note, that the reset is optional, since you need to
> > be able to
> > work with old DT blobs which don't have the reset property in them.
> > 
> 
> Thank you. I will let the reset property be optional and modify the
> error-checking in the driver accordingly in V3.

No need to change the error checking, just use 
devm_reset_control_get_optional_shared().


regards
Philipp
diff mbox series

Patch

diff --git a/arch/arm/boot/dts/aspeed-g6.dtsi b/arch/arm/boot/dts/aspeed-g6.dtsi
index c32e87fad4dc..ab20ea8d829d 100644
--- a/arch/arm/boot/dts/aspeed-g6.dtsi
+++ b/arch/arm/boot/dts/aspeed-g6.dtsi
@@ -181,6 +181,7 @@  mdio0: mdio@1e650000 {
 			status = "disabled";
 			pinctrl-names = "default";
 			pinctrl-0 = <&pinctrl_mdio1_default>;
+			resets = <&syscon ASPEED_RESET_MII>;
 		};
 
 		mdio1: mdio@1e650008 {
@@ -191,6 +192,7 @@  mdio1: mdio@1e650008 {
 			status = "disabled";
 			pinctrl-names = "default";
 			pinctrl-0 = <&pinctrl_mdio2_default>;
+			resets = <&syscon ASPEED_RESET_MII>;
 		};
 
 		mdio2: mdio@1e650010 {
@@ -201,6 +203,7 @@  mdio2: mdio@1e650010 {
 			status = "disabled";
 			pinctrl-names = "default";
 			pinctrl-0 = <&pinctrl_mdio3_default>;
+			resets = <&syscon ASPEED_RESET_MII>;
 		};
 
 		mdio3: mdio@1e650018 {
@@ -211,6 +214,7 @@  mdio3: mdio@1e650018 {
 			status = "disabled";
 			pinctrl-names = "default";
 			pinctrl-0 = <&pinctrl_mdio4_default>;
+			resets = <&syscon ASPEED_RESET_MII>;
 		};
 
 		mac0: ftgmac@1e660000 {