Re: [PATCH 3/3] net-next: dsa: add new driver for qca8xxx family

2016-09-13 Thread John Crispin 


On 13/09/2016 21:07, Andrew Lunn wrote:
>> Since the former alternative is prefered, we may want to remove the
>> latter soon from DSA. If this phy_port_map is needed for that case, it'd
>> be preferable not to add it.
> 
> O.K, so maybe we should solve it the device tree way:
> 
> 
> {
>   phy_port1: phy@0 {
>   reg = <0>;
>   };
> 
>   phy_port2: phy@1 {
>   reg = <1>;
>   };
> 
>   phy_port3: phy@2 {
>   reg = <2>;
>   };
> 
>   phy_port4: phy@3 {
>   reg = <3>;
>   };
> 
>   phy_port5: phy@4 {
>   reg = <4>;
>   };
> 
>   switch@0 {
>compatible = "qca,qca8337";
> 
>#address-cells = <1>;
>#size-cells = <0>;
>reg = <30>;
> 
>ports {
>port@11 {
>reg = <11>;
>label = "cpu";
>ethernet = <>;
>phy-mode = "rgmii";
>};
> 
>port@1 {
>reg = <1>;
>label = "lan1";
>  phy-handle = <_port1>;
>};
> 
>port@2 {
>reg = <2>;
>label = "lan2";
>  phy-handle = <_port2>;
>};
> 
>port@3 {
>reg = <3>;
>label = "lan3";
>  phy-handle = <_port3>;
>};
> 
>port@4 {
>reg = <4>;
>label = "lan4";
>  phy-handle = <_port4>;
>};
>};
>};
>};
> 
> and remove the phy_read() and phy_write() functions.
> 
> 
>Andrew
> 

Hi Andrew

ok, will give it a spin in the morning and add a note to the binding doc
explaining this. thanks for taking the time !

John

Re: [PATCH 3/3] net-next: dsa: add new driver for qca8xxx family

2016-09-13 Thread Andrew Lunn 
> Since the former alternative is prefered, we may want to remove the
> latter soon from DSA. If this phy_port_map is needed for that case, it'd
> be preferable not to add it.

O.K, so maybe we should solve it the device tree way:


{
phy_port1: phy@0 {
reg = <0>;
};

phy_port2: phy@1 {
reg = <1>;
};

phy_port3: phy@2 {
reg = <2>;
};

phy_port4: phy@3 {
reg = <3>;
};

phy_port5: phy@4 {
reg = <4>;
};

switch@0 {
   compatible = "qca,qca8337";

   #address-cells = <1>;
   #size-cells = <0>;
   reg = <30>;

   ports {
   port@11 {
   reg = <11>;
   label = "cpu";
   ethernet = <>;
   phy-mode = "rgmii";
   };

   port@1 {
   reg = <1>;
   label = "lan1";
   phy-handle = <_port1>;
   };

   port@2 {
   reg = <2>;
   label = "lan2";
   phy-handle = <_port2>;
   };

   port@3 {
   reg = <3>;
   label = "lan3";
   phy-handle = <_port3>;
   };

   port@4 {
   reg = <4>;
   label = "lan4";
   phy-handle = <_port4>;
   };
   };
   };
   };

and remove the phy_read() and phy_write() functions.


   Andrew

Re: [PATCH 3/3] net-next: dsa: add new driver for qca8xxx family

2016-09-13 Thread Florian Fainelli 
On 09/13/2016 11:07 AM, John Crispin wrote:
> 
> 
> On 13/09/2016 19:09, Florian Fainelli wrote:
>> On 09/13/2016 08:59 AM, Andrew Lunn wrote:
 Hi Andrew,

 this function does indeed duplicate the functionality of
 phy_ethtool_get_eee() with the small difference, that e->eee_active is
 also set which phy_ethtool_get_eee() does not set.

 dsa_slave_get_eee() will call phy_ethtool_get_eee() right after the
 get_eee() op has been called. would it be ok to move the code setting
 eee_active to  phy_ethtool_get_eee().
>>
>> Humm, AFAIR, the reason why eee_active is set outside of
>> phy_ethtool_set_eee() is because this is a MAC + PHY thing, both need to
>> agree and support that, and so while the PHY may be configured to have
>> EEE advertised and enabled, you also need to take care of the MAC
>> portion and enable EEE in there as well. Is not there such a thing for
>> the qca8k switch where the PHY needs to be configured through the
>> standard phylib calls, but the switch's transmitter/receiver also needs
>> to have EEE enabled?
>>
> 
> Hi Florian,
> 
> the switch needs to enable the eee on a per mac absis, but there is no
> way to tell if the autonegotiate worked and eee is enabled without
> reading the phys registers.

OK, that does not sound atypical here, most drivers I see do have a way
to tell if EEE is active by reading e.g: the LPI indication register, or
something that is able to reflect the negotiated result.

> 
> setting the eee_active inside phy_ethtool_get_eee() would break those
> dsa drivers that have a register telling if AN worked. if it is ok i
> will just call phy_ethtool_get_eee() inside get_eee().

Ok, let's see the code and then we can discuss from there, not very
clear on the proposed change here.
-- 
Florian

Re: [PATCH 3/3] net-next: dsa: add new driver for qca8xxx family

2016-09-13 Thread John Crispin 


On 13/09/2016 19:09, Florian Fainelli wrote:
> On 09/13/2016 08:59 AM, Andrew Lunn wrote:
>>> Hi Andrew,
>>>
>>> this function does indeed duplicate the functionality of
>>> phy_ethtool_get_eee() with the small difference, that e->eee_active is
>>> also set which phy_ethtool_get_eee() does not set.
>>>
>>> dsa_slave_get_eee() will call phy_ethtool_get_eee() right after the
>>> get_eee() op has been called. would it be ok to move the code setting
>>> eee_active to  phy_ethtool_get_eee().
> 
> Humm, AFAIR, the reason why eee_active is set outside of
> phy_ethtool_set_eee() is because this is a MAC + PHY thing, both need to
> agree and support that, and so while the PHY may be configured to have
> EEE advertised and enabled, you also need to take care of the MAC
> portion and enable EEE in there as well. Is not there such a thing for
> the qca8k switch where the PHY needs to be configured through the
> standard phylib calls, but the switch's transmitter/receiver also needs
> to have EEE enabled?
>

Hi Florian,

the switch needs to enable the eee on a per mac absis, but there is no
way to tell if the autonegotiate worked and eee is enabled without
reading the phys registers.

setting the eee_active inside phy_ethtool_get_eee() would break those
dsa drivers that have a register telling if AN worked. if it is ok i
will just call phy_ethtool_get_eee() inside get_eee().

John

Re: [PATCH 3/3] net-next: dsa: add new driver for qca8xxx family

2016-09-13 Thread Vivien Didelot 
Hi Andrew,

Andrew Lunn  writes:

>> ok, i will simply substract 1 from the phy_addr inside the mdio
>> callbacks. this would make the code more readable and make the DT
>> binding compliant with the ePAPR spec.
>
> It does however need well commenting. It is setting a trap for anybody
> who puts an external PHY on port 6. If they access that PHY via these
> functions, the address is off by one.
>
> This is the first silicon vendor who made their MDIO addresses for
> PHYs illogical. So i'm thinking we maybe should add a new function to
> dsa_switch_ops.
>
>   /* Return the MDIO address for the PHY for this port. */
> int (*phy_port_map(struct dsa_switch *ds, int port);
>
> This should return the MDIO address for integrated PHYs only, or
> -ENODEV if the port does not have an integrated PHY. For an external
> PHY, a phy-handle should be used. This phy_port_map() is used in
> dsa_slave_phy_setup(). But dsa_slave_phy_setup() is already too
> complex, so it needs doing with care.

Note that some switch drivers *have to* register their slave MDIO bus
themselves (e.g. bcm_sf2). This becomes confusing with the DSA
phy_{read,write} ops.

Since the former alternative is prefered, we may want to remove the
latter soon from DSA. If this phy_port_map is needed for that case, it'd
be preferable not to add it.

Thanks,

Vivien

Re: [PATCH 3/3] net-next: dsa: add new driver for qca8xxx family

2016-09-13 Thread Florian Fainelli 
On 09/13/2016 08:59 AM, Andrew Lunn wrote:
>> Hi Andrew,
>>
>> this function does indeed duplicate the functionality of
>> phy_ethtool_get_eee() with the small difference, that e->eee_active is
>> also set which phy_ethtool_get_eee() does not set.
>>
>> dsa_slave_get_eee() will call phy_ethtool_get_eee() right after the
>> get_eee() op has been called. would it be ok to move the code setting
>> eee_active to  phy_ethtool_get_eee().

Humm, AFAIR, the reason why eee_active is set outside of
phy_ethtool_set_eee() is because this is a MAC + PHY thing, both need to
agree and support that, and so while the PHY may be configured to have
EEE advertised and enabled, you also need to take care of the MAC
portion and enable EEE in there as well. Is not there such a thing for
the qca8k switch where the PHY needs to be configured through the
standard phylib calls, but the switch's transmitter/receiver also needs
to have EEE enabled?
-- 
Florian

Re: [PATCH 3/3] net-next: dsa: add new driver for qca8xxx family

2016-09-13 Thread Andrew Lunn 
> Hi Andrew,
> 
> this function does indeed duplicate the functionality of
> phy_ethtool_get_eee() with the small difference, that e->eee_active is
> also set which phy_ethtool_get_eee() does not set.
> 
> dsa_slave_get_eee() will call phy_ethtool_get_eee() right after the
> get_eee() op has been called. would it be ok to move the code setting
> eee_active to  phy_ethtool_get_eee().

Hi John

I think that is a question for Florian.

  Andrew

Re: [PATCH 3/3] net-next: dsa: add new driver for qca8xxx family

2016-09-13 Thread Andrew Lunn 
On Tue, Sep 13, 2016 at 11:40:43AM +0200, John Crispin wrote:
> 
> 
> On 13/09/2016 03:23, Andrew Lunn wrote:
> > So lets see if i have this right.
> > 
> > Port 0 has no internal phy.
> > Port 1 has an internal PHY at MDIO address 0.
> > Port 2 has an internal PHY at MDIO address 1.
> > ...
> > Port 5 has an internal PHY ad MDIO address 4.
> > Port 6 has no internal PHY.
> 
> Hi Andrew
> 
> correct. port 0 is the cpu port. I initially thought that port6 can also
> be used as te cpu port but there are various places in the datasheet
> stating that the cpu port is 0.

O.K, please correct the comments in the code, and make this clear in
the device tree binding.

> in some of the reference designs, port6
> is wired to a 2nd gmac of the cpu and in those cases port 6 is then
> hardwired to port 5 of the switch and called wan. right now the driver
> does not support this feature.

Hum, why not?

brctl addbr br1
brctl addif br1 port5
brctl addif br1 port6

They are just ports on a switch, so bridge them together.

> ok, i will simply substract 1 from the phy_addr inside the mdio
> callbacks. this would make the code more readable and make the DT
> binding compliant with the ePAPR spec.

It does however need well commenting. It is setting a trap for anybody
who puts an external PHY on port 6. If they access that PHY via these
functions, the address is off by one.

This is the first silicon vendor who made their MDIO addresses for
PHYs illogical. So i'm thinking we maybe should add a new function to
dsa_switch_ops.

/* Return the MDIO address for the PHY for this port. */
int (*phy_port_map(struct dsa_switch *ds, int port);

This should return the MDIO address for integrated PHYs only, or
-ENODEV if the port does not have an integrated PHY. For an external
PHY, a phy-handle should be used. This phy_port_map() is used in
dsa_slave_phy_setup(). But dsa_slave_phy_setup() is already too
complex, so it needs doing with care.

 Andrew

Re: [PATCH 3/3] net-next: dsa: add new driver for qca8xxx family

2016-09-13 Thread John Crispin 


On 13/09/2016 03:23, Andrew Lunn wrote:
> So lets see if i have this right.
> 
> Port 0 has no internal phy.
> Port 1 has an internal PHY at MDIO address 0.
> Port 2 has an internal PHY at MDIO address 1.
> ...
> Port 5 has an internal PHY ad MDIO address 4.
> Port 6 has no internal PHY.

Hi Andrew

correct. port 0 is the cpu port. I initially thought that port6 can also
be used as te cpu port but there are various places in the datasheet
stating that the cpu port is 0. in some of the reference designs, port6
is wired to a 2nd gmac of the cpu and in those cases port 6 is then
hardwired to port 5 of the switch and called wan. right now the driver
does not support this feature. i have changed the code to always assume
that port is the cpu port and will send a patch later to allow the
port5/6 wan port setup once the series got accepted.

> 
> This is why you have funky port numbers, and phy_to_port.

this is legacy code from the series Matthieu posted. i agree though that
its a bit dirty. Sergey already told me that the devicetree is also bad
because of this as the unit address of the device tree node and reg
property are not aligned.

> 
> I think it would be a lot cleaner to handle this in qca8k_phy_read()
> and qca8k_phy_write(). 

ok, i will simply substract 1 from the phy_addr inside the mdio
callbacks. this would make the code more readable and make the DT
binding compliant with the ePAPR spec.

> 
> Also, the comment it a bit misleading. You are probing the PHY ID, not
> the switch ID. At least for the Marvell switches, different switches
> can have the same embedded PHY. It would be annoying to find there is
> another incompatible switch with the same PHY ID.

there is only an 8bit field inside the MASK_CTRL register (0x000) which
is 0x13. I've sent an email to QCA asking if this a unique identifier.

> Is the embedded PHY compatible with the at803x driver?

I've sent an email to QCA asking about this

John

Re: [PATCH 3/3] net-next: dsa: add new driver for qca8xxx family

2016-09-13 Thread John Crispin 


On 13/09/2016 02:40, Andrew Lunn wrote:
>> > +static int
>> > +qca8k_get_eee(struct dsa_switch *ds, int port,
>> > +struct ethtool_eee *e)
>> > +{
>> > +  struct qca8k_priv *priv = qca8k_to_priv(ds);
>> > +  struct ethtool_eee *p = >port_sts[qca8k_phy_to_port(port)].eee;
>> > +  u32 lp, adv, supported;
>> > +  u16 val;
>> > +
>> > +  /* The switch has no way to tell the result of the AN so we need to
>> > +   * read the result directly from the PHYs MMD registers
>> > +   */
>> > +  val = qca8k_phy_mmd_read(priv, port, MDIO_MMD_PCS, MDIO_PCS_EEE_ABLE);
>> > +  supported = mmd_eee_cap_to_ethtool_sup_t(val);
>> > +
>> > +  val = qca8k_phy_mmd_read(priv, port, MDIO_MMD_AN, MDIO_AN_EEE_ADV);
>> > +  adv = mmd_eee_adv_to_ethtool_adv_t(val);
>> > +
>> > +  val = qca8k_phy_mmd_read(priv, port, MDIO_MMD_AN, MDIO_AN_EEE_LPABLE);
>> > +  lp = mmd_eee_adv_to_ethtool_adv_t(val);
>> > +
>> > +  e->eee_enabled = p->eee_enabled;
>> > +  e->eee_active = !!(supported & adv & lp);
>> > +
>> > +  return 0;
>> > +}
> Couldn't you just call phy_ethtool_get_eee(phydev)? Then you don't
> need qca8k_phy_mmd_read()?

Hi Andrew,

this function does indeed duplicate the functionality of
phy_ethtool_get_eee() with the small difference, that e->eee_active is
also set which phy_ethtool_get_eee() does not set.

dsa_slave_get_eee() will call phy_ethtool_get_eee() right after the
get_eee() op has been called. would it be ok to move the code setting
eee_active to  phy_ethtool_get_eee(). if thats possible then we could
just have a stub inside the dsa driver with a note saying that the dsa
layer will do the magic for us.

John

Re: [PATCH 3/3] net-next: dsa: add new driver for qca8xxx family

2016-09-12 Thread Andrew Lunn 
> +static int
> +qca8k_setup(struct dsa_switch *ds)
> +{
> + struct qca8k_priv *priv = qca8k_to_priv(ds);
> + int ret, i, phy_mode = -1;
> +
> + /* Keep track of which port is the connected to the cpu. This can be
> +  * phy 11 / port 0 or phy 5 / port 6.
> +  */
> + switch (dsa_upstream_port(ds)) {
> + case 11:
> + priv->cpu_port = 0;
> + break;
> + case 5:
> + priv->cpu_port = 6;
> + break;
> + default:
> + return -EOPNOTSUPP;
> + }

Hi John

Can you explain this funkiness with CPU port numbers. Why use 11
instead of 0? I ideally i would like to use 0 here, but if that it not
possible, it needs documenting in the device tree binding that 5 and
11 are special, representing ports 0 and 6.

> +
> + /* Start by setting up the register mapping */
> + priv->regmap = devm_regmap_init(ds->dev, NULL, priv,
> + _regmap_config);
> +
> + if (IS_ERR(priv->regmap))
> + pr_warn("regmap initialization failed");
> +
> + /* Initialize CPU port pad mode (xMII type, delays...) */
> + phy_mode = of_get_phy_mode(ds->ports[ds->dst->cpu_port].dn);
> + if (phy_mode < 0) {
> + pr_err("Can't find phy-mode for master device\n");
> + return phy_mode;
> + }
> + ret = qca8k_set_pad_ctrl(priv, priv->cpu_port, phy_mode);
> + if (ret < 0)
> + return ret;
> +
> + /* Enable CPU Port */
> + qca8k_reg_set(priv, QCA8K_REG_GLOBAL_FW_CTRL0,
> +   QCA8K_GLOBAL_FW_CTRL0_CPU_PORT_EN);

Does it matter which port is the CPU port here? 11 or 5

> +
> + /* Enable MIB counters */
> + qca8k_reg_set(priv, QCA8K_REG_MIB, QCA8K_MIB_CPU_KEEP);
> + qca8k_write(priv, QCA8K_REG_MODULE_EN, QCA8K_MODULE_EN_MIB);
> +
> + /* Enable QCA header mode on Port 0 */
> + qca8k_write(priv, QCA8K_REG_PORT_HDR_CTRL(priv->cpu_port),
> + QCA8K_PORT_HDR_CTRL_ALL << QCA8K_PORT_HDR_CTRL_TX_S |
> + QCA8K_PORT_HDR_CTRL_ALL << QCA8K_PORT_HDR_CTRL_RX_S);

Does the header mode only work on port 0?

> +static int
> +qca8k_phy_read(struct dsa_switch *ds, int phy, int regnum)
> +{
> + struct qca8k_priv *priv = qca8k_to_priv(ds);
> +
> + return mdiobus_read(priv->bus, phy, regnum);
> +}
> +
> +static int
> +qca8k_phy_write(struct dsa_switch *ds, int phy, int regnum, u16 val)
> +{
> + struct qca8k_priv *priv = qca8k_to_priv(ds);
> +
> + return mdiobus_write(priv->bus, phy, regnum, val);
> +}

snip

> +static int
> +qca8k_sw_probe(struct mdio_device *mdiodev)
> +{
> + struct qca8k_priv *priv;
> + u32 phy_id;
> +
> + /* sw_addr is irrelevant as the switch occupies the MDIO bus from
> +  * addresses 0 to 4 (PHYs) and 16-23 (for MDIO 32bits protocol). So
> +  * we'll probe address 0 to see if we see the right switch family.
> +  */

So lets see if i have this right.

Port 0 has no internal phy.
Port 1 has an internal PHY at MDIO address 0.
Port 2 has an internal PHY at MDIO address 1.
...
Port 5 has an internal PHY ad MDIO address 4.
Port 6 has no internal PHY.

This is why you have funky port numbers, and phy_to_port.

I think it would be a lot cleaner to handle this in qca8k_phy_read()
and qca8k_phy_write(). 

Also, the comment it a bit misleading. You are probing the PHY ID, not
the switch ID. At least for the Marvell switches, different switches
can have the same embedded PHY. It would be annoying to find there is
another incompatible switch with the same PHY ID.

Is the embedded PHY compatible with the at803x driver?

   Thanks
Andrew

Re: [PATCH 3/3] net-next: dsa: add new driver for qca8xxx family

2016-09-12 Thread Andrew Lunn 
Hi John

> +static u16
> +qca8k_phy_mmd_read(struct qca8k_priv *priv, int phy_addr, u16 addr, u16 reg)
> +{
> + u16 data;
> +
> + mutex_lock(>bus->mdio_lock);
> +
> + priv->bus->write(priv->bus, phy_addr, MII_ATH_MMD_ADDR, addr);
> + priv->bus->write(priv->bus, phy_addr, MII_ATH_MMD_DATA, reg);
> + priv->bus->write(priv->bus, phy_addr, MII_ATH_MMD_ADDR, addr | 0x4000);
> + data = priv->bus->read(priv->bus, phy_addr, MII_ATH_MMD_DATA);
> +
> + mutex_unlock(>bus->mdio_lock);
> +
> + return data;
> +}

snip

> +static int
> +qca8k_get_eee(struct dsa_switch *ds, int port,
> +   struct ethtool_eee *e)
> +{
> + struct qca8k_priv *priv = qca8k_to_priv(ds);
> + struct ethtool_eee *p = >port_sts[qca8k_phy_to_port(port)].eee;
> + u32 lp, adv, supported;
> + u16 val;
> +
> + /* The switch has no way to tell the result of the AN so we need to
> +  * read the result directly from the PHYs MMD registers
> +  */
> + val = qca8k_phy_mmd_read(priv, port, MDIO_MMD_PCS, MDIO_PCS_EEE_ABLE);
> + supported = mmd_eee_cap_to_ethtool_sup_t(val);
> +
> + val = qca8k_phy_mmd_read(priv, port, MDIO_MMD_AN, MDIO_AN_EEE_ADV);
> + adv = mmd_eee_adv_to_ethtool_adv_t(val);
> +
> + val = qca8k_phy_mmd_read(priv, port, MDIO_MMD_AN, MDIO_AN_EEE_LPABLE);
> + lp = mmd_eee_adv_to_ethtool_adv_t(val);
> +
> + e->eee_enabled = p->eee_enabled;
> + e->eee_active = !!(supported & adv & lp);
> +
> + return 0;
> +}

Couldn't you just call phy_ethtool_get_eee(phydev)? Then you don't
need qca8k_phy_mmd_read()?

 Andrew

Re: [PATCH 3/3] net-next: dsa: add new driver for qca8xxx family

2016-09-12 Thread Andrew Lunn 
Hi John

> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 

One linux/phy.h is enough.

> + /* Setup connection between CPU ports & PHYs */
> + for (i = 0; i < DSA_MAX_PORTS; i++) {
> + /* CPU port gets connected to all PHYs in the switch */
> + if (dsa_is_cpu_port(ds, i)) {
> + qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(priv->cpu_port),
> +   QCA8K_PORT_LOOKUP_MEMBER,
> +   ds->enabled_port_mask << 1);
> + }
> +
> + /* Invividual PHYs gets connected to CPU port only */
> + if (ds->enabled_port_mask & BIT(i)) {
> + int port = qca8k_phy_to_port(i);
> + int shift = 16 * (port % 2);
> +

snip

> +static void
> +qca8k_get_ethtool_stats(struct dsa_switch *ds, int phy,
> + uint64_t *data)
> +{
> + struct qca8k_priv *priv = qca8k_to_priv(ds);
> + const struct qca8k_mib_desc *mib;
> + u32 reg, i, port;
> + u64 hi;
> +
> + port = qca8k_phy_to_port(phy);

snip

> +static inline int qca8k_phy_to_port(int phy)
> +{
> + if (phy < 5)
> + return phy + 1;
> +
> + return -1;
> +}

What keep throwing me while reading this code is the use of PHY/phy.

What is meant by a phy in this driver?

DSA is all about switches. Switches are a switch fabric and a number
of ports. The ports contain Ethernet MACs, and optionally contain
embedded PHYs. If there are not embedded PHYs, there are often
external PHYs, and sometimes we just have MACs connected back to back.

Generally, DSA drivers don't need to do much with the MAC. Maybe set
the speed and duplex, and maybe signal delays. They also don't need to
do much with the PHY, phylib and a phy driver does all the work. DSA
is all about the switch fabric.

Yet i see phy scattered in a few places in this driver, and it seems
like they have nothing to do with the PHY.

Please can you change the terminology here? It might help if you can
remove qca8k_phy_to_port(). Why do you need to add 1? Could this be
eliminated via a better choice of reg in the device tree?

Thanks
Andrew

Re: [PATCH 3/3] net-next: dsa: add new driver for qca8xxx family

2016-09-12 Thread Andrew Lunn 
> +static u32
> +qca8k_mii_read32(struct mii_bus *bus, int phy_id, u32 regnum)
> +{
> + u16 lo, hi;
> +
> + lo = bus->read(bus, phy_id, regnum);
> + hi = bus->read(bus, phy_id, regnum + 1);
> +
> + return (hi << 16) | lo;
> +}
> +
> +static void
> +qca8k_mii_write32(struct mii_bus *bus, int phy_id, u32 regnum, u32 val)
> +{
> + u16 lo, hi;
> +
> + lo = val & 0x;
> + hi = (u16)(val >> 16);
> +
> + bus->write(bus, phy_id, regnum, lo);
> + bus->write(bus, phy_id, regnum + 1, hi);
> +}

Hi John

Thanks for picking up this driver and continuing its journey towards
mainline.

bus->read() and bus->write() can return an error code. There is a lot
of error handling in the mv88e6xxx driver looking at the error code
from these two functions. And it very rarely happens. So it seems
overkill to me. However, i have had errors, when bringing up a new
board and the device tree is wrong somehow.

But ignoring potential error altogether does not seem wise. I think at
minimum you should look at the return code in these functions and do a
rate limited dev_err().

> +
> +static void
> +qca8k_set_page(struct mii_bus *bus, u16 page)
> +{
> + if (page == qca8k_current_page)
> + return;
> +
> + bus->write(bus, 0x18, 0, page);
> + udelay(5);

Is that delay in the data sheet? What about other accesses, like
reading the stats which is going to generate a lot of back to back
reads?

> + qca8k_current_page = page;
> +}
> +
> +static u32
> +qca8k_read(struct qca8k_priv *priv, u32 reg)
> +{
> + u16 r1, r2, page;
> + u32 val;
> +
> + qca8k_split_addr(reg, , , );
> +
> + mutex_lock(>bus->mdio_lock);
> +
> + qca8k_set_page(priv->bus, page);
> + val = qca8k_mii_read32(priv->bus, 0x10 | r2, r1);
> +
> + mutex_unlock(>bus->mdio_lock);
> +
> + return val;
> +}
> +
> +static void
> +qca8k_write(struct qca8k_priv *priv, u32 reg, u32 val)
> +{
> + u16 r1, r2, page;
> +
> + qca8k_split_addr(reg, , , );
> +
> + mutex_lock(>bus->mdio_lock);
> +
> + qca8k_set_page(priv->bus, page);
> + qca8k_mii_write32(priv->bus, 0x10 | r2, r1, val);
> +
> + mutex_unlock(>bus->mdio_lock);
> +}
> +
> +static u32
> +qca8k_rmw(struct qca8k_priv *priv, u32 reg, u32 mask, u32 val)
> +{
> + u16 r1, r2, page;
> + u32 ret;
> +
> + qca8k_split_addr(reg, , , );
> +
> + mutex_lock(>bus->mdio_lock);
> +
> + qca8k_set_page(priv->bus, page);
> + ret = qca8k_mii_read32(priv->bus, 0x10 | r2, r1);
> + ret &= ~mask;
> + ret |= val;
> + qca8k_mii_write32(priv->bus, 0x10 | r2, r1, ret);
> +
> + mutex_unlock(>bus->mdio_lock);
> +
> + return ret;
> +}
> +
> +static inline void
> +qca8k_reg_set(struct qca8k_priv *priv, u32 reg, u32 val)
> +{
> + qca8k_rmw(priv, reg, 0, val);
> +}
> +
> +static inline void
> +qca8k_reg_clear(struct qca8k_priv *priv, u32 reg, u32 val)
> +{
> + qca8k_rmw(priv, reg, val, 0);
> +}

Drop the inline please.

> +static u16
> +qca8k_phy_mmd_read(struct qca8k_priv *priv, int phy_addr, u16 addr, u16 reg)
> +{
> + u16 data;
> +
> + mutex_lock(>bus->mdio_lock);
> +
> + priv->bus->write(priv->bus, phy_addr, MII_ATH_MMD_ADDR, addr);
> + priv->bus->write(priv->bus, phy_addr, MII_ATH_MMD_DATA, reg);
> + priv->bus->write(priv->bus, phy_addr, MII_ATH_MMD_ADDR, addr | 0x4000);
> + data = priv->bus->read(priv->bus, phy_addr, MII_ATH_MMD_DATA);
> +
> + mutex_unlock(>bus->mdio_lock);

Have you run lockdep on this? The mv88e6xxx has something similar, and
were getying false positive splats. We needed to use
mdiobus_write_nested(). Since you are using bus->write directly, not
using the wrapper, maybe this is not an issue. But i'm wondering if
ignoring the wrapped is the right thing to do, with nested MDIO
busses. Something i need to think about.

> +static int
> +qca8k_fdb_busy_wait(struct qca8k_priv *priv)
> +{
> + unsigned long timeout;
> +
> + timeout = jiffies + msecs_to_jiffies(20);
> +
> + /* loop until the busy flag has cleared */
> + do {
> + u32 reg = qca8k_read(priv, QCA8K_REG_ATU_FUNC);
> + int busy = reg & QCA8K_ATU_FUNC_BUSY;
> +
> + if (!busy)
> + break;
> + } while (!time_after_eq(jiffies, timeout));
> +
> + return time_after_eq(jiffies, timeout);
> +}

No sleep? You busy loop for 20ms?

> +
> +static void
> +qca8k_fdb_read(struct qca8k_priv *priv, struct qca8k_fdb *fdb)
> +{
> + u32 reg[4];
> + int i;
> +
> + /* load the ARL table into an array */
> + for (i = 0; i < 4; i++)
> + reg[i] = qca8k_read(priv, QCA8K_REG_ATU_DATA0 + (i * 4));
> +
> + /* vid - 83:72 */
> + fdb->vid = (reg[2] >> QCA8K_ATU_VID_S) & QCA8K_ATU_VID_M;
> + /* aging - 67:64 */
> + fdb->aging = reg[2] & QCA8K_ATU_STATUS_M;
> + /* portmask - 54:48 */
> + fdb->port_mask = (reg[1] >> QCA8K_ATU_PORT_S) & QCA8K_ATU_PORT_M;
> + /* mac - 47:0 */
> + fdb->mac[0] =