RE: RE: [PATCH RFC 02/11] dpaa_eth: add support for DPAA Ethernet
From: Madalin-Cristian Bucur
Date: Wed Mar 18 2015 - 14:05:09 EST
> -----Original Message-----
> From: Kumar Gala [mailto:galak@xxxxxxxxxxxxxxxxxxx]
>
> On Mar 17, 2015, at 1:58 PM, Madalin Bucur <madalin.bucur@xxxxxxxxxxxxx>
> wrote:
>
> > This introduces the Freescale Data Path Acceleration Architecture
> > (DPAA) Ethernet driver (dpaa_eth) that builds upon the DPAA QMan,
> > BMan, PAMU and FMan drivers to deliver Ethernet connectivity on
> > the Freescale DPAA QorIQ platforms.
> >
> > Signed-off-by: Madalin Bucur <madalin.bucur@xxxxxxxxxxxxx>
> > ---
[snip]
> > diff --git a/drivers/net/ethernet/freescale/Makefile
> b/drivers/net/ethernet/freescale/Makefile
> > index 71debd1..b6c10ab 100644
> > --- a/drivers/net/ethernet/freescale/Makefile
> > +++ b/drivers/net/ethernet/freescale/Makefile
> > @@ -12,6 +12,7 @@ obj-$(CONFIG_FS_ENET) += fs_enet/
> > obj-$(CONFIG_FSL_PQ_MDIO) += fsl_pq_mdio.o
> > obj-$(CONFIG_FSL_XGMAC_MDIO) += xgmac_mdio.o
> > obj-$(CONFIG_GIANFAR) += gianfar_driver.o
> > +obj-$(if $(CONFIG_FSL_DPAA_ETH),y) += dpaa/
>
> Why isn't
>
> obj-$(CONFIG_FSL_DPAA_ETH) += dpaa/
>
> enough?
It is, will fix.
> > diff --git a/drivers/net/ethernet/freescale/dpaa/Kconfig
> b/drivers/net/ethernet/freescale/dpaa/Kconfig
> > new file mode 100644
> > index 0000000..7ef703c
> > --- /dev/null
> > +++ b/drivers/net/ethernet/freescale/dpaa/Kconfig
> > @@ -0,0 +1,49 @@
> > +menuconfig FSL_DPAA_ETH
> > + tristate "DPAA Ethernet"
> > + depends on FSL_SOC && FSL_BMAN && FSL_QMAN && FSL_FMAN
> > + select PHYLIB
> > + select FSL_FMAN_MAC
> > + ---help---
> > + Data Path Acceleration Architecture Ethernet driver,
> > + supporting the Freescale QorIQ chips.
> > + Depends on Freescale Buffer Manager and Queue Manager
> > + driver and Frame Manager Driver.
> > +
> > +if FSL_DPAA_ETH
> > +
> > +config FSL_DPAA_CS_THRESHOLD_1G
> > + hex "Egress congestion threshold on 1G ports"
> > + depends on FSL_DPAA_ETH
> > + range 0x1000 0x10000000
> > + default "0x06000000"
> > + ---help---
> > + The size in bytes of the egress Congestion State notification
> threshold on 1G ports.
> > + The 1G dTSECs can quite easily be flooded by cores doing Tx in a
> tight loop
> > + (e.g. by sending UDP datagrams at "while(1) speed"),
> > + and the larger the frame size, the more acute the problem.
> > + So we have to find a balance between these factors:
> > + - avoiding the device staying congested for a prolonged time
> (risking
> > + the netdev watchdog to fire - see also the tx_timeout module
> param);
> > + - affecting performance of protocols such as TCP, which otherwise
> > + behave well under the congestion notification mechanism;
> > + - preventing the Tx cores from tightly-looping (as if the
> congestion
> > + threshold was too low to be effective);
> > + - running out of memory if the CS threshold is set too high.
> > +
> > +config FSL_DPAA_CS_THRESHOLD_10G
> > + hex "Egress congestion threshold on 10G ports"
> > + depends on FSL_DPAA_ETH
> > + range 0x1000 0x20000000
> > + default "0x10000000"
> > + ---help ---
> > + The size in bytes of the egress Congestion State notification
> threshold on 10G ports.
> > +
> > +config FSL_DPAA_INGRESS_CS_THRESHOLD
> > + hex "Ingress congestion threshold on FMan ports"
> > + depends on FSL_DPAA_ETH
> > + default "0x10000000"
> > + ---help---
> > + The size in bytes of the ingress tail-drop threshold on FMan ports.
> > + Traffic piling up above this value will be rejected by QMan and
> discarded by FMan.
> > +
>
> Do these thresholds really need to be kconfig options? Are they not
> changeable at runtime?
Probably Bogdan thought it was an easy way to provide basic means to tweak them
and also provide some explanation for them. The current setting is chosen to keep
them mostly out of the way but still address a corner case a customer reported, if
I remember correctly. No runtime change is supported.
> > diff --git a/drivers/net/ethernet/freescale/dpaa/Makefile
> b/drivers/net/ethernet/freescale/dpaa/Makefile
> > new file mode 100644
> > index 0000000..bdeb04e
> > --- /dev/null
> > +++ b/drivers/net/ethernet/freescale/dpaa/Makefile
> > @@ -0,0 +1,14 @@
> > +#
> > +# Makefile for the Freescale DPAA Ethernet controllers
> > +#
> > +ccflags-y += -DVERSION=\"\"
>
> Is this really needed, if so we need to fix that.
Unused, will remove.
[snip]
> > +
> > +#define DPA_DESCRIPTION "FSL DPAA Ethernet driver"
> > +
> > +MODULE_LICENSE("Dual BSD/GPL");
> > +
> > +MODULE_AUTHOR("Andy Fleming <afleming@xxxxxxxxxxxxx>");
> > +
> > +MODULE_DESCRIPTION(DPA_DESCRIPTION);
>
> These are typically at the end of the file, and kept together (ie no blank lines
> between them)
Will move them.
[snip]
> > +
> > +static struct platform_device_id dpa_devtype[] = {
> > + {
> > + .name = "dpaa-ethernet",
> > + .driver_data = 0,
> > + }, {
> > + }
> > +};
> > +MODULE_DEVICE_TABLE(platform, dpa_devtype);
> > +
> > +static struct platform_driver dpa_driver = {
> > + .driver = {
> > + .name = KBUILD_MODNAME,
> > + .owner = THIS_MODULE,
> > + },
> > + .id_table = dpa_devtype,
> > + .probe = dpaa_eth_priv_probe,
> > + .remove = dpa_remove
> > +};
> > +
> > +static int __init __cold dpa_load(void)
> > +{
> > + int _errno;
> > +
> > + pr_info(DPA_DESCRIPTION " (" VERSION ")\n");
> > +
> > + /* initialise dpaa_eth mirror values */
> > + dpa_rx_extra_headroom = fm_get_rx_extra_headroom();
> > + dpa_max_frm = fm_get_max_frm();
> > +
> > + _errno = platform_driver_register(&dpa_driver);
> > + if (unlikely(_errno < 0)) {
> > + pr_err(KBUILD_MODNAME
> > + ": %s:%hu:%s(): platform_driver_register() = %d\n",
> > + KBUILD_BASENAME ".c", __LINE__, __func__,
> _errno);
> > + }
> > +
> > + pr_debug(KBUILD_MODNAME ": %s:%s() ->\n",
> > + KBUILD_BASENAME ".c", __func__);
> > +
> > + return _errno;
> > +}
> > +module_init(dpa_load);
> > +
> > +static void __exit __cold dpa_unload(void)
> > +{
> > + pr_debug(KBUILD_MODNAME ": -> %s:%s()\n",
> > + KBUILD_BASENAME ".c", __func__);
> > +
> > + platform_driver_unregister(&dpa_driver);
> > +
> > + /* Only one channel is used and needs to be relased after all
> > + * interfaces are removed
> > + */
> > + dpa_release_channel();
> > +
> > + pr_debug(KBUILD_MODNAME ": %s:%s() ->\n",
> > + KBUILD_BASENAME ".c", __func__);
> > +}
> > +module_exit(dpa_unload);
>
> What's the point of this dpa platform driver?
>
It replaces the device tree node probing, based on the feedback [1] received from Scott.
The FMan MAC driver creates a device for the Ethernet driver to probe against in
dpaa_eth_add_device(), see [2]. This replaces the rejected model where a configuration
node for Ethernet was added to the device tree with a reference to the FMan MAC node.
The platform device provides the Ethernet driver with FMan MAC identification and the
MAC dev needed back in the adjust_link() call:
...
struct dpaa_eth_data {
struct device_node *mac_node;
struct mac_device *mac_dev;
int mac_hw_id;
int fman_hw_id;
};
...
static void adjust_link(struct net_device *net_dev)
{
struct device *dev = net_dev->dev.parent;
struct dpaa_eth_data *eth_data = dev->platform_data;
struct mac_device *mac_dev = eth_data->mac_dev;
...
[snip]
> > +static inline int __hot dpa_xmit(struct dpa_priv_s *priv,
> > + struct rtnl_link_stats64 *percpu_stats,
> > + int queue,
> > + struct qm_fd *fd)
>
> Is there a reason this isnt just in the .c file?
It's also used by the un-submitted advanced drivers that compile against the
submitted code (there are a few other things used in common - such as the code
extracted in the dpaa_eth_common.c file).
> > +EXPORT_SYMBOL(dpa_mac_dev_get);
>
> why is this exported?
>
Same reason as above.
[snip]
Thank you for your time,
Madalin
[1] https://patchwork.ozlabs.org/patch/415831/
[2] https://patchwork.ozlabs.org/patch/448874/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/