> > +static int airoha_set_gdma_port(struct airoha_eth *eth, int port, bool enable)
> > +{
> > + u32 vip_port, cfg_addr, val = enable ? FE_DP_PPE : FE_DP_DROP;
> > +
> > + switch (port) {
> > + case 0:
> > + vip_port = BIT(22);
> > + cfg_addr = REG_GDM3_FWD_CFG;
> > + break;
> > + case 1:
> > + vip_port = BIT(23);
> > + cfg_addr = REG_GDM3_FWD_CFG;
> > + break;
> > + case 2:
> > + vip_port = BIT(25);
> > + cfg_addr = REG_GDM4_FWD_CFG;
> > + break;
> > + case 4:
> > + vip_port = BIT(24);
> > + cfg_addr = REG_GDM4_FWD_CFG;
> > + break;
>
> Please add some #defines for the BIT(), so there is descriptive
> names. Please do the same other places you have BIT macros, it makes
> the code easier to understand.
ack, I will do in v2
>
> > +static int airoha_set_gdma_ports(struct airoha_eth *eth, bool enable)
> > +{
> > + const int port_list[] = { 0, 1, 2, 4 };
> > + int i;
>
> Maybe add a comment about port 3?
>
> > +static void airoha_fe_vip_setup(struct airoha_eth *eth)
> > +{
> > + airoha_fe_wr(eth, REG_FE_VIP_PATN(3), 0x8863); /* ETH->PPP (0x8863) */
>
> Rather than a comment, use ETH_P_PPP_DISC
ack, I will do in v2
>
> > + airoha_fe_wr(eth, REG_FE_VIP_EN(3), PATN_FCPU_EN_MASK | PATN_EN_MASK);
> > +
> > + airoha_fe_wr(eth, REG_FE_VIP_PATN(4), 0xc021); /* PPP->LCP (0xc021) */
>
> PPP_LCP
ack, I will do in v2
>
> > + airoha_fe_wr(eth, REG_FE_VIP_EN(4),
> > + PATN_FCPU_EN_MASK | FIELD_PREP(PATN_TYPE_MASK, 1) |
> > + PATN_EN_MASK);
> > +
> > + airoha_fe_wr(eth, REG_FE_VIP_PATN(6), 0x8021); /* PPP->IPCP (0x8021) */
>
> PPP_IPCP
>
> etc...
ack, I will do in v2
>
> > +static int airoha_qdma_fill_rx_queue(struct airoha_queue *q)
> > +{
> > + struct airoha_eth *eth = q->eth;
> > + struct device *dev = eth->net_dev->dev.parent;
> > + int qid = q - ð->q_rx[0], nframes = 0;
>
> Reverse Christmass tree. Which means you will need to move some of the
> assignments into the body of the function.
ack, I will fix it in v2
>
> > +static int airoha_dev_open(struct net_device *dev)
> > +{
> > + struct airoha_eth *eth = netdev_priv(dev);
> > + int err;
> > +
> > + if (netdev_uses_dsa(dev))
> > + airoha_fe_set(eth, REG_GDM1_INGRESS_CFG, GDM1_STAG_EN_MASK);
> > + else
> > + airoha_fe_clear(eth, REG_GDM1_INGRESS_CFG, GDM1_STAG_EN_MASK);
>
> Does this imply the hardware can be used in a situation where it is
> not connected to a switch? Does it have an MII and MDIO bus? Could a
> PHY be connected? If it can be used as a conventional NIC, we need to
> ensure there is a path to use usage without an ABI breakage.
I tested the driver removing the dsa switch from the board dts and
resetting the switch at bootstrap in order to erase uboot running
configuration. Doing so the driver works fine.
Moreover, I will add in the future connections to different phys through
GDM{2,3,4} ports (so far we support just GDM1 that is connected the mt7530
switch).
>
> > +static int airoha_register_debugfs(struct airoha_eth *eth)
> > +{
> > + eth->debugfs_dir = debugfs_create_dir(KBUILD_MODNAME, NULL);
> > + if (IS_ERR(eth->debugfs_dir))
> > + return PTR_ERR(eth->debugfs_dir);
>
> No error checking should be performed with debugfs calls. Just keep
> going and it will work out O.K.
ack, I will fix it in v2
>
> > + err = of_get_ethdev_address(np, dev);
> > + if (err) {
> > + if (err == -EPROBE_DEFER)
> > + return err;
> > +
> > + eth_hw_addr_random(dev);
> > + dev_err(&pdev->dev, "generated random MAC address %pM\n",
> > + dev->dev_addr);
>
> dev_info() would be better here, since it is not considered an error.
ack, I will fix it in v2
>
> > + err = airoha_hw_init(eth);
> > + if (err)
> > + return err;
> > +
> > + airoha_qdma_start_napi(eth);
> > + err = register_netdev(dev);
> > + if (err)
> > + return err;
> > +
> > + err = airoha_register_debugfs(eth);
> > + if (err)
> > + return err;
> > +
> > + platform_set_drvdata(pdev, eth);
>
> Is this required? As soon as you call register_netdev(), the device is
> live and in use. It can be sending the first packets before the
> function returns. So if anything needs this connection between the
> platform data and the eth, it will not be in place, and bad things
> will happen.
it is used just in the remove callback but I can move it before
register_netdev() and I will set it to NULL in case of error.
>
> > +static inline void airoha_qdma_start_napi(struct airoha_eth *eth)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < ARRAY_SIZE(eth->q_tx_irq); i++)
> > + napi_enable(ð->q_tx_irq[i].napi);
> > +
> > + airoha_qdma_for_each_q_rx(eth, i)
> > + napi_enable(ð->q_rx[i].napi);
> > +}
> > +
> > +static inline void airoha_qdma_stop_napi(struct airoha_eth *eth)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < ARRAY_SIZE(eth->q_tx_irq); i++)
> > + napi_disable(ð->q_tx_irq[i].napi);
> > +
> > + airoha_qdma_for_each_q_rx(eth, i)
> > + napi_disable(ð->q_rx[i].napi);
> > +}
>
> These seem off to be in a header file?
ack, I will move them in .c.
Regards,
Lorenzo
>
> Andrew
>
> ---
> pw-bot: cr
Attachment:
signature.asc
Description: PGP signature
