On Thu, Feb 16, 2017 at 08:05:24PM +0100, Maxime Ripard wrote:
> Hi,
> 

[...]
> > +
> > +struct emac_variant {
> > +   u32 default_syscon_value;
> 
> Why do you need a default value? Can't you read it from the syscon
> directly?
> 

Why not, but you can see the default value as "value for disabled state".
My fear is that something (uboot) modify it (keep it activated) before driver 
load.

[...]
> > +static void sun8i_dwmac_dump_regs(void __iomem *ioaddr)
> > +{
> > +   int i;
> > +
> > +   pr_info(" DMA registers\n");
> 
> Logging this as pr_info is bad already...
> 
> > +   for (i = 0; i < 0xC8; i += 4) {
> > +           if (i == 0x32 || i == 0x3C)
> > +                   continue;
> > +           pr_err("Reg 0x%x: 0x%08x\n", i, readl(ioaddr + i));
> 
> ... But this is worse.
> 
> Why do you need to do that? Can't you create a file in debugfs
> instead?
> 

I just do as other glue does. But yes this is uglyi, no excuse.
Reworking all stmmac register dump (ethtool, stmmac_ops->dump_regs and 
stmmac_dma_ops->dump_regs) was on my todo list,
but I postponed it.

I will propose something better based on debugfs.

[...]
> > +static void sun8i_dwmac_dma_start_tx(void __iomem *ioaddr)
> > +{
> > +   u32 v;
> > +
> > +   v = readl(ioaddr + EMAC_TX_CTL0);
> > +   v |= EMAC_TX_TRANSMITTER_EN;
> > +   writel(v, ioaddr + EMAC_TX_CTL0);
> > +
> > +   v = readl(ioaddr + EMAC_TX_CTL1);
> > +   v |= EMAC_TX_DMA_START;
> > +   v |= EMAC_TX_DMA_EN;
> > +   writel(v, ioaddr + EMAC_TX_CTL1);
> 
> This is a bit worrying. There's not a single lock in your driver,
> while you have a significant number of read / modify / write.
> 
> Where is the locking handled?
> 

All thoses function are handled by the "stmmac_ops framework", all other glue 
drivers does not lock anything.
The few functions that need locking already got it on the calling stmmac side.

[...]
> > +static int sun8i_dwmac_init(struct platform_device *pdev, void *priv)
> > +{
> > +   struct sunxi_priv_data *gmac = priv;
> > +   int ret;
> > +
> > +   if (gmac->regulator) {
> > +           ret = regulator_enable(gmac->regulator);
> > +           if (ret) {
> > +                   dev_err(&pdev->dev, "Fail to enable regulator\n");
> > +                   return ret;
> > +           }
> > +   }
> > +
> > +   ret = clk_prepare_enable(gmac->tx_clk);
> > +   if (ret) {
> > +           dev_err(&pdev->dev, "Could not enable AHB clock\n");
> 
> If that call fails, you leave the regulator (if there was any) enabled.
> 

I will fix it

> > +           return ret;
> > +   }
> > +
> > +   return 0;
> > +}
> > +
> > +static void sun8i_dwmac_core_init(struct mac_device_info *hw, int mtu)
> > +{
> > +   void __iomem *ioaddr = hw->pcsr;
> > +   u32 v;
> > +
> > +   v = (8 << 24);/* burst len */
> > +   writel(v, ioaddr + EMAC_BASIC_CTL1);
> 
> do you need an intermediate value? you should make a define for that
> too.
> 

I will fix it

[...]
> > +static int sun8i_dwmac_set_syscon(struct stmmac_priv *priv)
> > +{
> > +   struct sunxi_priv_data *gmac = priv->plat->bsp_priv;
> > +   struct device_node *node = priv->device->of_node;
> > +   int ret;
> > +   u32 reg, val;
> > +
> > +   reg = gmac->variant->default_syscon_value;
> > +
> > +   if (gmac->variant->internal_phy) {
> > +           if (!gmac->use_internal_phy) {
> > +                   /* switch to external PHY interface */
> > +                   reg &= ~H3_EPHY_SELECT;
> > +           } else {
> > +                   reg |= H3_EPHY_SELECT;
> > +                   reg &= ~H3_EPHY_SHUTDOWN;
> > +                   dev_info(priv->device, "Select internal_phy %x\n", reg);
> 
> The logging level is too high
> 

I will reduce it

> > +
> > +                   if (of_property_read_bool(priv->plat->phy_node,
> > +                                             "allwinner,leds-active-low"))
> > +                           reg |= H3_EPHY_LED_POL;
> > +                   else
> > +                           reg &= ~H3_EPHY_LED_POL;
> > +
> > +                   ret = of_mdio_parse_addr(priv->device,
> > +                                            priv->plat->phy_node);
> > +                   if (ret < 0) {
> > +                           dev_err(priv->device, "Could not parse MDIO 
> > addr\n");
> > +                           return ret;
> > +                   }
> > +                   /* of_mdio_parse_addr returns a valid (0 ~ 31) PHY
> > +                    * address. No need to mask it again.
> > +                    */
> > +                   reg |= ret << H3_EPHY_ADDR_SHIFT;
> > +           }
> > +   }
> > +
> > +   if (!of_property_read_u32(node, "allwinner,tx-delay", &val)) {
> 
> How do you compute it? Can't this be done through auto-training?
> 

The value is the same as used in vendor BSP kernel.
I do not understand what you mean by auto-training.

> > +           dev_info(priv->device, "set tx-delay to %x\n", val);
> 
> change the logging level here too.
> 

I agree

> > +           if (val <= SYSCON_ETXDC_MASK) {
> > +                   reg &= ~(SYSCON_ETXDC_MASK << SYSCON_ETXDC_SHIFT);
> > +                   reg |= (val << SYSCON_ETXDC_SHIFT);
> > +           } else {
> > +                   dev_warn(priv->device, "Invalid TX clock delay: %d\n",
> > +                            val);
> 
> If it's invalid, why don't you treat it as an error and return?
> 

Ok

[...]
> > +static struct mac_device_info *sun8i_dwmac_setup(void __iomem *ioaddr,
> > +                                            int mcbins,
> > +                                            int perfect_uc_entries,
> > +                                            int *synopsys_id)
> > +{
> > +   struct mac_device_info *mac;
> > +
> > +   mac = kzalloc(sizeof(const struct mac_device_info), GFP_KERNEL);
> > +   if (!mac)
> > +           return NULL;
> 
> Do you ever free that memory?
> 

Good catch, I believed that the "stmmac framework" would free it.
I will send a fix for this memory leak.

[...]
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c 
> > b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> > index 5ff6bc4..11db658 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> > @@ -450,6 +450,9 @@ static void stmmac_ethtool_gregs(struct net_device *dev,
> >             for (i = 0; i < 22; i++)
> >                     reg_space[i + 55] =
> >                         readl(priv->ioaddr + (DMA_BUS_MODE + (i * 4)));
> > +   } else if (priv->plat->has_sun8i) {
> 
> Surely we don't want to add a new flag to the common structure for
> every new platform supported.
> 
> Can't you base that on the compatible instead?

This part will be fixed with the debugfs speaked early in the mail.

But yes I have tried to avoid use of has_sun8i at maximum.

> 
> Thanks a lot for your work,
> Maxime
> 

Thanks for the review

Corentin Labbe

-- 
You received this message because you are subscribed to the Google Groups 
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to linux-sunxi+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Reply via email to