* Re: kernel-doc unbuildable with sphinx 1.7
From: Jonathan Corbet @ 2018-03-02 14:52 UTC (permalink / raw)
To: Takashi Iwai; +Cc: Jiri Slaby, Jani Nikula, linux-doc
In-Reply-To: <s5hvaeefsf6.wl-tiwai@suse.de>
On Fri, 02 Mar 2018 15:44:45 +0100
Takashi Iwai <tiwai@suse.de> wrote:
> Sure, just waited for the feedback from Jon :)
I want to try it with both old and new Sphinx, of course, but will have a
hard time doing it today - on the road all day. But it looks reasonable
at a first glance; I'll try to get it into -rc4 (and -stable, I guess) if
I can.
Thanks,
jon
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] Documentation/sphinx: Fix Directive import error
From: Takashi Iwai @ 2018-03-02 14:55 UTC (permalink / raw)
To: Matthew Wilcox; +Cc: Jonathan Corbet, Jani Nikula, linux-doc, linux-kernel
In-Reply-To: <20180302145055.GA18137@bombadil.infradead.org>
On Fri, 02 Mar 2018 15:50:55 +0100,
Matthew Wilcox wrote:
>
> On Fri, Mar 02, 2018 at 12:49:03PM +0100, Takashi Iwai wrote:
> > I'm no expert of sphinx nor python, so something might be wrong.
> > Please check it.
>
> I'm also not a pythonista, but ...
>
> > --- a/Documentation/sphinx/kerneldoc.py
> > +++ b/Documentation/sphinx/kerneldoc.py
> > @@ -37,7 +37,10 @@ import glob
> > from docutils import nodes, statemachine
> > from docutils.statemachine import ViewList
> > from docutils.parsers.rst import directives
> > -from sphinx.util.compat import Directive
> > +try:
> > + from sphinx.util.compat import Directive
> > +except ImportError:
> > + from docutils.parsers.rst import directives, Directive
>
> It seems to me the previous line already imported
> docutils.parsers.rst.directives, and we should probably prefer the newer
> parser even with Sphinx 1.6, so I would think this would work better:
>
> -from sphinx.util.compat import Directive
> +try:
> + from docutils.parsers.rst import Directive
> +except ImportError:
> + from sphinx.util.compat import Directive
>
> (it works on Debian with Sphinx 1.6.7)
Makes more sense, yes.
Jon, would you like me resending the patch? Or it's trivial enough as
you can fix in place.
thanks,
Takashi
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] Documentation/sphinx: Fix Directive import error
From: Jonathan Corbet @ 2018-03-02 15:17 UTC (permalink / raw)
To: Takashi Iwai; +Cc: Matthew Wilcox, Jani Nikula, linux-doc, linux-kernel
In-Reply-To: <s5hr2p2frxx.wl-tiwai@suse.de>
On Fri, 02 Mar 2018 15:55:06 +0100
Takashi Iwai <tiwai@suse.de> wrote:
> Jon, would you like me resending the patch? Or it's trivial enough as
> you can fix in place.
If you can resend that would be great - one less thing for me to have to
squeeze in :)
Thanks,
jon
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH v2] Documentation/sphinx: Fix Directive import error
From: Takashi Iwai @ 2018-03-02 15:28 UTC (permalink / raw)
To: Jonathan Corbet
Cc: Jani Nikula, Jiri Slaby, Matthew Wilcox, linux-doc, linux-kernel
The sphinx.util.compat for Directive stuff was deprecated in the
recent Sphinx version, and now we get a build error.
Let's import from the new place, docutils.parsers.rst, while keeping
the old sphinx.util.compat as fallback.
Bugzilla: https://bugzilla.opensuse.org/show_bug.cgi?id=1083694
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
v1->v2: Change the fallback order as Matthew suggested, the new one at first
Documentation/sphinx/kerneldoc.py | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/Documentation/sphinx/kerneldoc.py b/Documentation/sphinx/kerneldoc.py
index 39aa9e8697cc..34396976eb0a 100644
--- a/Documentation/sphinx/kerneldoc.py
+++ b/Documentation/sphinx/kerneldoc.py
@@ -37,7 +37,10 @@ import glob
from docutils import nodes, statemachine
from docutils.statemachine import ViewList
from docutils.parsers.rst import directives
-from sphinx.util.compat import Directive
+try:
+ from docutils.parsers.rst import directives, Directive
+except ImportError:
+ from sphinx.util.compat import Directive
from sphinx.ext.autodoc import AutodocReporter
__version__ = '1.0'
--
2.16.2
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* Re: [PATCH v2] Documentation/sphinx: Fix Directive import error
From: Jani Nikula @ 2018-03-02 16:01 UTC (permalink / raw)
To: Takashi Iwai, Jonathan Corbet
Cc: Jiri Slaby, Matthew Wilcox, linux-doc, linux-kernel
In-Reply-To: <20180302152831.11510-1-tiwai@suse.de>
On Fri, 02 Mar 2018, Takashi Iwai <tiwai@suse.de> wrote:
> The sphinx.util.compat for Directive stuff was deprecated in the
> recent Sphinx version, and now we get a build error.
>
> Let's import from the new place, docutils.parsers.rst, while keeping
> the old sphinx.util.compat as fallback.
>
> Bugzilla: https://bugzilla.opensuse.org/show_bug.cgi?id=1083694
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
> v1->v2: Change the fallback order as Matthew suggested, the new one at first
So this crossed my mind as well... and then I thought it'll probably
succeed on older Sphinx, and the fallback is not needed. The question
is, are these equal? Can we just import from docutils.parsers.rst?
I'm sorry I don't have the time to find the answers to these questions
as well. :(
BR,
Jani.
>
> Documentation/sphinx/kerneldoc.py | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/sphinx/kerneldoc.py b/Documentation/sphinx/kerneldoc.py
> index 39aa9e8697cc..34396976eb0a 100644
> --- a/Documentation/sphinx/kerneldoc.py
> +++ b/Documentation/sphinx/kerneldoc.py
> @@ -37,7 +37,10 @@ import glob
> from docutils import nodes, statemachine
> from docutils.statemachine import ViewList
> from docutils.parsers.rst import directives
> -from sphinx.util.compat import Directive
> +try:
> + from docutils.parsers.rst import directives, Directive
> +except ImportError:
> + from sphinx.util.compat import Directive
> from sphinx.ext.autodoc import AutodocReporter
>
> __version__ = '1.0'
--
Jani Nikula, Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v2] Documentation/sphinx: Fix Directive import error
From: Matthew Wilcox @ 2018-03-02 16:34 UTC (permalink / raw)
To: Takashi Iwai
Cc: Jonathan Corbet, Jani Nikula, Jiri Slaby, linux-doc, linux-kernel
In-Reply-To: <20180302152831.11510-1-tiwai@suse.de>
On Fri, Mar 02, 2018 at 04:28:31PM +0100, Takashi Iwai wrote:
> from docutils.parsers.rst import directives
> -from sphinx.util.compat import Directive
> +try:
> + from docutils.parsers.rst import directives, Directive
We also don't need 'directives' on this line as it was imported on the
previous line.
> +except ImportError:
> + from sphinx.util.compat import Directive
> from sphinx.ext.autodoc import AutodocReporter
>
> __version__ = '1.0'
Here's what I tested on Debian:
+++ b/Documentation/sphinx/kerneldoc.py
@@ -37,7 +37,10 @@ import glob
from docutils import nodes, statemachine
from docutils.statemachine import ViewList
from docutils.parsers.rst import directives
-from sphinx.util.compat import Directive
+try:
+ from docutils.parsers.rst import Directive
+except ImportError:
+ from sphinx.util.compat import Directive
from sphinx.ext.autodoc import AutodocReporter
__version__ = '1.0'
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v2] Documentation/sphinx: Fix Directive import error
From: Matthew Wilcox @ 2018-03-02 16:42 UTC (permalink / raw)
To: Jani Nikula
Cc: Takashi Iwai, Jonathan Corbet, Jiri Slaby, linux-doc,
linux-kernel
In-Reply-To: <871sh2v53l.fsf@intel.com>
On Fri, Mar 02, 2018 at 06:01:50PM +0200, Jani Nikula wrote:
> On Fri, 02 Mar 2018, Takashi Iwai <tiwai@suse.de> wrote:
> > The sphinx.util.compat for Directive stuff was deprecated in the
> > recent Sphinx version, and now we get a build error.
> >
> > Let's import from the new place, docutils.parsers.rst, while keeping
> > the old sphinx.util.compat as fallback.
> >
> > Bugzilla: https://bugzilla.opensuse.org/show_bug.cgi?id=1083694
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> > v1->v2: Change the fallback order as Matthew suggested, the new one at first
>
> So this crossed my mind as well... and then I thought it'll probably
> succeed on older Sphinx, and the fallback is not needed. The question
> is, are these equal? Can we just import from docutils.parsers.rst?
I found a github page which implies that docutils.parsers.rst.Directive
was added 12 years ago (!) so we're probably safe to rely on it:
https://github.com/docutils-mirror/docutils/commit/9649abee47b4ce4db51be1d90fcb1fb500fa78b3
Again, I'm no pythonista, so I may have muddled this.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] xfs: Change URL for the project in xfs.txt
From: Darrick J. Wong @ 2018-03-02 17:24 UTC (permalink / raw)
To: Masanari Iida; +Cc: linux-xfs, linux-kernel, corbet, linux-doc
In-Reply-To: <20180302133013.9744-1-standby24x7@gmail.com>
On Fri, Mar 02, 2018 at 10:30:13PM +0900, Masanari Iida wrote:
> The oss.sgi.com doesn't exist any more.
> Change it to current project URL, https://xfs.wiki.kernel.org/
>
> Signed-off-by: Masanari Iida <standby24x7@gmail.com>
> ---
> Documentation/filesystems/xfs.txt | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Documentation/filesystems/xfs.txt b/Documentation/filesystems/xfs.txt
> index 3b9b5c149f32..4d9ff0a7f8e1 100644
> --- a/Documentation/filesystems/xfs.txt
> +++ b/Documentation/filesystems/xfs.txt
> @@ -9,7 +9,7 @@ variable block sizes, is extent based, and makes extensive use of
> Btrees (directories, extents, free space) to aid both performance
> and scalability.
>
> -Refer to the documentation at http://oss.sgi.com/projects/xfs/
> +Refer to the documentation at https://xfs.wiki.kernel.org/
> for further details. This implementation is on-disk compatible
> with the IRIX version of XFS.
Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
--D
>
> --
> 2.16.2.345.g7e31236f652a
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH] Input: trackpoint: document sysfs interface
From: Aishwarya Pant @ 2018-03-02 17:30 UTC (permalink / raw)
To: Dmitry Torokhov, linux-input, linux-kernel, Jonathan Corbet,
Greg KH
Cc: Julia Lawall, linux-doc
Descriptions have been collected from git commit logs, code commits and
the TrackPoint System Version 4.0 Engineering Specification.
Signed-off-by: Aishwarya Pant <aishpant@gmail.com>
---
.../ABI/testing/sysfs-devices-platform-trackpoint | 115 +++++++++++++++++++++
1 file changed, 115 insertions(+)
create mode 100644 Documentation/ABI/testing/sysfs-devices-platform-trackpoint
diff --git a/Documentation/ABI/testing/sysfs-devices-platform-trackpoint b/Documentation/ABI/testing/sysfs-devices-platform-trackpoint
new file mode 100644
index 000000000000..df11901a6b3d
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-devices-platform-trackpoint
@@ -0,0 +1,115 @@
+What: /sys/devices/platform/i8042/.../sensitivity
+Date: Aug, 2005
+KernelVersion: 2.6.14
+Contact: linux-input@vger.kernel.org
+Description:
+ (RW) Trackpoint sensitivity.
+
+What: /sys/devices/platform/i8042/.../intertia
+Date: Aug, 2005
+KernelVersion: 2.6.14
+Contact: linux-input@vger.kernel.org
+Description:
+ (RW) Negative inertia factor. High values cause the cursor to
+ snap backward when the trackpoint is released.
+
+What: /sys/devices/platform/i8042/.../reach
+Date: Aug, 2005
+KernelVersion: 2.6.14
+Contact: linux-input@vger.kernel.org
+Description:
+ (RW) Backup range for z-axis press.
+
+What: /sys/devices/platform/i8042/.../draghys
+Date: Aug, 2005
+KernelVersion: 2.6.14
+Contact: linux-input@vger.kernel.org
+Description:
+ (RW) The drag hysteresis controls how hard it is to drag with
+ z-axis pressed.
+
+What: /sys/devices/platform/i8042/.../mindrag
+Date: Aug, 2005
+KernelVersion: 2.6.14
+Contact: linux-input@vger.kernel.org
+Description:
+ (RW) Minimum amount of force needed to trigger dragging.
+
+What: /sys/devices/platform/i8042/.../speed
+Date: Aug, 2005
+KernelVersion: 2.6.14
+Contact: linux-input@vger.kernel.org
+Description:
+ (RW) Speed of the trackpoint cursor.
+
+What: /sys/devices/platform/i8042/.../thresh
+Date: Aug, 2005
+KernelVersion: 2.6.14
+Contact: linux-input@vger.kernel.org
+Description:
+ (RW) Minimum value for z-axis force required to trigger a press
+ or release, relative to the running average.
+
+What: /sys/devices/platform/i8042/.../upthresh
+Date: Aug, 2005
+KernelVersion: 2.6.14
+Contact: linux-input@vger.kernel.org
+Description:
+ (RW) The offset from the running average required to generate a
+ select (click) on z-axis on release.
+
+What: /sys/devices/platform/i8042/.../ztime
+Date: Aug, 2005
+KernelVersion: 2.6.14
+Contact: linux-input@vger.kernel.org
+Description:
+ (RW) This attribute determines how sharp a press has to be in
+ order to be recognized.
+
+What: /sys/devices/platform/i8042/.../jenks
+Date: Aug, 2005
+KernelVersion: 2.6.14
+Contact: linux-input@vger.kernel.org
+Description:
+ (RW) Minimum curvature in degrees required to generate a double
+ click without a release.
+
+What: /sys/devices/platform/i8042/.../skipback
+Date: Aug, 2005
+KernelVersion: 2.6.14
+Contact: linux-input@vger.kernel.org
+Description:
+ (RW) When the skipback bit is set, backup cursor movement during
+ releases from drags will be suppressed. The default value for
+ this bit is 0.
+
+What: /sys/devices/platform/i8042/.../ext_dev
+Date: Aug, 2005
+KernelVersion: 2.6.14
+Contact: linux-input@vger.kernel.org
+Description:
+ (RW) Disable (0) or enable (1) external pointing device.
+
+What: /sys/devices/platform/i8042/.../press_to_select
+Date: Aug, 2005
+KernelVersion: 2.6.14
+Contact: linux-input@vger.kernel.org
+Description:
+ (RW) Writing a value of 1 to this file will enable the Press to
+ Select functions like tapping the control stick to simulate a
+ left click, and writing 0 will disable it.
+
+What: /sys/devices/platform/i8042/.../drift_time
+Date: Dec, 2014
+KernelVersion: 3.19
+Contact: linux-input@vger.kernel.org
+Description:
+ (RW) This parameter controls the period of time to test for a
+ ‘hands off’ condition (i.e. when no force is applied) before a
+ drift (noise) calibration occurs.
+
+ IBM Trackpoints have a feature to compensate for drift by
+ recalibrating themselves periodically. By default, if for 0.5
+ seconds there is no change in position, it's used as the new
+ zero. This duration is too low. Often, the calibration happens
+ when the trackpoint is in fact being used.
--
2.16.2
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* Re: [PATCH v2] Documentation/sphinx: Fix Directive import error
From: Matthew Wilcox @ 2018-03-02 18:28 UTC (permalink / raw)
To: Jani Nikula
Cc: Takashi Iwai, Jonathan Corbet, Jiri Slaby, linux-doc,
linux-kernel
In-Reply-To: <20180302164254.GB31400@bombadil.infradead.org>
On Fri, Mar 02, 2018 at 08:42:54AM -0800, Matthew Wilcox wrote:
> On Fri, Mar 02, 2018 at 06:01:50PM +0200, Jani Nikula wrote:
> > On Fri, 02 Mar 2018, Takashi Iwai <tiwai@suse.de> wrote:
> > > The sphinx.util.compat for Directive stuff was deprecated in the
> > > recent Sphinx version, and now we get a build error.
> > >
> > > Let's import from the new place, docutils.parsers.rst, while keeping
> > > the old sphinx.util.compat as fallback.
> > >
> > > Bugzilla: https://bugzilla.opensuse.org/show_bug.cgi?id=1083694
> > > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > > ---
> > > v1->v2: Change the fallback order as Matthew suggested, the new one at first
> >
> > So this crossed my mind as well... and then I thought it'll probably
> > succeed on older Sphinx, and the fallback is not needed. The question
> > is, are these equal? Can we just import from docutils.parsers.rst?
>
> I found a github page which implies that docutils.parsers.rst.Directive
> was added 12 years ago (!) so we're probably safe to rely on it:
>
> https://github.com/docutils-mirror/docutils/commit/9649abee47b4ce4db51be1d90fcb1fb500fa78b3
>
> Again, I'm no pythonista, so I may have muddled this.
I spent some time delving through the Sphinx codebase. The compat
version was added so you could use Directive with docutils 0.4 onwards.
docutils 0.5 onwards has Directive. So as long as we're comfortable
mandating docutils from 2009, we're fine ;-)
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v2] earlycon: Allow specifying a uartclk in options
From: Daniel Kurtz @ 2018-03-02 18:35 UTC (permalink / raw)
To: Andy Shevchenko
Cc: adurbin, Brian Norris, Jonathan Corbet, Greg Kroah-Hartman,
jslaby, mingo, Thomas Gleixner, Christoffer Dall, Paul McKenney,
marc.zyngier, frederic, dwmw, tom.saeger, zohar, alexander.levin,
linux-doc, linux-kernel, linux-serial
In-Reply-To: <CAHp75VfL82ujwBa1h0B8nEgBtFcBJPmS9ir7H5YWByDgxFGskA@mail.gmail.com>
On Thu, Mar 1, 2018 at 1:02 PM Andy Shevchenko <andy.shevchenko@gmail.com>
wrote:
> On Thu, Mar 1, 2018 at 9:22 PM, Daniel Kurtz <djkurtz@chromium.org> wrote:
> > On Thu, Mar 1, 2018 at 11:47 AM Andy Shevchenko <
andy.shevchenko@gmail.com>
> > wrote:
> > "earlycon simply does not utilize the information".
> >
> > earlycon parses iotype, mapbase and baud (from options). However, it is
> > hard-coded to assume that the clock used to generate the UART bitclock
is
> > always "BASE_BAUD * 16" (1843200). While this may be true for many
UARTs,
> > it isn't true for AMD's CZ/ST which has a 8250_dw and uses a fixed 48
MHz
> > clock. The main 8250_dw driver uses devm_clk_get to get the "baudclk"
and
> > uses its rate to initialize uartclk. For AMD CZ/ST, this "baudclk" is
> > actually a set up in acpi_apd.c when there is an acpi match for
"AMD0020",
> > with a rate read from the .fixed_clk_rate param of the corresponding
> > apd_device_desc.
> >
> > This patch attempts to add a way to inform earlycon about this clock.
As
> > noted above, the information is actually already in the kernel and used
by
> > 8250_dw - I would happy be to hear recommendations for wiring this data
> > into earlycon that doesn't require adding another command line arg.
> And it should not require that for sure!
> I would look to this later. It's late here. I need to do a bit of
> research for the answer.
> > I see that support was also added recently to earlycon to let it use
ACPI
> > SPCR to choose a console and configure its parameters... but AFAICT,
this
> > path also doesn't allow specifying the uart clock.
> Fix your firmware then. It should set console to 115200 like (almost)
> everyone does.
> Okay, configures a necessary IPs to feed UART with expected 1.8432M clock.
The console is 115200 when it is enabled. However, the firmware does not
always enable it by default.
The problem is that the UART IP block has a fixed 48 MHz input clock, but
earlycon assumes this clock is always 1843200.
I looked a bit further, and I think this patch (or something similar) is
still required to teach generic earlycon how to handle an explicit
port->uartclk (ie, one that is not 1843200).
The extended string can then be explicitly set on the kernel command line
for this kind of hardware.
In addition, we can add another patch with a new quirk detector in
drivers/acpi/spcr.c:acpi_parse_spcr() to handle this hardware.
acpi_parse_spcr() can then use the extended option string to pass in the
appropriate UART clock to setup_eralycon().
This would again allow a user to just use the simple command line parameter
"earlycon" if the device's firmware has a correctly confiured ACPI SPCR
table.
Thanks,
-Dan
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH v3] Documentation/sphinx: Fix Directive import error
From: Matthew Wilcox @ 2018-03-02 18:40 UTC (permalink / raw)
To: Takashi Iwai
Cc: Jonathan Corbet, Jani Nikula, Jiri Slaby, linux-doc, linux-kernel
In-Reply-To: <20180302152831.11510-1-tiwai@suse.de>
From: Matthew Wilcox <mawilcox@microsoft.com>
Sphinx 1.7 removed sphinx.util.compat.Directive so people
who have upgraded cannot build the documentation. Switch to
docutils.parsers.rst.Directive which has been available since
docutils 0.5 released in 2009.
Bugzilla: https://bugzilla.opensuse.org/show_bug.cgi?id=1083694
Co-developed-by: Takashi Iwai <tiwai@suse.de>
Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
diff --git a/Documentation/sphinx/kerneldoc.py b/Documentation/sphinx/kerneldoc.py
index 39aa9e8697cc..fbedcc39460b 100644
--- a/Documentation/sphinx/kerneldoc.py
+++ b/Documentation/sphinx/kerneldoc.py
@@ -36,8 +36,7 @@ import glob
from docutils import nodes, statemachine
from docutils.statemachine import ViewList
-from docutils.parsers.rst import directives
-from sphinx.util.compat import Directive
+from docutils.parsers.rst import directives, Directive
from sphinx.ext.autodoc import AutodocReporter
__version__ = '1.0'
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* Re: [PATCH v3] Documentation/sphinx: Fix Directive import error
From: Jani Nikula @ 2018-03-02 19:00 UTC (permalink / raw)
To: Matthew Wilcox, Takashi Iwai
Cc: Jonathan Corbet, Jiri Slaby, linux-doc, linux-kernel
In-Reply-To: <20180302184014.GG31400@bombadil.infradead.org>
On Fri, 02 Mar 2018, Matthew Wilcox <willy@infradead.org> wrote:
> From: Matthew Wilcox <mawilcox@microsoft.com>
>
> Sphinx 1.7 removed sphinx.util.compat.Directive so people
> who have upgraded cannot build the documentation. Switch to
> docutils.parsers.rst.Directive which has been available since
> docutils 0.5 released in 2009.
>
> Bugzilla: https://bugzilla.opensuse.org/show_bug.cgi?id=1083694
> Co-developed-by: Takashi Iwai <tiwai@suse.de>
> Signed-off-by: Matthew Wilcox <mawilcox@microsoft.com>
I think this is the best approach.
FWIW,
Acked-by: Jani Nikula <jani.nikula@intel.com>
>
> diff --git a/Documentation/sphinx/kerneldoc.py b/Documentation/sphinx/kerneldoc.py
> index 39aa9e8697cc..fbedcc39460b 100644
> --- a/Documentation/sphinx/kerneldoc.py
> +++ b/Documentation/sphinx/kerneldoc.py
> @@ -36,8 +36,7 @@ import glob
>
> from docutils import nodes, statemachine
> from docutils.statemachine import ViewList
> -from docutils.parsers.rst import directives
> -from sphinx.util.compat import Directive
> +from docutils.parsers.rst import directives, Directive
> from sphinx.ext.autodoc import AutodocReporter
>
> __version__ = '1.0'
--
Jani Nikula, Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v3 2/4] soc: qcom: Add GENI based QUP Wrapper driver
From: Stephen Boyd @ 2018-03-02 20:41 UTC (permalink / raw)
To: Karthikeyan Ramasubramanian, andy.gross, corbet, david.brown,
gregkh, mark.rutland, robh+dt, wsa
Cc: Karthikeyan Ramasubramanian, linux-doc, linux-arm-msm, devicetree,
linux-i2c, linux-serial, jslaby, evgreen, acourbot, Sagar Dharia,
Girish Mahadevan
In-Reply-To: <1519781889-16117-3-git-send-email-kramasub@codeaurora.org>
Quoting Karthikeyan Ramasubramanian (2018-02-27 17:38:07)
> This driver manages the Generic Interface (GENI) firmware based Qualcomm
> Universal Peripheral (QUP) Wrapper. GENI based QUP is the next generation
> programmable module composed of multiple Serial Engines (SE) and supports
> a wide range of serial interfaces like UART, SPI, I2C, I3C, etc. This
> driver also enables managing the serial interface independent aspects of
> Serial Engines.
>
> Signed-off-by: Karthikeyan Ramasubramanian <kramasub@codeaurora.org>
> Signed-off-by: Sagar Dharia <sdharia@codeaurora.org>
> Signed-off-by: Girish Mahadevan <girishm@codeaurora.org>
> ---
> drivers/soc/qcom/Kconfig | 9 +
> drivers/soc/qcom/Makefile | 1 +
> drivers/soc/qcom/qcom-geni-se.c | 971 ++++++++++++++++++++++++++++++++++++++++
> include/linux/qcom-geni-se.h | 247 ++++++++++
> 4 files changed, 1228 insertions(+)
> create mode 100644 drivers/soc/qcom/qcom-geni-se.c
> create mode 100644 include/linux/qcom-geni-se.h
>
> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index e050eb8..cc460d0 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -3,6 +3,15 @@
> #
> menu "Qualcomm SoC drivers"
>
> +config QCOM_GENI_SE
> + tristate "QCOM GENI Serial Engine Driver"
> + depends on ARCH_QCOM
Add || COMPILE_TEST?
> + help
> + This module is used to manage Generic Interface (GENI) firmware based
s/module/driver?
> + Qualcomm Technologies, Inc. Universal Peripheral (QUP) Wrapper. This
> + module is also used to manage the common aspects of multiple Serial
s/module/driver?
> + Engines present in the QUP.
> +
> config QCOM_GLINK_SSR
> tristate "Qualcomm Glink SSR driver"
> depends on RPMSG
> diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
> new file mode 100644
> index 0000000..61335b8
> --- /dev/null
> +++ b/drivers/soc/qcom/qcom-geni-se.c
> @@ -0,0 +1,971 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
> +
> +#include <linux/clk.h>
> +#include <linux/slab.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/qcom-geni-se.h>
#include <linux/platform_device.h>
> +
> +/**
> + * DOC: Overview
> + *
> + * Generic Interface (GENI) Serial Engine (SE) Wrapper driver is introduced
> + * to manage GENI firmware based Qualcomm Universal Peripheral (QUP) Wrapper
> + * controller. QUP Wrapper is designed to support various serial bus protocols
> + * like UART, SPI, I2C, I3C, etc.
> + */
> +
> +/**
> + * DOC: Hardware description
> + *
> + * GENI based QUP is a highly-flexible and programmable module for supporting
> + * a wide range of serial interfaces like UART, SPI, I2C, I3C, etc. A single
> + * QUP module can provide upto 8 Serial Interfaces, using its internal
> + * Serial Engines. The actual configuration is determined by the target
> + * platform configuration. The protocol supported by each interface is
> + * determined by the firmware loaded to the Serial Engine. Each SE consists
> + * of a DMA Engine and GENI sub modules which enable Serial Engines to
> + * support FIFO and DMA modes of operation.
> + *
> + *
> + * +-----------------------------------------+
> + * |QUP Wrapper |
> + * | +----------------------------+ |
> + * --QUP & SE Clocks--> | Serial Engine N | +-IO------>
> + * | | ... | | Interface
> + * <---Clock Perf.----+ +----+-----------------------+ | |
> + * State Interface | | Serial Engine 1 | | |
> + * | | | | |
> + * | | | | |
> + * <--------AHB-------> | | | |
> + * | | +----+ |
> + * | | | |
> + * | | | |
> + * <------SE IRQ------+ +----------------------------+ |
> + * | |
> + * +-----------------------------------------+
> + *
> + * Figure 1: GENI based QUP Wrapper
The code talks about primary and secondary sequencers, but this hardware
description doesn't talk about it. Can you add some more information
here about that aspect too?
> + */
> +
> +/**
> + * DOC: Software description
> + *
> + * GENI SE Wrapper driver is structured into 2 parts:
> + *
> + * geni_wrapper represents QUP Wrapper controller. This part of the driver
> + * manages QUP Wrapper information such as hardware version, clock
> + * performance table that is common to all the internal Serial Engines.
> + *
> + * geni_se represents Serial Engine. This part of the driver manages Serial
> + * Engine information such as clocks, containing QUP Wrapper etc. This part
Insert a comma here ^
> + * of driver also supports operations(eg. initialize the concerned Serial
Space ^
> + * Engine, select between FIFO and DMA mode of operation etc.) that are
> + * common to all the Serial Engines and are independent of Serial Interfaces.
Why are Serial Interfaces and Serial Engine always capitalized?
> + */
> +
> +#define MAX_CLK_PERF_LEVEL 32
> +#define NUM_AHB_CLKS 2
> +static const char m_ahb_clk[] = "m-ahb";
> +static const char s_ahb_clk[] = "s-ahb";
These are used in one place. Inline them?
> +
> +/**
> + * @struct geni_wrapper - Data structure to represent the QUP Wrapper Core
> + * @dev: Device pointer of the QUP wrapper core.
> + * @base: Base address of this instance of QUP wrapper core.
> + * @ahb_clks: Handle to the primary & secondary AHB clocks.
> + * @lock: Lock to protect the device elements.
What does 'device elements' mean?
> + * @num_clk_levels: Number of valid clock levels in clk_perf_tbl.
> + * @clk_perf_tbl: Table of clock frequency input to Serial Engine clock.
Kernel-doc normally doesn't have a full-stop on member descriptions.
> + */
> +struct geni_wrapper {
> + struct device *dev;
> + void __iomem *base;
> + struct clk_bulk_data ahb_clks[NUM_AHB_CLKS];
> + struct mutex lock;
> + unsigned int num_clk_levels;
> + unsigned long *clk_perf_tbl;
> +};
> +
> +/* Offset of QUP Hardware Version Register */
Useless comment?
> +#define QUP_HW_VER_REG 0x4
> +
> +#define HW_VER_MAJOR_MASK GENMASK(31, 28)
> +#define HW_VER_MAJOR_SHFT 28
> +#define HW_VER_MINOR_MASK GENMASK(27, 16)
> +#define HW_VER_MINOR_SHFT 16
> +#define HW_VER_STEP_MASK GENMASK(15, 0)
> +
> +/* Common SE registers */
> +#define GENI_INIT_CFG_REVISION 0x0
> +#define GENI_S_INIT_CFG_REVISION 0x4
> +#define GENI_OUTPUT_CTRL 0x24
> +#define GENI_CGC_CTRL 0x28
> +#define GENI_CLK_CTRL_RO 0x60
> +#define GENI_IF_DISABLE_RO 0x64
> +#define GENI_FW_REVISION_RO 0x68
> +#define GENI_FW_S_REVISION_RO 0x6c
> +#define SE_GENI_BYTE_GRAN 0x254
> +#define SE_GENI_TX_PACKING_CFG0 0x260
> +#define SE_GENI_TX_PACKING_CFG1 0x264
> +#define SE_GENI_RX_PACKING_CFG0 0x284
> +#define SE_GENI_RX_PACKING_CFG1 0x288
> +#define SE_GENI_M_GP_LENGTH 0x910
> +#define SE_GENI_S_GP_LENGTH 0x914
> +#define SE_DMA_TX_PTR_L 0xc30
> +#define SE_DMA_TX_PTR_H 0xc34
> +#define SE_DMA_TX_ATTR 0xc38
> +#define SE_DMA_TX_LEN 0xc3c
> +#define SE_DMA_TX_IRQ_EN 0xc48
> +#define SE_DMA_TX_IRQ_EN_SET 0xc4c
> +#define SE_DMA_TX_IRQ_EN_CLR 0xc50
> +#define SE_DMA_TX_LEN_IN 0xc54
> +#define SE_DMA_TX_MAX_BURST 0xc5c
> +#define SE_DMA_RX_PTR_L 0xd30
> +#define SE_DMA_RX_PTR_H 0xd34
> +#define SE_DMA_RX_ATTR 0xd38
> +#define SE_DMA_RX_LEN 0xd3c
> +#define SE_DMA_RX_IRQ_EN 0xd48
> +#define SE_DMA_RX_IRQ_EN_SET 0xd4c
> +#define SE_DMA_RX_IRQ_EN_CLR 0xd50
> +#define SE_DMA_RX_LEN_IN 0xd54
> +#define SE_DMA_RX_MAX_BURST 0xd5c
> +#define SE_DMA_RX_FLUSH 0xd60
> +#define SE_GSI_EVENT_EN 0xe18
> +#define SE_IRQ_EN 0xe1c
> +#define SE_HW_PARAM_0 0xe24
> +#define SE_HW_PARAM_1 0xe28
> +#define SE_DMA_GENERAL_CFG 0xe30
> +
> +/* GENI_OUTPUT_CTRL fields */
> +#define DEFAULT_IO_OUTPUT_CTRL_MSK GENMASK(6, 0)
> +
> +/* GENI_CGC_CTRL fields */
> +#define CFG_AHB_CLK_CGC_ON BIT(0)
> +#define CFG_AHB_WR_ACLK_CGC_ON BIT(1)
> +#define DATA_AHB_CLK_CGC_ON BIT(2)
> +#define SCLK_CGC_ON BIT(3)
> +#define TX_CLK_CGC_ON BIT(4)
> +#define RX_CLK_CGC_ON BIT(5)
> +#define EXT_CLK_CGC_ON BIT(6)
> +#define PROG_RAM_HCLK_OFF BIT(8)
> +#define PROG_RAM_SCLK_OFF BIT(9)
> +#define DEFAULT_CGC_EN GENMASK(6, 0)
> +
> +/* FW_REVISION_RO fields */
> +#define FW_REV_PROTOCOL_MSK GENMASK(15, 8)
> +#define FW_REV_PROTOCOL_SHFT 8
> +
> +/* SE_GSI_EVENT_EN fields */
> +#define DMA_RX_EVENT_EN BIT(0)
> +#define DMA_TX_EVENT_EN BIT(1)
> +#define GENI_M_EVENT_EN BIT(2)
> +#define GENI_S_EVENT_EN BIT(3)
> +
> +/* SE_IRQ_EN fields */
> +#define DMA_RX_IRQ_EN BIT(0)
> +#define DMA_TX_IRQ_EN BIT(1)
> +#define GENI_M_IRQ_EN BIT(2)
> +#define GENI_S_IRQ_EN BIT(3)
> +
> +/* SE_HW_PARAM_0 fields */
> +#define TX_FIFO_WIDTH_MSK GENMASK(29, 24)
> +#define TX_FIFO_WIDTH_SHFT 24
> +#define TX_FIFO_DEPTH_MSK GENMASK(21, 16)
> +#define TX_FIFO_DEPTH_SHFT 16
> +
> +/* SE_HW_PARAM_1 fields */
> +#define RX_FIFO_WIDTH_MSK GENMASK(29, 24)
> +#define RX_FIFO_WIDTH_SHFT 24
> +#define RX_FIFO_DEPTH_MSK GENMASK(21, 16)
> +#define RX_FIFO_DEPTH_SHFT 16
> +
> +/* SE_DMA_GENERAL_CFG */
> +#define DMA_RX_CLK_CGC_ON BIT(0)
> +#define DMA_TX_CLK_CGC_ON BIT(1)
> +#define DMA_AHB_SLV_CFG_ON BIT(2)
> +#define AHB_SEC_SLV_CLK_CGC_ON BIT(3)
> +#define DUMMY_RX_NON_BUFFERABLE BIT(4)
> +#define RX_DMA_ZERO_PADDING_EN BIT(5)
> +#define RX_DMA_IRQ_DELAY_MSK GENMASK(8, 6)
> +#define RX_DMA_IRQ_DELAY_SHFT 6
> +
> +/**
> + * geni_se_get_qup_hw_version() - Read the QUP wrapper Hardware version
> + * @se: Pointer to the corresponding Serial Engine.
> + * @major: Buffer for Major Version field.
> + * @minor: Buffer for Minor Version field.
> + * @step: Buffer for Step Version field.
> + */
> +void geni_se_get_qup_hw_version(struct geni_se *se, unsigned int *major,
> + unsigned int *minor, unsigned int *step)
> +{
> + unsigned int version;
> + struct geni_wrapper *wrapper = se->wrapper;
> +
> + version = readl_relaxed(wrapper->base + QUP_HW_VER_REG);
> + *major = (version & HW_VER_MAJOR_MASK) >> HW_VER_MAJOR_SHFT;
> + *minor = (version & HW_VER_MINOR_MASK) >> HW_VER_MINOR_SHFT;
> + *step = version & HW_VER_STEP_MASK;
> +}
> +EXPORT_SYMBOL(geni_se_get_qup_hw_version);
Is this used?
> +
> +/**
> + * geni_se_read_proto() - Read the protocol configured for a Serial Engine
> + * @se: Pointer to the concerned Serial Engine.
> + *
> + * Return: Protocol value as configured in the serial engine.
> + */
> +u32 geni_se_read_proto(struct geni_se *se)
> +{
> + u32 val;
> +
> + val = readl_relaxed(se->base + GENI_FW_REVISION_RO);
> +
> + return (val & FW_REV_PROTOCOL_MSK) >> FW_REV_PROTOCOL_SHFT;
> +}
> +EXPORT_SYMBOL(geni_se_read_proto);
Is this API really needed outside of this file? It would seem like the
drivers that implement the protocol, which are child devices, would only
use this API to confirm that the protocol chosen is for their particular
protocol.
> +
> +static void geni_se_io_set_mode(void __iomem *base)
> +{
> + u32 val;
> +
> + val = readl_relaxed(base + SE_IRQ_EN);
> + val |= (GENI_M_IRQ_EN | GENI_S_IRQ_EN);
> + val |= (DMA_TX_IRQ_EN | DMA_RX_IRQ_EN);
Drop useless parenthesis please.
> + writel_relaxed(val, base + SE_IRQ_EN);
> +
> + val = readl_relaxed(base + SE_GENI_DMA_MODE_EN);
> + val &= ~GENI_DMA_MODE_EN;
> + writel_relaxed(val, base + SE_GENI_DMA_MODE_EN);
> +
> + writel_relaxed(0, base + SE_GSI_EVENT_EN);
> +}
> +
> +static void geni_se_io_init(void __iomem *base)
> +{
> + u32 val;
> +
> + val = readl_relaxed(base + GENI_CGC_CTRL);
> + val |= DEFAULT_CGC_EN;
> + writel_relaxed(val, base + GENI_CGC_CTRL);
> +
> + val = readl_relaxed(base + SE_DMA_GENERAL_CFG);
> + val |= AHB_SEC_SLV_CLK_CGC_ON | DMA_AHB_SLV_CFG_ON;
> + val |= DMA_TX_CLK_CGC_ON | DMA_RX_CLK_CGC_ON;
> + writel_relaxed(val, base + SE_DMA_GENERAL_CFG);
> +
> + writel_relaxed(DEFAULT_IO_OUTPUT_CTRL_MSK, base + GENI_OUTPUT_CTRL);
> + writel_relaxed(FORCE_DEFAULT, base + GENI_FORCE_DEFAULT_REG);
> +}
> +
> +/**
> + * geni_se_init() - Initialize the GENI Serial Engine
> + * @se: Pointer to the concerned Serial Engine.
> + * @rx_wm: Receive watermark, in units of FIFO words.
> + * @rx_rfr_wm: Ready-for-receive watermark, in units of FIFO words.
> + *
> + * This function is used to initialize the GENI serial engine, configure
> + * receive watermark and ready-for-receive watermarks.
> + *
> + * Return: 0 on success, standard Linux error codes on failure/error.
It never returns an error. Change to void?
> + */
> +int geni_se_init(struct geni_se *se, u32 rx_wm, u32 rx_rfr)
> +{
> + u32 val;
> +
> + geni_se_io_init(se->base);
> + geni_se_io_set_mode(se->base);
> +
> + writel_relaxed(rx_wm, se->base + SE_GENI_RX_WATERMARK_REG);
> + writel_relaxed(rx_rfr, se->base + SE_GENI_RX_RFR_WATERMARK_REG);
> +
> + val = readl_relaxed(se->base + SE_GENI_M_IRQ_EN);
> + val |= M_COMMON_GENI_M_IRQ_EN;
> + writel_relaxed(val, se->base + SE_GENI_M_IRQ_EN);
> +
> + val = readl_relaxed(se->base + SE_GENI_S_IRQ_EN);
> + val |= S_COMMON_GENI_S_IRQ_EN;
> + writel_relaxed(val, se->base + SE_GENI_S_IRQ_EN);
> + return 0;
> +}
> +EXPORT_SYMBOL(geni_se_init);
> +
> +static void geni_se_select_fifo_mode(struct geni_se *se)
> +{
> + u32 proto = geni_se_read_proto(se);
> + u32 val;
> +
> + writel_relaxed(0, se->base + SE_GSI_EVENT_EN);
> + writel_relaxed(0xffffffff, se->base + SE_GENI_M_IRQ_CLEAR);
> + writel_relaxed(0xffffffff, se->base + SE_GENI_S_IRQ_CLEAR);
> + writel_relaxed(0xffffffff, se->base + SE_DMA_TX_IRQ_CLR);
> + writel_relaxed(0xffffffff, se->base + SE_DMA_RX_IRQ_CLR);
> + writel_relaxed(0xffffffff, se->base + SE_IRQ_EN);
> +
> + val = readl_relaxed(se->base + SE_GENI_M_IRQ_EN);
> + if (proto != GENI_SE_UART) {
> + val |= M_CMD_DONE_EN | M_TX_FIFO_WATERMARK_EN;
> + val |= M_RX_FIFO_WATERMARK_EN | M_RX_FIFO_LAST_EN;
> + }
> + writel_relaxed(val, se->base + SE_GENI_M_IRQ_EN);
> +
> + val = readl_relaxed(se->base + SE_GENI_S_IRQ_EN);
> + if (proto != GENI_SE_UART)
> + val |= S_CMD_DONE_EN;
> + writel_relaxed(val, se->base + SE_GENI_S_IRQ_EN);
> +
> + val = readl_relaxed(se->base + SE_GENI_DMA_MODE_EN);
> + val &= ~GENI_DMA_MODE_EN;
> + writel_relaxed(val, se->base + SE_GENI_DMA_MODE_EN);
> +}
> +
> +static void geni_se_select_dma_mode(struct geni_se *se)
> +{
> + u32 val;
> +
> + writel_relaxed(0, se->base + SE_GSI_EVENT_EN);
> + writel_relaxed(0xffffffff, se->base + SE_GENI_M_IRQ_CLEAR);
> + writel_relaxed(0xffffffff, se->base + SE_GENI_S_IRQ_CLEAR);
> + writel_relaxed(0xffffffff, se->base + SE_DMA_TX_IRQ_CLR);
> + writel_relaxed(0xffffffff, se->base + SE_DMA_RX_IRQ_CLR);
> + writel_relaxed(0xffffffff, se->base + SE_IRQ_EN);
> +
> + val = readl_relaxed(se->base + SE_GENI_DMA_MODE_EN);
> + val |= GENI_DMA_MODE_EN;
> + writel_relaxed(val, se->base + SE_GENI_DMA_MODE_EN);
> +}
> +
> +/**
> + * geni_se_select_mode() - Select the serial engine transfer mode
> + * @se: Pointer to the concerned Serial Engine.
> + * @mode: Transfer mode to be selected.
> + */
> +void geni_se_select_mode(struct geni_se *se, int mode)
enum mode?
> +{
> + WARN_ON(mode != GENI_SE_FIFO && mode != GENI_SE_DMA);
> +
> + switch (mode) {
> + case GENI_SE_FIFO:
> + geni_se_select_fifo_mode(se);
> + break;
> + case GENI_SE_DMA:
> + geni_se_select_dma_mode(se);
> + break;
> + }
> +}
> +EXPORT_SYMBOL(geni_se_select_mode);
> +
> +/**
> + * geni_se_setup_m_cmd() - Setup the primary sequencer
> + * @se: Pointer to the concerned Serial Engine.
> + * @cmd: Command/Operation to setup in the primary sequencer.
> + * @params: Parameter for the sequencer command.
> + *
> + * This function is used to configure the primary sequencer with the
> + * command and its associated parameters.
> + */
> +void geni_se_setup_m_cmd(struct geni_se *se, u32 cmd, u32 params)
> +{
> + u32 m_cmd;
> +
> + m_cmd = (cmd << M_OPCODE_SHFT) | (params & M_PARAMS_MSK);
> + writel_relaxed(m_cmd, se->base + SE_GENI_M_CMD0);
> +}
> +EXPORT_SYMBOL(geni_se_setup_m_cmd);
> +
> +/**
> + * geni_se_setup_s_cmd() - Setup the secondary sequencer
> + * @se: Pointer to the concerned Serial Engine.
> + * @cmd: Command/Operation to setup in the secondary sequencer.
> + * @params: Parameter for the sequencer command.
> + *
> + * This function is used to configure the secondary sequencer with the
> + * command and its associated parameters.
> + */
> +void geni_se_setup_s_cmd(struct geni_se *se, u32 cmd, u32 params)
> +{
> + u32 s_cmd;
> +
> + s_cmd = readl_relaxed(se->base + SE_GENI_S_CMD0);
> + s_cmd &= ~(S_OPCODE_MSK | S_PARAMS_MSK);
> + s_cmd |= (cmd << S_OPCODE_SHFT);
> + s_cmd |= (params & S_PARAMS_MSK);
> + writel_relaxed(s_cmd, se->base + SE_GENI_S_CMD0);
> +}
> +EXPORT_SYMBOL(geni_se_setup_s_cmd);
> +
> +/**
> + * geni_se_cancel_m_cmd() - Cancel the command configured in the primary
> + * sequencer
> + * @se: Pointer to the concerned Serial Engine.
> + *
> + * This function is used to cancel the currently configured command in the
> + * primary sequencer.
> + */
> +void geni_se_cancel_m_cmd(struct geni_se *se)
> +{
> + writel_relaxed(M_GENI_CMD_CANCEL, se->base + SE_GENI_M_CMD_CTRL_REG);
> +}
> +EXPORT_SYMBOL(geni_se_cancel_m_cmd);
> +
> +/**
> + * geni_se_cancel_s_cmd() - Cancel the command configured in the secondary
> + * sequencer
> + * @se: Pointer to the concerned Serial Engine.
> + *
> + * This function is used to cancel the currently configured command in the
> + * secondary sequencer.
> + */
> +void geni_se_cancel_s_cmd(struct geni_se *se)
> +{
> + writel_relaxed(S_GENI_CMD_CANCEL, se->base + SE_GENI_S_CMD_CTRL_REG);
> +}
> +EXPORT_SYMBOL(geni_se_cancel_s_cmd);
> +
> +/**
> + * geni_se_abort_m_cmd() - Abort the command configured in the primary sequencer
> + * @se: Pointer to the concerned Serial Engine.
> + *
> + * This function is used to force abort the currently configured command in the
> + * primary sequencer.
> + */
> +void geni_se_abort_m_cmd(struct geni_se *se)
> +{
> + writel_relaxed(M_GENI_CMD_ABORT, se->base + SE_GENI_M_CMD_CTRL_REG);
> +}
> +EXPORT_SYMBOL(geni_se_abort_m_cmd);
> +
> +/**
> + * geni_se_abort_s_cmd() - Abort the command configured in the secondary
> + * sequencer
> + * @se: Pointer to the concerned Serial Engine.
> + *
> + * This function is used to force abort the currently configured command in the
> + * secondary sequencer.
> + */
> +void geni_se_abort_s_cmd(struct geni_se *se)
> +{
> + writel_relaxed(S_GENI_CMD_ABORT, se->base + SE_GENI_S_CMD_CTRL_REG);
> +}
> +EXPORT_SYMBOL(geni_se_abort_s_cmd);
Can these one-liners go into the header file and be marked static
inline? I would guess call-sites already have se->base in hand, so
registers might be reused more efficiently and it may result in a single
store instruction instead of a branch and load/store.
> +
> +/**
> + * geni_se_get_tx_fifo_depth() - Get the TX fifo depth of the serial engine
> + * @se: Pointer to the concerned Serial Engine.
> + *
> + * This function is used to get the depth i.e. number of elements in the
> + * TX fifo of the serial engine.
> + *
> + * Return: TX fifo depth in units of FIFO words.
> + */
> +u32 geni_se_get_tx_fifo_depth(struct geni_se *se)
> +{
> + u32 val;
> +
> + val = readl_relaxed(se->base + SE_HW_PARAM_0);
> +
> + return (val & TX_FIFO_DEPTH_MSK) >> TX_FIFO_DEPTH_SHFT;
> +}
> +EXPORT_SYMBOL(geni_se_get_tx_fifo_depth);
> +
> +/**
> + * geni_se_get_tx_fifo_width() - Get the TX fifo width of the serial engine
> + * @se: Pointer to the concerned Serial Engine.
> + *
> + * This function is used to get the width i.e. word size per element in the
> + * TX fifo of the serial engine.
> + *
> + * Return: TX fifo width in bits
> + */
> +u32 geni_se_get_tx_fifo_width(struct geni_se *se)
> +{
> + u32 val;
> +
> + val = readl_relaxed(se->base + SE_HW_PARAM_0);
> +
> + return (val & TX_FIFO_WIDTH_MSK) >> TX_FIFO_WIDTH_SHFT;
> +}
> +EXPORT_SYMBOL(geni_se_get_tx_fifo_width);
> +
> +/**
> + * geni_se_get_rx_fifo_depth() - Get the RX fifo depth of the serial engine
> + * @se: Pointer to the concerned Serial Engine.
> + *
> + * This function is used to get the depth i.e. number of elements in the
> + * RX fifo of the serial engine.
> + *
> + * Return: RX fifo depth in units of FIFO words
> + */
> +u32 geni_se_get_rx_fifo_depth(struct geni_se *se)
> +{
> + u32 val;
> +
> + val = readl_relaxed(se->base + SE_HW_PARAM_1);
> +
> + return (val & RX_FIFO_DEPTH_MSK) >> RX_FIFO_DEPTH_SHFT;
> +}
> +EXPORT_SYMBOL(geni_se_get_rx_fifo_depth);
These ones too, can probably just be static inline.
> +
> +/**
> + * DOC: Overview
> + *
> + * GENI FIFO packing is highly configurable. TX/RX packing/unpacking consist
> + * of up to 4 operations, each operation represented by 4 configuration vectors
> + * of 10 bits programmed in GENI_TX_PACKING_CFG0 and GENI_TX_PACKING_CFG1 for
> + * TX FIFO and in GENI_RX_PACKING_CFG0 and GENI_RX_PACKING_CFG1 for RX FIFO.
> + * Refer to below examples for detailed bit-field description.
> + *
> + * Example 1: word_size = 7, packing_mode = 4 x 8, msb_to_lsb = 1
> + *
> + * +-----------+-------+-------+-------+-------+
> + * | | vec_0 | vec_1 | vec_2 | vec_3 |
> + * +-----------+-------+-------+-------+-------+
> + * | start | 0x6 | 0xe | 0x16 | 0x1e |
> + * | direction | 1 | 1 | 1 | 1 |
> + * | length | 6 | 6 | 6 | 6 |
> + * | stop | 0 | 0 | 0 | 1 |
> + * +-----------+-------+-------+-------+-------+
> + *
> + * Example 2: word_size = 15, packing_mode = 2 x 16, msb_to_lsb = 0
> + *
> + * +-----------+-------+-------+-------+-------+
> + * | | vec_0 | vec_1 | vec_2 | vec_3 |
> + * +-----------+-------+-------+-------+-------+
> + * | start | 0x0 | 0x8 | 0x10 | 0x18 |
> + * | direction | 0 | 0 | 0 | 0 |
> + * | length | 7 | 6 | 7 | 6 |
> + * | stop | 0 | 0 | 0 | 1 |
> + * +-----------+-------+-------+-------+-------+
> + *
> + * Example 3: word_size = 23, packing_mode = 1 x 32, msb_to_lsb = 1
> + *
> + * +-----------+-------+-------+-------+-------+
> + * | | vec_0 | vec_1 | vec_2 | vec_3 |
> + * +-----------+-------+-------+-------+-------+
> + * | start | 0x16 | 0xe | 0x6 | 0x0 |
> + * | direction | 1 | 1 | 1 | 1 |
> + * | length | 7 | 7 | 6 | 0 |
> + * | stop | 0 | 0 | 1 | 0 |
> + * +-----------+-------+-------+-------+-------+
> + *
> + */
> +
> +#define NUM_PACKING_VECTORS 4
> +#define PACKING_START_SHIFT 5
> +#define PACKING_DIR_SHIFT 4
> +#define PACKING_LEN_SHIFT 1
> +#define PACKING_STOP_BIT BIT(0)
> +#define PACKING_VECTOR_SHIFT 10
> +/**
> + * geni_se_config_packing() - Packing configuration of the serial engine
> + * @se: Pointer to the concerned Serial Engine
> + * @bpw: Bits of data per transfer word.
> + * @pack_words: Number of words per fifo element.
> + * @msb_to_lsb: Transfer from MSB to LSB or vice-versa.
> + * @tx_cfg: Flag to configure the TX Packing.
> + * @rx_cfg: Flag to configure the RX Packing.
> + *
> + * This function is used to configure the packing rules for the current
> + * transfer.
> + */
> +void geni_se_config_packing(struct geni_se *se, int bpw, int pack_words,
> + bool msb_to_lsb, bool tx_cfg, bool rx_cfg)
> +{
> + u32 cfg0, cfg1, cfg[NUM_PACKING_VECTORS] = {0};
> + int len;
> + int temp_bpw = bpw;
> + int idx_start = msb_to_lsb ? bpw - 1 : 0;
> + int idx = idx_start;
> + int idx_delta = msb_to_lsb ? -BITS_PER_BYTE : BITS_PER_BYTE;
> + int ceil_bpw = (bpw + (BITS_PER_BYTE - 1)) & ~(BITS_PER_BYTE - 1);
ALIGN(bpw, BITS_PER_BYTE)?
> + int iter = (ceil_bpw * pack_words) / BITS_PER_BYTE;
> + int i;
> +
> + if (iter <= 0 || iter > NUM_PACKING_VECTORS)
> + return;
> +
> + for (i = 0; i < iter; i++) {
> + if (temp_bpw < BITS_PER_BYTE)
> + len = temp_bpw - 1;
> + else
> + len = BITS_PER_BYTE - 1;
len = min(temp_bpw, BITS_PER_BYTE) - 1;
> +
> + cfg[i] = idx << PACKING_START_SHIFT;
> + cfg[i] |= msb_to_lsb << PACKING_DIR_SHIFT;
> + cfg[i] |= len << PACKING_LEN_SHIFT;
> +
> + if (temp_bpw <= BITS_PER_BYTE) {
> + idx = ((i + 1) * BITS_PER_BYTE) + idx_start;
> + temp_bpw = bpw;
> + } else {
> + idx = idx + idx_delta;
> + temp_bpw = temp_bpw - BITS_PER_BYTE;
> + }
> + }
> + cfg[iter - 1] |= PACKING_STOP_BIT;
> + cfg0 = cfg[0] | (cfg[1] << PACKING_VECTOR_SHIFT);
> + cfg1 = cfg[2] | (cfg[3] << PACKING_VECTOR_SHIFT);
> +
> + if (tx_cfg) {
> + writel_relaxed(cfg0, se->base + SE_GENI_TX_PACKING_CFG0);
> + writel_relaxed(cfg1, se->base + SE_GENI_TX_PACKING_CFG1);
> + }
> + if (rx_cfg) {
> + writel_relaxed(cfg0, se->base + SE_GENI_RX_PACKING_CFG0);
> + writel_relaxed(cfg1, se->base + SE_GENI_RX_PACKING_CFG1);
> + }
> +
> + /*
> + * Number of protocol words in each FIFO entry
> + * 0 - 4x8, four words in each entry, max word size of 8 bits
> + * 1 - 2x16, two words in each entry, max word size of 16 bits
> + * 2 - 1x32, one word in each entry, max word size of 32 bits
> + * 3 - undefined
> + */
> + if (pack_words || bpw == 32)
> + writel_relaxed(bpw / 16, se->base + SE_GENI_BYTE_GRAN);
> +}
> +EXPORT_SYMBOL(geni_se_config_packing);
> +
> +static void geni_se_clks_off(struct geni_se *se)
> +{
> + struct geni_wrapper *wrapper = se->wrapper;
> +
> + clk_disable_unprepare(se->clk);
> + clk_bulk_disable_unprepare(ARRAY_SIZE(wrapper->ahb_clks),
> + wrapper->ahb_clks);
> +}
> +
> +/**
> + * geni_se_resources_off() - Turn off resources associated with the serial
> + * engine
> + * @se: Pointer to the concerned Serial Engine.
> + *
> + * Return: 0 on success, standard Linux error codes on failure/error.
> + */
> +int geni_se_resources_off(struct geni_se *se)
> +{
> + int ret;
> +
> + ret = pinctrl_pm_select_sleep_state(se->dev);
> + if (ret)
> + return ret;
> +
> + geni_se_clks_off(se);
> + return 0;
> +}
> +EXPORT_SYMBOL(geni_se_resources_off);
> +
> +static int geni_se_clks_on(struct geni_se *se)
> +{
> + int ret;
> + struct geni_wrapper *wrapper = se->wrapper;
> +
> + ret = clk_bulk_prepare_enable(ARRAY_SIZE(wrapper->ahb_clks),
> + wrapper->ahb_clks);
> + if (ret)
> + return ret;
> +
> + ret = clk_prepare_enable(se->clk);
> + if (ret)
> + clk_bulk_disable_unprepare(ARRAY_SIZE(wrapper->ahb_clks),
> + wrapper->ahb_clks);
> + return ret;
> +}
> +
> +/**
> + * geni_se_resources_on() - Turn on resources associated with the serial
> + * engine
> + * @se: Pointer to the concerned Serial Engine.
> + *
> + * Return: 0 on success, standard Linux error codes on failure/error.
> + */
> +int geni_se_resources_on(struct geni_se *se)
> +{
> + int ret = 0;
Don't assign variables and then reassign them on the next line.
> +
> + ret = geni_se_clks_on(se);
> + if (ret)
> + return ret;
> +
> + ret = pinctrl_pm_select_default_state(se->dev);
> + if (ret)
> + geni_se_clks_off(se);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL(geni_se_resources_on);
IS there a reason why we can't use runtime PM or normal linux PM
infrastructure to power on the wrapper and keep it powered while the
protocol driver is active?
> +
> +/**
> + * geni_se_clk_tbl_get() - Get the clock table to program DFS
> + * @se: Pointer to the concerned Serial Engine.
> + * @tbl: Table in which the output is returned.
> + *
> + * This function is called by the protocol drivers to determine the different
> + * clock frequencies supported by Serial Engine Core Clock. The protocol
> + * drivers use the output to determine the clock frequency index to be
> + * programmed into DFS.
> + *
> + * Return: number of valid performance levels in the table on success,
> + * standard Linux error codes on failure.
> + */
> +int geni_se_clk_tbl_get(struct geni_se *se, unsigned long **tbl)
> +{
> + struct geni_wrapper *wrapper = se->wrapper;
> + unsigned long freq = 0;
> + int i;
> + int ret = 0;
> +
> + mutex_lock(&wrapper->lock);
> + if (wrapper->clk_perf_tbl) {
> + *tbl = wrapper->clk_perf_tbl;
> + ret = wrapper->num_clk_levels;
> + goto out_unlock;
> + }
> +
> + wrapper->clk_perf_tbl = kcalloc(MAX_CLK_PERF_LEVEL,
> + sizeof(*wrapper->clk_perf_tbl),
> + GFP_KERNEL);
> + if (!wrapper->clk_perf_tbl) {
> + ret = -ENOMEM;
> + goto out_unlock;
> + }
> +
> + for (i = 0; i < MAX_CLK_PERF_LEVEL; i++) {
> + freq = clk_round_rate(se->clk, freq + 1);
> + if (!freq || freq == wrapper->clk_perf_tbl[i - 1])
> + break;
> + wrapper->clk_perf_tbl[i] = freq;
> + }
> + wrapper->num_clk_levels = i;
> + *tbl = wrapper->clk_perf_tbl;
> + ret = wrapper->num_clk_levels;
> +out_unlock:
> + mutex_unlock(&wrapper->lock);
Is this lock actually protecting anything? I mean to say, is any more
than one geni protocol driver calling this function at a time? Or is
the same geni protocol driver calling this from multiple threads at the
same time? The lock looks almost useless.
> + return ret;
> +}
> +EXPORT_SYMBOL(geni_se_clk_tbl_get);
> +
> +/**
> + * geni_se_clk_freq_match() - Get the matching or closest SE clock frequency
> + * @se: Pointer to the concerned Serial Engine.
> + * @req_freq: Requested clock frequency.
> + * @index: Index of the resultant frequency in the table.
> + * @res_freq: Resultant frequency which matches or is closer to the
> + * requested frequency.
> + * @exact: Flag to indicate exact multiple requirement of the requested
> + * frequency.
> + *
> + * This function is called by the protocol drivers to determine the matching
> + * or exact multiple of the requested frequency, as provided by the Serial
> + * Engine clock in order to meet the performance requirements. If there is
> + * no matching or exact multiple of the requested frequency found, then it
> + * selects the closest floor frequency, if exact flag is not set.
> + *
> + * Return: 0 on success, standard Linux error codes on failure.
> + */
> +int geni_se_clk_freq_match(struct geni_se *se, unsigned long req_freq,
> + unsigned int *index, unsigned long *res_freq,
> + bool exact)
> +{
> + unsigned long *tbl;
> + int num_clk_levels;
> + int i;
> +
> + num_clk_levels = geni_se_clk_tbl_get(se, &tbl);
> + if (num_clk_levels < 0)
> + return num_clk_levels;
> +
> + if (num_clk_levels == 0)
> + return -EFAULT;
I believe this would mean userspace thought the syscall faulted.
Perhaps -EINVAL instead?
> +
> + *res_freq = 0;
> + for (i = 0; i < num_clk_levels; i++) {
> + if (!(tbl[i] % req_freq)) {
> + *index = i;
> + *res_freq = tbl[i];
> + return 0;
> + }
> +
> + if (!(*res_freq) || ((tbl[i] > *res_freq) &&
> + (tbl[i] < req_freq))) {
> + *index = i;
> + *res_freq = tbl[i];
> + }
> + }
> +
> + if (exact)
> + return -ENOKEY;
Interesting error code. Doubtful this is correct because it seems to be
related to crypto keys.
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(geni_se_clk_freq_match);
> +
> +#define GENI_SE_DMA_DONE_EN BIT(0)
> +#define GENI_SE_DMA_EOT_EN BIT(1)
> +#define GENI_SE_DMA_AHB_ERR_EN BIT(2)
> +#define GENI_SE_DMA_EOT_BUF BIT(0)
> +/**
> + * geni_se_tx_dma_prep() - Prepare the Serial Engine for TX DMA transfer
> + * @se: Pointer to the concerned Serial Engine.
> + * @buf: Pointer to the TX buffer.
> + * @len: Length of the TX buffer.
> + *
> + * This function is used to prepare the buffers for DMA TX.
> + *
> + * Return: Mapped DMA Address of the buffer on success, NULL on failure.
> + */
> +dma_addr_t geni_se_tx_dma_prep(struct geni_se *se, void *buf, size_t len)
> +{
> + dma_addr_t iova;
> + struct geni_wrapper *wrapper = se->wrapper;
> + u32 val;
> +
> + iova = dma_map_single(wrapper->dev, buf, len, DMA_TO_DEVICE);
> + if (dma_mapping_error(wrapper->dev, iova))
> + return (dma_addr_t)NULL;
> +
> + val = GENI_SE_DMA_DONE_EN;
> + val |= GENI_SE_DMA_EOT_EN;
> + val |= GENI_SE_DMA_AHB_ERR_EN;
> + writel_relaxed(val, se->base + SE_DMA_TX_IRQ_EN_SET);
> + writel_relaxed((u32)iova, se->base + SE_DMA_TX_PTR_L);
lower_32_bits()
> + writel_relaxed((u32)(iova >> 32), se->base + SE_DMA_TX_PTR_H);
upper_32_bits()
> + writel_relaxed(GENI_SE_DMA_EOT_BUF, se->base + SE_DMA_TX_ATTR);
> + writel_relaxed((u32)len, se->base + SE_DMA_TX_LEN);
Useless cast.
> + return iova;
> +}
> +EXPORT_SYMBOL(geni_se_tx_dma_prep);
> +
> +/**
> + * geni_se_rx_dma_prep() - Prepare the Serial Engine for RX DMA transfer
> + * @se: Pointer to the concerned Serial Engine.
> + * @buf: Pointer to the RX buffer.
> + * @len: Length of the RX buffer.
> + *
> + * This function is used to prepare the buffers for DMA RX.
> + *
> + * Return: Mapped DMA Address of the buffer on success, NULL on failure.
> + */
> +dma_addr_t geni_se_rx_dma_prep(struct geni_se *se, void *buf, size_t len)
> +{
> + dma_addr_t iova;
> + struct geni_wrapper *wrapper = se->wrapper;
> + u32 val;
> +
> + iova = dma_map_single(wrapper->dev, buf, len, DMA_FROM_DEVICE);
> + if (dma_mapping_error(wrapper->dev, iova))
> + return (dma_addr_t)NULL;
Can't return a dma_mapping_error address to the caller and have them
figure it out?
> +
> + val = GENI_SE_DMA_DONE_EN;
> + val |= GENI_SE_DMA_EOT_EN;
> + val |= GENI_SE_DMA_AHB_ERR_EN;
> + writel_relaxed(val, se->base + SE_DMA_RX_IRQ_EN_SET);
> + writel_relaxed((u32)iova, se->base + SE_DMA_RX_PTR_L);
> + writel_relaxed((u32)(iova >> 32), se->base + SE_DMA_RX_PTR_H);
upper/lower macros again.
> + /* RX does not have EOT buffer type bit. So just reset RX_ATTR */
> + writel_relaxed(0, se->base + SE_DMA_RX_ATTR);
> + writel_relaxed((u32)len, se->base + SE_DMA_RX_LEN);
Drop cast?
> + return iova;
> +}
> +EXPORT_SYMBOL(geni_se_rx_dma_prep);
> +
> +/**
> + * geni_se_tx_dma_unprep() - Unprepare the Serial Engine after TX DMA transfer
> + * @se: Pointer to the concerned Serial Engine.
> + * @iova: DMA address of the TX buffer.
> + * @len: Length of the TX buffer.
> + *
> + * This function is used to unprepare the DMA buffers after DMA TX.
> + */
> +void geni_se_tx_dma_unprep(struct geni_se *se, dma_addr_t iova, size_t len)
> +{
> + struct geni_wrapper *wrapper = se->wrapper;
> +
> + if (iova)
> + dma_unmap_single(wrapper->dev, iova, len, DMA_TO_DEVICE);
> +}
> +EXPORT_SYMBOL(geni_se_tx_dma_unprep);
> +
> +/**
> + * geni_se_rx_dma_unprep() - Unprepare the Serial Engine after RX DMA transfer
> + * @se: Pointer to the concerned Serial Engine.
> + * @iova: DMA address of the RX buffer.
> + * @len: Length of the RX buffer.
> + *
> + * This function is used to unprepare the DMA buffers after DMA RX.
> + */
> +void geni_se_rx_dma_unprep(struct geni_se *se, dma_addr_t iova, size_t len)
> +{
> + struct geni_wrapper *wrapper = se->wrapper;
> +
> + if (iova)
> + dma_unmap_single(wrapper->dev, iova, len, DMA_FROM_DEVICE);
> +}
> +EXPORT_SYMBOL(geni_se_rx_dma_unprep);
Instead of having the functions exported, could we set the dma_ops on
all child devices of the wrapper that this driver populates and then
implement the DMA ops for those devices here? I assume that there's
never another DMA master between the wrapper and the serial engine, so I
think it would work.
> +
> +static int geni_se_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct resource *res;
> + struct geni_wrapper *wrapper;
> + int ret;
> +
> + wrapper = devm_kzalloc(dev, sizeof(*wrapper), GFP_KERNEL);
> + if (!wrapper)
> + return -ENOMEM;
> +
> + wrapper->dev = dev;
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + wrapper->base = devm_ioremap_resource(dev, res);
> + if (IS_ERR(wrapper->base)) {
> + dev_err(dev, "%s: Error mapping the resource\n", __func__);
Drop error message, devm_ioremap_resource() already does it.
> + return -EFAULT;
return PTR_ERR(wrapper->base);
> + }
> +
> + wrapper->ahb_clks[0].id = m_ahb_clk;
> + wrapper->ahb_clks[1].id = s_ahb_clk;
> + ret = devm_clk_bulk_get(dev, NUM_AHB_CLKS, wrapper->ahb_clks);
> + if (ret) {
> + dev_err(dev, "Err getting AHB clks %d\n", ret);
> + return ret;
> + }
> +
> + mutex_init(&wrapper->lock);
> + dev_set_drvdata(dev, wrapper);
> + dev_dbg(dev, "GENI SE Driver probed\n");
> + return devm_of_platform_populate(dev);
> +}
> +
> +static int geni_se_remove(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct geni_wrapper *wrapper = dev_get_drvdata(dev);
> +
> + kfree(wrapper->clk_perf_tbl);
Why not devm_kzalloc() this?
> + return 0;
> +}
> +
> +static const struct of_device_id geni_se_dt_match[] = {
> + { .compatible = "qcom,geni-se-qup", },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, geni_se_dt_match);
> +
> +static struct platform_driver geni_se_driver = {
> + .driver = {
> + .name = "geni_se_qup",
> + .of_match_table = geni_se_dt_match,
> + },
> + .probe = geni_se_probe,
> + .remove = geni_se_remove,
> +};
> +module_platform_driver(geni_se_driver);
> +
> +MODULE_DESCRIPTION("GENI Serial Engine Driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/qcom-geni-se.h b/include/linux/qcom-geni-se.h
> new file mode 100644
> index 0000000..4996de7
> --- /dev/null
> +++ b/include/linux/qcom-geni-se.h
> @@ -0,0 +1,247 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +/*
> + * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
> + */
> +
> +#ifndef _LINUX_QCOM_GENI_SE
> +#define _LINUX_QCOM_GENI_SE
> +#include <linux/clk.h>
Please forward declare struct clk and drop this include here.
> +#include <linux/dma-direction.h>
Drop?
> +
> +/* Transfer mode supported by GENI Serial Engines */
> +enum geni_se_xfer_mode {
> + GENI_SE_INVALID,
> + GENI_SE_FIFO,
> + GENI_SE_DMA,
> +};
> +
> +/* Protocols supported by GENI Serial Engines */
> +enum geni_se_protocol_types {
> + GENI_SE_NONE,
> + GENI_SE_SPI,
> + GENI_SE_UART,
> + GENI_SE_I2C,
> + GENI_SE_I3C,
> +};
> +
> +/**
> + * struct geni_se - GENI Serial Engine
> + * @base: Base Address of the Serial Engine's register block.
> + * @dev: Pointer to the Serial Engine device.
> + * @wrapper: Pointer to the parent QUP Wrapper core.
> + * @clk: Handle to the core serial engine clock.
> + */
> +struct geni_se {
> + void __iomem *base;
> + struct device *dev;
> + void *wrapper;
Can this get the geni_wrapper type? It could be opaque if you like.
> + struct clk *clk;
> +};
> +
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v3 2/4] soc: qcom: Add GENI based QUP Wrapper driver
From: Evan Green @ 2018-03-02 20:58 UTC (permalink / raw)
To: Stephen Boyd
Cc: Karthikeyan Ramasubramanian, Andy Gross, Jonathan Corbet,
David Brown, gregkh, mark.rutland, robh+dt, wsa, linux-doc,
linux-arm-msm, devicetree, linux-i2c, linux-serial, jslaby,
acourbot, Sagar Dharia, Girish Mahadevan
In-Reply-To: <152002326567.108663.16836885081217739460@swboyd.mtv.corp.google.com>
Hi Karthik,
On Tue, Feb 27, 2018 at 5:38 PM, Karthikeyan Ramasubramanian
<kramasub@codeaurora.org> wrote:
> This driver manages the Generic Interface (GENI) firmware based Qualcomm
> Universal Peripheral (QUP) Wrapper. GENI based QUP is the next generation
> programmable module composed of multiple Serial Engines (SE) and supports
> a wide range of serial interfaces like UART, SPI, I2C, I3C, etc. This
> driver also enables managing the serial interface independent aspects of
> Serial Engines.
>
> Signed-off-by: Karthikeyan Ramasubramanian <kramasub@codeaurora.org>
> Signed-off-by: Sagar Dharia <sdharia@codeaurora.org>
> Signed-off-by: Girish Mahadevan <girishm@codeaurora.org>
> ---
> drivers/soc/qcom/Kconfig | 9 +
> drivers/soc/qcom/Makefile | 1 +
> drivers/soc/qcom/qcom-geni-se.c | 971 ++++++++++++++++++++++++++++++++++++++++
> include/linux/qcom-geni-se.h | 247 ++++++++++
> 4 files changed, 1228 insertions(+)
> create mode 100644 drivers/soc/qcom/qcom-geni-se.c
> create mode 100644 include/linux/qcom-geni-se.h
>
[...]
> diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
> new file mode 100644
> index 0000000..61335b8
> --- /dev/null
> +++ b/drivers/soc/qcom/qcom-geni-se.c
> +
> +/**
> + * geni_se_clk_tbl_get() - Get the clock table to program DFS
> + * @se: Pointer to the concerned Serial Engine.
> + * @tbl: Table in which the output is returned.
> + *
> + * This function is called by the protocol drivers to determine the different
> + * clock frequencies supported by Serial Engine Core Clock. The protocol
> + * drivers use the output to determine the clock frequency index to be
> + * programmed into DFS.
> + *
> + * Return: number of valid performance levels in the table on success,
> + * standard Linux error codes on failure.
> + */
> +int geni_se_clk_tbl_get(struct geni_se *se, unsigned long **tbl)
> +{
> + struct geni_wrapper *wrapper = se->wrapper;
> + unsigned long freq = 0;
> + int i;
> + int ret = 0;
> +
> + mutex_lock(&wrapper->lock);
> + if (wrapper->clk_perf_tbl) {
> + *tbl = wrapper->clk_perf_tbl;
> + ret = wrapper->num_clk_levels;
> + goto out_unlock;
> + }
> +
> + wrapper->clk_perf_tbl = kcalloc(MAX_CLK_PERF_LEVEL,
> + sizeof(*wrapper->clk_perf_tbl),
> + GFP_KERNEL);
> + if (!wrapper->clk_perf_tbl) {
> + ret = -ENOMEM;
> + goto out_unlock;
> + }
> +
> + for (i = 0; i < MAX_CLK_PERF_LEVEL; i++) {
> + freq = clk_round_rate(se->clk, freq + 1);
> + if (!freq || freq == wrapper->clk_perf_tbl[i - 1])
> + break;
> + wrapper->clk_perf_tbl[i] = freq;
> + }
> + wrapper->num_clk_levels = i;
> + *tbl = wrapper->clk_perf_tbl;
> + ret = wrapper->num_clk_levels;
> +out_unlock:
> + mutex_unlock(&wrapper->lock);
> + return ret;
> +}
> +EXPORT_SYMBOL(geni_se_clk_tbl_get);
I think Bjorn had this feedback before, but if you did this work in
probe you could remove the mutex altogether.
> + wrapper->ahb_clks[0].id = m_ahb_clk;
> + wrapper->ahb_clks[1].id = s_ahb_clk;
> + ret = devm_clk_bulk_get(dev, NUM_AHB_CLKS, wrapper->ahb_clks);
> + if (ret) {
> + dev_err(dev, "Err getting AHB clks %d\n", ret);
> + return ret;
> + }
> +
> + mutex_init(&wrapper->lock);
> + dev_set_drvdata(dev, wrapper);
> + dev_dbg(dev, "GENI SE Driver probed\n");
> + return devm_of_platform_populate(dev);
> +}
> +
> +static int geni_se_remove(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct geni_wrapper *wrapper = dev_get_drvdata(dev);
> +
> + kfree(wrapper->clk_perf_tbl);
Maybe null out clk_perf_tbl for safety?
> diff --git a/include/linux/qcom-geni-se.h b/include/linux/qcom-geni-se.h
> new file mode 100644
> index 0000000..4996de7
> --- /dev/null
> +++ b/include/linux/qcom-geni-se.h
[...]
> +/* SE_DMA_RX_IRQ_STAT Register fields */
> +#define RX_DMA_DONE BIT(0)
> +#define RX_EOT BIT(1)
> +#define RX_SBE BIT(2)
> +#define RX_RESET_DONE BIT(3)
> +#define RX_FLUSH_DONE BIT(4)
> +#define RX_GENI_GP_IRQ GENMASK(10, 5)
> +#define RX_GENI_CANCEL_IRQ BIT(11)
> +#define RX_GENI_GP_IRQ_EXT GENMASK(13, 12)
> +
> +#ifdef CONFIG_QCOM_GENI_SE
I think this needs to be #if IS_ENABLED(CONFIG_QCOM_GENI_SE), since
the function prototypes below won't light up if this is built as a
module.
Thanks,
Evan
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] xfs: Change URL for the project in xfs.txt
From: Dave Chinner @ 2018-03-02 21:57 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: Masanari Iida, linux-xfs, linux-kernel, corbet, linux-doc
In-Reply-To: <20180302172401.GT19312@magnolia>
On Fri, Mar 02, 2018 at 09:24:01AM -0800, Darrick J. Wong wrote:
> On Fri, Mar 02, 2018 at 10:30:13PM +0900, Masanari Iida wrote:
> > The oss.sgi.com doesn't exist any more.
> > Change it to current project URL, https://xfs.wiki.kernel.org/
> >
> > Signed-off-by: Masanari Iida <standby24x7@gmail.com>
> > ---
> > Documentation/filesystems/xfs.txt | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/Documentation/filesystems/xfs.txt b/Documentation/filesystems/xfs.txt
> > index 3b9b5c149f32..4d9ff0a7f8e1 100644
> > --- a/Documentation/filesystems/xfs.txt
> > +++ b/Documentation/filesystems/xfs.txt
> > @@ -9,7 +9,7 @@ variable block sizes, is extent based, and makes extensive use of
> > Btrees (directories, extents, free space) to aid both performance
> > and scalability.
> >
> > -Refer to the documentation at http://oss.sgi.com/projects/xfs/
> > +Refer to the documentation at https://xfs.wiki.kernel.org/
Did I miss a memo?
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] xfs: Change URL for the project in xfs.txt
From: Eric Sandeen @ 2018-03-02 22:08 UTC (permalink / raw)
To: Dave Chinner, Darrick J. Wong
Cc: Masanari Iida, linux-xfs, linux-kernel, corbet, linux-doc
In-Reply-To: <20180302215731.GT30854@dastard>
On 3/2/18 3:57 PM, Dave Chinner wrote:
> On Fri, Mar 02, 2018 at 09:24:01AM -0800, Darrick J. Wong wrote:
>> On Fri, Mar 02, 2018 at 10:30:13PM +0900, Masanari Iida wrote:
>>> The oss.sgi.com doesn't exist any more.
>>> Change it to current project URL, https://xfs.wiki.kernel.org/
>>>
>>> Signed-off-by: Masanari Iida <standby24x7@gmail.com>
>>> ---
>>> Documentation/filesystems/xfs.txt | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/filesystems/xfs.txt b/Documentation/filesystems/xfs.txt
>>> index 3b9b5c149f32..4d9ff0a7f8e1 100644
>>> --- a/Documentation/filesystems/xfs.txt
>>> +++ b/Documentation/filesystems/xfs.txt
>>> @@ -9,7 +9,7 @@ variable block sizes, is extent based, and makes extensive use of
>>> Btrees (directories, extents, free space) to aid both performance
>>> and scalability.
>>>
>>> -Refer to the documentation at http://oss.sgi.com/projects/xfs/
>>> +Refer to the documentation at https://xfs.wiki.kernel.org/
>
> Did I miss a memo?
About which part, the loss of oss.sgi or the addition of the kernel.org wiki?
The kernel.org wiki is pretty bare though. OTOH xfs.org is a bit less
official. We really need to resolve this issue.
-Eric
> Cheers,
>
> Dave.
>
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v3 4/4] tty: serial: msm_geni_serial: Add serial driver support for GENI based QUP
From: Stephen Boyd @ 2018-03-02 22:11 UTC (permalink / raw)
To: Karthikeyan Ramasubramanian, andy.gross, corbet, david.brown,
gregkh, mark.rutland, robh+dt, wsa
Cc: Karthikeyan Ramasubramanian, linux-doc, linux-arm-msm, devicetree,
linux-i2c, linux-serial, jslaby, evgreen, acourbot,
Girish Mahadevan, Sagar Dharia, Doug Anderson
In-Reply-To: <1519781889-16117-5-git-send-email-kramasub@codeaurora.org>
Quoting Karthikeyan Ramasubramanian (2018-02-27 17:38:09)
> diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
> index 3682fd3..c6b1500 100644
> --- a/drivers/tty/serial/Kconfig
> +++ b/drivers/tty/serial/Kconfig
> @@ -1104,6 +1104,17 @@ config SERIAL_MSM_CONSOLE
> select SERIAL_CORE_CONSOLE
> select SERIAL_EARLYCON
>
> +config SERIAL_QCOM_GENI
> + bool "QCOM on-chip GENI based serial port support"
This can be tristate.
> + depends on ARCH_QCOM
|| COMPILE_TEST
?
> + depends on QCOM_GENI_SE
> + select SERIAL_CORE
This can stay.
> + select SERIAL_CORE_CONSOLE
> + select SERIAL_EARLYCON
These two can go to a new config option, like SERIAL_QCOM_GENI_CONSOLE,
and that would be bool. Please take a look at the existing SERIAL_MSM
and SERIAL_MSM_CONSOLE setup to understand how to do it.
> + help
> + Serial driver for Qualcomm Technologies Inc's GENI based QUP
> + hardware.
> +
> config SERIAL_VT8500
> bool "VIA VT8500 on-chip serial port support"
> depends on ARCH_VT8500
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> new file mode 100644
> index 0000000..8536b7d
> --- /dev/null
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -0,0 +1,1181 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2017-2018, The Linux foundation. All rights reserved.
> +
> +#include <linux/console.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/qcom-geni-se.h>
> +#include <linux/serial.h>
> +#include <linux/serial_core.h>
> +#include <linux/slab.h>
> +#include <linux/tty.h>
> +#include <linux/tty_flip.h>
> +
> +/* UART specific GENI registers */
> +#define SE_UART_TX_TRANS_CFG 0x25c
> +#define SE_UART_TX_WORD_LEN 0x268
> +#define SE_UART_TX_STOP_BIT_LEN 0x26c
> +#define SE_UART_TX_TRANS_LEN 0x270
> +#define SE_UART_RX_TRANS_CFG 0x280
> +#define SE_UART_RX_WORD_LEN 0x28c
> +#define SE_UART_RX_STALE_CNT 0x294
> +#define SE_UART_TX_PARITY_CFG 0x2a4
> +#define SE_UART_RX_PARITY_CFG 0x2a8
> +
> +/* SE_UART_TRANS_CFG */
> +#define UART_TX_PAR_EN BIT(0)
> +#define UART_CTS_MASK BIT(1)
> +
> +/* SE_UART_TX_WORD_LEN */
> +#define TX_WORD_LEN_MSK GENMASK(9, 0)
> +
> +/* SE_UART_TX_STOP_BIT_LEN */
> +#define TX_STOP_BIT_LEN_MSK GENMASK(23, 0)
> +#define TX_STOP_BIT_LEN_1 0
> +#define TX_STOP_BIT_LEN_1_5 1
> +#define TX_STOP_BIT_LEN_2 2
> +
> +/* SE_UART_TX_TRANS_LEN */
> +#define TX_TRANS_LEN_MSK GENMASK(23, 0)
> +
> +/* SE_UART_RX_TRANS_CFG */
> +#define UART_RX_INS_STATUS_BIT BIT(2)
> +#define UART_RX_PAR_EN BIT(3)
> +
> +/* SE_UART_RX_WORD_LEN */
> +#define RX_WORD_LEN_MASK GENMASK(9, 0)
> +
> +/* SE_UART_RX_STALE_CNT */
> +#define RX_STALE_CNT GENMASK(23, 0)
> +
> +/* SE_UART_TX_PARITY_CFG/RX_PARITY_CFG */
> +#define PAR_CALC_EN BIT(0)
> +#define PAR_MODE_MSK GENMASK(2, 1)
> +#define PAR_MODE_SHFT 1
> +#define PAR_EVEN 0x00
> +#define PAR_ODD 0x01
> +#define PAR_SPACE 0x10
> +#define PAR_MARK 0x11
> +
> +/* UART M_CMD OP codes */
> +#define UART_START_TX 0x1
> +#define UART_START_BREAK 0x4
> +#define UART_STOP_BREAK 0x5
> +/* UART S_CMD OP codes */
> +#define UART_START_READ 0x1
> +#define UART_PARAM 0x1
> +
> +#define UART_OVERSAMPLING 32
> +#define STALE_TIMEOUT 16
> +#define DEFAULT_BITS_PER_CHAR 10
> +#define GENI_UART_CONS_PORTS 1
> +#define DEF_FIFO_DEPTH_WORDS 16
> +#define DEF_TX_WM 2
> +#define DEF_FIFO_WIDTH_BITS 32
> +#define UART_CONSOLE_RX_WM 2
> +
> +#ifdef CONFIG_CONSOLE_POLL
> +#define RX_BYTES_PW 1
> +#else
> +#define RX_BYTES_PW 4
> +#endif
> +
> +struct qcom_geni_serial_port {
> + struct uart_port uport;
> + struct geni_se se;
> + char name[20];
> + u32 tx_fifo_depth;
> + u32 tx_fifo_width;
> + u32 rx_fifo_depth;
> + u32 tx_wm;
> + u32 rx_wm;
> + u32 rx_rfr;
> + int xfer_mode;
Can this be an enum?
> + bool port_setup;
Maybe just 'setup'? Port is in the type already.
> + int (*handle_rx)(struct uart_port *uport,
> + u32 rx_bytes, bool drop_rx);
s/rx_bytes/bytes/
s/drop_rx/drop/
> + unsigned int xmit_size;
> + unsigned int cur_baud;
s/cur//
> + unsigned int tx_bytes_pw;
> + unsigned int rx_bytes_pw;
> +};
> +
> +static const struct uart_ops qcom_geni_serial_pops;
> +static struct uart_driver qcom_geni_console_driver;
> +static int handle_rx_console(struct uart_port *uport,
> + u32 rx_bytes, bool drop_rx);
s/rx_bytes/bytes/
s/drop_rx/drop/
> +static unsigned int qcom_geni_serial_tx_empty(struct uart_port *port);
> +static bool qcom_geni_serial_poll_bit(struct uart_port *uport,
> + int offset, int bit_field, bool set);
No need to forward declare this?
s/bit_// ?
> +static void qcom_geni_serial_stop_rx(struct uart_port *uport);
> +
> +static atomic_t uart_line_id = ATOMIC_INIT(0);
Do we need this? How about rely on DT to always have aliases instead?
Given we only have one port I don't actually understand how this is
supposed to work anyway.
> +static const unsigned long root_freq[] = {7372800, 14745600, 19200000, 29491200,
> + 32000000, 48000000, 64000000, 80000000,
> + 96000000, 100000000};
> +
> +#define to_dev_port(ptr, member) \
> + container_of(ptr, struct qcom_geni_serial_port, member)
> +
> +static struct qcom_geni_serial_port qcom_geni_console_port;
Why singleton? Couldn't there be many?
> +
> +static void qcom_geni_serial_config_port(struct uart_port *uport, int cfg_flags)
> +{
> + if (cfg_flags & UART_CONFIG_TYPE)
> + uport->type = PORT_MSM;
> +}
> +
> +static unsigned int qcom_geni_cons_get_mctrl(struct uart_port *uport)
> +{
> + return TIOCM_DSR | TIOCM_CAR | TIOCM_CTS;
> +}
> +
> +static void qcom_geni_cons_set_mctrl(struct uart_port *uport,
> + unsigned int mctrl)
> +{
> +}
> +
> +static const char *qcom_geni_serial_get_type(struct uart_port *uport)
> +{
> + return "MSM";
> +}
> +
> +static struct qcom_geni_serial_port *get_port_from_line(int line)
> +{
> + if ((line < 0) || (line >= GENI_UART_CONS_PORTS))
Drop useless parenthesis please.
> + return ERR_PTR(-ENXIO);
> + return &qcom_geni_console_port;
> +}
> +
> +static bool qcom_geni_serial_poll_bit(struct uart_port *uport,
> + int offset, int bit_field, bool set)
> +{
> + u32 reg;
> + struct qcom_geni_serial_port *port;
> + unsigned int baud;
> + unsigned int fifo_bits;
> + unsigned long timeout_us = 20000;
> +
> + /* Ensure polling is not re-ordered before the prior writes/reads */
> + mb();
> +
> + if (uport->private_data) {
> + port = to_dev_port(uport, uport);
> + baud = port->cur_baud;
> + if (!baud)
> + baud = 115200;
> + fifo_bits = port->tx_fifo_depth * port->tx_fifo_width;
> + /*
> + * Total polling iterations based on FIFO worth of bytes to be
> + * sent at current baud .Add a little fluff to the wait.
Bad space here ^
> + */
> + timeout_us = ((fifo_bits * USEC_PER_SEC) / baud) + 500;
> + }
> +
> + return !readl_poll_timeout_atomic(uport->membase + offset, reg,
> + (bool)(reg & bit_field) == set, 10, timeout_us);
> +}
> +
> +static void qcom_geni_serial_setup_tx(struct uart_port *uport, u32 xmit_size)
> +{
> + u32 m_cmd;
> +
> + writel_relaxed(xmit_size, uport->membase + SE_UART_TX_TRANS_LEN);
> + m_cmd = UART_START_TX << M_OPCODE_SHFT;
> + writel(m_cmd, uport->membase + SE_GENI_M_CMD0);
> +}
> +
> +static void qcom_geni_serial_poll_tx_done(struct uart_port *uport)
> +{
> + int done;
> + u32 irq_clear = M_CMD_DONE_EN;
> +
> + done = qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
> + M_CMD_DONE_EN, true);
> + if (!done) {
> + writel_relaxed(M_GENI_CMD_ABORT, uport->membase +
> + SE_GENI_M_CMD_CTRL_REG);
> + irq_clear |= M_CMD_ABORT_EN;
> + qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
> + M_CMD_ABORT_EN, true);
> + }
> + writel_relaxed(irq_clear, uport->membase + SE_GENI_M_IRQ_CLEAR);
> +}
> +
> +static void qcom_geni_serial_abort_rx(struct uart_port *uport)
> +{
> + u32 irq_clear = S_CMD_DONE_EN | S_CMD_ABORT_EN;
> +
> + writel(S_GENI_CMD_ABORT, uport->membase + SE_GENI_S_CMD_CTRL_REG);
> + qcom_geni_serial_poll_bit(uport, SE_GENI_S_CMD_CTRL_REG,
> + S_GENI_CMD_ABORT, false);
> + writel_relaxed(irq_clear, uport->membase + SE_GENI_S_IRQ_CLEAR);
> + writel_relaxed(FORCE_DEFAULT, uport->membase + GENI_FORCE_DEFAULT_REG);
> +}
> +
> +#ifdef CONFIG_CONSOLE_POLL
> +static int qcom_geni_serial_get_char(struct uart_port *uport)
> +{
> + u32 rx_fifo;
> + u32 status;
> +
> + status = readl_relaxed(uport->membase + SE_GENI_M_IRQ_STATUS);
> + writel_relaxed(status, uport->membase + SE_GENI_M_IRQ_CLEAR);
> +
> + status = readl_relaxed(uport->membase + SE_GENI_S_IRQ_STATUS);
> + writel_relaxed(status, uport->membase + SE_GENI_S_IRQ_CLEAR);
> +
> + /*
> + * Ensure the writes to clear interrupts is not re-ordered after
> + * reading the data.
> + */
> + mb();
> +
> + status = readl_relaxed(uport->membase + SE_GENI_RX_FIFO_STATUS);
> + if (!(status & RX_FIFO_WC_MSK))
> + return NO_POLL_CHAR;
> +
> + rx_fifo = readl(uport->membase + SE_GENI_RX_FIFOn);
> + return rx_fifo & 0xff;
> +}
> +
> +static void qcom_geni_serial_poll_put_char(struct uart_port *uport,
> + unsigned char c)
> +{
> + struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
> +
> + writel_relaxed(port->tx_wm, uport->membase + SE_GENI_TX_WATERMARK_REG);
> + qcom_geni_serial_setup_tx(uport, 1);
> + WARN_ON(!qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
> + M_TX_FIFO_WATERMARK_EN, true));
> + writel_relaxed((u32)c, uport->membase + SE_GENI_TX_FIFOn);
Drop useless cast.
> + writel_relaxed(M_TX_FIFO_WATERMARK_EN, uport->membase +
> + SE_GENI_M_IRQ_CLEAR);
> + qcom_geni_serial_poll_tx_done(uport);
> +}
> +#endif
> +
> +#if defined(CONFIG_SERIAL_CORE_CONSOLE) || defined(CONFIG_CONSOLE_POLL)
> +static void qcom_geni_serial_wr_char(struct uart_port *uport, int ch)
> +{
> + writel_relaxed(ch, uport->membase + SE_GENI_TX_FIFOn);
> +}
> +
> +static void
> +__qcom_geni_serial_console_write(struct uart_port *uport, const char *s,
> + unsigned int count)
> +{
> + int new_line = 0;
Drop
> + int i;
> + u32 bytes_to_send = count;
> +
> + for (i = 0; i < count; i++) {
> + if (s[i] == '\n')
> + new_line++;
bytes_to_send++;
> + }
> +
> + bytes_to_send += new_line;
Drop.
> + writel_relaxed(DEF_TX_WM, uport->membase + SE_GENI_TX_WATERMARK_REG);
> + qcom_geni_serial_setup_tx(uport, bytes_to_send);
> + i = 0;
> + while (i < count) {
for (i = 0; i < count; ) {
would be more normal, but ok.
> + size_t chars_to_write = 0;
> + size_t avail = DEF_FIFO_DEPTH_WORDS - DEF_TX_WM;
> +
> + /*
> + * If the WM bit never set, then the Tx state machine is not
> + * in a valid state, so break, cancel/abort any existing
> + * command. Unfortunately the current data being written is
> + * lost.
> + */
> + while (!qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
> + M_TX_FIFO_WATERMARK_EN, true))
Does this ever timeout? So many nested while loops makes it hard to
follow.
> + break;
> + chars_to_write = min_t(size_t, (size_t)(count - i), avail / 2);
> + uart_console_write(uport, (s + i), chars_to_write,
Drop useless parenthesis please.
> + qcom_geni_serial_wr_char);
> + writel_relaxed(M_TX_FIFO_WATERMARK_EN, uport->membase +
> + SE_GENI_M_IRQ_CLEAR);
> + i += chars_to_write;
> + }
> + qcom_geni_serial_poll_tx_done(uport);
> +}
> +
> +static void qcom_geni_serial_console_write(struct console *co, const char *s,
> + unsigned int count)
> +{
> + struct uart_port *uport;
> + struct qcom_geni_serial_port *port;
> + bool locked = true;
> + unsigned long flags;
> +
> + WARN_ON(co->index < 0 || co->index >= GENI_UART_CONS_PORTS);
> +
> + port = get_port_from_line(co->index);
> + if (IS_ERR(port))
> + return;
> +
> + uport = &port->uport;
> + if (oops_in_progress)
> + locked = spin_trylock_irqsave(&uport->lock, flags);
> + else
> + spin_lock_irqsave(&uport->lock, flags);
> +
> + if (locked) {
> + __qcom_geni_serial_console_write(uport, s, count);
So if oops is in progress, and we didn't lock here, we don't output
data? I'd think we would always want to write to the fifo, just make the
lock grab/release conditional.
> + spin_unlock_irqrestore(&uport->lock, flags);
> + }
> +}
> +
> +static int handle_rx_console(struct uart_port *uport, u32 rx_bytes, bool drop)
> +{
> + u32 i = rx_bytes;
> + u32 rx_fifo;
> + unsigned char *buf;
> + struct tty_port *tport;
> + struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
> +
> + tport = &uport->state->port;
> + while (i > 0) {
> + int c;
> + int bytes = i > port->rx_bytes_pw ? port->rx_bytes_pw : i;
> +
> + rx_fifo = readl_relaxed(uport->membase + SE_GENI_RX_FIFOn);
Please use ioread32_rep(..., 1) here.
> + i -= bytes;
> + if (drop)
> + continue;
> + buf = (unsigned char *)&rx_fifo;
So that this cast becomes unnecessary, and endian agnostic.
> +
> + for (c = 0; c < bytes; c++) {
> + int sysrq;
> +
> + uport->icount.rx++;
> + sysrq = uart_handle_sysrq_char(uport, buf[c]);
And so this does the right thing in whatever world we live in.
> + if (!sysrq)
> + tty_insert_flip_char(tport, buf[c], TTY_NORMAL);
> + }
> + }
> + if (!drop)
> + tty_flip_buffer_push(tport);
> + return 0;
> +}
> +#else
> +static int handle_rx_console(struct uart_port *uport,
> + unsigned int rx_fifo_wc,
> + unsigned int rx_last_byte_valid,
> + unsigned int rx_last,
> + bool drop_rx)
> +{
> + return -EPERM;
> +}
> +
> +#endif /* (CONFIG_SERIAL_CORE_CONSOLE) || defined(CONFIG_CONSOLE_POLL)) */
> +
> +static void qcom_geni_serial_start_tx(struct uart_port *uport)
> +{
> + u32 irq_en;
> + struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
> + u32 status;
> +
> + if (port->xfer_mode == GENI_SE_FIFO) {
> + status = readl_relaxed(uport->membase + SE_GENI_STATUS);
> + if (status & M_GENI_CMD_ACTIVE)
> + return;
> +
> + if (!qcom_geni_serial_tx_empty(uport))
> + return;
> +
> + /*
> + * Ensure writing to IRQ_EN & watermark registers are not
> + * re-ordered before checking the status of the Serial
> + * Engine and TX FIFO
> + */
> + mb();
> +
> + irq_en = readl_relaxed(uport->membase + SE_GENI_M_IRQ_EN);
> + irq_en |= M_TX_FIFO_WATERMARK_EN | M_CMD_DONE_EN;
> +
> + writel_relaxed(port->tx_wm, uport->membase +
> + SE_GENI_TX_WATERMARK_REG);
> + writel_relaxed(irq_en, uport->membase + SE_GENI_M_IRQ_EN);
> + }
> +}
> +
> +static void qcom_geni_serial_stop_tx(struct uart_port *uport)
> +{
> + u32 irq_en;
> + u32 status;
> + struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
> +
> + irq_en = readl_relaxed(uport->membase + SE_GENI_M_IRQ_EN);
> + irq_en &= ~M_CMD_DONE_EN;
> + if (port->xfer_mode == GENI_SE_FIFO) {
> + irq_en &= ~M_TX_FIFO_WATERMARK_EN;
> + writel_relaxed(0, uport->membase +
> + SE_GENI_TX_WATERMARK_REG);
> + }
> + port->xmit_size = 0;
> + writel_relaxed(irq_en, uport->membase + SE_GENI_M_IRQ_EN);
> + status = readl_relaxed(uport->membase + SE_GENI_STATUS);
> + /* Possible stop tx is called multiple times. */
> + if (!(status & M_GENI_CMD_ACTIVE))
> + return;
> +
> + /*
> + * Ensure cancel command write is not re-ordered before checking
> + * checking the status of the Primary Sequencer.
> + */
> + mb();
> +
> + geni_se_cancel_m_cmd(&port->se);
> + if (!qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
> + M_CMD_CANCEL_EN, true)) {
> + geni_se_abort_m_cmd(&port->se);
> + qcom_geni_serial_poll_bit(uport, SE_GENI_M_IRQ_STATUS,
> + M_CMD_ABORT_EN, true);
> + writel_relaxed(M_CMD_ABORT_EN, uport->membase +
> + SE_GENI_M_IRQ_CLEAR);
> + }
> + writel_relaxed(M_CMD_CANCEL_EN, uport->membase + SE_GENI_M_IRQ_CLEAR);
> +}
> +
> +static void qcom_geni_serial_start_rx(struct uart_port *uport)
> +{
> + u32 irq_en;
> + u32 status;
> + struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
> +
> + status = readl_relaxed(uport->membase + SE_GENI_STATUS);
> + if (status & S_GENI_CMD_ACTIVE)
> + qcom_geni_serial_stop_rx(uport);
> +
> + /*
> + * Ensure setup command write is not re-ordered before checking
> + * checking the status of the Secondary Sequencer.
> + */
> + mb();
> +
> + geni_se_setup_s_cmd(&port->se, UART_START_READ, 0);
> +
> + if (port->xfer_mode == GENI_SE_FIFO) {
> + irq_en = readl_relaxed(uport->membase + SE_GENI_S_IRQ_EN);
> + irq_en |= S_RX_FIFO_WATERMARK_EN | S_RX_FIFO_LAST_EN;
> + writel_relaxed(irq_en, uport->membase + SE_GENI_S_IRQ_EN);
> +
> + irq_en = readl_relaxed(uport->membase + SE_GENI_M_IRQ_EN);
> + irq_en |= M_RX_FIFO_WATERMARK_EN | M_RX_FIFO_LAST_EN;
> + writel_relaxed(irq_en, uport->membase + SE_GENI_M_IRQ_EN);
> + }
> +}
> +
> +static void qcom_geni_serial_stop_rx(struct uart_port *uport)
> +{
> + u32 irq_en;
> + u32 status;
> + struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
> + u32 irq_clear = S_CMD_DONE_EN;
> +
> + if (port->xfer_mode == GENI_SE_FIFO) {
> + irq_en = readl_relaxed(uport->membase + SE_GENI_S_IRQ_EN);
> + irq_en &= ~(S_RX_FIFO_WATERMARK_EN | S_RX_FIFO_LAST_EN);
> + writel_relaxed(irq_en, uport->membase + SE_GENI_S_IRQ_EN);
> +
> + irq_en = readl_relaxed(uport->membase + SE_GENI_M_IRQ_EN);
> + irq_en &= ~(M_RX_FIFO_WATERMARK_EN | M_RX_FIFO_LAST_EN);
> + writel_relaxed(irq_en, uport->membase + SE_GENI_M_IRQ_EN);
> + }
> +
> + status = readl_relaxed(uport->membase + SE_GENI_STATUS);
> + /* Possible stop rx is called multiple times. */
> + if (!(status & S_GENI_CMD_ACTIVE))
> + return;
> +
> + /*
> + * Ensure cancel command write is not re-ordered before checking
> + * checking the status of the Secondary Sequencer.
Each of these comments has 'checking' twice.
> + */
> + mb();
> +
> + geni_se_cancel_s_cmd(&port->se);
> + qcom_geni_serial_poll_bit(uport, SE_GENI_S_CMD_CTRL_REG,
> + S_GENI_CMD_CANCEL, false);
> + status = readl_relaxed(uport->membase + SE_GENI_STATUS);
> + writel_relaxed(irq_clear, uport->membase + SE_GENI_S_IRQ_CLEAR);
> + if (status & S_GENI_CMD_ACTIVE)
> + qcom_geni_serial_abort_rx(uport);
> +}
> +
> +static void qcom_geni_serial_handle_rx(struct uart_port *uport, bool drop_rx)
s/drop_rx/drop/
> +{
> + u32 status;
> + u32 word_cnt;
> + u32 last_word_byte_cnt;
> + u32 last_word_partial;
> + u32 total_bytes;
> + struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
> +
> + status = readl_relaxed(uport->membase + SE_GENI_RX_FIFO_STATUS);
> + word_cnt = status & RX_FIFO_WC_MSK;
> + last_word_partial = status & RX_LAST;
> + last_word_byte_cnt = (status & RX_LAST_BYTE_VALID_MSK) >>
> + RX_LAST_BYTE_VALID_SHFT;
> +
> + if (!word_cnt)
> + return;
> + total_bytes = port->rx_bytes_pw * (word_cnt - 1);
> + if (last_word_partial && last_word_byte_cnt)
> + total_bytes += last_word_byte_cnt;
> + else
> + total_bytes += port->rx_bytes_pw;
> + port->handle_rx(uport, total_bytes, drop_rx);
> +}
> +
> +static int qcom_geni_serial_handle_tx(struct uart_port *uport)
> +{
> + int ret = 0;
> + struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
> + struct circ_buf *xmit = &uport->state->xmit;
> + size_t avail;
> + size_t remaining;
> + int i = 0;
> + u32 status;
> + unsigned int chunk;
> + int tail;
> +
> + chunk = uart_circ_chars_pending(xmit);
> + status = readl_relaxed(uport->membase + SE_GENI_TX_FIFO_STATUS);
> + /* Both FIFO and framework buffer are drained */
> + if ((chunk == port->xmit_size) && !status) {
Drop useless parenthesis.
> + port->xmit_size = 0;
> + uart_circ_clear(xmit);
> + qcom_geni_serial_stop_tx(uport);
> + goto out_write_wakeup;
> + }
> + chunk -= port->xmit_size;
> +
> + avail = (port->tx_fifo_depth - port->tx_wm) * port->tx_bytes_pw;
> + tail = (xmit->tail + port->xmit_size) & (UART_XMIT_SIZE - 1);
> + if (chunk > (UART_XMIT_SIZE - tail))
> + chunk = UART_XMIT_SIZE - tail;
> + if (chunk > avail)
> + chunk = avail;
> +
> + if (!chunk)
> + goto out_write_wakeup;
> +
> + qcom_geni_serial_setup_tx(uport, chunk);
> +
> + remaining = chunk;
> + while (i < chunk) {
for (i = 0; i < chunk; ) {
> + unsigned int tx_bytes;
> + unsigned int buf = 0;
> + int c;
> +
> + tx_bytes = min_t(size_t, remaining, (size_t)port->tx_bytes_pw);
> + for (c = 0; c < tx_bytes ; c++)
> + buf |= (xmit->buf[tail + c] << (c * BITS_PER_BYTE));
> +
> + writel_relaxed(buf, uport->membase + SE_GENI_TX_FIFOn);
> +
> + i += tx_bytes;
> + tail = (tail + tx_bytes) & (UART_XMIT_SIZE - 1);
> + uport->icount.tx += tx_bytes;
> + remaining -= tx_bytes;
> + }
> + qcom_geni_serial_poll_tx_done(uport);
> + port->xmit_size += chunk;
> +out_write_wakeup:
> + uart_write_wakeup(uport);
> + return ret;
> +}
> +
> +static irqreturn_t qcom_geni_serial_isr(int isr, void *dev)
> +{
> + unsigned int m_irq_status;
> + unsigned int s_irq_status;
> + struct uart_port *uport = dev;
> + unsigned long flags;
> + unsigned int m_irq_en;
> + bool drop_rx = false;
> + struct tty_port *tport = &uport->state->port;
> +
> + if (uport->suspended)
> + return IRQ_HANDLED;
> +
> + spin_lock_irqsave(&uport->lock, flags);
> + m_irq_status = readl_relaxed(uport->membase + SE_GENI_M_IRQ_STATUS);
> + s_irq_status = readl_relaxed(uport->membase + SE_GENI_S_IRQ_STATUS);
> + m_irq_en = readl_relaxed(uport->membase + SE_GENI_M_IRQ_EN);
> + writel_relaxed(m_irq_status, uport->membase + SE_GENI_M_IRQ_CLEAR);
> + writel_relaxed(s_irq_status, uport->membase + SE_GENI_S_IRQ_CLEAR);
> +
> + if (WARN_ON(m_irq_status & M_ILLEGAL_CMD_EN))
> + goto out_unlock;
> +
> + if (s_irq_status & S_RX_FIFO_WR_ERR_EN) {
> + uport->icount.overrun++;
> + tty_insert_flip_char(tport, 0, TTY_OVERRUN);
> + }
> +
> + if (m_irq_status & (M_TX_FIFO_WATERMARK_EN | M_CMD_DONE_EN) &&
> + m_irq_en & (M_TX_FIFO_WATERMARK_EN | M_CMD_DONE_EN))
> + qcom_geni_serial_handle_tx(uport);
> +
> + if (s_irq_status & S_GP_IRQ_0_EN || s_irq_status & S_GP_IRQ_1_EN) {
> + if (s_irq_status & S_GP_IRQ_0_EN)
> + uport->icount.parity++;
> + drop_rx = true;
> + } else if (s_irq_status & S_GP_IRQ_2_EN ||
> + s_irq_status & S_GP_IRQ_3_EN) {
> + uport->icount.brk++;
How does break character handling work? I see the accounting here, but
don't see any uart_handle_break() call anywhere.
> + }
> +
> + if (s_irq_status & S_RX_FIFO_WATERMARK_EN ||
> + s_irq_status & S_RX_FIFO_LAST_EN)
> + qcom_geni_serial_handle_rx(uport, drop_rx);
> +
> +out_unlock:
> + spin_unlock_irqrestore(&uport->lock, flags);
> + return IRQ_HANDLED;
> +}
> +
> +static int get_tx_fifo_size(struct qcom_geni_serial_port *port)
> +{
> + struct uart_port *uport;
> +
> + if (!port)
> + return -ENODEV;
> +
> + uport = &port->uport;
> + port->tx_fifo_depth = geni_se_get_tx_fifo_depth(&port->se);
> + if (!port->tx_fifo_depth) {
> + dev_err(uport->dev, "%s:Invalid TX FIFO depth read\n",
> + __func__);
> + return -ENXIO;
> + }
> +
> + port->tx_fifo_width = geni_se_get_tx_fifo_width(&port->se);
> + if (!port->tx_fifo_width) {
> + dev_err(uport->dev, "%s:Invalid TX FIFO width read\n",
> + __func__);
> + return -ENXIO;
> + }
> +
> + port->rx_fifo_depth = geni_se_get_rx_fifo_depth(&port->se);
> + if (!port->rx_fifo_depth) {
> + dev_err(uport->dev, "%s:Invalid RX FIFO depth read\n",
> + __func__);
> + return -ENXIO;
> + }
Are these checks verifying the hardware has a proper setting for fifo
depth and width? How is that possible to mess up? Do these ever fail?
> +
> + uport->fifosize =
> + (port->tx_fifo_depth * port->tx_fifo_width) / BITS_PER_BYTE;
> + return 0;
> +}
> +
> +static void set_rfr_wm(struct qcom_geni_serial_port *port)
> +{
> + /*
> + * Set RFR (Flow off) to FIFO_DEPTH - 2.
> + * RX WM level at 10% RX_FIFO_DEPTH.
> + * TX WM level at 10% TX_FIFO_DEPTH.
> + */
> + port->rx_rfr = port->rx_fifo_depth - 2;
> + port->rx_wm = UART_CONSOLE_RX_WM;
> + port->tx_wm = 2;
port->tx_wm = DEF_TX_WM?
> +}
> +
> +static void qcom_geni_serial_shutdown(struct uart_port *uport)
> +{
> + unsigned long flags;
> +
> + /* Stop the console before stopping the current tx */
> + console_stop(uport->cons);
> +
> + disable_irq(uport->irq);
> + free_irq(uport->irq, uport);
> + spin_lock_irqsave(&uport->lock, flags);
> + qcom_geni_serial_stop_tx(uport);
> + qcom_geni_serial_stop_rx(uport);
> + spin_unlock_irqrestore(&uport->lock, flags);
> +}
> +
> +static int qcom_geni_serial_port_setup(struct uart_port *uport)
> +{
> + int ret;
> + struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
> + unsigned int rxstale = DEFAULT_BITS_PER_CHAR * STALE_TIMEOUT;
> +
> + set_rfr_wm(port);
> + writel_relaxed(rxstale, uport->membase + SE_UART_RX_STALE_CNT);
> + /*
> + * Make an unconditional cancel on the main sequencer to reset
> + * it else we could end up in data loss scenarios.
> + */
> + port->xfer_mode = GENI_SE_FIFO;
> + qcom_geni_serial_poll_tx_done(uport);
> + geni_se_config_packing(&port->se, BITS_PER_BYTE, port->tx_bytes_pw,
> + false, true, false);
> + geni_se_config_packing(&port->se, BITS_PER_BYTE, port->rx_bytes_pw,
> + false, false, true);
> + ret = geni_se_init(&port->se, port->rx_wm, port->rx_rfr);
> + if (ret) {
> + dev_err(uport->dev, "%s: Fail\n", __func__);
> + return ret;
> + }
> +
> + geni_se_select_mode(&port->se, port->xfer_mode);
> + port->port_setup = true;
> + return ret;
> +}
> +
> +static int qcom_geni_serial_startup(struct uart_port *uport)
> +{
> + int ret;
> + struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
> +
> + scnprintf(port->name, sizeof(port->name),
> + "qcom_serial_geni%d", uport->line);
> +
> + if (geni_se_read_proto(&port->se) != GENI_SE_UART) {
> + dev_err(uport->dev, "Invalid FW %d loaded.\n",
> + geni_se_read_proto(&port->se));
Please don't read proto twice.
> + return -ENXIO;
> + }
> +
> + get_tx_fifo_size(port);
> + if (!port->port_setup) {
> + ret = qcom_geni_serial_port_setup(uport);
> + if (ret)
> + return ret;
> + }
> +
> + ret = request_irq(uport->irq, qcom_geni_serial_isr, IRQF_TRIGGER_HIGH,
> + port->name, uport);
> + if (ret)
> + dev_err(uport->dev, "Failed to get IRQ ret %d\n", ret);
> + return ret;
> +}
> +
> +static unsigned long get_clk_cfg(unsigned long clk_freq)
> +{
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(root_freq); i++) {
> + if (!(root_freq[i] % clk_freq))
> + return root_freq[i];
> + }
> + return 0;
> +}
> +
> +static void geni_serial_write_term_regs(struct uart_port *uport,
> + u32 tx_trans_cfg, u32 tx_parity_cfg, u32 rx_trans_cfg,
> + u32 rx_parity_cfg, u32 bits_per_char, u32 stop_bit_len,
> + u32 s_clk_cfg)
> +{
> + writel_relaxed(tx_trans_cfg, uport->membase + SE_UART_TX_TRANS_CFG);
> + writel_relaxed(tx_parity_cfg, uport->membase + SE_UART_TX_PARITY_CFG);
> + writel_relaxed(rx_trans_cfg, uport->membase + SE_UART_RX_TRANS_CFG);
> + writel_relaxed(rx_parity_cfg, uport->membase + SE_UART_RX_PARITY_CFG);
> + writel_relaxed(bits_per_char, uport->membase + SE_UART_TX_WORD_LEN);
> + writel_relaxed(bits_per_char, uport->membase + SE_UART_RX_WORD_LEN);
> + writel_relaxed(stop_bit_len, uport->membase + SE_UART_TX_STOP_BIT_LEN);
> + writel_relaxed(s_clk_cfg, uport->membase + GENI_SER_M_CLK_CFG);
> + writel_relaxed(s_clk_cfg, uport->membase + GENI_SER_S_CLK_CFG);
Can you please inline this function into the caller and put the writels
where the values are calculated? It would reduce the mental work to keep
track of all the variables to find out that they just get written in the
end. Also, this is weirdly placed in the file when get_clk_div_rate()
calls get_clk_cfg() but this function is between them.
> +}
> +
> +static unsigned long get_clk_div_rate(unsigned int baud, unsigned int *clk_div)
> +{
> + unsigned long ser_clk;
> + unsigned long desired_clk;
> +
> + desired_clk = baud * UART_OVERSAMPLING;
> + ser_clk = get_clk_cfg(desired_clk);
> + if (!ser_clk) {
> + pr_err("%s: Can't find matching DFS entry for baud %d\n",
> + __func__, baud);
> + return ser_clk;
> + }
> +
> + *clk_div = ser_clk / desired_clk;
How wide can clk_div be? It may be better to implement the ser_clk as an
actual clk in the common clk framework, and then have the serial driver
or the i2c driver call clk_set_rate() on that clk and have the CCF
implementation take care of determining the rate that the parent clk can
supply and how it can fit it into the frequency that the divider can
support.
> + return ser_clk;
> +}
> +
> +static void qcom_geni_serial_set_termios(struct uart_port *uport,
> + struct ktermios *termios, struct ktermios *old)
> +{
> + unsigned int baud;
> + unsigned int bits_per_char;
> + unsigned int tx_trans_cfg;
> + unsigned int tx_parity_cfg;
> + unsigned int rx_trans_cfg;
> + unsigned int rx_parity_cfg;
> + unsigned int stop_bit_len;
> + unsigned int clk_div;
> + unsigned long ser_clk_cfg;
> + struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
> + unsigned long clk_rate;
> +
> + qcom_geni_serial_stop_rx(uport);
> + /* baud rate */
> + baud = uart_get_baud_rate(uport, termios, old, 300, 4000000);
> + port->cur_baud = baud;
> + clk_rate = get_clk_div_rate(baud, &clk_div);
> + if (!clk_rate)
> + goto out_restart_rx;
> +
> + uport->uartclk = clk_rate;
> + clk_set_rate(port->se.clk, clk_rate);
> + ser_clk_cfg = SER_CLK_EN;
> + ser_clk_cfg |= (clk_div << CLK_DIV_SHFT);
Drop useless cast.
> +
> + /* parity */
> + tx_trans_cfg = readl_relaxed(uport->membase + SE_UART_TX_TRANS_CFG);
> + tx_parity_cfg = readl_relaxed(uport->membase + SE_UART_TX_PARITY_CFG);
> + rx_trans_cfg = readl_relaxed(uport->membase + SE_UART_RX_TRANS_CFG);
> + rx_parity_cfg = readl_relaxed(uport->membase + SE_UART_RX_PARITY_CFG);
> + if (termios->c_cflag & PARENB) {
> + tx_trans_cfg |= UART_TX_PAR_EN;
> + rx_trans_cfg |= UART_RX_PAR_EN;
> + tx_parity_cfg |= PAR_CALC_EN;
> + rx_parity_cfg |= PAR_CALC_EN;
> + if (termios->c_cflag & PARODD) {
> + tx_parity_cfg |= PAR_ODD;
> + rx_parity_cfg |= PAR_ODD;
> + } else if (termios->c_cflag & CMSPAR) {
> + tx_parity_cfg |= PAR_SPACE;
> + rx_parity_cfg |= PAR_SPACE;
> + } else {
> + tx_parity_cfg |= PAR_EVEN;
> + rx_parity_cfg |= PAR_EVEN;
> + }
> + } else {
> + tx_trans_cfg &= ~UART_TX_PAR_EN;
> + rx_trans_cfg &= ~UART_RX_PAR_EN;
> + tx_parity_cfg &= ~PAR_CALC_EN;
> + rx_parity_cfg &= ~PAR_CALC_EN;
> + }
> +
> + /* bits per char */
> + switch (termios->c_cflag & CSIZE) {
> + case CS5:
> + bits_per_char = 5;
> + break;
> + case CS6:
> + bits_per_char = 6;
> + break;
> + case CS7:
> + bits_per_char = 7;
> + break;
> + case CS8:
> + default:
> + bits_per_char = 8;
> + break;
> + }
> +
> + /* stop bits */
> + if (termios->c_cflag & CSTOPB)
> + stop_bit_len = TX_STOP_BIT_LEN_2;
> + else
> + stop_bit_len = TX_STOP_BIT_LEN_1;
> +
> + /* flow control, clear the CTS_MASK bit if using flow control. */
> + if (termios->c_cflag & CRTSCTS)
> + tx_trans_cfg &= ~UART_CTS_MASK;
> + else
> + tx_trans_cfg |= UART_CTS_MASK;
> +
> + if (baud)
> + uart_update_timeout(uport, termios->c_cflag, baud);
> +
> + geni_serial_write_term_regs(uport, tx_trans_cfg, tx_parity_cfg,
> + rx_trans_cfg, rx_parity_cfg, bits_per_char, stop_bit_len,
> + ser_clk_cfg);
> +out_restart_rx:
> + qcom_geni_serial_start_rx(uport);
> +}
> +
> +static unsigned int qcom_geni_serial_tx_empty(struct uart_port *uport)
> +{
> + return !readl_relaxed(uport->membase + SE_GENI_TX_FIFO_STATUS);
> +}
> +
> +#if defined(CONFIG_SERIAL_CORE_CONSOLE) || defined(CONFIG_CONSOLE_POLL)
> +static int __init qcom_geni_console_setup(struct console *co, char *options)
> +{
> + struct uart_port *uport;
> + struct qcom_geni_serial_port *port;
> + int baud;
> + int bits = 8;
> + int parity = 'n';
> + int flow = 'n';
> +
> + if (co->index >= GENI_UART_CONS_PORTS || co->index < 0)
> + return -ENXIO;
> +
> + port = get_port_from_line(co->index);
> + if (IS_ERR(port)) {
> + pr_err("Invalid line %d(%d)\n", co->index, (int)PTR_ERR(port));
> + return PTR_ERR(port);
> + }
> +
> + uport = &port->uport;
> +
> + if (unlikely(!uport->membase))
> + return -ENXIO;
> +
> + if (geni_se_resources_on(&port->se)) {
> + dev_err(port->se.dev, "Error turning on resources\n");
> + return -ENXIO;
> + }
> +
> + if (unlikely(geni_se_read_proto(&port->se) != GENI_SE_UART)) {
Looks like we're validating the configuration of the DT here. Maybe this
can go into the wrapper code and be put behind some DEBUG_KERNEL check
so we can debug bad bootloader configurations if needed? Especially if
this is the only API that's left exposed from the wrapper to the serial
engine/protocol driver.
> + geni_se_resources_off(&port->se);
> + return -ENXIO;
> + }
> +
> + if (!port->port_setup) {
> + port->tx_bytes_pw = 1;
> + port->rx_bytes_pw = RX_BYTES_PW;
> + qcom_geni_serial_stop_rx(uport);
> + qcom_geni_serial_port_setup(uport);
> + }
> +
> + if (options)
> + uart_parse_options(options, &baud, &parity, &bits, &flow);
> +
> + return uart_set_options(uport, co, baud, parity, bits, flow);
> +}
> +
> +static int console_register(struct uart_driver *drv)
__init
> +{
> + return uart_register_driver(drv);
> +}
> +
> +static void console_unregister(struct uart_driver *drv)
> +{
> + uart_unregister_driver(drv);
> +}
> +
> +static struct console cons_ops = {
> + .name = "ttyMSM",
> + .write = qcom_geni_serial_console_write,
> + .device = uart_console_device,
> + .setup = qcom_geni_console_setup,
> + .flags = CON_PRINTBUFFER,
> + .index = -1,
> + .data = &qcom_geni_console_driver,
> +};
> +
> +static struct uart_driver qcom_geni_console_driver = {
> + .owner = THIS_MODULE,
> + .driver_name = "qcom_geni_console",
> + .dev_name = "ttyMSM",
> + .nr = GENI_UART_CONS_PORTS,
> + .cons = &cons_ops,
> +};
> +#else
> +static int console_register(struct uart_driver *drv)
> +{
> + return 0;
> +}
> +
> +static void console_unregister(struct uart_driver *drv)
> +{
> +}
> +#endif /* defined(CONFIG_SERIAL_CORE_CONSOLE) || defined(CONFIG_CONSOLE_POLL) */
> +
> +static void qcom_geni_serial_cons_pm(struct uart_port *uport,
> + unsigned int new_state, unsigned int old_state)
> +{
> + struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
> +
> + if (unlikely(!uart_console(uport)))
> + return;
> +
> + if (new_state == UART_PM_STATE_ON && old_state == UART_PM_STATE_OFF)
> + geni_se_resources_on(&port->se);
> + else if (new_state == UART_PM_STATE_OFF &&
> + old_state == UART_PM_STATE_ON)
> + geni_se_resources_off(&port->se);
> +}
> +
> +static const struct uart_ops qcom_geni_console_pops = {
> + .tx_empty = qcom_geni_serial_tx_empty,
> + .stop_tx = qcom_geni_serial_stop_tx,
> + .start_tx = qcom_geni_serial_start_tx,
> + .stop_rx = qcom_geni_serial_stop_rx,
> + .set_termios = qcom_geni_serial_set_termios,
> + .startup = qcom_geni_serial_startup,
> + .config_port = qcom_geni_serial_config_port,
> + .shutdown = qcom_geni_serial_shutdown,
> + .type = qcom_geni_serial_get_type,
> + .set_mctrl = qcom_geni_cons_set_mctrl,
> + .get_mctrl = qcom_geni_cons_get_mctrl,
> +#ifdef CONFIG_CONSOLE_POLL
> + .poll_get_char = qcom_geni_serial_get_char,
> + .poll_put_char = qcom_geni_serial_poll_put_char,
> +#endif
> + .pm = qcom_geni_serial_cons_pm,
> +};
> +
> +static int qcom_geni_serial_probe(struct platform_device *pdev)
> +{
> + int ret = 0;
> + int line = -1;
> + struct qcom_geni_serial_port *port;
> + struct uart_port *uport;
> + struct resource *res;
> + struct uart_driver *drv;
> +
> + drv = (void *)of_device_get_match_data(&pdev->dev);
Useless cast.
> + if (!drv) {
> + dev_err(&pdev->dev, "%s: No matching device found", __func__);
> + return -ENODEV;
> + }
> +
> + if (pdev->dev.of_node)
> + line = of_alias_get_id(pdev->dev.of_node, "serial");
> + else
> + line = pdev->id;
> +
> + if (line < 0)
> + line = atomic_inc_return(&uart_line_id) - 1;
> +
> + if ((line < 0) || (line >= GENI_UART_CONS_PORTS))
Useless parenthesis.
> + return -ENXIO;
> + port = get_port_from_line(line);
> + if (IS_ERR(port)) {
> + ret = PTR_ERR(port);
> + dev_err(&pdev->dev, "Invalid line %d(%d)\n", line, ret);
> + return ret;
> + }
> +
> + uport = &port->uport;
> + /* Don't allow 2 drivers to access the same port */
> + if (uport->private_data)
> + return -ENODEV;
> +
> + uport->dev = &pdev->dev;
> + port->se.dev = &pdev->dev;
> + port->se.wrapper = dev_get_drvdata(pdev->dev.parent);
> + port->se.clk = devm_clk_get(&pdev->dev, "se");
> + if (IS_ERR(port->se.clk)) {
> + ret = PTR_ERR(port->se.clk);
> + dev_err(&pdev->dev, "Err getting SE Core clk %d\n", ret);
> + return ret;
> + }
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + uport->mapbase = res->start;
> + uport->membase = devm_ioremap_resource(&pdev->dev, res);
> + if (!uport->membase) {
Check for IS_ERR()
> + dev_err(&pdev->dev, "Err IO mapping serial iomem");
No need for error message with devm_ioremap_resource()
> + return -ENOMEM;
return PTR_ERR(..)
Also, I see some serial drivers do the mapping when the port is
requested. That can't be done here?
> + }
> + port->se.base = uport->membase;
> +
> + port->tx_fifo_depth = DEF_FIFO_DEPTH_WORDS;
> + port->rx_fifo_depth = DEF_FIFO_DEPTH_WORDS;
> + port->tx_fifo_width = DEF_FIFO_WIDTH_BITS;
> +
> + uport->irq = platform_get_irq(pdev, 0);
> + if (uport->irq < 0) {
> + dev_err(&pdev->dev, "Failed to get IRQ %d\n", uport->irq);
> + return uport->irq;
> + }
> +
> + uport->private_data = drv;
> + platform_set_drvdata(pdev, port);
> + port->handle_rx = handle_rx_console;
> + port->port_setup = false;
> + return uart_add_one_port(drv, uport);
> +}
> +
> +static int qcom_geni_serial_remove(struct platform_device *pdev)
> +{
> + struct qcom_geni_serial_port *port = platform_get_drvdata(pdev);
> + struct uart_driver *drv = port->uport.private_data;
> +
> + uart_remove_one_port(drv, &port->uport);
> + return 0;
> +}
> +
> +static int __maybe_unused qcom_geni_serial_sys_suspend_noirq(struct device *dev)
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + struct qcom_geni_serial_port *port = platform_get_drvdata(pdev);
> + struct uart_port *uport = &port->uport;
> +
> + uart_suspend_port(uport->private_data, uport);
> + return 0;
> +}
> +
> +static int __maybe_unused qcom_geni_serial_sys_resume_noirq(struct device *dev)
> +{
> + struct platform_device *pdev = to_platform_device(dev);
> + struct qcom_geni_serial_port *port = platform_get_drvdata(pdev);
> + struct uart_port *uport = &port->uport;
> +
> + if (console_suspend_enabled && uport->suspended) {
> + uart_resume_port(uport->private_data, uport);
> + disable_irq(uport->irq);
> + }
> + return 0;
> +}
> +
> +static const struct dev_pm_ops qcom_geni_serial_pm_ops = {
> + .suspend_noirq = qcom_geni_serial_sys_suspend_noirq,
> + .resume_noirq = qcom_geni_serial_sys_resume_noirq,
Why are these noirq variants? Please add a comment.
> +};
> +
> +static const struct of_device_id qcom_geni_serial_match_table[] = {
> + { .compatible = "qcom,geni-debug-uart",
> + .data = &qcom_geni_console_driver, },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, qcom_geni_serial_match_table);
> +
> +static struct platform_driver qcom_geni_serial_platform_driver = {
> + .remove = qcom_geni_serial_remove,
> + .probe = qcom_geni_serial_probe,
> + .driver = {
> + .name = "qcom_geni_serial",
> + .of_match_table = qcom_geni_serial_match_table,
> + .pm = &qcom_geni_serial_pm_ops,
> + },
> +};
> +
> +static int __init qcom_geni_serial_init(void)
> +{
> + int ret = 0;
Drop assignment please.
> +
> + qcom_geni_console_port.uport.iotype = UPIO_MEM;
> + qcom_geni_console_port.uport.ops = &qcom_geni_console_pops;
> + qcom_geni_console_port.uport.flags = UPF_BOOT_AUTOCONF;
> + qcom_geni_console_port.uport.line = 0;
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] xfs: Change URL for the project in xfs.txt
From: Dave Chinner @ 2018-03-02 22:43 UTC (permalink / raw)
To: Eric Sandeen
Cc: Darrick J. Wong, Masanari Iida, linux-xfs, linux-kernel, corbet,
linux-doc
In-Reply-To: <27d9f522-fa75-6766-3ca6-79f99522e861@sandeen.net>
On Fri, Mar 02, 2018 at 04:08:24PM -0600, Eric Sandeen wrote:
>
>
> On 3/2/18 3:57 PM, Dave Chinner wrote:
> > On Fri, Mar 02, 2018 at 09:24:01AM -0800, Darrick J. Wong wrote:
> >> On Fri, Mar 02, 2018 at 10:30:13PM +0900, Masanari Iida wrote:
> >>> The oss.sgi.com doesn't exist any more.
> >>> Change it to current project URL, https://xfs.wiki.kernel.org/
> >>>
> >>> Signed-off-by: Masanari Iida <standby24x7@gmail.com>
> >>> ---
> >>> Documentation/filesystems/xfs.txt | 2 +-
> >>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/Documentation/filesystems/xfs.txt b/Documentation/filesystems/xfs.txt
> >>> index 3b9b5c149f32..4d9ff0a7f8e1 100644
> >>> --- a/Documentation/filesystems/xfs.txt
> >>> +++ b/Documentation/filesystems/xfs.txt
> >>> @@ -9,7 +9,7 @@ variable block sizes, is extent based, and makes extensive use of
> >>> Btrees (directories, extents, free space) to aid both performance
> >>> and scalability.
> >>>
> >>> -Refer to the documentation at http://oss.sgi.com/projects/xfs/
> >>> +Refer to the documentation at https://xfs.wiki.kernel.org/
> >
> > Did I miss a memo?
>
> About which part, the loss of oss.sgi or the addition of the kernel.org wiki?
>
> The kernel.org wiki is pretty bare though. OTOH xfs.org is a bit less
> official. We really need to resolve this issue.
Moving everything to kernel.org wiki. As I mentioned on IRC, I'd
much prefer we move away from wiki's to something we can edit
locally, review via email, has proper revision control and a
"publish" mechanism that pushes built documentation out to the
public website.
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v2 0/2] mux: add overview and add to driver-api docs
From: Peter Rosin @ 2018-03-03 0:04 UTC (permalink / raw)
To: Jonathan Corbet; +Cc: linux-kernel, linux-doc
In-Reply-To: <4fc09b21-6226-5b82-a841-cb4bf1334a5c@axentia.se>
On 2017-12-21 23:11, Peter Rosin wrote:
> On 2017-12-21 22:35, Jonathan Corbet wrote:
>> On Tue, 12 Dec 2017 09:46:31 +0100
>> Peter Rosin <peda@axentia.se> wrote:
>>
>>> Changes since v1:
>>> - added a short introductory paragraph to mux.rst
>>> - added an entry in MAINTAINERS.
>>>
>>> I forgot to mention that this applies on top of linux-next, I suspect
>>> there will be a trivial conflict with an SPDX conversion in mux/core.c
>>> on Linus upstream.
>>
>> Sorry, I've had this just sitting here for a bit. Did you want me to take
>> both patches through the docs tree?
>
> I think that's going to be easiest, thanks! Let me know if you want me
> to rebase them to something w/o the SPDX change.
Hi,
Gentle ping...
Cheers,
Peter
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v3 4/4] tty: serial: msm_geni_serial: Add serial driver support for GENI based QUP
From: Evan Green @ 2018-03-03 0:11 UTC (permalink / raw)
To: Karthikeyan Ramasubramanian
Cc: Jonathan Corbet, Andy Gross, David Brown, robh+dt, mark.rutland,
wsa, gregkh, linux-doc, linux-arm-msm, devicetree, linux-i2c,
linux-serial, jslaby, acourbot, Girish Mahadevan, Sagar Dharia,
Doug Anderson
In-Reply-To: <1519781889-16117-5-git-send-email-kramasub@codeaurora.org>
Hello Karthik,
On Tue, Feb 27, 2018 at 5:38 PM, Karthikeyan Ramasubramanian
<kramasub@codeaurora.org> wrote:
> This driver supports GENI based UART Controller in the Qualcomm SOCs. The
> Qualcomm Generic Interface (GENI) is a programmable module supporting a
> wide range of serial interfaces including UART. This driver support console
> operations using FIFO mode of transfer.
>
> Signed-off-by: Girish Mahadevan <girishm@codeaurora.org>
> Signed-off-by: Karthikeyan Ramasubramanian <kramasub@codeaurora.org>
> Signed-off-by: Sagar Dharia <sdharia@codeaurora.org>
> Signed-off-by: Doug Anderson <dianders@google.com>
> ---
> drivers/tty/serial/Kconfig | 11 +
> drivers/tty/serial/Makefile | 1 +
> drivers/tty/serial/qcom_geni_serial.c | 1181 +++++++++++++++++++++++++++++++++
> 3 files changed, 1193 insertions(+)
> create mode 100644 drivers/tty/serial/qcom_geni_serial.c
>
> diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
> index 3682fd3..c6b1500 100644
> --- a/drivers/tty/serial/Kconfig
> +++ b/drivers/tty/serial/Kconfig
> @@ -1104,6 +1104,17 @@ config SERIAL_MSM_CONSOLE
> select SERIAL_CORE_CONSOLE
> select SERIAL_EARLYCON
>
> +config SERIAL_QCOM_GENI
> + bool "QCOM on-chip GENI based serial port support"
> + depends on ARCH_QCOM
> + depends on QCOM_GENI_SE
My understanding is that this has to be "bool" because there's a
console in there, and consoles cannot be built as modules. Stephen is
suggesting splitting this option up into two, so you could have serial
with or without the console. That's fine, and probably the preferred
way. However, you do want to make sure that if serial (or what's soon
to be serial+console) is enabled, that QCOM_GENI_SE has to be built
=y, and not =m. I'd suggest "select QCOM_GENI_SE" in the new
SERIAL_QCOM_GENI_CONSOLE (or whatever it's called). As it is now, if
SERIAL_QCOM_GENI=y and QCOM_GENI_SE=m, there's a build failure.
GENI_SE is allowed to be built as a module if serial is not enabled
and I2C is built as a module. In order to keep the dependency arrows
going in the same direction, you might want the I2C driver to "select
QCOM_GENI_SE" as well, in order to upgrade GENI_SE to y if I2C is y.
> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
> new file mode 100644
> index 0000000..8536b7d
> --- /dev/null
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -0,0 +1,1181 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2017-2018, The Linux foundation. All rights reserved.
> +
> +#include <linux/console.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/qcom-geni-se.h>
> +#include <linux/serial.h>
> +#include <linux/serial_core.h>
> +#include <linux/slab.h>
> +#include <linux/tty.h>
> +#include <linux/tty_flip.h>
> +
> +/* UART specific GENI registers */
> +#define SE_UART_TX_TRANS_CFG 0x25c
> +#define SE_UART_TX_WORD_LEN 0x268
> +#define SE_UART_TX_STOP_BIT_LEN 0x26c
> +#define SE_UART_TX_TRANS_LEN 0x270
> +#define SE_UART_RX_TRANS_CFG 0x280
> +#define SE_UART_RX_WORD_LEN 0x28c
> +#define SE_UART_RX_STALE_CNT 0x294
> +#define SE_UART_TX_PARITY_CFG 0x2a4
> +#define SE_UART_RX_PARITY_CFG 0x2a8
> +
> +/* SE_UART_TRANS_CFG */
> +#define UART_TX_PAR_EN BIT(0)
> +#define UART_CTS_MASK BIT(1)
> +
> +/* SE_UART_TX_WORD_LEN */
> +#define TX_WORD_LEN_MSK GENMASK(9, 0)
> +
> +/* SE_UART_TX_STOP_BIT_LEN */
> +#define TX_STOP_BIT_LEN_MSK GENMASK(23, 0)
> +#define TX_STOP_BIT_LEN_1 0
> +#define TX_STOP_BIT_LEN_1_5 1
> +#define TX_STOP_BIT_LEN_2 2
> +
> +/* SE_UART_TX_TRANS_LEN */
> +#define TX_TRANS_LEN_MSK GENMASK(23, 0)
> +
> +/* SE_UART_RX_TRANS_CFG */
> +#define UART_RX_INS_STATUS_BIT BIT(2)
> +#define UART_RX_PAR_EN BIT(3)
> +
> +/* SE_UART_RX_WORD_LEN */
> +#define RX_WORD_LEN_MASK GENMASK(9, 0)
> +
> +/* SE_UART_RX_STALE_CNT */
> +#define RX_STALE_CNT GENMASK(23, 0)
> +
> +/* SE_UART_TX_PARITY_CFG/RX_PARITY_CFG */
> +#define PAR_CALC_EN BIT(0)
> +#define PAR_MODE_MSK GENMASK(2, 1)
> +#define PAR_MODE_SHFT 1
> +#define PAR_EVEN 0x00
> +#define PAR_ODD 0x01
> +#define PAR_SPACE 0x10
> +#define PAR_MARK 0x11
> +
> +/* UART M_CMD OP codes */
> +#define UART_START_TX 0x1
> +#define UART_START_BREAK 0x4
> +#define UART_STOP_BREAK 0x5
> +/* UART S_CMD OP codes */
> +#define UART_START_READ 0x1
> +#define UART_PARAM 0x1
> +
> +#define UART_OVERSAMPLING 32
> +#define STALE_TIMEOUT 16
> +#define DEFAULT_BITS_PER_CHAR 10
> +#define GENI_UART_CONS_PORTS 1
> +#define DEF_FIFO_DEPTH_WORDS 16
> +#define DEF_TX_WM 2
> +#define DEF_FIFO_WIDTH_BITS 32
> +#define UART_CONSOLE_RX_WM 2
> +
> +#ifdef CONFIG_CONSOLE_POLL
> +#define RX_BYTES_PW 1
> +#else
> +#define RX_BYTES_PW 4
> +#endif
This seems fishy to me. Does either setting work? If so, why not just
have one value?
> +static bool qcom_geni_serial_poll_bit(struct uart_port *uport,
> + int offset, int bit_field, bool set)
> +{
> + u32 reg;
> + struct qcom_geni_serial_port *port;
> + unsigned int baud;
> + unsigned int fifo_bits;
> + unsigned long timeout_us = 20000;
> +
> + /* Ensure polling is not re-ordered before the prior writes/reads */
> + mb();
> +
> + if (uport->private_data) {
> + port = to_dev_port(uport, uport);
> + baud = port->cur_baud;
> + if (!baud)
> + baud = 115200;
> + fifo_bits = port->tx_fifo_depth * port->tx_fifo_width;
> + /*
> + * Total polling iterations based on FIFO worth of bytes to be
> + * sent at current baud .Add a little fluff to the wait.
> + */
> + timeout_us = ((fifo_bits * USEC_PER_SEC) / baud) + 500;
This fluff is a little mysterious, can it be explained at all? Do you
think the fluff factor is in units of time (as you have it) or bits?
Time makes sense I guess if we're worried about clock source
differences.
> +
> +static void qcom_geni_serial_console_write(struct console *co, const char *s,
> + unsigned int count)
> +{
> + struct uart_port *uport;
> + struct qcom_geni_serial_port *port;
> + bool locked = true;
> + unsigned long flags;
> +
> + WARN_ON(co->index < 0 || co->index >= GENI_UART_CONS_PORTS);
> +
> + port = get_port_from_line(co->index);
> + if (IS_ERR(port))
> + return;
> +
> + uport = &port->uport;
> + if (oops_in_progress)
> + locked = spin_trylock_irqsave(&uport->lock, flags);
> + else
> + spin_lock_irqsave(&uport->lock, flags);
> +
> + if (locked) {
> + __qcom_geni_serial_console_write(uport, s, count);
> + spin_unlock_irqrestore(&uport->lock, flags);
I too am a little lost on the locking here. What exactly is the lock
protecting? Looks like for the most part it's trying to synchronize
with the ISR? What specifically in the ISR? I just wanted to go
through and check to make sure whatever the shared resource is is
appropriately protected.
> + }
> +}
> +
> +static int handle_rx_console(struct uart_port *uport, u32 rx_bytes, bool drop)
> +{
> + u32 i = rx_bytes;
> + u32 rx_fifo;
> + unsigned char *buf;
> + struct tty_port *tport;
> + struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
> +
> + tport = &uport->state->port;
> + while (i > 0) {
> + int c;
> + int bytes = i > port->rx_bytes_pw ? port->rx_bytes_pw : i;
Please replace this with a min macro.
> +static int qcom_geni_serial_handle_tx(struct uart_port *uport)
> +{
> + int ret = 0;
> + struct qcom_geni_serial_port *port = to_dev_port(uport, uport);
> + struct circ_buf *xmit = &uport->state->xmit;
> + size_t avail;
> + size_t remaining;
> + int i = 0;
> + u32 status;
> + unsigned int chunk;
> + int tail;
> +
> + chunk = uart_circ_chars_pending(xmit);
> + status = readl_relaxed(uport->membase + SE_GENI_TX_FIFO_STATUS);
> + /* Both FIFO and framework buffer are drained */
> + if ((chunk == port->xmit_size) && !status) {
> + port->xmit_size = 0;
> + uart_circ_clear(xmit);
> + qcom_geni_serial_stop_tx(uport);
> + goto out_write_wakeup;
> + }
> + chunk -= port->xmit_size;
> +
> + avail = (port->tx_fifo_depth - port->tx_wm) * port->tx_bytes_pw;
> + tail = (xmit->tail + port->xmit_size) & (UART_XMIT_SIZE - 1);
> + if (chunk > (UART_XMIT_SIZE - tail))
> + chunk = UART_XMIT_SIZE - tail;
> + if (chunk > avail)
> + chunk = avail;
> +
> + if (!chunk)
> + goto out_write_wakeup;
> +
> + qcom_geni_serial_setup_tx(uport, chunk);
> +
> + remaining = chunk;
> + while (i < chunk) {
> + unsigned int tx_bytes;
> + unsigned int buf = 0;
> + int c;
> +
> + tx_bytes = min_t(size_t, remaining, (size_t)port->tx_bytes_pw);
> + for (c = 0; c < tx_bytes ; c++)
> + buf |= (xmit->buf[tail + c] << (c * BITS_PER_BYTE));
> +
> + writel_relaxed(buf, uport->membase + SE_GENI_TX_FIFOn);
> +
> + i += tx_bytes;
> + tail = (tail + tx_bytes) & (UART_XMIT_SIZE - 1);
> + uport->icount.tx += tx_bytes;
> + remaining -= tx_bytes;
> + }
> + qcom_geni_serial_poll_tx_done(uport);
> + port->xmit_size += chunk;
> +out_write_wakeup:
> + uart_write_wakeup(uport);
> + return ret;
> +}
This function can't fail, please change the return type to void.
> +
> +static void qcom_geni_serial_shutdown(struct uart_port *uport)
> +{
> + unsigned long flags;
> +
> + /* Stop the console before stopping the current tx */
> + console_stop(uport->cons);
> +
> + disable_irq(uport->irq);
> + free_irq(uport->irq, uport);
> + spin_lock_irqsave(&uport->lock, flags);
> + qcom_geni_serial_stop_tx(uport);
> + qcom_geni_serial_stop_rx(uport);
> + spin_unlock_irqrestore(&uport->lock, flags);
This is one part of where I'm confused. What are we protecting here
with the lock? disable_irq waits for any pending ISRs to finish
according to its comment, so you know you're not racing with the ISR.
> +static void geni_serial_write_term_regs(struct uart_port *uport,
> + u32 tx_trans_cfg, u32 tx_parity_cfg, u32 rx_trans_cfg,
> + u32 rx_parity_cfg, u32 bits_per_char, u32 stop_bit_len,
> + u32 s_clk_cfg)
> +{
> + writel_relaxed(tx_trans_cfg, uport->membase + SE_UART_TX_TRANS_CFG);
> + writel_relaxed(tx_parity_cfg, uport->membase + SE_UART_TX_PARITY_CFG);
> + writel_relaxed(rx_trans_cfg, uport->membase + SE_UART_RX_TRANS_CFG);
> + writel_relaxed(rx_parity_cfg, uport->membase + SE_UART_RX_PARITY_CFG);
> + writel_relaxed(bits_per_char, uport->membase + SE_UART_TX_WORD_LEN);
> + writel_relaxed(bits_per_char, uport->membase + SE_UART_RX_WORD_LEN);
> + writel_relaxed(stop_bit_len, uport->membase + SE_UART_TX_STOP_BIT_LEN);
> + writel_relaxed(s_clk_cfg, uport->membase + GENI_SER_M_CLK_CFG);
> + writel_relaxed(s_clk_cfg, uport->membase + GENI_SER_S_CLK_CFG);
> +}
> +
I agree with Stephen's comment, this should be inlined into the single
place it's called from.
Thanks Karthik!
-Evan
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v3 2/4] soc: qcom: Add GENI based QUP Wrapper driver
From: Karthik Ramasubramanian @ 2018-03-03 0:58 UTC (permalink / raw)
To: Stephen Boyd, andy.gross, corbet, david.brown, gregkh,
mark.rutland, robh+dt, wsa
Cc: linux-doc, linux-arm-msm, devicetree, linux-i2c, linux-serial,
jslaby, evgreen, acourbot, Sagar Dharia, Girish Mahadevan
In-Reply-To: <152002326567.108663.16836885081217739460@swboyd.mtv.corp.google.com>
On 3/2/2018 1:41 PM, Stephen Boyd wrote:
> Quoting Karthikeyan Ramasubramanian (2018-02-27 17:38:07)
>> This driver manages the Generic Interface (GENI) firmware based Qualcomm
>> Universal Peripheral (QUP) Wrapper. GENI based QUP is the next generation
>> programmable module composed of multiple Serial Engines (SE) and supports
>> a wide range of serial interfaces like UART, SPI, I2C, I3C, etc. This
>> driver also enables managing the serial interface independent aspects of
>> Serial Engines.
>>
>> Signed-off-by: Karthikeyan Ramasubramanian <kramasub@codeaurora.org>
>> Signed-off-by: Sagar Dharia <sdharia@codeaurora.org>
>> Signed-off-by: Girish Mahadevan <girishm@codeaurora.org>
>> ---
>> drivers/soc/qcom/Kconfig | 9 +
>> drivers/soc/qcom/Makefile | 1 +
>> drivers/soc/qcom/qcom-geni-se.c | 971 ++++++++++++++++++++++++++++++++++++++++
>> include/linux/qcom-geni-se.h | 247 ++++++++++
>> 4 files changed, 1228 insertions(+)
>> create mode 100644 drivers/soc/qcom/qcom-geni-se.c
>> create mode 100644 include/linux/qcom-geni-se.h
>>
>> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
>> index e050eb8..cc460d0 100644
>> --- a/drivers/soc/qcom/Kconfig
>> +++ b/drivers/soc/qcom/Kconfig
>> @@ -3,6 +3,15 @@
>> #
>> menu "Qualcomm SoC drivers"
>>
>> +config QCOM_GENI_SE
>> + tristate "QCOM GENI Serial Engine Driver"
>> + depends on ARCH_QCOM
>
> Add || COMPILE_TEST?
Ok.
>
>> + help
>> + This module is used to manage Generic Interface (GENI) firmware based
>
> s/module/driver?
>
>> + Qualcomm Technologies, Inc. Universal Peripheral (QUP) Wrapper. This
>> + module is also used to manage the common aspects of multiple Serial
> s/module/driver?
Ok.
>
>> + Engines present in the QUP.
>> +
>> config QCOM_GLINK_SSR
>> tristate "Qualcomm Glink SSR driver"
>> depends on RPMSG
>> diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
>> new file mode 100644
>> index 0000000..61335b8
>> --- /dev/null
>> +++ b/drivers/soc/qcom/qcom-geni-se.c
>> @@ -0,0 +1,971 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
>> +
>> +#include <linux/clk.h>
>> +#include <linux/slab.h>
>> +#include <linux/dma-mapping.h>
>> +#include <linux/io.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/qcom-geni-se.h>
>
> #include <linux/platform_device.h>
Ok
>
>> +
>> +/**
>> + * DOC: Overview
>> + *
>> + * Generic Interface (GENI) Serial Engine (SE) Wrapper driver is introduced
>> + * to manage GENI firmware based Qualcomm Universal Peripheral (QUP) Wrapper
>> + * controller. QUP Wrapper is designed to support various serial bus protocols
>> + * like UART, SPI, I2C, I3C, etc.
>> + */
>> +
>> +/**
>> + * DOC: Hardware description
>> + *
>> + * GENI based QUP is a highly-flexible and programmable module for supporting
>> + * a wide range of serial interfaces like UART, SPI, I2C, I3C, etc. A single
>> + * QUP module can provide upto 8 Serial Interfaces, using its internal
>> + * Serial Engines. The actual configuration is determined by the target
>> + * platform configuration. The protocol supported by each interface is
>> + * determined by the firmware loaded to the Serial Engine. Each SE consists
>> + * of a DMA Engine and GENI sub modules which enable Serial Engines to
>> + * support FIFO and DMA modes of operation.
>> + *
>> + *
>> + * +-----------------------------------------+
>> + * |QUP Wrapper |
>> + * | +----------------------------+ |
>> + * --QUP & SE Clocks--> | Serial Engine N | +-IO------>
>> + * | | ... | | Interface
>> + * <---Clock Perf.----+ +----+-----------------------+ | |
>> + * State Interface | | Serial Engine 1 | | |
>> + * | | | | |
>> + * | | | | |
>> + * <--------AHB-------> | | | |
>> + * | | +----+ |
>> + * | | | |
>> + * | | | |
>> + * <------SE IRQ------+ +----------------------------+ |
>> + * | |
>> + * +-----------------------------------------+
>> + *
>> + * Figure 1: GENI based QUP Wrapper
>
> The code talks about primary and secondary sequencers, but this hardware
> description doesn't talk about it. Can you add some more information
> here about that aspect too?
Ok.
>
>> + */
>> +
>> +/**
>> + * DOC: Software description
>> + *
>> + * GENI SE Wrapper driver is structured into 2 parts:
>> + *
>> + * geni_wrapper represents QUP Wrapper controller. This part of the driver
>> + * manages QUP Wrapper information such as hardware version, clock
>> + * performance table that is common to all the internal Serial Engines.
>> + *
>> + * geni_se represents Serial Engine. This part of the driver manages Serial
>> + * Engine information such as clocks, containing QUP Wrapper etc. This part
>
> Insert a comma here ^
Ok.
>
>> + * of driver also supports operations(eg. initialize the concerned Serial
>
> Space
Ok.
^
>
>> + * Engine, select between FIFO and DMA mode of operation etc.) that are
>> + * common to all the Serial Engines and are independent of Serial Interfaces.
>
> Why are Serial Interfaces and Serial Engine always capitalized?
No special reason. I will change it to small letter.
>
>> + */
>> +
>> +#define MAX_CLK_PERF_LEVEL 32
>> +#define NUM_AHB_CLKS 2
>> +static const char m_ahb_clk[] = "m-ahb";
>> +static const char s_ahb_clk[] = "s-ahb";
>
> These are used in one place. Inline them?
Ok.
>
>> +
>> +/**
>> + * @struct geni_wrapper - Data structure to represent the QUP Wrapper Core
>> + * @dev: Device pointer of the QUP wrapper core.
>> + * @base: Base address of this instance of QUP wrapper core.
>> + * @ahb_clks: Handle to the primary & secondary AHB clocks.
>> + * @lock: Lock to protect the device elements.
>
> What does 'device elements' mean?
It means members of geni_wrapper structure. I will document that way.
>
>> + * @num_clk_levels: Number of valid clock levels in clk_perf_tbl.
>> + * @clk_perf_tbl: Table of clock frequency input to Serial Engine clock.
>
> Kernel-doc normally doesn't have a full-stop on member descriptions.
Ok. I will remove the full-stop.
>
>> + */
>> +struct geni_wrapper {
>> + struct device *dev;
>> + void __iomem *base;
>> + struct clk_bulk_data ahb_clks[NUM_AHB_CLKS];
>> + struct mutex lock;
>> + unsigned int num_clk_levels;
>> + unsigned long *clk_perf_tbl;
>> +};
>> +
>> +/* Offset of QUP Hardware Version Register */
>
> Useless comment?
I will remove it.
>
>> +#define QUP_HW_VER_REG 0x4
>> +
>> +#define HW_VER_MAJOR_MASK GENMASK(31, 28)
>> +#define HW_VER_MAJOR_SHFT 28
>> +#define HW_VER_MINOR_MASK GENMASK(27, 16)
>> +#define HW_VER_MINOR_SHFT 16
>> +#define HW_VER_STEP_MASK GENMASK(15, 0)
>> +
>> +/* Common SE registers */
>> +#define GENI_INIT_CFG_REVISION 0x0
>> +#define GENI_S_INIT_CFG_REVISION 0x4
>> +#define GENI_OUTPUT_CTRL 0x24
>> +#define GENI_CGC_CTRL 0x28
>> +#define GENI_CLK_CTRL_RO 0x60
>> +#define GENI_IF_DISABLE_RO 0x64
>> +#define GENI_FW_REVISION_RO 0x68
>> +#define GENI_FW_S_REVISION_RO 0x6c
>> +#define SE_GENI_BYTE_GRAN 0x254
>> +#define SE_GENI_TX_PACKING_CFG0 0x260
>> +#define SE_GENI_TX_PACKING_CFG1 0x264
>> +#define SE_GENI_RX_PACKING_CFG0 0x284
>> +#define SE_GENI_RX_PACKING_CFG1 0x288
>> +#define SE_GENI_M_GP_LENGTH 0x910
>> +#define SE_GENI_S_GP_LENGTH 0x914
>> +#define SE_DMA_TX_PTR_L 0xc30
>> +#define SE_DMA_TX_PTR_H 0xc34
>> +#define SE_DMA_TX_ATTR 0xc38
>> +#define SE_DMA_TX_LEN 0xc3c
>> +#define SE_DMA_TX_IRQ_EN 0xc48
>> +#define SE_DMA_TX_IRQ_EN_SET 0xc4c
>> +#define SE_DMA_TX_IRQ_EN_CLR 0xc50
>> +#define SE_DMA_TX_LEN_IN 0xc54
>> +#define SE_DMA_TX_MAX_BURST 0xc5c
>> +#define SE_DMA_RX_PTR_L 0xd30
>> +#define SE_DMA_RX_PTR_H 0xd34
>> +#define SE_DMA_RX_ATTR 0xd38
>> +#define SE_DMA_RX_LEN 0xd3c
>> +#define SE_DMA_RX_IRQ_EN 0xd48
>> +#define SE_DMA_RX_IRQ_EN_SET 0xd4c
>> +#define SE_DMA_RX_IRQ_EN_CLR 0xd50
>> +#define SE_DMA_RX_LEN_IN 0xd54
>> +#define SE_DMA_RX_MAX_BURST 0xd5c
>> +#define SE_DMA_RX_FLUSH 0xd60
>> +#define SE_GSI_EVENT_EN 0xe18
>> +#define SE_IRQ_EN 0xe1c
>> +#define SE_HW_PARAM_0 0xe24
>> +#define SE_HW_PARAM_1 0xe28
>> +#define SE_DMA_GENERAL_CFG 0xe30
>> +
>> +/* GENI_OUTPUT_CTRL fields */
>> +#define DEFAULT_IO_OUTPUT_CTRL_MSK GENMASK(6, 0)
>> +
>> +/* GENI_CGC_CTRL fields */
>> +#define CFG_AHB_CLK_CGC_ON BIT(0)
>> +#define CFG_AHB_WR_ACLK_CGC_ON BIT(1)
>> +#define DATA_AHB_CLK_CGC_ON BIT(2)
>> +#define SCLK_CGC_ON BIT(3)
>> +#define TX_CLK_CGC_ON BIT(4)
>> +#define RX_CLK_CGC_ON BIT(5)
>> +#define EXT_CLK_CGC_ON BIT(6)
>> +#define PROG_RAM_HCLK_OFF BIT(8)
>> +#define PROG_RAM_SCLK_OFF BIT(9)
>> +#define DEFAULT_CGC_EN GENMASK(6, 0)
>> +
>> +/* FW_REVISION_RO fields */
>> +#define FW_REV_PROTOCOL_MSK GENMASK(15, 8)
>> +#define FW_REV_PROTOCOL_SHFT 8
>> +
>> +/* SE_GSI_EVENT_EN fields */
>> +#define DMA_RX_EVENT_EN BIT(0)
>> +#define DMA_TX_EVENT_EN BIT(1)
>> +#define GENI_M_EVENT_EN BIT(2)
>> +#define GENI_S_EVENT_EN BIT(3)
>> +
>> +/* SE_IRQ_EN fields */
>> +#define DMA_RX_IRQ_EN BIT(0)
>> +#define DMA_TX_IRQ_EN BIT(1)
>> +#define GENI_M_IRQ_EN BIT(2)
>> +#define GENI_S_IRQ_EN BIT(3)
>> +
>> +/* SE_HW_PARAM_0 fields */
>> +#define TX_FIFO_WIDTH_MSK GENMASK(29, 24)
>> +#define TX_FIFO_WIDTH_SHFT 24
>> +#define TX_FIFO_DEPTH_MSK GENMASK(21, 16)
>> +#define TX_FIFO_DEPTH_SHFT 16
>> +
>> +/* SE_HW_PARAM_1 fields */
>> +#define RX_FIFO_WIDTH_MSK GENMASK(29, 24)
>> +#define RX_FIFO_WIDTH_SHFT 24
>> +#define RX_FIFO_DEPTH_MSK GENMASK(21, 16)
>> +#define RX_FIFO_DEPTH_SHFT 16
>> +
>> +/* SE_DMA_GENERAL_CFG */
>> +#define DMA_RX_CLK_CGC_ON BIT(0)
>> +#define DMA_TX_CLK_CGC_ON BIT(1)
>> +#define DMA_AHB_SLV_CFG_ON BIT(2)
>> +#define AHB_SEC_SLV_CLK_CGC_ON BIT(3)
>> +#define DUMMY_RX_NON_BUFFERABLE BIT(4)
>> +#define RX_DMA_ZERO_PADDING_EN BIT(5)
>> +#define RX_DMA_IRQ_DELAY_MSK GENMASK(8, 6)
>> +#define RX_DMA_IRQ_DELAY_SHFT 6
>> +
>> +/**
>> + * geni_se_get_qup_hw_version() - Read the QUP wrapper Hardware version
>> + * @se: Pointer to the corresponding Serial Engine.
>> + * @major: Buffer for Major Version field.
>> + * @minor: Buffer for Minor Version field.
>> + * @step: Buffer for Step Version field.
>> + */
>> +void geni_se_get_qup_hw_version(struct geni_se *se, unsigned int *major,
>> + unsigned int *minor, unsigned int *step)
>> +{
>> + unsigned int version;
>> + struct geni_wrapper *wrapper = se->wrapper;
>> +
>> + version = readl_relaxed(wrapper->base + QUP_HW_VER_REG);
>> + *major = (version & HW_VER_MAJOR_MASK) >> HW_VER_MAJOR_SHFT;
>> + *minor = (version & HW_VER_MINOR_MASK) >> HW_VER_MINOR_SHFT;
>> + *step = version & HW_VER_STEP_MASK;
>> +}
>> +EXPORT_SYMBOL(geni_se_get_qup_hw_version);
>
> Is this used?
SPI controller driver uses this API and it will be uploaded sooner.
>
>> +
>> +/**
>> + * geni_se_read_proto() - Read the protocol configured for a Serial Engine
>> + * @se: Pointer to the concerned Serial Engine.
>> + *
>> + * Return: Protocol value as configured in the serial engine.
>> + */
>> +u32 geni_se_read_proto(struct geni_se *se)
>> +{
>> + u32 val;
>> +
>> + val = readl_relaxed(se->base + GENI_FW_REVISION_RO);
>> +
>> + return (val & FW_REV_PROTOCOL_MSK) >> FW_REV_PROTOCOL_SHFT;
>> +}
>> +EXPORT_SYMBOL(geni_se_read_proto);
>
> Is this API really needed outside of this file? It would seem like the
> drivers that implement the protocol, which are child devices, would only
> use this API to confirm that the protocol chosen is for their particular
> protocol.
No, this API is meant for the protocol drivers to confirm that the
serial engine is programmed with the firmware for the concerned protocol
before using the serial engine. If the check fails, the protocol drivers
stop using the serial engine.
>
>
>> +
>> +static void geni_se_io_set_mode(void __iomem *base)
>> +{
>> + u32 val;
>> +
>> + val = readl_relaxed(base + SE_IRQ_EN);
>> + val |= (GENI_M_IRQ_EN | GENI_S_IRQ_EN);
>> + val |= (DMA_TX_IRQ_EN | DMA_RX_IRQ_EN);
>
> Drop useless parenthesis please.
Ok.
>
>> + writel_relaxed(val, base + SE_IRQ_EN);
>> +
>> + val = readl_relaxed(base + SE_GENI_DMA_MODE_EN);
>> + val &= ~GENI_DMA_MODE_EN;
>> + writel_relaxed(val, base + SE_GENI_DMA_MODE_EN);
>> +
>> + writel_relaxed(0, base + SE_GSI_EVENT_EN);
>> +}
>> +
>> +static void geni_se_io_init(void __iomem *base)
>> +{
>> + u32 val;
>> +
>> + val = readl_relaxed(base + GENI_CGC_CTRL);
>> + val |= DEFAULT_CGC_EN;
>> + writel_relaxed(val, base + GENI_CGC_CTRL);
>> +
>> + val = readl_relaxed(base + SE_DMA_GENERAL_CFG);
>> + val |= AHB_SEC_SLV_CLK_CGC_ON | DMA_AHB_SLV_CFG_ON;
>> + val |= DMA_TX_CLK_CGC_ON | DMA_RX_CLK_CGC_ON;
>> + writel_relaxed(val, base + SE_DMA_GENERAL_CFG);
>> +
>> + writel_relaxed(DEFAULT_IO_OUTPUT_CTRL_MSK, base + GENI_OUTPUT_CTRL);
>> + writel_relaxed(FORCE_DEFAULT, base + GENI_FORCE_DEFAULT_REG);
>> +}
>> +
>> +/**
>> + * geni_se_init() - Initialize the GENI Serial Engine
>> + * @se: Pointer to the concerned Serial Engine.
>> + * @rx_wm: Receive watermark, in units of FIFO words.
>> + * @rx_rfr_wm: Ready-for-receive watermark, in units of FIFO words.
>> + *
>> + * This function is used to initialize the GENI serial engine, configure
>> + * receive watermark and ready-for-receive watermarks.
>> + *
>> + * Return: 0 on success, standard Linux error codes on failure/error.
>
> It never returns an error. Change to void?
Ok.
>
>> + */
>> +int geni_se_init(struct geni_se *se, u32 rx_wm, u32 rx_rfr)
>> +{
>> + u32 val;
>> +
>> + geni_se_io_init(se->base);
>> + geni_se_io_set_mode(se->base);
>> +
>> + writel_relaxed(rx_wm, se->base + SE_GENI_RX_WATERMARK_REG);
>> + writel_relaxed(rx_rfr, se->base + SE_GENI_RX_RFR_WATERMARK_REG);
>> +
>> + val = readl_relaxed(se->base + SE_GENI_M_IRQ_EN);
>> + val |= M_COMMON_GENI_M_IRQ_EN;
>> + writel_relaxed(val, se->base + SE_GENI_M_IRQ_EN);
>> +
>> + val = readl_relaxed(se->base + SE_GENI_S_IRQ_EN);
>> + val |= S_COMMON_GENI_S_IRQ_EN;
>> + writel_relaxed(val, se->base + SE_GENI_S_IRQ_EN);
>> + return 0;
>> +}
>> +EXPORT_SYMBOL(geni_se_init);
>> +
>> +static void geni_se_select_fifo_mode(struct geni_se *se)
>> +{
>> + u32 proto = geni_se_read_proto(se);
>> + u32 val;
>> +
>> + writel_relaxed(0, se->base + SE_GSI_EVENT_EN);
>> + writel_relaxed(0xffffffff, se->base + SE_GENI_M_IRQ_CLEAR);
>> + writel_relaxed(0xffffffff, se->base + SE_GENI_S_IRQ_CLEAR);
>> + writel_relaxed(0xffffffff, se->base + SE_DMA_TX_IRQ_CLR);
>> + writel_relaxed(0xffffffff, se->base + SE_DMA_RX_IRQ_CLR);
>> + writel_relaxed(0xffffffff, se->base + SE_IRQ_EN);
>> +
>> + val = readl_relaxed(se->base + SE_GENI_M_IRQ_EN);
>> + if (proto != GENI_SE_UART) {
>> + val |= M_CMD_DONE_EN | M_TX_FIFO_WATERMARK_EN;
>> + val |= M_RX_FIFO_WATERMARK_EN | M_RX_FIFO_LAST_EN;
>> + }
>> + writel_relaxed(val, se->base + SE_GENI_M_IRQ_EN);
>> +
>> + val = readl_relaxed(se->base + SE_GENI_S_IRQ_EN);
>> + if (proto != GENI_SE_UART)
>> + val |= S_CMD_DONE_EN;
>> + writel_relaxed(val, se->base + SE_GENI_S_IRQ_EN);
>> +
>> + val = readl_relaxed(se->base + SE_GENI_DMA_MODE_EN);
>> + val &= ~GENI_DMA_MODE_EN;
>> + writel_relaxed(val, se->base + SE_GENI_DMA_MODE_EN);
>> +}
>> +
>> +static void geni_se_select_dma_mode(struct geni_se *se)
>> +{
>> + u32 val;
>> +
>> + writel_relaxed(0, se->base + SE_GSI_EVENT_EN);
>> + writel_relaxed(0xffffffff, se->base + SE_GENI_M_IRQ_CLEAR);
>> + writel_relaxed(0xffffffff, se->base + SE_GENI_S_IRQ_CLEAR);
>> + writel_relaxed(0xffffffff, se->base + SE_DMA_TX_IRQ_CLR);
>> + writel_relaxed(0xffffffff, se->base + SE_DMA_RX_IRQ_CLR);
>> + writel_relaxed(0xffffffff, se->base + SE_IRQ_EN);
>> +
>> + val = readl_relaxed(se->base + SE_GENI_DMA_MODE_EN);
>> + val |= GENI_DMA_MODE_EN;
>> + writel_relaxed(val, se->base + SE_GENI_DMA_MODE_EN);
>> +}
>> +
>> +/**
>> + * geni_se_select_mode() - Select the serial engine transfer mode
>> + * @se: Pointer to the concerned Serial Engine.
>> + * @mode: Transfer mode to be selected.
>> + */
>> +void geni_se_select_mode(struct geni_se *se, int mode)
>
> enum mode?
Ok.
>
>> +{
>> + WARN_ON(mode != GENI_SE_FIFO && mode != GENI_SE_DMA);
>> +
>> + switch (mode) {
>> + case GENI_SE_FIFO:
>> + geni_se_select_fifo_mode(se);
>> + break;
>> + case GENI_SE_DMA:
>> + geni_se_select_dma_mode(se);
>> + break;
>> + }
>> +}
>> +EXPORT_SYMBOL(geni_se_select_mode);
>> +
>> +/**
>> + * geni_se_setup_m_cmd() - Setup the primary sequencer
>> + * @se: Pointer to the concerned Serial Engine.
>> + * @cmd: Command/Operation to setup in the primary sequencer.
>> + * @params: Parameter for the sequencer command.
>> + *
>> + * This function is used to configure the primary sequencer with the
>> + * command and its associated parameters.
>> + */
>> +void geni_se_setup_m_cmd(struct geni_se *se, u32 cmd, u32 params)
>> +{
>> + u32 m_cmd;
>> +
>> + m_cmd = (cmd << M_OPCODE_SHFT) | (params & M_PARAMS_MSK);
>> + writel_relaxed(m_cmd, se->base + SE_GENI_M_CMD0);
>> +}
>> +EXPORT_SYMBOL(geni_se_setup_m_cmd);
>> +
>> +/**
>> + * geni_se_setup_s_cmd() - Setup the secondary sequencer
>> + * @se: Pointer to the concerned Serial Engine.
>> + * @cmd: Command/Operation to setup in the secondary sequencer.
>> + * @params: Parameter for the sequencer command.
>> + *
>> + * This function is used to configure the secondary sequencer with the
>> + * command and its associated parameters.
>> + */
>> +void geni_se_setup_s_cmd(struct geni_se *se, u32 cmd, u32 params)
>> +{
>> + u32 s_cmd;
>> +
>> + s_cmd = readl_relaxed(se->base + SE_GENI_S_CMD0);
>> + s_cmd &= ~(S_OPCODE_MSK | S_PARAMS_MSK);
>> + s_cmd |= (cmd << S_OPCODE_SHFT);
>> + s_cmd |= (params & S_PARAMS_MSK);
>> + writel_relaxed(s_cmd, se->base + SE_GENI_S_CMD0);
>> +}
>> +EXPORT_SYMBOL(geni_se_setup_s_cmd);
>> +
>> +/**
>> + * geni_se_cancel_m_cmd() - Cancel the command configured in the primary
>> + * sequencer
>> + * @se: Pointer to the concerned Serial Engine.
>> + *
>> + * This function is used to cancel the currently configured command in the
>> + * primary sequencer.
>> + */
>> +void geni_se_cancel_m_cmd(struct geni_se *se)
>> +{
>> + writel_relaxed(M_GENI_CMD_CANCEL, se->base + SE_GENI_M_CMD_CTRL_REG);
>> +}
>> +EXPORT_SYMBOL(geni_se_cancel_m_cmd);
>> +
>> +/**
>> + * geni_se_cancel_s_cmd() - Cancel the command configured in the secondary
>> + * sequencer
>> + * @se: Pointer to the concerned Serial Engine.
>> + *
>> + * This function is used to cancel the currently configured command in the
>> + * secondary sequencer.
>> + */
>> +void geni_se_cancel_s_cmd(struct geni_se *se)
>> +{
>> + writel_relaxed(S_GENI_CMD_CANCEL, se->base + SE_GENI_S_CMD_CTRL_REG);
>> +}
>> +EXPORT_SYMBOL(geni_se_cancel_s_cmd);
>> +
>> +/**
>> + * geni_se_abort_m_cmd() - Abort the command configured in the primary sequencer
>> + * @se: Pointer to the concerned Serial Engine.
>> + *
>> + * This function is used to force abort the currently configured command in the
>> + * primary sequencer.
>> + */
>> +void geni_se_abort_m_cmd(struct geni_se *se)
>> +{
>> + writel_relaxed(M_GENI_CMD_ABORT, se->base + SE_GENI_M_CMD_CTRL_REG);
>> +}
>> +EXPORT_SYMBOL(geni_se_abort_m_cmd);
>> +
>> +/**
>> + * geni_se_abort_s_cmd() - Abort the command configured in the secondary
>> + * sequencer
>> + * @se: Pointer to the concerned Serial Engine.
>> + *
>> + * This function is used to force abort the currently configured command in the
>> + * secondary sequencer.
>> + */
>> +void geni_se_abort_s_cmd(struct geni_se *se)
>> +{
>> + writel_relaxed(S_GENI_CMD_ABORT, se->base + SE_GENI_S_CMD_CTRL_REG);
>> +}
>> +EXPORT_SYMBOL(geni_se_abort_s_cmd);
>
> Can these one-liners go into the header file and be marked static
> inline? I would guess call-sites already have se->base in hand, so
> registers might be reused more efficiently and it may result in a single
> store instruction instead of a branch and load/store.
Ok.
>
>> +
>> +/**
>> + * geni_se_get_tx_fifo_depth() - Get the TX fifo depth of the serial engine
>> + * @se: Pointer to the concerned Serial Engine.
>> + *
>> + * This function is used to get the depth i.e. number of elements in the
>> + * TX fifo of the serial engine.
>> + *
>> + * Return: TX fifo depth in units of FIFO words.
>> + */
>> +u32 geni_se_get_tx_fifo_depth(struct geni_se *se)
>> +{
>> + u32 val;
>> +
>> + val = readl_relaxed(se->base + SE_HW_PARAM_0);
>> +
>> + return (val & TX_FIFO_DEPTH_MSK) >> TX_FIFO_DEPTH_SHFT;
>> +}
>> +EXPORT_SYMBOL(geni_se_get_tx_fifo_depth);
>> +
>> +/**
>> + * geni_se_get_tx_fifo_width() - Get the TX fifo width of the serial engine
>> + * @se: Pointer to the concerned Serial Engine.
>> + *
>> + * This function is used to get the width i.e. word size per element in the
>> + * TX fifo of the serial engine.
>> + *
>> + * Return: TX fifo width in bits
>> + */
>> +u32 geni_se_get_tx_fifo_width(struct geni_se *se)
>> +{
>> + u32 val;
>> +
>> + val = readl_relaxed(se->base + SE_HW_PARAM_0);
>> +
>> + return (val & TX_FIFO_WIDTH_MSK) >> TX_FIFO_WIDTH_SHFT;
>> +}
>> +EXPORT_SYMBOL(geni_se_get_tx_fifo_width);
>> +
>> +/**
>> + * geni_se_get_rx_fifo_depth() - Get the RX fifo depth of the serial engine
>> + * @se: Pointer to the concerned Serial Engine.
>> + *
>> + * This function is used to get the depth i.e. number of elements in the
>> + * RX fifo of the serial engine.
>> + *
>> + * Return: RX fifo depth in units of FIFO words
>> + */
>> +u32 geni_se_get_rx_fifo_depth(struct geni_se *se)
>> +{
>> + u32 val;
>> +
>> + val = readl_relaxed(se->base + SE_HW_PARAM_1);
>> +
>> + return (val & RX_FIFO_DEPTH_MSK) >> RX_FIFO_DEPTH_SHFT;
>> +}
>> +EXPORT_SYMBOL(geni_se_get_rx_fifo_depth);
>
> These ones too, can probably just be static inline.
Ok. Just for my knowledge - is there any reference guideline regarding
when to use static inline myself and when to let the compiler do the
clever thing?
>
>> +
>> +/**
>> + * DOC: Overview
>> + *
>> + * GENI FIFO packing is highly configurable. TX/RX packing/unpacking consist
>> + * of up to 4 operations, each operation represented by 4 configuration vectors
>> + * of 10 bits programmed in GENI_TX_PACKING_CFG0 and GENI_TX_PACKING_CFG1 for
>> + * TX FIFO and in GENI_RX_PACKING_CFG0 and GENI_RX_PACKING_CFG1 for RX FIFO.
>> + * Refer to below examples for detailed bit-field description.
>> + *
>> + * Example 1: word_size = 7, packing_mode = 4 x 8, msb_to_lsb = 1
>> + *
>> + * +-----------+-------+-------+-------+-------+
>> + * | | vec_0 | vec_1 | vec_2 | vec_3 |
>> + * +-----------+-------+-------+-------+-------+
>> + * | start | 0x6 | 0xe | 0x16 | 0x1e |
>> + * | direction | 1 | 1 | 1 | 1 |
>> + * | length | 6 | 6 | 6 | 6 |
>> + * | stop | 0 | 0 | 0 | 1 |
>> + * +-----------+-------+-------+-------+-------+
>> + *
>> + * Example 2: word_size = 15, packing_mode = 2 x 16, msb_to_lsb = 0
>> + *
>> + * +-----------+-------+-------+-------+-------+
>> + * | | vec_0 | vec_1 | vec_2 | vec_3 |
>> + * +-----------+-------+-------+-------+-------+
>> + * | start | 0x0 | 0x8 | 0x10 | 0x18 |
>> + * | direction | 0 | 0 | 0 | 0 |
>> + * | length | 7 | 6 | 7 | 6 |
>> + * | stop | 0 | 0 | 0 | 1 |
>> + * +-----------+-------+-------+-------+-------+
>> + *
>> + * Example 3: word_size = 23, packing_mode = 1 x 32, msb_to_lsb = 1
>> + *
>> + * +-----------+-------+-------+-------+-------+
>> + * | | vec_0 | vec_1 | vec_2 | vec_3 |
>> + * +-----------+-------+-------+-------+-------+
>> + * | start | 0x16 | 0xe | 0x6 | 0x0 |
>> + * | direction | 1 | 1 | 1 | 1 |
>> + * | length | 7 | 7 | 6 | 0 |
>> + * | stop | 0 | 0 | 1 | 0 |
>> + * +-----------+-------+-------+-------+-------+
>> + *
>> + */
>> +
>> +#define NUM_PACKING_VECTORS 4
>> +#define PACKING_START_SHIFT 5
>> +#define PACKING_DIR_SHIFT 4
>> +#define PACKING_LEN_SHIFT 1
>> +#define PACKING_STOP_BIT BIT(0)
>> +#define PACKING_VECTOR_SHIFT 10
>> +/**
>> + * geni_se_config_packing() - Packing configuration of the serial engine
>> + * @se: Pointer to the concerned Serial Engine
>> + * @bpw: Bits of data per transfer word.
>> + * @pack_words: Number of words per fifo element.
>> + * @msb_to_lsb: Transfer from MSB to LSB or vice-versa.
>> + * @tx_cfg: Flag to configure the TX Packing.
>> + * @rx_cfg: Flag to configure the RX Packing.
>> + *
>> + * This function is used to configure the packing rules for the current
>> + * transfer.
>> + */
>> +void geni_se_config_packing(struct geni_se *se, int bpw, int pack_words,
>> + bool msb_to_lsb, bool tx_cfg, bool rx_cfg)
>> +{
>> + u32 cfg0, cfg1, cfg[NUM_PACKING_VECTORS] = {0};
>> + int len;
>> + int temp_bpw = bpw;
>> + int idx_start = msb_to_lsb ? bpw - 1 : 0;
>> + int idx = idx_start;
>> + int idx_delta = msb_to_lsb ? -BITS_PER_BYTE : BITS_PER_BYTE;
>> + int ceil_bpw = (bpw + (BITS_PER_BYTE - 1)) & ~(BITS_PER_BYTE - 1);
>
> ALIGN(bpw, BITS_PER_BYTE)?
Ok.
>
>> + int iter = (ceil_bpw * pack_words) / BITS_PER_BYTE;
>> + int i;
>> +
>> + if (iter <= 0 || iter > NUM_PACKING_VECTORS)
>> + return;
>> +
>> + for (i = 0; i < iter; i++) {
>> + if (temp_bpw < BITS_PER_BYTE)
>> + len = temp_bpw - 1;
>> + else
>> + len = BITS_PER_BYTE - 1;
>
> len = min(temp_bpw, BITS_PER_BYTE) - 1;
Ok.
>
>> +
>> + cfg[i] = idx << PACKING_START_SHIFT;
>> + cfg[i] |= msb_to_lsb << PACKING_DIR_SHIFT;
>> + cfg[i] |= len << PACKING_LEN_SHIFT;
>> +
>> + if (temp_bpw <= BITS_PER_BYTE) {
>> + idx = ((i + 1) * BITS_PER_BYTE) + idx_start;
>> + temp_bpw = bpw;
>> + } else {
>> + idx = idx + idx_delta;
>> + temp_bpw = temp_bpw - BITS_PER_BYTE;
>> + }
>> + }
>> + cfg[iter - 1] |= PACKING_STOP_BIT;
>> + cfg0 = cfg[0] | (cfg[1] << PACKING_VECTOR_SHIFT);
>> + cfg1 = cfg[2] | (cfg[3] << PACKING_VECTOR_SHIFT);
>> +
>> + if (tx_cfg) {
>> + writel_relaxed(cfg0, se->base + SE_GENI_TX_PACKING_CFG0);
>> + writel_relaxed(cfg1, se->base + SE_GENI_TX_PACKING_CFG1);
>> + }
>> + if (rx_cfg) {
>> + writel_relaxed(cfg0, se->base + SE_GENI_RX_PACKING_CFG0);
>> + writel_relaxed(cfg1, se->base + SE_GENI_RX_PACKING_CFG1);
>> + }
>> +
>> + /*
>> + * Number of protocol words in each FIFO entry
>> + * 0 - 4x8, four words in each entry, max word size of 8 bits
>> + * 1 - 2x16, two words in each entry, max word size of 16 bits
>> + * 2 - 1x32, one word in each entry, max word size of 32 bits
>> + * 3 - undefined
>> + */
>> + if (pack_words || bpw == 32)
>> + writel_relaxed(bpw / 16, se->base + SE_GENI_BYTE_GRAN);
>> +}
>> +EXPORT_SYMBOL(geni_se_config_packing);
>> +
>> +static void geni_se_clks_off(struct geni_se *se)
>> +{
>> + struct geni_wrapper *wrapper = se->wrapper;
>> +
>> + clk_disable_unprepare(se->clk);
>> + clk_bulk_disable_unprepare(ARRAY_SIZE(wrapper->ahb_clks),
>> + wrapper->ahb_clks);
>> +}
>> +
>> +/**
>> + * geni_se_resources_off() - Turn off resources associated with the serial
>> + * engine
>> + * @se: Pointer to the concerned Serial Engine.
>> + *
>> + * Return: 0 on success, standard Linux error codes on failure/error.
>> + */
>> +int geni_se_resources_off(struct geni_se *se)
>> +{
>> + int ret;
>> +
>> + ret = pinctrl_pm_select_sleep_state(se->dev);
>> + if (ret)
>> + return ret;
>> +
>> + geni_se_clks_off(se);
>> + return 0;
>> +}
>> +EXPORT_SYMBOL(geni_se_resources_off);
>> +
>> +static int geni_se_clks_on(struct geni_se *se)
>> +{
>> + int ret;
>> + struct geni_wrapper *wrapper = se->wrapper;
>> +
>> + ret = clk_bulk_prepare_enable(ARRAY_SIZE(wrapper->ahb_clks),
>> + wrapper->ahb_clks);
>> + if (ret)
>> + return ret;
>> +
>> + ret = clk_prepare_enable(se->clk);
>> + if (ret)
>> + clk_bulk_disable_unprepare(ARRAY_SIZE(wrapper->ahb_clks),
>> + wrapper->ahb_clks);
>> + return ret;
>> +}
>> +
>> +/**
>> + * geni_se_resources_on() - Turn on resources associated with the serial
>> + * engine
>> + * @se: Pointer to the concerned Serial Engine.
>> + *
>> + * Return: 0 on success, standard Linux error codes on failure/error.
>> + */
>> +int geni_se_resources_on(struct geni_se *se)
>> +{
>> + int ret = 0;
>
> Don't assign variables and then reassign them on the next line.
Ok.
>
>> +
>> + ret = geni_se_clks_on(se);
>> + if (ret)
>> + return ret;
>> +
>> + ret = pinctrl_pm_select_default_state(se->dev);
>> + if (ret)
>> + geni_se_clks_off(se);
>> +
>> + return ret;
>> +}
>> +EXPORT_SYMBOL(geni_se_resources_on);
>
> IS there a reason why we can't use runtime PM or normal linux PM
> infrastructure to power on the wrapper and keep it powered while the
> protocol driver is active?
Besides turning on the clocks & pinctrl settings, wrapper also has to do
the bus scaling votes. The bus scaling votes depend on the individual
serial interface bandwidth requirements. The bus scaling votes is not
present currently. But once the support comes in, this function enables
adding it.
>
>> +
>> +/**
>> + * geni_se_clk_tbl_get() - Get the clock table to program DFS
>> + * @se: Pointer to the concerned Serial Engine.
>> + * @tbl: Table in which the output is returned.
>> + *
>> + * This function is called by the protocol drivers to determine the different
>> + * clock frequencies supported by Serial Engine Core Clock. The protocol
>> + * drivers use the output to determine the clock frequency index to be
>> + * programmed into DFS.
>> + *
>> + * Return: number of valid performance levels in the table on success,
>> + * standard Linux error codes on failure.
>> + */
>> +int geni_se_clk_tbl_get(struct geni_se *se, unsigned long **tbl)
>> +{
>> + struct geni_wrapper *wrapper = se->wrapper;
>> + unsigned long freq = 0;
>> + int i;
>> + int ret = 0;
>> +
>> + mutex_lock(&wrapper->lock);
>> + if (wrapper->clk_perf_tbl) {
>> + *tbl = wrapper->clk_perf_tbl;
>> + ret = wrapper->num_clk_levels;
>> + goto out_unlock;
>> + }
>> +
>> + wrapper->clk_perf_tbl = kcalloc(MAX_CLK_PERF_LEVEL,
>> + sizeof(*wrapper->clk_perf_tbl),
>> + GFP_KERNEL);
>> + if (!wrapper->clk_perf_tbl) {
>> + ret = -ENOMEM;
>> + goto out_unlock;
>> + }
>> +
>> + for (i = 0; i < MAX_CLK_PERF_LEVEL; i++) {
>> + freq = clk_round_rate(se->clk, freq + 1);
>> + if (!freq || freq == wrapper->clk_perf_tbl[i - 1])
>> + break;
>> + wrapper->clk_perf_tbl[i] = freq;
>> + }
>> + wrapper->num_clk_levels = i;
>> + *tbl = wrapper->clk_perf_tbl;
>> + ret = wrapper->num_clk_levels;
>> +out_unlock:
>> + mutex_unlock(&wrapper->lock);
>
> Is this lock actually protecting anything? I mean to say, is any more
> than one geni protocol driver calling this function at a time? Or is
> the same geni protocol driver calling this from multiple threads at the
> same time? The lock looks almost useless.
Yes, there is a possibility of multiple I2C instances within the same
wrapper trying to get this table simultaneously.
As Evan mentioned in the other thread, Bjorn had the comment to move it
to the probe and remove the lock. I looked into the possibility of it.
From the hardware perspective, this table belongs to the wrapper and is
shared by all the serial engines within the wrapper. But due to software
implementation reasons, clk_round_rate can be be performed only on the
clocks that are tagged as DFS compatible and only the serial engine
clocks are tagged so. At least this was the understanding based on our
earlier discussion with the concerned folks. We will revisit it and
check if anything has changed recently.
>
>> + return ret;
>> +}
>> +EXPORT_SYMBOL(geni_se_clk_tbl_get);
>> +
>> +/**
>> + * geni_se_clk_freq_match() - Get the matching or closest SE clock frequency
>> + * @se: Pointer to the concerned Serial Engine.
>> + * @req_freq: Requested clock frequency.
>> + * @index: Index of the resultant frequency in the table.
>> + * @res_freq: Resultant frequency which matches or is closer to the
>> + * requested frequency.
>> + * @exact: Flag to indicate exact multiple requirement of the requested
>> + * frequency.
>> + *
>> + * This function is called by the protocol drivers to determine the matching
>> + * or exact multiple of the requested frequency, as provided by the Serial
>> + * Engine clock in order to meet the performance requirements. If there is
>> + * no matching or exact multiple of the requested frequency found, then it
>> + * selects the closest floor frequency, if exact flag is not set.
>> + *
>> + * Return: 0 on success, standard Linux error codes on failure.
>> + */
>> +int geni_se_clk_freq_match(struct geni_se *se, unsigned long req_freq,
>> + unsigned int *index, unsigned long *res_freq,
>> + bool exact)
>> +{
>> + unsigned long *tbl;
>> + int num_clk_levels;
>> + int i;
>> +
>> + num_clk_levels = geni_se_clk_tbl_get(se, &tbl);
>> + if (num_clk_levels < 0)
>> + return num_clk_levels;
>> +
>> + if (num_clk_levels == 0)
>> + return -EFAULT;
>
> I believe this would mean userspace thought the syscall faulted.
> Perhaps -EINVAL instead?
Ok.
>
>> +
>> + *res_freq = 0;
>> + for (i = 0; i < num_clk_levels; i++) {
>> + if (!(tbl[i] % req_freq)) {
>> + *index = i;
>> + *res_freq = tbl[i];
>> + return 0;
>> + }
>> +
>> + if (!(*res_freq) || ((tbl[i] > *res_freq) &&
>> + (tbl[i] < req_freq))) {
>> + *index = i;
>> + *res_freq = tbl[i];
>> + }
>> + }
>> +
>> + if (exact)
>> + return -ENOKEY;
>
> Interesting error code. Doubtful this is correct because it seems to be
> related to crypto keys.
Ok.
>
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL(geni_se_clk_freq_match);
>> +
>> +#define GENI_SE_DMA_DONE_EN BIT(0)
>> +#define GENI_SE_DMA_EOT_EN BIT(1)
>> +#define GENI_SE_DMA_AHB_ERR_EN BIT(2)
>> +#define GENI_SE_DMA_EOT_BUF BIT(0)
>> +/**
>> + * geni_se_tx_dma_prep() - Prepare the Serial Engine for TX DMA transfer
>> + * @se: Pointer to the concerned Serial Engine.
>> + * @buf: Pointer to the TX buffer.
>> + * @len: Length of the TX buffer.
>> + *
>> + * This function is used to prepare the buffers for DMA TX.
>> + *
>> + * Return: Mapped DMA Address of the buffer on success, NULL on failure.
>> + */
>> +dma_addr_t geni_se_tx_dma_prep(struct geni_se *se, void *buf, size_t len)
>> +{
>> + dma_addr_t iova;
>> + struct geni_wrapper *wrapper = se->wrapper;
>> + u32 val;
>> +
>> + iova = dma_map_single(wrapper->dev, buf, len, DMA_TO_DEVICE);
>> + if (dma_mapping_error(wrapper->dev, iova))
>> + return (dma_addr_t)NULL;
>> +
>> + val = GENI_SE_DMA_DONE_EN;
>> + val |= GENI_SE_DMA_EOT_EN;
>> + val |= GENI_SE_DMA_AHB_ERR_EN;
>> + writel_relaxed(val, se->base + SE_DMA_TX_IRQ_EN_SET);
>> + writel_relaxed((u32)iova, se->base + SE_DMA_TX_PTR_L);
>
> lower_32_bits()
Ok.
>
>> + writel_relaxed((u32)(iova >> 32), se->base + SE_DMA_TX_PTR_H);
>
> upper_32_bits()
Ok.
>
>> + writel_relaxed(GENI_SE_DMA_EOT_BUF, se->base + SE_DMA_TX_ATTR);
>> + writel_relaxed((u32)len, se->base + SE_DMA_TX_LEN);
>
> Useless cast.
I will remove the casting.
>
>> + return iova;
>> +}
>> +EXPORT_SYMBOL(geni_se_tx_dma_prep);
>> +
>> +/**
>> + * geni_se_rx_dma_prep() - Prepare the Serial Engine for RX DMA transfer
>> + * @se: Pointer to the concerned Serial Engine.
>> + * @buf: Pointer to the RX buffer.
>> + * @len: Length of the RX buffer.
>> + *
>> + * This function is used to prepare the buffers for DMA RX.
>> + *
>> + * Return: Mapped DMA Address of the buffer on success, NULL on failure.
>> + */
>> +dma_addr_t geni_se_rx_dma_prep(struct geni_se *se, void *buf, size_t len)
>> +{
>> + dma_addr_t iova;
>> + struct geni_wrapper *wrapper = se->wrapper;
>> + u32 val;
>> +
>> + iova = dma_map_single(wrapper->dev, buf, len, DMA_FROM_DEVICE);
>> + if (dma_mapping_error(wrapper->dev, iova))
>> + return (dma_addr_t)NULL;
>
> Can't return a dma_mapping_error address to the caller and have them
> figure it out?
Earlier we used to return the DMA_ERROR_CODE which has been removed
recently in arm64 architecture. If we return the dma_mapping_error, then
the caller also needs the device which encountered the mapping error.
The serial interface drivers can use their parent currently to resolve
the mapping error. Once the wrapper starts mapping using IOMMU context
bank, then the serial interface drivers do not know which device to use
to know if there is an error.
Having said that, the dma_ops suggestion might help with handling this
situation. I will look into it further.
>
>> +
>> + val = GENI_SE_DMA_DONE_EN;
>> + val |= GENI_SE_DMA_EOT_EN;
>> + val |= GENI_SE_DMA_AHB_ERR_EN;
>> + writel_relaxed(val, se->base + SE_DMA_RX_IRQ_EN_SET);
>> + writel_relaxed((u32)iova, se->base + SE_DMA_RX_PTR_L);
>> + writel_relaxed((u32)(iova >> 32), se->base + SE_DMA_RX_PTR_H);
>
> upper/lower macros again.
Ok.
>
>> + /* RX does not have EOT buffer type bit. So just reset RX_ATTR */
>> + writel_relaxed(0, se->base + SE_DMA_RX_ATTR);
>> + writel_relaxed((u32)len, se->base + SE_DMA_RX_LEN);
>
> Drop cast?
Ok.
>
>> + return iova;
>> +}
>> +EXPORT_SYMBOL(geni_se_rx_dma_prep);
>> +
>> +/**
>> + * geni_se_tx_dma_unprep() - Unprepare the Serial Engine after TX DMA transfer
>> + * @se: Pointer to the concerned Serial Engine.
>> + * @iova: DMA address of the TX buffer.
>> + * @len: Length of the TX buffer.
>> + *
>> + * This function is used to unprepare the DMA buffers after DMA TX.
>> + */
>> +void geni_se_tx_dma_unprep(struct geni_se *se, dma_addr_t iova, size_t len)
>> +{
>> + struct geni_wrapper *wrapper = se->wrapper;
>> +
>> + if (iova)
>> + dma_unmap_single(wrapper->dev, iova, len, DMA_TO_DEVICE);
>> +}
>> +EXPORT_SYMBOL(geni_se_tx_dma_unprep);
>> +
>> +/**
>> + * geni_se_rx_dma_unprep() - Unprepare the Serial Engine after RX DMA transfer
>> + * @se: Pointer to the concerned Serial Engine.
>> + * @iova: DMA address of the RX buffer.
>> + * @len: Length of the RX buffer.
>> + *
>> + * This function is used to unprepare the DMA buffers after DMA RX.
>> + */
>> +void geni_se_rx_dma_unprep(struct geni_se *se, dma_addr_t iova, size_t len)
>> +{
>> + struct geni_wrapper *wrapper = se->wrapper;
>> +
>> + if (iova)
>> + dma_unmap_single(wrapper->dev, iova, len, DMA_FROM_DEVICE);
>> +}
>> +EXPORT_SYMBOL(geni_se_rx_dma_unprep);
>
> Instead of having the functions exported, could we set the dma_ops on
> all child devices of the wrapper that this driver populates and then
> implement the DMA ops for those devices here? I assume that there's
> never another DMA master between the wrapper and the serial engine, so I
> think it would work.
This suggestion looks like it will work.
>
>> +
>> +static int geni_se_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct resource *res;
>> + struct geni_wrapper *wrapper;
>> + int ret;
>> +
>> + wrapper = devm_kzalloc(dev, sizeof(*wrapper), GFP_KERNEL);
>> + if (!wrapper)
>> + return -ENOMEM;
>> +
>> + wrapper->dev = dev;
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + wrapper->base = devm_ioremap_resource(dev, res);
>> + if (IS_ERR(wrapper->base)) {
>> + dev_err(dev, "%s: Error mapping the resource\n", __func__);
>
> Drop error message, devm_ioremap_resource() already does it.
Ok.
>
>> + return -EFAULT;
>
> return PTR_ERR(wrapper->base);
>
>> + }
>> +
>> + wrapper->ahb_clks[0].id = m_ahb_clk;
>> + wrapper->ahb_clks[1].id = s_ahb_clk;
>> + ret = devm_clk_bulk_get(dev, NUM_AHB_CLKS, wrapper->ahb_clks);
>> + if (ret) {
>> + dev_err(dev, "Err getting AHB clks %d\n", ret);
>> + return ret;
>> + }
>> +
>> + mutex_init(&wrapper->lock);
>> + dev_set_drvdata(dev, wrapper);
>> + dev_dbg(dev, "GENI SE Driver probed\n");
>> + return devm_of_platform_populate(dev);
>> +}
>> +
>> +static int geni_se_remove(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct geni_wrapper *wrapper = dev_get_drvdata(dev);
>> +
>> + kfree(wrapper->clk_perf_tbl);
>
> Why not devm_kzalloc() this?
I will check it.
>
>> + return 0;
>> +}
>> +
>> +static const struct of_device_id geni_se_dt_match[] = {
>> + { .compatible = "qcom,geni-se-qup", },
>> + {}
>> +};
>> +MODULE_DEVICE_TABLE(of, geni_se_dt_match);
>> +
>> +static struct platform_driver geni_se_driver = {
>> + .driver = {
>> + .name = "geni_se_qup",
>> + .of_match_table = geni_se_dt_match,
>> + },
>> + .probe = geni_se_probe,
>> + .remove = geni_se_remove,
>> +};
>> +module_platform_driver(geni_se_driver);
>> +
>> +MODULE_DESCRIPTION("GENI Serial Engine Driver");
>> +MODULE_LICENSE("GPL v2");
>> diff --git a/include/linux/qcom-geni-se.h b/include/linux/qcom-geni-se.h
>> new file mode 100644
>> index 0000000..4996de7
>> --- /dev/null
>> +++ b/include/linux/qcom-geni-se.h
>> @@ -0,0 +1,247 @@
>> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
>> +/*
>> + * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
>> + */
>> +
>> +#ifndef _LINUX_QCOM_GENI_SE
>> +#define _LINUX_QCOM_GENI_SE
>> +#include <linux/clk.h>
>
> Please forward declare struct clk and drop this include here.
Ok.
>
>> +#include <linux/dma-direction.h>
>
> Drop?
There was a map function which got dropped in v3 patch series. I will
drop including this header file.
>
>> +
>> +/* Transfer mode supported by GENI Serial Engines */
>> +enum geni_se_xfer_mode {
>> + GENI_SE_INVALID,
>> + GENI_SE_FIFO,
>> + GENI_SE_DMA,
>> +};
>> +
>> +/* Protocols supported by GENI Serial Engines */
>> +enum geni_se_protocol_types {
>> + GENI_SE_NONE,
>> + GENI_SE_SPI,
>> + GENI_SE_UART,
>> + GENI_SE_I2C,
>> + GENI_SE_I3C,
>> +};
>> +
>> +/**
>> + * struct geni_se - GENI Serial Engine
>> + * @base: Base Address of the Serial Engine's register block.
>> + * @dev: Pointer to the Serial Engine device.
>> + * @wrapper: Pointer to the parent QUP Wrapper core.
>> + * @clk: Handle to the core serial engine clock.
>> + */
>> +struct geni_se {
>> + void __iomem *base;
>> + struct device *dev;
>> + void *wrapper;
>
> Can this get the geni_wrapper type? It could be opaque if you like.
I am not sure if it is ok to have the children know the details of the
parent. That is why it is kept as opaque.
>
>> + struct clk *clk;
>> +};
>> +
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-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH v10 0/5] set VSESR_EL2 by user space and support NOTIFY_SEI notification
From: Dongjiu Geng @ 2018-03-03 16:09 UTC (permalink / raw)
To: rkrcmar, corbet, christoffer.dall, marc.zyngier, james.morse,
linux, catalin.marinas, rjw, bp, lenb, kvm, linux-doc,
linux-kernel, linux-arm-kernel, kvmarm, linux-acpi, devel
Cc: gengdongjiu, huangshaoyu
1. Detect whether KVM can set set guest SError syndrome
2. Support to Set VSESR_EL2 and inject SError by user space.
3. Support live migration to keep SError pending state and VSESR_EL2 value.
4. ACPI 6.1 adds support for NOTIFY_SEI as a GHES notification mechanism, so support this
notification in software, KVM or kernel ARCH code call handle_guest_sei() to let ACP driver
to handle this notification.
Dongjiu Geng (5):
arm64: KVM: Prepare set virtual SEI syndrome value
arm64: KVM: export the capability to set guest SError syndrome
arm/arm64: KVM: Introduce set and get per-vcpu event
ACPI / APEI: Add SEI notification type support for ARMv8
arm64: handle NOTIFY_SEI notification by the APEI driver
Documentation/virtual/kvm/api.txt | 37 +++++++++++++++++++++++--
arch/arm/include/asm/kvm_host.h | 6 ++++
arch/arm/kvm/guest.c | 12 ++++++++
arch/arm64/include/asm/kvm_emulate.h | 5 ++++
arch/arm64/include/asm/kvm_host.h | 7 +++++
arch/arm64/include/asm/system_misc.h | 1 +
arch/arm64/include/uapi/asm/kvm.h | 10 +++++++
arch/arm64/kernel/traps.c | 4 +++
arch/arm64/kvm/guest.c | 26 ++++++++++++++++++
arch/arm64/kvm/inject_fault.c | 5 ++++
arch/arm64/kvm/reset.c | 4 +++
arch/arm64/mm/fault.c | 10 +++++++
drivers/acpi/apei/Kconfig | 15 ++++++++++
drivers/acpi/apei/ghes.c | 53 ++++++++++++++++++++++++++++++++++++
include/acpi/ghes.h | 1 +
include/uapi/linux/kvm.h | 1 +
virt/kvm/arm/arm.c | 18 ++++++++++++
17 files changed, 213 insertions(+), 2 deletions(-)
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH v10 3/5] arm/arm64: KVM: Introduce set and get per-vcpu event
From: Dongjiu Geng @ 2018-03-03 16:09 UTC (permalink / raw)
To: rkrcmar, corbet, christoffer.dall, marc.zyngier, james.morse,
linux, catalin.marinas, rjw, bp, lenb, kvm, linux-doc,
linux-kernel, linux-arm-kernel, kvmarm, linux-acpi, devel
Cc: gengdongjiu, huangshaoyu
In-Reply-To: <1520093380-42577-1-git-send-email-gengdongjiu@huawei.com>
RAS Extension provides VSESR_EL2 register to specify
virtual SError syndrome value, this patch adds a new
IOCTL to export user-invisible states related to
SError exceptions. User space can setup the
kvm_vcpu_events to inject specified SError, also it
can support live migration.
Signed-off-by: Dongjiu Geng <gengdongjiu@huawei.com>
---
Documentation/virtual/kvm/api.txt | 26 ++++++++++++++++++++++++--
arch/arm/include/asm/kvm_host.h | 6 ++++++
arch/arm/kvm/guest.c | 12 ++++++++++++
arch/arm64/include/asm/kvm_host.h | 5 +++++
arch/arm64/include/uapi/asm/kvm.h | 10 ++++++++++
arch/arm64/kvm/guest.c | 26 ++++++++++++++++++++++++++
arch/arm64/kvm/reset.c | 1 +
virt/kvm/arm/arm.c | 18 ++++++++++++++++++
8 files changed, 102 insertions(+), 2 deletions(-)
diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index 8a3d708..26ae151 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -819,11 +819,13 @@ struct kvm_clock_data {
Capability: KVM_CAP_VCPU_EVENTS
Extended by: KVM_CAP_INTR_SHADOW
-Architectures: x86
+Architectures: x86, arm, arm64
Type: vm ioctl
Parameters: struct kvm_vcpu_event (out)
Returns: 0 on success, -1 on error
+X86:
+
Gets currently pending exceptions, interrupts, and NMIs as well as related
states of the vcpu.
@@ -865,15 +867,29 @@ Only two fields are defined in the flags field:
- KVM_VCPUEVENT_VALID_SMM may be set in the flags field to signal that
smi contains a valid state.
+ARM, ARM64:
+
+Gets currently pending SError exceptions as well as related states of the vcpu.
+
+struct kvm_vcpu_events {
+ struct {
+ bool serror_pending;
+ bool serror_has_esr;
+ u64 serror_esr;
+ } exception;
+};
+
4.32 KVM_SET_VCPU_EVENTS
Capability: KVM_CAP_VCPU_EVENTS
Extended by: KVM_CAP_INTR_SHADOW
-Architectures: x86
+Architectures: x86, arm, arm64
Type: vm ioctl
Parameters: struct kvm_vcpu_event (in)
Returns: 0 on success, -1 on error
+X86:
+
Set pending exceptions, interrupts, and NMIs as well as related states of the
vcpu.
@@ -894,6 +910,12 @@ shall be written into the VCPU.
KVM_VCPUEVENT_VALID_SMM can only be set if KVM_CAP_X86_SMM is available.
+ARM, ARM64:
+
+Set pending SError exceptions as well as related states of the vcpu.
+
+See KVM_GET_VCPU_EVENTS for the data structure.
+
4.33 KVM_GET_DEBUGREGS
diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index ef54013..d81621e 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -211,6 +211,12 @@ struct kvm_vcpu_stat {
int kvm_arm_copy_reg_indices(struct kvm_vcpu *vcpu, u64 __user *indices);
int kvm_arm_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg);
int kvm_arm_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg);
+int kvm_arm_vcpu_get_events(struct kvm_vcpu *vcpu,
+ struct kvm_vcpu_events *events);
+
+int kvm_arm_vcpu_set_events(struct kvm_vcpu *vcpu,
+ struct kvm_vcpu_events *events);
+
unsigned long kvm_call_hyp(void *hypfn, ...);
void force_vm_exit(const cpumask_t *mask);
diff --git a/arch/arm/kvm/guest.c b/arch/arm/kvm/guest.c
index 1e0784e..39f895d 100644
--- a/arch/arm/kvm/guest.c
+++ b/arch/arm/kvm/guest.c
@@ -248,6 +248,18 @@ int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu,
return -EINVAL;
}
+int kvm_arm_vcpu_get_events(struct kvm_vcpu *vcpu,
+ struct kvm_vcpu_events *events)
+{
+ return -EINVAL;
+}
+
+int kvm_arm_vcpu_set_events(struct kvm_vcpu *vcpu,
+ struct kvm_vcpu_events *events)
+{
+ return -EINVAL;
+}
+
int __attribute_const__ kvm_target_cpu(void)
{
switch (read_cpuid_part()) {
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 3dc49b7..1125540 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -326,6 +326,11 @@ struct kvm_vcpu_stat {
int kvm_arm_copy_reg_indices(struct kvm_vcpu *vcpu, u64 __user *indices);
int kvm_arm_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg);
int kvm_arm_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg);
+int kvm_arm_vcpu_get_events(struct kvm_vcpu *vcpu,
+ struct kvm_vcpu_events *events);
+
+int kvm_arm_vcpu_set_events(struct kvm_vcpu *vcpu,
+ struct kvm_vcpu_events *events);
#define KVM_ARCH_WANT_MMU_NOTIFIER
int kvm_unmap_hva(struct kvm *kvm, unsigned long hva);
diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
index 9abbf30..32c0eae 100644
--- a/arch/arm64/include/uapi/asm/kvm.h
+++ b/arch/arm64/include/uapi/asm/kvm.h
@@ -39,6 +39,7 @@
#define __KVM_HAVE_GUEST_DEBUG
#define __KVM_HAVE_IRQ_LINE
#define __KVM_HAVE_READONLY_MEM
+#define __KVM_HAVE_VCPU_EVENTS
#define KVM_COALESCED_MMIO_PAGE_OFFSET 1
@@ -153,6 +154,15 @@ struct kvm_sync_regs {
struct kvm_arch_memory_slot {
};
+/* for KVM_GET/SET_VCPU_EVENTS */
+struct kvm_vcpu_events {
+ struct {
+ bool serror_pending;
+ bool serror_has_esr;
+ u64 serror_esr;
+ } exception;
+};
+
/* If you need to interpret the index values, here is the key: */
#define KVM_REG_ARM_COPROC_MASK 0x000000000FFF0000
#define KVM_REG_ARM_COPROC_SHIFT 16
diff --git a/arch/arm64/kvm/guest.c b/arch/arm64/kvm/guest.c
index 5c7f657..62d49c2 100644
--- a/arch/arm64/kvm/guest.c
+++ b/arch/arm64/kvm/guest.c
@@ -277,6 +277,32 @@ int kvm_arch_vcpu_ioctl_set_sregs(struct kvm_vcpu *vcpu,
return -EINVAL;
}
+int kvm_arm_vcpu_get_events(struct kvm_vcpu *vcpu,
+ struct kvm_vcpu_events *events)
+{
+ events->exception.serror_pending = (vcpu_get_hcr(vcpu) & HCR_VSE);
+ events->exception.serror_has_esr =
+ cpus_have_const_cap(ARM64_HAS_RAS_EXTN) &&
+ (!!vcpu_get_vsesr(vcpu));
+ events->exception.serror_esr = vcpu_get_vsesr(vcpu);
+
+ return 0;
+}
+
+int kvm_arm_vcpu_set_events(struct kvm_vcpu *vcpu,
+ struct kvm_vcpu_events *events)
+{
+ bool injected = events->exception.serror_pending;
+ bool has_esr = events->exception.serror_has_esr;
+
+ if (injected && has_esr)
+ kvm_set_sei_esr(vcpu, events->exception.serror_esr);
+ else if (injected)
+ kvm_inject_vabt(vcpu);
+
+ return 0;
+}
+
int __attribute_const__ kvm_target_cpu(void)
{
unsigned long implementor = read_cpuid_implementor();
diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
index 38c8a64..20e919a 100644
--- a/arch/arm64/kvm/reset.c
+++ b/arch/arm64/kvm/reset.c
@@ -82,6 +82,7 @@ int kvm_arch_dev_ioctl_check_extension(struct kvm *kvm, long ext)
break;
case KVM_CAP_SET_GUEST_DEBUG:
case KVM_CAP_VCPU_ATTRIBUTES:
+ case KVM_CAP_VCPU_EVENTS:
r = 1;
break;
default:
diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
index 7e3941f..30c56e0 100644
--- a/virt/kvm/arm/arm.c
+++ b/virt/kvm/arm/arm.c
@@ -1051,6 +1051,24 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
return -EFAULT;
return kvm_arm_vcpu_has_attr(vcpu, &attr);
}
+ case KVM_GET_VCPU_EVENTS: {
+ struct kvm_vcpu_events events;
+
+ kvm_arm_vcpu_get_events(vcpu, &events);
+
+ if (copy_to_user(argp, &events, sizeof(struct kvm_vcpu_events)))
+ return -EFAULT;
+
+ return 0;
+ }
+ case KVM_SET_VCPU_EVENTS: {
+ struct kvm_vcpu_events events;
+
+ if (copy_from_user(&events, argp, sizeof(struct kvm_vcpu_events)))
+ return -EFAULT;
+
+ return kvm_arm_vcpu_set_events(vcpu, &events);
+ }
default:
return -EINVAL;
}
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* [PATCH v10 1/5] arm64: KVM: Prepare set virtual SEI syndrome value
From: Dongjiu Geng @ 2018-03-03 16:09 UTC (permalink / raw)
To: rkrcmar, corbet, christoffer.dall, marc.zyngier, james.morse,
linux, catalin.marinas, rjw, bp, lenb, kvm, linux-doc,
linux-kernel, linux-arm-kernel, kvmarm, linux-acpi, devel
Cc: gengdongjiu, huangshaoyu
In-Reply-To: <1520093380-42577-1-git-send-email-gengdongjiu@huawei.com>
Export one API to specify virtual SEI syndrome value
for guest, and add a helper to get the VSESR_EL2 value.
Signed-off-by: Dongjiu Geng <gengdongjiu@huawei.com>
---
arch/arm64/include/asm/kvm_emulate.h | 5 +++++
arch/arm64/include/asm/kvm_host.h | 2 ++
arch/arm64/kvm/inject_fault.c | 5 +++++
3 files changed, 12 insertions(+)
diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
index 413dc82..3294885 100644
--- a/arch/arm64/include/asm/kvm_emulate.h
+++ b/arch/arm64/include/asm/kvm_emulate.h
@@ -71,6 +71,11 @@ static inline void vcpu_set_hcr(struct kvm_vcpu *vcpu, unsigned long hcr)
vcpu->arch.hcr_el2 = hcr;
}
+static inline unsigned long vcpu_get_vsesr(struct kvm_vcpu *vcpu)
+{
+ return vcpu->arch.vsesr_el2;
+}
+
static inline void vcpu_set_vsesr(struct kvm_vcpu *vcpu, u64 vsesr)
{
vcpu->arch.vsesr_el2 = vsesr;
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index a73f63a..3dc49b7 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -354,6 +354,8 @@ void handle_exit_early(struct kvm_vcpu *vcpu, struct kvm_run *run,
int kvm_perf_init(void);
int kvm_perf_teardown(void);
+void kvm_set_sei_esr(struct kvm_vcpu *vcpu, u64 syndrome);
+
struct kvm_vcpu *kvm_mpidr_to_vcpu(struct kvm *kvm, unsigned long mpidr);
static inline void __cpu_init_hyp_mode(phys_addr_t pgd_ptr,
diff --git a/arch/arm64/kvm/inject_fault.c b/arch/arm64/kvm/inject_fault.c
index 60666a0..78ecb28 100644
--- a/arch/arm64/kvm/inject_fault.c
+++ b/arch/arm64/kvm/inject_fault.c
@@ -186,3 +186,8 @@ void kvm_inject_vabt(struct kvm_vcpu *vcpu)
{
pend_guest_serror(vcpu, ESR_ELx_ISV);
}
+
+void kvm_set_sei_esr(struct kvm_vcpu *vcpu, u64 syndrome)
+{
+ pend_guest_serror(vcpu, syndrome & ESR_ELx_ISS_MASK);
+}
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox