There are two variants of the MT7531 switch IC which got different
features (and pins) regarding port 5:
 * MT7531AE: SGMII/1000Base-X/2500Base-X SerDes PCS
 * MT7531BE: RGMII

Moving the creation of the SerDes PCS from mt753x_setup to mt7530_probe
with commit 6de285229773 ("net: dsa: mt7530: move SGMII PCS creation
to mt7530_probe function") works fine for MT7531AE which got two
instances of mtk-pcs-lynxi, however, MT7531BE requires mt7531_pll_setup
to setup clocks before the single PCS on port 6 (usually used as CPU
port) starts to work and hence the PCS creation failed on MT7531BE.

Fix this by introducing a pointer to mt7531_create_sgmii function in
struct mt7530_priv and call it again at the end of mt753x_setup like it
was before commit 6de285229773 ("net: dsa: mt7530: move SGMII PCS
creation to mt7530_probe function").

Fixes: 6de285229773 ("net: dsa: mt7530: move SGMII PCS creation to mt7530_probe function")
Signed-off-by: Daniel Golle <daniel@makrotopia.org>
---
 drivers/net/dsa/mt7530-mdio.c | 16 ++++++++--------
 drivers/net/dsa/mt7530.c      |  6 ++++++
 drivers/net/dsa/mt7530.h      |  4 ++--
 3 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/drivers/net/dsa/mt7530-mdio.c b/drivers/net/dsa/mt7530-mdio.c
index 34a547b88e497..088533663b83b 100644
--- a/drivers/net/dsa/mt7530-mdio.c
+++ b/drivers/net/dsa/mt7530-mdio.c
@@ -81,14 +81,17 @@ static const struct regmap_bus mt7530_regmap_bus = {
 };
 
 static int
-mt7531_create_sgmii(struct mt7530_priv *priv)
+mt7531_create_sgmii(struct mt7530_priv *priv, bool dual_sgmii)
 {
-	struct regmap_config *mt7531_pcs_config[2];
+	struct regmap_config *mt7531_pcs_config[2] = {};
 	struct phylink_pcs *pcs;
 	struct regmap *regmap;
 	int i, ret = 0;
 
-	for (i = 0; i < 2; i++) {
+	/* MT7531AE has two SGMII units for port 5 and port 6
+	 * MT7531BE has only one SGMII unit for port 6
+	 */
+	for (i = dual_sgmii ? 0 : 1; i < 2; i++) {
 		mt7531_pcs_config[i] = devm_kzalloc(priv->dev,
 						    sizeof(struct regmap_config),
 						    GFP_KERNEL);
@@ -208,11 +211,8 @@ mt7530_probe(struct mdio_device *mdiodev)
 	if (IS_ERR(priv->regmap))
 		return PTR_ERR(priv->regmap);
 
-	if (priv->id == ID_MT7531) {
-		ret = mt7531_create_sgmii(priv);
-		if (ret)
-			return ret;
-	}
+	if (priv->id == ID_MT7531)
+		priv->create_sgmii = mt7531_create_sgmii;
 
 	return dsa_register_switch(priv->ds);
 }
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index e4bb5037d3525..c680873819b01 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -3018,6 +3018,12 @@ mt753x_setup(struct dsa_switch *ds)
 	if (ret && priv->irq)
 		mt7530_free_irq_common(priv);
 
+	if (priv->create_sgmii) {
+		ret = priv->create_sgmii(priv, mt7531_dual_sgmii_supported(priv));
+		if (ret && priv->irq)
+			mt7530_free_irq(priv);
+	}
+
 	return ret;
 }
 
diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
index 01db5c9724fa8..5084f48a88691 100644
--- a/drivers/net/dsa/mt7530.h
+++ b/drivers/net/dsa/mt7530.h
@@ -748,10 +748,10 @@ struct mt753x_info {
  *			registers
  * @p6_interface	Holding the current port 6 interface
  * @p5_intf_sel:	Holding the current port 5 interface select
- *
  * @irq:		IRQ number of the switch
  * @irq_domain:		IRQ domain of the switch irq_chip
  * @irq_enable:		IRQ enable bits, synced to SYS_INT_EN
+ * @create_sgmii:	Pointer to function creating SGMII PCS instance(s)
  */
 struct mt7530_priv {
 	struct device		*dev;
@@ -770,7 +770,6 @@ struct mt7530_priv {
 	unsigned int		p5_intf_sel;
 	u8			mirror_rx;
 	u8			mirror_tx;
-
 	struct mt7530_port	ports[MT7530_NUM_PORTS];
 	struct mt753x_pcs	pcs[MT7530_NUM_PORTS];
 	/* protect among processes for registers access*/
@@ -778,6 +777,7 @@ struct mt7530_priv {
 	int irq;
 	struct irq_domain *irq_domain;
 	u32 irq_enable;
+	int (*create_sgmii)(struct mt7530_priv *priv, bool dual_sgmii);
 };
 
 struct mt7530_hw_vlan_entry {

base-commit: e61caf04b9f8a2626714ffd12e734c555c758af4
-- 
2.40.0

Hello:

This patch was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Sun, 16 Apr 2023 13:08:14 +0100 you wrote:
> There are two variants of the MT7531 switch IC which got different
> features (and pins) regarding port 5:
>  * MT7531AE: SGMII/1000Base-X/2500Base-X SerDes PCS
>  * MT7531BE: RGMII
> 
> Moving the creation of the SerDes PCS from mt753x_setup to mt7530_probe
> with commit 6de285229773 ("net: dsa: mt7530: move SGMII PCS creation
> to mt7530_probe function") works fine for MT7531AE which got two
> instances of mtk-pcs-lynxi, however, MT7531BE requires mt7531_pll_setup
> to setup clocks before the single PCS on port 6 (usually used as CPU
> port) starts to work and hence the PCS creation failed on MT7531BE.
> 
> [...]

Here is the summary with links:
  - [net-next,v2] net: dsa: mt7530: fix support for MT7531BE
    https://git.kernel.org/netdev/net-next/c/91daa4f62ce8

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

On Sun, Apr 16, 2023 at 01:08:14PM +0100, Daniel Golle wrote:
> There are two variants of the MT7531 switch IC which got different
> features (and pins) regarding port 5:
>  * MT7531AE: SGMII/1000Base-X/2500Base-X SerDes PCS
>  * MT7531BE: RGMII
> 
> Moving the creation of the SerDes PCS from mt753x_setup to mt7530_probe
> with commit 6de285229773 ("net: dsa: mt7530: move SGMII PCS creation
> to mt7530_probe function") works fine for MT7531AE which got two
> instances of mtk-pcs-lynxi, however, MT7531BE requires mt7531_pll_setup
> to setup clocks before the single PCS on port 6 (usually used as CPU
> port) starts to work and hence the PCS creation failed on MT7531BE.
> 
> Fix this by introducing a pointer to mt7531_create_sgmii function in
> struct mt7530_priv and call it again at the end of mt753x_setup like it
> was before commit 6de285229773 ("net: dsa: mt7530: move SGMII PCS
> creation to mt7530_probe function").
> 
> Fixes: 6de285229773 ("net: dsa: mt7530: move SGMII PCS creation to mt7530_probe function")
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> ---

This v2 submission addresses the comments made by Jesse Brandeburg
regarding zero-initializing regmap_config as we are now not necessarily
using both of them. Comments by Arınç ÜNAL have also been discussed
and resulting in receiving Ack.

However, I can see in patchwork that the patch has been set to
"Changes Requested".

Can someone please tell me which further changes are needed?
I don't see any other comments on the mailing list or patchwork.

Thank you!


Daniel

On Thu, 20 Apr 2023 01:22:20 +0100 Daniel Golle wrote:
> This v2 submission addresses the comments made by Jesse Brandeburg
> regarding zero-initializing regmap_config as we are now not necessarily
> using both of them. Comments by Arınç ÜNAL have also been discussed
> and resulting in receiving Ack.
> 
> However, I can see in patchwork that the patch has been set to
> "Changes Requested".
> 
> Can someone please tell me which further changes are needed?
> I don't see any other comments on the mailing list or patchwork.

Someone mis-categorized :( I was keeping an eye, tho, so it wouldn't
have gotten lost.

Applied now, thanks!

On 16.04.2023 15:08, Daniel Golle wrote:
> There are two variants of the MT7531 switch IC which got different
> features (and pins) regarding port 5:
>   * MT7531AE: SGMII/1000Base-X/2500Base-X SerDes PCS
>   * MT7531BE: RGMII
> 
> Moving the creation of the SerDes PCS from mt753x_setup to mt7530_probe
> with commit 6de285229773 ("net: dsa: mt7530: move SGMII PCS creation
> to mt7530_probe function") works fine for MT7531AE which got two
> instances of mtk-pcs-lynxi, however, MT7531BE requires mt7531_pll_setup
> to setup clocks before the single PCS on port 6 (usually used as CPU
> port) starts to work and hence the PCS creation failed on MT7531BE.
> 
> Fix this by introducing a pointer to mt7531_create_sgmii function in
> struct mt7530_priv and call it again at the end of mt753x_setup like it
> was before commit 6de285229773 ("net: dsa: mt7530: move SGMII PCS
> creation to mt7530_probe function").
> 
> Fixes: 6de285229773 ("net: dsa: mt7530: move SGMII PCS creation to mt7530_probe function")
> Signed-off-by: Daniel Golle <daniel@makrotopia.org>

I'll put my 2 cents about the patch along with responding to your points 
on the other thread here.

> Why don't we use my original solution [1] which has some advantages:
> 
>  * It doesn't requrire additional export of mt7530_regmap_bus
> 
>  * It doesn't move PCS creation to mt7530.c, hence PCS_MTK_LYNXI is
>    only required for MDIO-connected switches
>    (with your patch we would have to move the dependency on PCS_MTK_LYNXI
>    from NET_DSA_MT7530_MDIO to NET_DSA_MT7530)

Maybe this is what should happen. Maybe the PCS creation (and therefore 
mt7530_regmap_bus) should be on the core driver. Both are on the MDIO 
driver for the sole reason of only the devices on the MDIO driver 
currently using it. It's not an MDIO-specific operation as far as I can 
tell. Having it on the core driver would make more sense in the long run.

> 
>  * It doesn't expose the dysfunctional SerDes PCS for port 5 on MT7531BE
>    This will still fail and hence result in probing on MT7531 to exit
>    prematurely, preventing the switch driver from being loaded.
>    Before 9ecc00164dc23 ("net: dsa: mt7530: refactor SGMII PCS creation")
>    the return value of mtk_pcs_lynxi_create was ignored, now it isn't...

Ok, so checking whether port 5 is SGMII or not on the PCS creation code 
should be done on the same patch that fixes this issue.

> 
>  * It changes much less in terms of LoC

I'd rather prefer a better logic than the "least amount of changes 
possible" approach.

Let's analyse what this patch does:

With this patch, mt7531_create_sgmii() is run after mt7530_setup_mdio is 
run, under mt753x_setup(). mt7531_pll_setup() and, as the last 
requirement, mt7530_setup_mdio() must be run to be able to create the 
PCS instances. That also means running mt7530_free_irq_common must be 
avoided since the device uses MDIO so mt7530_free_mdio_irq needs to be 
run too.

While probing the driver, the priv->create_sgmii pointer will be made to 
point to mt7531_create_sgmii, if MT7531 is detected. Why? This pointer 
won't be used for any other devices and sgmii will always be created for 
any MT7531 variants, so it's always going to point to 
mt7531_create_sgmii when priv->id is ID_MT7531. So you're introducing a 
new pointer just to be able to call mt7531_create_sgmii() on 
mt7530-mdio.c from mt7530.c.

On mt753x_setup(), if priv->create_sgmii is pointing to something it 
will now run whatever it points to with two arguments. One being the 
priv table and the other being mt7531_dual_sgmii_supported() which 
returns 1 or 0 by looking at the very same priv table. That looks bad. 
What could be done instead is introduce a new field on the priv table 
that keeps the information of whether port 5 on the MT7531 switch is 
SGMII or not.

A similar logic is already there on the U-Boot MediaTek ethernet driver.

https://github.com/u-boot/u-boot/blob/a94ab561e2f49a80d8579930e840b810ab1a1330/drivers/net/mtk_eth.c#L903

So this patch fixes the issue with the only consideration being changing 
as less lines of code as possible. And that's okay. We can make the 
least amount of changes to fix the issue first, then improve the driver. 
But there's nothing new made on the driver after the commit that caused 
this issue, backportability to the stable trees is a non-issue. So why 
not do it properly the first time?

Whatever the outcome with this patch is, on my upcoming patch series, I 
intend to move mt7531_create_sgmii to mt7530.c. Then introduce 
priv->p5_sgmii to get rid of mt7531_dual_sgmii_supported().

Arınç

On Sun, Apr 16, 2023 at 04:48:23PM +0300, Arınç ÜNAL wrote:
> On 16.04.2023 15:08, Daniel Golle wrote:
> > There are two variants of the MT7531 switch IC which got different
> > features (and pins) regarding port 5:
> >   * MT7531AE: SGMII/1000Base-X/2500Base-X SerDes PCS
> >   * MT7531BE: RGMII
> > 
> > Moving the creation of the SerDes PCS from mt753x_setup to mt7530_probe
> > with commit 6de285229773 ("net: dsa: mt7530: move SGMII PCS creation
> > to mt7530_probe function") works fine for MT7531AE which got two
> > instances of mtk-pcs-lynxi, however, MT7531BE requires mt7531_pll_setup
> > to setup clocks before the single PCS on port 6 (usually used as CPU
> > port) starts to work and hence the PCS creation failed on MT7531BE.
> > 
> > Fix this by introducing a pointer to mt7531_create_sgmii function in
> > struct mt7530_priv and call it again at the end of mt753x_setup like it
> > was before commit 6de285229773 ("net: dsa: mt7530: move SGMII PCS
> > creation to mt7530_probe function").
> > 
> > Fixes: 6de285229773 ("net: dsa: mt7530: move SGMII PCS creation to mt7530_probe function")
> > Signed-off-by: Daniel Golle <daniel@makrotopia.org>
> 
> I'll put my 2 cents about the patch along with responding to your points on
> the other thread here.
> 
> > Why don't we use my original solution [1] which has some advantages:
> > 
> >  * It doesn't requrire additional export of mt7530_regmap_bus
> > 
> >  * It doesn't move PCS creation to mt7530.c, hence PCS_MTK_LYNXI is
> >    only required for MDIO-connected switches
> >    (with your patch we would have to move the dependency on PCS_MTK_LYNXI
> >    from NET_DSA_MT7530_MDIO to NET_DSA_MT7530)
> 
> Maybe this is what should happen. Maybe the PCS creation (and therefore
> mt7530_regmap_bus) should be on the core driver. Both are on the MDIO driver
> for the sole reason of only the devices on the MDIO driver currently using
> it. It's not an MDIO-specific operation as far as I can tell. Having it on
> the core driver would make more sense in the long run.

Which "long run" are you talking about?
regmap creation is bus-specific, and so is the existence of LynxI PCS.
There simply aren't any MMIO-connected switches which come with that IP.
And I strongly doubt there ever will be. And even if, why should we now
prepare for an entirely speculative future? If it actually happens, ie.
in case there is going to be a new SoC with MMIO-connected switch which
does comes with LynxI PCS (e.g. for port 5 only) we can still move the
code.

> 
> > 
> >  * It doesn't expose the dysfunctional SerDes PCS for port 5 on MT7531BE
> >    This will still fail and hence result in probing on MT7531 to exit
> >    prematurely, preventing the switch driver from being loaded.
> >    Before 9ecc00164dc23 ("net: dsa: mt7530: refactor SGMII PCS creation")
> >    the return value of mtk_pcs_lynxi_create was ignored, now it isn't...
> 
> Ok, so checking whether port 5 is SGMII or not on the PCS creation code
> should be done on the same patch that fixes this issue.
> 
> > 
> >  * It changes much less in terms of LoC
> 
> I'd rather prefer a better logic than the "least amount of changes possible"
> approach.
> 
> Let's analyse what this patch does:
> 
> With this patch, mt7531_create_sgmii() is run after mt7530_setup_mdio is
> run, under mt753x_setup(). mt7531_pll_setup() and, as the last requirement,
> mt7530_setup_mdio() must be run to be able to create the PCS instances. That
> also means running mt7530_free_irq_common must be avoided since the device
> uses MDIO so mt7530_free_mdio_irq needs to be run too.
> 
> While probing the driver, the priv->create_sgmii pointer will be made to
> point to mt7531_create_sgmii, if MT7531 is detected. Why? This pointer won't
> be used for any other devices and sgmii will always be created for any
> MT7531 variants, so it's always going to point to mt7531_create_sgmii when
> priv->id is ID_MT7531. So you're introducing a new pointer just to be able
> to call mt7531_create_sgmii() on mt7530-mdio.c from mt7530.c.
> 
> On mt753x_setup(), if priv->create_sgmii is pointing to something it will
> now run whatever it points to with two arguments. One being the priv table
> and the other being mt7531_dual_sgmii_supported() which returns 1 or 0 by
> looking at the very same priv table. That looks bad. What could be done
> instead is introduce a new field on the priv table that keeps the
> information of whether port 5 on the MT7531 switch is SGMII or not.

Yes, and on a 64-bit system that means 8 bytes of memory for each instance.
Exporting a function or const implies significantly more overhead, and
it would not be as nicely limited in scope as a function pointer would be.

There are no other users in the kernel of the const you would export in
your variant of the fix, so why have it exported?

> 
> A similar logic is already there on the U-Boot MediaTek ethernet driver.
> 
> https://github.com/u-boot/u-boot/blob/a94ab561e2f49a80d8579930e840b810ab1a1330/drivers/net/mtk_eth.c#L903
> 
> So this patch fixes the issue with the only consideration being changing as
> less lines of code as possible.

You are ignore two more important arguments:
 * It doesn't requrire additional export of mt7530_regmap_bus
   (which would imply significantly more storage overhead compared to
   an additional function pointer in a priv struct)

 * It doesn't move PCS creation to mt7530.c, hence PCS_MTK_LYNXI is
   only required for MDIO-connected switches
   (with your patch we would have to move the dependency on PCS_MTK_LYNXI
   from NET_DSA_MT7530_MDIO to NET_DSA_MT7530)

> And that's okay. We can make the least
> amount of changes to fix the issue first, then improve the driver. But
> there's nothing new made on the driver after the commit that caused this
> issue, backportability to the stable trees is a non-issue. So why not do it
> properly the first time?

Most of all I'd rather have it fixed before net-next is merged to Linus'
tree and also before net-next will close again.

However, I also simply don't see what would be more "proper" about your
solution.

> 
> Whatever the outcome with this patch is, on my upcoming patch series, I
> intend to move mt7531_create_sgmii to mt7530.c. Then introduce
> priv->p5_sgmii to get rid of mt7531_dual_sgmii_supported().

What is the argument for that?

There is not a single MMIO-connected switch which comes with LynxI PCS.
(see above)

Imho we should rather try to work into the opposite direction and move
more code only used on either MDIO or MMIO from core to the
bus-specific drivers. If needed we can even split them more, eg. have
different modules for MT7530 and MT7531, so that even the driver for
MDIO-connected MT7530 would not require MTK_PCS_LYNXI.

In that sense I'm a big fan of the structure of the mt76 wireless
driver: Have a core module for shared helper functions and then
device-specific driver modules. Unfortunately many if not most drivers
are doing the exact opposite approach, ie. having some abstration layer
which will always need to be extended and changed with every
unforeseeable new hardware to be supported which just results in lots
of overhead and is a burden to maintain. You can see that in the rt2x00
wireless driver which I also worked on a lot: Most of the abstractions
aren't even useful with any of the latest hardware generations.

tl;dr: What's wrong with moving functions specific to either variant
(MMIO vs. MDIO) into the corresponding modules and keeping the core
slim and really only cover shared functionality? This is also why I
originally wanted the names of files and Kconfig symbols to reflect the
supported hardware rather than the supported bus-type -- I've changed
that upon your request and now believe I should have argued more
clearly why I made my choice like I did...

On 16/04/2023 17:18, Daniel Golle wrote:
> On Sun, Apr 16, 2023 at 04:48:23PM +0300, Arınç ÜNAL wrote:
>> On 16.04.2023 15:08, Daniel Golle wrote:
>>> There are two variants of the MT7531 switch IC which got different
>>> features (and pins) regarding port 5:
>>>    * MT7531AE: SGMII/1000Base-X/2500Base-X SerDes PCS
>>>    * MT7531BE: RGMII
>>>
>>> Moving the creation of the SerDes PCS from mt753x_setup to mt7530_probe
>>> with commit 6de285229773 ("net: dsa: mt7530: move SGMII PCS creation
>>> to mt7530_probe function") works fine for MT7531AE which got two
>>> instances of mtk-pcs-lynxi, however, MT7531BE requires mt7531_pll_setup
>>> to setup clocks before the single PCS on port 6 (usually used as CPU
>>> port) starts to work and hence the PCS creation failed on MT7531BE.
>>>
>>> Fix this by introducing a pointer to mt7531_create_sgmii function in
>>> struct mt7530_priv and call it again at the end of mt753x_setup like it
>>> was before commit 6de285229773 ("net: dsa: mt7530: move SGMII PCS
>>> creation to mt7530_probe function").
>>>
>>> Fixes: 6de285229773 ("net: dsa: mt7530: move SGMII PCS creation to mt7530_probe function")
>>> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
>>
>> I'll put my 2 cents about the patch along with responding to your points on
>> the other thread here.
>>
>>> Why don't we use my original solution [1] which has some advantages:
>>>
>>>   * It doesn't requrire additional export of mt7530_regmap_bus
>>>
>>>   * It doesn't move PCS creation to mt7530.c, hence PCS_MTK_LYNXI is
>>>     only required for MDIO-connected switches
>>>     (with your patch we would have to move the dependency on PCS_MTK_LYNXI
>>>     from NET_DSA_MT7530_MDIO to NET_DSA_MT7530)
>>
>> Maybe this is what should happen. Maybe the PCS creation (and therefore
>> mt7530_regmap_bus) should be on the core driver. Both are on the MDIO driver
>> for the sole reason of only the devices on the MDIO driver currently using
>> it. It's not an MDIO-specific operation as far as I can tell. Having it on
>> the core driver would make more sense in the long run.
> 
> Which "long run" are you talking about?
> regmap creation is bus-specific, and so is the existence of LynxI PCS.
> There simply aren't any MMIO-connected switches which come with that IP.
> And I strongly doubt there ever will be. And even if, why should we now
> prepare for an entirely speculative future? If it actually happens, ie.
> in case there is going to be a new SoC with MMIO-connected switch which
> does comes with LynxI PCS (e.g. for port 5 only) we can still move the
> code.

Makes sense.

> 
>>
>>>
>>>   * It doesn't expose the dysfunctional SerDes PCS for port 5 on MT7531BE
>>>     This will still fail and hence result in probing on MT7531 to exit
>>>     prematurely, preventing the switch driver from being loaded.
>>>     Before 9ecc00164dc23 ("net: dsa: mt7530: refactor SGMII PCS creation")
>>>     the return value of mtk_pcs_lynxi_create was ignored, now it isn't...
>>
>> Ok, so checking whether port 5 is SGMII or not on the PCS creation code
>> should be done on the same patch that fixes this issue.
>>
>>>
>>>   * It changes much less in terms of LoC
>>
>> I'd rather prefer a better logic than the "least amount of changes possible"
>> approach.
>>
>> Let's analyse what this patch does:
>>
>> With this patch, mt7531_create_sgmii() is run after mt7530_setup_mdio is
>> run, under mt753x_setup(). mt7531_pll_setup() and, as the last requirement,
>> mt7530_setup_mdio() must be run to be able to create the PCS instances. That
>> also means running mt7530_free_irq_common must be avoided since the device
>> uses MDIO so mt7530_free_mdio_irq needs to be run too.
>>
>> While probing the driver, the priv->create_sgmii pointer will be made to
>> point to mt7531_create_sgmii, if MT7531 is detected. Why? This pointer won't
>> be used for any other devices and sgmii will always be created for any
>> MT7531 variants, so it's always going to point to mt7531_create_sgmii when
>> priv->id is ID_MT7531. So you're introducing a new pointer just to be able
>> to call mt7531_create_sgmii() on mt7530-mdio.c from mt7530.c.
>>
>> On mt753x_setup(), if priv->create_sgmii is pointing to something it will
>> now run whatever it points to with two arguments. One being the priv table
>> and the other being mt7531_dual_sgmii_supported() which returns 1 or 0 by
>> looking at the very same priv table. That looks bad. What could be done
>> instead is introduce a new field on the priv table that keeps the
>> information of whether port 5 on the MT7531 switch is SGMII or not.
> 
> Yes, and on a 64-bit system that means 8 bytes of memory for each instance.
> Exporting a function or const implies significantly more overhead, and
> it would not be as nicely limited in scope as a function pointer would be.
> 
> There are no other users in the kernel of the const you would export in
> your variant of the fix, so why have it exported?
> 
>>
>> A similar logic is already there on the U-Boot MediaTek ethernet driver.
>>
>> https://github.com/u-boot/u-boot/blob/a94ab561e2f49a80d8579930e840b810ab1a1330/drivers/net/mtk_eth.c#L903
>>
>> So this patch fixes the issue with the only consideration being changing as
>> less lines of code as possible.
> 
> You are ignore two more important arguments:
>   * It doesn't requrire additional export of mt7530_regmap_bus
>     (which would imply significantly more storage overhead compared to
>     an additional function pointer in a priv struct)
> 
>   * It doesn't move PCS creation to mt7530.c, hence PCS_MTK_LYNXI is
>     only required for MDIO-connected switches
>     (with your patch we would have to move the dependency on PCS_MTK_LYNXI
>     from NET_DSA_MT7530_MDIO to NET_DSA_MT7530)

Understood.

> 
>> And that's okay. We can make the least
>> amount of changes to fix the issue first, then improve the driver. But
>> there's nothing new made on the driver after the commit that caused this
>> issue, backportability to the stable trees is a non-issue. So why not do it
>> properly the first time?
> 
> Most of all I'd rather have it fixed before net-next is merged to Linus'
> tree and also before net-next will close again.
> 
> However, I also simply don't see what would be more "proper" about your
> solution.

Nothing. Your patch here is perfectly fine after reading your points. 
One thing I'd like to ask is, if I understand correctly, instead of 
exporting mt7531_create_sgmii(), defining a pointer that points to it 
causes less overhead?

The current patch looks very similar to exporting a function. Instead of 
putting EXPORT_SYMBOL_GPL and declaring the function prototype on the 
header file, you declare a function pointer on the priv structure, then 
assign it to the function.

> 
>>
>> Whatever the outcome with this patch is, on my upcoming patch series, I
>> intend to move mt7531_create_sgmii to mt7530.c. Then introduce
>> priv->p5_sgmii to get rid of mt7531_dual_sgmii_supported().
> 
> What is the argument for that?

Nothing for moving mt7531_create_sgmii() but I think introducing 
priv->p5_sgmii with later patches is in the clear?

> 
> There is not a single MMIO-connected switch which comes with LynxI PCS.
> (see above)
> 
> Imho we should rather try to work into the opposite direction and move
> more code only used on either MDIO or MMIO from core to the
> bus-specific drivers. If needed we can even split them more, eg. have
> different modules for MT7530 and MT7531, so that even the driver for
> MDIO-connected MT7530 would not require MTK_PCS_LYNXI.

Interesting, I may work on this in the future. This could benefit my 
folks too.

> 
> In that sense I'm a big fan of the structure of the mt76 wireless
> driver: Have a core module for shared helper functions and then
> device-specific driver modules. Unfortunately many if not most drivers
> are doing the exact opposite approach, ie. having some abstration layer
> which will always need to be extended and changed with every
> unforeseeable new hardware to be supported which just results in lots
> of overhead and is a burden to maintain. You can see that in the rt2x00
> wireless driver which I also worked on a lot: Most of the abstractions
> aren't even useful with any of the latest hardware generations.
> 
> tl;dr: What's wrong with moving functions specific to either variant
> (MMIO vs. MDIO) into the corresponding modules and keeping the core
> slim and really only cover shared functionality? This is also why I
> originally wanted the names of files and Kconfig symbols to reflect the
> supported hardware rather than the supported bus-type -- I've changed
> that upon your request and now believe I should have argued more
> clearly why I made my choice like I did...

Ah that makes sense. I'd like to address this. I was already planning to 
to do some renaming on the driver. Please, allow me to do the work.

I intend to do this slightly different than your initial patch series 
though. Like calling the core driver core, instead of common, and making 
it selectable, and only imply the MT7530 MDIO driver.

We could split the core and mdio/mmio drivers in a way that there's just 
the core, then the device-specific driver modules. This would mean 
splitting the MT7530 MDIO driver to MT7530 and MT7531 along with moving 
code from core to these two drivers. I believe this would apply for 
MT7988 too. There would be a bit of reused code but it should follow the 
idea of what you say above. Then we can configure the kconfig accordingly.

Arınç

On Mon, Apr 17, 2023 at 12:28:57AM +0300, Arınç ÜNAL wrote:
> On 16/04/2023 17:18, Daniel Golle wrote:
> > On Sun, Apr 16, 2023 at 04:48:23PM +0300, Arınç ÜNAL wrote:
> > > On 16.04.2023 15:08, Daniel Golle wrote:
> > > > [...]
> > > >   * It doesn't move PCS creation to mt7530.c, hence PCS_MTK_LYNXI is
> > > >     only required for MDIO-connected switches
> > > >     (with your patch we would have to move the dependency on PCS_MTK_LYNXI
> > > >     from NET_DSA_MT7530_MDIO to NET_DSA_MT7530)
> > > 
> > > Maybe this is what should happen. Maybe the PCS creation (and therefore
> > > mt7530_regmap_bus) should be on the core driver. Both are on the MDIO driver
> > > for the sole reason of only the devices on the MDIO driver currently using
> > > it. It's not an MDIO-specific operation as far as I can tell. Having it on
> > > the core driver would make more sense in the long run.
> > 
> > Which "long run" are you talking about?
> > regmap creation is bus-specific, and so is the existence of LynxI PCS.
> > There simply aren't any MMIO-connected switches which come with that IP.
> > And I strongly doubt there ever will be. And even if, why should we now
> > prepare for an entirely speculative future? If it actually happens, ie.
> > in case there is going to be a new SoC with MMIO-connected switch which
> > does comes with LynxI PCS (e.g. for port 5 only) we can still move the
> > code.
> 
> Makes sense.


> > > [...]
> > > 
> > > A similar logic is already there on the U-Boot MediaTek ethernet driver.
> > > 
> > > https://github.com/u-boot/u-boot/blob/a94ab561e2f49a80d8579930e840b810ab1a1330/drivers/net/mtk_eth.c#L903
> > > 
> > > So this patch fixes the issue with the only consideration being changing as
> > > less lines of code as possible.
> > 
> > You are ignore two more important arguments:
> >   * It doesn't requrire additional export of mt7530_regmap_bus
> >     (which would imply significantly more storage overhead compared to
> >     an additional function pointer in a priv struct)
> > 
> >   * It doesn't move PCS creation to mt7530.c, hence PCS_MTK_LYNXI is
> >     only required for MDIO-connected switches
> >     (with your patch we would have to move the dependency on PCS_MTK_LYNXI
> >     from NET_DSA_MT7530_MDIO to NET_DSA_MT7530)
> 
> Understood.

> > > And that's okay. We can make the least
> > > amount of changes to fix the issue first, then improve the driver. But
> > > there's nothing new made on the driver after the commit that caused this
> > > issue, backportability to the stable trees is a non-issue. So why not do it
> > > properly the first time?
> > 
> > Most of all I'd rather have it fixed before net-next is merged to Linus'
> > tree and also before net-next will close again.
> > 
> > However, I also simply don't see what would be more "proper" about your
> > solution.
> 
> Nothing. Your patch here is perfectly fine after reading your points. One
> thing I'd like to ask is, if I understand correctly, instead of exporting
> mt7531_create_sgmii(), defining a pointer that points to it causes less
> overhead?

Yes. Depending on build configuration and debugging options an exported
function or constant will require different amounts of storage, ie.
function name and parameter prototypes need to be stored in the kernel
symbol table, any module calling the exporting functions and in the
exporting modules ELF header, the latter being the most significant.
Even if kernel modules aren't used and it's all built-in the overhead
is still more than a few bytes for the struct member definition as well
as the growth of the per-instance allocated struct member itself --
especially given that I have only heard about one board using two
MT7531AE, most boards use exactly one of them.

> 
> The current patch looks very similar to exporting a function. Instead of
> putting EXPORT_SYMBOL_GPL and declaring the function prototype on the header
> file, you declare a function pointer on the priv structure, then assign it
> to the function.

The effect is similar, just limited in scope as a caller needs to have
access to the priv struct (opposed to an EXPORT_SYMBOL* which will
make the function or const available globally).

Also note that exporting mt7531_create_sgmii() would not work equally
well as the result would be a hard dependency of NET_DSA_MT7530 on
NET_DSA_MT7530_MDIO for the exported function being linkable.
The function pointer has the advantage that it can be set to NULL and
in that way we can model a weak dependency.

> 
> > 
> > > 
> > > Whatever the outcome with this patch is, on my upcoming patch series, I
> > > intend to move mt7531_create_sgmii to mt7530.c. Then introduce
> > > priv->p5_sgmii to get rid of mt7531_dual_sgmii_supported().
> > 
> > What is the argument for that?
> 
> Nothing for moving mt7531_create_sgmii() but I think introducing
> priv->p5_sgmii with later patches is in the clear?

Yes, I agree that introducing priv->p5_sgmii can make sense, given that
it would prevent having to export mt7531_dual_sgmii_supported() or
passing its return value as a function parameter, or even just having
to call it many times.

Regarding this current patch (see subject), do you still agree that we
should apply it as-is and then either you or me will prepare another
series further refactoring the driver?


> 
> > 
> > There is not a single MMIO-connected switch which comes with LynxI PCS.
> > (see above)
> > 
> > Imho we should rather try to work into the opposite direction and move
> > more code only used on either MDIO or MMIO from core to the
> > bus-specific drivers. If needed we can even split them more, eg. have
> > different modules for MT7530 and MT7531, so that even the driver for
> > MDIO-connected MT7530 would not require MTK_PCS_LYNXI.
> 
> Interesting, I may work on this in the future. This could benefit my folks
> too.
> 
> > 
> > In that sense I'm a big fan of the structure of the mt76 wireless
> > driver: Have a core module for shared helper functions and then
> > device-specific driver modules. Unfortunately many if not most drivers
> > are doing the exact opposite approach, ie. having some abstration layer
> > which will always need to be extended and changed with every
> > unforeseeable new hardware to be supported which just results in lots
> > of overhead and is a burden to maintain. You can see that in the rt2x00
> > wireless driver which I also worked on a lot: Most of the abstractions
> > aren't even useful with any of the latest hardware generations.
> > 
> > tl;dr: What's wrong with moving functions specific to either variant
> > (MMIO vs. MDIO) into the corresponding modules and keeping the core
> > slim and really only cover shared functionality? This is also why I
> > originally wanted the names of files and Kconfig symbols to reflect the
> > supported hardware rather than the supported bus-type -- I've changed
> > that upon your request and now believe I should have argued more
> > clearly why I made my choice like I did...
> 
> Ah that makes sense. I'd like to address this. I was already planning to to
> do some renaming on the driver. Please, allow me to do the work.

Sure, your efforts are appreciated, and I'll happily review and test
your suggestions.

> 
> I intend to do this slightly different than your initial patch series
> though. Like calling the core driver core, instead of common, and making it
> selectable, and only imply the MT7530 MDIO driver.
> 
> We could split the core and mdio/mmio drivers in a way that there's just the
> core, then the device-specific driver modules. This would mean splitting the
> MT7530 MDIO driver to MT7530 and MT7531 along with moving code from core to
> these two drivers. I believe this would apply for MT7988 too. There would be
> a bit of reused code but it should follow the idea of what you say above.
> Then we can configure the kconfig accordingly.
> 
> Arınç

On 17.04.2023 14:39, Daniel Golle wrote:
> On Mon, Apr 17, 2023 at 12:28:57AM +0300, Arınç ÜNAL wrote:
>> On 16/04/2023 17:18, Daniel Golle wrote:
>>> On Sun, Apr 16, 2023 at 04:48:23PM +0300, Arınç ÜNAL wrote:
>>>> On 16.04.2023 15:08, Daniel Golle wrote:
>>>>> [...]
>>>>>    * It doesn't move PCS creation to mt7530.c, hence PCS_MTK_LYNXI is
>>>>>      only required for MDIO-connected switches
>>>>>      (with your patch we would have to move the dependency on PCS_MTK_LYNXI
>>>>>      from NET_DSA_MT7530_MDIO to NET_DSA_MT7530)
>>>>
>>>> Maybe this is what should happen. Maybe the PCS creation (and therefore
>>>> mt7530_regmap_bus) should be on the core driver. Both are on the MDIO driver
>>>> for the sole reason of only the devices on the MDIO driver currently using
>>>> it. It's not an MDIO-specific operation as far as I can tell. Having it on
>>>> the core driver would make more sense in the long run.
>>>
>>> Which "long run" are you talking about?
>>> regmap creation is bus-specific, and so is the existence of LynxI PCS.
>>> There simply aren't any MMIO-connected switches which come with that IP.
>>> And I strongly doubt there ever will be. And even if, why should we now
>>> prepare for an entirely speculative future? If it actually happens, ie.
>>> in case there is going to be a new SoC with MMIO-connected switch which
>>> does comes with LynxI PCS (e.g. for port 5 only) we can still move the
>>> code.
>>
>> Makes sense.
> 
> 
>>>> [...]
>>>>
>>>> A similar logic is already there on the U-Boot MediaTek ethernet driver.
>>>>
>>>> https://github.com/u-boot/u-boot/blob/a94ab561e2f49a80d8579930e840b810ab1a1330/drivers/net/mtk_eth.c#L903
>>>>
>>>> So this patch fixes the issue with the only consideration being changing as
>>>> less lines of code as possible.
>>>
>>> You are ignore two more important arguments:
>>>    * It doesn't requrire additional export of mt7530_regmap_bus
>>>      (which would imply significantly more storage overhead compared to
>>>      an additional function pointer in a priv struct)
>>>
>>>    * It doesn't move PCS creation to mt7530.c, hence PCS_MTK_LYNXI is
>>>      only required for MDIO-connected switches
>>>      (with your patch we would have to move the dependency on PCS_MTK_LYNXI
>>>      from NET_DSA_MT7530_MDIO to NET_DSA_MT7530)
>>
>> Understood.
> 
>>>> And that's okay. We can make the least
>>>> amount of changes to fix the issue first, then improve the driver. But
>>>> there's nothing new made on the driver after the commit that caused this
>>>> issue, backportability to the stable trees is a non-issue. So why not do it
>>>> properly the first time?
>>>
>>> Most of all I'd rather have it fixed before net-next is merged to Linus'
>>> tree and also before net-next will close again.
>>>
>>> However, I also simply don't see what would be more "proper" about your
>>> solution.
>>
>> Nothing. Your patch here is perfectly fine after reading your points. One
>> thing I'd like to ask is, if I understand correctly, instead of exporting
>> mt7531_create_sgmii(), defining a pointer that points to it causes less
>> overhead?
> 
> Yes. Depending on build configuration and debugging options an exported
> function or constant will require different amounts of storage, ie.
> function name and parameter prototypes need to be stored in the kernel
> symbol table, any module calling the exporting functions and in the
> exporting modules ELF header, the latter being the most significant.
> Even if kernel modules aren't used and it's all built-in the overhead
> is still more than a few bytes for the struct member definition as well
> as the growth of the per-instance allocated struct member itself --
> especially given that I have only heard about one board using two
> MT7531AE, most boards use exactly one of them.
> 
>>
>> The current patch looks very similar to exporting a function. Instead of
>> putting EXPORT_SYMBOL_GPL and declaring the function prototype on the header
>> file, you declare a function pointer on the priv structure, then assign it
>> to the function.
> 
> The effect is similar, just limited in scope as a caller needs to have
> access to the priv struct (opposed to an EXPORT_SYMBOL* which will
> make the function or const available globally).
> 
> Also note that exporting mt7531_create_sgmii() would not work equally
> well as the result would be a hard dependency of NET_DSA_MT7530 on
> NET_DSA_MT7530_MDIO for the exported function being linkable.
> The function pointer has the advantage that it can be set to NULL and
> in that way we can model a weak dependency.

Very nice, thanks for the explanation.

> 
>>
>>>
>>>>
>>>> Whatever the outcome with this patch is, on my upcoming patch series, I
>>>> intend to move mt7531_create_sgmii to mt7530.c. Then introduce
>>>> priv->p5_sgmii to get rid of mt7531_dual_sgmii_supported().
>>>
>>> What is the argument for that?
>>
>> Nothing for moving mt7531_create_sgmii() but I think introducing
>> priv->p5_sgmii with later patches is in the clear?
> 
> Yes, I agree that introducing priv->p5_sgmii can make sense, given that
> it would prevent having to export mt7531_dual_sgmii_supported() or
> passing its return value as a function parameter, or even just having
> to call it many times.
> 
> Regarding this current patch (see subject), do you still agree that we
> should apply it as-is and then either you or me will prepare another
> series further refactoring the driver?

This patch is fine as is and should go in, I will base my upcoming RFC 
series on top of this.

Acked-by: Arınç ÜNAL <arinc.unal@arinc9.com>

> 
> 
>>
>>>
>>> There is not a single MMIO-connected switch which comes with LynxI PCS.
>>> (see above)
>>>
>>> Imho we should rather try to work into the opposite direction and move
>>> more code only used on either MDIO or MMIO from core to the
>>> bus-specific drivers. If needed we can even split them more, eg. have
>>> different modules for MT7530 and MT7531, so that even the driver for
>>> MDIO-connected MT7530 would not require MTK_PCS_LYNXI.
>>
>> Interesting, I may work on this in the future. This could benefit my folks
>> too.
>>
>>>
>>> In that sense I'm a big fan of the structure of the mt76 wireless
>>> driver: Have a core module for shared helper functions and then
>>> device-specific driver modules. Unfortunately many if not most drivers
>>> are doing the exact opposite approach, ie. having some abstration layer
>>> which will always need to be extended and changed with every
>>> unforeseeable new hardware to be supported which just results in lots
>>> of overhead and is a burden to maintain. You can see that in the rt2x00
>>> wireless driver which I also worked on a lot: Most of the abstractions
>>> aren't even useful with any of the latest hardware generations.
>>>
>>> tl;dr: What's wrong with moving functions specific to either variant
>>> (MMIO vs. MDIO) into the corresponding modules and keeping the core
>>> slim and really only cover shared functionality? This is also why I
>>> originally wanted the names of files and Kconfig symbols to reflect the
>>> supported hardware rather than the supported bus-type -- I've changed
>>> that upon your request and now believe I should have argued more
>>> clearly why I made my choice like I did...
>>
>> Ah that makes sense. I'd like to address this. I was already planning to to
>> do some renaming on the driver. Please, allow me to do the work.
> 
> Sure, your efforts are appreciated, and I'll happily review and test
> your suggestions.

Sounds good, till then.

Arınç