[PATCH] net: dsa: microchip: KSZ9896 register regmap alignment to 32 bit boundaries

[PATCH] net: dsa: microchip: KSZ9896 register regmap alignment to 32 bit boundaries

[Jesse Van Gavere]

Commit (SHA1: 8d7ae22ae9f8c8a4407f8e993df64440bdbd0cee) fixed this issue
for the KSZ9477 by adjusting the regmap ranges.

The same issue presents itself on the KSZ9896 regs and is fixed with 
the same regmap range modification.

Signed-off-by: Jesse Van Gavere <jesse.vangavere@scioteq.com>
---
 drivers/net/dsa/microchip/ksz_common.c | 42 +++++++++++---------------
 1 file changed, 18 insertions(+), 24 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 920443ee8ffd..8a03baa6aecc 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -1100,10 +1100,9 @@ static const struct regmap_range ksz9896_valid_regs[] = {
 	regmap_reg_range(0x1030, 0x1030),
 	regmap_reg_range(0x1100, 0x1115),
 	regmap_reg_range(0x111a, 0x111f),
-	regmap_reg_range(0x1122, 0x1127),
-	regmap_reg_range(0x112a, 0x112b),
-	regmap_reg_range(0x1136, 0x1139),
-	regmap_reg_range(0x113e, 0x113f),
+	regmap_reg_range(0x1120, 0x112b),
+	regmap_reg_range(0x1134, 0x113b),
+	regmap_reg_range(0x113c, 0x113f),
 	regmap_reg_range(0x1400, 0x1401),
 	regmap_reg_range(0x1403, 0x1403),
 	regmap_reg_range(0x1410, 0x1417),
@@ -1130,10 +1129,9 @@ static const struct regmap_range ksz9896_valid_regs[] = {
 	regmap_reg_range(0x2030, 0x2030),
 	regmap_reg_range(0x2100, 0x2115),
 	regmap_reg_range(0x211a, 0x211f),
-	regmap_reg_range(0x2122, 0x2127),
-	regmap_reg_range(0x212a, 0x212b),
-	regmap_reg_range(0x2136, 0x2139),
-	regmap_reg_range(0x213e, 0x213f),
+	regmap_reg_range(0x2120, 0x212b),
+	regmap_reg_range(0x2134, 0x213b),
+	regmap_reg_range(0x213c, 0x213f),
 	regmap_reg_range(0x2400, 0x2401),
 	regmap_reg_range(0x2403, 0x2403),
 	regmap_reg_range(0x2410, 0x2417),
@@ -1160,10 +1158,9 @@ static const struct regmap_range ksz9896_valid_regs[] = {
 	regmap_reg_range(0x3030, 0x3030),
 	regmap_reg_range(0x3100, 0x3115),
 	regmap_reg_range(0x311a, 0x311f),
-	regmap_reg_range(0x3122, 0x3127),
-	regmap_reg_range(0x312a, 0x312b),
-	regmap_reg_range(0x3136, 0x3139),
-	regmap_reg_range(0x313e, 0x313f),
+	regmap_reg_range(0x3120, 0x312b),
+	regmap_reg_range(0x3134, 0x313b),
+	regmap_reg_range(0x313c, 0x313f),
 	regmap_reg_range(0x3400, 0x3401),
 	regmap_reg_range(0x3403, 0x3403),
 	regmap_reg_range(0x3410, 0x3417),
@@ -1190,10 +1187,9 @@ static const struct regmap_range ksz9896_valid_regs[] = {
 	regmap_reg_range(0x4030, 0x4030),
 	regmap_reg_range(0x4100, 0x4115),
 	regmap_reg_range(0x411a, 0x411f),
-	regmap_reg_range(0x4122, 0x4127),
-	regmap_reg_range(0x412a, 0x412b),
-	regmap_reg_range(0x4136, 0x4139),
-	regmap_reg_range(0x413e, 0x413f),
+	regmap_reg_range(0x4120, 0x412b),
+	regmap_reg_range(0x4134, 0x413b),
+	regmap_reg_range(0x413c, 0x413f),
 	regmap_reg_range(0x4400, 0x4401),
 	regmap_reg_range(0x4403, 0x4403),
 	regmap_reg_range(0x4410, 0x4417),
@@ -1220,10 +1216,9 @@ static const struct regmap_range ksz9896_valid_regs[] = {
 	regmap_reg_range(0x5030, 0x5030),
 	regmap_reg_range(0x5100, 0x5115),
 	regmap_reg_range(0x511a, 0x511f),
-	regmap_reg_range(0x5122, 0x5127),
-	regmap_reg_range(0x512a, 0x512b),
-	regmap_reg_range(0x5136, 0x5139),
-	regmap_reg_range(0x513e, 0x513f),
+	regmap_reg_range(0x5120, 0x512b),
+	regmap_reg_range(0x5134, 0x513b),
+	regmap_reg_range(0x513c, 0x513f),
 	regmap_reg_range(0x5400, 0x5401),
 	regmap_reg_range(0x5403, 0x5403),
 	regmap_reg_range(0x5410, 0x5417),
@@ -1250,10 +1245,9 @@ static const struct regmap_range ksz9896_valid_regs[] = {
 	regmap_reg_range(0x6030, 0x6030),
 	regmap_reg_range(0x6100, 0x6115),
 	regmap_reg_range(0x611a, 0x611f),
-	regmap_reg_range(0x6122, 0x6127),
-	regmap_reg_range(0x612a, 0x612b),
-	regmap_reg_range(0x6136, 0x6139),
-	regmap_reg_range(0x613e, 0x613f),
+	regmap_reg_range(0x6120, 0x612b),
+	regmap_reg_range(0x6134, 0x613b),
+	regmap_reg_range(0x613c, 0x613f),
 	regmap_reg_range(0x6300, 0x6301),
 	regmap_reg_range(0x6400, 0x6401),
 	regmap_reg_range(0x6403, 0x6403),
-- 
2.34.1

Re: [PATCH] net: dsa: microchip: KSZ9896 register regmap alignment to 32 bit boundaries

[Jakub Kicinski]

On Wed, 27 Nov 2024 23:11:29 +0100 Jesse Van Gavere wrote:
> Commit (SHA1: 8d7ae22ae9f8c8a4407f8e993df64440bdbd0cee) fixed this issue
> for the KSZ9477 by adjusting the regmap ranges.

The correct format for referring to other commits in Linux kernel is:
 %h (\"%s\")
IOW:

 Commit 8d7ae22ae9f8 ("net: dsa: microchip: KSZ9477 register regmap
 alignment to 32 bit boundaries") fixed this issue...

> The same issue presents itself on the KSZ9896 regs and is fixed with 
> the same regmap range modification.

Could you explain the impact? What will not work / break without this
change? Please add a Fixes tag indicating where buggy code was added
to make sure backporters know how far to backport.
-- 
pw-bot: cr

Re: [PATCH] net: dsa: microchip: KSZ9896 register regmap alignment to 32 bit boundaries

[Jesse Van Gavere]

Hello Jakub, all,

Op za 30 nov 2024 om 23:07 schreef Jakub Kicinski <kuba@kernel.org>:
>
> On Wed, 27 Nov 2024 23:11:29 +0100 Jesse Van Gavere wrote:
> > Commit (SHA1: 8d7ae22ae9f8c8a4407f8e993df64440bdbd0cee) fixed this issue
> > for the KSZ9477 by adjusting the regmap ranges.
>
> The correct format for referring to other commits in Linux kernel is:
>  %h (\"%s\")
> IOW:
>
>  Commit 8d7ae22ae9f8 ("net: dsa: microchip: KSZ9477 register regmap
>  alignment to 32 bit boundaries") fixed this issue...
>
> > The same issue presents itself on the KSZ9896 regs and is fixed with
> > the same regmap range modification.
>
> Could you explain the impact? What will not work / break without this
> change? Please add a Fixes tag indicating where buggy code was added
> to make sure backporters know how far to backport.
Will do, still learning the ropes of contributing, thanks for the feedback!
> --
> pw-bot: cr

What do you think I preferably do to account for Tristram's feedback
in my next patch?
Should I incorporate it as-is, keep my patch with requested changes,
or perhaps even "fix" it with below suggestion across all registers
sets?
> The port address range 0x#100-0x#13F just maps to the PHY registers 0-31,
> so it can be simply one single regmap_reg_range(0x1100, 0x113f) instead
> of many.  I suggest using regmap_reg_range(0x1100, 0x111f) and
> regmap_reg_range(0x1120, 0x113f) to remind people the high range address
> needs special handling.

> I also do not know why those _register_set are not enforced across all
> KSZ9897 related switches.

Best regards,
Jesse

Re: [PATCH] net: dsa: microchip: KSZ9896 register regmap alignment to 32 bit boundaries

[Jesse Van Gavere]

Hi,

A quick ping on this, how do I best proceed here?
Do I keep the original commit and take in account the feedback for the
commit message or should I e.g. like Tristram recommended just modify
it to 2 regmap reg ranges for these PHY registers?
In that case I might just as well modify this commit to make this
modification for all the existing regmap reg range arrays defined.
(There's probably also something to say about enforcing these ranges
across more chips but that's a bit outside the scope of what I'm
trying to do here)

Best regards,
Jesse

Op di 3 dec 2024 om 06:29 schreef Jesse Van Gavere <jesseevg@gmail.com>:
>
> Hello Jakub, all,
>
> Op za 30 nov 2024 om 23:07 schreef Jakub Kicinski <kuba@kernel.org>:
> >
> > On Wed, 27 Nov 2024 23:11:29 +0100 Jesse Van Gavere wrote:
> > > Commit (SHA1: 8d7ae22ae9f8c8a4407f8e993df64440bdbd0cee) fixed this issue
> > > for the KSZ9477 by adjusting the regmap ranges.
> >
> > The correct format for referring to other commits in Linux kernel is:
> >  %h (\"%s\")
> > IOW:
> >
> >  Commit 8d7ae22ae9f8 ("net: dsa: microchip: KSZ9477 register regmap
> >  alignment to 32 bit boundaries") fixed this issue...
> >
> > > The same issue presents itself on the KSZ9896 regs and is fixed with
> > > the same regmap range modification.
> >
> > Could you explain the impact? What will not work / break without this
> > change? Please add a Fixes tag indicating where buggy code was added
> > to make sure backporters know how far to backport.
> Will do, still learning the ropes of contributing, thanks for the feedback!
> > --
> > pw-bot: cr
>
> What do you think I preferably do to account for Tristram's feedback
> in my next patch?
> Should I incorporate it as-is, keep my patch with requested changes,
> or perhaps even "fix" it with below suggestion across all registers
> sets?
> > The port address range 0x#100-0x#13F just maps to the PHY registers 0-31,
> > so it can be simply one single regmap_reg_range(0x1100, 0x113f) instead
> > of many.  I suggest using regmap_reg_range(0x1100, 0x111f) and
> > regmap_reg_range(0x1120, 0x113f) to remind people the high range address
> > needs special handling.
>
> > I also do not know why those _register_set are not enforced across all
> > KSZ9897 related switches.
>
> Best regards,
> Jesse

Re: [PATCH] net: dsa: microchip: KSZ9896 register regmap alignment to 32 bit boundaries

[Jakub Kicinski]

On Fri, 6 Dec 2024 23:12:56 +0100 Jesse Van Gavere wrote:
> A quick ping on this, how do I best proceed here?
> Do I keep the original commit and take in account the feedback for the
> commit message or should I e.g. like Tristram recommended just modify
> it to 2 regmap reg ranges for these PHY registers?

I don't have enough practical regmap experience to understand Tristram
suggestion, TBH. Is he saying to collapse multiple ranges into one by
extending it to register that are currently not used? Or to somehow
ignore high bits? Rhetorical question, see below..

> In that case I might just as well modify this commit to make this
> modification for all the existing regmap reg range arrays defined.
> (There's probably also something to say about enforcing these ranges
> across more chips but that's a bit outside the scope of what I'm
> trying to do here)

Whatever makes the changes easier to review. If the combined commit is
small (per stable rules) and easy to review follow the feedback. If
it's easier to review the fix and then cleanup separately - separate.

RE: [PATCH] net: dsa: microchip: KSZ9896 register regmap alignment to 32 bit boundaries

[Tristram.Ha]

> Commit (SHA1: 8d7ae22ae9f8c8a4407f8e993df64440bdbd0cee) fixed this issue
> for the KSZ9477 by adjusting the regmap ranges.
> 
> The same issue presents itself on the KSZ9896 regs and is fixed with
> the same regmap range modification.
> 
> Signed-off-by: Jesse Van Gavere <jesse.vangavere@scioteq.com>
> ---
>  drivers/net/dsa/microchip/ksz_common.c | 42 +++++++++++---------------
>  1 file changed, 18 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/net/dsa/microchip/ksz_common.c
> b/drivers/net/dsa/microchip/ksz_common.c
> index 920443ee8ffd..8a03baa6aecc 100644
> --- a/drivers/net/dsa/microchip/ksz_common.c
> +++ b/drivers/net/dsa/microchip/ksz_common.c
> @@ -1100,10 +1100,9 @@ static const struct regmap_range ksz9896_valid_regs[] = {
>         regmap_reg_range(0x1030, 0x1030),
>         regmap_reg_range(0x1100, 0x1115),
>         regmap_reg_range(0x111a, 0x111f),
> -       regmap_reg_range(0x1122, 0x1127),
> -       regmap_reg_range(0x112a, 0x112b),
> -       regmap_reg_range(0x1136, 0x1139),
> -       regmap_reg_range(0x113e, 0x113f),
> +       regmap_reg_range(0x1120, 0x112b),
> +       regmap_reg_range(0x1134, 0x113b),
> +       regmap_reg_range(0x113c, 0x113f),
>         regmap_reg_range(0x1400, 0x1401),
>         regmap_reg_range(0x1403, 0x1403),
>         regmap_reg_range(0x1410, 0x1417),
> @@ -1130,10 +1129,9 @@ static const struct regmap_range ksz9896_valid_regs[] = {
>         regmap_reg_range(0x2030, 0x2030),
>         regmap_reg_range(0x2100, 0x2115),
>         regmap_reg_range(0x211a, 0x211f),
> -       regmap_reg_range(0x2122, 0x2127),
> -       regmap_reg_range(0x212a, 0x212b),
> -       regmap_reg_range(0x2136, 0x2139),
> -       regmap_reg_range(0x213e, 0x213f),
> +       regmap_reg_range(0x2120, 0x212b),
> +       regmap_reg_range(0x2134, 0x213b),
> +       regmap_reg_range(0x213c, 0x213f),
>         regmap_reg_range(0x2400, 0x2401),
>         regmap_reg_range(0x2403, 0x2403),
>         regmap_reg_range(0x2410, 0x2417),
> @@ -1160,10 +1158,9 @@ static const struct regmap_range ksz9896_valid_regs[] = {
>         regmap_reg_range(0x3030, 0x3030),
>         regmap_reg_range(0x3100, 0x3115),
>         regmap_reg_range(0x311a, 0x311f),
> -       regmap_reg_range(0x3122, 0x3127),
> -       regmap_reg_range(0x312a, 0x312b),
> -       regmap_reg_range(0x3136, 0x3139),
> -       regmap_reg_range(0x313e, 0x313f),
> +       regmap_reg_range(0x3120, 0x312b),
> +       regmap_reg_range(0x3134, 0x313b),
> +       regmap_reg_range(0x313c, 0x313f),
>         regmap_reg_range(0x3400, 0x3401),
>         regmap_reg_range(0x3403, 0x3403),
>         regmap_reg_range(0x3410, 0x3417),
> @@ -1190,10 +1187,9 @@ static const struct regmap_range ksz9896_valid_regs[] = {
>         regmap_reg_range(0x4030, 0x4030),
>         regmap_reg_range(0x4100, 0x4115),
>         regmap_reg_range(0x411a, 0x411f),
> -       regmap_reg_range(0x4122, 0x4127),
> -       regmap_reg_range(0x412a, 0x412b),
> -       regmap_reg_range(0x4136, 0x4139),
> -       regmap_reg_range(0x413e, 0x413f),
> +       regmap_reg_range(0x4120, 0x412b),
> +       regmap_reg_range(0x4134, 0x413b),
> +       regmap_reg_range(0x413c, 0x413f),
>         regmap_reg_range(0x4400, 0x4401),
>         regmap_reg_range(0x4403, 0x4403),
>         regmap_reg_range(0x4410, 0x4417),
> @@ -1220,10 +1216,9 @@ static const struct regmap_range ksz9896_valid_regs[] = {
>         regmap_reg_range(0x5030, 0x5030),
>         regmap_reg_range(0x5100, 0x5115),
>         regmap_reg_range(0x511a, 0x511f),
> -       regmap_reg_range(0x5122, 0x5127),
> -       regmap_reg_range(0x512a, 0x512b),
> -       regmap_reg_range(0x5136, 0x5139),
> -       regmap_reg_range(0x513e, 0x513f),
> +       regmap_reg_range(0x5120, 0x512b),
> +       regmap_reg_range(0x5134, 0x513b),
> +       regmap_reg_range(0x513c, 0x513f),
>         regmap_reg_range(0x5400, 0x5401),
>         regmap_reg_range(0x5403, 0x5403),
>         regmap_reg_range(0x5410, 0x5417),
> @@ -1250,10 +1245,9 @@ static const struct regmap_range ksz9896_valid_regs[] = {
>         regmap_reg_range(0x6030, 0x6030),
>         regmap_reg_range(0x6100, 0x6115),
>         regmap_reg_range(0x611a, 0x611f),
> -       regmap_reg_range(0x6122, 0x6127),
> -       regmap_reg_range(0x612a, 0x612b),
> -       regmap_reg_range(0x6136, 0x6139),
> -       regmap_reg_range(0x613e, 0x613f),
> +       regmap_reg_range(0x6120, 0x612b),
> +       regmap_reg_range(0x6134, 0x613b),
> +       regmap_reg_range(0x613c, 0x613f),
>         regmap_reg_range(0x6300, 0x6301),
>         regmap_reg_range(0x6400, 0x6401),
>         regmap_reg_range(0x6403, 0x6403),

The port address range 0x#100-0x#13F just maps to the PHY registers 0-31,
so it can be simply one single regmap_reg_range(0x1100, 0x113f) instead
of many.  I suggest using regmap_reg_range(0x1100, 0x111f) and
regmap_reg_range(0x1120, 0x113f) to remind people the high range address
needs special handling.

I also do not know why those _register_set are not enforced across all
KSZ9897 related switches.