linux-kernel - Re: [RFC PATCH] can: m_can: Add driver for M

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Yk2vOj8wKi4FdPg2@orome>
Date:   Wed, 6 Apr 2022 17:18:18 +0200
From:   Thierry Reding <thierry.reding@...il.com>
To:     Brian Silverman <bsilver16384@...il.com>
Cc:     Brian Silverman <brian.silverman@...erivertech.com>,
        Wolfgang Grandegger <wg@...ndegger.com>,
        Marc Kleine-Budde <mkl@...gutronix.de>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Jonathan Hunter <jonathanh@...dia.com>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        Dan Murphy <dmurphy@...com>,
        open list <linux-kernel@...r.kernel.org>,
        "open list:CAN NETWORK DRIVERS" <linux-can@...r.kernel.org>,
        "open list:NETWORKING DRIVERS" <netdev@...r.kernel.org>,
        "open list:TEGRA ARCHITECTURE SUPPORT" <linux-tegra@...r.kernel.org>
Subject: Re: [RFC PATCH] can: m_can: Add driver for M_CAN hardware in NVIDIA
 devices

On Wed, Jan 05, 2022 at 04:25:09PM -0800, Brian Silverman wrote:
> It's a M_TTCAN with some NVIDIA-specific glue logic and clocks. The
> existing m_can driver works with it after handling the glue logic.
> 
> The code is a combination of pieces from m_can_platform and NVIDIA's
> driver [1].
> 
> [1] https://github.com/hartkopp/nvidia-t18x-can/blob/master/r32.2.1/nvidia/drivers/net/can/mttcan/hal/m_ttcan.c
> 
> Signed-off-by: Brian Silverman <brian.silverman@...erivertech.com>
> ---
> I ran into bugs with the error handling in NVIDIA's m_ttcan driver, so I
> switched to m_can which has been much better. I'm looking for feedback
> on whether I should ensure rebasing hasn't broken anything, write up DT
> documentation, and submit this patch for real. The driver works great,
> but I've got some questions about submitting it.
> 
> question: This has liberal copying of GPL code from NVIDIA's
> non-upstreamed m_ttcan driver. Is that OK?
> 
> corollary: I don't know what any of this glue logic does. I do know the
> device doesn't work without it. I can't find any documentation of what
> these addresses do.
> 
> question: There is some duplication between this and m_can_platform. It
> doesn't seem too bad to me, but is this the preferred way to do it or is
> there another alternative?
> 
> question: Do new DT bindings need to be in the YAML format, or is the
> .txt one OK?
> 
>  drivers/net/can/m_can/Kconfig       |  10 +
>  drivers/net/can/m_can/Makefile      |   1 +
>  drivers/net/can/m_can/m_can_tegra.c | 362 ++++++++++++++++++++++++++++
>  3 files changed, 373 insertions(+)
>  create mode 100644 drivers/net/can/m_can/m_can_tegra.c

Sorry for the late reply, I completely missed this. I think along with
the DT bindings it'd be great if you could provide DT updates for the
platform that you tested this on so we can get that upstream as well.

I don't know much about CAN so I can't comment on those pieces, so just
a few thoughts on the integration bits.

> diff --git a/drivers/net/can/m_can/m_can_tegra.c b/drivers/net/can/m_can/m_can_tegra.c
[...]
> +static int m_can_tegra_probe(struct platform_device *pdev)
> +{
[...]
> +	ret = clk_set_parent(can_clk, pclk);
> +	if (ret) {
> +		goto probe_fail;
> +	}
> +
> +	ret = fwnode_property_read_u32(dev_fwnode(&pdev->dev), "can-clk-rate", &rate);
> +	if (ret) {
> +		goto probe_fail;
> +	}
> +
> +	new_rate = clk_round_rate(can_clk, rate);
> +	if (!new_rate)
> +		dev_warn(&pdev->dev, "incorrect CAN clock rate\n");
> +
> +	ret = clk_set_rate(can_clk, new_rate > 0 ? new_rate : rate);
> +	if (ret) {
> +		goto probe_fail;
> +	}
> +
> +	ret = clk_set_rate(host_clk, new_rate > 0 ? new_rate : rate);
> +	if (ret) {
> +		goto probe_fail;
> +	}
> +
> +	if (core_clk) {
> +		ret = fwnode_property_read_u32(dev_fwnode(&pdev->dev), "core-clk-rate", &rate);
> +		if (ret) {
> +			goto probe_fail;
> +		}
> +		new_rate = clk_round_rate(core_clk, rate);
> +		if (!new_rate)
> +			dev_warn(&pdev->dev, "incorrect CAN_CORE clock rate\n");
> +
> +		ret = clk_set_rate(core_clk, new_rate > 0 ? new_rate : rate);
> +		if (ret) {
> +			goto probe_fail;
> +		}
> +	}

Can all of this clock setup not be simplified by using the standard
assigned-clocks, assigned-clock-parent and assigned-clock-rates DT
properties?

> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "m_can");
> +	addr = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(addr)) {
> +		ret = PTR_ERR(addr);
> +		goto probe_fail;
> +	}
> +
> +	irq = platform_get_irq_byname(pdev, "int0");
> +	if (irq < 0) {
> +		ret = -ENODEV;
> +		goto probe_fail;
> +	}

If there's only one of these, it doesn't make much sense to name them.
But perhaps this can be discussed when reviewing the DT bindings.

[...]
> +static const struct of_device_id m_can_of_table[] = {
> +	{ .compatible = "nvidia,tegra194-m_can", .data = NULL },

We typically name the compatible string after the IP block name. The TRM
references this as simply "CAN", so I think "nvidia,tegra194-can" would
be more in line with what we use for existing Tegra hardware.

Thierry

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)