Re: [v2,8/8] tty: serial: qcom_geni_serial: Add early console support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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



[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux PPP]     [Linux FS]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Linmodem]     [Device Mapper]     [Linux Kernel for ARM]

  Powered by Linux