Netdev Archive on
help / color / mirror / Atom feed
From: Helmut Grohne <>
To: Andrew Lunn <>
Cc: Nicolas Ferre <>,
	Alexandre Belloni <>,
	Ludovic Desroches <>,
	Woojung Huh <>,
	Microchip Linux Driver Support <>,
	Vivien Didelot <>,
	Florian Fainelli <>,
	"David S. Miller" <>,
	Jakub Kicinski <>,
	"Rob Herring" <>,
	"" <>,
	"" <>
Subject: Re: [PATCH] net: dsa: microchip: look for phy-mode in port nodes
Date: Thu, 16 Jul 2020 12:07:43 +0200	[thread overview]
Message-ID: <20200716100743.GA3275@laureti-dev> (raw)
In-Reply-To: <20200716070000.GA27587@laureti-dev>

On Thu, Jul 16, 2020 at 09:00:00AM +0200, Helmut Grohne wrote:
> I've prepared a patch based one the one-CPU-port assumption. It really
> becomes way simpler that way. I'd like to give it a little more testing
> before sending it.

I'm sorry, but it is not that simple. Testing revealed a fatal flaw.

At the time ksz_switch_register is called, dev->cpu_port is not
necessarily initialized. For ksz8795, it is initialized during detect
and that is fine. For ksz9477, it is initialized in
ksz9477_config_cpu_port, which comes much too late. The ksz9477 driver
actually handles the case where we choose a CPU port and selects it
using dsa_is_cpu_port (which derives this information from the device
tree during dsa_register_switch).

So in ksz_switch_register, I have no good way of knowing which port will
end up being the CPU port. Options include:
 * Replicating the logic from ksz9477_config_cpu_port. I expect that
   doing this would make the driver harder to maintain.
 * Move the cpu_port computation from ksz9477_config_cpu_port to
   ksz9477_switch_detect. I don't think this would work, because we
   presently call detect before dsa_register_switch, which parses the
   device tree. During detect, dsa_is_cpu_port is not usable.
 * Add a function to ksz_common.c that looks up the phy mode for a port
   from the device tree on demand. That way, ksz9477_config_cpu_port
   could call into the common code instead of reading a ksz_device
   property. It would defer parsing the device tree though and it could
   issue the warning multiple times.
 * Stick with my previous patch that stores the phy mode per-port.

Given the above, the last option seems least bad to me. Do you agree?


  reply	other threads:[~2020-07-16 10:07 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-17  8:22 net/dsa/microchip: correct placement of dt property phy-mode? Helmut Grohne
2020-06-17 21:18 ` Andrew Lunn
2020-07-14 12:08 ` [PATCH] net: dsa: microchip: look for phy-mode in port nodes Helmut Grohne
2020-07-14 22:27   ` Andrew Lunn
2020-07-15  7:31     ` Helmut Grohne
2020-07-15 13:00       ` Andrew Lunn
2020-07-16  7:00         ` Helmut Grohne
2020-07-16 10:07           ` Helmut Grohne [this message]
2020-08-20  6:03             ` [RESEND PATCH] " Helmut Grohne
2020-08-24 22:37               ` David Miller
2020-09-04  8:14                 ` [PATCH v2] " Helmut Grohne
2020-09-04 12:59                   ` Alexandre Belloni
2020-09-04 13:52                   ` Andrew Lunn
2020-09-07  6:15                     ` Helmut Grohne
2020-09-07 12:55                       ` Andrew Lunn
2020-09-08  8:01                         ` [PATCH v3] " Helmut Grohne
2020-09-10 19:32                           ` David Miller
2020-09-24  8:37                             ` [PATCH] net: dsa: microchip: really " Helmut Grohne
2020-09-25  3:10                               ` David Miller

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200716100743.GA3275@laureti-dev \ \ \ \ \ \ \ \ \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).