| Message ID | 4A16BAAE.3070401@grandegger.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
Hi Wolfgang, thanks for the quick response. Comments below... On Fri, May 22, 2009 at 8:46 AM, Wolfgang Grandegger <wg@grandegger.com> wrote: > +++ net-next-2.6/Documentation/powerpc/dts-bindings/can/sja1000.txt > @@ -0,0 +1,37 @@ > +Memory mapped SJA1000 CAN controller from NXP (formerly Philips) > + > +Required properties: > + > +- compatible : should be "nxp,sja1000". > +- reg : should specify the chip select, address offset and size used > + for the chip depending on the bus it is connected to. > +- interrupts: property with a value describing the interrupt source > + (number and sensitivity) for that device. The encoding depends > + on the type of interrupt controller used. Hmmm, "reg", "interrupts", and "interrupt-parent" are well understood properties. I don't think we need to keep boilerplate defining the meaning every time a new binding is added. (general musing; not an ack or nack of this patch) However, what should be defined is *what* the register range is (ie. one tuple; location of device registers), and what the interrupts are (ie. single tuple for device's irq line). Granted this is a trivial case, but in the case of devices with more than one address range or irq line, the meaning of each tuple is critical information. I think it would be a good pattern to establish. > +Optional properties: > + > +- interrupt-parent : the phandle for the interrupt controller that > + services interrupts for that device. Thinking further; I wouldn't even mention "interrupt-parent" here. Anyone working with this stuff must already understand irq routing. > +- clock-frequency : CAN system clock frequency in Hz, which is normally > + half of the oscillator clock frequency. If not specified, a > + default value of 8000000 (8 MHz) is used. A clock-frequency property typically refers to the bus clock frequency. Something like can-frequency would be better. > +- cdr-reg : value of the SJA1000 clock divider register according to > + the SJA1000 data sheet. If not specified, a default value of > + 0x48 is used. Ewh. The driver should be clueful enough to derive the clock divider value given both the bus and can frequencies. I don't like this property. > +- ocr-reg : value of the SJA1000 output control register according to > + the SJA1000 data sheet. If not specified, a default value of > + 0x0a is used. Ditto here; the binding should describe the usage mode; not the register settings to get the usage mode. What sort of settings will the .dts author be writing here? Cheers, g.
Hi Grant, Grant Likely wrote: > Hi Wolfgang, thanks for the quick response. Comments below... > > On Fri, May 22, 2009 at 8:46 AM, Wolfgang Grandegger <wg@grandegger.com> wrote: >> +++ net-next-2.6/Documentation/powerpc/dts-bindings/can/sja1000.txt >> @@ -0,0 +1,37 @@ >> +Memory mapped SJA1000 CAN controller from NXP (formerly Philips) >> + >> +Required properties: >> + >> +- compatible : should be "nxp,sja1000". >> +- reg : should specify the chip select, address offset and size used >> + for the chip depending on the bus it is connected to. >> +- interrupts: property with a value describing the interrupt source >> + (number and sensitivity) for that device. The encoding depends >> + on the type of interrupt controller used. > > Hmmm, "reg", "interrupts", and "interrupt-parent" are well understood > properties. I don't think we need to keep boilerplate defining the > meaning every time a new binding is added. (general musing; not an > ack or nack of this patch) OK. > However, what should be defined is *what* the register range is (ie. > one tuple; location of device registers), and what the interrupts are > (ie. single tuple for device's irq line). Granted this is a trivial > case, but in the case of devices with more than one address range or > irq line, the meaning of each tuple is critical information. I think > it would be a good pattern to establish. Sounds reasonable. >> +Optional properties: >> + >> +- interrupt-parent : the phandle for the interrupt controller that >> + services interrupts for that device. > > Thinking further; I wouldn't even mention "interrupt-parent" here. > Anyone working with this stuff must already understand irq routing. OK, I will remove it. >> +- clock-frequency : CAN system clock frequency in Hz, which is normally >> + half of the oscillator clock frequency. If not specified, a >> + default value of 8000000 (8 MHz) is used. > > A clock-frequency property typically refers to the bus clock > frequency. Something like can-frequency would be better. Ah, right, but I'm also not happy with "can-frequency". The manual speaks about the "internal clock", which is half of the external oscillator frequency. Maybe "internal-clock-frequency" might be better. >> +- cdr-reg : value of the SJA1000 clock divider register according to >> + the SJA1000 data sheet. If not specified, a default value of >> + 0x48 is used. > > Ewh. The driver should be clueful enough to derive the clock divider > value given both the bus and can frequencies. I don't like this > property. The clock divider register controls the CLKOUT frequency for the microcontroller another CAN controller and allows to deactivate the CLKOUT pin. It's not used to configure the CAN bus frequency. > >> +- ocr-reg : value of the SJA1000 output control register according to >> + the SJA1000 data sheet. If not specified, a default value of >> + 0x0a is used. > > Ditto here; the binding should describe the usage mode; not the > register settings to get the usage mode. What sort of settings will > the .dts author be writing here? Unfortunately, there are many: clkout-frequency bypass-comperator tx1-output-on-rx-irq #define OCR_MODE_BIPHASE 0x00 #define OCR_MODE_TEST 0x01 #define OCR_MODE_NORMAL 0x02 #define OCR_MODE_CLOCK 0x03 #define OCR_TX0_INVERT 0x04 #define OCR_TX0_PULLDOWN 0x08 #define OCR_TX0_PULLUP 0x10 #define OCR_TX0_PUSHPULL 0x18 #define OCR_TX1_INVERT 0x20 #define OCR_TX1_PULLDOWN 0x40 #define OCR_TX1_PULLUP 0x80 #define OCR_TX1_PUSHPULL 0xc0 I think implementing properties for each option is overkill. Wolfgang. > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday 22 May 2009, Wolfgang Grandegger wrote: > This patch adds a generic driver for SJA1000 chips on the OpenFirmware > platform bus found on embedded PowerPC systems. Nice driver! > +static u8 sja1000_ofp_read_reg(const struct net_device *dev, int reg) > +{ > + return in_8((void __iomem *)(dev->base_addr + reg)); > +} > + > +static void sja1000_ofp_write_reg(const struct net_device *dev, int reg, u8 val) > +{ > + out_8((void __iomem *)(dev->base_addr + reg), val); > +} Minor nitpicking: dev->base_addr should be defined as an __iomem pointer so you can avoid the cast here and in the ioremap/iounmap path. Arnd <>< -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Wolfgang Grandegger wrote: > Hi Grant, > > Grant Likely wrote: >> Hi Wolfgang, thanks for the quick response. Comments below... >> >> On Fri, May 22, 2009 at 8:46 AM, Wolfgang Grandegger <wg@grandegger.com> wrote: >>> +++ net-next-2.6/Documentation/powerpc/dts-bindings/can/sja1000.txt >>> @@ -0,0 +1,37 @@ >>> +Memory mapped SJA1000 CAN controller from NXP (formerly Philips) >>> + >>> +Required properties: >>> + >>> +- compatible : should be "nxp,sja1000". >>> +- reg : should specify the chip select, address offset and size used >>> + for the chip depending on the bus it is connected to. >>> +- interrupts: property with a value describing the interrupt source >>> + (number and sensitivity) for that device. The encoding depends >>> + on the type of interrupt controller used. >> Hmmm, "reg", "interrupts", and "interrupt-parent" are well understood >> properties. I don't think we need to keep boilerplate defining the >> meaning every time a new binding is added. (general musing; not an >> ack or nack of this patch) > > OK. > >> However, what should be defined is *what* the register range is (ie. >> one tuple; location of device registers), and what the interrupts are >> (ie. single tuple for device's irq line). Granted this is a trivial >> case, but in the case of devices with more than one address range or >> irq line, the meaning of each tuple is critical information. I think >> it would be a good pattern to establish. > > Sounds reasonable. > >>> +Optional properties: >>> + >>> +- interrupt-parent : the phandle for the interrupt controller that >>> + services interrupts for that device. >> Thinking further; I wouldn't even mention "interrupt-parent" here. >> Anyone working with this stuff must already understand irq routing. > > OK, I will remove it. > >>> +- clock-frequency : CAN system clock frequency in Hz, which is normally >>> + half of the oscillator clock frequency. If not specified, a >>> + default value of 8000000 (8 MHz) is used. >> A clock-frequency property typically refers to the bus clock >> frequency. Something like can-frequency would be better. > > Ah, right, but I'm also not happy with "can-frequency". The manual > speaks about the "internal clock", which is half of the external > oscillator frequency. Maybe "internal-clock-frequency" might be better. > >>> +- cdr-reg : value of the SJA1000 clock divider register according to >>> + the SJA1000 data sheet. If not specified, a default value of >>> + 0x48 is used. >> Ewh. The driver should be clueful enough to derive the clock divider >> value given both the bus and can frequencies. I don't like this >> property. > > The clock divider register controls the CLKOUT frequency for the > microcontroller another CAN controller and allows to deactivate the > CLKOUT pin. It's not used to configure the CAN bus frequency. > >>> +- ocr-reg : value of the SJA1000 output control register according to >>> + the SJA1000 data sheet. If not specified, a default value of >>> + 0x0a is used. >> Ditto here; the binding should describe the usage mode; not the >> register settings to get the usage mode. What sort of settings will >> the .dts author be writing here? > > Unfortunately, there are many: > > clkout-frequency > bypass-comperator > tx1-output-on-rx-irq > > #define OCR_MODE_BIPHASE 0x00 > #define OCR_MODE_TEST 0x01 > #define OCR_MODE_NORMAL 0x02 > #define OCR_MODE_CLOCK 0x03 > > #define OCR_TX0_INVERT 0x04 > #define OCR_TX0_PULLDOWN 0x08 > #define OCR_TX0_PULLUP 0x10 > #define OCR_TX0_PUSHPULL 0x18 > #define OCR_TX1_INVERT 0x20 > #define OCR_TX1_PULLDOWN 0x40 > #define OCR_TX1_PULLUP 0x80 > #define OCR_TX1_PUSHPULL 0xc0 > > I think implementing properties for each option is overkill. Would the following more descriptive properties be OK? clock-out-frequency = <0>, // CLKOUT pin clock off = <4000000>; // frequency on CLKOUT pin bypass-input-comparator; // allows to bypass the CAN input comparator. tx1-output-on-rx-irq; // allows the TX1 output to be used as a // dedicated RX interrupt output. output-control-mode = <0x0> // bi-phase output mode <0x1> // test output mode <0x2> // normal output mode (default) <0x3> // clock output mode output-pin-config = <0x01> // TX0 invert <0x02> // TX0 pull-down <0x04> // TX0 pull-up <0x06> // TX0 push-pull <0x08> // TX1 invert <0x10> // TX1 pull-down <0x20> // TX1 pull-up <0x30> // TX1 push-pull Wolfgang. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Arnd Bergmann wrote: > On Friday 22 May 2009, Wolfgang Grandegger wrote: >> This patch adds a generic driver for SJA1000 chips on the OpenFirmware >> platform bus found on embedded PowerPC systems. > > Nice driver! > >> +static u8 sja1000_ofp_read_reg(const struct net_device *dev, int reg) >> +{ >> + return in_8((void __iomem *)(dev->base_addr + reg)); >> +} >> + >> +static void sja1000_ofp_write_reg(const struct net_device *dev, int reg, u8 val) >> +{ >> + out_8((void __iomem *)(dev->base_addr + reg), val); >> +} > > Minor nitpicking: dev->base_addr should be defined as an __iomem pointer > so you can avoid the cast here and in the ioremap/iounmap path. Here the member "base_addr" of "struct net_device" is used and it's not up to me to change the type. Wolfgang. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Saturday 23 May 2009, Wolfgang Grandegger wrote: > Arnd Bergmann wrote: > > > > Minor nitpicking: dev->base_addr should be defined as an __iomem pointer > > so you can avoid the cast here and in the ioremap/iounmap path. > > Here the member "base_addr" of "struct net_device" is used and it's not > up to me to change the type. Right, that makes sense. However, most drivers use the field to store the physical address, not the iomap token. Maybe there should be a new field in struct sja1000_priv for the virtual address, but that would be a change to the base driver, not just to the OF portion. Thanks, Arnd <>< -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, May 23, 2009 at 10:44 AM, Wolfgang Grandegger <wg@grandegger.com> wrote: > Wolfgang Grandegger wrote: >> Grant Likely wrote: >>>> +- clock-frequency : CAN system clock frequency in Hz, which is normally >>>> + half of the oscillator clock frequency. If not specified, a >>>> + default value of 8000000 (8 MHz) is used. >>> A clock-frequency property typically refers to the bus clock >>> frequency. Something like can-frequency would be better. >> >> Ah, right, but I'm also not happy with "can-frequency". The manual >> speaks about the "internal clock", which is half of the external >> oscillator frequency. Maybe "internal-clock-frequency" might be better. Would it be better to specify the external clock frequency, and the driver know that internal freq is half that? I ask because external clock freq is a value the HW designer actually has control over. >>>> +- cdr-reg : value of the SJA1000 clock divider register according to >>>> + the SJA1000 data sheet. If not specified, a default value of >>>> + 0x48 is used. >>> Ewh. The driver should be clueful enough to derive the clock divider >>> value given both the bus and can frequencies. I don't like this >>> property. >> >> The clock divider register controls the CLKOUT frequency for the >> microcontroller another CAN controller and allows to deactivate the >> CLKOUT pin. It's not used to configure the CAN bus frequency. >> >>>> +- ocr-reg : value of the SJA1000 output control register according to >>>> + the SJA1000 data sheet. If not specified, a default value of >>>> + 0x0a is used. >>> Ditto here; the binding should describe the usage mode; not the >>> register settings to get the usage mode. What sort of settings will >>> the .dts author be writing here? >> >> Unfortunately, there are many: >> >> clkout-frequency >> bypass-comperator >> tx1-output-on-rx-irq >> >> #define OCR_MODE_BIPHASE 0x00 >> #define OCR_MODE_TEST 0x01 >> #define OCR_MODE_NORMAL 0x02 >> #define OCR_MODE_CLOCK 0x03 >> >> #define OCR_TX0_INVERT 0x04 >> #define OCR_TX0_PULLDOWN 0x08 >> #define OCR_TX0_PULLUP 0x10 >> #define OCR_TX0_PUSHPULL 0x18 >> #define OCR_TX1_INVERT 0x20 >> #define OCR_TX1_PULLDOWN 0x40 >> #define OCR_TX1_PULLUP 0x80 >> #define OCR_TX1_PUSHPULL 0xc0 >> >> I think implementing properties for each option is overkill. Ugh, I see what you mean. > Would the following more descriptive properties be OK? > > clock-out-frequency = <0>, // CLKOUT pin clock off > = <4000000>; // frequency on CLKOUT pin Or how about CLKOUT pin off if the property isn't present? Otherwise, this looks okay. BTW, I'd consider prefixing this with 'nxp,' or 'sja1000,' to protect the namespace. clock-out-frequency sounds like one of those names which could be commonly used in the future. I'd so the same for the other chip-specific properties too. Segher, what's your opinion on this? > bypass-input-comparator; // allows to bypass the CAN input comparator. > > tx1-output-on-rx-irq; // allows the TX1 output to be used as a > // dedicated RX interrupt output. > > output-control-mode = <0x0> // bi-phase output mode > <0x1> // test output mode > <0x2> // normal output mode (default) > <0x3> // clock output mode > > output-pin-config = <0x01> // TX0 invert > <0x02> // TX0 pull-down > <0x04> // TX0 pull-up > <0x06> // TX0 push-pull > <0x08> // TX1 invert > <0x10> // TX1 pull-down > <0x20> // TX1 pull-up > <0x30> // TX1 push-pull hmmm; It is very chip specific and it is a lot of mucking around. If you prefix the property with the manufacturer name, then perhaps the explicit register setting is okay. Are TX0 & TX1 protocol pins or GPIOs? If gpio, then maybe it is worth the mucking about to then use the gpios binding to specify the pin mode. g.
Arnd Bergmann wrote: > On Saturday 23 May 2009, Wolfgang Grandegger wrote: >> Arnd Bergmann wrote: >>> Minor nitpicking: dev->base_addr should be defined as an __iomem pointer >>> so you can avoid the cast here and in the ioremap/iounmap path. >> Here the member "base_addr" of "struct net_device" is used and it's not >> up to me to change the type. > > Right, that makes sense. However, most drivers use the field to store the > physical address, not the iomap token. Maybe there should be a new field > in struct sja1000_priv for the virtual address, but that would be a change > to the base driver, not just to the OF portion. Is that common practice? If yes, I will add a member to store the virtual address to struct sja1000_priv. Wolfgang. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Grant Likely wrote: > On Sat, May 23, 2009 at 10:44 AM, Wolfgang Grandegger <wg@grandegger.com> wrote: >> Wolfgang Grandegger wrote: >>> Grant Likely wrote: >>>>> +- clock-frequency : CAN system clock frequency in Hz, which is normally >>>>> + half of the oscillator clock frequency. If not specified, a >>>>> + default value of 8000000 (8 MHz) is used. >>>> A clock-frequency property typically refers to the bus clock >>>> frequency. Something like can-frequency would be better. >>> Ah, right, but I'm also not happy with "can-frequency". The manual >>> speaks about the "internal clock", which is half of the external >>> oscillator frequency. Maybe "internal-clock-frequency" might be better. > > Would it be better to specify the external clock frequency, and the > driver know that internal freq is half that? I ask because external > clock freq is a value the HW designer actually has control over. I'm sharing your arguments: "external-clock-frequency". There is always some confusion about external and internal clock frequency but the name should make it clear. >>>>> +- cdr-reg : value of the SJA1000 clock divider register according to >>>>> + the SJA1000 data sheet. If not specified, a default value of >>>>> + 0x48 is used. >>>> Ewh. The driver should be clueful enough to derive the clock divider >>>> value given both the bus and can frequencies. I don't like this >>>> property. >>> The clock divider register controls the CLKOUT frequency for the >>> microcontroller another CAN controller and allows to deactivate the >>> CLKOUT pin. It's not used to configure the CAN bus frequency. >>> >>>>> +- ocr-reg : value of the SJA1000 output control register according to >>>>> + the SJA1000 data sheet. If not specified, a default value of >>>>> + 0x0a is used. >>>> Ditto here; the binding should describe the usage mode; not the >>>> register settings to get the usage mode. What sort of settings will >>>> the .dts author be writing here? >>> Unfortunately, there are many: >>> >>> clkout-frequency >>> bypass-comperator >>> tx1-output-on-rx-irq >>> >>> #define OCR_MODE_BIPHASE 0x00 >>> #define OCR_MODE_TEST 0x01 >>> #define OCR_MODE_NORMAL 0x02 >>> #define OCR_MODE_CLOCK 0x03 >>> >>> #define OCR_TX0_INVERT 0x04 >>> #define OCR_TX0_PULLDOWN 0x08 >>> #define OCR_TX0_PULLUP 0x10 >>> #define OCR_TX0_PUSHPULL 0x18 >>> #define OCR_TX1_INVERT 0x20 >>> #define OCR_TX1_PULLDOWN 0x40 >>> #define OCR_TX1_PULLUP 0x80 >>> #define OCR_TX1_PUSHPULL 0xc0 >>> >>> I think implementing properties for each option is overkill. > > Ugh, I see what you mean. > >> Would the following more descriptive properties be OK? >> >> clock-out-frequency = <0>, // CLKOUT pin clock off >> = <4000000>; // frequency on CLKOUT pin > > Or how about CLKOUT pin off if the property isn't present? Otherwise, Yep, that will be the default anyhow. > this looks okay. BTW, I'd consider prefixing this with 'nxp,' or > 'sja1000,' to protect the namespace. clock-out-frequency sounds like > one of those names which could be commonly used in the future. I'd so > the same for the other chip-specific properties too. > Segher, what's your opinion on this? I personally don't have a real preference. >> bypass-input-comparator; // allows to bypass the CAN input comparator. >> >> tx1-output-on-rx-irq; // allows the TX1 output to be used as a >> // dedicated RX interrupt output. >> >> output-control-mode = <0x0> // bi-phase output mode >> <0x1> // test output mode >> <0x2> // normal output mode (default) >> <0x3> // clock output mode >> >> output-pin-config = <0x01> // TX0 invert >> <0x02> // TX0 pull-down >> <0x04> // TX0 pull-up >> <0x06> // TX0 push-pull >> <0x08> // TX1 invert >> <0x10> // TX1 pull-down >> <0x20> // TX1 pull-up >> <0x30> // TX1 push-pull > > hmmm; It is very chip specific and it is a lot of mucking around. If > you prefix the property with the manufacturer name, then perhaps the > explicit register setting is okay. OK. > Are TX0 & TX1 protocol pins or GPIOs? If gpio, then maybe it is worth > the mucking about to then use the gpios binding to specify the pin > mode. These are the output from the CAN output driver 0 or 1 to the physical bus line. E.g., in normal output mode the CAN bit sequence is sent via TX0 or TX1. From a non-hardware expert point of view, they must be programmed properly to get the hardware to work. Wolfgang. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday 25 May 2009, Wolfgang Grandegger wrote: > > Right, that makes sense. However, most drivers use the field to store the > > physical address, not the iomap token. Maybe there should be a new field > > in struct sja1000_priv for the virtual address, but that would be a change > > to the base driver, not just to the OF portion. > > Is that common practice? If yes, I will add a member to store the > virtual address to struct sja1000_priv. I grepped through the network driver for usage of ->base_addr, and it's somewhat inconsistent. The majority of the users use it for a physical address, but there are also a few that use it for the __iomem token. Casts between unsigned long and qualified (__iomem, __user, const, ...) pointers do not cause a warning, but can easily lead to bugs when another user casts to an unqualified pointer. Arnd <>< -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Arnd Bergmann <arnd@arndb.de> Date: Tue, 26 May 2009 10:10:30 +0100 > On Monday 25 May 2009, Wolfgang Grandegger wrote: >> > Right, that makes sense. However, most drivers use the field to store the >> > physical address, not the iomap token. Maybe there should be a new field >> > in struct sja1000_priv for the virtual address, but that would be a change >> > to the base driver, not just to the OF portion. >> >> Is that common practice? If yes, I will add a member to store the >> virtual address to struct sja1000_priv. > > I grepped through the network driver for usage of ->base_addr, and > it's somewhat inconsistent. The majority of the users use it for > a physical address, but there are also a few that use it for the > __iomem token. > > Casts between unsigned long and qualified (__iomem, __user, const, ...) > pointers do not cause a warning, but can easily lead to bugs when > another user casts to an unqualified pointer. It's such a baroque thing, there is no reason to set it at all if you ask me. It's only use is to allow ISA and similar primitive bus devices to have their I/O ports changed via ifconfig. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2009-05-26 at 10:10 +0100, Arnd Bergmann wrote: > On Monday 25 May 2009, Wolfgang Grandegger wrote: > > > Right, that makes sense. However, most drivers use the field to store the > > > physical address, not the iomap token. Maybe there should be a new field > > > in struct sja1000_priv for the virtual address, but that would be a change > > > to the base driver, not just to the OF portion. > > > > Is that common practice? If yes, I will add a member to store the > > virtual address to struct sja1000_priv. > > I grepped through the network driver for usage of ->base_addr, and > it's somewhat inconsistent. The majority of the users use it for > a physical address, but there are also a few that use it for the > __iomem token. In addition, iirc, it's not big enough to hold >32 bit physical addresses on 32-bit platforms no ? Ben. > Casts between unsigned long and qualified (__iomem, __user, const, ...) > pointers do not cause a warning, but can easily lead to bugs when > another user casts to an unqualified pointer. > > Arnd <>< > > _______________________________________________ > Linuxppc-dev mailing list > Linuxppc-dev@ozlabs.org > https://ozlabs.org/mailman/listinfo/linuxppc-dev -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 26 May 2009, David Miller wrote: > It's such a baroque thing, there is no reason to set it at all if you > ask me. It's only use is to allow ISA and similar primitive bus > devices to have their I/O ports changed via ifconfig. My original comment was about the fact that sja1000 was doing dev->base_addr = (unsigned long)ioremap(phys_addr, size), I didn't even think about SIOCGIFMAP and command line overrides, but that surely makes it worse and the driver should be changed to store the virtual register address in its private data structure. drivers/net/fec.c seems to have the same problem, which manifests in a number of ugly casts and direct pointer dereferences in places where it should do writel() or out_be32(). Arnd <>< -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 26, 2009 at 10:42:05AM +0100, Arnd Bergmann wrote: > On Tuesday 26 May 2009, David Miller wrote: > > It's such a baroque thing, there is no reason to set it at all if you > > ask me. It's only use is to allow ISA and similar primitive bus > > devices to have their I/O ports changed via ifconfig. > > My original comment was about the fact that sja1000 was doing > dev->base_addr = (unsigned long)ioremap(phys_addr, size), I didn't > even think about SIOCGIFMAP and command line overrides, but that > surely makes it worse and the driver should be changed to > store the virtual register address in its private data structure. > > drivers/net/fec.c seems to have the same problem, which manifests > in a number of ugly casts and direct pointer dereferences in places > where it should do writel() or out_be32(). Ack. I'll prepare a patch for fec.c. Internally the driver already uses a void __iomem * and writel/readl in -next. There is only one usage left. Sascha
David Miller wrote: > From: Arnd Bergmann <arnd@arndb.de> > Date: Tue, 26 May 2009 10:10:30 +0100 > >> On Monday 25 May 2009, Wolfgang Grandegger wrote: >>>> Right, that makes sense. However, most drivers use the field to store the >>>> physical address, not the iomap token. Maybe there should be a new field >>>> in struct sja1000_priv for the virtual address, but that would be a change >>>> to the base driver, not just to the OF portion. >>> Is that common practice? If yes, I will add a member to store the >>> virtual address to struct sja1000_priv. >> I grepped through the network driver for usage of ->base_addr, and >> it's somewhat inconsistent. The majority of the users use it for >> a physical address, but there are also a few that use it for the >> __iomem token. >> >> Casts between unsigned long and qualified (__iomem, __user, const, ...) >> pointers do not cause a warning, but can easily lead to bugs when >> another user casts to an unqualified pointer. > > It's such a baroque thing, there is no reason to set it at all if you > ask me. It's only use is to allow ISA and similar primitive bus > devices to have their I/O ports changed via ifconfig. OK, I see, there are good reasons not to (mis-)use dev->base_addr. I will prepare a patch for the SJA1000 CAN drivers. Wolfgang. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Wolfgang Grandegger wrote: > David Miller wrote: >> From: Arnd Bergmann <arnd@arndb.de> >> Date: Tue, 26 May 2009 10:10:30 +0100 >> >>> On Monday 25 May 2009, Wolfgang Grandegger wrote: >>>>> Right, that makes sense. However, most drivers use the field to store the >>>>> physical address, not the iomap token. Maybe there should be a new field >>>>> in struct sja1000_priv for the virtual address, but that would be a change >>>>> to the base driver, not just to the OF portion. >>>> Is that common practice? If yes, I will add a member to store the >>>> virtual address to struct sja1000_priv. >>> I grepped through the network driver for usage of ->base_addr, and >>> it's somewhat inconsistent. The majority of the users use it for >>> a physical address, but there are also a few that use it for the >>> __iomem token. >>> >>> Casts between unsigned long and qualified (__iomem, __user, const, ...) >>> pointers do not cause a warning, but can easily lead to bugs when >>> another user casts to an unqualified pointer. >> It's such a baroque thing, there is no reason to set it at all if you >> ask me. It's only use is to allow ISA and similar primitive bus >> devices to have their I/O ports changed via ifconfig. > > OK, I see, there are good reasons not to (mis-)use dev->base_addr. I > will prepare a patch for the SJA1000 CAN drivers. I have just sent out a patch series fixing this issue and providing a revised patch for the SJA1000 OF platform driver: [net-next-2.6 PATCH 0/3] can: sja1000: misused netdev->base_addr and OF platform driver Wolfgang. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Index: net-next-2.6/drivers/net/can/Kconfig =================================================================== --- net-next-2.6.orig/drivers/net/can/Kconfig +++ net-next-2.6/drivers/net/can/Kconfig @@ -51,6 +51,15 @@ config CAN_SJA1000_PLATFORM boards from Phytec (http://www.phytec.de) like the PCM027, PCM038. +config CAN_SJA1000_OF_PLATFORM + depends on CAN_SJA1000 && PPC_OF + tristate "Generic OF Platform Bus based SJA1000 driver" + ---help--- + This driver adds support for the SJA1000 chips connected to + the OpenFirmware "platform bus" found on embedded systems with + OpenFirmware bindings, e.g. if you have a PowerPC based system + you may want to enable this option. + config CAN_EMS_PCI tristate "EMS CPC-PCI and CPC-PCIe Card" depends on PCI && CAN_SJA1000 Index: net-next-2.6/drivers/net/can/sja1000/Makefile =================================================================== --- net-next-2.6.orig/drivers/net/can/sja1000/Makefile +++ net-next-2.6/drivers/net/can/sja1000/Makefile @@ -4,6 +4,7 @@ obj-$(CONFIG_CAN_SJA1000) += sja1000.o obj-$(CONFIG_CAN_SJA1000_PLATFORM) += sja1000_platform.o +obj-$(CONFIG_CAN_SJA1000_OF_PLATFORM) += sja1000_of_platform.o obj-$(CONFIG_CAN_EMS_PCI) += ems_pci.o obj-$(CONFIG_CAN_KVASER_PCI) += kvaser_pci.o Index: net-next-2.6/drivers/net/can/sja1000/sja1000_of_platform.c =================================================================== --- /dev/null +++ net-next-2.6/drivers/net/can/sja1000/sja1000_of_platform.c @@ -0,0 +1,215 @@ +/* + * Driver for SJA1000 CAN controllers on the OpenFirmware platform bus + * + * Copyright (C) 2008-2009 Wolfgang Grandegger <wg@grandegger.com> + * + * 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. + */ + +/* This is a generic driver for SJA1000 chips on the OpenFirmware platform + * bus found on embedded PowerPC systems. You need a SJA1000 CAN node + * definition in your flattened device tree source (DTS) file similar to: + * + * can@3,100 { + * compatible = "philips,sja1000"; + * reg = <3 0x100 0x80>; + * clock-frequency = <8000000>; + * cdr-reg = <0x48>; + * ocr-reg = <0x0a>; + * interrupts = <2 0>; + * interrupt-parent = <&mpic>; + * }; + */ + +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/interrupt.h> +#include <linux/netdevice.h> +#include <linux/delay.h> +#include <linux/can.h> +#include <linux/can/dev.h> + +#include <linux/of_platform.h> +#include <asm/prom.h> + +#include "sja1000.h" + +#define DRV_NAME "sja1000_of_platform" + +MODULE_AUTHOR("Wolfgang Grandegger <wg@grandegger.com>"); +MODULE_DESCRIPTION("Socket-CAN driver for SJA1000 on the OF platform bus"); +MODULE_LICENSE("GPL v2"); + +#define SJA1000_OFP_CAN_CLOCK (16000000 / 2) + +#define SJA1000_OFP_OCR OCR_TX0_PULLDOWN +#define SJA1000_OFP_CDR (CDR_CBP | CDR_CLK_OFF) + +static u8 sja1000_ofp_read_reg(const struct net_device *dev, int reg) +{ + return in_8((void __iomem *)(dev->base_addr + reg)); +} + +static void sja1000_ofp_write_reg(const struct net_device *dev, int reg, u8 val) +{ + out_8((void __iomem *)(dev->base_addr + reg), val); +} + +static int __devexit sja1000_ofp_remove(struct of_device *ofdev) +{ + struct net_device *dev = dev_get_drvdata(&ofdev->dev); + struct device_node *np = ofdev->node; + struct resource res; + + dev_set_drvdata(&ofdev->dev, NULL); + + unregister_sja1000dev(dev); + free_sja1000dev(dev); + iounmap((void __iomem *)dev->base_addr); + irq_dispose_mapping(dev->irq); + + of_address_to_resource(np, 0, &res); + release_mem_region(res.start, resource_size(&res)); + + return 0; +} + +static int __devinit sja1000_ofp_probe(struct of_device *ofdev, + const struct of_device_id *id) +{ + struct device_node *np = ofdev->node; + struct net_device *dev; + struct sja1000_priv *priv; + struct resource res; + const u32 *prop; + int err, irq, res_size, prop_size; + void __iomem *base; + + err = of_address_to_resource(np, 0, &res); + if (err) { + dev_err(&ofdev->dev, "invalid address\n"); + return err; + } + + res_size = resource_size(&res); + + if (!request_mem_region(res.start, res_size, DRV_NAME)) { + dev_err(&ofdev->dev, "couldn't request %#x..%#x\n", + res.start, res.end); + return -EBUSY; + } + + base = ioremap_nocache(res.start, res_size); + if (!base) { + dev_err(&ofdev->dev, "couldn't ioremap %#x..%#x\n", + res.start, res.end); + err = -ENOMEM; + goto exit_release_mem; + } + + irq = irq_of_parse_and_map(np, 0); + if (irq == NO_IRQ) { + dev_err(&ofdev->dev, "no irq found\n"); + err = -ENODEV; + goto exit_unmap_mem; + } + + dev = alloc_sja1000dev(0); + if (!dev) { + err = -ENOMEM; + goto exit_dispose_irq; + } + + priv = netdev_priv(dev); + + priv->read_reg = sja1000_ofp_read_reg; + priv->write_reg = sja1000_ofp_write_reg; + + prop = of_get_property(np, "clock-frequency", &prop_size); + if (prop && (prop_size == sizeof(u32))) + priv->can.clock.freq = *prop; + else + priv->can.clock.freq = SJA1000_OFP_CAN_CLOCK; + + prop = of_get_property(np, "ocr-reg", &prop_size); + if (prop && (prop_size == sizeof(u32))) + priv->ocr = (u8)*prop; + else + priv->ocr = SJA1000_OFP_OCR; + + prop = of_get_property(np, "cdr-reg", &prop_size); + if (prop && (prop_size == sizeof(u32))) + priv->cdr = (u8)*prop; + else + priv->cdr = SJA1000_OFP_CDR; + + priv->irq_flags = IRQF_SHARED; + + dev->irq = irq; + dev->base_addr = (unsigned long)base; + + dev_info(&ofdev->dev, + "base=0x%lx irq=%d clock=%d ocr=0x%02x cdr=0x%02x\n", + dev->base_addr, dev->irq, priv->can.clock.freq, + priv->ocr, priv->cdr); + + dev_set_drvdata(&ofdev->dev, dev); + SET_NETDEV_DEV(dev, &ofdev->dev); + + err = register_sja1000dev(dev); + if (err) { + dev_err(&ofdev->dev, "registering %s failed (err=%d)\n", + DRV_NAME, err); + goto exit_free_sja1000; + } + + return 0; + +exit_free_sja1000: + free_sja1000dev(dev); +exit_dispose_irq: + irq_dispose_mapping(irq); +exit_unmap_mem: + iounmap(base); +exit_release_mem: + release_mem_region(res.start, res_size); + + return err; +} + +static struct of_device_id __devinitdata sja1000_ofp_table[] = { + {.compatible = "philips,sja1000"}, + {.compatible = "nxp,sja1000"}, + {}, +}; + +static struct of_platform_driver sja1000_ofp_driver = { + .owner = THIS_MODULE, + .name = DRV_NAME, + .probe = sja1000_ofp_probe, + .remove = __devexit_p(sja1000_ofp_remove), + .match_table = sja1000_ofp_table, +}; + +static int __init sja1000_ofp_init(void) +{ + return of_register_platform_driver(&sja1000_ofp_driver); +} +module_init(sja1000_ofp_init); + +static void __exit sja1000_ofp_exit(void) +{ + return of_unregister_platform_driver(&sja1000_ofp_driver); +}; +module_exit(sja1000_ofp_exit); Index: net-next-2.6/Documentation/powerpc/dts-bindings/can/sja1000.txt =================================================================== --- /dev/null +++ net-next-2.6/Documentation/powerpc/dts-bindings/can/sja1000.txt @@ -0,0 +1,37 @@ +Memory mapped SJA1000 CAN controller from NXP (formerly Philips) + +Required properties: + +- compatible : should be "nxp,sja1000". +- reg : should specify the chip select, address offset and size used + for the chip depending on the bus it is connected to. +- interrupts: property with a value describing the interrupt source + (number and sensitivity) for that device. The encoding depends + on the type of interrupt controller used. + +Optional properties: + +- interrupt-parent : the phandle for the interrupt controller that + services interrupts for that device. +- clock-frequency : CAN system clock frequency in Hz, which is normally + half of the oscillator clock frequency. If not specified, a + default value of 8000000 (8 MHz) is used. +- cdr-reg : value of the SJA1000 clock divider register according to + the SJA1000 data sheet. If not specified, a default value of + 0x48 is used. +- ocr-reg : value of the SJA1000 output control register according to + the SJA1000 data sheet. If not specified, a default value of + 0x0a is used. + +Examples: + +can@3,100 { + compatible = "nxp,sja1000"; + reg = <3 0x100 0x80>; + clock-frequency = <8000000>; + cdr-reg = <0x48>; + ocr-reg = <0x0a>; + interrupts = <2 0>; + interrupt-parent = <&mpic>; +}; +
This patch adds a generic driver for SJA1000 chips on the OpenFirmware platform bus found on embedded PowerPC systems. You need a SJA1000 node definition in your flattened device tree source (DTS) file similar to: can@3,100 { compatible = "nxp,sja1000"; reg = <3 0x100 0x80>; clock-frequency = <8000000>; cdr-reg = <0x48>; ocr-reg = <0x0a>; interrupts = <2 0>; interrupt-parent = <&mpic>; }; See also Documentation/powerpc/dts-bindings/can/sja1000.txt. Signed-off-by: Wolfgang Grandegger <wg@grandegger.com> --- Documentation/powerpc/dts-bindings/can/sja1000.txt | 37 +++ drivers/net/can/Kconfig | 9 drivers/net/can/sja1000/Makefile | 1 drivers/net/can/sja1000/sja1000_of_platform.c | 215 +++++++++++++++++++++ 4 files changed, 262 insertions(+) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html