mail archive of the barebox mailing list
 help / color / mirror / Atom feed
* [PATCH v1 1/2] include/phy: add driver_data to resume more of kernel code
@ 2021-10-12 10:08 Oleksij Rempel
  2021-10-12 10:08 ` [PATCH v1 2/2] net: phy: micrel: port clock select support Oleksij Rempel
  0 siblings, 1 reply; 8+ messages in thread
From: Oleksij Rempel @ 2021-10-12 10:08 UTC (permalink / raw)
  To: barebox; +Cc: Oleksij Rempel

Add driver_data pointer to be able to port more of kernel code for
micrel phy.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 include/linux/phy.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/phy.h b/include/linux/phy.h
index a4cda3e28d..202eeb4d44 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -212,6 +212,7 @@ struct phy_device {
  * phy_id_mask: Defines the important bits of the phy_id
  * features: A list of features (speed, duplex, etc) supported
  *   by this PHY
+ * @driver_data: Static driver data
  *
  * The drivers must implement config_aneg and read_status.  All
  * other functions are optional. Note that none of these
@@ -225,6 +226,7 @@ struct phy_driver {
 	u32 phy_id;
 	unsigned int phy_id_mask;
 	u32 features;
+	const void *driver_data;
 
 	/*
 	 * Called to initialize the PHY,
-- 
2.30.2


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH v1 2/2] net: phy: micrel: port clock select support
  2021-10-12 10:08 [PATCH v1 1/2] include/phy: add driver_data to resume more of kernel code Oleksij Rempel
@ 2021-10-12 10:08 ` Oleksij Rempel
  2021-10-12 17:12   ` Trent Piepho
  0 siblings, 1 reply; 8+ messages in thread
From: Oleksij Rempel @ 2021-10-12 10:08 UTC (permalink / raw)
  To: barebox; +Cc: Oleksij Rempel

Port devicetree based clock select support from kernel micrel driver (v5.15-rc1).
This support is needed to make netboot work on boards with PHY node and
"rmii-ref" property.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/phy/micrel.c | 101 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 100 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index ea193c84a7..958abdd9d6 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -14,6 +14,7 @@
 
 #include <common.h>
 #include <init.h>
+#include <linux/clk.h>
 #include <linux/mii.h>
 #include <linux/ethtool.h>
 #include <linux/phy.h>
@@ -38,6 +39,10 @@
 /* PHY Control 1 */
 #define MII_KSZPHY_CTRL_1			0x1e
 
+/* PHY Control 2 / PHY Control (if no PHY Control 1) */
+#define MII_KSZPHY_CTRL_2			0x1f
+#define KSZPHY_RMII_REF_CLK_SEL			BIT(7)
+
 /* Write/read to/from extended registers */
 #define MII_KSZPHY_EXTREG                       0x0b
 #define KSZPHY_EXTREG_WRITE                     0x8000
@@ -52,6 +57,20 @@
 
 #define PS_TO_REG				200
 
+struct kszphy_type {
+	bool has_rmii_ref_clk_sel;
+};
+
+struct kszphy_priv {
+	const struct kszphy_type *type;
+	bool rmii_ref_clk_sel;
+	bool rmii_ref_clk_sel_val;
+};
+
+static const struct kszphy_type ksz8081_type = {
+	.has_rmii_ref_clk_sel	= true,
+};
+
 static int kszphy_extended_write(struct phy_device *phydev,
 				u32 regnum, u16 val)
 {
@@ -66,6 +85,22 @@ static int kszphy_extended_read(struct phy_device *phydev,
 	return phy_read(phydev, MII_KSZPHY_EXTREG_READ);
 }
 
+static int kszphy_rmii_clk_sel(struct phy_device *phydev, bool val)
+{
+	int ctrl;
+
+	ctrl = phy_read(phydev, MII_KSZPHY_CTRL);
+	if (ctrl < 0)
+		return ctrl;
+
+	if (val)
+		ctrl |= KSZPHY_RMII_REF_CLK_SEL;
+	else
+		ctrl &= ~KSZPHY_RMII_REF_CLK_SEL;
+
+	return phy_write(phydev, MII_KSZPHY_CTRL, ctrl);
+}
+
 /* Handle LED mode, shift = position of first led mode bit, usually 4 or 14 */
 static int kszphy_led_mode(struct phy_device *phydev, int reg, int shift)
 {
@@ -83,6 +118,24 @@ static int kszphy_led_mode(struct phy_device *phydev, int reg, int shift)
 	return 0;
 }
 
+/* Some config bits need to be set again on resume, handle them here. */
+static int kszphy_config_reset(struct phy_device *phydev)
+{
+	struct kszphy_priv *priv = phydev->priv;
+	int ret;
+
+	if (priv->rmii_ref_clk_sel) {
+		ret = kszphy_rmii_clk_sel(phydev, priv->rmii_ref_clk_sel_val);
+		if (ret) {
+			dev_err(&phydev->dev,
+				   "failed to set rmii reference clock\n");
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
 static int kszphy_config_init(struct phy_device *phydev)
 {
 	kszphy_led_mode(phydev, MII_KSZPHY_CTRL_1, 14);
@@ -96,7 +149,7 @@ static int ksz8021_config_init(struct phy_device *phydev)
 
 	kszphy_led_mode(phydev, MII_KSZPHY_CTRL, 4);
 
-	return 0;
+	return kszphy_config_reset(phydev);
 }
 
 static int ks8051_config_init(struct phy_device *phydev)
@@ -468,6 +521,50 @@ static int ksz8873mll_config_init(struct phy_device *phydev)
 	return 0;
 }
 
+static int kszphy_probe(struct phy_device *phydev)
+{
+	struct device_d *dev = &phydev->dev;
+	struct device_node *np = dev->device_node;
+	struct phy_driver *drv = to_phy_driver(dev->driver);
+	const struct kszphy_type *type = drv->driver_data;
+	struct kszphy_priv *priv;
+	struct clk *clk;
+
+	priv = xzalloc(sizeof(*priv));
+
+	phydev->priv = priv;
+
+	priv->type = type;
+
+	clk = clk_get(dev, "rmii-ref");
+	/* NOTE: clk may be NULL if building without CONFIG_HAVE_CLK */
+	if (!IS_ERR_OR_NULL(clk)) {
+		unsigned long rate = clk_get_rate(clk);
+		bool rmii_ref_clk_sel_25_mhz;
+
+		priv->rmii_ref_clk_sel = type->has_rmii_ref_clk_sel;
+		rmii_ref_clk_sel_25_mhz = of_property_read_bool(np,
+				"micrel,rmii-reference-clock-select-25-mhz");
+
+		if (rate > 24500000 && rate < 25500000) {
+			priv->rmii_ref_clk_sel_val = rmii_ref_clk_sel_25_mhz;
+		} else if (rate > 49500000 && rate < 50500000) {
+			priv->rmii_ref_clk_sel_val = !rmii_ref_clk_sel_25_mhz;
+		} else {
+			dev_err(dev, "Clock rate out of range: %ld\n", rate);
+			return -EINVAL;
+		}
+	}
+
+	/* Support legacy board-file configuration */
+	if (phydev->dev_flags & MICREL_PHY_50MHZ_CLK) {
+		priv->rmii_ref_clk_sel = true;
+		priv->rmii_ref_clk_sel_val = true;
+	}
+
+	return 0;
+}
+
 static struct phy_driver ksphy_driver[] = {
 {
 	.phy_id		= PHY_ID_KS8737,
@@ -517,7 +614,9 @@ static struct phy_driver ksphy_driver[] = {
 	.phy_id		= PHY_ID_KSZ8081,
 	.phy_id_mask	= MICREL_PHY_ID_MASK,
 	.drv.name	= "Micrel KSZ8081/91",
+	.driver_data	= &ksz8081_type,
 	.features	= (PHY_BASIC_FEATURES | SUPPORTED_Pause),
+	.probe		= kszphy_probe,
 	.config_init	= ksz8021_config_init,
 	.config_aneg	= genphy_config_aneg,
 	.read_status	= genphy_read_status,
-- 
2.30.2


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v1 2/2] net: phy: micrel: port clock select support
  2021-10-12 10:08 ` [PATCH v1 2/2] net: phy: micrel: port clock select support Oleksij Rempel
@ 2021-10-12 17:12   ` Trent Piepho
  2021-10-13  7:28     ` Oleksij Rempel
  0 siblings, 1 reply; 8+ messages in thread
From: Trent Piepho @ 2021-10-12 17:12 UTC (permalink / raw)
  To: Oleksij Rempel; +Cc: Barebox List

On Tue, Oct 12, 2021 at 3:10 AM Oleksij Rempel <o.rempel@pengutronix.de> wrote:
>
> Port devicetree based clock select support from kernel micrel driver (v5.15-rc1).
> This support is needed to make netboot work on boards with PHY node and
> "rmii-ref" property.

Existing boards use a phy fixup to handle this case, e.g. fsl,imx6ull-14x14-evk:

static int nxp_imx6ull_evk_init(void)
{
         phy_register_fixup_for_uid(PHY_ID_KSZ8081, MICREL_PHY_ID_MASK,
                       ksz8081_phy_fixup);

static int ksz8081_phy_fixup(struct phy_device *dev)
{
       phy_write(dev, 0x1f, 0x8190);
       phy_write(dev, 0x16, 0x202);

I thought about extending micrel driver like this when I added support
for NXP imx6ul (single L, not double L as above) dev kit but decided
it was a lot of code for what ends up being one register write for
boards using a ksz8081 and the barebox phy fixup concept, which
handles this in only a couple lines, was easier.

Also look at ks8051_config_init(), it checks phydev->dev_flags to set
this same bit.  But AFAICT, dev_flags is not used in Barebox.

> +/* PHY Control 2 / PHY Control (if no PHY Control 1) */
> +#define MII_KSZPHY_CTRL_2                      0x1f
> +#define KSZPHY_RMII_REF_CLK_SEL                        BIT(7)

These are already in this file as MII_KSZPHY_CTRL and KSZ8051_RMII_50MHZ_CLK.

> +struct kszphy_type {
> +       bool has_rmii_ref_clk_sel;
> +};
> +
> +struct kszphy_priv {
> +       const struct kszphy_type *type;

type does not appear to be used from the state struct.

> +       bool rmii_ref_clk_sel;
> +       bool rmii_ref_clk_sel_val;

All you need is a single flag to indicate the KSZ8051_RMII_50MHZ_CLK
bit should be set after a reset.  Otherwise it can be left unset,
which is the default value.

> +static const struct kszphy_type ksz8081_type = {
> +       .has_rmii_ref_clk_sel   = true,
> +};

Note that not just KSZ8081 has this bit.  Also KSZ8021, KSZ8031, and
KSZ8051, which has the existing different method to handle it, as
described earlier.

> +static int kszphy_rmii_clk_sel(struct phy_device *phydev, bool val)
> +{
> +       int ctrl;
> +
> +       ctrl = phy_read(phydev, MII_KSZPHY_CTRL);
> +       if (ctrl < 0)
> +               return ctrl;
> +
> +       if (val)
> +               ctrl |= KSZPHY_RMII_REF_CLK_SEL;
> +       else
> +               ctrl &= ~KSZPHY_RMII_REF_CLK_SEL;
> +
> +       return phy_write(phydev, MII_KSZPHY_CTRL, ctrl);
> +}

phy_set_bits(phydev, MII_KSZPHY_CTRL, KSZ8051_RMII_50MHZ_CLK);

> +
> +       /* Support legacy board-file configuration */
> +       if (phydev->dev_flags & MICREL_PHY_50MHZ_CLK) {
> +               priv->rmii_ref_clk_sel = true;
> +               priv->rmii_ref_clk_sel_val = true;
> +       }

Can't code in ksz8051_config_init be removed then?

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v1 2/2] net: phy: micrel: port clock select support
  2021-10-12 17:12   ` Trent Piepho
@ 2021-10-13  7:28     ` Oleksij Rempel
  2021-10-13  8:48       ` Trent Piepho
  0 siblings, 1 reply; 8+ messages in thread
From: Oleksij Rempel @ 2021-10-13  7:28 UTC (permalink / raw)
  To: Trent Piepho; +Cc: Barebox List

Hi Trent,

thank you for your review.

On Tue, Oct 12, 2021 at 10:12:13AM -0700, Trent Piepho wrote:
> On Tue, Oct 12, 2021 at 3:10 AM Oleksij Rempel <o.rempel@pengutronix.de> wrote:
> >
> > Port devicetree based clock select support from kernel micrel driver (v5.15-rc1).
> > This support is needed to make netboot work on boards with PHY node and
> > "rmii-ref" property.
> 
> Existing boards use a phy fixup to handle this case, e.g. fsl,imx6ull-14x14-evk:
> 
> static int nxp_imx6ull_evk_init(void)
> {
>          phy_register_fixup_for_uid(PHY_ID_KSZ8081, MICREL_PHY_ID_MASK,
>                        ksz8081_phy_fixup);
> 
> static int ksz8081_phy_fixup(struct phy_device *dev)
> {
>        phy_write(dev, 0x1f, 0x8190);
>        phy_write(dev, 0x16, 0x202);
> 
> I thought about extending micrel driver like this when I added support
> for NXP imx6ul (single L, not double L as above) dev kit but decided
> it was a lot of code for what ends up being one register write for
> boards using a ksz8081 and the barebox phy fixup concept, which
> handles this in only a couple lines, was easier.

No, I would hardly not recommend to do it this way :)

> Also look at ks8051_config_init(), it checks phydev->dev_flags to set
> this same bit.  But AFAICT, dev_flags is not used in Barebox.

Good point. I'll remove it.
> 
> > +/* PHY Control 2 / PHY Control (if no PHY Control 1) */
> > +#define MII_KSZPHY_CTRL_2                      0x1f
> > +#define KSZPHY_RMII_REF_CLK_SEL                        BIT(7)
> 
> These are already in this file as MII_KSZPHY_CTRL and KSZ8051_RMII_50MHZ_CLK.
> 
> > +struct kszphy_type {
> > +       bool has_rmii_ref_clk_sel;
> > +};
> > +
> > +struct kszphy_priv {
> > +       const struct kszphy_type *type;
> 
> type does not appear to be used from the state struct.
> 
> > +       bool rmii_ref_clk_sel;
> > +       bool rmii_ref_clk_sel_val;
> 
> All you need is a single flag to indicate the KSZ8051_RMII_50MHZ_CLK
> bit should be set after a reset.  Otherwise it can be left unset,
> which is the default value.
> 
> > +static const struct kszphy_type ksz8081_type = {
> > +       .has_rmii_ref_clk_sel   = true,
> > +};
> 
> Note that not just KSZ8081 has this bit.  Also KSZ8021, KSZ8031, and
> KSZ8051, which has the existing different method to handle it, as
> described earlier.

ok, i'll sync all of this PHYs with the state of the kernel driver.
The board fixups should be removed by someone who can confirm it.

> > +static int kszphy_rmii_clk_sel(struct phy_device *phydev, bool val)
> > +{
> > +       int ctrl;
> > +
> > +       ctrl = phy_read(phydev, MII_KSZPHY_CTRL);
> > +       if (ctrl < 0)
> > +               return ctrl;
> > +
> > +       if (val)
> > +               ctrl |= KSZPHY_RMII_REF_CLK_SEL;
> > +       else
> > +               ctrl &= ~KSZPHY_RMII_REF_CLK_SEL;
> > +
> > +       return phy_write(phydev, MII_KSZPHY_CTRL, ctrl);
> > +}
> 
> phy_set_bits(phydev, MII_KSZPHY_CTRL, KSZ8051_RMII_50MHZ_CLK);

No, it should be synced with kernel not in the opposite way.

> > +
> > +       /* Support legacy board-file configuration */
> > +       if (phydev->dev_flags & MICREL_PHY_50MHZ_CLK) {
> > +               priv->rmii_ref_clk_sel = true;
> > +               priv->rmii_ref_clk_sel_val = true;
> > +       }
> 
> Can't code in ksz8051_config_init be removed then?

good point, i'll remove it.

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v1 2/2] net: phy: micrel: port clock select support
  2021-10-13  7:28     ` Oleksij Rempel
@ 2021-10-13  8:48       ` Trent Piepho
  2021-10-13 10:23         ` Oleksij Rempel
  0 siblings, 1 reply; 8+ messages in thread
From: Trent Piepho @ 2021-10-13  8:48 UTC (permalink / raw)
  To: Oleksij Rempel; +Cc: Barebox List

On Wed, Oct 13, 2021 at 12:29 AM Oleksij Rempel <o.rempel@pengutronix.de> wrote:
> > Note that not just KSZ8081 has this bit.  Also KSZ8021, KSZ8031, and
> > KSZ8051, which has the existing different method to handle it, as
> > described earlier.
>
> ok, i'll sync all of this PHYs with the state of the kernel driver.
> The board fixups should be removed by someone who can confirm it.

If you copy the entire kernel driver you will massively bloat this
code with stuff that is totally unused.

> > > +static int kszphy_rmii_clk_sel(struct phy_device *phydev, bool val)
> > > +{
> > > +       int ctrl;
> > > +
> > > +       ctrl = phy_read(phydev, MII_KSZPHY_CTRL);
> > > +       if (ctrl < 0)
> > > +               return ctrl;
> > > +
> > > +       if (val)
> > > +               ctrl |= KSZPHY_RMII_REF_CLK_SEL;
> > > +       else
> > > +               ctrl &= ~KSZPHY_RMII_REF_CLK_SEL;
> > > +
> > > +       return phy_write(phydev, MII_KSZPHY_CTRL, ctrl);
> > > +}
> >
> > phy_set_bits(phydev, MII_KSZPHY_CTRL, KSZ8051_RMII_50MHZ_CLK);
>
> No, it should be synced with kernel not in the opposite way.

If you want the code to match, then improve the kernel code rather
than make Barebox code worse.  phy_set_bits, phy_clear_bits and
phy_modify exist.  One can not use them if a kernel driver has not
been refactored to use them?

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v1 2/2] net: phy: micrel: port clock select support
  2021-10-13  8:48       ` Trent Piepho
@ 2021-10-13 10:23         ` Oleksij Rempel
  2021-10-13 10:43           ` Trent Piepho
  0 siblings, 1 reply; 8+ messages in thread
From: Oleksij Rempel @ 2021-10-13 10:23 UTC (permalink / raw)
  To: Trent Piepho; +Cc: Barebox List

On Wed, Oct 13, 2021 at 01:48:20AM -0700, Trent Piepho wrote:
> On Wed, Oct 13, 2021 at 12:29 AM Oleksij Rempel <o.rempel@pengutronix.de> wrote:
> > > Note that not just KSZ8081 has this bit.  Also KSZ8021, KSZ8031, and
> > > KSZ8051, which has the existing different method to handle it, as
> > > described earlier.
> >
> > ok, i'll sync all of this PHYs with the state of the kernel driver.
> > The board fixups should be removed by someone who can confirm it.
> 
> If you copy the entire kernel driver you will massively bloat this
> code with stuff that is totally unused.

Yes.

> > > > +static int kszphy_rmii_clk_sel(struct phy_device *phydev, bool val)
> > > > +{
> > > > +       int ctrl;
> > > > +
> > > > +       ctrl = phy_read(phydev, MII_KSZPHY_CTRL);
> > > > +       if (ctrl < 0)
> > > > +               return ctrl;
> > > > +
> > > > +       if (val)
> > > > +               ctrl |= KSZPHY_RMII_REF_CLK_SEL;
> > > > +       else
> > > > +               ctrl &= ~KSZPHY_RMII_REF_CLK_SEL;
> > > > +
> > > > +       return phy_write(phydev, MII_KSZPHY_CTRL, ctrl);
> > > > +}
> > >
> > > phy_set_bits(phydev, MII_KSZPHY_CTRL, KSZ8051_RMII_50MHZ_CLK);
> >
> > No, it should be synced with kernel not in the opposite way.
> 
> If you want the code to match, then improve the kernel code rather
> than make Barebox code worse.  phy_set_bits, phy_clear_bits and
> phy_modify exist.  One can not use them if a kernel driver has not
> been refactored to use them?

Sure. Can you please show example of the kszphy_rmii_clk_sel() refactoring.
And how much optimization will it introduce? Please compared with disassembled
part. 

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v1 2/2] net: phy: micrel: port clock select support
  2021-10-13 10:23         ` Oleksij Rempel
@ 2021-10-13 10:43           ` Trent Piepho
  2021-10-13 11:02             ` Oleksij Rempel
  0 siblings, 1 reply; 8+ messages in thread
From: Trent Piepho @ 2021-10-13 10:43 UTC (permalink / raw)
  To: Oleksij Rempel; +Cc: Barebox List

On Wed, Oct 13, 2021 at 3:23 AM Oleksij Rempel <o.rempel@pengutronix.de> wrote:
>
> On Wed, Oct 13, 2021 at 01:48:20AM -0700, Trent Piepho wrote:
> > On Wed, Oct 13, 2021 at 12:29 AM Oleksij Rempel <o.rempel@pengutronix.de> wrote:
> > > > Note that not just KSZ8081 has this bit.  Also KSZ8021, KSZ8031, and
> > > > KSZ8051, which has the existing different method to handle it, as
> > > > described earlier.
> > >
> > > ok, i'll sync all of this PHYs with the state of the kernel driver.
> > > The board fixups should be removed by someone who can confirm it.
> >
> > If you copy the entire kernel driver you will massively bloat this
> > code with stuff that is totally unused.
>
> Yes.

Can you get network support into a Barebox that fits in IMX8 OCRAM?
Bloat matters. There are things barebox can not do anymore because it
has grown too large.

> > > > > +static int kszphy_rmii_clk_sel(struct phy_device *phydev, bool val)
> > > > > +{
> > > > > +       int ctrl;
> > > > > +
> > > > > +       ctrl = phy_read(phydev, MII_KSZPHY_CTRL);
> > > > > +       if (ctrl < 0)
> > > > > +               return ctrl;
> > > > > +
> > > > > +       if (val)
> > > > > +               ctrl |= KSZPHY_RMII_REF_CLK_SEL;
> > > > > +       else
> > > > > +               ctrl &= ~KSZPHY_RMII_REF_CLK_SEL;
> > > > > +
> > > > > +       return phy_write(phydev, MII_KSZPHY_CTRL, ctrl);
> > > > > +}
> > > >
> > > > phy_set_bits(phydev, MII_KSZPHY_CTRL, KSZ8051_RMII_50MHZ_CLK);
> > >
> > > No, it should be synced with kernel not in the opposite way.
> >
> > If you want the code to match, then improve the kernel code rather
> > than make Barebox code worse.  phy_set_bits, phy_clear_bits and
> > phy_modify exist.  One can not use them if a kernel driver has not
> > been refactored to use them?
>
> Sure. Can you please show example of the kszphy_rmii_clk_sel() refactoring.
> And how much optimization will it introduce? Please compared with disassembled
> part.

struct kszphy_priv *priv = phydev->priv;
if (priv->set_rmii_clk_sel)
    phy_set_bits(phydev, MII_KSZPHY_CTRL, KSZ8051_RMII_50MHZ_CLK);

But did you really need me to provide that example?

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v1 2/2] net: phy: micrel: port clock select support
  2021-10-13 10:43           ` Trent Piepho
@ 2021-10-13 11:02             ` Oleksij Rempel
  0 siblings, 0 replies; 8+ messages in thread
From: Oleksij Rempel @ 2021-10-13 11:02 UTC (permalink / raw)
  To: Trent Piepho; +Cc: Barebox List

On Wed, Oct 13, 2021 at 03:43:46AM -0700, Trent Piepho wrote:
> On Wed, Oct 13, 2021 at 3:23 AM Oleksij Rempel <o.rempel@pengutronix.de> wrote:
> >
> > On Wed, Oct 13, 2021 at 01:48:20AM -0700, Trent Piepho wrote:
> > > On Wed, Oct 13, 2021 at 12:29 AM Oleksij Rempel <o.rempel@pengutronix.de> wrote:
> > > > > Note that not just KSZ8081 has this bit.  Also KSZ8021, KSZ8031, and
> > > > > KSZ8051, which has the existing different method to handle it, as
> > > > > described earlier.
> > > >
> > > > ok, i'll sync all of this PHYs with the state of the kernel driver.
> > > > The board fixups should be removed by someone who can confirm it.
> > >
> > > If you copy the entire kernel driver you will massively bloat this
> > > code with stuff that is totally unused.
> >
> > Yes.
> 
> Can you get network support into a Barebox that fits in IMX8 OCRAM?
> Bloat matters. There are things barebox can not do anymore because it
> has grown too large.

Yes. barebox depends on devicetree and most of special PHY
configuration is already provided by devicetree. Your initial suggestion
was to duplicate information provide by DT and put it in to the board
file. Which is exactly opposite of what you wont.

At same time, fixups introduce more issue:
- they are applied on multiple PHYs detected on one board. For example
  boards with switches.
- they are applied on external HW. For example USB adapter with same
  PHY.

> > > > > > +static int kszphy_rmii_clk_sel(struct phy_device *phydev, bool val)
> > > > > > +{
> > > > > > +       int ctrl;
> > > > > > +
> > > > > > +       ctrl = phy_read(phydev, MII_KSZPHY_CTRL);
> > > > > > +       if (ctrl < 0)
> > > > > > +               return ctrl;
> > > > > > +
> > > > > > +       if (val)
> > > > > > +               ctrl |= KSZPHY_RMII_REF_CLK_SEL;
> > > > > > +       else
> > > > > > +               ctrl &= ~KSZPHY_RMII_REF_CLK_SEL;
> > > > > > +
> > > > > > +       return phy_write(phydev, MII_KSZPHY_CTRL, ctrl);
> > > > > > +}
> > > > >
> > > > > phy_set_bits(phydev, MII_KSZPHY_CTRL, KSZ8051_RMII_50MHZ_CLK);
> > > >
> > > > No, it should be synced with kernel not in the opposite way.
> > >
> > > If you want the code to match, then improve the kernel code rather
> > > than make Barebox code worse.  phy_set_bits, phy_clear_bits and
> > > phy_modify exist.  One can not use them if a kernel driver has not
> > > been refactored to use them?
> >
> > Sure. Can you please show example of the kszphy_rmii_clk_sel() refactoring.
> > And how much optimization will it introduce? Please compared with disassembled
> > part.
> 
> struct kszphy_priv *priv = phydev->priv;
> if (priv->set_rmii_clk_sel)
>     phy_set_bits(phydev, MII_KSZPHY_CTRL, KSZ8051_RMII_50MHZ_CLK);

Sorry, it is not correct. It should be:
if (priv->set_rmii_clk_sel)
	phy_set_bits(phydev, MII_KSZPHY_CTRL, KSZ8051_RMII_50MHZ_CLK);
else
	phy_clear_bits(phydev, MII_KSZPHY_CTRL, KSZ8051_RMII_50MHZ_CLK);

There are KSZ PHY variants with opposite meaning of the same bit. So, at
the end I see no advantage of changing it.

> But did you really need me to provide that example?

No, i wont to see what is so special for you about this driver.
There should be reason why thing which you blame, make no sense for me.

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2021-10-13 12:35 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-12 10:08 [PATCH v1 1/2] include/phy: add driver_data to resume more of kernel code Oleksij Rempel
2021-10-12 10:08 ` [PATCH v1 2/2] net: phy: micrel: port clock select support Oleksij Rempel
2021-10-12 17:12   ` Trent Piepho
2021-10-13  7:28     ` Oleksij Rempel
2021-10-13  8:48       ` Trent Piepho
2021-10-13 10:23         ` Oleksij Rempel
2021-10-13 10:43           ` Trent Piepho
2021-10-13 11:02             ` Oleksij Rempel

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox