Re: [net-next 1/6] net: marvell: prestera: Add driver for Prestera family ASIC devices
On Sat, Jun 20, 2020 at 03:56:39PM +0300, Vadym Kochan wrote: > But it will look same as prestera_destroy_ports(), do you think > this is not a problem to have a same logic doubled ? No, error paths of init() usually share logic with fini(). The benefits of being consistent, always having init() followed by fini() and making sure they are symmetric, out-weigh the benefit of saving a few lines of code.
Re: [net-next 1/6] net: marvell: prestera: Add driver for Prestera family ASIC devices
Hi Ido,
On Wed, Jun 03, 2020 at 12:23:58PM +0300, Ido Schimmel wrote:
> On Mon, Jun 01, 2020 at 01:50:13PM +0300, Vadym Kochan wrote:
> > Hi Ido,
> >
> > On Sat, May 30, 2020 at 06:48:01PM +0300, Ido Schimmel wrote:
> > > On Thu, May 28, 2020 at 06:12:40PM +0300, Vadym Kochan wrote:
> > >
> >
> > [...]
> >
> > > Nit: "From" ?
> > >
> > > > + PRESTERA_DSA_CMD_FROM_CPU,
> > > > +};
> > > > +
> > > > +struct prestera_dsa_vlan {
> > > > + u16 vid;
> > > > + u8 vpt;
> > > > + u8 cfi_bit;
> > > > + bool is_tagged;
> > > > +};
> > > > +
> > > > +struct prestera_dsa {
> > > > + struct prestera_dsa_vlan vlan;
> > > > + u32 hw_dev_num;
> > > > + u32 port_num;
> > > > +};
> > > > +
> > > > +int prestera_dsa_parse(struct prestera_dsa *dsa, const u8 *dsa_buf);
> > > > +int prestera_dsa_build(const struct prestera_dsa *dsa, u8 *dsa_buf);
> > > > +
> > > > +#endif /* _PRESTERA_DSA_H_ */
> > > > diff --git a/drivers/net/ethernet/marvell/prestera/prestera_hw.c
> > > > b/drivers/net/ethernet/marvell/prestera/prestera_hw.c
> > > > new file mode 100644
> > > > index ..3aa3974f957a
> > > > --- /dev/null
> > > > +++ b/drivers/net/ethernet/marvell/prestera/prestera_hw.c
> > > > @@ -0,0 +1,610 @@
> > > > +// SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0
> > > > +/* Copyright (c) 2019-2020 Marvell International Ltd. All rights
> > > > reserved */
> > > > +
> > > > +#include
> > > > +#include
> > > > +#include
> > > > +#include
> > > > +
> > > > +#include "prestera.h"
> > > > +#include "prestera_hw.h"
> > > > +
> > > > +#define PRESTERA_SWITCH_INIT_TIMEOUT 3000 /* 30sec */
> > >
> > > Out of curiosity, how long does it actually take you to initialize the
> > > hardware?
It might be minimum 10-15 secs.
> > >
> > > Also, I find it useful to note the units in the name, so:
> > >
> > > #define PRESTERA_SWITCH_INIT_TIMEOUT_US (30 * 1000 * 1000)
> > >
> > > BTW, it says 30 seconds in comment, but the call chain where it is used
> > > is:
> > >
> > > prestera_cmd_ret_wait(, PRESTERA_SWITCH_INIT_TIMEOUT)
> > > __prestera_cmd_ret(..., wait)
> > > prestera_fw_send_req(..., waitms)
> > > prestera_fw_cmd_send(..., waitms)
> > > prestera_fw_wait_reg32(..., waitms)
> > > readl_poll_timeout(..., waitms * 1000)
> > >
> > > So I think you should actually define it as:
> > >
> > > #define PRESTERA_SWITCH_INIT_TIMEOUT_MS (30 * 1000)
> > >
> > > And rename all these 'wait' arguments to 'waitms' so it's clearer which
> > > unit they expect.
> > >
> > [...]
> > > > +static int __prestera_cmd_ret(struct prestera_switch *sw,
> > > > + enum prestera_cmd_type_t type,
> > > > + struct prestera_msg_cmd *cmd, size_t clen,
> > > > + struct prestera_msg_ret *ret, size_t rlen,
> > > > + int wait)
> > > > +{
> > > > + struct prestera_device *dev = sw->dev;
> > > > + int err;
> > > > +
> > > > + cmd->type = type;
> > > > +
> > > > + err = dev->send_req(dev, (u8 *)cmd, clen, (u8 *)ret, rlen,
> > > > wait);
> > > > + if (err)
> > > > + return err;
> > > > +
> > > > + if (ret->cmd.type != PRESTERA_CMD_TYPE_ACK)
> > > > + return -EBADE;
> > > > + if (ret->status != PRESTERA_CMD_ACK_OK)
> > >
> > > You don't have more states here other than OK / FAIL ? It might help you
> > > in debugging if you include them. You might find trace_devlink_hwerr()
> > > useful.
> > Thanks, I will consider this.
> >
> > >
> > > > + return -EINVAL;
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +static int prestera_cmd_ret(struct prestera_switch *sw,
> > > > + enum prestera_cmd_type_t type,
> > > > + struct prestera_msg_cmd *cmd, size_t clen,
> > > > + struct prestera_msg_ret *ret, size_t rlen)
> > > > +{
> > > > + return __prestera_cmd_ret(sw, type, cmd, clen, ret, rlen, 0);
> > > > +}
> > > > +
> > > > +static int prestera_cmd_ret_wait(struct prestera_switch *sw,
> > > > +enum prestera_cmd_type_t type,
> > > > +struct prestera_msg_cmd *cmd, size_t
> > > > clen,
> > > > +struct prestera_msg_ret *ret, size_t
> > > > rlen,
> > > > +int wait)
> > > > +{
> > > > + return __prestera_cmd_ret(sw, type, cmd, clen, ret, rlen, wait);
> > > > +}
> > > > +
> > > > +static int prestera_cmd(struct prestera_switch *sw,
> > > > + enum prestera_cmd_type_t type,
> > > > + struct prestera_msg_cmd *cmd, size_t clen)
> > > > +{
> > > > + struct prestera_msg_common_resp resp;
> > > > +
> > > > + return prestera_cmd_ret(sw, type, cmd, clen, ,
> > > > sizeof(resp));
> > > > +}
> > > > +
> > > > +static int
Re: [net-next 1/6] net: marvell: prestera: Add driver for Prestera family ASIC devices
On Wed, Jun 03, 2020 at 04:16:32PM +0200, Jiri Pirko wrote:
> Thu, May 28, 2020 at 05:12:40PM CEST, vadym.koc...@plvision.eu wrote:
>
> [...]
>
> >+}
> >+
> >+int prestera_hw_port_info_get(const struct prestera_port *port,
> >+ u16 *fp_id, u32 *hw_id, u32 *dev_id)
>
> Please unify the ordering of "hw_id" and "dev_id" with the rest of the
> functions having the same args.
>
OK, will do.
>
>
> >+{
> >+struct prestera_msg_port_info_resp resp;
> >+struct prestera_msg_port_info_req req = {
> >+.port = port->id
> >+};
> >+int err;
> >+
> >+err = prestera_cmd_ret(port->sw, PRESTERA_CMD_TYPE_PORT_INFO_GET,
> >+ , sizeof(req), , sizeof(resp));
> >+if (err)
> >+return err;
> >+
> >+*hw_id = resp.hw_id;
> >+*dev_id = resp.dev_id;
> >+*fp_id = resp.fp_id;
> >+
> >+return 0;
> >+}
> >+
> >+int prestera_hw_switch_mac_set(struct prestera_switch *sw, char *mac)
> >+{
> >+struct prestera_msg_switch_attr_req req = {
> >+.attr = PRESTERA_CMD_SWITCH_ATTR_MAC,
> >+};
> >+
> >+memcpy(req.param.mac, mac, sizeof(req.param.mac));
> >+
> >+return prestera_cmd(sw, PRESTERA_CMD_TYPE_SWITCH_ATTR_SET,
> >+, sizeof(req));
> >+}
> >+
> >+int prestera_hw_switch_init(struct prestera_switch *sw)
> >+{
> >+struct prestera_msg_switch_init_resp resp;
> >+struct prestera_msg_common_req req;
> >+int err;
> >+
> >+INIT_LIST_HEAD(>event_handlers);
> >+
> >+err = prestera_cmd_ret_wait(sw, PRESTERA_CMD_TYPE_SWITCH_INIT,
> >+, sizeof(req),
> >+, sizeof(resp),
> >+PRESTERA_SWITCH_INIT_TIMEOUT);
> >+if (err)
> >+return err;
> >+
> >+sw->id = resp.switch_id;
> >+sw->port_count = resp.port_count;
> >+sw->mtu_min = PRESTERA_MIN_MTU;
> >+sw->mtu_max = resp.mtu_max;
> >+sw->dev->recv_msg = prestera_evt_recv;
> >+sw->dev->recv_pkt = prestera_pkt_recv;
> >+
> >+return 0;
> >+}
> >+
> >+int prestera_hw_port_state_set(const struct prestera_port *port,
> >+ bool admin_state)
> >+{
> >+struct prestera_msg_port_attr_req req = {
> >+.attr = PRESTERA_CMD_PORT_ATTR_ADMIN_STATE,
> >+.port = port->hw_id,
> >+.dev = port->dev_id,
> >+.param = {.admin_state = admin_state}
> >+};
> >+
> >+return prestera_cmd(port->sw, PRESTERA_CMD_TYPE_PORT_ATTR_SET,
> >+, sizeof(req));
> >+}
> >+
> >+int prestera_hw_port_mtu_set(const struct prestera_port *port, u32 mtu)
> >+{
> >+struct prestera_msg_port_attr_req req = {
> >+.attr = PRESTERA_CMD_PORT_ATTR_MTU,
> >+.port = port->hw_id,
> >+.dev = port->dev_id,
> >+.param = {.mtu = mtu}
> >+};
> >+
> >+return prestera_cmd(port->sw, PRESTERA_CMD_TYPE_PORT_ATTR_SET,
> >+, sizeof(req));
> >+}
> >+
> >+int prestera_hw_port_mac_set(const struct prestera_port *port, char *mac)
> >+{
> >+struct prestera_msg_port_attr_req req = {
> >+.attr = PRESTERA_CMD_PORT_ATTR_MAC,
> >+.port = port->hw_id,
> >+.dev = port->dev_id
> >+};
> >+memcpy(, mac, sizeof(req.param.mac));
> >+
> >+return prestera_cmd(port->sw, PRESTERA_CMD_TYPE_PORT_ATTR_SET,
> >+, sizeof(req));
> >+}
> >+
> >+int prestera_hw_port_cap_get(const struct prestera_port *port,
> >+ struct prestera_port_caps *caps)
> >+{
> >+struct prestera_msg_port_attr_resp resp;
> >+struct prestera_msg_port_attr_req req = {
> >+.attr = PRESTERA_CMD_PORT_ATTR_CAPABILITY,
> >+.port = port->hw_id,
> >+.dev = port->dev_id
> >+};
> >+int err;
> >+
> >+err = prestera_cmd_ret(port->sw, PRESTERA_CMD_TYPE_PORT_ATTR_GET,
> >+ , sizeof(req), , sizeof(resp));
> >+if (err)
> >+return err;
> >+
> >+caps->supp_link_modes = resp.param.cap.link_mode;
> >+caps->supp_fec = resp.param.cap.fec;
> >+caps->type = resp.param.cap.type;
> >+caps->transceiver = resp.param.cap.transceiver;
> >+
> >+return err;
> >+}
> >+
> >+int prestera_hw_port_autoneg_set(const struct prestera_port *port,
> >+ bool autoneg, u64 link_modes, u8 fec)
> >+{
> >+struct prestera_msg_port_attr_req req = {
> >+.attr = PRESTERA_CMD_PORT_ATTR_AUTONEG,
> >+.port = port->hw_id,
> >+.dev = port->dev_id,
> >+.param = {.autoneg = {.link_mode = link_modes,
> >+ .enable = autoneg,
> >+ .fec = fec}
> >+}
> >+};
> >+
> >+return prestera_cmd(port->sw, PRESTERA_CMD_TYPE_PORT_ATTR_SET,
> >+, sizeof(req));
> >+}
> >+
> >+int
Re: [net-next 1/6] net: marvell: prestera: Add driver for Prestera family ASIC devices
Wed, Jun 03, 2020 at 04:29:44PM CEST, j...@resnulli.us wrote:
>Thu, May 28, 2020 at 05:12:40PM CEST, vadym.koc...@plvision.eu wrote:
>
>[...]
>
>>+static int prestera_port_create(struct prestera_switch *sw, u32 id)
>>+{
>>+ struct prestera_port *port;
>>+ struct net_device *dev;
>>+ int err;
>>+
>>+ dev = alloc_etherdev(sizeof(*port));
>>+ if (!dev)
>>+ return -ENOMEM;
>>+
>>+ port = netdev_priv(dev);
>>+
>>+ port->dev = dev;
>>+ port->id = id;
>>+ port->sw = sw;
>>+
>>+ err = prestera_hw_port_info_get(port, >fp_id,
>>+ >hw_id, >dev_id);
>>+ if (err) {
>>+ dev_err(prestera_dev(sw), "Failed to get port(%u) info\n", id);
>>+ goto err_port_init;
>>+ }
>>+
>>+ dev->features |= NETIF_F_NETNS_LOCAL;
>>+ dev->netdev_ops = _ops;
>>+
>>+ netif_carrier_off(dev);
>
>No need.
Actually, it is. Sorry :)
Re: [net-next 1/6] net: marvell: prestera: Add driver for Prestera family ASIC devices
Thu, May 28, 2020 at 05:12:40PM CEST, vadym.koc...@plvision.eu wrote:
[...]
>+static int prestera_port_create(struct prestera_switch *sw, u32 id)
>+{
>+ struct prestera_port *port;
>+ struct net_device *dev;
>+ int err;
>+
>+ dev = alloc_etherdev(sizeof(*port));
>+ if (!dev)
>+ return -ENOMEM;
>+
>+ port = netdev_priv(dev);
>+
>+ port->dev = dev;
>+ port->id = id;
>+ port->sw = sw;
>+
>+ err = prestera_hw_port_info_get(port, >fp_id,
>+ >hw_id, >dev_id);
>+ if (err) {
>+ dev_err(prestera_dev(sw), "Failed to get port(%u) info\n", id);
>+ goto err_port_init;
>+ }
>+
>+ dev->features |= NETIF_F_NETNS_LOCAL;
>+ dev->netdev_ops = _ops;
>+
>+ netif_carrier_off(dev);
No need.
Re: [net-next 1/6] net: marvell: prestera: Add driver for Prestera family ASIC devices
Thu, May 28, 2020 at 05:12:40PM CEST, vadym.koc...@plvision.eu wrote:
[...]
>+}
>+
>+int prestera_hw_port_info_get(const struct prestera_port *port,
>+u16 *fp_id, u32 *hw_id, u32 *dev_id)
Please unify the ordering of "hw_id" and "dev_id" with the rest of the
functions having the same args.
>+{
>+ struct prestera_msg_port_info_resp resp;
>+ struct prestera_msg_port_info_req req = {
>+ .port = port->id
>+ };
>+ int err;
>+
>+ err = prestera_cmd_ret(port->sw, PRESTERA_CMD_TYPE_PORT_INFO_GET,
>+ , sizeof(req), , sizeof(resp));
>+ if (err)
>+ return err;
>+
>+ *hw_id = resp.hw_id;
>+ *dev_id = resp.dev_id;
>+ *fp_id = resp.fp_id;
>+
>+ return 0;
>+}
>+
>+int prestera_hw_switch_mac_set(struct prestera_switch *sw, char *mac)
>+{
>+ struct prestera_msg_switch_attr_req req = {
>+ .attr = PRESTERA_CMD_SWITCH_ATTR_MAC,
>+ };
>+
>+ memcpy(req.param.mac, mac, sizeof(req.param.mac));
>+
>+ return prestera_cmd(sw, PRESTERA_CMD_TYPE_SWITCH_ATTR_SET,
>+ , sizeof(req));
>+}
>+
>+int prestera_hw_switch_init(struct prestera_switch *sw)
>+{
>+ struct prestera_msg_switch_init_resp resp;
>+ struct prestera_msg_common_req req;
>+ int err;
>+
>+ INIT_LIST_HEAD(>event_handlers);
>+
>+ err = prestera_cmd_ret_wait(sw, PRESTERA_CMD_TYPE_SWITCH_INIT,
>+ , sizeof(req),
>+ , sizeof(resp),
>+ PRESTERA_SWITCH_INIT_TIMEOUT);
>+ if (err)
>+ return err;
>+
>+ sw->id = resp.switch_id;
>+ sw->port_count = resp.port_count;
>+ sw->mtu_min = PRESTERA_MIN_MTU;
>+ sw->mtu_max = resp.mtu_max;
>+ sw->dev->recv_msg = prestera_evt_recv;
>+ sw->dev->recv_pkt = prestera_pkt_recv;
>+
>+ return 0;
>+}
>+
>+int prestera_hw_port_state_set(const struct prestera_port *port,
>+ bool admin_state)
>+{
>+ struct prestera_msg_port_attr_req req = {
>+ .attr = PRESTERA_CMD_PORT_ATTR_ADMIN_STATE,
>+ .port = port->hw_id,
>+ .dev = port->dev_id,
>+ .param = {.admin_state = admin_state}
>+ };
>+
>+ return prestera_cmd(port->sw, PRESTERA_CMD_TYPE_PORT_ATTR_SET,
>+ , sizeof(req));
>+}
>+
>+int prestera_hw_port_mtu_set(const struct prestera_port *port, u32 mtu)
>+{
>+ struct prestera_msg_port_attr_req req = {
>+ .attr = PRESTERA_CMD_PORT_ATTR_MTU,
>+ .port = port->hw_id,
>+ .dev = port->dev_id,
>+ .param = {.mtu = mtu}
>+ };
>+
>+ return prestera_cmd(port->sw, PRESTERA_CMD_TYPE_PORT_ATTR_SET,
>+ , sizeof(req));
>+}
>+
>+int prestera_hw_port_mac_set(const struct prestera_port *port, char *mac)
>+{
>+ struct prestera_msg_port_attr_req req = {
>+ .attr = PRESTERA_CMD_PORT_ATTR_MAC,
>+ .port = port->hw_id,
>+ .dev = port->dev_id
>+ };
>+ memcpy(, mac, sizeof(req.param.mac));
>+
>+ return prestera_cmd(port->sw, PRESTERA_CMD_TYPE_PORT_ATTR_SET,
>+ , sizeof(req));
>+}
>+
>+int prestera_hw_port_cap_get(const struct prestera_port *port,
>+ struct prestera_port_caps *caps)
>+{
>+ struct prestera_msg_port_attr_resp resp;
>+ struct prestera_msg_port_attr_req req = {
>+ .attr = PRESTERA_CMD_PORT_ATTR_CAPABILITY,
>+ .port = port->hw_id,
>+ .dev = port->dev_id
>+ };
>+ int err;
>+
>+ err = prestera_cmd_ret(port->sw, PRESTERA_CMD_TYPE_PORT_ATTR_GET,
>+ , sizeof(req), , sizeof(resp));
>+ if (err)
>+ return err;
>+
>+ caps->supp_link_modes = resp.param.cap.link_mode;
>+ caps->supp_fec = resp.param.cap.fec;
>+ caps->type = resp.param.cap.type;
>+ caps->transceiver = resp.param.cap.transceiver;
>+
>+ return err;
>+}
>+
>+int prestera_hw_port_autoneg_set(const struct prestera_port *port,
>+ bool autoneg, u64 link_modes, u8 fec)
>+{
>+ struct prestera_msg_port_attr_req req = {
>+ .attr = PRESTERA_CMD_PORT_ATTR_AUTONEG,
>+ .port = port->hw_id,
>+ .dev = port->dev_id,
>+ .param = {.autoneg = {.link_mode = link_modes,
>+.enable = autoneg,
>+.fec = fec}
>+ }
>+ };
>+
>+ return prestera_cmd(port->sw, PRESTERA_CMD_TYPE_PORT_ATTR_SET,
>+ , sizeof(req));
>+}
>+
>+int prestera_hw_port_stats_get(const struct prestera_port *port,
>+ struct prestera_port_stats *st)
>+{
>+ struct prestera_msg_port_stats_resp resp;
>+ struct prestera_msg_port_attr_req req
Re: [net-next 1/6] net: marvell: prestera: Add driver for Prestera family ASIC devices
On Mon, Jun 01, 2020 at 01:50:13PM +0300, Vadym Kochan wrote:
> Hi Ido,
>
> On Sat, May 30, 2020 at 06:48:01PM +0300, Ido Schimmel wrote:
> > On Thu, May 28, 2020 at 06:12:40PM +0300, Vadym Kochan wrote:
> >
>
> [...]
>
> > Nit: "From" ?
> >
> > > + PRESTERA_DSA_CMD_FROM_CPU,
> > > +};
> > > +
> > > +struct prestera_dsa_vlan {
> > > + u16 vid;
> > > + u8 vpt;
> > > + u8 cfi_bit;
> > > + bool is_tagged;
> > > +};
> > > +
> > > +struct prestera_dsa {
> > > + struct prestera_dsa_vlan vlan;
> > > + u32 hw_dev_num;
> > > + u32 port_num;
> > > +};
> > > +
> > > +int prestera_dsa_parse(struct prestera_dsa *dsa, const u8 *dsa_buf);
> > > +int prestera_dsa_build(const struct prestera_dsa *dsa, u8 *dsa_buf);
> > > +
> > > +#endif /* _PRESTERA_DSA_H_ */
> > > diff --git a/drivers/net/ethernet/marvell/prestera/prestera_hw.c
> > > b/drivers/net/ethernet/marvell/prestera/prestera_hw.c
> > > new file mode 100644
> > > index ..3aa3974f957a
> > > --- /dev/null
> > > +++ b/drivers/net/ethernet/marvell/prestera/prestera_hw.c
> > > @@ -0,0 +1,610 @@
> > > +// SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0
> > > +/* Copyright (c) 2019-2020 Marvell International Ltd. All rights
> > > reserved */
> > > +
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +
> > > +#include "prestera.h"
> > > +#include "prestera_hw.h"
> > > +
> > > +#define PRESTERA_SWITCH_INIT_TIMEOUT 3000/* 30sec */
> >
> > Out of curiosity, how long does it actually take you to initialize the
> > hardware?
> >
> > Also, I find it useful to note the units in the name, so:
> >
> > #define PRESTERA_SWITCH_INIT_TIMEOUT_US (30 * 1000 * 1000)
> >
> > BTW, it says 30 seconds in comment, but the call chain where it is used
> > is:
> >
> > prestera_cmd_ret_wait(, PRESTERA_SWITCH_INIT_TIMEOUT)
> > __prestera_cmd_ret(..., wait)
> > prestera_fw_send_req(..., waitms)
> > prestera_fw_cmd_send(..., waitms)
> > prestera_fw_wait_reg32(..., waitms)
> > readl_poll_timeout(..., waitms * 1000)
> >
> > So I think you should actually define it as:
> >
> > #define PRESTERA_SWITCH_INIT_TIMEOUT_MS (30 * 1000)
> >
> > And rename all these 'wait' arguments to 'waitms' so it's clearer which
> > unit they expect.
> >
> [...]
> > > +static int __prestera_cmd_ret(struct prestera_switch *sw,
> > > + enum prestera_cmd_type_t type,
> > > + struct prestera_msg_cmd *cmd, size_t clen,
> > > + struct prestera_msg_ret *ret, size_t rlen,
> > > + int wait)
> > > +{
> > > + struct prestera_device *dev = sw->dev;
> > > + int err;
> > > +
> > > + cmd->type = type;
> > > +
> > > + err = dev->send_req(dev, (u8 *)cmd, clen, (u8 *)ret, rlen, wait);
> > > + if (err)
> > > + return err;
> > > +
> > > + if (ret->cmd.type != PRESTERA_CMD_TYPE_ACK)
> > > + return -EBADE;
> > > + if (ret->status != PRESTERA_CMD_ACK_OK)
> >
> > You don't have more states here other than OK / FAIL ? It might help you
> > in debugging if you include them. You might find trace_devlink_hwerr()
> > useful.
> Thanks, I will consider this.
>
> >
> > > + return -EINVAL;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int prestera_cmd_ret(struct prestera_switch *sw,
> > > + enum prestera_cmd_type_t type,
> > > + struct prestera_msg_cmd *cmd, size_t clen,
> > > + struct prestera_msg_ret *ret, size_t rlen)
> > > +{
> > > + return __prestera_cmd_ret(sw, type, cmd, clen, ret, rlen, 0);
> > > +}
> > > +
> > > +static int prestera_cmd_ret_wait(struct prestera_switch *sw,
> > > + enum prestera_cmd_type_t type,
> > > + struct prestera_msg_cmd *cmd, size_t clen,
> > > + struct prestera_msg_ret *ret, size_t rlen,
> > > + int wait)
> > > +{
> > > + return __prestera_cmd_ret(sw, type, cmd, clen, ret, rlen, wait);
> > > +}
> > > +
> > > +static int prestera_cmd(struct prestera_switch *sw,
> > > + enum prestera_cmd_type_t type,
> > > + struct prestera_msg_cmd *cmd, size_t clen)
> > > +{
> > > + struct prestera_msg_common_resp resp;
> > > +
> > > + return prestera_cmd_ret(sw, type, cmd, clen, , sizeof(resp));
> > > +}
> > > +
> > > +static int prestera_fw_parse_port_evt(u8 *msg, struct prestera_event
> > > *evt)
> > > +{
> > > + struct prestera_msg_event_port *hw_evt;
> > > +
> > > + hw_evt = (struct prestera_msg_event_port *)msg;
> > > +
> > > + evt->port_evt.port_id = hw_evt->port_id;
> > > +
> > > + if (evt->id == PRESTERA_PORT_EVENT_STATE_CHANGED)
> > > + evt->port_evt.data.oper_state = hw_evt->param.oper_state;
> > > + else
> > > + return -EINVAL;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static struct prestera_fw_evt_parser {
> > > + int (*func)(u8 *msg, struct prestera_event *evt);
> > > +}
Re: [net-next 1/6] net: marvell: prestera: Add driver for Prestera family ASIC devices
Hi Ido,
On Sat, May 30, 2020 at 06:48:01PM +0300, Ido Schimmel wrote:
> On Thu, May 28, 2020 at 06:12:40PM +0300, Vadym Kochan wrote:
>
[...]
> Nit: "From" ?
>
> > + PRESTERA_DSA_CMD_FROM_CPU,
> > +};
> > +
> > +struct prestera_dsa_vlan {
> > + u16 vid;
> > + u8 vpt;
> > + u8 cfi_bit;
> > + bool is_tagged;
> > +};
> > +
> > +struct prestera_dsa {
> > + struct prestera_dsa_vlan vlan;
> > + u32 hw_dev_num;
> > + u32 port_num;
> > +};
> > +
> > +int prestera_dsa_parse(struct prestera_dsa *dsa, const u8 *dsa_buf);
> > +int prestera_dsa_build(const struct prestera_dsa *dsa, u8 *dsa_buf);
> > +
> > +#endif /* _PRESTERA_DSA_H_ */
> > diff --git a/drivers/net/ethernet/marvell/prestera/prestera_hw.c
> > b/drivers/net/ethernet/marvell/prestera/prestera_hw.c
> > new file mode 100644
> > index ..3aa3974f957a
> > --- /dev/null
> > +++ b/drivers/net/ethernet/marvell/prestera/prestera_hw.c
> > @@ -0,0 +1,610 @@
> > +// SPDX-License-Identifier: BSD-3-Clause OR GPL-2.0
> > +/* Copyright (c) 2019-2020 Marvell International Ltd. All rights reserved
> > */
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#include "prestera.h"
> > +#include "prestera_hw.h"
> > +
> > +#define PRESTERA_SWITCH_INIT_TIMEOUT 3000 /* 30sec */
>
> Out of curiosity, how long does it actually take you to initialize the
> hardware?
>
> Also, I find it useful to note the units in the name, so:
>
> #define PRESTERA_SWITCH_INIT_TIMEOUT_US (30 * 1000 * 1000)
>
> BTW, it says 30 seconds in comment, but the call chain where it is used
> is:
>
> prestera_cmd_ret_wait(, PRESTERA_SWITCH_INIT_TIMEOUT)
> __prestera_cmd_ret(..., wait)
> prestera_fw_send_req(..., waitms)
> prestera_fw_cmd_send(..., waitms)
> prestera_fw_wait_reg32(..., waitms)
> readl_poll_timeout(..., waitms * 1000)
>
> So I think you should actually define it as:
>
> #define PRESTERA_SWITCH_INIT_TIMEOUT_MS (30 * 1000)
>
> And rename all these 'wait' arguments to 'waitms' so it's clearer which
> unit they expect.
>
[...]
> > +static int __prestera_cmd_ret(struct prestera_switch *sw,
> > + enum prestera_cmd_type_t type,
> > + struct prestera_msg_cmd *cmd, size_t clen,
> > + struct prestera_msg_ret *ret, size_t rlen,
> > + int wait)
> > +{
> > + struct prestera_device *dev = sw->dev;
> > + int err;
> > +
> > + cmd->type = type;
> > +
> > + err = dev->send_req(dev, (u8 *)cmd, clen, (u8 *)ret, rlen, wait);
> > + if (err)
> > + return err;
> > +
> > + if (ret->cmd.type != PRESTERA_CMD_TYPE_ACK)
> > + return -EBADE;
> > + if (ret->status != PRESTERA_CMD_ACK_OK)
>
> You don't have more states here other than OK / FAIL ? It might help you
> in debugging if you include them. You might find trace_devlink_hwerr()
> useful.
Thanks, I will consider this.
>
> > + return -EINVAL;
> > +
> > + return 0;
> > +}
> > +
> > +static int prestera_cmd_ret(struct prestera_switch *sw,
> > + enum prestera_cmd_type_t type,
> > + struct prestera_msg_cmd *cmd, size_t clen,
> > + struct prestera_msg_ret *ret, size_t rlen)
> > +{
> > + return __prestera_cmd_ret(sw, type, cmd, clen, ret, rlen, 0);
> > +}
> > +
> > +static int prestera_cmd_ret_wait(struct prestera_switch *sw,
> > +enum prestera_cmd_type_t type,
> > +struct prestera_msg_cmd *cmd, size_t clen,
> > +struct prestera_msg_ret *ret, size_t rlen,
> > +int wait)
> > +{
> > + return __prestera_cmd_ret(sw, type, cmd, clen, ret, rlen, wait);
> > +}
> > +
> > +static int prestera_cmd(struct prestera_switch *sw,
> > + enum prestera_cmd_type_t type,
> > + struct prestera_msg_cmd *cmd, size_t clen)
> > +{
> > + struct prestera_msg_common_resp resp;
> > +
> > + return prestera_cmd_ret(sw, type, cmd, clen, , sizeof(resp));
> > +}
> > +
> > +static int prestera_fw_parse_port_evt(u8 *msg, struct prestera_event *evt)
> > +{
> > + struct prestera_msg_event_port *hw_evt;
> > +
> > + hw_evt = (struct prestera_msg_event_port *)msg;
> > +
> > + evt->port_evt.port_id = hw_evt->port_id;
> > +
> > + if (evt->id == PRESTERA_PORT_EVENT_STATE_CHANGED)
> > + evt->port_evt.data.oper_state = hw_evt->param.oper_state;
> > + else
> > + return -EINVAL;
> > +
> > + return 0;
> > +}
> > +
> > +static struct prestera_fw_evt_parser {
> > + int (*func)(u8 *msg, struct prestera_event *evt);
> > +} fw_event_parsers[PRESTERA_EVENT_TYPE_MAX] = {
> > + [PRESTERA_EVENT_TYPE_PORT] = {.func = prestera_fw_parse_port_evt},
> > +};
> > +
> > +static struct prestera_fw_event_handler *
> > +__find_event_handler(const struct prestera_switch *sw,
> > +enum prestera_event_type type)
> > +{
> > + struct
Re: [net-next 1/6] net: marvell: prestera: Add driver for Prestera family ASIC devices
On Thu, May 28, 2020 at 06:12:40PM +0300, Vadym Kochan wrote: > Marvell Prestera 98DX326x integrates up to 24 ports of 1GbE with 8 > ports of 10GbE uplinks or 2 ports of 40Gbps stacking for a largely > wireless SMB deployment. > > The current implementation supports only boards designed for the Marvell > Switchdev solution and requires special firmware. > > The core Prestera switching logic is implemented in prestera_main.c, > there is an intermediate hw layer between core logic and firmware. It is > implemented in prestera_hw.c, the purpose of it is to encapsulate hw > related logic, in future there is a plan to support more devices with > different HW related configurations. > > This patch contains only basic switch initialization and RX/TX support > over SDMA mechanism. > > Currently supported devices have DMA access range <= 32bit and require > ZONE_DMA to be enabled, for such cases SDMA driver checks if the skb > allocated in proper range supported by the Prestera device. > > Also meanwhile there is no TX interrupt support in current firmware > version so recycling work is scheduled on each xmit. > > Port's mac address is generated from the switch base mac which may be > provided via device-tree (static one or as nvme cell), or randomly > generated. > > Signed-off-by: Andrii Savka > Signed-off-by: Oleksandr Mazur > Signed-off-by: Serhiy Boiko > Signed-off-by: Serhiy Pshyk > Signed-off-by: Taras Chornyi > Signed-off-by: Volodymyr Mytnyk > Signed-off-by: Vadym Kochan > --- > drivers/net/ethernet/marvell/Kconfig | 1 + > drivers/net/ethernet/marvell/Makefile | 1 + > drivers/net/ethernet/marvell/prestera/Kconfig | 13 + > .../net/ethernet/marvell/prestera/Makefile| 4 + > .../net/ethernet/marvell/prestera/prestera.h | 172 > .../ethernet/marvell/prestera/prestera_dsa.c | 134 +++ > .../ethernet/marvell/prestera/prestera_dsa.h | 37 + > .../ethernet/marvell/prestera/prestera_hw.c | 610 + > .../ethernet/marvell/prestera/prestera_hw.h | 71 ++ > .../ethernet/marvell/prestera/prestera_main.c | 506 +++ > .../ethernet/marvell/prestera/prestera_rxtx.c | 860 ++ > .../ethernet/marvell/prestera/prestera_rxtx.h | 21 + > 12 files changed, 2430 insertions(+) > create mode 100644 drivers/net/ethernet/marvell/prestera/Kconfig > create mode 100644 drivers/net/ethernet/marvell/prestera/Makefile > create mode 100644 drivers/net/ethernet/marvell/prestera/prestera.h > create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_dsa.c > create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_dsa.h > create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_hw.c > create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_hw.h > create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_main.c > create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_rxtx.c > create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_rxtx.h > > diff --git a/drivers/net/ethernet/marvell/Kconfig > b/drivers/net/ethernet/marvell/Kconfig > index 3d5caea096fb..74313d9e1fc0 100644 > --- a/drivers/net/ethernet/marvell/Kconfig > +++ b/drivers/net/ethernet/marvell/Kconfig > @@ -171,5 +171,6 @@ config SKY2_DEBUG > > > source "drivers/net/ethernet/marvell/octeontx2/Kconfig" > +source "drivers/net/ethernet/marvell/prestera/Kconfig" > > endif # NET_VENDOR_MARVELL > diff --git a/drivers/net/ethernet/marvell/Makefile > b/drivers/net/ethernet/marvell/Makefile > index 89dea7284d5b..9f88fe822555 100644 > --- a/drivers/net/ethernet/marvell/Makefile > +++ b/drivers/net/ethernet/marvell/Makefile > @@ -12,3 +12,4 @@ obj-$(CONFIG_PXA168_ETH) += pxa168_eth.o > obj-$(CONFIG_SKGE) += skge.o > obj-$(CONFIG_SKY2) += sky2.o > obj-y+= octeontx2/ > +obj-y+= prestera/ > diff --git a/drivers/net/ethernet/marvell/prestera/Kconfig > b/drivers/net/ethernet/marvell/prestera/Kconfig > new file mode 100644 > index ..76b68613ea7a > --- /dev/null > +++ b/drivers/net/ethernet/marvell/prestera/Kconfig > @@ -0,0 +1,13 @@ > +# SPDX-License-Identifier: GPL-2.0-only > +# > +# Marvell Prestera drivers configuration > +# > + > +config PRESTERA > + tristate "Marvell Prestera Switch ASICs support" > + depends on NET_SWITCHDEV && VLAN_8021Q > + help > + This driver supports Marvell Prestera Switch ASICs family. > + > + To compile this driver as a module, choose M here: the > + module will be called prestera. > diff --git a/drivers/net/ethernet/marvell/prestera/Makefile > b/drivers/net/ethernet/marvell/prestera/Makefile > new file mode 100644 > index ..610d75032b78 > --- /dev/null > +++ b/drivers/net/ethernet/marvell/prestera/Makefile > @@ -0,0 +1,4 @@ > +# SPDX-License-Identifier: GPL-2.0 > +obj-$(CONFIG_PRESTERA) += prestera.o > +prestera-objs:= prestera_main.o prestera_hw.o prestera_dsa.o > \ > +
Re: [net-next 1/6] net: marvell: prestera: Add driver for Prestera family ASIC devices
On Sat, May 30, 2020 at 03:46:22AM +0300, Vadym Kochan wrote: > Hi David, > > On Fri, May 29, 2020 at 05:18:39PM -0700, David Miller wrote: > > > > Please remove all of the __packed attributes. > > > > I looked at your data structures and all of them use fixed sized types > > and are multiples of 4 so the __packed attribute is completely > > unnecessary. > > > > The alignment attribute is also unnecessary so please remove that too. > > Some of the fields are u8, so I assume there might be holes added by > the compiler ? Also these attributes guarantee some ABI compatibility > with FW side, I will try to remove them and check but it sounds for me a bit > risky. Hi Vadym You might want to play with pahole(1). Andrew
Re: [net-next 1/6] net: marvell: prestera: Add driver for Prestera family ASIC devices
Hi David, On Fri, May 29, 2020 at 05:18:39PM -0700, David Miller wrote: > > Please remove all of the __packed attributes. > > I looked at your data structures and all of them use fixed sized types > and are multiples of 4 so the __packed attribute is completely > unnecessary. > > The alignment attribute is also unnecessary so please remove that too. Some of the fields are u8, so I assume there might be holes added by the compiler ? Also these attributes guarantee some ABI compatibility with FW side, I will try to remove them and check but it sounds for me a bit risky.
Re: [net-next 1/6] net: marvell: prestera: Add driver for Prestera family ASIC devices
Please remove all of the __packed attributes. I looked at your data structures and all of them use fixed sized types and are multiples of 4 so the __packed attribute is completely unnecessary. The alignment attribute is also unnecessary so please remove that too.
[net-next 1/6] net: marvell: prestera: Add driver for Prestera family ASIC devices
Marvell Prestera 98DX326x integrates up to 24 ports of 1GbE with 8 ports of 10GbE uplinks or 2 ports of 40Gbps stacking for a largely wireless SMB deployment. The current implementation supports only boards designed for the Marvell Switchdev solution and requires special firmware. The core Prestera switching logic is implemented in prestera_main.c, there is an intermediate hw layer between core logic and firmware. It is implemented in prestera_hw.c, the purpose of it is to encapsulate hw related logic, in future there is a plan to support more devices with different HW related configurations. This patch contains only basic switch initialization and RX/TX support over SDMA mechanism. Currently supported devices have DMA access range <= 32bit and require ZONE_DMA to be enabled, for such cases SDMA driver checks if the skb allocated in proper range supported by the Prestera device. Also meanwhile there is no TX interrupt support in current firmware version so recycling work is scheduled on each xmit. Port's mac address is generated from the switch base mac which may be provided via device-tree (static one or as nvme cell), or randomly generated. Signed-off-by: Andrii Savka Signed-off-by: Oleksandr Mazur Signed-off-by: Serhiy Boiko Signed-off-by: Serhiy Pshyk Signed-off-by: Taras Chornyi Signed-off-by: Volodymyr Mytnyk Signed-off-by: Vadym Kochan --- drivers/net/ethernet/marvell/Kconfig | 1 + drivers/net/ethernet/marvell/Makefile | 1 + drivers/net/ethernet/marvell/prestera/Kconfig | 13 + .../net/ethernet/marvell/prestera/Makefile| 4 + .../net/ethernet/marvell/prestera/prestera.h | 172 .../ethernet/marvell/prestera/prestera_dsa.c | 134 +++ .../ethernet/marvell/prestera/prestera_dsa.h | 37 + .../ethernet/marvell/prestera/prestera_hw.c | 610 + .../ethernet/marvell/prestera/prestera_hw.h | 71 ++ .../ethernet/marvell/prestera/prestera_main.c | 506 +++ .../ethernet/marvell/prestera/prestera_rxtx.c | 860 ++ .../ethernet/marvell/prestera/prestera_rxtx.h | 21 + 12 files changed, 2430 insertions(+) create mode 100644 drivers/net/ethernet/marvell/prestera/Kconfig create mode 100644 drivers/net/ethernet/marvell/prestera/Makefile create mode 100644 drivers/net/ethernet/marvell/prestera/prestera.h create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_dsa.c create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_dsa.h create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_hw.c create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_hw.h create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_main.c create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_rxtx.c create mode 100644 drivers/net/ethernet/marvell/prestera/prestera_rxtx.h diff --git a/drivers/net/ethernet/marvell/Kconfig b/drivers/net/ethernet/marvell/Kconfig index 3d5caea096fb..74313d9e1fc0 100644 --- a/drivers/net/ethernet/marvell/Kconfig +++ b/drivers/net/ethernet/marvell/Kconfig @@ -171,5 +171,6 @@ config SKY2_DEBUG source "drivers/net/ethernet/marvell/octeontx2/Kconfig" +source "drivers/net/ethernet/marvell/prestera/Kconfig" endif # NET_VENDOR_MARVELL diff --git a/drivers/net/ethernet/marvell/Makefile b/drivers/net/ethernet/marvell/Makefile index 89dea7284d5b..9f88fe822555 100644 --- a/drivers/net/ethernet/marvell/Makefile +++ b/drivers/net/ethernet/marvell/Makefile @@ -12,3 +12,4 @@ obj-$(CONFIG_PXA168_ETH) += pxa168_eth.o obj-$(CONFIG_SKGE) += skge.o obj-$(CONFIG_SKY2) += sky2.o obj-y += octeontx2/ +obj-y += prestera/ diff --git a/drivers/net/ethernet/marvell/prestera/Kconfig b/drivers/net/ethernet/marvell/prestera/Kconfig new file mode 100644 index ..76b68613ea7a --- /dev/null +++ b/drivers/net/ethernet/marvell/prestera/Kconfig @@ -0,0 +1,13 @@ +# SPDX-License-Identifier: GPL-2.0-only +# +# Marvell Prestera drivers configuration +# + +config PRESTERA + tristate "Marvell Prestera Switch ASICs support" + depends on NET_SWITCHDEV && VLAN_8021Q + help + This driver supports Marvell Prestera Switch ASICs family. + + To compile this driver as a module, choose M here: the + module will be called prestera. diff --git a/drivers/net/ethernet/marvell/prestera/Makefile b/drivers/net/ethernet/marvell/prestera/Makefile new file mode 100644 index ..610d75032b78 --- /dev/null +++ b/drivers/net/ethernet/marvell/prestera/Makefile @@ -0,0 +1,4 @@ +# SPDX-License-Identifier: GPL-2.0 +obj-$(CONFIG_PRESTERA) += prestera.o +prestera-objs := prestera_main.o prestera_hw.o prestera_dsa.o \ + prestera_rxtx.o diff --git a/drivers/net/ethernet/marvell/prestera/prestera.h b/drivers/net/ethernet/marvell/prestera/prestera.h new file mode 100644 index ..5079d872e18a --- /dev/null +++ b/drivers/net/ethernet/marvell/prestera/prestera.h @@ -0,0 +1,172 @@ +/*
