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.
--
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-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
