Re: [PATCH V2 1/7] net: sxgbe: add basic framework for Samsung 10Gb ethernet driver

2014-03-17 Thread Mark Rutland 
On Mon, Mar 17, 2014 at 05:51:21AM +, Byungho An wrote:
 Mark Rutland mark.rutl...@arm.com :
  On Wed, Mar 12, 2014 at 01:28:00PM +, Byungho An wrote:
   From: Siva Reddy siva.kal...@samsung.com
  
   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.kal...@samsung.com
   Signed-off-by: Vipul Pandya vipul.pan...@samsung.com
   Signed-off-by: Girish K S ks.g...@samsung.com
   Neatening-by: Joe Perches j...@perches.com
   Signed-off-by: Byungho An bh74...@samsung.com

[...]

   +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.

Sure, this was a general observation that wasn't specific to this
driver.

 
 
   +   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@x {
 
 };

Huh?

The acquisition of the alias ID is not performed based on the node name,
but rather the node and stem of the entry in the /alias node that points
to it.

Consider this dts fragment:

/ {
...

alias {
ethernet5 = /bus/arbitrarily_named_sxgbe@deadbeef;
};

bus {
...

arbtirarily_named_sxgbe@0xdeadbeef {
compatible = samsung,sxgbe-v2.0a;
reg = 0xdeadbeef 0, ... ;
...
};
};
}

The above call to of_alias_get_id(np, ethernet) would return 5 for the
sxgbe node, despite the node name being completely arbitrary.  While the
node should probably be named ethernet, this doesn't matter for the
purpose of alias id handling.

If your binding has a particular form of alias, please document it so DT
authors can be aware of it and make use of it.

That said we probably need better guidance on the use of aliases. The
usage model is undocumented beyond aliases being informational, which is
somewhat vague. Unfortunately the description in ePAPR also glosses over
the expected usage.

Grant, would you be able to elaborate on the expected usage of aliases?

Should alias names always be from a standard set (e.g. ethernet,
serial, pci per ePAPR), or are binding-specific names (e.g. the
Exynos S-Scalar's gsc) something we're happy with?

Are aliases allowed to be required, or are they always supposed to be an
optional hint to the OS?

Cheers,
Mark.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH V2 1/7] net: sxgbe: add basic framework for Samsung 10Gb ethernet driver

2014-03-16 Thread Byungho An 
Mark Rutland mark.rutl...@arm.com :
 On Wed, Mar 12, 2014 at 01:28:00PM +, Byungho An wrote:
  From: Siva Reddy siva.kal...@samsung.com
 
  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.kal...@samsung.com
  Signed-off-by: Vipul Pandya vipul.pan...@samsung.com
  Signed-off-by: Girish K S ks.g...@samsung.com
  Neatening-by: Joe Perches j...@perches.com
  Signed-off-by: Byungho An bh74...@samsung.com
  ---
 
 [...]
 
  diff --git a/Documentation/devicetree/bindings/net/samsung-sxgbe.txt
  b/Documentation/devicetree/bindings/net/samsung-sxgbe.txt
  new file mode 100644
  index 000..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-mapProgram the possible bursts supported by
sxgbe
  +- samsung,fixed-burst  Program the DMA to use the fixed burst mode
  +- samsung,adv-addr-modeprogram 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_modeForce 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_modeForce 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@x {

};

 
  +
  +   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);

Re: [PATCH V2 1/7] net: sxgbe: add basic framework for Samsung 10Gb ethernet driver

2014-03-12 Thread Mark Rutland 
On Wed, Mar 12, 2014 at 01:28:00PM +, Byungho An wrote:
 From: Siva Reddy siva.kal...@samsung.com
 
 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.kal...@samsung.com
 Signed-off-by: Vipul Pandya vipul.pan...@samsung.com
 Signed-off-by: Girish K S ks.g...@samsung.com
 Neatening-by: Joe Perches j...@perches.com
 Signed-off-by: Byungho An bh74...@samsung.com
 ---

[...]

 diff --git a/Documentation/devicetree/bindings/net/samsung-sxgbe.txt
 b/Documentation/devicetree/bindings/net/samsung-sxgbe.txt
 new file mode 100644
 index 000..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?

 @@ -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?

 +- samsung,burst-mapProgram the possible bursts supported by sxgbe
 +- samsung,fixed-burst  Program the DMA to use the fixed burst mode
 +- samsung,adv-addr-modeprogram the DMA to use Enhanced address mode

What are valid values?

Units/types?

Please describe what these actually do.

 +- samsung,force_thresh_dma_modeForce 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?

 +- samsung,force_sf_dma_modeForce 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.

[...]

 +/* 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.

[...]

 +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* ?

 +   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.

 +
 +   of_property_read_u32(np, samsung,phy-addr, plat-phy_addr);

Neither was this.

 +
 +   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?

[...]

 +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?

 +   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);
 +