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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
