| Message ID | 1d8ba1dea24da541389e8175097aa4f67eddc298.1494660546.git.jan.kiszka@siemens.com |
|---|---|
| State | New |
| Headers | show |
On Sat, May 13, 2017 at 10:29 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote: > This implements the setup of RS232 and the switch-over to RS485 or RS422 > for the Siemens IOT2040. That uses an EXAR XR17V352 with external logic > to switch between the different modes. The external logic is controlled > via MPIO pins of the EXAR controller. > > Only pin 10 can be exported as GPIO on the IOT2040. It is connected to > an LED. > > As the XR17V352 used on the IOT2040 is not equipped with an external > EEPROM, it cannot present itself as IOT2040-variant via subvendor/ > subdevice IDs. Thus, we have to check via DMI for the target platform. > > Co-developed with Sascha Weisenberger. > Please, refactor that using properly formed DMI structrure and use its callback and driver data facilities/ stmmac's pattern that you obviously applied does not actually suit here.
On 2017-05-13 15:54, Andy Shevchenko wrote: > On Sat, May 13, 2017 at 10:29 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote: >> This implements the setup of RS232 and the switch-over to RS485 or RS422 >> for the Siemens IOT2040. That uses an EXAR XR17V352 with external logic >> to switch between the different modes. The external logic is controlled >> via MPIO pins of the EXAR controller. >> >> Only pin 10 can be exported as GPIO on the IOT2040. It is connected to >> an LED. >> >> As the XR17V352 used on the IOT2040 is not equipped with an external >> EEPROM, it cannot present itself as IOT2040-variant via subvendor/ >> subdevice IDs. Thus, we have to check via DMI for the target platform. >> >> Co-developed with Sascha Weisenberger. >> > > Please, refactor that using properly formed DMI structrure and use its > callback and driver data facilities/ Could you point to a specific example? The callback of the dmi_system_id structure is not designed to handle device initializations like this one - not to speak of having those two different initialization points here. Thanks, Jan
On Thu, May 18, 2017 at 8:06 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote: > On 2017-05-13 15:54, Andy Shevchenko wrote: >> On Sat, May 13, 2017 at 10:29 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote: >>> This implements the setup of RS232 and the switch-over to RS485 or RS422 >>> for the Siemens IOT2040. That uses an EXAR XR17V352 with external logic >>> to switch between the different modes. The external logic is controlled >>> via MPIO pins of the EXAR controller. >>> >>> Only pin 10 can be exported as GPIO on the IOT2040. It is connected to >>> an LED. >>> >>> As the XR17V352 used on the IOT2040 is not equipped with an external >>> EEPROM, it cannot present itself as IOT2040-variant via subvendor/ >>> subdevice IDs. Thus, we have to check via DMI for the target platform. >>> >>> Co-developed with Sascha Weisenberger. >>> >> >> Please, refactor that using properly formed DMI structrure and use its >> callback and driver data facilities/ > > Could you point to a specific example? The callback of the dmi_system_id > structure is not designed to handle device initializations like this one > - not to speak of having those two different initialization points here. Sure. drivers/platform/x86/dell-laptop.c: static const struct dmi_system_id dell_quirks[] __initconst = { { .callback = dmi_matched, .ident = "Dell Vostro V130", .matches = { DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."), DMI_MATCH(DMI_PRODUCT_NAME, "Vostro V130"), }, .driver_data = &quirk_dell_vostro_v130, }, (just in case, dmi_matched() is part of that module)
On 2017-05-18 12:17, Andy Shevchenko wrote: > On Thu, May 18, 2017 at 8:06 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote: >> On 2017-05-13 15:54, Andy Shevchenko wrote: >>> On Sat, May 13, 2017 at 10:29 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote: >>>> This implements the setup of RS232 and the switch-over to RS485 or RS422 >>>> for the Siemens IOT2040. That uses an EXAR XR17V352 with external logic >>>> to switch between the different modes. The external logic is controlled >>>> via MPIO pins of the EXAR controller. >>>> >>>> Only pin 10 can be exported as GPIO on the IOT2040. It is connected to >>>> an LED. >>>> >>>> As the XR17V352 used on the IOT2040 is not equipped with an external >>>> EEPROM, it cannot present itself as IOT2040-variant via subvendor/ >>>> subdevice IDs. Thus, we have to check via DMI for the target platform. >>>> >>>> Co-developed with Sascha Weisenberger. >>>> >>> >>> Please, refactor that using properly formed DMI structrure and use its >>> callback and driver data facilities/ >> >> Could you point to a specific example? The callback of the dmi_system_id >> structure is not designed to handle device initializations like this one >> - not to speak of having those two different initialization points here. > > Sure. > > drivers/platform/x86/dell-laptop.c: > > static const struct dmi_system_id dell_quirks[] __initconst = { > { > .callback = dmi_matched, > .ident = "Dell Vostro V130", > .matches = { > DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."), > DMI_MATCH(DMI_PRODUCT_NAME, "Vostro V130"), > }, > .driver_data = &quirk_dell_vostro_v130, > }, > > > (just in case, dmi_matched() is part of that module) > OK, but that would basically replace the two lines for matching against the DMI values with the structure above. We would still need the existing hooks at the two points where we have them right now. Doesn't look to me as if the code would become clearer or better separated or easier extensible (the latter being hard anyway without knowing any potential third special case). Jan
On Thu, May 18, 2017 at 1:37 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote: > On 2017-05-18 12:17, Andy Shevchenko wrote: >> On Thu, May 18, 2017 at 8:06 AM, Jan Kiszka <jan.kiszka@siemens.com> wrote: >>> On 2017-05-13 15:54, Andy Shevchenko wrote: >>>> Please, refactor that using properly formed DMI structrure and use its >>>> callback and driver data facilities/ >>> >>> Could you point to a specific example? The callback of the dmi_system_id >>> structure is not designed to handle device initializations like this one >>> - not to speak of having those two different initialization points here. >> >> Sure. >> >> drivers/platform/x86/dell-laptop.c: >> >> static const struct dmi_system_id dell_quirks[] __initconst = { >> { >> .callback = dmi_matched, >> .ident = "Dell Vostro V130", >> .matches = { >> DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."), >> DMI_MATCH(DMI_PRODUCT_NAME, "Vostro V130"), >> }, >> .driver_data = &quirk_dell_vostro_v130, >> }, >> >> >> (just in case, dmi_matched() is part of that module) >> > > OK, but that would basically replace the two lines for matching against > the DMI values with the structure above. We would still need the > existing hooks at the two points where we have them right now. Right. > Doesn't > look to me as if the code would become clearer or better separated or > easier extensible (the latter being hard anyway without knowing any > potential third special case). stmmac, that you chose as an example, should go that way in the first place because it was my though behind which is on-to-one to yours above.
diff --git a/drivers/tty/serial/8250/8250_exar.c b/drivers/tty/serial/8250/8250_exar.c index 8e9c0e9495f5..62fe0f1ddcfe 100644 --- a/drivers/tty/serial/8250/8250_exar.c +++ b/drivers/tty/serial/8250/8250_exar.c @@ -19,6 +19,7 @@ #include <linux/string.h> #include <linux/tty.h> #include <linux/8250_pci.h> +#include <linux/dmi.h> #include <linux/property.h> #include <asm/byteorder.h> @@ -61,6 +62,43 @@ #define UART_EXAR_MPIOSEL_15_8 0x99 /* MPIOSEL[15:8] */ #define UART_EXAR_MPIOOD_15_8 0x9a /* MPIOOD[15:8] */ +#define UART_EXAR_RS485_DLY(x) (x << 4) + +/* + * IOT2040 MPIO wiring semantics: + * + * MPIO Port Function + * ---- ---- -------- + * 0 2 Mode bit 0 + * 1 2 Mode bit 1 + * 2 2 Terminate bus + * 3 - <reserved> + * 4 3 Mode bit 0 + * 5 3 Mode bit 1 + * 6 3 Terminate bus + * 7 - <reserved> + * 8 2 Enable + * 9 3 Enable + * 10 - Red LED + * 11..15 - <unused> + */ + +/* IOT2040 MPIOs 0..7 */ +#define IOT2040_UART_MODE_RS232 0x01 +#define IOT2040_UART_MODE_RS485 0x02 +#define IOT2040_UART_MODE_RS422 0x03 +#define IOT2040_UART_TERMINATE_BUS 0x04 + +#define IOT2040_UART1_MASK 0x0f +#define IOT2040_UART2_SHIFT 4 + +#define IOT2040_UARTS_DEFAULT_MODE 0x11 /* both RS232 */ +#define IOT2040_UARTS_GPIO_LO_MODE 0x88 /* reserved pins as input */ + +/* IOT2040 MPIOs 8..15 */ +#define IOT2040_UARTS_ENABLE 0x03 +#define IOT2040_UARTS_GPIO_HI_MODE 0xF8 /* enable & LED as outputs */ + struct exar8250; /** @@ -217,6 +255,65 @@ xr17v35x_register_gpio(struct pci_dev *pcidev, unsigned int first_gpio, return pdev; } +static int iot2040_rs485_config(struct uart_port *port, + struct serial_rs485 *rs485) +{ + u8 __iomem *p = port->membase; + u8 mask = IOT2040_UART1_MASK; + u8 mode, value; + bool is_rs485 = false; + + if (rs485->flags & SER_RS485_ENABLED) { + is_rs485 = true; + if (rs485->flags & SER_RS485_RX_DURING_TX) + mode = IOT2040_UART_MODE_RS422; + else + mode = IOT2040_UART_MODE_RS485; + + if (rs485->flags & SER_RS485_TERMINATE_BUS) + mode |= IOT2040_UART_TERMINATE_BUS; + } else { + mode = IOT2040_UART_MODE_RS232; + } + + if (port->line == 3) { + mask <<= IOT2040_UART2_SHIFT; + mode <<= IOT2040_UART2_SHIFT; + } + + value = readb(p + UART_EXAR_MPIOLVL_7_0); + value &= ~mask; + value |= mode; + writeb(value, p + UART_EXAR_MPIOLVL_7_0); + + value = readb(p + UART_EXAR_FCTR); + if (is_rs485) + value |= UART_FCTR_EXAR_485; + else + value &= ~UART_FCTR_EXAR_485; + writeb(value, p + UART_EXAR_FCTR); + + if (is_rs485) + writeb(UART_EXAR_RS485_DLY(4), p + UART_MSR); + + return 0; +} + +static int iot2000_setup_gpio(struct pci_dev *pcidev, + struct uart_8250_port *port) +{ + u8 __iomem *p = port->port.membase; + + writeb(IOT2040_UARTS_DEFAULT_MODE, p + UART_EXAR_MPIOLVL_7_0); + writeb(IOT2040_UARTS_GPIO_LO_MODE, p + UART_EXAR_MPIOSEL_7_0); + writeb(IOT2040_UARTS_ENABLE, p + UART_EXAR_MPIOLVL_15_8); + writeb(IOT2040_UARTS_GPIO_HI_MODE, p + UART_EXAR_MPIOSEL_15_8); + + port->port.private_data = xr17v35x_register_gpio(pcidev, 10, 1); + + return 0; +} + static int pci_xr17v35x_setup(struct exar8250 *priv, struct pci_dev *pcidev, struct uart_8250_port *port, int idx) @@ -224,10 +321,20 @@ pci_xr17v35x_setup(struct exar8250 *priv, struct pci_dev *pcidev, const struct exar8250_board *board = priv->board; unsigned int offset = idx * 0x400; unsigned int baud = 7812500; + bool is_iot2040; u8 __iomem *p; int ret; port->port.uartclk = baud * 16; + + is_iot2040 = + strcmp(dmi_get_system_info(DMI_BOARD_NAME), + "SIMATIC IOT2000") == 0 && + strcmp(dmi_get_system_info(DMI_BOARD_ASSET_TAG), + "6ES7647-0AA00-1YA2") == 0; + if (is_iot2040) + port->port.rs485_config = iot2040_rs485_config; + /* * Setup the uart clock for the devices on expansion slot to * half the clock speed of the main chip (which is 125MHz) @@ -246,14 +353,18 @@ pci_xr17v35x_setup(struct exar8250 *priv, struct pci_dev *pcidev, writeb(128, p + UART_EXAR_TXTRG); writeb(128, p + UART_EXAR_RXTRG); - if (idx == 0) { - /* Setup Multipurpose Input/Output pins. */ - setup_gpio(p); + if (idx != 0) + return 0; + + /* Setup Multipurpose Input/Output pins. */ + setup_gpio(p); + if (is_iot2040) + ret = iot2000_setup_gpio(pcidev, port); + else port->port.private_data = xr17v35x_register_gpio(pcidev, 0, 16); - } - return 0; + return ret; } static void pci_xr17v35x_exit(struct pci_dev *pcidev)