Messages in this thread Patch in this message | | | Date | Wed, 15 Dec 2021 11:49:12 +0200 | From | Vladimir Oltean <> | Subject | Re: [net-next PATCH RFC v6 12/16] net: dsa: qca8k: add support for mdio read/write in Ethernet packet |
| |
On Tue, Dec 14, 2021 at 11:44:05PM +0100, Ansuel Smith wrote: > Add qca8k side support for mdio read/write in Ethernet packet. > qca8k supports some specially crafted Ethernet packet that can be used > for mdio read/write instead of the legacy method uart/internal mdio. > This add support for the qca8k side to craft the packet and enqueue it. > Each port and the qca8k_priv have a special struct to put data in it. > The completion API is used to wait for the packet to be received back > with the requested data. > > The various steps are: > 1. Craft the special packet with the qca hdr set to mdio read/write > mode. > 2. Set the lock in the dedicated mdio struct. > 3. Reinit the completion. > 4. Enqueue the packet. > 5. Wait the packet to be received. > 6. Use the data set by the tagger to complete the mdio operation. > > If the completion timeouts or the ack value is not true, the legacy > mdio way is used. > > It has to be considered that in the initial setup mdio is still used and > mdio is still used until DSA is ready to accept and tag packet. > > tag_proto_connect() is used to fill the required handler for the tagger > to correctly parse and elaborate the special Ethernet mdio packet. > > Locking is added to qca8k_master_change() to make sure no mdio Ethernet > are in progress. > > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com> > --- > drivers/net/dsa/qca8k.c | 192 ++++++++++++++++++++++++++++++++++++++++ > drivers/net/dsa/qca8k.h | 13 +++ > 2 files changed, 205 insertions(+) > > diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c > index f317f527dd6d..b35ba26a0696 100644 > --- a/drivers/net/dsa/qca8k.c > +++ b/drivers/net/dsa/qca8k.c > @@ -20,6 +20,7 @@ > #include <linux/phylink.h> > #include <linux/gpio/consumer.h> > #include <linux/etherdevice.h> > +#include <linux/dsa/tag_qca.h> > > #include "qca8k.h" > > @@ -170,6 +171,158 @@ qca8k_rmw(struct qca8k_priv *priv, u32 reg, u32 mask, u32 write_val) > return regmap_update_bits(priv->regmap, reg, mask, write_val); > } > > +static void qca8k_rw_reg_ack_handler(struct dsa_port *dp, struct sk_buff *skb) > +{ > + struct qca8k_mdio_hdr_data *mdio_hdr_data; > + struct qca8k_priv *priv = dp->ds->priv; > + struct mdio_ethhdr *mdio_ethhdr; > + u8 len, cmd; > + > + mdio_ethhdr = (struct mdio_ethhdr *)skb_mac_header(skb); > + mdio_hdr_data = &priv->mdio_hdr_data; > + > + cmd = FIELD_GET(QCA_HDR_MDIO_CMD, mdio_ethhdr->command); > + len = FIELD_GET(QCA_HDR_MDIO_LENGTH, mdio_ethhdr->command); > + > + /* Make sure the seq match the requested packet */ > + if (mdio_ethhdr->seq == mdio_hdr_data->seq) > + mdio_hdr_data->ack = true; > + > + if (cmd == MDIO_READ) { > + mdio_hdr_data->data[0] = mdio_ethhdr->mdio_data; > + > + /* Get the rest of the 12 byte of data */ > + if (len > QCA_HDR_MDIO_DATA1_LEN) > + memcpy(mdio_hdr_data->data + 1, skb->data, > + QCA_HDR_MDIO_DATA2_LEN); > + } > + > + complete(&mdio_hdr_data->rw_done); > +} > + > +static struct sk_buff *qca8k_alloc_mdio_header(enum mdio_cmd cmd, u32 reg, u32 *val, > + int seq_num, int priority) > +{ > + struct mdio_ethhdr *mdio_ethhdr; > + struct sk_buff *skb; > + u16 hdr; > + > + skb = dev_alloc_skb(QCA_HDR_MDIO_PKG_LEN);
Still no if (!skb) checking... Not only here, but also at the call sites of this.
> + > + skb_reset_mac_header(skb); > + skb_set_network_header(skb, skb->len); > + > + mdio_ethhdr = skb_push(skb, QCA_HDR_MDIO_HEADER_LEN + QCA_HDR_LEN); > + > + hdr = FIELD_PREP(QCA_HDR_XMIT_VERSION, QCA_HDR_VERSION); > + hdr |= FIELD_PREP(QCA_HDR_XMIT_PRIORITY, priority); > + hdr |= QCA_HDR_XMIT_FROM_CPU; > + hdr |= FIELD_PREP(QCA_HDR_XMIT_DP_BIT, BIT(0)); > + hdr |= FIELD_PREP(QCA_HDR_XMIT_CONTROL, QCA_HDR_XMIT_TYPE_RW_REG); > + > + mdio_ethhdr->seq = FIELD_PREP(QCA_HDR_MDIO_SEQ_NUM, seq_num); > + > + mdio_ethhdr->command = FIELD_PREP(QCA_HDR_MDIO_ADDR, reg); > + mdio_ethhdr->command |= FIELD_PREP(QCA_HDR_MDIO_LENGTH, 4); > + mdio_ethhdr->command |= FIELD_PREP(QCA_HDR_MDIO_CMD, cmd); > + mdio_ethhdr->command |= FIELD_PREP(QCA_HDR_MDIO_CHECK_CODE, MDIO_CHECK_CODE_VAL); > + > + if (cmd == MDIO_WRITE) > + mdio_ethhdr->mdio_data = *val; > + > + mdio_ethhdr->hdr = htons(hdr); > + > + skb_put_zero(skb, QCA_HDR_MDIO_DATA2_LEN); > + skb_put_zero(skb, QCA_HDR_MDIO_PADDING_LEN);
Maybe a single call to skb_put_zero, and pass the sum as argument?
> + > + return skb; > +} > + > +static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val) > +{ > + struct qca8k_mdio_hdr_data *mdio_hdr_data = &priv->mdio_hdr_data; > + struct sk_buff *skb; > + bool ack; > + int ret; > + > + skb = qca8k_alloc_mdio_header(MDIO_READ, reg, NULL, 200, QCA8K_ETHERNET_MDIO_PRIORITY);
You hardcode "seq" to 200? Aren't you supposed to increment it or something?
> + skb->dev = (struct net_device *)priv->master;
You access priv->master outside of priv->mdio_hdr_data.mutex from qca8k_master_change(), that can't be good.
> + > + mutex_lock(&mdio_hdr_data->mutex); > + > + reinit_completion(&mdio_hdr_data->rw_done); > + mdio_hdr_data->seq = 200;
Why do you rewrite the seq here?
> + mdio_hdr_data->ack = false; > + > + dev_queue_xmit(skb); > + > + ret = wait_for_completion_timeout(&mdio_hdr_data->rw_done, > + msecs_to_jiffies(QCA8K_ETHERNET_TIMEOUT)); > + > + *val = mdio_hdr_data->data[0]; > + ack = mdio_hdr_data->ack; > + > + mutex_unlock(&mdio_hdr_data->mutex); > + > + if (ret <= 0) > + return -ETIMEDOUT; > + > + if (!ack) > + return -EINVAL; > + > + return 0; > +} > + > +static int qca8k_write_eth(struct qca8k_priv *priv, u32 reg, u32 val) > +{ > + struct qca8k_mdio_hdr_data *mdio_hdr_data = &priv->mdio_hdr_data; > + struct sk_buff *skb; > + bool ack; > + int ret; > + > + skb = qca8k_alloc_mdio_header(MDIO_WRITE, reg, &val, 200, QCA8K_ETHERNET_MDIO_PRIORITY); > + skb->dev = (struct net_device *)priv->master; > + > + mutex_lock(&mdio_hdr_data->mutex); > + > + reinit_completion(&mdio_hdr_data->rw_done); > + mdio_hdr_data->ack = false; > + mdio_hdr_data->seq = 200; > + > + dev_queue_xmit(skb); > + > + ret = wait_for_completion_timeout(&mdio_hdr_data->rw_done, > + msecs_to_jiffies(QCA8K_ETHERNET_TIMEOUT)); > + > + ack = mdio_hdr_data->ack; > + > + mutex_unlock(&mdio_hdr_data->mutex); > + > + if (ret <= 0) > + return -ETIMEDOUT; > + > + if (!ack) > + return -EINVAL; > + > + return 0; > +} > + > +static int > +qca8k_regmap_update_bits_eth(struct qca8k_priv *priv, u32 reg, u32 mask, u32 write_val) > +{ > + u32 val = 0; > + int ret; > + > + ret = qca8k_read_eth(priv, reg, &val); > + if (ret) > + return ret; > + > + val &= ~mask; > + val |= write_val; > + > + return qca8k_write_eth(priv, reg, val); > +} > + > static int > qca8k_regmap_read(void *ctx, uint32_t reg, uint32_t *val) > { > @@ -178,6 +331,9 @@ qca8k_regmap_read(void *ctx, uint32_t reg, uint32_t *val) > u16 r1, r2, page; > int ret; > > + if (priv->master && !qca8k_read_eth(priv, reg, val)) > + return 0; > + > qca8k_split_addr(reg, &r1, &r2, &page); > > mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED); > @@ -201,6 +357,9 @@ qca8k_regmap_write(void *ctx, uint32_t reg, uint32_t val) > u16 r1, r2, page; > int ret; > > + if (priv->master && !qca8k_write_eth(priv, reg, val)) > + return 0; > + > qca8k_split_addr(reg, &r1, &r2, &page); > > mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED); > @@ -225,6 +384,10 @@ qca8k_regmap_update_bits(void *ctx, uint32_t reg, uint32_t mask, uint32_t write_ > u32 val; > int ret; > > + if (priv->master && > + !qca8k_regmap_update_bits_eth(priv, reg, mask, write_val)) > + return 0; > + > qca8k_split_addr(reg, &r1, &r2, &page); > > mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED); > @@ -2394,10 +2557,38 @@ qca8k_master_change(struct dsa_switch *ds, const struct net_device *master, > if (dp->index != 0) > return; > > + mutex_lock(&priv->mdio_hdr_data.mutex); > + > if (operational) > priv->master = master; > else > priv->master = NULL; > + > + mutex_unlock(&priv->mdio_hdr_data.mutex); > +} > + > +static int qca8k_connect_tag_protocol(struct dsa_switch *ds, > + enum dsa_tag_protocol proto) > +{ > + struct qca8k_priv *qca8k_priv = ds->priv; > + > + switch (proto) { > + case DSA_TAG_PROTO_QCA: > + struct tag_qca_priv *priv; > + > + priv = ds->tagger_data; > + > + mutex_init(&qca8k_priv->mdio_hdr_data.mutex); > + init_completion(&qca8k_priv->mdio_hdr_data.rw_done);
I think having these initializations here dilutes the purpose of this callback. Could you please move these two lines to qca8k_sw_probe()?
> + > + priv->rw_reg_ack_handler = qca8k_rw_reg_ack_handler; > + > + break; > + default: > + return -EOPNOTSUPP; > + } > + > + return 0; > } > > static const struct dsa_switch_ops qca8k_switch_ops = { > @@ -2436,6 +2627,7 @@ static const struct dsa_switch_ops qca8k_switch_ops = { > .port_lag_join = qca8k_port_lag_join, > .port_lag_leave = qca8k_port_lag_leave, > .master_state_change = qca8k_master_change, > + .connect_tag_protocol = qca8k_connect_tag_protocol, > }; > > static int qca8k_read_switch_id(struct qca8k_priv *priv) > diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h > index 6edd6adc3063..dbe8c74c9793 100644 > --- a/drivers/net/dsa/qca8k.h > +++ b/drivers/net/dsa/qca8k.h > @@ -11,6 +11,10 @@ > #include <linux/delay.h> > #include <linux/regmap.h> > #include <linux/gpio.h> > +#include <linux/dsa/tag_qca.h> > + > +#define QCA8K_ETHERNET_MDIO_PRIORITY 7 > +#define QCA8K_ETHERNET_TIMEOUT 100 > > #define QCA8K_NUM_PORTS 7 > #define QCA8K_NUM_CPU_PORTS 2 > @@ -328,6 +332,14 @@ enum { > QCA8K_CPU_PORT6, > }; > > +struct qca8k_mdio_hdr_data {
What do you think about the "qca8k_eth_mgmt_data" name rather than "mdio_hdr_data"? I don't think this has anything to do with MDIO.
> + struct completion rw_done; > + struct mutex mutex; /* Enforce one mdio read/write at time */ > + bool ack; > + u32 seq; > + u32 data[4]; > +}; > + > struct qca8k_ports_config { > bool sgmii_rx_clk_falling_edge; > bool sgmii_tx_clk_falling_edge; > @@ -354,6 +366,7 @@ struct qca8k_priv { > struct gpio_desc *reset_gpio; > unsigned int port_mtu[QCA8K_NUM_PORTS]; > const struct net_device *master; /* Track if mdio/mib Ethernet is available */ > + struct qca8k_mdio_hdr_data mdio_hdr_data; > }; > > struct qca8k_mib_desc { > -- > 2.33.1 >
| |