| Message ID | 20171124183509.12810-2-kernel@martin.sperl.org |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [1/2] dt-binding: can: mcp2517fd: document device tree bindings | expand |
On Fri, Nov 24, 2017 at 06:35:08PM +0000, kernel@martin.sperl.org wrote: > From: Martin Sperl <kernel@martin.sperl.org> > > Add device-tree bindings for Microcip CanFD Controller mcp2517fd s/Microcip/Microchip/ > > Signed-off-by: Martin Sperl <kernel@martin.sperl.org> > --- > .../bindings/net/can/microchip,mcp2517fd.txt | 47 ++++++++++++++++++++++ > 1 file changed, 47 insertions(+) > create mode 100644 Documentation/devicetree/bindings/net/can/microchip,mcp2517fd.txt > > diff --git a/Documentation/devicetree/bindings/net/can/microchip,mcp2517fd.txt b/Documentation/devicetree/bindings/net/can/microchip,mcp2517fd.txt > new file mode 100644 > index 000000000000..96cbf0c96895 > --- /dev/null > +++ b/Documentation/devicetree/bindings/net/can/microchip,mcp2517fd.txt > @@ -0,0 +1,47 @@ > +* Microchip MCP2517 stand-alone CAN controller device tree bindings > + > +Required properties: > + - compatible: Should be one of the following: > + - "microchip,mcp2517fd" for MCP2517fd. > + - reg: SPI chip select. > + - clocks: The clock feeding the CAN controller. > + - interrupt-parent: The parent interrupt controller. > + - interrupts: Should contain IRQ line for the CAN controller. > + > +Optional properties: > + - vdd-supply: Regulator that powers the CAN controller. > + - xceiver-supply: Regulator that powers the CAN transceiver. > + - microchip,clock_out_div = <0|1|2|4|10>: Clock output pin divider s/_/-/ And on the rest. Don't use underscores... > + 0 = Start of Frame output > + default: 10 > + - microchip,clock_div = <1|2>: internal clock divider - default 1 > + - microchip,gpio_opendrain: gpio (int0,1) in open drain mode > + instead of default push/pull > + - microchip,int_opendrain: int pin in open drain mode > + instead of default push/pull IIRC, we have a standard property for this. > + - microchip,txcan_opendrain: txcan pin in open drain mode > + instead of default push/pull > + - microchip,gpio0_mode : gpio mode functionality > + 0 = input > + 1 = TX interrupt output - default > + 2 = output default low > + 3 = output default high > + 4 = (tx) transceiver standby > + - microchip,gpio1_mode : gpio mode functionality > + 0 = input - default > + 1 = RX interrupt output - default > + 2 = output default low > + 3 = output default high > + > +Example: > + can0: can@1 { > + compatible = "microchip,mcp2515"; > + reg = <1>; > + clocks = <&clk24m>; > + interrupt-parent = <&gpio4>; > + interrupts = <13 0x8>; > + vdd-supply = <®5v0>; > + xceiver-supply = <®5v0>; > + microchip,gpio0_mode = <4>; > + microchip,gpio0_mode = <1>; > + }; > -- > 2.11.0 > -- 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
Hi Rob! Thanks for all the effort - I shall incorporate those changes into V2. > s/Microcip/Microchip/ > s/_/-/ > >> + 0 = Start of Frame output >> + default: 10 >> + - microchip,clock_div = <1|2>: internal clock divider - default 1 >> + - microchip,gpio_opendrain: gpio (int0,1) in open drain mode >> + instead of default push/pull >> + - microchip,int_opendrain: int pin in open drain mode >> + instead of default push/pull > > IIRC, we have a standard property for this. Would you know what that could be - I did a quick search for standard properties, but could not find a definitive list… The only thing I found in: Documentation/devicetree/bindings/w1/w1-gpio.txt is: linux,open-drain But there is also: nvidia,open-drain open_drain open-drain drive-open-drain (pinctrl) st,irq-open-drain Only the last is specific to the interrupt line and none to the other GPIOs of the chip (txcan, GPIO) - and each can get set differently. So how shall I implement that? Thanks, Martin-- 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
Hello Martin, I didn't catch the whole discussion but you may want to check include/dt-bindings/gpio/gpio.h > #define GPIO_OPEN_DRAIN (GPIO_SINGLE_ENDED | GPIO_ACTIVE_LOW) This corresponds to Documentation/devicetree/bindings/gpio/gpio.txt > Most controllers are however specifying a generic flag bitfield > in the last cell, so for these, use the macros defined in > include/dt-bindings/gpio/gpio.h whenever possible: > > Example of a node using GPIOs: > > node { > enable-gpios = <&qe_pio_e 18 GPIO_ACTIVE_HIGH>; > }; > > GPIO_ACTIVE_HIGH is 0, so in this example gpio-specifier is "18 0" and > encodes > GPIO pin number, and GPIO flags as accepted by the "qe_pio_e" > gpio-controller. > > Optional standard bitfield specifiers for the last cell: > > - Bit 0: 0 means active high, 1 means active low > - Bit 1: 1 means single-ended wiring, see: > https://en.wikipedia.org/wiki/Single-ended_triode > When used with active-low, this means open drain/collector, see: > https://en.wikipedia.org/wiki/Open_collector > When used with active-high, this means open source/emitter Regards, Patrick Am 29.11.2017 um 12:55 schrieb kernel@martin.sperl.org: > Hi Rob! > > Thanks for all the effort - I shall incorporate those changes into V2. > >> s/Microcip/Microchip/ >> s/_/-/ >> >>> + 0 = Start of Frame output >>> + default: 10 >>> + - microchip,clock_div = <1|2>: internal clock divider - default 1 >>> + - microchip,gpio_opendrain: gpio (int0,1) in open drain mode >>> + instead of default push/pull >>> + - microchip,int_opendrain: int pin in open drain mode >>> + instead of default push/pull >> IIRC, we have a standard property for this. > Would you know what that could be - I did a quick search for > standard properties, but could not find a definitive list… > > The only thing I found in: > Documentation/devicetree/bindings/w1/w1-gpio.txt > is: > linux,open-drain > > But there is also: > nvidia,open-drain > open_drain > open-drain > drive-open-drain (pinctrl) > st,irq-open-drain > > Only the last is specific to the interrupt line and none to the other > GPIOs of the chip (txcan, GPIO) - and each can get set differently. > > So how shall I implement that? > > Thanks, > Martin-- > To unsubscribe from this list: send the line "unsubscribe linux-can" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Patrick! > On 29.11.2017, at 21:35, Patrick Menschel <menschel.p@posteo.de> wrote: > > Hello Martin, > > > I didn't catch the whole discussion but you may want to check > > include/dt-bindings/gpio/gpio.h > >> #define GPIO_OPEN_DRAIN (GPIO_SINGLE_ENDED | GPIO_ACTIVE_LOW) > > This corresponds to > > Documentation/devicetree/bindings/gpio/gpio.txt I understand, but the question still is: how to present the information in a valid way. To use gpio propperly it would require that the driver implements a “sub-driver” pinctrl with all the extra (boilerplate) code overhead. Also this would mean mixing different types of logical drivers into a single source - I doubt that would be easy to get accepted... Here again a summary of all the GPIOs that the mcp2517fd has: * TXCAN: dedicated GPIO with single function, individually conigurable as push/pull or open drain * INT: main interrupt line - configurable as push/pull or individually conigurable as push/pull or open drain * GPIO0: general GPIO with in/out option, but 2 special “cases”: tx-irq and TX-disable group configurable as push/pull or open drain * GPIO1: general GPIO with in/out potion, but 1 special “cases”: rx-irq group configurable as push/pull or open drain * CLKO/SOF: clock output at (1/10th, 1/5th, 1/2, 1 of the core frequency) or start of frame output possibly group configurable as push/pull or open drain (not explicitly specified in datasheet) How would you try to present that HW-configuration in the device tree instead? How would it impact the driver design? Thanks, Martin -- 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
Am 30.11.2017 um 08:24 schrieb kernel@martin.sperl.org: > I understand, but the question still is: how to > present the information in a valid way. > > To use gpio propperly it would require that the driver > implements a “sub-driver” pinctrl with all the extra > (boilerplate) code overhead. > > Also this would mean mixing different types of > logical drivers into a single source - I doubt that > would be easy to get accepted... > > Here again a summary of all the GPIOs that the mcp2517fd has: > * TXCAN: dedicated GPIO with single function, > individually conigurable as push/pull or open drain > * INT: main interrupt line - configurable as push/pull or > individually conigurable as push/pull or open drain > * GPIO0: general GPIO with in/out option, but 2 special “cases”: > tx-irq and TX-disable > group configurable as push/pull or open drain > * GPIO1: general GPIO with in/out potion, but 1 special “cases”: > rx-irq > group configurable as push/pull or open drain > * CLKO/SOF: clock output at (1/10th, 1/5th, 1/2, 1 of the > core frequency) or start of frame output > possibly group configurable as push/pull or open drain > (not explicitly specified in datasheet) > > How would you try to present that HW-configuration in the > device tree instead? > How would it impact the driver design? > Hi, I'm afraid I don't know what is best practice but you may want to look at the max310x driver which declares it's GPIOs and GPIO based interrupts in the regular driver. drivers/tty/serial/max310x.c Documentation/devicetree/bindings/serial/maxim,max310x.txt Look for "#ifdef CONFIG_GPIOLIB". My first try would be a single dt node like the max310x uses in the dt example. Imho it is better to make things useful before making them complicated. Regards, Patrick
Hi Patrick! > On 30.11.2017, at 17:44, Patrick Menschel <menschel.p@posteo.de> wrote: >> How would you try to present that HW-configuration in the >> device tree instead? >> How would it impact the driver design? >> > Hi, > I'm afraid I don't know what is best practice but you may want to look > at the max310x driver which declares it's GPIOs and GPIO based > interrupts in the regular driver. > > drivers/tty/serial/max310x.c > Documentation/devicetree/bindings/serial/maxim,max310x.txt > Look for "#ifdef CONFIG_GPIOLIB”. This is a gpio-controller, for which this is what I would implement. The problem comes more from the fact that the mcp2517fd is primarily a CAN controller, which has a few GPIO pins. So implementing a gpio-controller for those (rarely used) GPIOs in the same driver seems a bit of an overkill. The mcp2515 chip also supports 5 GPIO pins, but the driver does not really make use of them, so they are left out. The problem here is more the fact that the mcp2517fd supports also push-pull/open-drain on some of those gpios. And at least openDrain may be required on TXCan if used on a network without a transceiver… > My first try would be a single dt node like the max310x uses in the dt > example. > Imho it is better to make things useful before making them complicated. The settings as they are now are the “simple” version. implementing a separate GPIO-controller driver just to implement the same logic we have now would make it a much bigger driver without lots of extra benefits. So I hope the current proposal is ok... Martin-- 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
Am 30.11.2017 um 17:58 schrieb kernel@martin.sperl.org: > >> drivers/tty/serial/max310x.c >> Documentation/devicetree/bindings/serial/maxim,max310x.txt >> Look for "#ifdef CONFIG_GPIOLIB”. > This is a gpio-controller, for which this is what I would implement. > Hi, you are mistaken about that, max310x chips are spi to uart bridges with strong ties to rs485 which is very similar to CAN except for the missing arbitration in hardware. Max14830 has GPIOs with clocking support, so this chip's driver also fused the main function with secondary gpio functions. https://datasheets.maximintegrated.com/en/ds/MAX14830.pdf Anyway I'm curious what is best practice for this sort of configuration. Regards, Patrick
Hi Patrick! I had a quick look starting to implement gpiolib, but I believe I would need to implement pin-ctrl on top to make the Alternate functions work propperly. I wonder if this effort is really worth it. Martin On 11/30/2017 06:49 PM, Patrick Menschel wrote: > Am 30.11.2017 um 17:58 schriebkernel@martin.sperl.org: >>> drivers/tty/serial/max310x.c >>> Documentation/devicetree/bindings/serial/maxim,max310x.txt >>> Look for "#ifdef CONFIG_GPIOLIB”. >> This is a gpio-controller, for which this is what I would implement. >> > Hi, > > you are mistaken about that, max310x chips are spi to uart bridges with > strong ties to rs485 which is very similar to CAN except for the missing > arbitration in hardware. Max14830 has GPIOs with clocking support, so > this chip's driver also fused the main function with secondary gpio > functions. > https://datasheets.maximintegrated.com/en/ds/MAX14830.pdf > > Anyway I'm curious what is best practice for this sort of configuration. > > Regards, > Patrick > -- 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
Hi Patrick! So I started implementing the GPIO_LIB implementation but I have hit an issue where I would like to get some feedback. So in principle the gpiolib works, but only if the device is not asleep and the clock is enabled, which at this very moment means that the can interface has to be up and running. So at this very moment the only option that I see would be to disable the sleep mode which the device enters after probing until the can-network interface is enabled (which enables the clock and start the oscillator) - with SLEEP enabled when GPIOLIB support is disabled. This is obviously not optimal from the power perspective… The other option would be starting the clock and oscillator as soon as a set_direction* (or reques/free) function is called. (where I would need to start using locks to avoid race conditions as well as clock usage counters...). Which of the above is the preferred solution? Are there other ideas how I could solve this dilemma? Martin > On 05.12.2017, at 19:28, Patrick Menschel <menschel.p@posteo.de> wrote: > > Hi Martin, > > it depends on the daily usage of those alternate functions. > I just took a quick glimpse on the datasheet and if you check appendix B table 9-1 , you'll see that some functions are optional because ISO suggested them, not because there is a practical reason for it. > > CLKO/SOF does make little to no sense with a higher grade embedded system. CLKO is meant to share the osc with the microcontroller. >> The CLKO pin provides the clock to the >> microcontroller. > SOF makes sense for testing in lab configuration, not for regular operation. > > TXCAN also does make little sense as you can only use it in the lab. >> TXCANOD: TXCAN can be configured as Push- >> Pull or as Open Drain out >> put. Open Drain outputs >> allows the user to connect multiple controllers >> together to build a CAN network without using a >> transceiver. > > INTOD is initialized as Push/Pull. Configuring it open drain would be processor specific when the microcontroller has limited options for pin-ctrl, e.g. some PICs. >> INTOD: >> Interrupt pins Open Drain Mode >> 1 >> = Open Drain Output >> 0 >> = Push/Pull Output > > To sum it up, I would drop: > - CLKO/SOF > - TXCAN > - INTOD > > I would implement higher level functions in a second step for INT0/GPIO0/XSTBY and INT1/GPIO1. > Such functions can be: > - power saving via XSTBY > - RX / TX LEDs via INT0 / INT1 > > Actually this second step would wait until a request for feature arises. > > Regards, > Patrick > > Am 05.12.2017 um 11:26 schrieb Martin Sperl: >> Hi Patrick! >> >> >> I had a quick look starting to implement gpiolib, >> >> but I believe I would need to implement pin-ctrl on top to >> >> make the Alternate functions work propperly. >> >> I wonder if this effort is really worth it. >> >> Martin >> -- 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
Hi Martin, as I wrote earlier, I would save the GPIO_LIB part for a driver update later on as to me there seems to be no daily use case. I think SLEEP is the preferred solution. If the device is in SLEEP after probing, the logical step would be to disable GPIO functions and configure INT0/GPIO0/XSTBY to XSTBY thus the transceiver is also sleeping. Best practice usually comes from practical usage. If there already is a reference design PCB with the mcp2517fd, it would make sense to see what the designer did connect to the optional GPIO pins and then guess what additional use cases could be of interest. Without that knowledge, you're prone to do over-engineering. Regards, Patrick Am 17.12.2017 um 15:34 schrieb kernel@martin.sperl.org: > Hi Patrick! > > So I started implementing the GPIO_LIB implementation but > I have hit an issue where I would like to get some feedback. > > So in principle the gpiolib works, but only if the device is > not asleep and the clock is enabled, which at this very moment means > that the can interface has to be up and running. > > So at this very moment the only option that I see would be to > disable the sleep mode which the device enters after probing until > the can-network interface is enabled (which enables the clock and > start the oscillator) - with SLEEP enabled when GPIOLIB support > is disabled. > > This is obviously not optimal from the power perspective… > > The other option would be starting the clock and oscillator > as soon as a set_direction* (or reques/free) function is called. > (where I would need to start using locks to avoid race conditions > as well as clock usage counters...). > > Which of the above is the preferred solution? Are there other ideas > how I could solve this dilemma? > > > Martin >
diff --git a/Documentation/devicetree/bindings/net/can/microchip,mcp2517fd.txt b/Documentation/devicetree/bindings/net/can/microchip,mcp2517fd.txt new file mode 100644 index 000000000000..96cbf0c96895 --- /dev/null +++ b/Documentation/devicetree/bindings/net/can/microchip,mcp2517fd.txt @@ -0,0 +1,47 @@ +* Microchip MCP2517 stand-alone CAN controller device tree bindings + +Required properties: + - compatible: Should be one of the following: + - "microchip,mcp2517fd" for MCP2517fd. + - reg: SPI chip select. + - clocks: The clock feeding the CAN controller. + - interrupt-parent: The parent interrupt controller. + - interrupts: Should contain IRQ line for the CAN controller. + +Optional properties: + - vdd-supply: Regulator that powers the CAN controller. + - xceiver-supply: Regulator that powers the CAN transceiver. + - microchip,clock_out_div = <0|1|2|4|10>: Clock output pin divider + 0 = Start of Frame output + default: 10 + - microchip,clock_div = <1|2>: internal clock divider - default 1 + - microchip,gpio_opendrain: gpio (int0,1) in open drain mode + instead of default push/pull + - microchip,int_opendrain: int pin in open drain mode + instead of default push/pull + - microchip,txcan_opendrain: txcan pin in open drain mode + instead of default push/pull + - microchip,gpio0_mode : gpio mode functionality + 0 = input + 1 = TX interrupt output - default + 2 = output default low + 3 = output default high + 4 = (tx) transceiver standby + - microchip,gpio1_mode : gpio mode functionality + 0 = input - default + 1 = RX interrupt output - default + 2 = output default low + 3 = output default high + +Example: + can0: can@1 { + compatible = "microchip,mcp2515"; + reg = <1>; + clocks = <&clk24m>; + interrupt-parent = <&gpio4>; + interrupts = <13 0x8>; + vdd-supply = <®5v0>; + xceiver-supply = <®5v0>; + microchip,gpio0_mode = <4>; + microchip,gpio0_mode = <1>; + };