Mark Rutland <mark.rutland@xxxxxxx> :
> On Wed, Mar 12, 2014 at 01:28:00PM +0000, Byungho An wrote:
> > From: Siva Reddy <siva.kallam@xxxxxxxxxxx>
> >
> > This patch adds support for Samsung 10Gb ethernet driver(sxgbe).
> >
> > - sxgbe core initialization
> > - Tx and Rx support
> > - MDIO support
> > - ISRs for Tx and Rx
> > - ifconfig support to driver
> >
> > Signed-off-by: Siva Reddy Kallam <siva.kallam@xxxxxxxxxxx>
> > Signed-off-by: Vipul Pandya <vipul.pandya@xxxxxxxxxxx>
> > Signed-off-by: Girish K S <ks.giri@xxxxxxxxxxx>
> > Neatening-by: Joe Perches <joe@xxxxxxxxxxx>
> > Signed-off-by: Byungho An <bh74.an@xxxxxxxxxxx>
> > ---
>
> [...]
>
> > diff --git a/Documentation/devicetree/bindings/net/samsung-sxgbe.txt
> > b/Documentation/devicetree/bindings/net/samsung-sxgbe.txt
> > new file mode 100644
> > index 0000000..f2abf65
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/samsung-sxgbe.txt
>
> Please split the binding into a separate patch from the code (it makes it
far
> easier for us DT guys to find that portions relevant to use).
>
> Is there any public documentation?
No.
>
> > @@ -0,0 +1,39 @@
> > +* Samsung 10G Ethernet driver (SXGBE)
> > +
> > +Required properties:
> > +- compatible: Should be "samsung,sxgbe-v2.0a"
> > +- reg: Address and length of the register set for the device
> > +- interrupt-parent: Should be the phandle for the interrupt
> > +controller
> > + that services interrupts for this device
> > +- interrupts: Should contain the SXGBE interrupts
>
> How many, in which order, what are each of them for?
total = 26, first mandatory interrupt common to all, followed by 8 tx
interrupt (which can vary based on platform), 16 rx interrupts (which can
vary based on platform) and a optional lpi interrupt
>
> > +- samsung,burst-map Program the possible bursts supported by
sxgbe
> > +- samsung,fixed-burst Program the DMA to use the fixed burst mode
> > +- samsung,adv-addr-mode program the DMA to use Enhanced address
> mode
>
> What are valid values?
>
> Units/types?
>
> Please describe what these actually do.
These will be updated in the binding document as per your review.
>
> > +- samsung,force_thresh_dma_mode Force DMA to use the threshold
> mode
> > for
> > + both tx and rx
>
> Odd formatting here.
>
> s/_/-/ in property names please.
>
> When and why should this property be set in a dt?
Addressed in the new document
>
> > +- samsung,force_sf_dma_mode Force DMA to use the Store and
> Forward
> > + mode for both tx and rx. This flag is
> > + ignored if force_thresh_dma_mode is set.
>
> Likewise, for all points.
OK.
>
> [...]
>
> > +/* Clean the tx descriptor as soon as the tx irq is received */
> > +static void sxgbe_release_tx_desc(struct sxgbe_tx_norm_desc *p) {
> > + memset(p, 0, sizeof(struct sxgbe_tx_norm_desc));
>
> You can use sizeof(*p) here.
OK.
>
> [...]
>
> > +static int sxgbe_probe_config_dt(struct platform_device *pdev,
> > + struct sxgbe_plat_data *plat,
> > + const char **mac) {
> > + struct device_node *np = pdev->dev.of_node;
> > + struct sxgbe_dma_cfg *dma_cfg;
> > + u32 phy_addr;
> > +
> > + if (!np)
> > + return -ENODEV;
> > +
> > + *mac = of_get_mac_address(np);
>
> I see that of_get_mac_address returns a *void rather than *char, but it's
> always a string of hex digits. Would it make sense to change the
> of_get_mac_address prototype to return a char* ?
This implementation is of_ specific, our driver only uses it.
>
> > + plat->interface = of_get_phy_mode(np);
> > +
> > + plat->bus_id = of_alias_get_id(np, "ethernet");
> > + if (plat->bus_id < 0)
> > + plat->bus_id = 0;
>
> This wasn't mentioned in the binding.
It doesn't need to be in the binding document, because we get the alias id
by passing the node name. Here ethernet is not a property it is the dt node
name
e.g.
ethernet@xxxxx {
};
>
> > +
> > + of_property_read_u32(np, "samsung,phy-addr", &plat->phy_addr);
>
> Neither was this.
OK.
>
> > +
> > + plat->mdio_bus_data = devm_kzalloc(&pdev->dev,
> > + sizeof(struct
> > sxgbe_mdio_bus_data),
> > + GFP_KERNEL);
> > +
> > + if (of_device_is_compatible(np, "samsung,sxgbe-v2.0a"))
> > + plat->pmt = 1;
>
> Only one compatible string is documented. When would this _not_ be the
> case?
Removed as it is unused
>
> [...]
>
> > +static int sxgbe_platform_probe(struct platform_device *pdev) {
> > + int ret = 0;
> > + int loop = 0;
> > + int index1, index2;
> > + struct resource *res;
> > + struct device *dev = &pdev->dev;
> > + void __iomem *addr = NULL;
> > + struct sxgbe_priv_data *priv = NULL;
> > + struct sxgbe_plat_data *plat_dat = NULL;
> > + const char *mac = NULL;
> > + int total_dma_channels = SXGBE_TX_QUEUES + SXGBE_RX_QUEUES;
> > +
> > + /* Get memory resource */
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + if (!res)
> > + return -ENODEV;
> > +
> > + addr = devm_ioremap_resource(dev, res);
> > + if (IS_ERR(addr))
> > + return PTR_ERR(addr);
> > +
> > + plat_dat = pdev->dev.platform_data;
>
> This is a new dt-driven driver. Why would you need additional platform
data?
> Why can this information not come from dt?
Same above.
>
> > + if (pdev->dev.of_node) {
> > + if (!plat_dat)
> > + plat_dat = devm_kzalloc(&pdev->dev,
> > + sizeof(struct
> > sxgbe_plat_data),
> > + GFP_KERNEL);
> > + if (!plat_dat)
> > + return -ENOMEM;
> > +
> > + ret = sxgbe_probe_config_dt(pdev, plat_dat, &mac);
> > + if (ret) {
> > + pr_err("%s: main dt probe failed\n", __func__);
> > + return ret;
> > + }
> > + }
> > +
> > + priv = sxgbe_dvr_probe(&(pdev->dev), plat_dat, addr);
> > + if (!priv) {
> > + pr_err("%s: main driver probe failed\n", __func__);
> > + return -ENODEV;
> > + }
> > +
> > + /* Get MAC address if available (DT) */
> > + if (mac)
> > + ether_addr_copy(priv->dev->dev_addr, mac);
> > +
> > + /* Get the SXGBE common INT information */
> > + priv->dev->irq = irq_of_parse_and_map(dev->of_node, loop++);
> > + if (priv->dev->irq <= 0) {
> > + dev_err(dev, "sxgbe common irq parsing failed\n");
> > + return -EINVAL;
> > + }
>
> So the first IRQ is assumed to be a common interrupt...
Right.
>
> > +
> > + /* Get the TX/RX IRQ numbers */
> > + for (index1 = 0, index2 = 0 ; loop < total_dma_channels; loop++)
{
> > + if (loop < SXGBE_TX_QUEUES) {
> > + (priv->txq[index1])->irq_no =
> > + irq_of_parse_and_map(dev->of_node,
> > + loop);
>
> then there's a TX IRQ for each queue, in order...
>
> > + if ((priv->txq[index1++])->irq_no <= 0) {
> > + dev_err(dev, "sxgbe tx irq parsing
> > failed\n");
> > + return -EINVAL;
> > + }
> > + } else {
> > + (priv->rxq[index2])->irq_no =
> > + irq_of_parse_and_map(dev->of_node,
> > + loop);
>
> then an RX IRQ for each queue, in order.
>
> That should be in the binding document.
taken care in the binding document.
Thanks Mark for your comments
>
> Cheers,
> Mark.
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in the
body
> of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at
> http://vger.kernel.org/majordomo-info.html
--
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
