P.B.Cheblakov wrote:
> Hello!
>
> It's a next version of a driver.
The driver already looks quite nice now. Just a few more minor issues...
>
> The changes are:
> - removed
> enum plx_cards;
> - removed
> void __iomem *base_addr[PLX_PCI_MAX_CHAN];
> - added prefixes "plx_pci_" for some structures;
> - struct channel_map ch_map_tbl[PLX_PCI_MAX_CHAN + 1]
> splited into two structures:
> struct channel_map conf_map;
> struct channel_map chan_map_tbl[PLX_PCI_MAX_CHAN];
> - plx_card_info_tbl[] replaced by set of static structures for each card;
> - readb/writeb replaced by ioread8/iowrite8
> - rewrited probing mechanism.
> sja1000 set into PeliCAN mode and Reset mode.
> Then check states of some registers.
> - avoided using dev->base_addr;
> - added two additional fields into struct plx_pci_card_info for supporting
> custom OCR and CDR.
>
> Also I've noticed that struct pci_dev *pci_dev;
>
> in
>
> struct plx_pci_card {
> int channels;
>
> struct pci_dev *pci_dev;
> struct net_device *net_dev[PLX_PCI_MAX_CHAN];
>
> void __iomem *conf_addr;
> };
>
> may be removed as it is not necessary in this driver anyway.
> Are there some reasons to leave it?
If it's not needed it should be removed, of course.
>> You inline patch is somehow white space mangled. Tabs are replaced with
>> 4 spaces, etc. Please make sure that your mail client does not change
>> the patch.
>>
>> P.B.Cheblakov wrote:
>>
>>> > Index: kernel/2.6/drivers/net/can/sja1000/plx_pci.c
>>> > ===================================================================
>>> > --- kernel/2.6/drivers/net/can/sja1000/plx_pci.c
>>> (.../svn+ssh://chebla...@v5cvs/socketcan/vendor/socketcan/1095)
>>> (revision 0)
>>> > +++ kernel/2.6/drivers/net/can/sja1000/plx_pci.c (revision 201)
>>> > @@ -0,0 +1,436 @@
>>> > +/*
> As I see in your reply my inline patch is right-formatted except sign
> "+" that
> mangles only its space-aligned lines. I use Thunderbird with preformat
> text style.
Thunderbird is well known to mangle inline patches as you can see below.
I use the Thunderbird plugin "Toggle Word Wrap" to include patches.
> Pavel.
>
>
> Index: 2.6/drivers/net/can/sja1000/plx_pci.c
> ===================================================================
> --- 2.6/drivers/net/can/sja1000/plx_pci.c
> (.../vendor/socketcan/1095/kernel) (revision 0)
> +++ 2.6/drivers/net/can/sja1000/plx_pci.c
> (.../socketcan/trunk/kernel) (revision 203)
[snip}
> +#define PLX_INTCSR 0x4c /* Interrup Control/Status */
> +#define PLX_CNTRL 0x50 /* User I/O, Direct Slave Response,
> + * Serial EEPROM, and Initialization
> + * Control register
> + */
> +
> +#define PLX_LINT1_EN 0x1 /* Local interrupt 1 enable */
> +#define PLX_LINT2_EN (1 << 3) /* Local interrupt 2 enable */
> +#define PLX_PCI_INT_EN (1 << 6) /* PCI Interrupt Enable */
> +#define PLX_PCI_RESET (1 << 30) /* PCI Adapter Software Reset */
> +
> +/*
> + * The board configuration is probably following:
> + * RX1 is connected to ground.
> + * TX1 is not connected.
> + * CLKO is not connected.
> + * Setting the OCR register to 0xDA is a good idea.
> + * This means normal output mode, push-pull and the correct polarity.
> + */
> +#define PLX_PCI_OCR (OCR_TX0_PUSHPULL | OCR_TX1_PUSHPULL)
> +
> +/*
> + * In the CDR register, you should set CBP to 1.
> + * You will probably also want to set the clock divider value to 7
> + * (meaning direct oscillator output) because the second SJA1000 chip
> + * is driven by the first one CLKOUT output.
> + */
> +#define PLX_PCI_CDR (CDR_CBP | CDR_CLKOUT_MASK)
> +
> +#define ADLINK_PCI_VENDOR_ID 0x144A
> +#define ADLINK_PCI_DEVICE_ID 0x7841
> +
> +#define MARATHON_PCI_DEVICE_ID 0x2715
> +
> +#define TEWS_PCI_VENDOR_ID 0x1498
> +#define TEWS_PCI_DEVICE_ID_TMPC810 0x032A
> +
> +struct plx_pci_channel_map {
> + u32 bar;
> + u32 offset;
> + u32 size; /* 0x00 - auto, e.g. length of entire bar */
> +};
Here and above, tabs are replaced with spaces, etc. Therefore I include
your attached patch manually below again.
> /*
> * Copyright (C) 2008-2010 Pavel Cheblakov <[email protected]>
> *
> * Derived from the ems_pci.c driver:
> * Copyright (C) 2007 Wolfgang Grandegger <[email protected]>
> * Copyright (C) 2008 Markus Plessing <[email protected]>
> * Copyright (C) 2008 Sebastian Haas <[email protected]>
> *
> *
> * This program is free software; you can redistribute it and/or modify
> * it under the terms of the version 2 of the GNU General Public License
> * as published by the Free Software Foundation
> *
> * This program is distributed in the hope that it will be useful,
> * but WITHOUT ANY WARRANTY; without even the implied warranty of
> * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> * GNU General Public License for more details.
> *
> * You should have received a copy of the GNU General Public License
> * along with this program; if not, write to the Free Software Foundation,
> * Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA.
> */
>
> /*
> * Driver sja1000_plx_pci, version 1.2.0, 2010-01-15
> */
Please remove this version string as it will never be
updated/maintained, for sure. The complete comment seems not really
descriptive.
> #include <linux/kernel.h>
> #include <linux/module.h>
> #include <linux/interrupt.h>
> #include <linux/netdevice.h>
> #include <linux/delay.h>
> #include <linux/pci.h>
> #include <socketcan/can.h>
> #include <socketcan/can/dev.h>
> #if LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 16)
> #include <linux/io.h>
> #else
> #include <asm/io.h>
> #endif
>
> #include "sja1000.h"
>
> #define DRV_NAME "sja1000_plx_pci"
>
> MODULE_AUTHOR("Pavel Cheblakov <[email protected]>");
> MODULE_DESCRIPTION("Socket-CAN driver for PLX9xxx PCI-bridge cards with "
> "sja1000 chips, version 1.2.0");
No version string please.
> MODULE_SUPPORTED_DEVICE("Marathon CAN-bus-PCI, "
> "Adlink PCI-7841/cPCI-7841, "
> "Adlink PCI-7841/cPCI-7841 SE, "
> "TEWS TECHNOLOGIES TPMC810");
> MODULE_LICENSE("GPL v2");
>
> #define PLX_PCI_MAX_CHAN 2
>
> struct plx_pci_card {
> int channels; /* detected channels count */
>
> struct pci_dev *pci_dev;
It should be removed if not needed.
> struct net_device *net_dev[PLX_PCI_MAX_CHAN];
>
> void __iomem *conf_addr;
> };
>
> #define PLX_PCI_CAN_CLOCK (16000000 / 2)
>
> /*
> * PLX9xxx registers
> */
> #define PLX_INTCSR 0x4c /* Interrup Control/Status */
> #define PLX_CNTRL 0x50 /* User I/O, Direct Slave Response,
> * Serial EEPROM, and Initialization
> * Control register
> */
>
> #define PLX_LINT1_EN 0x1 /* Local interrupt 1 enable */
> #define PLX_LINT2_EN (1 << 3) /* Local interrupt 2 enable */
> #define PLX_PCI_INT_EN (1 << 6) /* PCI Interrupt Enable */
> #define PLX_PCI_RESET (1 << 30) /* PCI Adapter Software Reset */
>
> /*
> * The board configuration is probably following:
> * RX1 is connected to ground.
> * TX1 is not connected.
> * CLKO is not connected.
> * Setting the OCR register to 0xDA is a good idea.
> * This means normal output mode, push-pull and the correct polarity.
> */
> #define PLX_PCI_OCR (OCR_TX0_PUSHPULL | OCR_TX1_PUSHPULL)
>
> /*
> * In the CDR register, you should set CBP to 1.
> * You will probably also want to set the clock divider value to 7
> * (meaning direct oscillator output) because the second SJA1000 chip
> * is driven by the first one CLKOUT output.
> */
> #define PLX_PCI_CDR (CDR_CBP | CDR_CLKOUT_MASK)
>
> #define ADLINK_PCI_VENDOR_ID 0x144A
> #define ADLINK_PCI_DEVICE_ID 0x7841
>
> #define MARATHON_PCI_DEVICE_ID 0x2715
>
> #define TEWS_PCI_VENDOR_ID 0x1498
> #define TEWS_PCI_DEVICE_ID_TMPC810 0x032A
>
> struct plx_pci_channel_map {
> u32 bar;
> u32 offset;
> u32 size; /* 0x00 - auto, e.g. length of entire bar */
> };
>
> struct plx_pci_card_info {
> const char *name;
> u32 channel_count;
Type int like "channels" above?
> int can_clock;
Should be of type u32.
> u8 ocr; /* output control register */
> u8 cdr; /* clock divider register */
>
> /* Parameters for mapping local configuration space */
> struct plx_pci_channel_map conf_map;
>
> /* Parametrs for mapping sja1000 chips */
Typo?
> struct plx_pci_channel_map chan_map_tbl[PLX_PCI_MAX_CHAN];
> };
>
> static struct plx_pci_card_info plx_pci_card_info_marathon __devinitdata = {
> "Marathon CAN-bus-PCI", 2,
> PLX_PCI_CAN_CLOCK, PLX_PCI_OCR, PLX_PCI_CDR,
> {0, 0x00, 0x00}, { {2, 0x00, 0x00}, {4, 0x00, 0x00} }
> };
>
> static struct plx_pci_card_info plx_pci_card_info_adlink __devinitdata = {
> "Adlink PCI-7841/cPCI-7841", 2,
> PLX_PCI_CAN_CLOCK, PLX_PCI_OCR, PLX_PCI_CDR,
> {1, 0x00, 0x00}, { {2, 0x00, 0x80}, {2, 0x80, 0x80} }
> };
>
> static struct plx_pci_card_info plx_pci_card_info_adlink_se __devinitdata = {
> "Adlink PCI-7841/cPCI-7841 SE", 2,
> PLX_PCI_CAN_CLOCK, PLX_PCI_OCR, PLX_PCI_CDR,
> {0, 0x00, 0x00}, { {2, 0x00, 0x80}, {2, 0x80, 0x80} }
> };
>
> static struct plx_pci_card_info plx_pci_card_info_tews __devinitdata = {
> "TEWS TECHNOLOGIES TPMC810", 2,
> PLX_PCI_CAN_CLOCK, PLX_PCI_OCR, PLX_PCI_CDR,
> {0, 0x00, 0x00}, { {2, 0x000, 0x80}, {2, 0x100, 0x80} }
> };
>
> static struct pci_device_id plx_pci_tbl[] = {
> {
> /* Marathon CAN-bus-PCI card */
> PCI_VENDOR_ID_PLX, MARATHON_PCI_DEVICE_ID,
> PCI_ANY_ID, PCI_ANY_ID,
> 0, 0,
> (kernel_ulong_t)&plx_pci_card_info_marathon
> },
> {
> /* Adlink PCI-7841/cPCI-7841 */
> ADLINK_PCI_VENDOR_ID, ADLINK_PCI_DEVICE_ID,
> PCI_ANY_ID, PCI_ANY_ID,
> PCI_CLASS_NETWORK_OTHER << 8, ~0,
> (kernel_ulong_t)&plx_pci_card_info_adlink
> },
> {
> /* Adlink PCI-7841/cPCI-7841 SE */
> ADLINK_PCI_VENDOR_ID, ADLINK_PCI_DEVICE_ID,
> PCI_ANY_ID, PCI_ANY_ID,
> PCI_CLASS_COMMUNICATION_OTHER << 8, ~0,
> (kernel_ulong_t)&plx_pci_card_info_adlink_se
> },
> {
> /* TEWS TECHNOLOGIES TPMC810 card */
> TEWS_PCI_VENDOR_ID, TEWS_PCI_DEVICE_ID_TMPC810,
> PCI_ANY_ID, PCI_ANY_ID,
> 0, 0,
> (kernel_ulong_t)&plx_pci_card_info_tews
> },
> { 0,}
> };
> MODULE_DEVICE_TABLE(pci, plx_pci_tbl);
>
> static u8 plx_pci_read_reg(const struct sja1000_priv *priv, int port)
> {
> return ioread8(priv->reg_base + port);
> }
>
> static void plx_pci_write_reg(const struct sja1000_priv *priv, int port, u8
> val)
> {
> iowrite8(val, priv->reg_base + port);
> }
>
> /*
> * Check if a CAN controller is present at the specified location
> * by trying to set 'em into the PeliCAN mode.
> * Also check states of some registers in reset mode.
> */
> static inline int plx_pci_check_sja1000(const struct sja1000_priv *priv)
> {
> /* Make sure SJA1000 is in reset mode */
> priv->write_reg(priv, REG_MOD, 1);
>
> priv->write_reg(priv, REG_CDR, CDR_PELICAN);
>
> /* See states of these registers after reset on p. 23 of Datasheet*/
> if ((priv->read_reg(priv, REG_MOD) & 0xf1) == 0x01 &&
> (priv->read_reg(priv, REG_SR) & 0x37) == 0x34 &&
> (priv->read_reg(priv, REG_IR) & 0xfb) == 0x00 &&
> priv->read_reg(priv, REG_CDR) == CDR_PELICAN)
> return 1;
>
> return 0;
> }
Does plx_pci_reset() really reset the SJA1000. Does the value check from
the IXXAT driver work on your cards as well? The idea is to check if the
chip is present by reading and comparing some register values when the
chip comes out of the reset. For this check it's not necessary to write
any registers.
> /*
> * Software reset PLX9xxx
> * Also LRESET# asserts and brings to reset device on the Local Bus
> */
> static void plx_pci_reset(struct plx_pci_card *card)
> {
> u32 cntrl;
>
> cntrl = ioread32(card->conf_addr + PLX_CNTRL);
> cntrl |= PLX_PCI_RESET;
> iowrite32(cntrl, card->conf_addr + PLX_CNTRL);
> udelay(100);
> cntrl ^= PLX_PCI_RESET;
> iowrite32(cntrl, card->conf_addr + PLX_CNTRL);
> };
>
> static void plx_pci_del_card(struct pci_dev *pdev)
> {
> struct plx_pci_card *card = pci_get_drvdata(pdev);
> struct net_device *dev;
> struct sja1000_priv *priv;
>
Please remove the empty line above.
> int i = 0;
>
> for (i = 0; i < card->channels; i++) {
> dev = card->net_dev[i];
> if (!dev)
> continue;
>
> dev_info(&pdev->dev, "Removing %s\n", dev->name);
> unregister_sja1000dev(dev);
> priv = netdev_priv(dev);
> if (priv->reg_base)
> pci_iounmap(pdev, priv->reg_base);
> free_sja1000dev(dev);
> }
>
> plx_pci_reset(card);
>
> /*
> * Disable interrupts from PCI-card (PLX9xxx) and disable Local_1,
> * Local_2 interrupts
> */
> iowrite32(0x0, card->conf_addr + PLX_INTCSR);
>
> if (card->conf_addr)
> pci_iounmap(pdev, card->conf_addr);
>
> kfree(card);
>
> pci_disable_device(pdev);
> pci_set_drvdata(pdev, NULL);
> }
>
> /*
> * Probe PLX9xxx based device for SJA1000 chips and register each available
> * CAN channel to SJA1000 Socket-CAN subsystem.
> */
> static int __devinit plx_pci_add_card(struct pci_dev *pdev,
> const struct pci_device_id *ent)
> {
> struct sja1000_priv *priv;
> struct net_device *dev;
> struct plx_pci_card *card;
> struct plx_pci_card_info *ci;
> int err, i;
> u32 val;
> void __iomem *addr;
>
> ci = (struct plx_pci_card_info *)ent->driver_data;
>
> if (pci_enable_device(pdev) < 0) {
> dev_err(&pdev->dev, "Failed to enable PCI device\n");
> return -ENODEV;
> }
>
> dev_info(&pdev->dev, "Detected \"%s\" card at slot #%i\n",
> ci->name, PCI_SLOT(pdev->devfn));
>
> /* Allocate card structures to hold addresses, ... */
> card = kzalloc(sizeof(*card), GFP_KERNEL);
> if (!card) {
> dev_err(&pdev->dev, "Unable to allocate memory\n");
> pci_disable_device(pdev);
> return -ENOMEM;
> }
>
> pci_set_drvdata(pdev, card);
>
> card->pci_dev = pdev;
> card->channels = 0;
>
> /* Remap PLX9xxx configuration space */
> addr = pci_iomap(pdev, ci->conf_map.bar, ci->conf_map.size);
> if (!addr) {
> err = -ENOMEM;
> dev_err(&pdev->dev,
> "Failed to remap configuration space (BAR%d)\n",
> ci->conf_map.bar);
This call may fit on two lines.
> goto failure_cleanup;
> }
> card->conf_addr = addr + ci->conf_map.offset;
> dev_dbg(&pdev->dev, "Configuration space remaped to 0x%p\n",
> card->conf_addr);
This message is mainly useful for debugging. Consider removing it.
> plx_pci_reset(card);
>
> /* Detect available channels */
> for (i = 0; i < ci->channel_count; i++) {
> struct plx_pci_channel_map *cm = &ci->chan_map_tbl[i];
Add one empty line here, please.
> dev = alloc_sja1000dev(0);
> if (!dev) {
> err = -ENOMEM;
> goto failure_cleanup;
> }
>
> card->net_dev[i] = dev;
> priv = netdev_priv(dev);
> priv->priv = card;
> #if LINUX_VERSION_CODE < KERNEL_VERSION(2, 6, 18)
> priv->irq_flags = SA_SHIRQ;
> #else
> priv->irq_flags = IRQF_SHARED;
> #endif
>
> dev->irq = pdev->irq;
>
> /*
> * Remap IO space of SJA1000 chips
> * This is device-dependent mapping
> */
> addr = pci_iomap(pdev, cm->bar, cm->size);
> if (!addr) {
> err = -ENOMEM;
> dev_err(&pdev->dev, "Failed to remap BAR%d\n", cm->bar);
> goto failure_cleanup;
> }
>
> priv->reg_base = addr + cm->offset;
> priv->read_reg = plx_pci_read_reg;
> priv->write_reg = plx_pci_write_reg;
>
> dev_dbg(&pdev->dev, "IO space of chip #%d remaped to 0x%p\n",
> i + 1, priv->reg_base);
Ditto
> /* Check if channel is present */
> if (plx_pci_check_sja1000(priv)) {
> priv->can.clock.freq = ci->can_clock;
> priv->ocr = ci->ocr;
> priv->cdr = ci->cdr;
>
> SET_NETDEV_DEV(dev, &pdev->dev);
>
> /* Register SJA1000 device */
> err = register_sja1000dev(dev);
> if (err) {
> dev_err(&pdev->dev, "Registering device failed "
> "(err=%d)\n", err);
> free_sja1000dev(dev);
> goto failure_cleanup;
> }
>
> card->channels++;
>
> dev_info(&pdev->dev, "Channel #%d at 0x%p, irq %d "
> "registered as %s\n",
> i + 1, priv->reg_base,
> dev->irq, dev->name);
This call may fit on three lines.
> } else {
> dev_err(&pdev->dev, "Channel #%d not detected\n",
> i + 1);
> free_sja1000dev(dev);
> }
> }
>
> if (!card->channels) {
> err = -ENODEV;
> goto failure_cleanup;
> }
>
> /*
> * Enable interrupts from PCI-card (PLX9xxx) and enable Local_1,
> * Local_2 interrupts from SJA1000 chips
> */
> val = ioread32(card->conf_addr + PLX_INTCSR);
> val |= PLX_LINT1_EN | PLX_LINT2_EN | PLX_PCI_INT_EN;
> iowrite32(val, card->conf_addr + PLX_INTCSR);
>
> return 0;
>
> failure_cleanup:
> dev_err(&pdev->dev, "Error: %d. Cleaning Up.\n", err);
>
> plx_pci_del_card(pdev);
>
> return err;
> }
>
> static struct pci_driver plx_pci_driver = {
> .name = DRV_NAME,
> .id_table = plx_pci_tbl,
> .probe = plx_pci_add_card,
> .remove = plx_pci_del_card,
> };
>
> static int __init plx_pci_init(void)
> {
> return pci_register_driver(&plx_pci_driver);
> }
>
> static void __exit plx_pci_exit(void)
> {
> pci_unregister_driver(&plx_pci_driver);
> }
>
> module_init(plx_pci_init);
> module_exit(plx_pci_exit);
The next version of your patch will likely be accepted. Therefore please
add the missing hunks for the Kconfig and Makefile and add a patch
description including your signed-off-by line.
Wolfgang.
_______________________________________________
Socketcan-core mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/socketcan-core
