diff mbox series

[RFC,1/3] arcnet: com20020: Add memory map of com20020

Message ID 20180505213448.8180-1-andrea.greco.gapmilano@gmail.com
State Changes Requested, archived
Headers show
Series [RFC,1/3] arcnet: com20020: Add memory map of com20020 | expand

Commit Message

Andrea Greco May 5, 2018, 9:34 p.m. UTC
From: Andrea Greco <a.greco@4sigma.it>

Add support for com20022I/com20020, memory mapped chip version.
Support bus: Intel 80xx and Motorola 68xx.
Bus size: Only 8 bit bus size is supported.
Added related device tree bindings

Signed-off-by: Andrea Greco <a.greco@4sigma.it>
---
 .../devicetree/bindings/net/smsc-com20020.txt      |  23 +++
 drivers/net/arcnet/Kconfig                         |  12 +-
 drivers/net/arcnet/Makefile                        |   1 +
 drivers/net/arcnet/arcdevice.h                     |  27 ++-
 drivers/net/arcnet/com20020-membus.c               | 191 +++++++++++++++++++++
 drivers/net/arcnet/com20020.c                      |   9 +-
 6 files changed, 253 insertions(+), 10 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/smsc-com20020.txt
 create mode 100644 drivers/net/arcnet/com20020-membus.c

Comments

Tobin C. Harding May 7, 2018, 2:55 a.m. UTC | #1
On Sat, May 05, 2018 at 11:34:45PM +0200, Andrea Greco wrote:
> From: Andrea Greco <a.greco@4sigma.it>

Hi Andrea,

Here are some (mostly stylistic) suggestions to help you get your driver merged.

> Add support for com20022I/com20020, memory mapped chip version.
> Support bus: Intel 80xx and Motorola 68xx.
> Bus size: Only 8 bit bus size is supported.
> Added related device tree bindings
> 
> Signed-off-by: Andrea Greco <a.greco@4sigma.it>
> ---
>  .../devicetree/bindings/net/smsc-com20020.txt      |  23 +++
>  drivers/net/arcnet/Kconfig                         |  12 +-
>  drivers/net/arcnet/Makefile                        |   1 +
>  drivers/net/arcnet/arcdevice.h                     |  27 ++-
>  drivers/net/arcnet/com20020-membus.c               | 191 +++++++++++++++++++++
>  drivers/net/arcnet/com20020.c                      |   9 +-
>  6 files changed, 253 insertions(+), 10 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/net/smsc-com20020.txt
>  create mode 100644 drivers/net/arcnet/com20020-membus.c
> 
> diff --git a/Documentation/devicetree/bindings/net/smsc-com20020.txt b/Documentation/devicetree/bindings/net/smsc-com20020.txt
> new file mode 100644
> index 000000000000..39c5b19c55af
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/smsc-com20020.txt
> @@ -0,0 +1,23 @@
> +SMSC com20020, com20022I
> +
> +timeout: Arcnet timeout, checkout datashet
> +clockp: Clock Prescaler, checkout datashet
> +clockm: Clock multiplier, checkout datasheet
> +
> +phy-reset-gpios: Chip reset ppin
> +phy-irq-gpios: Chip irq pin
> +
> +com20020_A@0 {
> +    compatible = "smsc,com20020";
> +
> +	timeout	= <0x3>;
> +	backplane = <0x0>;
> +
> +	clockp = <0x0>;
> +	clockm = <0x3>;
> +
> +	phy-reset-gpios = <&gpio3 21 GPIO_ACTIVE_LOW>;
> +	phy-irq-gpios = <&gpio2 10 GPIO_ACTIVE_LOW>;
> +
> +	status = "okay";
> +};
> diff --git a/drivers/net/arcnet/Kconfig b/drivers/net/arcnet/Kconfig
> index 39bd16f3f86d..d39faf45be1e 100644
> --- a/drivers/net/arcnet/Kconfig
> +++ b/drivers/net/arcnet/Kconfig
> @@ -3,7 +3,7 @@
>  #
>  
>  menuconfig ARCNET
> -	depends on NETDEVICES && (ISA || PCI || PCMCIA)
> +	depends on NETDEVICES
>  	tristate "ARCnet support"
>  	---help---
>  	  If you have a network card of this type, say Y and check out the
> @@ -129,5 +129,15 @@ config ARCNET_COM20020_CS
>  
>  	  To compile this driver as a module, choose M here: the module will be
>  	  called com20020_cs.  If unsure, say N.
> +config ARCNET_COM20020_MEMORY_BUS
> +	bool "Support for COM20020 on external memory"
> +	depends on ARCNET_COM20020 && !(ARCNET_COM20020_PCI || ARCNET_COM20020_ISA || ARCNET_COM20020_CS)
> +	help
> +	  Say Y here if on your custom board mount com20020 or friends.
> +
> +	  Com20022I support arcnet bus 10Mbitps.
> +	  This driver support only 8bit

This driver only supports 8bit bus size.

>         	    	 	       , and DMA is not supported is attached on your board at external interface bus.

This bit does not make sense, sorry.

> +	  Supported bus Intel80xx / Motorola 68xx.
> +	  This driver not work with other com20020 module: PCI or PCMCIA compiled as [M].

I'm not sure exactly what you want to say here, perhaps:

  	  This driver does not work with other com20020 modules compiled
	  as PCI or PCMCIA [M].
>  
>  endif # ARCNET
> diff --git a/drivers/net/arcnet/Makefile b/drivers/net/arcnet/Makefile
> index 53525e8ea130..19425c1e06f4 100644
> --- a/drivers/net/arcnet/Makefile
> +++ b/drivers/net/arcnet/Makefile
> @@ -14,3 +14,4 @@ obj-$(CONFIG_ARCNET_COM20020) += com20020.o
>  obj-$(CONFIG_ARCNET_COM20020_ISA) += com20020-isa.o
>  obj-$(CONFIG_ARCNET_COM20020_PCI) += com20020-pci.o
>  obj-$(CONFIG_ARCNET_COM20020_CS) += com20020_cs.o
> +obj-$(CONFIG_ARCNET_COM20020_MEMORY_BUS) += com20020-membus.o
> diff --git a/drivers/net/arcnet/arcdevice.h b/drivers/net/arcnet/arcdevice.h
> index d09b2b46ab63..16c608269cca 100644
> --- a/drivers/net/arcnet/arcdevice.h
> +++ b/drivers/net/arcnet/arcdevice.h
> @@ -201,7 +201,7 @@ struct ArcProto {
>  	void (*rx)(struct net_device *dev, int bufnum,
>  		   struct archdr *pkthdr, int length);
>  	int (*build_header)(struct sk_buff *skb, struct net_device *dev,
> -			    unsigned short ethproto, uint8_t daddr);
> +				unsigned short ethproto, uint8_t daddr);

  +			    unsigned short ethproto, uint8_t daddr);

Please use Linux coding style style, parameters continuing on separate
line are aligned with opening parenthesis.

>  	/* these functions return '1' if the skb can now be freed */
>  	int (*prepare_tx)(struct net_device *dev, struct archdr *pkt,
> @@ -326,9 +326,9 @@ struct arcnet_local {
>  		void (*recontrigger) (struct net_device * dev, int enable);
>  
>  		void (*copy_to_card)(struct net_device *dev, int bufnum,
> -				     int offset, void *buf, int count);
> +					 int offset, void *buf, int count);
>  		void (*copy_from_card)(struct net_device *dev, int bufnum,
> -				       int offset, void *buf, int count);
> +					   int offset, void *buf, int count);
>  	} hw;
>  
>  	void __iomem *mem_start;	/* pointer to ioremap'ed MMIO */
> @@ -360,7 +360,7 @@ struct net_device *alloc_arcdev(const char *name);
>  int arcnet_open(struct net_device *dev);
>  int arcnet_close(struct net_device *dev);
>  netdev_tx_t arcnet_send_packet(struct sk_buff *skb,
> -			       struct net_device *dev);
> +				   struct net_device *dev);
>  void arcnet_timeout(struct net_device *dev);
>  
>  /* I/O equivalents */
> @@ -371,7 +371,23 @@ void arcnet_timeout(struct net_device *dev);
>  #define BUS_ALIGN  1
>  #endif
>  
> -/* addr and offset allow register like names to define the actual IO  address.
> +#ifdef CONFIG_ARCNET_COM20020_MEMORY_BUS
> +#define arcnet_inb(addr, offset)					\
> +	ioread8((void __iomem *)(addr) + BUS_ALIGN * (offset))
> +
> +#define arcnet_outb(value, addr, offset)				\
> +	iowrite8(value, (void __iomem *)(addr) + BUS_ALIGN * (offset))
> +
> +#define arcnet_insb(addr, offset, buffer, count)			\
> +	ioread8_rep((void __iomem *)					\
> +	(addr) + BUS_ALIGN * (offset), buffer, count)
> +
> +#define arcnet_outsb(addr, offset, buffer, count)			\
> +	iowrite8_rep((void __iomem *)					\
> +	(addr) + BUS_ALIGN * (offset), buffer, count)
> +#else
> +/**
> + * addr and offset allow register like names to define the actual IO  address.
>   * A configuration option multiplies the offset for alignment.
>   */
>  #define arcnet_inb(addr, offset)					\
> @@ -388,6 +404,7 @@ void arcnet_timeout(struct net_device *dev);
>  	readb((addr) + (offset))
>  #define arcnet_writeb(value, addr, offset)				\
>  	writeb(value, (addr) + (offset))
> +#endif
>  
>  #endif				/* __KERNEL__ */
>  #endif				/* _LINUX_ARCDEVICE_H */
> diff --git a/drivers/net/arcnet/com20020-membus.c b/drivers/net/arcnet/com20020-membus.c
> new file mode 100644
> index 000000000000..6e4a2f3a84f7
> --- /dev/null
> +++ b/drivers/net/arcnet/com20020-membus.c
> @@ -0,0 +1,191 @@
> +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
> +/* Linux ARCnet driver for com 20020.
> + *
> + * This datasheet:
> + * http://ww1.microchip.com/downloads/en/DeviceDoc/200223vrevc.pdf
> + * http://ww1.microchip.com/downloads/en/DeviceDoc/20020.pdf
> + *
> + * This driver support:

    * This driver supports:

> + * - com20020,
> + * - com20022
> + * - com20022I-3v3
> + *
> + * This driver support only, 8bit read and write.

    * This driver supports only 8bit read and write.

> + * DMA is not supported by this driver.
> + */
> +#include <linux/module.h>
> +#include <linux/types.h>
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/errno.h>
> +#include <linux/platform_device.h>
> +#include <linux/netdevice.h>
> +#include <linux/of_address.h>
> +#include <linux/of_gpio.h>
> +#include <linux/sizes.h>
> +#include <linux/interrupt.h>
> +#include <linux/ioport.h>
> +#include <linux/random.h>
> +
> +#include <linux/delay.h>
> +#include "arcdevice.h"
> +#include "com20020.h"

White space line is not needed here, you might have meant to have it one
line down?

> +
> +#define VERSION "arcnet: COM20020 MEMORY BUS support loaded.\n"
> +
> +static int com20020_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np;
> +	struct net_device *dev;
> +	struct arcnet_local *lp;
> +	struct resource res, *iores;
> +	int ret, phy_reset, err;
> +	u32 timeout, backplane, clockp, clockm;
> +	void __iomem *ioaddr;
> +
> +	np = pdev->dev.of_node;
> +
> +	iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +
> +	if (of_address_to_resource(np, 0, &res))
> +		return -EINVAL;
> +
> +	ret = of_property_read_u32(np, "timeout", &timeout);
> +	if (ret) {
> +		dev_err(&pdev->dev, "timeout is required param");
> +		return ret;
> +	}
> +
> +	ret = of_property_read_u32(np, "backplane", &backplane);
> +	if (ret) {
> +		dev_err(&pdev->dev, "backplane is required param");
> +		return ret;
> +	}
> +
> +	ret = of_property_read_u32(np, "clockp", &clockp);
> +	if (ret) {
> +		dev_err(&pdev->dev, "clockp is required param");
> +		return ret;
> +	}
> +
> +	ret = of_property_read_u32(np, "clockm", &clockm);
> +	if (ret) {
> +		dev_err(&pdev->dev, "clockm is required param");
> +		return ret;
> +	}
> +
> +	phy_reset = of_get_named_gpio(np, "phy-reset-gpios", 0);
> +	if (phy_reset == -EPROBE_DEFER) {
> +		return phy_reset;
> +	} else if (!gpio_is_valid(phy_reset)) {
> +		dev_err(&pdev->dev, "phy-reset-gpios not valid !");
> +		return 0;
> +	}
> +
> +	err = devm_gpio_request_one(&pdev->dev, phy_reset, GPIOF_OUT_INIT_LOW,
> +				    "arcnet-phy-reset");
> +	if (err) {
> +		dev_err(&pdev->dev, "failed to get phy-reset-gpios: %d\n", err);
> +		return err;
> +	}
> +
> +	dev = alloc_arcdev(NULL);// Let autoassign name arc%d

/* C89 style comments please */

> +	dev->netdev_ops = &com20020_netdev_ops;
> +	lp = netdev_priv(dev);
> +
> +	lp->card_flags = ARC_CAN_10MBIT;/* pretend all of them can 10Mbit */
> +
> +	// Force address to 0

Unnecessary, we can see this in the code :)  Please don't comment 'what'
the code does (unless it is obfuscated or difficult to read).  You may
still like to comment 'why' the code does what it does though.

> +	// Will be set by user with `ip set dev arc0 address YOUR_NODE_ID`
> +	dev->dev_addr[0] = 0;
> +
> +	// request to system this memory region

Same as above

> +	if (!devm_request_mem_region(&pdev->dev, res.start, resource_size(&res),
> +				     lp->card_name))
> +		return -EBUSY;
> +
> +	ioaddr = devm_ioremap(&pdev->dev, iores->start, resource_size(iores));
> +	if (!ioaddr) {
> +		dev_err(&pdev->dev, "ioremap fallied\n");
> +		return -ENOMEM;
> +	}
> +
> +	// Reset time is 5 * xTalFreq, minimal xtal is 10Mhz
> +	// (5 * 1000) / 10Mhz = 500ns

perhaps a macro definition
#define MAX_XTAL_RESET_TIME ??

> +
> +	gpio_set_value_cansleep(phy_reset, 0);
> +	ndelay(500);
> +	gpio_set_value_cansleep(phy_reset, 1);
> +	ndelay(500);
> +
> +	/* Dummy access after Reset
> +	 * ARCNET controller needs
> +	 * this access to detect bustype
> +	 */

nit: Upto 72 characters wide is fine for comments

	/* Dummy access after Reset ARCNET controller needs
	 * this access to detect bustype
	 */

> +	arcnet_outb(0x00, ioaddr, COM20020_REG_W_COMMAND);
> +	arcnet_inb(ioaddr, COM20020_REG_R_DIAGSTAT);
> +
> +	dev->base_addr = (unsigned long)ioaddr;
> +	get_random_bytes(dev->dev_addr, sizeof(u8));
> +
> +	dev->irq = of_get_named_gpio(np, "phy-irq-gpios", 0);
> +	if (dev->irq == -EPROBE_DEFER) {
> +		return dev->irq;
> +	} else if (!gpio_is_valid(dev->irq)) {
> +		dev_err(&pdev->dev, "phy-irq-gpios not valid !");
> +		return 0;
> +	}
> +	dev->irq = gpio_to_irq(dev->irq);
> +
> +	lp->backplane = backplane;
> +	lp->clockp = clockp & 7;
> +	lp->clockm = clockm & 3;
> +	lp->timeout = timeout;
> +	lp->hw.owner = THIS_MODULE;
> +
> +	if (arcnet_inb(ioaddr, COM20020_REG_R_STATUS) == 0xFF) {
> +		ret = -EIO;
> +		goto err_release_mem;
> +	}
> +
> +	if (com20020_check(dev)) {
> +		ret = -EIO;
> +		goto err_release_mem;
> +	}
> +
> +	ret = com20020_found(dev, IRQF_TRIGGER_FALLING);
> +	if (ret)
> +		goto err_release_mem;
> +
> +	dev_dbg(&pdev->dev, "probe Done\n");
> +	return 0;
> +
> +err_release_mem:
> +	devm_iounmap(&pdev->dev, (void __iomem *)ioaddr);
> +	devm_release_mem_region(&pdev->dev, res.start, resource_size(&res));
> +	dev_err(&pdev->dev, "probe failed!\n");
> +	return ret;
> +}
> +
> +static const struct of_device_id of_com20020_match[] = {
> +	{ .compatible = "smsc,com20020",	},
> +	{ },
> +};
> +
> +MODULE_DEVICE_TABLE(of, of_com20020_match);
> +
> +static struct platform_driver of_com20020_driver = {
> +	.driver			= {
> +		.name		= "com20020-memory-bus",
> +		.of_match_table = of_com20020_match,
> +	},
> +	.probe			= com20020_probe,
> +};
> +
> +static int com20020_init(void)
> +{
> +	return platform_driver_register(&of_com20020_driver);
> +}
> +late_initcall(com20020_init);
> +
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/net/arcnet/com20020.c b/drivers/net/arcnet/com20020.c
> index 78043a9c5981..f09ea77dd6a8 100644
> --- a/drivers/net/arcnet/com20020.c
> +++ b/drivers/net/arcnet/com20020.c
> @@ -43,7 +43,7 @@
>  #include "com20020.h"
>  
>  static const char * const clockrates[] = {
> -	"XXXXXXX", "XXXXXXXX", "XXXXXX", "2.5 Mb/s",
> +	"10 Mb/s", "XXXXXXXX", "XXXXXX", "2.5 Mb/s",
>  	"1.25Mb/s", "625 Kb/s", "312.5 Kb/s", "156.25 Kb/s",
>  	"Reserved", "Reserved", "Reserved"
>  };
> @@ -391,9 +391,10 @@ static void com20020_set_mc_list(struct net_device *dev)
>  	}
>  }
>  
> -#if defined(CONFIG_ARCNET_COM20020_PCI_MODULE) || \
> -    defined(CONFIG_ARCNET_COM20020_ISA_MODULE) || \
> -    defined(CONFIG_ARCNET_COM20020_CS_MODULE)
> +#if	defined(CONFIG_ARCNET_COM20020_PCI_MODULE) || \
> +	defined(CONFIG_ARCNET_COM20020_ISA_MODULE) || \
> +	defined(CONFIG_ARCNET_COM20020_CS_MODULE)  || \
> +	defined(CONFIG_ARCNET_COM20020_MEMORY_BUS)

Why the whitespace change?

Hope this helps,
Tobin.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andrea Greco May 8, 2018, 9:36 a.m. UTC | #2
On 05/07/2018 04:55 AM, Tobin C. Harding wrote:
> On Sat, May 05, 2018 at 11:34:45PM +0200, Andrea Greco wrote:
>> From: Andrea Greco <a.greco@4sigma.it>
> 
> Hi Andrea,
> 
> Here are some (mostly stylistic) suggestions to help you get your driver merged.
> 
>> Add support for com20022I/com20020, memory mapped chip version.
>> Support bus: Intel 80xx and Motorola 68xx.
>> Bus size: Only 8 bit bus size is supported.
>> Added related device tree bindings
>>
>> Signed-off-by: Andrea Greco <a.greco@4sigma.it>
>> ---
>>   .../devicetree/bindings/net/smsc-com20020.txt      |  23 +++
>>   drivers/net/arcnet/Kconfig                         |  12 +-
>>   drivers/net/arcnet/Makefile                        |   1 +
>>   drivers/net/arcnet/arcdevice.h                     |  27 ++-
>>   drivers/net/arcnet/com20020-membus.c               | 191 +++++++++++++++++++++
>>   drivers/net/arcnet/com20020.c                      |   9 +-
>>   6 files changed, 253 insertions(+), 10 deletions(-)
>>   create mode 100644 Documentation/devicetree/bindings/net/smsc-com20020.txt
>>   create mode 100644 drivers/net/arcnet/com20020-membus.c
>>
>> diff --git a/Documentation/devicetree/bindings/net/smsc-com20020.txt b/Documentation/devicetree/bindings/net/smsc-com20020.txt
>> new file mode 100644
>> index 000000000000..39c5b19c55af
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/net/smsc-com20020.txt
>> @@ -0,0 +1,23 @@
>> +SMSC com20020, com20022I
>> +
>> +timeout: Arcnet timeout, checkout datashet
>> +clockp: Clock Prescaler, checkout datashet
>> +clockm: Clock multiplier, checkout datasheet
>> +
>> +phy-reset-gpios: Chip reset ppin
>> +phy-irq-gpios: Chip irq pin

ppin Corrected by my-self.

>> +
>> +com20020_A@0 {
>> +    compatible = "smsc,com20020";
>> +
>> +	timeout	= <0x3>;
>> +	backplane = <0x0>;
>> +
>> +	clockp = <0x0>;
>> +	clockm = <0x3>;
>> +
>> +	phy-reset-gpios = <&gpio3 21 GPIO_ACTIVE_LOW>;
>> +	phy-irq-gpios = <&gpio2 10 GPIO_ACTIVE_LOW>;
>> +
>> +	status = "okay";
>> +};
>> diff --git a/drivers/net/arcnet/Kconfig b/drivers/net/arcnet/Kconfig
>> index 39bd16f3f86d..d39faf45be1e 100644
>> --- a/drivers/net/arcnet/Kconfig
>> +++ b/drivers/net/arcnet/Kconfig
>> @@ -3,7 +3,7 @@
>>   #
>>   
>>   menuconfig ARCNET
>> -	depends on NETDEVICES && (ISA || PCI || PCMCIA)
>> +	depends on NETDEVICES
>>   	tristate "ARCnet support"
>>   	---help---
>>   	  If you have a network card of this type, say Y and check out the
>> @@ -129,5 +129,15 @@ config ARCNET_COM20020_CS
>>   
>>   	  To compile this driver as a module, choose M here: the module will be
>>   	  called com20020_cs.  If unsure, say N.
>> +config ARCNET_COM20020_MEMORY_BUS
>> +	bool "Support for COM20020 on external memory"
>> +	depends on ARCNET_COM20020 && !(ARCNET_COM20020_PCI || ARCNET_COM20020_ISA || ARCNET_COM20020_CS)
>> +	help
>> +	  Say Y here if on your custom board mount com20020 or friends.
>> +
>> +	  Com20022I support arcnet bus 10Mbitps.
>> +	  This driver support only 8bit
> 
> This driver only supports 8bit bus size.
> 
>>          	    	 	       , and DMA is not supported is attached on your board at external interface bus.
> 
> This bit does not make sense, sorry.
Removed description,

Proposal for v2:
Say Y here if your custom board mount com20020 chipset or friends.
Supported Chipset: com20020, com20022, com20022I-3v3.
If unsure, say N.

>> +	  Supported bus Intel80xx / Motorola 68xx.
>> +	  This driver not work with other com20020 module: PCI or PCMCIA compiled as [M].
> 
> I'm not sure exactly what you want to say here, perhaps:
> 
>    	  This driver does not work with other com20020 modules compiled
> 	  as PCI or PCMCIA [M].

About this, all removed from kconfig
For PCI, PCMCIA, checkout downside

>>   
>>   endif # ARCNET
>> diff --git a/drivers/net/arcnet/Makefile b/drivers/net/arcnet/Makefile
>> index 53525e8ea130..19425c1e06f4 100644
>> --- a/drivers/net/arcnet/Makefile
>> +++ b/drivers/net/arcnet/Makefile
>> @@ -14,3 +14,4 @@ obj-$(CONFIG_ARCNET_COM20020) += com20020.o
>>   obj-$(CONFIG_ARCNET_COM20020_ISA) += com20020-isa.o
>>   obj-$(CONFIG_ARCNET_COM20020_PCI) += com20020-pci.o
>>   obj-$(CONFIG_ARCNET_COM20020_CS) += com20020_cs.o
>> +obj-$(CONFIG_ARCNET_COM20020_MEMORY_BUS) += com20020-membus.o
>> diff --git a/drivers/net/arcnet/arcdevice.h b/drivers/net/arcnet/arcdevice.h
>> index d09b2b46ab63..16c608269cca 100644
>> --- a/drivers/net/arcnet/arcdevice.h
>> +++ b/drivers/net/arcnet/arcdevice.h
>> @@ -201,7 +201,7 @@ struct ArcProto {
>>   	void (*rx)(struct net_device *dev, int bufnum,
>>   		   struct archdr *pkthdr, int length);
>>   	int (*build_header)(struct sk_buff *skb, struct net_device *dev,
>> -			    unsigned short ethproto, uint8_t daddr);
>> +				unsigned short ethproto, uint8_t daddr);
> 
>    +			    unsigned short ethproto, uint8_t daddr);
> 
> Please use Linux coding style style, parameters continuing on separate
> line are aligned with opening parenthesis.
> 
>>   	/* these functions return '1' if the skb can now be freed */
>>   	int (*prepare_tx)(struct net_device *dev, struct archdr *pkt,
>> @@ -326,9 +326,9 @@ struct arcnet_local {
>>   		void (*recontrigger) (struct net_device * dev, int enable);
>>   
>>   		void (*copy_to_card)(struct net_device *dev, int bufnum,
>> -				     int offset, void *buf, int count);
>> +					 int offset, void *buf, int count);
>>   		void (*copy_from_card)(struct net_device *dev, int bufnum,
>> -				       int offset, void *buf, int count);
>> +					   int offset, void *buf, int count);
>>   	} hw;
>>   
>>   	void __iomem *mem_start;	/* pointer to ioremap'ed MMIO */
>> @@ -360,7 +360,7 @@ struct net_device *alloc_arcdev(const char *name);
>>   int arcnet_open(struct net_device *dev);
>>   int arcnet_close(struct net_device *dev);
>>   netdev_tx_t arcnet_send_packet(struct sk_buff *skb,
>> -			       struct net_device *dev);
>> +				   struct net_device *dev);
>>   void arcnet_timeout(struct net_device *dev);

Not required modification then all removed.

>>   
>>   /* I/O equivalents */
>> @@ -371,7 +371,23 @@ void arcnet_timeout(struct net_device *dev);
>>   #define BUS_ALIGN  1
>>   #endif
>>   
>> -/* addr and offset allow register like names to define the actual IO  address.
>> +#ifdef CONFIG_ARCNET_COM20020_MEMORY_BUS
>> +#define arcnet_inb(addr, offset)					\
>> +	ioread8((void __iomem *)(addr) + BUS_ALIGN * (offset))
>> +
>> +#define arcnet_outb(value, addr, offset)				\
>> +	iowrite8(value, (void __iomem *)(addr) + BUS_ALIGN * (offset))
>> +
>> +#define arcnet_insb(addr, offset, buffer, count)			\
>> +	ioread8_rep((void __iomem *)					\
>> +	(addr) + BUS_ALIGN * (offset), buffer, count)
>> +
>> +#define arcnet_outsb(addr, offset, buffer, count)			\
>> +	iowrite8_rep((void __iomem *)					\
>> +	(addr) + BUS_ALIGN * (offset), buffer, count)
>> +#else
>> +/**
>> + * addr and offset allow register like names to define the actual IO  address.
>>    * A configuration option multiplies the offset for alignment.
>>    */
>>   #define arcnet_inb(addr, offset)					\
>> @@ -388,6 +404,7 @@ void arcnet_timeout(struct net_device *dev);
>>   	readb((addr) + (offset))
>>   #define arcnet_writeb(value, addr, offset)				\
>>   	writeb(value, (addr) + (offset))
>> +#endif
>>   
>>   #endif				/* __KERNEL__ */
>>   #endif				/* _LINUX_ARCDEVICE_H */


This is most important part where a suggestion will be very appreciated.
This part define how arcnet drivers read and write over physical.
The old driver can always use readb/writeb and friends, this driver rise 
big kernel panic.

This driver make a ioreamp with: devm_ioremap.
Then, for r/w operation i use ioread8/iowrite8 and friends.

My quickly solution was make a ugly #ifdef.

With #ifndef all other driver implementation could not be used together
this driver, because break, how driver write over physical.
A proposal could be add a read/write callback to arcdevice.h struct hw.

PROS:
Every driver fill this callback, this is a solution.

CONS:
This solution require a small change for all com20020 driver
implementations. I don't dispose of all hardware for make a accurate
test. I could only test memory bus version.

Any other ideas, will be very appreciated.

>> diff --git a/drivers/net/arcnet/com20020-membus.c b/drivers/net/arcnet/com20020-membus.c
>> new file mode 100644
>> index 000000000000..6e4a2f3a84f7
>> --- /dev/null
>> +++ b/drivers/net/arcnet/com20020-membus.c
>> @@ -0,0 +1,191 @@
>> +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
>> +/* Linux ARCnet driver for com 20020.
>> + *
>> + * This datasheet:
>> + * http://ww1.microchip.com/downloads/en/DeviceDoc/200223vrevc.pdf
>> + * http://ww1.microchip.com/downloads/en/DeviceDoc/20020.pdf
>> + *
>> + * This driver support:
> 
>      * This driver supports:
> 
>> + * - com20020,
>> + * - com20022
>> + * - com20022I-3v3
>> + *
>> + * This driver support only, 8bit read and write.
> 
>      * This driver supports only 8bit read and write.
> 
>> + * DMA is not supported by this driver.
>> + */
>> +#include <linux/module.h>
>> +#include <linux/types.h>
>> +#include <linux/device.h>
>> +#include <linux/kernel.h>
>> +#include <linux/errno.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/netdevice.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_gpio.h>
>> +#include <linux/sizes.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/ioport.h>
>> +#include <linux/random.h>
>> +
>> +#include <linux/delay.h>
>> +#include "arcdevice.h"
>> +#include "com20020.h"
> 
> White space line is not needed here, you might have meant to have it one
> line down?
>
Removed

>> +
>> +#define VERSION "arcnet: COM20020 MEMORY BUS support loaded.\n"
>> +
>> +static int com20020_probe(struct platform_device *pdev)
>> +{
>> +	struct device_node *np;
>> +	struct net_device *dev;
>> +	struct arcnet_local *lp;
>> +	struct resource res, *iores;
>> +	int ret, phy_reset, err;
>> +	u32 timeout, backplane, clockp, clockm;
>> +	void __iomem *ioaddr;
>> +
>> +	np = pdev->dev.of_node;
>> +
>> +	iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +
>> +	if (of_address_to_resource(np, 0, &res))
>> +		return -EINVAL;
>> +
>> +	ret = of_property_read_u32(np, "timeout", &timeout);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "timeout is required param");
>> +		return ret;
>> +	}
>> +
>> +	ret = of_property_read_u32(np, "backplane", &backplane);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "backplane is required param");
>> +		return ret;
>> +	}
>> +
>> +	ret = of_property_read_u32(np, "clockp", &clockp);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "clockp is required param");
>> +		return ret;
>> +	}
>> +
>> +	ret = of_property_read_u32(np, "clockm", &clockm);
>> +	if (ret) {
>> +		dev_err(&pdev->dev, "clockm is required param");
>> +		return ret;
>> +	}
>> +
>> +	phy_reset = of_get_named_gpio(np, "phy-reset-gpios", 0);
>> +	if (phy_reset == -EPROBE_DEFER) {
>> +		return phy_reset;
>> +	} else if (!gpio_is_valid(phy_reset)) {
>> +		dev_err(&pdev->dev, "phy-reset-gpios not valid !");
>> +		return 0;
>> +	}
>> +
>> +	err = devm_gpio_request_one(&pdev->dev, phy_reset, GPIOF_OUT_INIT_LOW,
>> +				    "arcnet-phy-reset");
>> +	if (err) {
>> +		dev_err(&pdev->dev, "failed to get phy-reset-gpios: %d\n", err);
>> +		return err;
>> +	}
>> +
>> +	dev = alloc_arcdev(NULL);// Let autoassign name arc%d
> 
> /* C89 style comments please */

All comment bring to C89

> 
>> +	dev->netdev_ops = &com20020_netdev_ops;
>> +	lp = netdev_priv(dev);
>> +
>> +	lp->card_flags = ARC_CAN_10MBIT;/* pretend all of them can 10Mbit */
>> +
>> +	// Force address to 0
> 
Removed
> Unnecessary, we can see this in the code :)  Please don't comment 'what'
> the code does (unless it is obfuscated or difficult to read).  You may
> still like to comment 'why' the code does what it does though.
> 
>> +	// Will be set by user with `ip set dev arc0 address YOUR_NODE_ID`
>> +	dev->dev_addr[0] = 0;
>> +

All this become
/* Will be set by userspace during if setup */

>> +	// request to system this memory region
> 
> Same as above
> 
Removed

>> +	if (!devm_request_mem_region(&pdev->dev, res.start, resource_size(&res),
>> +				     lp->card_name))
>> +		return -EBUSY;
>> +
>> +	ioaddr = devm_ioremap(&pdev->dev, iores->start, resource_size(iores));
>> +	if (!ioaddr) {
>> +		dev_err(&pdev->dev, "ioremap fallied\n");
>> +		return -ENOMEM;
>> +	}
>> +
>> +	// Reset time is 5 * xTalFreq, minimal xtal is 10Mhz
>> +	// (5 * 1000) / 10Mhz = 500ns
> 
> perhaps a macro definition
> #define MAX_XTAL_RESET_TIME ??

Macro made, on head of file.
Rereading Datasheet maby that last delay could be removed.
Then become
	gpio_set_value_cansleep(phy_reset, 0);
	ndelay(RESET_DELAY);
	gpio_set_value_cansleep(phy_reset, 1);
>> +
>> +	gpio_set_value_cansleep(phy_reset, 0);
>> +	ndelay(500);
>> +	gpio_set_value_cansleep(phy_reset, 1);
>> +	ndelay(500);
>> +
>> +	/* Dummy access after Reset
>> +	 * ARCNET controller needs
>> +	 * this access to detect bustype
>> +	 */
> 
> nit: Upto 72 characters wide is fine for comments
> 
> 	/* Dummy access after Reset ARCNET controller needs
> 	 * this access to detect bustype
> 	 */
> 
>> +	arcnet_outb(0x00, ioaddr, COM20020_REG_W_COMMAND);
>> +	arcnet_inb(ioaddr, COM20020_REG_R_DIAGSTAT);
>> +

Changed in:
	/* ARCNET controller needs this access to detect bustype */
	arcnet_outb(0x00, ioaddr, COM20020_REG_W_COMMAND);
	arcnet_inb(ioaddr, COM20020_REG_R_DIAGSTAT);

>> +	dev->base_addr = (unsigned long)ioaddr;
>> +	get_random_bytes(dev->dev_addr, sizeof(u8));
>> +
>> +	dev->irq = of_get_named_gpio(np, "phy-irq-gpios", 0);
>> +	if (dev->irq == -EPROBE_DEFER) {
>> +		return dev->irq;
>> +	} else if (!gpio_is_valid(dev->irq)) {
>> +		dev_err(&pdev->dev, "phy-irq-gpios not valid !");
>> +		return 0;
>> +	}
>> +	dev->irq = gpio_to_irq(dev->irq);
>> +
>> +	lp->backplane = backplane;
>> +	lp->clockp = clockp & 7;
>> +	lp->clockm = clockm & 3;
>> +	lp->timeout = timeout;
>> +	lp->hw.owner = THIS_MODULE;
>> +
>> +	if (arcnet_inb(ioaddr, COM20020_REG_R_STATUS) == 0xFF) {
>> +		ret = -EIO;
>> +		goto err_release_mem;
>> +	}
>> +
>> +	if (com20020_check(dev)) {
>> +		ret = -EIO;
>> +		goto err_release_mem;
>> +	}
>> +
>> +	ret = com20020_found(dev, IRQF_TRIGGER_FALLING);
>> +	if (ret)
>> +		goto err_release_mem;
>> +
>> +	dev_dbg(&pdev->dev, "probe Done\n");
>> +	return 0;
>> +
>> +err_release_mem:
>> +	devm_iounmap(&pdev->dev, (void __iomem *)ioaddr);
>> +	devm_release_mem_region(&pdev->dev, res.start, resource_size(&res));
>> +	dev_err(&pdev->dev, "probe failed!\n");
>> +	return ret;
>> +}
>> +
>> +static const struct of_device_id of_com20020_match[] = {
>> +	{ .compatible = "smsc,com20020",	},
>> +	{ },
>> +};
>> +
>> +MODULE_DEVICE_TABLE(of, of_com20020_match);
>> +
>> +static struct platform_driver of_com20020_driver = {
>> +	.driver			= {
>> +		.name		= "com20020-memory-bus",
>> +		.of_match_table = of_com20020_match,
>> +	},
>> +	.probe			= com20020_probe,
>> +};
>> +
>> +static int com20020_init(void)
>> +{
>> +	return platform_driver_register(&of_com20020_driver);
>> +}
>> +late_initcall(com20020_init);
>> +
>> +MODULE_LICENSE("GPL");
>> diff --git a/drivers/net/arcnet/com20020.c b/drivers/net/arcnet/com20020.c
>> index 78043a9c5981..f09ea77dd6a8 100644
>> --- a/drivers/net/arcnet/com20020.c
>> +++ b/drivers/net/arcnet/com20020.c
>> @@ -43,7 +43,7 @@
>>   #include "com20020.h"
>>   
>>   static const char * const clockrates[] = {
>> -	"XXXXXXX", "XXXXXXXX", "XXXXXX", "2.5 Mb/s",
>> +	"10 Mb/s", "XXXXXXXX", "XXXXXX", "2.5 Mb/s",
>>   	"1.25Mb/s", "625 Kb/s", "312.5 Kb/s", "156.25 Kb/s",
>>   	"Reserved", "Reserved", "Reserved"
>>   };
>> @@ -391,9 +391,10 @@ static void com20020_set_mc_list(struct net_device *dev)
>>   	}
>>   }
>>   
>> -#if defined(CONFIG_ARCNET_COM20020_PCI_MODULE) || \
>> -    defined(CONFIG_ARCNET_COM20020_ISA_MODULE) || \
>> -    defined(CONFIG_ARCNET_COM20020_CS_MODULE)
>> +#if	defined(CONFIG_ARCNET_COM20020_PCI_MODULE) || \
>> +	defined(CONFIG_ARCNET_COM20020_ISA_MODULE) || \
>> +	defined(CONFIG_ARCNET_COM20020_CS_MODULE)  || \
>> +	defined(CONFIG_ARCNET_COM20020_MEMORY_BUS)
> 
> Why the whitespace change?

      defined(CONFIG_ARCNET_COM20020_ISA_MODULE) || \
-    defined(CONFIG_ARCNET_COM20020_CS_MODULE)
+    defined(CONFIG_ARCNET_COM20020_CS_MODULE)  || \
+    defined(CONFIG_ARCNET_COM20020_MEMORY_BUS)

Removed useless withespace

> 
> Hope this helps,
> Tobin.
> 

Thank you for help,
Andrea
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rob Herring (Arm) May 8, 2018, 4:16 p.m. UTC | #3
On Sat, May 05, 2018 at 11:34:45PM +0200, Andrea Greco wrote:
> From: Andrea Greco <a.greco@4sigma.it>
> 
> Add support for com20022I/com20020, memory mapped chip version.
> Support bus: Intel 80xx and Motorola 68xx.
> Bus size: Only 8 bit bus size is supported.
> Added related device tree bindings
> 
> Signed-off-by: Andrea Greco <a.greco@4sigma.it>
> ---
>  .../devicetree/bindings/net/smsc-com20020.txt      |  23 +++

Please split bindings to separate patch.

>  drivers/net/arcnet/Kconfig                         |  12 +-
>  drivers/net/arcnet/Makefile                        |   1 +
>  drivers/net/arcnet/arcdevice.h                     |  27 ++-
>  drivers/net/arcnet/com20020-membus.c               | 191 +++++++++++++++++++++
>  drivers/net/arcnet/com20020.c                      |   9 +-
>  6 files changed, 253 insertions(+), 10 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/net/smsc-com20020.txt
>  create mode 100644 drivers/net/arcnet/com20020-membus.c
> 
> diff --git a/Documentation/devicetree/bindings/net/smsc-com20020.txt b/Documentation/devicetree/bindings/net/smsc-com20020.txt
> new file mode 100644
> index 000000000000..39c5b19c55af
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/smsc-com20020.txt
> @@ -0,0 +1,23 @@
> +SMSC com20020, com20022I

What does this device do?

> +
> +timeout: Arcnet timeout, checkout datashet
> +clockp: Clock Prescaler, checkout datashet

s/datashet/datasheet/

> +clockm: Clock multiplier, checkout datasheet

Would these 3 properties be common for arcnet devices? If not, then they 
should have a vendor prefix.

> +
> +phy-reset-gpios: Chip reset ppin

Use 'reset-gpios' as that is standard.

> +phy-irq-gpios: Chip irq pin

Use 'interrupts'. Interrupt capable gpio controllers are also interrupt 
controllers.

> +
> +com20020_A@0 {

Node names should be generic based on the class of device. I don't think 
we have one defined, but how about 'arcnet'.

Unit addresses must have a corresponding reg property. How is this 
device accessed?

> +    compatible = "smsc,com20020";

Not documented.

> +
> +	timeout	= <0x3>;
> +	backplane = <0x0>;
> +
> +	clockp = <0x0>;
> +	clockm = <0x3>;
> +
> +	phy-reset-gpios = <&gpio3 21 GPIO_ACTIVE_LOW>;
> +	phy-irq-gpios = <&gpio2 10 GPIO_ACTIVE_LOW>;
> +
> +	status = "okay";

Don't should status in examples.

> +};
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andrea Greco May 11, 2018, 10:50 a.m. UTC | #4
On 05/08/2018 06:16 PM, Rob Herring wrote:
> On Sat, May 05, 2018 at 11:34:45PM +0200, Andrea Greco wrote:
>> From: Andrea Greco <a.greco@4sigma.it>
>>
>> Add support for com20022I/com20020, memory mapped chip version.
>> Support bus: Intel 80xx and Motorola 68xx.
>> Bus size: Only 8 bit bus size is supported.
>> Added related device tree bindings
>>
>> Signed-off-by: Andrea Greco <a.greco@4sigma.it>
>> ---
>>   .../devicetree/bindings/net/smsc-com20020.txt      |  23 +++
>
> Please split bindings to separate patch.

Ok
>
>>   drivers/net/arcnet/Kconfig                         |  12 +-
>>   drivers/net/arcnet/Makefile                        |   1 +
>>   drivers/net/arcnet/arcdevice.h                     |  27 ++-
>>   drivers/net/arcnet/com20020-membus.c               | 191 +++++++++++++++++++++
>>   drivers/net/arcnet/com20020.c                      |   9 +-
>>   6 files changed, 253 insertions(+), 10 deletions(-)
>>   create mode 100644 Documentation/devicetree/bindings/net/smsc-com20020.txt
>>   create mode 100644 drivers/net/arcnet/com20020-membus.c
>>
>> diff --git a/Documentation/devicetree/bindings/net/smsc-com20020.txt b/Documentation/devicetree/bindings/net/smsc-com20020.txt
>> new file mode 100644
>> index 000000000000..39c5b19c55af
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/net/smsc-com20020.txt
>> @@ -0,0 +1,23 @@
>> +SMSC com20020, com20022I
>
> What does this device do?
>

Changed in:
SMSC com20020 Arcnet network controller

>> +
>> +timeout: Arcnet timeout, checkout datashet
>> +clockp: Clock Prescaler, checkout datashet
>
> s/datashet/datasheet/
>
>> +clockm: Clock multiplier, checkout datasheet
>
> Would these 3 properties be common for arcnet devices? If not, then they
> should have a vendor prefix.
>

Timeout is arcnet propelty:
Other is smsc params, then become:
- timeout: Arcnet timeout
- smsc-clockp: Clock Prescaler
- smsc-clockm: Clock multiplier
- smsc-backplane: Controller use backplane mode inside of transceiver

I forget backplane propelty, but is required

>> +
>> +phy-reset-gpios: Chip reset ppin
>
> Use 'reset-gpios' as that is standard.
>
>> +phy-irq-gpios: Chip irq pin
>
> Use 'interrupts'. Interrupt capable gpio controllers are also interrupt
> controllers.
>

Ok, change to standard

>> +
>> +com20020_A@0 {
>
> Node names should be generic based on the class of device. I don't think
> we have one defined, but how about 'arcnet'.
>
> Unit addresses must have a corresponding reg property. How is this
> device accessed?
>

Then: arcnet@28000000

>> +    compatible = "smsc,com20020";
>
> Not documented.
>
I miss something? Where add this doc?
Is not this file?
Documentation/devicetree/bindings/net/smsc-com20020.txt

>> +
>> + timeout = <0x3>;
>> + backplane = <0x0>;
>> +
>> + clockp = <0x0>;
>> + clockm = <0x3>;
>> +
>> + phy-reset-gpios = <&gpio3 21 GPIO_ACTIVE_LOW>;
>> + phy-irq-gpios = <&gpio2 10 GPIO_ACTIVE_LOW>;
>> +
>> + status = "okay";
>
> Don't should status in examples.
>
>> +};
Ok

Final result of new Patch, for bindings:

SMSC com20020 Arcnet network controller

Required propelty:
- timeout: Arcnet timeout
- smsc-clockp: Clock Prescaler
- smsc-clockm: Clock multiplier
- smsc-backplane: Controller use backplane mode inside of transceiver

- reset-gpios: Chip reset pin
- interrupts: Should contain controller interrupt

arcnet@28000000 {
     compatible = "smsc,com20020";

timeout = <0x3>;
smsc-backplane = <0x0>;
smsc-clockp = <0x0>;
smsc-clockm = <0x3>;

reset-gpios = <&gpio3 21 GPIO_ACTIVE_LOW>;
interrupts = <&gpio2 10 GPIO_ACTIVE_LOW>;
};

Andrea
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rob Herring (Arm) May 11, 2018, 1:35 p.m. UTC | #5
On Fri, May 11, 2018 at 5:50 AM, Andrea Greco
<andrea.greco.gapmilano@gmail.com> wrote:
> On 05/08/2018 06:16 PM, Rob Herring wrote:
>> On Sat, May 05, 2018 at 11:34:45PM +0200, Andrea Greco wrote:
>>> From: Andrea Greco <a.greco@4sigma.it>
>>>
>>> Add support for com20022I/com20020, memory mapped chip version.
>>> Support bus: Intel 80xx and Motorola 68xx.
>>> Bus size: Only 8 bit bus size is supported.
>>> Added related device tree bindings
>>>
>>> Signed-off-by: Andrea Greco <a.greco@4sigma.it>
>>> ---
>>>   .../devicetree/bindings/net/smsc-com20020.txt      |  23 +++
>>
>> Please split bindings to separate patch.
>
> Ok
>>
>>>   drivers/net/arcnet/Kconfig                         |  12 +-
>>>   drivers/net/arcnet/Makefile                        |   1 +
>>>   drivers/net/arcnet/arcdevice.h                     |  27 ++-
>>>   drivers/net/arcnet/com20020-membus.c               | 191 +++++++++++++++++++++
>>>   drivers/net/arcnet/com20020.c                      |   9 +-
>>>   6 files changed, 253 insertions(+), 10 deletions(-)
>>>   create mode 100644 Documentation/devicetree/bindings/net/smsc-com20020.txt
>>>   create mode 100644 drivers/net/arcnet/com20020-membus.c
>>>
>>> diff --git a/Documentation/devicetree/bindings/net/smsc-com20020.txt b/Documentation/devicetree/bindings/net/smsc-com20020.txt
>>> new file mode 100644
>>> index 000000000000..39c5b19c55af
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/net/smsc-com20020.txt
>>> @@ -0,0 +1,23 @@
>>> +SMSC com20020, com20022I
>>
>> What does this device do?
>>
>
> Changed in:
> SMSC com20020 Arcnet network controller
>
>>> +
>>> +timeout: Arcnet timeout, checkout datashet
>>> +clockp: Clock Prescaler, checkout datashet
>>
>> s/datashet/datasheet/
>>
>>> +clockm: Clock multiplier, checkout datasheet
>>
>> Would these 3 properties be common for arcnet devices? If not, then they
>> should have a vendor prefix.
>>
>
> Timeout is arcnet propelty:
> Other is smsc params, then become:
> - timeout: Arcnet timeout

Needs unit suffix as defined in property-units.txt.

> - smsc-clockp: Clock Prescaler
> - smsc-clockm: Clock multiplier
> - smsc-backplane: Controller use backplane mode inside of transceiver

Vendor properties are <vendor>,<prop-name>.

>
> I forget backplane propelty, but is required
>
>>> +
>>> +phy-reset-gpios: Chip reset ppin
>>
>> Use 'reset-gpios' as that is standard.
>>
>>> +phy-irq-gpios: Chip irq pin
>>
>> Use 'interrupts'. Interrupt capable gpio controllers are also interrupt
>> controllers.
>>
>
> Ok, change to standard
>
>>> +
>>> +com20020_A@0 {
>>
>> Node names should be generic based on the class of device. I don't think
>> we have one defined, but how about 'arcnet'.
>>
>> Unit addresses must have a corresponding reg property. How is this
>> device accessed?
>>
>
> Then: arcnet@28000000
>
>>> +    compatible = "smsc,com20020";
>>
>> Not documented.
>>
> I miss something? Where add this doc?
> Is not this file?

Yes, this file up above with all the other properties. The example is
just an example, not a binding definition.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/net/smsc-com20020.txt b/Documentation/devicetree/bindings/net/smsc-com20020.txt
new file mode 100644
index 000000000000..39c5b19c55af
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/smsc-com20020.txt
@@ -0,0 +1,23 @@ 
+SMSC com20020, com20022I
+
+timeout: Arcnet timeout, checkout datashet
+clockp: Clock Prescaler, checkout datashet
+clockm: Clock multiplier, checkout datasheet
+
+phy-reset-gpios: Chip reset ppin
+phy-irq-gpios: Chip irq pin
+
+com20020_A@0 {
+    compatible = "smsc,com20020";
+
+	timeout	= <0x3>;
+	backplane = <0x0>;
+
+	clockp = <0x0>;
+	clockm = <0x3>;
+
+	phy-reset-gpios = <&gpio3 21 GPIO_ACTIVE_LOW>;
+	phy-irq-gpios = <&gpio2 10 GPIO_ACTIVE_LOW>;
+
+	status = "okay";
+};
diff --git a/drivers/net/arcnet/Kconfig b/drivers/net/arcnet/Kconfig
index 39bd16f3f86d..d39faf45be1e 100644
--- a/drivers/net/arcnet/Kconfig
+++ b/drivers/net/arcnet/Kconfig
@@ -3,7 +3,7 @@ 
 #
 
 menuconfig ARCNET
-	depends on NETDEVICES && (ISA || PCI || PCMCIA)
+	depends on NETDEVICES
 	tristate "ARCnet support"
 	---help---
 	  If you have a network card of this type, say Y and check out the
@@ -129,5 +129,15 @@  config ARCNET_COM20020_CS
 
 	  To compile this driver as a module, choose M here: the module will be
 	  called com20020_cs.  If unsure, say N.
+config ARCNET_COM20020_MEMORY_BUS
+	bool "Support for COM20020 on external memory"
+	depends on ARCNET_COM20020 && !(ARCNET_COM20020_PCI || ARCNET_COM20020_ISA || ARCNET_COM20020_CS)
+	help
+	  Say Y here if on your custom board mount com20020 or friends.
+
+	  Com20022I support arcnet bus 10Mbitps.
+	  This driver support only 8bit, and DMA is not supported is attached on your board at external interface bus.
+	  Supported bus Intel80xx / Motorola 68xx.
+	  This driver not work with other com20020 module: PCI or PCMCIA compiled as [M].
 
 endif # ARCNET
diff --git a/drivers/net/arcnet/Makefile b/drivers/net/arcnet/Makefile
index 53525e8ea130..19425c1e06f4 100644
--- a/drivers/net/arcnet/Makefile
+++ b/drivers/net/arcnet/Makefile
@@ -14,3 +14,4 @@  obj-$(CONFIG_ARCNET_COM20020) += com20020.o
 obj-$(CONFIG_ARCNET_COM20020_ISA) += com20020-isa.o
 obj-$(CONFIG_ARCNET_COM20020_PCI) += com20020-pci.o
 obj-$(CONFIG_ARCNET_COM20020_CS) += com20020_cs.o
+obj-$(CONFIG_ARCNET_COM20020_MEMORY_BUS) += com20020-membus.o
diff --git a/drivers/net/arcnet/arcdevice.h b/drivers/net/arcnet/arcdevice.h
index d09b2b46ab63..16c608269cca 100644
--- a/drivers/net/arcnet/arcdevice.h
+++ b/drivers/net/arcnet/arcdevice.h
@@ -201,7 +201,7 @@  struct ArcProto {
 	void (*rx)(struct net_device *dev, int bufnum,
 		   struct archdr *pkthdr, int length);
 	int (*build_header)(struct sk_buff *skb, struct net_device *dev,
-			    unsigned short ethproto, uint8_t daddr);
+				unsigned short ethproto, uint8_t daddr);
 
 	/* these functions return '1' if the skb can now be freed */
 	int (*prepare_tx)(struct net_device *dev, struct archdr *pkt,
@@ -326,9 +326,9 @@  struct arcnet_local {
 		void (*recontrigger) (struct net_device * dev, int enable);
 
 		void (*copy_to_card)(struct net_device *dev, int bufnum,
-				     int offset, void *buf, int count);
+					 int offset, void *buf, int count);
 		void (*copy_from_card)(struct net_device *dev, int bufnum,
-				       int offset, void *buf, int count);
+					   int offset, void *buf, int count);
 	} hw;
 
 	void __iomem *mem_start;	/* pointer to ioremap'ed MMIO */
@@ -360,7 +360,7 @@  struct net_device *alloc_arcdev(const char *name);
 int arcnet_open(struct net_device *dev);
 int arcnet_close(struct net_device *dev);
 netdev_tx_t arcnet_send_packet(struct sk_buff *skb,
-			       struct net_device *dev);
+				   struct net_device *dev);
 void arcnet_timeout(struct net_device *dev);
 
 /* I/O equivalents */
@@ -371,7 +371,23 @@  void arcnet_timeout(struct net_device *dev);
 #define BUS_ALIGN  1
 #endif
 
-/* addr and offset allow register like names to define the actual IO  address.
+#ifdef CONFIG_ARCNET_COM20020_MEMORY_BUS
+#define arcnet_inb(addr, offset)					\
+	ioread8((void __iomem *)(addr) + BUS_ALIGN * (offset))
+
+#define arcnet_outb(value, addr, offset)				\
+	iowrite8(value, (void __iomem *)(addr) + BUS_ALIGN * (offset))
+
+#define arcnet_insb(addr, offset, buffer, count)			\
+	ioread8_rep((void __iomem *)					\
+	(addr) + BUS_ALIGN * (offset), buffer, count)
+
+#define arcnet_outsb(addr, offset, buffer, count)			\
+	iowrite8_rep((void __iomem *)					\
+	(addr) + BUS_ALIGN * (offset), buffer, count)
+#else
+/**
+ * addr and offset allow register like names to define the actual IO  address.
  * A configuration option multiplies the offset for alignment.
  */
 #define arcnet_inb(addr, offset)					\
@@ -388,6 +404,7 @@  void arcnet_timeout(struct net_device *dev);
 	readb((addr) + (offset))
 #define arcnet_writeb(value, addr, offset)				\
 	writeb(value, (addr) + (offset))
+#endif
 
 #endif				/* __KERNEL__ */
 #endif				/* _LINUX_ARCDEVICE_H */
diff --git a/drivers/net/arcnet/com20020-membus.c b/drivers/net/arcnet/com20020-membus.c
new file mode 100644
index 000000000000..6e4a2f3a84f7
--- /dev/null
+++ b/drivers/net/arcnet/com20020-membus.c
@@ -0,0 +1,191 @@ 
+// SPDX-License-Identifier: (GPL-2.0 OR MIT)
+/* Linux ARCnet driver for com 20020.
+ *
+ * This datasheet:
+ * http://ww1.microchip.com/downloads/en/DeviceDoc/200223vrevc.pdf
+ * http://ww1.microchip.com/downloads/en/DeviceDoc/20020.pdf
+ *
+ * This driver support:
+ * - com20020,
+ * - com20022
+ * - com20022I-3v3
+ *
+ * This driver support only, 8bit read and write.
+ * DMA is not supported by this driver.
+ */
+#include <linux/module.h>
+#include <linux/types.h>
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/errno.h>
+#include <linux/platform_device.h>
+#include <linux/netdevice.h>
+#include <linux/of_address.h>
+#include <linux/of_gpio.h>
+#include <linux/sizes.h>
+#include <linux/interrupt.h>
+#include <linux/ioport.h>
+#include <linux/random.h>
+
+#include <linux/delay.h>
+#include "arcdevice.h"
+#include "com20020.h"
+
+#define VERSION "arcnet: COM20020 MEMORY BUS support loaded.\n"
+
+static int com20020_probe(struct platform_device *pdev)
+{
+	struct device_node *np;
+	struct net_device *dev;
+	struct arcnet_local *lp;
+	struct resource res, *iores;
+	int ret, phy_reset, err;
+	u32 timeout, backplane, clockp, clockm;
+	void __iomem *ioaddr;
+
+	np = pdev->dev.of_node;
+
+	iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+
+	if (of_address_to_resource(np, 0, &res))
+		return -EINVAL;
+
+	ret = of_property_read_u32(np, "timeout", &timeout);
+	if (ret) {
+		dev_err(&pdev->dev, "timeout is required param");
+		return ret;
+	}
+
+	ret = of_property_read_u32(np, "backplane", &backplane);
+	if (ret) {
+		dev_err(&pdev->dev, "backplane is required param");
+		return ret;
+	}
+
+	ret = of_property_read_u32(np, "clockp", &clockp);
+	if (ret) {
+		dev_err(&pdev->dev, "clockp is required param");
+		return ret;
+	}
+
+	ret = of_property_read_u32(np, "clockm", &clockm);
+	if (ret) {
+		dev_err(&pdev->dev, "clockm is required param");
+		return ret;
+	}
+
+	phy_reset = of_get_named_gpio(np, "phy-reset-gpios", 0);
+	if (phy_reset == -EPROBE_DEFER) {
+		return phy_reset;
+	} else if (!gpio_is_valid(phy_reset)) {
+		dev_err(&pdev->dev, "phy-reset-gpios not valid !");
+		return 0;
+	}
+
+	err = devm_gpio_request_one(&pdev->dev, phy_reset, GPIOF_OUT_INIT_LOW,
+				    "arcnet-phy-reset");
+	if (err) {
+		dev_err(&pdev->dev, "failed to get phy-reset-gpios: %d\n", err);
+		return err;
+	}
+
+	dev = alloc_arcdev(NULL);// Let autoassign name arc%d
+	dev->netdev_ops = &com20020_netdev_ops;
+	lp = netdev_priv(dev);
+
+	lp->card_flags = ARC_CAN_10MBIT;/* pretend all of them can 10Mbit */
+
+	// Force address to 0
+	// Will be set by user with `ip set dev arc0 address YOUR_NODE_ID`
+	dev->dev_addr[0] = 0;
+
+	// request to system this memory region
+	if (!devm_request_mem_region(&pdev->dev, res.start, resource_size(&res),
+				     lp->card_name))
+		return -EBUSY;
+
+	ioaddr = devm_ioremap(&pdev->dev, iores->start, resource_size(iores));
+	if (!ioaddr) {
+		dev_err(&pdev->dev, "ioremap fallied\n");
+		return -ENOMEM;
+	}
+
+	// Reset time is 5 * xTalFreq, minimal xtal is 10Mhz
+	// (5 * 1000) / 10Mhz = 500ns
+
+	gpio_set_value_cansleep(phy_reset, 0);
+	ndelay(500);
+	gpio_set_value_cansleep(phy_reset, 1);
+	ndelay(500);
+
+	/* Dummy access after Reset
+	 * ARCNET controller needs
+	 * this access to detect bustype
+	 */
+	arcnet_outb(0x00, ioaddr, COM20020_REG_W_COMMAND);
+	arcnet_inb(ioaddr, COM20020_REG_R_DIAGSTAT);
+
+	dev->base_addr = (unsigned long)ioaddr;
+	get_random_bytes(dev->dev_addr, sizeof(u8));
+
+	dev->irq = of_get_named_gpio(np, "phy-irq-gpios", 0);
+	if (dev->irq == -EPROBE_DEFER) {
+		return dev->irq;
+	} else if (!gpio_is_valid(dev->irq)) {
+		dev_err(&pdev->dev, "phy-irq-gpios not valid !");
+		return 0;
+	}
+	dev->irq = gpio_to_irq(dev->irq);
+
+	lp->backplane = backplane;
+	lp->clockp = clockp & 7;
+	lp->clockm = clockm & 3;
+	lp->timeout = timeout;
+	lp->hw.owner = THIS_MODULE;
+
+	if (arcnet_inb(ioaddr, COM20020_REG_R_STATUS) == 0xFF) {
+		ret = -EIO;
+		goto err_release_mem;
+	}
+
+	if (com20020_check(dev)) {
+		ret = -EIO;
+		goto err_release_mem;
+	}
+
+	ret = com20020_found(dev, IRQF_TRIGGER_FALLING);
+	if (ret)
+		goto err_release_mem;
+
+	dev_dbg(&pdev->dev, "probe Done\n");
+	return 0;
+
+err_release_mem:
+	devm_iounmap(&pdev->dev, (void __iomem *)ioaddr);
+	devm_release_mem_region(&pdev->dev, res.start, resource_size(&res));
+	dev_err(&pdev->dev, "probe failed!\n");
+	return ret;
+}
+
+static const struct of_device_id of_com20020_match[] = {
+	{ .compatible = "smsc,com20020",	},
+	{ },
+};
+
+MODULE_DEVICE_TABLE(of, of_com20020_match);
+
+static struct platform_driver of_com20020_driver = {
+	.driver			= {
+		.name		= "com20020-memory-bus",
+		.of_match_table = of_com20020_match,
+	},
+	.probe			= com20020_probe,
+};
+
+static int com20020_init(void)
+{
+	return platform_driver_register(&of_com20020_driver);
+}
+late_initcall(com20020_init);
+
+MODULE_LICENSE("GPL");
diff --git a/drivers/net/arcnet/com20020.c b/drivers/net/arcnet/com20020.c
index 78043a9c5981..f09ea77dd6a8 100644
--- a/drivers/net/arcnet/com20020.c
+++ b/drivers/net/arcnet/com20020.c
@@ -43,7 +43,7 @@ 
 #include "com20020.h"
 
 static const char * const clockrates[] = {
-	"XXXXXXX", "XXXXXXXX", "XXXXXX", "2.5 Mb/s",
+	"10 Mb/s", "XXXXXXXX", "XXXXXX", "2.5 Mb/s",
 	"1.25Mb/s", "625 Kb/s", "312.5 Kb/s", "156.25 Kb/s",
 	"Reserved", "Reserved", "Reserved"
 };
@@ -391,9 +391,10 @@  static void com20020_set_mc_list(struct net_device *dev)
 	}
 }
 
-#if defined(CONFIG_ARCNET_COM20020_PCI_MODULE) || \
-    defined(CONFIG_ARCNET_COM20020_ISA_MODULE) || \
-    defined(CONFIG_ARCNET_COM20020_CS_MODULE)
+#if	defined(CONFIG_ARCNET_COM20020_PCI_MODULE) || \
+	defined(CONFIG_ARCNET_COM20020_ISA_MODULE) || \
+	defined(CONFIG_ARCNET_COM20020_CS_MODULE)  || \
+	defined(CONFIG_ARCNET_COM20020_MEMORY_BUS)
 EXPORT_SYMBOL(com20020_check);
 EXPORT_SYMBOL(com20020_found);
 EXPORT_SYMBOL(com20020_netdev_ops);