| Message ID | 20250206043055.177004-1-kylehendrydev@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | net: dsa: b53: Enable internal GPHY on BCM63268 | expand |
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
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
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
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.
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.
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
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.
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(-)