Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH] tty: serial: samsung: Enable baud clock during
 initialisation
To:     Stuart Menefy <stuart.menefy@mathembedded.com>,
        Krzysztof Kozlowski <krzk@kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Jiri Slaby <jslaby@suse.com>, linux-serial@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-samsung-soc@vger.kernel.org
From:   Marek Szyprowski <m.szyprowski@samsung.com>
Message-ID: <ed76663a-d50e-e8f1-df84-d44100af8ecd@samsung.com>
Date:   Thu, 28 Feb 2019 11:07:48 +0100
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:60.0) Gecko/20100101
        Thunderbird/60.5.1
MIME-Version: 1.0
In-Reply-To: <20190212214022.9446-1-stuart.menefy@mathembedded.com>
Content-Transfer-Encoding: 7bit
Content-Language: en-US
Content-Type: text/plain; charset="utf-8"
CMS-TYPE: 201P
References: <CGME20190212214034epcas5p41ddd5b935a64ee0999fe7186e0d5c608@epcas5p4.samsung.com>
        <20190212214022.9446-1-stuart.menefy@mathembedded.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Stuart,

On 2019-02-12 22:40, Stuart Menefy wrote:
> The Exynos 5260, like the 5433, appears to require baud clock as
> well as pclk to be running before accessing any of the registers,
> otherwise an external abort is raised.
>
> The serial driver already enables baud clock when required, but only
> if it knows which clock is baud clock. On older SoCs baud clock may be
> selected from a number of possible clocks so to support this the driver
> only selects which clock to use for baud clock when a port is opened,
> at which point the desired baud rate is known and the best clock can be
> selected.
>
> The result is that there are a number of circumstances in which
> registers are accessed without first explicitly enabling baud clock:
>  - while the driver is being initialised
>  - the initial parts of opening a port for the first time
>  - when resuming if the port hasn't been already opened
>
> The 5433 overcomes this currently by marking the baud clock as
> CLK_IGNORE_UNUSED, so the clock is always enabled, however
> for the 5260 I've been trying to avoid this.
>
> This change adds code to pick the first available clock to use
> as baud clock and enables it while initialising the driver.
>
> This code wouldn't be sufficient on a SoC which supports
> multiple possible baud clock sources _and_ requires the
> correct baud clock to be enabled before accessing any of the
> serial port registers (in particular the register which selects
> which clock to use as the baud clock).  As far as I know
> such hardware doesn't exist.

I think that we can use a simpler approach. The driver can simply keep
baud clock enabled together with pclk always when there is only one baud
clock source (s3c24xx_serial_drv_data.num_clks == 1). This will work
both on Exynos 5260 and 5433.

> Signed-off-by: Stuart Menefy <stuart.menefy@mathembedded.com>
> ---
>  drivers/tty/serial/samsung.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
>
> diff --git a/drivers/tty/serial/samsung.c b/drivers/tty/serial/samsung.c
> index 9fc3559f80d9..83fd51607741 100644
> --- a/drivers/tty/serial/samsung.c
> +++ b/drivers/tty/serial/samsung.c
> @@ -1694,6 +1694,42 @@ s3c24xx_serial_cpufreq_deregister(struct s3c24xx_uart_port *port)
>  }
>  #endif
>  
> +static int s3c24xx_serial_enable_baudclk(struct s3c24xx_uart_port *ourport)
> +{
> +	struct device *dev = ourport->port.dev;
> +	struct s3c24xx_uart_info *info = ourport->info;
> +	char clk_name[MAX_CLK_NAME_LENGTH];
> +	unsigned int clk_sel;
> +	struct clk *clk;
> +	int clk_num;
> +	int ret;
> +
> +	clk_sel = ourport->cfg->clk_sel ? : info->def_clk_sel;
> +	for (clk_num = 0; clk_num < info->num_clks; clk_num++) {
> +		if (!(clk_sel & (1 << clk_num)))
> +			continue;
> +
> +		sprintf(clk_name, "clk_uart_baud%d", clk_num);
> +		clk = clk_get(dev, clk_name);
> +		if (IS_ERR(clk))
> +			continue;
> +
> +		ret = clk_prepare_enable(clk);
> +		if (ret) {
> +			clk_put(clk);
> +			continue;
> +		}
> +
> +		ourport->baudclk = clk;
> +		ourport->baudclk_rate = clk_get_rate(clk);
> +		s3c24xx_serial_setsource(&ourport->port, clk_num);
> +
> +		return 0;
> +	}
> +
> +	return -EINVAL;
> +}
> +
>  /* s3c24xx_serial_init_port
>   *
>   * initialise a single serial port from the platform device given
> @@ -1788,6 +1824,10 @@ static int s3c24xx_serial_init_port(struct s3c24xx_uart_port *ourport,
>  		goto err;
>  	}
>  
> +	ret = s3c24xx_serial_enable_baudclk(ourport);
> +	if (ret)
> +		pr_warn("uart: failed to enable baudclk\n");
> +
>  	/* Keep all interrupts masked and cleared */
>  	if (s3c24xx_serial_has_interrupt_mask(port)) {
>  		wr_regl(port, S3C64XX_UINTM, 0xf);
> @@ -1901,6 +1941,8 @@ static int s3c24xx_serial_probe(struct platform_device *pdev)
>  	 * and keeps the clock enabled in this case.
>  	 */
>  	clk_disable_unprepare(ourport->clk);
> +	if (!IS_ERR(ourport->baudclk))
> +		clk_disable_unprepare(ourport->baudclk);
>  
>  	ret = s3c24xx_serial_cpufreq_register(ourport);
>  	if (ret < 0)

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland