| Message ID | 1523302721-19439-9-git-send-email-kramasub@codeaurora.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Andy Gross |
| Headers | show |
On Mon 09 Apr 12:38 PDT 2018, Karthikeyan Ramasubramanian wrote: > Add early console support in Qualcomm Technologies Inc., GENI based > UART controller. > Hi Karthikeyan, I pulled this into my SDM845 tree and added "earlycon" to my command line, the result is a working console up until the serial driver is probed at which point the console stops. I'm expecting that this should hand over to the normal console as it is probed, unless I specify keep_bootcon in which case I expect all log entries to be printed both through the earlycon and the normal console (i.e. show up twice on the uart). > Signed-off-by: Karthikeyan Ramasubramanian <kramasub@codeaurora.org> > Signed-off-by: Girish Mahadevan <girishm@codeaurora.org> This is a log of how the patch got to the list (and later to mainline), as such your name should be the last entry in this line and you should add an extra Co-Developed-by to indicate that you worked on this together with Girish. Regards, Bjorn > --- > drivers/tty/serial/qcom_geni_serial.c | 85 ++++++++++++++++++++++++++++++++++- > 1 file changed, 83 insertions(+), 2 deletions(-) > > diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c > index e40d4a4..2ce83a57 100644 > --- a/drivers/tty/serial/qcom_geni_serial.c > +++ b/drivers/tty/serial/qcom_geni_serial.c > @@ -196,8 +196,18 @@ static bool qcom_geni_serial_poll_bit(struct uart_port *uport, > timeout_us = ((fifo_bits * USEC_PER_SEC) / baud) + 500; > } > > - return !readl_poll_timeout_atomic(uport->membase + offset, reg, > - (bool)(reg & field) == set, 10, timeout_us); > + /* > + * Use custom implementation instead of readl_poll_atomic since ktimer > + * is not ready at the time of early console. > + */ > + while (timeout_us) { > + reg = readl_relaxed(uport->membase + offset); > + if ((bool)(reg & field) == set) > + return true; > + udelay(10); > + timeout_us -= 10; > + } > + return false; > } > > static void qcom_geni_serial_setup_tx(struct uart_port *uport, u32 xmit_size) > @@ -944,6 +954,77 @@ static int __init qcom_geni_console_setup(struct console *co, char *options) > return uart_set_options(uport, co, baud, parity, bits, flow); > } > > +static void qcom_geni_serial_earlycon_write(struct console *con, > + const char *s, unsigned int n) > +{ > + struct earlycon_device *dev = con->data; > + > + __qcom_geni_serial_console_write(&dev->port, s, n); > +} > + > +static int __init qcom_geni_serial_earlycon_setup(struct earlycon_device *dev, > + const char *opt) > +{ > + struct uart_port *uport = &dev->port; > + u32 tx_trans_cfg; > + u32 tx_parity_cfg = 0; /* Disable Tx Parity */ > + u32 rx_trans_cfg = 0; > + u32 rx_parity_cfg = 0; /* Disable Rx Parity */ > + u32 stop_bit_len = 0; /* Default stop bit length - 1 bit */ > + u32 bits_per_char; > + u32 ser_clk_cfg; > + u32 baud = 115200; > + unsigned int clk_div; > + unsigned long clk_rate; > + struct geni_se se; > + > + if (!uport->membase) > + return -EINVAL; > + > + memset(&se, 0, sizeof(se)); > + se.base = uport->membase; > + if (geni_se_read_proto(&se) != GENI_SE_UART) > + return -ENXIO; > + > + /* > + * Ignore Flow control. > + * n = 8. > + */ > + tx_trans_cfg = UART_CTS_MASK; > + bits_per_char = BITS_PER_BYTE; > + > + clk_rate = get_clk_div_rate(baud, &clk_div); > + if (!clk_rate) > + return -EINVAL; > + > + ser_clk_cfg = SER_CLK_EN | (clk_div << CLK_DIV_SHFT); > + /* > + * Make an unconditional cancel on the main sequencer to reset > + * it else we could end up in data loss scenarios. > + */ > + qcom_geni_serial_poll_tx_done(uport); > + qcom_geni_serial_abort_rx(uport); > + geni_se_config_packing(&se, BITS_PER_BYTE, 1, false, true, false); > + geni_se_init(&se, DEF_FIFO_DEPTH_WORDS / 2, DEF_FIFO_DEPTH_WORDS - 2); > + geni_se_select_mode(&se, GENI_SE_FIFO); > + > + writel_relaxed(tx_trans_cfg, uport->membase + SE_UART_TX_TRANS_CFG); > + writel_relaxed(tx_parity_cfg, uport->membase + SE_UART_TX_PARITY_CFG); > + writel_relaxed(rx_trans_cfg, uport->membase + SE_UART_RX_TRANS_CFG); > + writel_relaxed(rx_parity_cfg, uport->membase + SE_UART_RX_PARITY_CFG); > + writel_relaxed(bits_per_char, uport->membase + SE_UART_TX_WORD_LEN); > + writel_relaxed(bits_per_char, uport->membase + SE_UART_RX_WORD_LEN); > + writel_relaxed(stop_bit_len, uport->membase + SE_UART_TX_STOP_BIT_LEN); > + writel_relaxed(ser_clk_cfg, uport->membase + GENI_SER_M_CLK_CFG); > + writel_relaxed(ser_clk_cfg, uport->membase + GENI_SER_S_CLK_CFG); > + > + dev->con->write = qcom_geni_serial_earlycon_write; > + dev->con->setup = NULL; > + return 0; > +} > +OF_EARLYCON_DECLARE(qcom_serial, "qcom,geni-debug-uart", > + qcom_geni_serial_earlycon_setup); > + > static int __init console_register(struct uart_driver *drv) > { > return uart_register_driver(drv); > -- > Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > a Linux Foundation Collaborative Project > > -- > To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Bjorn, Thanks for pulling it into your tree and helping with the verification. On 4/9/2018 5:46 PM, Bjorn Andersson wrote: > On Mon 09 Apr 12:38 PDT 2018, Karthikeyan Ramasubramanian wrote: > >> Add early console support in Qualcomm Technologies Inc., GENI based >> UART controller. >> > > Hi Karthikeyan, > > I pulled this into my SDM845 tree and added "earlycon" to my command > line, the result is a working console up until the serial driver is > probed at which point the console stops. > > > I'm expecting that this should hand over to the normal console as it is > probed, unless I specify keep_bootcon in which case I expect all log > entries to be printed both through the earlycon and the normal console > (i.e. show up twice on the uart). I suspect the issue is something to do with clocks. I will look into it further and fix in the next submission. This is my current hypothesis: When the driver gets probed, it registers the uart port which in turn invokes the console setup. The console setup enables the "se" clock (geni_resources_on()->clk_prepare_enable()). Enabling the "se" clock is disturbing the rate that the clock is already on. Early console write is simultaneously going on while the console is being set up and this clock disturbance is causing the halt. > >> Signed-off-by: Karthikeyan Ramasubramanian <kramasub@codeaurora.org> >> Signed-off-by: Girish Mahadevan <girishm@codeaurora.org> > > This is a log of how the patch got to the list (and later to mainline), > as such your name should be the last entry in this line and you should > add an extra Co-Developed-by to indicate that you worked on this > together with Girish. Ok, I will do accordingly. > > Regards, > Bjorn > Regards, Karthik.
On Mon, Apr 09, 2018 at 01:38:41PM -0600, Karthikeyan Ramasubramanian wrote: > Add early console support in Qualcomm Technologies Inc., GENI based > UART controller. > > Signed-off-by: Karthikeyan Ramasubramanian <kramasub@codeaurora.org> > Signed-off-by: Girish Mahadevan <girishm@codeaurora.org> > --- > drivers/tty/serial/qcom_geni_serial.c | 85 ++++++++++++++++++++++++++++++++++- > 1 file changed, 83 insertions(+), 2 deletions(-) > > diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c > index e40d4a4..2ce83a57 100644 > --- a/drivers/tty/serial/qcom_geni_serial.c > +++ b/drivers/tty/serial/qcom_geni_serial.c > @@ -196,8 +196,18 @@ static bool qcom_geni_serial_poll_bit(struct uart_port *uport, > timeout_us = ((fifo_bits * USEC_PER_SEC) / baud) + 500; > } > > - return !readl_poll_timeout_atomic(uport->membase + offset, reg, > - (bool)(reg & field) == set, 10, timeout_us); > + /* > + * Use custom implementation instead of readl_poll_atomic since ktimer > + * is not ready at the time of early console. > + */ > + while (timeout_us) { > + reg = readl_relaxed(uport->membase + offset); > + if ((bool)(reg & field) == set) > + return true; > + udelay(10); > + timeout_us -= 10; > + } > + return false; > } > > static void qcom_geni_serial_setup_tx(struct uart_port *uport, u32 xmit_size) > @@ -944,6 +954,77 @@ static int __init qcom_geni_console_setup(struct console *co, char *options) > return uart_set_options(uport, co, baud, parity, bits, flow); > } > > +static void qcom_geni_serial_earlycon_write(struct console *con, > + const char *s, unsigned int n) > +{ > + struct earlycon_device *dev = con->data; > + > + __qcom_geni_serial_console_write(&dev->port, s, n); > +} > + > +static int __init qcom_geni_serial_earlycon_setup(struct earlycon_device *dev, > + const char *opt) > +{ > + struct uart_port *uport = &dev->port; > + u32 tx_trans_cfg; > + u32 tx_parity_cfg = 0; /* Disable Tx Parity */ > + u32 rx_trans_cfg = 0; > + u32 rx_parity_cfg = 0; /* Disable Rx Parity */ > + u32 stop_bit_len = 0; /* Default stop bit length - 1 bit */ > + u32 bits_per_char; > + u32 ser_clk_cfg; > + u32 baud = 115200; My understanding is that the baudrate should either be specified in the earlycon parmameter or left as set up by the bootloader: "If baud rate is not specified, the serial port must already be setup and configured." Documentation/admin-guide/kernel-parameters.txt > + unsigned int clk_div; > + unsigned long clk_rate; > + struct geni_se se; > + > + if (!uport->membase) > + return -EINVAL; > + > + memset(&se, 0, sizeof(se)); > + se.base = uport->membase; > + if (geni_se_read_proto(&se) != GENI_SE_UART) > + return -ENXIO; Declaring se on the stack and only initializing se.base is ugly, it makes the assumption that geni_se_read_proto() does not use other members of the struct, which is brittle. Possible alternatives could be to duplicate the code of geni_se_read_proto() here (also not ideal, though it's only a few lines) or add a geni_se_read_proto_xyz(void __iomem *base) which is then also called by geni_se_read_proto(). > + > + /* > + * Ignore Flow control. > + * n = 8. > + */ > + tx_trans_cfg = UART_CTS_MASK; > + bits_per_char = BITS_PER_BYTE; > + > + clk_rate = get_clk_div_rate(baud, &clk_div); > + if (!clk_rate) > + return -EINVAL; Here the parent clock rate and the corresponding divider are calculated, however later only the divider is programmed, apparently with the assumption that the calculated parent clock rate has been configured by the bootloader. I was told that at this stage the parent clock can't be reconfigured. If we effectively rely on the bootloader to do part of the initialization, shouldn't we then directly assume that the BL initializes the hardware and only do the minimum to integrate it with the kernel? > + ser_clk_cfg = SER_CLK_EN | (clk_div << CLK_DIV_SHFT); > + /* > + * Make an unconditional cancel on the main sequencer to reset > + * it else we could end up in data loss scenarios. > + */ > + qcom_geni_serial_poll_tx_done(uport); > + qcom_geni_serial_abort_rx(uport); > + geni_se_config_packing(&se, BITS_PER_BYTE, 1, false, true, false); > + geni_se_init(&se, DEF_FIFO_DEPTH_WORDS / 2, DEF_FIFO_DEPTH_WORDS - 2); > + geni_se_select_mode(&se, GENI_SE_FIFO); > + > + writel_relaxed(tx_trans_cfg, uport->membase + SE_UART_TX_TRANS_CFG); > + writel_relaxed(tx_parity_cfg, uport->membase + SE_UART_TX_PARITY_CFG); > + writel_relaxed(rx_trans_cfg, uport->membase + SE_UART_RX_TRANS_CFG); > + writel_relaxed(rx_parity_cfg, uport->membase + SE_UART_RX_PARITY_CFG); > + writel_relaxed(bits_per_char, uport->membase + SE_UART_TX_WORD_LEN); > + writel_relaxed(bits_per_char, uport->membase + SE_UART_RX_WORD_LEN); > + writel_relaxed(stop_bit_len, uport->membase + SE_UART_TX_STOP_BIT_LEN); > + writel_relaxed(ser_clk_cfg, uport->membase + GENI_SER_M_CLK_CFG); > + writel_relaxed(ser_clk_cfg, uport->membase + GENI_SER_S_CLK_CFG); > + > + dev->con->write = qcom_geni_serial_earlycon_write; > + dev->con->setup = NULL; > + return 0; > +} > +OF_EARLYCON_DECLARE(qcom_serial, "qcom,geni-debug-uart", > + qcom_geni_serial_earlycon_setup); > + > static int __init console_register(struct uart_driver *drv) > { > return uart_register_driver(drv); -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoting Karthikeyan Ramasubramanian (2018-04-09 12:38:41) > Add early console support in Qualcomm Technologies Inc., GENI based > UART controller. > > Signed-off-by: Karthikeyan Ramasubramanian <kramasub@codeaurora.org> > Signed-off-by: Girish Mahadevan <girishm@codeaurora.org> > --- > drivers/tty/serial/qcom_geni_serial.c | 85 ++++++++++++++++++++++++++++++++++- > 1 file changed, 83 insertions(+), 2 deletions(-) This patch needs to update the earlycon section of Documentation/admin-guide/kernel-parameters.txt as well. > > diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c > index e40d4a4..2ce83a57 100644 > --- a/drivers/tty/serial/qcom_geni_serial.c > +++ b/drivers/tty/serial/qcom_geni_serial.c > @@ -196,8 +196,18 @@ static bool qcom_geni_serial_poll_bit(struct uart_port *uport, > timeout_us = ((fifo_bits * USEC_PER_SEC) / baud) + 500; > } > > - return !readl_poll_timeout_atomic(uport->membase + offset, reg, > - (bool)(reg & field) == set, 10, timeout_us); > + /* > + * Use custom implementation instead of readl_poll_atomic since ktimer > + * is not ready at the time of early console. > + */ > + while (timeout_us) { > + reg = readl_relaxed(uport->membase + offset); > + if ((bool)(reg & field) == set) Is the cast needed? > + return true; > + udelay(10); > + timeout_us -= 10; > + } > + return false; > } > > static void qcom_geni_serial_setup_tx(struct uart_port *uport, u32 xmit_size) > @@ -944,6 +954,77 @@ static int __init qcom_geni_console_setup(struct console *co, char *options) > return uart_set_options(uport, co, baud, parity, bits, flow); > } [...] > + writel_relaxed(stop_bit_len, uport->membase + SE_UART_TX_STOP_BIT_LEN); > + writel_relaxed(ser_clk_cfg, uport->membase + GENI_SER_M_CLK_CFG); > + writel_relaxed(ser_clk_cfg, uport->membase + GENI_SER_S_CLK_CFG); > + > + dev->con->write = qcom_geni_serial_earlycon_write; > + dev->con->setup = NULL; > + return 0; > +} > +OF_EARLYCON_DECLARE(qcom_serial, "qcom,geni-debug-uart", Maybe geni_serial instead? Or qcom_geni? earlycon sort of implies serial already so it seems redundant. And yes I messed that up for msm_serial but it doesn't mean we should repeat the same mistake again. -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Tue, Apr 10, 2018 at 11:34 AM, Karthik Ramasubramanian <kramasub@codeaurora.org> wrote: > Hi Bjorn, > > Thanks for pulling it into your tree and helping with the verification. > > On 4/9/2018 5:46 PM, Bjorn Andersson wrote: >> On Mon 09 Apr 12:38 PDT 2018, Karthikeyan Ramasubramanian wrote: >> >>> Add early console support in Qualcomm Technologies Inc., GENI based >>> UART controller. >>> >> >> Hi Karthikeyan, >> >> I pulled this into my SDM845 tree and added "earlycon" to my command >> line, the result is a working console up until the serial driver is >> probed at which point the console stops. >> >> >> I'm expecting that this should hand over to the normal console as it is >> probed, unless I specify keep_bootcon in which case I expect all log >> entries to be printed both through the earlycon and the normal console >> (i.e. show up twice on the uart). > I suspect the issue is something to do with clocks. I will look into it > further and fix in the next submission. > > This is my current hypothesis: When the driver gets probed, it registers > the uart port which in turn invokes the console setup. The console setup > enables the "se" clock (geni_resources_on()->clk_prepare_enable()). > Enabling the "se" clock is disturbing the rate that the clock is already > on. Early console write is simultaneously going on while the console is > being set up and this clock disturbance is causing the halt. Or it could be a simple bug in this code. Specifically, your "custom implementation instead of readl_poll_atomic" is wrong. You don't ensure that "timeout_us" is evenly divisible by 10, yet you do: while (timeout_us) { ...; udelay(10); timeout_us -= 10; } One way to fix this is with this before the loop: timeout_us = DIV_ROUND_UP(timeout_us, 10) * 10; BTW: are you planning to spin this patch series anytime soon? -Doug -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 4/17/2018 12:12 AM, Stephen Boyd wrote: > Quoting Karthikeyan Ramasubramanian (2018-04-09 12:38:41) >> Add early console support in Qualcomm Technologies Inc., GENI based >> UART controller. >> >> Signed-off-by: Karthikeyan Ramasubramanian <kramasub@codeaurora.org> >> Signed-off-by: Girish Mahadevan <girishm@codeaurora.org> >> --- >> drivers/tty/serial/qcom_geni_serial.c | 85 ++++++++++++++++++++++++++++++++++- >> 1 file changed, 83 insertions(+), 2 deletions(-) > > This patch needs to update the earlycon section of > Documentation/admin-guide/kernel-parameters.txt as well. Thank you for pointing it out. I will update the documentation. > >> >> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c >> index e40d4a4..2ce83a57 100644 >> --- a/drivers/tty/serial/qcom_geni_serial.c >> +++ b/drivers/tty/serial/qcom_geni_serial.c >> @@ -196,8 +196,18 @@ static bool qcom_geni_serial_poll_bit(struct uart_port *uport, >> timeout_us = ((fifo_bits * USEC_PER_SEC) / baud) + 500; >> } >> >> - return !readl_poll_timeout_atomic(uport->membase + offset, reg, >> - (bool)(reg & field) == set, 10, timeout_us); >> + /* >> + * Use custom implementation instead of readl_poll_atomic since ktimer >> + * is not ready at the time of early console. >> + */ >> + while (timeout_us) { >> + reg = readl_relaxed(uport->membase + offset); >> + if ((bool)(reg & field) == set) > > Is the cast needed? Yes. Without that, "set" being a bool gets casted to 1 or 0 and the LHS is strictly compared against 1 or 0. That in turn leads to the function polling for the entirety of the timeout. > >> + return true; >> + udelay(10); >> + timeout_us -= 10; >> + } >> + return false; >> } >> >> static void qcom_geni_serial_setup_tx(struct uart_port *uport, u32 xmit_size) >> @@ -944,6 +954,77 @@ static int __init qcom_geni_console_setup(struct console *co, char *options) >> return uart_set_options(uport, co, baud, parity, bits, flow); >> } > [...] >> + writel_relaxed(stop_bit_len, uport->membase + SE_UART_TX_STOP_BIT_LEN); >> + writel_relaxed(ser_clk_cfg, uport->membase + GENI_SER_M_CLK_CFG); >> + writel_relaxed(ser_clk_cfg, uport->membase + GENI_SER_S_CLK_CFG); >> + >> + dev->con->write = qcom_geni_serial_earlycon_write; >> + dev->con->setup = NULL; >> + return 0; >> +} >> +OF_EARLYCON_DECLARE(qcom_serial, "qcom,geni-debug-uart", > > Maybe geni_serial instead? Or qcom_geni? earlycon sort of implies serial > already so it seems redundant. And yes I messed that up for msm_serial > but it doesn't mean we should repeat the same mistake again. Ok, I will update to qcom_geni. > Regards, Karthik.
On 4/16/2018 5:26 PM, Matthias Kaehlcke wrote: > On Mon, Apr 09, 2018 at 01:38:41PM -0600, Karthikeyan Ramasubramanian wrote: >> Add early console support in Qualcomm Technologies Inc., GENI based >> UART controller. >> +static int __init qcom_geni_serial_earlycon_setup(struct earlycon_device *dev, >> + const char *opt) >> +{ >> + struct uart_port *uport = &dev->port; >> + u32 tx_trans_cfg; >> + u32 tx_parity_cfg = 0; /* Disable Tx Parity */ >> + u32 rx_trans_cfg = 0; >> + u32 rx_parity_cfg = 0; /* Disable Rx Parity */ >> + u32 stop_bit_len = 0; /* Default stop bit length - 1 bit */ >> + u32 bits_per_char; >> + u32 ser_clk_cfg; >> + u32 baud = 115200; > > My understanding is that the baudrate should either be specified in > the earlycon parmameter or left as set up by the bootloader: > > "If baud rate is not specified, the serial port must already be setup > and configured." > > Documentation/admin-guide/kernel-parameters.txt Ok, I will look into the baud rate specified in earlycon parameter and if not specified, let the core run at the baud rate as set up by the bootloader. > >> + unsigned int clk_div; >> + unsigned long clk_rate; >> + struct geni_se se; >> + >> + if (!uport->membase) >> + return -EINVAL; >> + >> + memset(&se, 0, sizeof(se)); >> + se.base = uport->membase; >> + if (geni_se_read_proto(&se) != GENI_SE_UART) >> + return -ENXIO; > > Declaring se on the stack and only initializing se.base is ugly, it > makes the assumption that geni_se_read_proto() does not use other > members of the struct, which is brittle. > > Possible alternatives could be to duplicate the code of > geni_se_read_proto() here (also not ideal, though it's only a few > lines) or add a geni_se_read_proto_xyz(void __iomem *base) which is > then also called by geni_se_read_proto(). It is not just geni_se_read_proto_xyz, we also have to expose a whole bunch of helper functions just for earlycon: geni_se_init, geni_se_config_packing, geni_se_select_mode. It is for this reason, a consistent interface has been written in qcom-geni-se driver keeping earlycon and non-earlycon in mind. It is a choice between duplication of interface or duplication of code. > >> + >> + /* >> + * Ignore Flow control. >> + * n = 8. >> + */ >> + tx_trans_cfg = UART_CTS_MASK; >> + bits_per_char = BITS_PER_BYTE; >> + >> + clk_rate = get_clk_div_rate(baud, &clk_div); >> + if (!clk_rate) >> + return -EINVAL; > > Here the parent clock rate and the corresponding divider are > calculated, however later only the divider is programmed, apparently > with the assumption that the calculated parent clock rate has been > configured by the bootloader. > > I was told that at this stage the parent clock can't be > reconfigured. If we effectively rely on the bootloader to do part of > the initialization, shouldn't we then directly assume that the BL > initializes the hardware and only do the minimum to integrate it > with the kernel? True. I will double-check and remove configuring the divider. > >> + ser_clk_cfg = SER_CLK_EN | (clk_div << CLK_DIV_SHFT); >> + /* >> + * Make an unconditional cancel on the main sequencer to reset >> + * it else we could end up in data loss scenarios. Regards, Karthik.
On 4/30/2018 3:40 PM, Doug Anderson wrote: > Hi, > > On Tue, Apr 10, 2018 at 11:34 AM, Karthik Ramasubramanian > <kramasub@codeaurora.org> wrote: >> Hi Bjorn, >> >> Thanks for pulling it into your tree and helping with the verification. >> >> On 4/9/2018 5:46 PM, Bjorn Andersson wrote: >>> On Mon 09 Apr 12:38 PDT 2018, Karthikeyan Ramasubramanian wrote: >>> >>>> Add early console support in Qualcomm Technologies Inc., GENI based >>>> UART controller. >>>> >>> >>> Hi Karthikeyan, >>> >>> I pulled this into my SDM845 tree and added "earlycon" to my command >>> line, the result is a working console up until the serial driver is >>> probed at which point the console stops. >>> >>> >>> I'm expecting that this should hand over to the normal console as it is >>> probed, unless I specify keep_bootcon in which case I expect all log >>> entries to be printed both through the earlycon and the normal console >>> (i.e. show up twice on the uart). >> I suspect the issue is something to do with clocks. I will look into it >> further and fix in the next submission. >> >> This is my current hypothesis: When the driver gets probed, it registers >> the uart port which in turn invokes the console setup. The console setup >> enables the "se" clock (geni_resources_on()->clk_prepare_enable()). >> Enabling the "se" clock is disturbing the rate that the clock is already >> on. Early console write is simultaneously going on while the console is >> being set up and this clock disturbance is causing the halt. > > Or it could be a simple bug in this code. Specifically, your "custom > implementation instead of readl_poll_atomic" is wrong. You don't > ensure that "timeout_us" is evenly divisible by 10, yet you do: > > while (timeout_us) { > ...; > udelay(10); > timeout_us -= 10; > } > > One way to fix this is with this before the loop: > > timeout_us = DIV_ROUND_UP(timeout_us, 10) * 10; > > > BTW: are you planning to spin this patch series anytime soon? Thats a good catch. I will fix that. Let us see if it fixes the issue that Bjorn has reported. I will address all the comments and upload the next spin in couple of days. > > > -Doug > Regards, Karthik.
diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c index e40d4a4..2ce83a57 100644 --- a/drivers/tty/serial/qcom_geni_serial.c +++ b/drivers/tty/serial/qcom_geni_serial.c @@ -196,8 +196,18 @@ static bool qcom_geni_serial_poll_bit(struct uart_port *uport, timeout_us = ((fifo_bits * USEC_PER_SEC) / baud) + 500; } - return !readl_poll_timeout_atomic(uport->membase + offset, reg, - (bool)(reg & field) == set, 10, timeout_us); + /* + * Use custom implementation instead of readl_poll_atomic since ktimer + * is not ready at the time of early console. + */ + while (timeout_us) { + reg = readl_relaxed(uport->membase + offset); + if ((bool)(reg & field) == set) + return true; + udelay(10); + timeout_us -= 10; + } + return false; } static void qcom_geni_serial_setup_tx(struct uart_port *uport, u32 xmit_size) @@ -944,6 +954,77 @@ static int __init qcom_geni_console_setup(struct console *co, char *options) return uart_set_options(uport, co, baud, parity, bits, flow); } +static void qcom_geni_serial_earlycon_write(struct console *con, + const char *s, unsigned int n) +{ + struct earlycon_device *dev = con->data; + + __qcom_geni_serial_console_write(&dev->port, s, n); +} + +static int __init qcom_geni_serial_earlycon_setup(struct earlycon_device *dev, + const char *opt) +{ + struct uart_port *uport = &dev->port; + u32 tx_trans_cfg; + u32 tx_parity_cfg = 0; /* Disable Tx Parity */ + u32 rx_trans_cfg = 0; + u32 rx_parity_cfg = 0; /* Disable Rx Parity */ + u32 stop_bit_len = 0; /* Default stop bit length - 1 bit */ + u32 bits_per_char; + u32 ser_clk_cfg; + u32 baud = 115200; + unsigned int clk_div; + unsigned long clk_rate; + struct geni_se se; + + if (!uport->membase) + return -EINVAL; + + memset(&se, 0, sizeof(se)); + se.base = uport->membase; + if (geni_se_read_proto(&se) != GENI_SE_UART) + return -ENXIO; + + /* + * Ignore Flow control. + * n = 8. + */ + tx_trans_cfg = UART_CTS_MASK; + bits_per_char = BITS_PER_BYTE; + + clk_rate = get_clk_div_rate(baud, &clk_div); + if (!clk_rate) + return -EINVAL; + + ser_clk_cfg = SER_CLK_EN | (clk_div << CLK_DIV_SHFT); + /* + * Make an unconditional cancel on the main sequencer to reset + * it else we could end up in data loss scenarios. + */ + qcom_geni_serial_poll_tx_done(uport); + qcom_geni_serial_abort_rx(uport); + geni_se_config_packing(&se, BITS_PER_BYTE, 1, false, true, false); + geni_se_init(&se, DEF_FIFO_DEPTH_WORDS / 2, DEF_FIFO_DEPTH_WORDS - 2); + geni_se_select_mode(&se, GENI_SE_FIFO); + + writel_relaxed(tx_trans_cfg, uport->membase + SE_UART_TX_TRANS_CFG); + writel_relaxed(tx_parity_cfg, uport->membase + SE_UART_TX_PARITY_CFG); + writel_relaxed(rx_trans_cfg, uport->membase + SE_UART_RX_TRANS_CFG); + writel_relaxed(rx_parity_cfg, uport->membase + SE_UART_RX_PARITY_CFG); + writel_relaxed(bits_per_char, uport->membase + SE_UART_TX_WORD_LEN); + writel_relaxed(bits_per_char, uport->membase + SE_UART_RX_WORD_LEN); + writel_relaxed(stop_bit_len, uport->membase + SE_UART_TX_STOP_BIT_LEN); + writel_relaxed(ser_clk_cfg, uport->membase + GENI_SER_M_CLK_CFG); + writel_relaxed(ser_clk_cfg, uport->membase + GENI_SER_S_CLK_CFG); + + dev->con->write = qcom_geni_serial_earlycon_write; + dev->con->setup = NULL; + return 0; +} +OF_EARLYCON_DECLARE(qcom_serial, "qcom,geni-debug-uart", + qcom_geni_serial_earlycon_setup); + static int __init console_register(struct uart_driver *drv) { return uart_register_driver(drv);