| 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
| ||
|
Message-ID: <a167c98a02e8020c81f377d2aea0878a53965581.camel@nvidia.com>
Date: Thu, 13 Feb 2025 12:35:59 +0000
From: Kartik Rajput <kkartik@...dia.com>
To: "andriy.shevchenko@...ux.intel.com" <andriy.shevchenko@...ux.intel.com>
CC: Jon Hunter <jonathanh@...dia.com>, "robh@...nel.org" <robh@...nel.org>,
"robert.marko@...tura.hr" <robert.marko@...tura.hr>, "arnd@...nel.org"
<arnd@...nel.org>, "thierry.reding@...il.com" <thierry.reding@...il.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"conor+dt@...nel.org" <conor+dt@...nel.org>, "geert+renesas@...der.be"
<geert+renesas@...der.be>, "devicetree@...r.kernel.org"
<devicetree@...r.kernel.org>, "jirislaby@...nel.org" <jirislaby@...nel.org>,
"krzk+dt@...nel.org" <krzk+dt@...nel.org>, "hvilleneuve@...onoff.com"
<hvilleneuve@...onoff.com>, "gregkh@...uxfoundation.org"
<gregkh@...uxfoundation.org>, "schnelle@...ux.ibm.com"
<schnelle@...ux.ibm.com>, "linux-serial@...r.kernel.org"
<linux-serial@...r.kernel.org>, "linux-tegra@...r.kernel.org"
<linux-tegra@...r.kernel.org>
Subject: Re: [PATCH v4 2/2] serial: tegra-utc: Add driver for Tegra UART Trace
Controller (UTC)
On Thu, 2025-02-13 at 13:05 +0200, Andy Shevchenko wrote:
> External email: Use caution opening links or attachments
>
>
> On Thu, Feb 13, 2025 at 03:34:42PM +0530, Kartik Rajput wrote:
> > The Tegra264 SoC supports the UART Trace Controller (UTC), which
> > allows
> > multiple firmware clients (up to 16) to share a single physical
> > UART.
> > Each client is provided with its own interrupt and has access to a
> > 128-character wide FIFO for both transmit (TX) and receive (RX)
> > operations.
> >
> > Add tegra-utc driver to support Tegra UART Trace Controller (UTC)
> > client.
>
> Btw, neither the commit message nor cover letter explain why the new
> driver
> is needed. There are some serial Tegra drivers already. Is this one
> completely
> different from the existing drivers?
>
Tegra264 platforms have a single debug UART which is shared across
multiple firmwares and the OS. The Tegra UTC is added to address this
issue.
It is a completely different driver.
> ...
>
> > +#define TEGRA_UTC_ENABLE 0x0
>
> It would be nice to use fixed width values for the register offsets,
> e.g., 0x000 here.
>
Ack.
> > +#define TEGRA_UTC_ENABLE_CLIENT_ENABLE BIT(0)
> > +
> > +#define TEGRA_UTC_FIFO_THRESHOLD 0x8
> > +
> > +#define TEGRA_UTC_COMMAND 0xc
>
> Ditto.
>
Ack.
> > +#define TEGRA_UTC_COMMAND_RESET BIT(0)
> > +#define TEGRA_UTC_COMMAND_FLUSH BIT(1)
>
> > +#define TEGRA_UTC_DATA 0x20
>
> Ditto.
>
Ack.
> > +#define TEGRA_UTC_FIFO_STATUS 0x100
> > +#define TEGRA_UTC_FIFO_EMPTY BIT(0)
> > +#define TEGRA_UTC_FIFO_FULL BIT(1)
> > +#define TEGRA_UTC_FIFO_REQ BIT(2)
> > +#define TEGRA_UTC_FIFO_OVERFLOW BIT(3)
> > +#define TEGRA_UTC_FIFO_TIMEOUT BIT(4)
> > +
> > +#define TEGRA_UTC_FIFO_OCCUPANCY 0x104
> > +
> > +#define TEGRA_UTC_INTR_STATUS 0x108
> > +#define TEGRA_UTC_INTR_SET 0x10c
> > +#define TEGRA_UTC_INTR_MASK 0x110
> > +#define TEGRA_UTC_INTR_CLEAR 0x114
> > +#define TEGRA_UTC_INTR_EMPTY BIT(0)
> > +#define TEGRA_UTC_INTR_FULL BIT(1)
> > +#define TEGRA_UTC_INTR_REQ BIT(2)
> > +#define TEGRA_UTC_INTR_OVERFLOW BIT(3)
> > +#define TEGRA_UTC_INTR_TIMEOUT BIT(4)
>
> ...
>
> > +#if IS_ENABLED(CONFIG_SERIAL_TEGRA_UTC_CONSOLE)
> > +#define TEGRA_UTC_DEFAULT_FIFO_THRESHOLD 0x4
>
> Hmm... Is this a register offset? If not, why it's in a hexadecimal
> format?
>
This is not a register offset. I will use a decimal value here.
> > +#define TEGRA_UTC_EARLYCON_MAX_BURST_SIZE 128
>
> ...
>
> > +static int tegra_utc_probe(struct platform_device *pdev)
> > +{
> > + const unsigned int *soc_fifosize;
> > + struct device *dev = &pdev->dev;
> > + struct tegra_utc_port *tup;
> > + int ret;
> > +
> > + tup = devm_kzalloc(&pdev->dev, sizeof(*tup), GFP_KERNEL);
>
> Use dev?
Thanks for catching this, this should've been done before. I will
update this.
>
> > + if (!tup)
> > + return -ENOMEM;
> > +
> > + ret = device_property_read_u32(dev, "tx-threshold", &tup-
> > >tx_threshold);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "missing %s
> > property\n", "tx-threshold");
> > +
> > + ret = device_property_read_u32(dev, "rx-threshold", &tup-
> > >rx_threshold);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "missing %s
> > property\n", "rx-threshold");
> > +
> > + soc_fifosize = device_get_match_data(&pdev->dev);
> > +
This needs update as well.
> > tup->fifosize = *soc_fifosize;
> > +
> > + tup->tx_base = devm_platform_ioremap_resource_byname(pdev,
> > "tx");
> > + if (IS_ERR(tup->tx_base))
> > + return PTR_ERR(tup->tx_base);
> > +
> > + tup->rx_base = devm_platform_ioremap_resource_byname(pdev,
> > "rx");
> > + if (IS_ERR(tup->rx_base))
> > + return PTR_ERR(tup->rx_base);
>
> > + ret = tegra_utc_setup_port(&pdev->dev, tup);
>
> Ditto.
Ack.
>
> > + if (ret)
> > + dev_err_probe(dev, ret, "failed to setup uart
> > port\n");
> > +
> > + platform_set_drvdata(pdev, tup);
> > +
> > + return tegra_utc_register_port(tup);
> > +}
>
> ...
>
> With the above being addressed, FWIW,
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
Thanks & Regards,
Kartik
Powered by blists - more mailing lists