| Message ID | 1573642161-14189-1-git-send-email-akashast@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v5,1/3] tty: serial: qcom_geni_serial: IRQ cleanup | expand |
Please update the subject. This patch does more than remove the sysfs file. "Move loopback support to TIOCM_LOOP"? Quoting Akash Asthana (2019-11-13 02:49:21) > Remove code from the driver that create and maintain loopback sysfs node. > Instead use the ioctl TIOCMSET with TIOCM_LOOP argument to set HW to > loopback mode. > > Signed-off-by: Akash Asthana <akashast@codeaurora.org> > --- > Changes in V5: > - As per Greg's and Stephen's comment on v4 patch, removed loopback sysfs > file related code. > > drivers/tty/serial/qcom_geni_serial.c | 30 ++++-------------------------- > 1 file changed, 4 insertions(+), 26 deletions(-) > > diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c > index 56dad67..52f5ea2 100644 > --- a/drivers/tty/serial/qcom_geni_serial.c > +++ b/drivers/tty/serial/qcom_geni_serial.c > @@ -238,10 +214,14 @@ static void qcom_geni_serial_set_mctrl(struct uart_port *uport, > unsigned int mctrl) > { > u32 uart_manual_rfr = 0; > + struct qcom_geni_serial_port *port = to_dev_port(uport, uport); > > if (uart_console(uport)) > return; > > + if (mctrl & TIOCM_LOOP) > + port->loopback = MAX_LOOPBACK_CFG; How does this work? The loopback is supposed to be the max all the time and not match some qup number or something?
On 11/14/2019 11:06 PM, Stephen Boyd wrote: > Please update the subject. This patch does more than remove the sysfs > file. "Move loopback support to TIOCM_LOOP"? Ok, I will update this in next version. > Quoting Akash Asthana (2019-11-13 02:49:21) >> Remove code from the driver that create and maintain loopback sysfs node. >> Instead use the ioctl TIOCMSET with TIOCM_LOOP argument to set HW to >> loopback mode. >> >> Signed-off-by: Akash Asthana <akashast@codeaurora.org> >> --- >> Changes in V5: >> - As per Greg's and Stephen's comment on v4 patch, removed loopback sysfs >> file related code. >> >> drivers/tty/serial/qcom_geni_serial.c | 30 ++++-------------------------- >> 1 file changed, 4 insertions(+), 26 deletions(-) >> >> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c >> index 56dad67..52f5ea2 100644 >> --- a/drivers/tty/serial/qcom_geni_serial.c >> +++ b/drivers/tty/serial/qcom_geni_serial.c >> @@ -238,10 +214,14 @@ static void qcom_geni_serial_set_mctrl(struct uart_port *uport, >> unsigned int mctrl) >> { >> u32 uart_manual_rfr = 0; >> + struct qcom_geni_serial_port *port = to_dev_port(uport, uport); >> >> if (uart_console(uport)) >> return; >> >> + if (mctrl & TIOCM_LOOP) >> + port->loopback = MAX_LOOPBACK_CFG; > How does this work? The loopback is supposed to be the max all the time > and not match some qup number or something? port->loopback is unrelated to QUP number. However we have different modes to run loopback test, 1(rx-tx sorted), 2(cts-rts sorted), 3 or Max(rx-tx sorted and cts-rts sorted). In our internal testing we only use mode 3 hence we are initializing port->loopback variable to 3 and eventually it will be written to HW register from set_termios call.
Quoting Akash Asthana (2019-11-15 01:57:48) > > On 11/14/2019 11:06 PM, Stephen Boyd wrote: > > Please update the subject. This patch does more than remove the sysfs > > file. "Move loopback support to TIOCM_LOOP"? > Ok, I will update this in next version. > > Quoting Akash Asthana (2019-11-13 02:49:21) > >> Remove code from the driver that create and maintain loopback sysfs node. > >> Instead use the ioctl TIOCMSET with TIOCM_LOOP argument to set HW to > >> loopback mode. > >> > >> Signed-off-by: Akash Asthana <akashast@codeaurora.org> > >> --- > >> Changes in V5: > >> - As per Greg's and Stephen's comment on v4 patch, removed loopback sysfs > >> file related code. > >> > >> drivers/tty/serial/qcom_geni_serial.c | 30 ++++-------------------------- > >> 1 file changed, 4 insertions(+), 26 deletions(-) > >> > >> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c > >> index 56dad67..52f5ea2 100644 > >> --- a/drivers/tty/serial/qcom_geni_serial.c > >> +++ b/drivers/tty/serial/qcom_geni_serial.c > >> @@ -238,10 +214,14 @@ static void qcom_geni_serial_set_mctrl(struct uart_port *uport, > >> unsigned int mctrl) > >> { > >> u32 uart_manual_rfr = 0; > >> + struct qcom_geni_serial_port *port = to_dev_port(uport, uport); > >> > >> if (uart_console(uport)) > >> return; > >> > >> + if (mctrl & TIOCM_LOOP) > >> + port->loopback = MAX_LOOPBACK_CFG; > > How does this work? The loopback is supposed to be the max all the time > > and not match some qup number or something? > port->loopback is unrelated to QUP number. However we have different > modes to run loopback test, 1(rx-tx sorted), 2(cts-rts sorted), > > 3 or Max(rx-tx sorted and cts-rts sorted). In our internal testing we > only use mode 3 hence we are initializing port->loopback variable > > to 3 and eventually it will be written to HW register from set_termios > call. Ok. Thanks for clarifying. Can you rename this macro to RX_TX_CTS_RTS_SORTED? I imagine seeing something like: /* SE_UART_LOOPBACK_CFG */ #define RX_TX_SORTED BIT(0) #define CTS_RTS_SORTED BIT(1) #define RX_TX_CTS_RTS_SORTED RX_TX_SORTED | CTS_RTS_SORTED or whatever the bit field names really are.
On 11/16/2019 1:15 AM, Stephen Boyd wrote: > Quoting Akash Asthana (2019-11-15 01:57:48) >> On 11/14/2019 11:06 PM, Stephen Boyd wrote: >>> Please update the subject. This patch does more than remove the sysfs >>> file. "Move loopback support to TIOCM_LOOP"? >> Ok, I will update this in next version. >>> Quoting Akash Asthana (2019-11-13 02:49:21) >>>> Remove code from the driver that create and maintain loopback sysfs node. >>>> Instead use the ioctl TIOCMSET with TIOCM_LOOP argument to set HW to >>>> loopback mode. >>>> >>>> Signed-off-by: Akash Asthana <akashast@codeaurora.org> >>>> --- >>>> Changes in V5: >>>> - As per Greg's and Stephen's comment on v4 patch, removed loopback sysfs >>>> file related code. >>>> >>>> drivers/tty/serial/qcom_geni_serial.c | 30 ++++-------------------------- >>>> 1 file changed, 4 insertions(+), 26 deletions(-) >>>> >>>> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c >>>> index 56dad67..52f5ea2 100644 >>>> --- a/drivers/tty/serial/qcom_geni_serial.c >>>> +++ b/drivers/tty/serial/qcom_geni_serial.c >>>> @@ -238,10 +214,14 @@ static void qcom_geni_serial_set_mctrl(struct uart_port *uport, >>>> unsigned int mctrl) >>>> { >>>> u32 uart_manual_rfr = 0; >>>> + struct qcom_geni_serial_port *port = to_dev_port(uport, uport); >>>> >>>> if (uart_console(uport)) >>>> return; >>>> >>>> + if (mctrl & TIOCM_LOOP) >>>> + port->loopback = MAX_LOOPBACK_CFG; >>> How does this work? The loopback is supposed to be the max all the time >>> and not match some qup number or something? >> port->loopback is unrelated to QUP number. However we have different >> modes to run loopback test, 1(rx-tx sorted), 2(cts-rts sorted), >> >> 3 or Max(rx-tx sorted and cts-rts sorted). In our internal testing we >> only use mode 3 hence we are initializing port->loopback variable >> >> to 3 and eventually it will be written to HW register from set_termios >> call. > Ok. Thanks for clarifying. Can you rename this macro to > RX_TX_CTS_RTS_SORTED? I imagine seeing something like: > > /* SE_UART_LOOPBACK_CFG */ > #define RX_TX_SORTED BIT(0) > #define CTS_RTS_SORTED BIT(1) > #define RX_TX_CTS_RTS_SORTED RX_TX_SORTED | CTS_RTS_SORTED > > or whatever the bit field names really are. Ok, I will update this is v6.
diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c index 56dad67..52f5ea2 100644 --- a/drivers/tty/serial/qcom_geni_serial.c +++ b/drivers/tty/serial/qcom_geni_serial.c @@ -165,30 +165,6 @@ static struct qcom_geni_serial_port qcom_geni_uart_ports[GENI_UART_PORTS] = { }, }; -static ssize_t loopback_show(struct device *dev, - struct device_attribute *attr, char *buf) -{ - struct qcom_geni_serial_port *port = dev_get_drvdata(dev); - - return snprintf(buf, sizeof(u32), "%d\n", port->loopback); -} - -static ssize_t loopback_store(struct device *dev, - struct device_attribute *attr, const char *buf, - size_t size) -{ - struct qcom_geni_serial_port *port = dev_get_drvdata(dev); - u32 loopback; - - if (kstrtoint(buf, 0, &loopback) || loopback > MAX_LOOPBACK_CFG) { - dev_err(dev, "Invalid input\n"); - return -EINVAL; - } - port->loopback = loopback; - return size; -} -static DEVICE_ATTR_RW(loopback); - static struct qcom_geni_serial_port qcom_geni_console_port = { .uport = { .iotype = UPIO_MEM, @@ -238,10 +214,14 @@ static void qcom_geni_serial_set_mctrl(struct uart_port *uport, unsigned int mctrl) { u32 uart_manual_rfr = 0; + struct qcom_geni_serial_port *port = to_dev_port(uport, uport); if (uart_console(uport)) return; + if (mctrl & TIOCM_LOOP) + port->loopback = MAX_LOOPBACK_CFG; + if (!(mctrl & TIOCM_RTS)) uart_manual_rfr = UART_MANUAL_RFR_EN | UART_RFR_NOT_READY; writel(uart_manual_rfr, uport->membase + SE_UART_MANUAL_RFR); @@ -1311,8 +1291,6 @@ static int qcom_geni_serial_probe(struct platform_device *pdev) uport->private_data = drv; platform_set_drvdata(pdev, port); port->handle_rx = console ? handle_rx_console : handle_rx_uart; - if (!console) - device_create_file(uport->dev, &dev_attr_loopback); ret = uart_add_one_port(drv, uport); if (ret)
Remove code from the driver that create and maintain loopback sysfs node. Instead use the ioctl TIOCMSET with TIOCM_LOOP argument to set HW to loopback mode. Signed-off-by: Akash Asthana <akashast@codeaurora.org> --- Changes in V5: - As per Greg's and Stephen's comment on v4 patch, removed loopback sysfs file related code. drivers/tty/serial/qcom_geni_serial.c | 30 ++++-------------------------- 1 file changed, 4 insertions(+), 26 deletions(-)