[PATCH 0/4] net: dsa: b53: Enable internal GPHY on BCM63268

[PATCH 0/4] net: dsa: b53: Enable internal GPHY on BCM63268

[Kyle Hendry]

Some BCM63268 bootloaders do not enable the internal PHYs by default.
This patch series adds functionality for the switch driver to 
configure the gigabit ethernet PHY. 

Signed-off-by: Kyle Hendry <kylehendrydev@gmail.com>

Kyle Hendry (4):
  net: dsa: b53: Indicate which BCM63268 port is GPHY
  net: dsa: b53: mmap: Add gphy control register as a resource
  net: dsa: b53: Add phy_enable(), phy_disable() methods
  net: dsa: b53: mmap: Implement phy_enable for BCM63268 gphy

 drivers/net/dsa/b53/b53_common.c  |  9 +++++++
 drivers/net/dsa/b53/b53_mmap.c    | 42 ++++++++++++++++++++++++++++++-
 drivers/net/dsa/b53/b53_priv.h    |  3 +++
 drivers/net/dsa/b53/b53_regs.h    |  7 ++++++
 include/linux/platform_data/b53.h |  1 +
 5 files changed, 61 insertions(+), 1 deletion(-)

-- 
2.43.0

[PATCH 1/4] net: dsa: b53: Indicate which BCM63268 port is GPHY

[Kyle Hendry]

Add a gphy mask member and initialize for BCM63268.

Signed-off-by: Kyle Hendry <kylehendrydev@gmail.com>
---
 drivers/net/dsa/b53/b53_common.c | 3 +++
 drivers/net/dsa/b53/b53_priv.h   | 1 +
 2 files changed, 4 insertions(+)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index 79dc77835681..06739aea328d 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -2319,6 +2319,7 @@ struct b53_chip_data {
 	const char *dev_name;
 	u16 vlans;
 	u16 enabled_ports;
+	u16 internal_gphy_mask;
 	u8 imp_port;
 	u8 cpu_port;
 	u8 vta_regs[3];
@@ -2466,6 +2467,7 @@ static const struct b53_chip_data b53_switch_chips[] = {
 		.dev_name = "BCM63268",
 		.vlans = 4096,
 		.enabled_ports = 0, /* pdata must provide them */
+		.internal_gphy_mask = BIT(3),
 		.arl_bins = 4,
 		.arl_buckets = 1024,
 		.imp_port = 8,
@@ -2642,6 +2644,7 @@ static int b53_switch_init(struct b53_device *dev)
 			dev->num_vlans = chip->vlans;
 			dev->num_arl_bins = chip->arl_bins;
 			dev->num_arl_buckets = chip->arl_buckets;
+			dev->internal_gphy_mask = chip->internal_gphy_mask;
 			break;
 		}
 	}
diff --git a/drivers/net/dsa/b53/b53_priv.h b/drivers/net/dsa/b53/b53_priv.h
index 9e9b5bc0c5d6..cd565efbdec2 100644
--- a/drivers/net/dsa/b53/b53_priv.h
+++ b/drivers/net/dsa/b53/b53_priv.h
@@ -128,6 +128,7 @@ struct b53_device {
 
 	/* used ports mask */
 	u16 enabled_ports;
+	u16 internal_gphy_mask;
 	unsigned int imp_port;
 
 	/* connect specific data */
-- 
2.43.0

[PATCH 2/4] net: dsa: b53: mmap: Add gphy control register as a resource

[Kyle Hendry]

Give b53 driver access to the gphy control register by passing
it in as an optional second reg in the device tree.

Signed-off-by: Kyle Hendry <kylehendrydev@gmail.com>
---
 drivers/net/dsa/b53/b53_mmap.c    | 8 +++++++-
 include/linux/platform_data/b53.h | 1 +
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/b53/b53_mmap.c b/drivers/net/dsa/b53/b53_mmap.c
index c687360a5b7f..8157f9871133 100644
--- a/drivers/net/dsa/b53/b53_mmap.c
+++ b/drivers/net/dsa/b53/b53_mmap.c
@@ -28,6 +28,7 @@
 
 struct b53_mmap_priv {
 	void __iomem *regs;
+	void __iomem *gphy_ctrl;
 };
 
 static int b53_mmap_read8(struct b53_device *dev, u8 page, u8 reg, u8 *val)
@@ -251,7 +252,7 @@ static int b53_mmap_probe_of(struct platform_device *pdev,
 	struct device_node *of_ports, *of_port;
 	struct device *dev = &pdev->dev;
 	struct b53_platform_data *pdata;
-	void __iomem *mem;
+	void __iomem *mem, *gphy_ctrl;
 
 	mem = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(mem))
@@ -266,6 +267,10 @@ static int b53_mmap_probe_of(struct platform_device *pdev,
 	pdata->chip_id = (u32)(unsigned long)device_get_match_data(dev);
 	pdata->big_endian = of_property_read_bool(np, "big-endian");
 
+	gphy_ctrl = devm_platform_ioremap_resource(pdev, 1);
+	if (!IS_ERR(gphy_ctrl))
+		pdata->gphy_ctrl = gphy_ctrl;
+
 	of_ports = of_get_child_by_name(np, "ports");
 	if (!of_ports) {
 		dev_err(dev, "no ports child node found\n");
@@ -312,6 +317,7 @@ static int b53_mmap_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	priv->regs = pdata->regs;
+	priv->gphy_ctrl = pdata->gphy_ctrl;
 
 	dev = b53_switch_alloc(&pdev->dev, &b53_mmap_ops, priv);
 	if (!dev)
diff --git a/include/linux/platform_data/b53.h b/include/linux/platform_data/b53.h
index 6f6fed2b171d..ed73287e8ac5 100644
--- a/include/linux/platform_data/b53.h
+++ b/include/linux/platform_data/b53.h
@@ -32,6 +32,7 @@ struct b53_platform_data {
 	/* only used by MMAP'd driver */
 	unsigned big_endian:1;
 	void __iomem *regs;
+	void __iomem *gphy_ctrl;
 };
 
 #endif
-- 
2.43.0

[PATCH 3/4] net: dsa: b53: Add phy_enable(), phy_disable() methods

[Kyle Hendry]

Add phy enable/disable to b53 ops to be called when
enabling/disabling ports.

Signed-off-by: Kyle Hendry <kylehendrydev@gmail.com>
---
 drivers/net/dsa/b53/b53_common.c | 6 ++++++
 drivers/net/dsa/b53/b53_priv.h   | 2 ++
 2 files changed, 8 insertions(+)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index 06739aea328d..e5b0cc3428e5 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -592,6 +592,9 @@ int b53_enable_port(struct dsa_switch *ds, int port, struct phy_device *phy)
 	b53_port_set_mcast_flood(dev, port, true);
 	b53_port_set_learning(dev, port, false);
 
+	if (dev->ops->phy_enable)
+		dev->ops->phy_enable(dev, port);
+
 	if (dev->ops->irq_enable)
 		ret = dev->ops->irq_enable(dev, port);
 	if (ret)
@@ -630,6 +633,9 @@ void b53_disable_port(struct dsa_switch *ds, int port)
 	reg |= PORT_CTRL_RX_DISABLE | PORT_CTRL_TX_DISABLE;
 	b53_write8(dev, B53_CTRL_PAGE, B53_PORT_CTRL(port), reg);
 
+	if (dev->ops->phy_disable)
+		dev->ops->phy_disable(dev, port);
+
 	if (dev->ops->irq_disable)
 		dev->ops->irq_disable(dev, port);
 }
diff --git a/drivers/net/dsa/b53/b53_priv.h b/drivers/net/dsa/b53/b53_priv.h
index cd565efbdec2..6ffc36c1b2c3 100644
--- a/drivers/net/dsa/b53/b53_priv.h
+++ b/drivers/net/dsa/b53/b53_priv.h
@@ -45,6 +45,8 @@ struct b53_io_ops {
 	int (*phy_write16)(struct b53_device *dev, int addr, int reg, u16 value);
 	int (*irq_enable)(struct b53_device *dev, int port);
 	void (*irq_disable)(struct b53_device *dev, int port);
+	void (*phy_enable)(struct b53_device *dev, int port);
+	void (*phy_disable)(struct b53_device *dev, int port);
 	void (*phylink_get_caps)(struct b53_device *dev, int port,
 				 struct phylink_config *config);
 	struct phylink_pcs *(*phylink_mac_select_pcs)(struct b53_device *dev,
-- 
2.43.0

[PATCH 4/4] net: dsa: b53: mmap: Implement phy_enable for BCM63268 gphy

[Kyle Hendry]

Add register defines for gphy control register. When gphy is
enabled, disable low power mode.

Signed-off-by: Kyle Hendry <kylehendrydev@gmail.com>
---
 drivers/net/dsa/b53/b53_mmap.c | 34 ++++++++++++++++++++++++++++++++++
 drivers/net/dsa/b53/b53_regs.h |  7 +++++++
 2 files changed, 41 insertions(+)

diff --git a/drivers/net/dsa/b53/b53_mmap.c b/drivers/net/dsa/b53/b53_mmap.c
index 8157f9871133..ac0b8fc7eb4e 100644
--- a/drivers/net/dsa/b53/b53_mmap.c
+++ b/drivers/net/dsa/b53/b53_mmap.c
@@ -230,6 +230,38 @@ static int b53_mmap_phy_write16(struct b53_device *dev, int addr, int reg,
 	return -EIO;
 }
 
+static void bcm63268_gphy_set(struct b53_device *dev, bool enable)
+{
+	struct b53_mmap_priv *priv = dev->priv;
+	void __iomem *gphy_ctrl = priv->gphy_ctrl;
+	u32 val;
+
+	val = ioread32be(gphy_ctrl);
+
+	if (enable)
+		val &= ~(GPHY_CTRL_IDDQ_BIAS | GPHY_CTRL_LOW_PWR);
+	else
+		val |= GPHY_CTRL_IDDQ_BIAS | GPHY_CTRL_LOW_PWR;
+
+	iowrite32be(val, gphy_ctrl);
+}
+
+static void b53_mmap_phy_enable(struct b53_device *dev, int port)
+{
+	struct b53_mmap_priv *priv = dev->priv;
+
+	if ((dev->internal_gphy_mask & BIT(port)) && priv->gphy_ctrl)
+		bcm63268_gphy_set(dev, true);
+}
+
+static void b53_mmap_phy_disable(struct b53_device *dev, int port)
+{
+	struct b53_mmap_priv *priv = dev->priv;
+
+	if ((dev->internal_gphy_mask & BIT(port)) && priv->gphy_ctrl)
+		bcm63268_gphy_set(dev, false);
+}
+
 static const struct b53_io_ops b53_mmap_ops = {
 	.read8 = b53_mmap_read8,
 	.read16 = b53_mmap_read16,
@@ -243,6 +275,8 @@ static const struct b53_io_ops b53_mmap_ops = {
 	.write64 = b53_mmap_write64,
 	.phy_read16 = b53_mmap_phy_read16,
 	.phy_write16 = b53_mmap_phy_write16,
+	.phy_enable = b53_mmap_phy_enable,
+	.phy_disable = b53_mmap_phy_disable,
 };
 
 static int b53_mmap_probe_of(struct platform_device *pdev,
diff --git a/drivers/net/dsa/b53/b53_regs.h b/drivers/net/dsa/b53/b53_regs.h
index bfbcb66bef66..9607b28bbb86 100644
--- a/drivers/net/dsa/b53/b53_regs.h
+++ b/drivers/net/dsa/b53/b53_regs.h
@@ -525,4 +525,11 @@
 /* CFP Control Register with ports map (8 bit) */
 #define B53_CFP_CTRL			0x00
 
+/*************************************************************************
+ * Gigabit PHY Control Register
+ *************************************************************************/
+
+#define GPHY_CTRL_IDDQ_BIAS		BIT(0)
+#define GPHY_CTRL_LOW_PWR		BIT(3)
+
 #endif /* !__B53_REGS_H */
-- 
2.43.0

Re: [PATCH 0/4] net: dsa: b53: Enable internal GPHY on BCM63268

[Florian Fainelli]

Hi Kyle,

On 2/5/25 20:30, Kyle Hendry wrote:
> Some BCM63268 bootloaders do not enable the internal PHYs by default.
> This patch series adds functionality for the switch driver to
> configure the gigabit ethernet PHY.
> 
> Signed-off-by: Kyle Hendry <kylehendrydev@gmail.com>

So the register address you are manipulating logically belongs in the 
GPIO block (GPIO_GPHY_CTRL) which has become quite a bit of a sundry 
here. I don't have a strong objection about the approach picked up here 
but we will need a Device Tree binding update describing the second (and 
optional) register range.

Thanks
-- 
Florian

Re: [PATCH 0/4] net: dsa: b53: Enable internal GPHY on BCM63268

[Andrew Lunn]

On Thu, Feb 06, 2025 at 10:15:50AM -0800, Florian Fainelli wrote:
> Hi Kyle,
> 
> On 2/5/25 20:30, Kyle Hendry wrote:
> > Some BCM63268 bootloaders do not enable the internal PHYs by default.
> > This patch series adds functionality for the switch driver to
> > configure the gigabit ethernet PHY.
> > 
> > Signed-off-by: Kyle Hendry <kylehendrydev@gmail.com>
> 
> So the register address you are manipulating logically belongs in the GPIO
> block (GPIO_GPHY_CTRL) which has become quite a bit of a sundry here. I
> don't have a strong objection about the approach picked up here but we will
> need a Device Tree binding update describing the second (and optional)
> register range.

Despite this being internal, is this actually a GPIO? Should it be
modelled as a GPIO line connected to a reset input on the PHY? It
would then nicely fit in the existing phylib handling of a PHY with a
GPIO reset line?

	Andrew

Re: [PATCH 0/4] net: dsa: b53: Enable internal GPHY on BCM63268

[Kyle Hendry]

On 2025-02-06 12:17, Andrew Lunn wrote:
> On Thu, Feb 06, 2025 at 10:15:50AM -0800, Florian Fainelli wrote:
>> Hi Kyle,
>>
>> On 2/5/25 20:30, Kyle Hendry wrote:
>>> Some BCM63268 bootloaders do not enable the internal PHYs by default.
>>> This patch series adds functionality for the switch driver to
>>> configure the gigabit ethernet PHY.
>>>
>>> Signed-off-by: Kyle Hendry <kylehendrydev@gmail.com>
>> So the register address you are manipulating logically belongs in the GPIO
>> block (GPIO_GPHY_CTRL) which has become quite a bit of a sundry here. I
>> don't have a strong objection about the approach picked up here but we will
>> need a Device Tree binding update describing the second (and optional)
>> register range.
> Despite this being internal, is this actually a GPIO? Should it be
> modelled as a GPIO line connected to a reset input on the PHY? It
> would then nicely fit in the existing phylib handling of a PHY with a
> GPIO reset line?
>
> 	Andrew
The main reason I took this approach is because a SF2 register has
similar bits and I wanted to be consistent with that driver. If it
makes more sense to treat these bits as GPIOs/clocks/resets then it
would make the implementation simpler.

Best Regards,
Kyle

Re: [PATCH 0/4] net: dsa: b53: Enable internal GPHY on BCM63268

[Vladimir Oltean]

On Thu, Feb 06, 2025 at 05:41:19PM -0800, Kyle Hendry wrote:
> On 2025-02-06 12:17, Andrew Lunn wrote:
> > On Thu, Feb 06, 2025 at 10:15:50AM -0800, Florian Fainelli wrote:
> > > Hi Kyle,
> > > 
> > > On 2/5/25 20:30, Kyle Hendry wrote:
> > > > Some BCM63268 bootloaders do not enable the internal PHYs by default.
> > > > This patch series adds functionality for the switch driver to
> > > > configure the gigabit ethernet PHY.
> > > > 
> > > > Signed-off-by: Kyle Hendry <kylehendrydev@gmail.com>
> > > So the register address you are manipulating logically belongs in the GPIO
> > > block (GPIO_GPHY_CTRL) which has become quite a bit of a sundry here. I
> > > don't have a strong objection about the approach picked up here but we will
> > > need a Device Tree binding update describing the second (and optional)
> > > register range.
> > Despite this being internal, is this actually a GPIO? Should it be
> > modelled as a GPIO line connected to a reset input on the PHY? It
> > would then nicely fit in the existing phylib handling of a PHY with a
> > GPIO reset line?
> > 
> > 	Andrew
> The main reason I took this approach is because a SF2 register has
> similar bits and I wanted to be consistent with that driver. If it
> makes more sense to treat these bits as GPIOs/clocks/resets then it
> would make the implementation simpler.

This has not always been clear, but we prefer handling components
unrelated to Ethernet switching outside of DSA, if at all possible.

Re: [PATCH 0/4] net: dsa: b53: Enable internal GPHY on BCM63268

[Florian Fainelli]

On 2/6/25 17:41, Kyle Hendry wrote:
> 
> On 2025-02-06 12:17, Andrew Lunn wrote:
>> On Thu, Feb 06, 2025 at 10:15:50AM -0800, Florian Fainelli wrote:
>>> Hi Kyle,
>>>
>>> On 2/5/25 20:30, Kyle Hendry wrote:
>>>> Some BCM63268 bootloaders do not enable the internal PHYs by default.
>>>> This patch series adds functionality for the switch driver to
>>>> configure the gigabit ethernet PHY.
>>>>
>>>> Signed-off-by: Kyle Hendry <kylehendrydev@gmail.com>
>>> So the register address you are manipulating logically belongs in the 
>>> GPIO
>>> block (GPIO_GPHY_CTRL) which has become quite a bit of a sundry here. I
>>> don't have a strong objection about the approach picked up here but 
>>> we will
>>> need a Device Tree binding update describing the second (and optional)
>>> register range.
>> Despite this being internal, is this actually a GPIO? Should it be
>> modelled as a GPIO line connected to a reset input on the PHY? It
>> would then nicely fit in the existing phylib handling of a PHY with a
>> GPIO reset line?
>>
>>     Andrew
> The main reason I took this approach is because a SF2 register has
> similar bits and I wanted to be consistent with that driver. If it
> makes more sense to treat these bits as GPIOs/clocks/resets then it
> would make the implementation simpler.

I don't think there is a need to go that far, and I don't think any of 
those abstractions work really well in the sense that they are neither 
clocks, nor resets, nor GPIOs, they are just enable bits for the power 
gating logic of the PHY, power domains would be the closest to what this 
is, but this is a very heavy handed approach with little benefit IMHO.

What we do need is document this register in the binding however.
-- 
Florian

Re: [PATCH 0/4] net: dsa: b53: Enable internal GPHY on BCM63268

[Andrew Lunn]

On Fri, Feb 07, 2025 at 08:44:31AM -0800, Florian Fainelli wrote:
> On 2/6/25 17:41, Kyle Hendry wrote:
> > 
> > On 2025-02-06 12:17, Andrew Lunn wrote:
> > > On Thu, Feb 06, 2025 at 10:15:50AM -0800, Florian Fainelli wrote:
> > > > Hi Kyle,
> > > > 
> > > > On 2/5/25 20:30, Kyle Hendry wrote:
> > > > > Some BCM63268 bootloaders do not enable the internal PHYs by default.
> > > > > This patch series adds functionality for the switch driver to
> > > > > configure the gigabit ethernet PHY.
> > > > > 
> > > > > Signed-off-by: Kyle Hendry <kylehendrydev@gmail.com>
> > > > So the register address you are manipulating logically belongs
> > > > in the GPIO
> > > > block (GPIO_GPHY_CTRL) which has become quite a bit of a sundry here. I
> > > > don't have a strong objection about the approach picked up here
> > > > but we will
> > > > need a Device Tree binding update describing the second (and optional)
> > > > register range.
> > > Despite this being internal, is this actually a GPIO? Should it be
> > > modelled as a GPIO line connected to a reset input on the PHY? It
> > > would then nicely fit in the existing phylib handling of a PHY with a
> > > GPIO reset line?
> > > 
> > >     Andrew
> > The main reason I took this approach is because a SF2 register has
> > similar bits and I wanted to be consistent with that driver. If it
> > makes more sense to treat these bits as GPIOs/clocks/resets then it
> > would make the implementation simpler.
> 
> I don't think there is a need to go that far, and I don't think any of those
> abstractions work really well in the sense that they are neither clocks, nor
> resets, nor GPIOs, they are just enable bits for the power gating logic of
> the PHY, power domains would be the closest to what this is, but this is a
> very heavy handed approach with little benefit IMHO.

O.K. The naming is not particularly helpful. It is in the GPIO block,
and named GPIO_GPHY_CTRL so it kind of sounds like a GPIO. If the
existing GPIO driver could expose it as a GPIO it would of been a lot
simpler.

If the SF2 has similar bits, could the SF2 code be shared?

	Andrew

Re: [PATCH 0/4] net: dsa: b53: Enable internal GPHY on BCM63268

[Florian Fainelli]

On 2/9/25 15:30, Andrew Lunn wrote:
> On Fri, Feb 07, 2025 at 08:44:31AM -0800, Florian Fainelli wrote:
>> On 2/6/25 17:41, Kyle Hendry wrote:
>>>
>>> On 2025-02-06 12:17, Andrew Lunn wrote:
>>>> On Thu, Feb 06, 2025 at 10:15:50AM -0800, Florian Fainelli wrote:
>>>>> Hi Kyle,
>>>>>
>>>>> On 2/5/25 20:30, Kyle Hendry wrote:
>>>>>> Some BCM63268 bootloaders do not enable the internal PHYs by default.
>>>>>> This patch series adds functionality for the switch driver to
>>>>>> configure the gigabit ethernet PHY.
>>>>>>
>>>>>> Signed-off-by: Kyle Hendry <kylehendrydev@gmail.com>
>>>>> So the register address you are manipulating logically belongs
>>>>> in the GPIO
>>>>> block (GPIO_GPHY_CTRL) which has become quite a bit of a sundry here. I
>>>>> don't have a strong objection about the approach picked up here
>>>>> but we will
>>>>> need a Device Tree binding update describing the second (and optional)
>>>>> register range.
>>>> Despite this being internal, is this actually a GPIO? Should it be
>>>> modelled as a GPIO line connected to a reset input on the PHY? It
>>>> would then nicely fit in the existing phylib handling of a PHY with a
>>>> GPIO reset line?
>>>>
>>>>      Andrew
>>> The main reason I took this approach is because a SF2 register has
>>> similar bits and I wanted to be consistent with that driver. If it
>>> makes more sense to treat these bits as GPIOs/clocks/resets then it
>>> would make the implementation simpler.
>>
>> I don't think there is a need to go that far, and I don't think any of those
>> abstractions work really well in the sense that they are neither clocks, nor
>> resets, nor GPIOs, they are just enable bits for the power gating logic of
>> the PHY, power domains would be the closest to what this is, but this is a
>> very heavy handed approach with little benefit IMHO.
> 
> O.K. The naming is not particularly helpful. It is in the GPIO block,
> and named GPIO_GPHY_CTRL so it kind of sounds like a GPIO. If the
> existing GPIO driver could expose it as a GPIO it would of been a lot
> simpler.

You are no stranger to what HW designers like to do: use whatever 
existing hardware block already exists to add random enable and status 
bits to control whatever they fancy that was not exposed before. This 
happens whenever SW/FW people don't push back and ask for proper 
compartmentalization and abstractions to be used. Sometimes layout and 
schedule also play a role, but more often than not, it's "just software" 
so you can update to to account for the fact that bit is there, and not 
here, right?

This one is exactly what happened: there was spare room in the decoding 
address space of the register block, so it was natural from there to add 
a 32-bit word that would hold the enable bits for the internal Gigabit 
PHY... Those bits are not GPIOs, they are just simple enable/control 
bits feeding an internal signal.

> 
> If the SF2 has similar bits, could the SF2 code be shared?

The SF2 single GPHY control register is different and more standardized 
in many ways, so I don't see much value in sharing that code. The 
SWITCH_REG register block in that case does follow a consistent layout 
across different product lines.
-- 
Florian