* [PATCH] net: dsa: microchip: KSZ9896 register regmap alignment to 32 bit boundaries
@ 2024-11-27 22:11 Jesse Van Gavere
2024-11-30 22:06 ` Jakub Kicinski
2024-12-02 23:21 ` Tristram.Ha
0 siblings, 2 replies; 6+ messages in thread
From: Jesse Van Gavere @ 2024-11-27 22:11 UTC (permalink / raw)
To: netdev, woojung.huh, UNGLinuxDriver; +Cc: andrew, olteanv, 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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] net: dsa: microchip: KSZ9896 register regmap alignment to 32 bit boundaries
2024-11-27 22:11 [PATCH] net: dsa: microchip: KSZ9896 register regmap alignment to 32 bit boundaries Jesse Van Gavere
@ 2024-11-30 22:06 ` Jakub Kicinski
2024-12-03 5:29 ` Jesse Van Gavere
2024-12-02 23:21 ` Tristram.Ha
1 sibling, 1 reply; 6+ messages in thread
From: Jakub Kicinski @ 2024-11-30 22:06 UTC (permalink / raw)
To: Jesse Van Gavere
Cc: netdev, woojung.huh, UNGLinuxDriver, andrew, olteanv,
Jesse Van Gavere
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] net: dsa: microchip: KSZ9896 register regmap alignment to 32 bit boundaries
2024-11-27 22:11 [PATCH] net: dsa: microchip: KSZ9896 register regmap alignment to 32 bit boundaries Jesse Van Gavere
2024-11-30 22:06 ` Jakub Kicinski
@ 2024-12-02 23:21 ` Tristram.Ha
1 sibling, 0 replies; 6+ messages in thread
From: Tristram.Ha @ 2024-12-02 23:21 UTC (permalink / raw)
To: jesseevg
Cc: andrew, olteanv, jesse.vangavere, netdev, Woojung.Huh,
UNGLinuxDriver
> 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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net: dsa: microchip: KSZ9896 register regmap alignment to 32 bit boundaries
2024-11-30 22:06 ` Jakub Kicinski
@ 2024-12-03 5:29 ` Jesse Van Gavere
2024-12-06 22:12 ` Jesse Van Gavere
0 siblings, 1 reply; 6+ messages in thread
From: Jesse Van Gavere @ 2024-12-03 5:29 UTC (permalink / raw)
To: Jakub Kicinski
Cc: netdev, woojung.huh, UNGLinuxDriver, andrew, olteanv,
Jesse Van Gavere, Tristram.Ha
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net: dsa: microchip: KSZ9896 register regmap alignment to 32 bit boundaries
2024-12-03 5:29 ` Jesse Van Gavere
@ 2024-12-06 22:12 ` Jesse Van Gavere
2024-12-07 1:24 ` Jakub Kicinski
0 siblings, 1 reply; 6+ messages in thread
From: Jesse Van Gavere @ 2024-12-06 22:12 UTC (permalink / raw)
To: Jakub Kicinski
Cc: netdev, woojung.huh, UNGLinuxDriver, andrew, olteanv,
Jesse Van Gavere, Tristram.Ha
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] net: dsa: microchip: KSZ9896 register regmap alignment to 32 bit boundaries
2024-12-06 22:12 ` Jesse Van Gavere
@ 2024-12-07 1:24 ` Jakub Kicinski
0 siblings, 0 replies; 6+ messages in thread
From: Jakub Kicinski @ 2024-12-07 1:24 UTC (permalink / raw)
To: Jesse Van Gavere
Cc: netdev, woojung.huh, UNGLinuxDriver, andrew, olteanv,
Jesse Van Gavere, Tristram.Ha
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-12-07 1:24 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-11-27 22:11 [PATCH] net: dsa: microchip: KSZ9896 register regmap alignment to 32 bit boundaries Jesse Van Gavere
2024-11-30 22:06 ` Jakub Kicinski
2024-12-03 5:29 ` Jesse Van Gavere
2024-12-06 22:12 ` Jesse Van Gavere
2024-12-07 1:24 ` Jakub Kicinski
2024-12-02 23:21 ` Tristram.Ha
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).