* Re: [PATCH v2 1/8] [PATCH 1/8] drivers/peci: Add support for PECI bus driver core
From: Julia Cartwright @ 2018-03-07 3:19 UTC (permalink / raw)
To: Jae Hyun Yoo
Cc: joel, andrew, arnd, gregkh, jdelvare, linux, benh, andrew,
linux-kernel, linux-doc, devicetree, linux-hwmon,
linux-arm-kernel, openbmc
In-Reply-To: <20180221161606.32247-2-jae.hyun.yoo@linux.intel.com>
On Wed, Feb 21, 2018 at 08:15:59AM -0800, Jae Hyun Yoo wrote:
> This commit adds driver implementation for PECI bus into linux
> driver framework.
>
> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
> ---
[..]
> +static int peci_locked_xfer(struct peci_adapter *adapter,
> + struct peci_xfer_msg *msg,
> + bool do_retry,
> + bool has_aw_fcs)
_locked generally means that this function is invoked with some critical
lock held, what lock does the caller need to acquire before invoking
this function?
> +{
> + ktime_t start, end;
> + s64 elapsed_ms;
> + int rc = 0;
> +
> + if (!adapter->xfer) {
Is this really an optional feature of an adapter? If this is not
optional, then this check should be in place when the adapter is
registered, not here. (And it should WARN_ON(), because it's a driver
developer error).
> + dev_dbg(&adapter->dev, "PECI level transfers not supported\n");
> + return -ENODEV;
> + }
> +
> + if (in_atomic() || irqs_disabled()) {
As Andrew mentioned, this is broken.
You don't even need a might_sleep(). The locking functions you use here
will already include a might_sleep() w/ CONFIG_DEBUG_ATOMIC_SLEEP.
> + rt_mutex_trylock(&adapter->bus_lock);
> + if (!rc)
> + return -EAGAIN; /* PECI activity is ongoing */
> + } else {
> + rt_mutex_lock(&adapter->bus_lock);
> + }
> +
> + if (do_retry)
> + start = ktime_get();
> +
> + do {
> + rc = adapter->xfer(adapter, msg);
> +
> + if (!do_retry)
> + break;
> +
> + /* Per the PECI spec, need to retry commands that return 0x8x */
> + if (!(!rc && ((msg->rx_buf[0] & DEV_PECI_CC_RETRY_ERR_MASK) ==
> + DEV_PECI_CC_TIMEOUT)))
> + break;
This is pretty difficult to parse. Can you split it into two different
conditions?
> +
> + /* Set the retry bit to indicate a retry attempt */
> + msg->tx_buf[1] |= DEV_PECI_RETRY_BIT;
Are you sure this bit is to be set in the _second_ byte of tx_buf?
> +
> + /* Recalculate the AW FCS if it has one */
> + if (has_aw_fcs)
> + msg->tx_buf[msg->tx_len - 1] = 0x80 ^
> + peci_aw_fcs((u8 *)msg,
> + 2 + msg->tx_len);
> +
> + /* Retry for at least 250ms before returning an error */
> + end = ktime_get();
> + elapsed_ms = ktime_to_ms(ktime_sub(end, start));
> + if (elapsed_ms >= DEV_PECI_RETRY_TIME_MS) {
> + dev_dbg(&adapter->dev, "Timeout retrying xfer!\n");
> + break;
> + }
> + } while (true);
> +
> + rt_mutex_unlock(&adapter->bus_lock);
> +
> + return rc;
> +}
> +
> +static int peci_xfer(struct peci_adapter *adapter, struct peci_xfer_msg *msg)
> +{
> + return peci_locked_xfer(adapter, msg, false, false);
> +}
> +
> +static int peci_xfer_with_retries(struct peci_adapter *adapter,
> + struct peci_xfer_msg *msg,
> + bool has_aw_fcs)
> +{
> + return peci_locked_xfer(adapter, msg, true, has_aw_fcs);
> +}
> +
> +static int peci_scan_cmd_mask(struct peci_adapter *adapter)
> +{
> + struct peci_xfer_msg msg;
> + u32 dib;
> + int rc = 0;
> +
> + /* Update command mask just once */
> + if (adapter->cmd_mask & BIT(PECI_CMD_PING))
> + return 0;
> +
> + msg.addr = PECI_BASE_ADDR;
> + msg.tx_len = GET_DIB_WR_LEN;
> + msg.rx_len = GET_DIB_RD_LEN;
> + msg.tx_buf[0] = GET_DIB_PECI_CMD;
> +
> + rc = peci_xfer(adapter, &msg);
> + if (rc < 0) {
> + dev_dbg(&adapter->dev, "PECI xfer error, rc : %d\n", rc);
> + return rc;
> + }
> +
> + dib = msg.rx_buf[0] | (msg.rx_buf[1] << 8) |
> + (msg.rx_buf[2] << 16) | (msg.rx_buf[3] << 24);
> +
> + /* Check special case for Get DIB command */
> + if (dib == 0x00) {
> + dev_dbg(&adapter->dev, "DIB read as 0x00\n");
> + return -1;
> + }
> +
> + if (!rc) {
You should change this to:
if (rc) {
dev_dbg(&adapter->dev, "Error reading DIB, rc : %d\n", rc);
return rc;
}
And then leave the happy path below unindented.
> + /**
> + * setting up the supporting commands based on minor rev#
> + * see PECI Spec Table 3-1
> + */
> + dib = (dib >> 8) & 0xF;
> +
> + if (dib >= 0x1) {
> + adapter->cmd_mask |= BIT(PECI_CMD_RD_PKG_CFG);
> + adapter->cmd_mask |= BIT(PECI_CMD_WR_PKG_CFG);
> + }
> +
> + if (dib >= 0x2)
> + adapter->cmd_mask |= BIT(PECI_CMD_RD_IA_MSR);
> +
> + if (dib >= 0x3) {
> + adapter->cmd_mask |= BIT(PECI_CMD_RD_PCI_CFG_LOCAL);
> + adapter->cmd_mask |= BIT(PECI_CMD_WR_PCI_CFG_LOCAL);
> + }
> +
> + if (dib >= 0x4)
> + adapter->cmd_mask |= BIT(PECI_CMD_RD_PCI_CFG);
> +
> + if (dib >= 0x5)
> + adapter->cmd_mask |= BIT(PECI_CMD_WR_PCI_CFG);
> +
> + if (dib >= 0x6)
> + adapter->cmd_mask |= BIT(PECI_CMD_WR_IA_MSR);
> +
> + adapter->cmd_mask |= BIT(PECI_CMD_GET_TEMP);
> + adapter->cmd_mask |= BIT(PECI_CMD_GET_DIB);
> + adapter->cmd_mask |= BIT(PECI_CMD_PING);
These cmd_mask updates are not done with any locking in mind. Is this
intentional? Or: is synchronization not necessary because this is
always done during enumeration prior to exposing the adapter to users?
> + } else {
> + dev_dbg(&adapter->dev, "Error reading DIB, rc : %d\n", rc);
> + }
> +
> + return rc;
> +}
> +
> +static int peci_cmd_support(struct peci_adapter *adapter, enum peci_cmd cmd)
> +{
> + if (!(adapter->cmd_mask & BIT(PECI_CMD_PING)) &&
> + peci_scan_cmd_mask(adapter) < 0) {
> + dev_dbg(&adapter->dev, "Failed to scan command mask\n");
> + return -EIO;
> + }
> +
> + if (!(adapter->cmd_mask & BIT(cmd))) {
> + dev_dbg(&adapter->dev, "Command %d is not supported\n", cmd);
> + return -EINVAL;
> + }
It would be nicer if you did this check prior to dispatching to the
various subfunctions (peci_ioctl_ping, peci_ioctl_get_dib, etc.). In
that way, these functions could just assume the adapter supports them.
[..]
> +static int peci_register_adapter(struct peci_adapter *adapter)
> +{
> + int res = -EINVAL;
> +
> + /* Can't register until after driver model init */
> + if (WARN_ON(!is_registered)) {
Is this solving a problem you actually ran into?
[.. skipped review due to fatigue ..]
> +++ b/include/linux/peci.h
> @@ -0,0 +1,97 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2018 Intel Corporation
> +
> +#ifndef __LINUX_PECI_H
> +#define __LINUX_PECI_H
> +
> +#include <linux/cdev.h>
> +#include <linux/device.h>
> +#include <linux/peci-ioctl.h>
> +#include <linux/rtmutex.h>
> +
> +#define PECI_BUFFER_SIZE 32
> +#define PECI_NAME_SIZE 32
> +
> +struct peci_xfer_msg {
> + u8 addr;
> + u8 tx_len;
> + u8 rx_len;
> + u8 tx_buf[PECI_BUFFER_SIZE];
> + u8 rx_buf[PECI_BUFFER_SIZE];
> +} __attribute__((__packed__));
The packed attribute has historically caused gcc to emit atrocious code,
as it seems to assume packed implies members might not be naturally
aligned. Seeing as you're only working with u8s in this case, though,
this shouldn't be a problem.
> +struct peci_board_info {
> + char type[PECI_NAME_SIZE];
> + u8 addr; /* CPU client address */
> + struct device_node *of_node;
> +};
> +
> +struct peci_adapter {
> + struct module *owner;
> + struct rt_mutex bus_lock;
Why an rt_mutex, instead of a regular mutex. Do you explicitly need PI
in mainline?
> + struct device dev;
> + struct cdev cdev;
> + int nr;
> + char name[PECI_NAME_SIZE];
> + int (*xfer)(struct peci_adapter *adapter,
> + struct peci_xfer_msg *msg);
> + uint cmd_mask;
> +};
> +
> +#define to_peci_adapter(d) container_of(d, struct peci_adapter, dev)
You can also do this with a static inline, which provides a marginally
better error when screwed up.
Julia
--
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] docs: clarify security-bugs disclosure policy
From: Kees Cook @ 2018-03-07 0:30 UTC (permalink / raw)
To: Dave Hansen
Cc: LKML, Thomas Gleixner, Greg KH, Linus Torvalds, Alan Cox,
Andrea Arcangeli, Andy Lutomirski, Tim Chen, Dan Williams,
Al Viro, Andrew Morton, linux-doc, Jonathan Corbet, Mark Rutland
In-Reply-To: <20180306233140.268BD8E1@viggo.jf.intel.com>
On Tue, Mar 6, 2018 at 3:31 PM, Dave Hansen <dave.hansen@linux.intel.com> wrote:
>
> From: Dave Hansen <dave.hansen@linux.intel.com>
>
> I think we need to soften the language a bit. It might scare folks
> off, especially the:
>
> We prefer to fully disclose the bug as soon as possible.
>
> which is not really the case. As Greg mentioned in private mail, we
> really do not prefer to disclose things until *after* a fix. The
> whole "we have the final say" is also a bit harsh.
> [...]
> ----------
>
> -The goal of the Linux kernel security team is to work with the
> -bug submitter to bug resolution as well as disclosure. We prefer
> -to fully disclose the bug as soon as possible. It is reasonable to
> -delay disclosure when the bug or the fix is not yet fully understood,
> -the solution is not well-tested or for vendor coordination. However, we
> -expect these delays to be short, measurable in days, not weeks or months.
> +The goal of the Linux kernel security team is to work with the bug
> +submitter to bug resolution as well as disclosure. We prefer to fully
> +disclose the bug as soon as possible after a fix is available. It is
> +customary to delay disclosure when the bug or the fix is not yet fully
> +understood, the solution is not well-tested or for vendor coordination.
> +However, we expect these delays to typically be short, measurable in
> +days, not weeks or months.
> +
> A disclosure date is negotiated by the security team working with the
> -bug submitter as well as vendors. However, the kernel security team
> -holds the final say when setting a disclosure date. The timeframe for
> -disclosure is from immediate (esp. if it's already publicly known)
> -to a few weeks. As a basic default policy, we expect report date to
> -disclosure date to be on the order of 7 days.
> +bug submitter as well as affected vendors. The security team prefers
> +coordinated disclosure and will consider pre-existing, reasonable
> +disclosure dates.
> +
> +The timeframe for disclosure ranges from immediate (esp. if it's
> +already publicly known) to a few weeks. As a basic default policy, we
> +expect report date to disclosure date to be on the order of 7 days.
This seems reasonable. Though I have two thoughts related to "we have
final say" and why it was there:
Sometimes we get insane embargo requests, and Linus has wanted to go
public ASAP. The wording was chosen to let reporters know that it is
possible for issues that don't need to wait 7 days to not wait 7 days.
For example, letting security@kernel.org know about a flaw and then
tell us to sit on it for 2 months until some public presentation,
that's not going to happen.
Additionally, we frequently make all network bugs immediately public,
since the net subsystem tends to reject embargoes.
So, maybe we could be more explicit about these cases? Or explicitly
describe what "reasonable" means (it's only hinted at).
-Kees
--
Kees Cook
Pixel Security
--
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] docs: clarify security-bugs disclosure policy
From: Dave Hansen @ 2018-03-06 23:31 UTC (permalink / raw)
To: linux-kernel
Cc: Dave Hansen, tglx, gregkh, torvalds, gnomes, aarcange, luto,
keescook, tim.c.chen, dan.j.williams, viro, akpm, linux-doc,
corbet, mark.rutland
From: Dave Hansen <dave.hansen@linux.intel.com>
I think we need to soften the language a bit. It might scare folks
off, especially the:
We prefer to fully disclose the bug as soon as possible.
which is not really the case. As Greg mentioned in private mail, we
really do not prefer to disclose things until *after* a fix. The
whole "we have the final say" is also a bit harsh.
Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Alan Cox <gnomes@lxorguk.ukuu.org.uk>
Cc: Andrea Arcangeli <aarcange@redhat.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Kees Cook <keescook@google.com>
Cc: Tim Chen <tim.c.chen@linux.intel.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: linux-doc@vger.kernel.org
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Mark Rutland <mark.rutland@arm.com>
---
b/Documentation/admin-guide/security-bugs.rst | 26 +++++++++++++++-----------
1 file changed, 15 insertions(+), 11 deletions(-)
diff -puN Documentation/admin-guide/security-bugs.rst~embargo Documentation/admin-guide/security-bugs.rst
--- a/Documentation/admin-guide/security-bugs.rst~embargo 2018-03-06 14:47:04.519431230 -0800
+++ b/Documentation/admin-guide/security-bugs.rst 2018-03-06 14:57:46.410429629 -0800
@@ -29,18 +29,22 @@ made public.
Disclosure
----------
-The goal of the Linux kernel security team is to work with the
-bug submitter to bug resolution as well as disclosure. We prefer
-to fully disclose the bug as soon as possible. It is reasonable to
-delay disclosure when the bug or the fix is not yet fully understood,
-the solution is not well-tested or for vendor coordination. However, we
-expect these delays to be short, measurable in days, not weeks or months.
+The goal of the Linux kernel security team is to work with the bug
+submitter to bug resolution as well as disclosure. We prefer to fully
+disclose the bug as soon as possible after a fix is available. It is
+customary to delay disclosure when the bug or the fix is not yet fully
+understood, the solution is not well-tested or for vendor coordination.
+However, we expect these delays to typically be short, measurable in
+days, not weeks or months.
+
A disclosure date is negotiated by the security team working with the
-bug submitter as well as vendors. However, the kernel security team
-holds the final say when setting a disclosure date. The timeframe for
-disclosure is from immediate (esp. if it's already publicly known)
-to a few weeks. As a basic default policy, we expect report date to
-disclosure date to be on the order of 7 days.
+bug submitter as well as affected vendors. The security team prefers
+coordinated disclosure and will consider pre-existing, reasonable
+disclosure dates.
+
+The timeframe for disclosure ranges from immediate (esp. if it's
+already publicly known) to a few weeks. As a basic default policy, we
+expect report date to disclosure date to be on the order of 7 days.
Coordination
------------
_
--
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 5/7] thermal/drivers/cpu_cooling: Add idle cooling device documentation
From: Pavel Machek @ 2018-03-06 23:19 UTC (permalink / raw)
To: Daniel Lezcano
Cc: edubezval, kevin.wangtao, leo.yan, vincent.guittot, amit.kachhap,
linux-kernel, javi.merino, rui.zhang, daniel.thompson, linux-pm,
Jonathan Corbet, open list:DOCUMENTATION
In-Reply-To: <1519226968-19821-6-git-send-email-daniel.lezcano@linaro.org>
[-- Attachment #1: Type: text/plain, Size: 4470 bytes --]
Hi!
> --- /dev/null
> +++ b/Documentation/thermal/cpu-idle-cooling.txt
> @@ -0,0 +1,165 @@
> +
> +Situation:
> +----------
> +
Can we have some real header here? Also if this is .rst, maybe it
should be marked so?
> +Under certain circumstances, the SoC reaches a temperature exceeding
> +the allocated power budget or the maximum temperature limit. The
I don't understand. Power budget is in W, temperature is in
kelvin. Temperature can't exceed power budget AFAICT.
> +former must be mitigated to stabilize the SoC temperature around the
> +temperature control using the defined cooling devices, the latter
later?
> +catastrophic situation where a radical decision must be taken to
> +reduce the temperature under the critical threshold, that can impact
> +the performances.
performance.
> +Another situation is when the silicon reaches a certain temperature
> +which continues to increase even if the dynamic leakage is reduced to
> +its minimum by clock gating the component. The runaway phenomena will
> +continue with the static leakage and only powering down the component,
> +thus dropping the dynamic and static leakage will allow the component
> +to cool down. This situation is critical.
Critical here, critical there. I have trouble following
it. Theoretically hardware should protect itself, because you don't
want kernel bug to damage your CPU?
> +Last but not least, the system can ask for a specific power budget but
> +because of the OPP density, we can only choose an OPP with a power
> +budget lower than the requested one and underuse the CPU, thus losing
> +performances. In other words, one OPP under uses the CPU with a
performance.
> +lesser than the power budget and the next OPP exceed the power budget,
> +an intermediate OPP could have been used if it were present.
was.
> +Solutions:
> +----------
> +
> +If we can remove the static and the dynamic leakage for a specific
> +duration in a controlled period, the SoC temperature will
> +decrease. Acting at the idle state duration or the idle cycle
"should" decrease? If you are in bad environment..
> +The Operating Performance Point (OPP) density has a great influence on
> +the control precision of cpufreq, however different vendors have a
> +plethora of OPP density, and some have large power gap between OPPs,
> +that will result in loss of performance during thermal control and
> +loss of power in other scenes.
scene seems to be wrong word here.
> +At a specific OPP, we can assume injecting idle cycle on all CPUs,
Extra comma?
> +Idle Injection:
> +---------------
> +
> +The base concept of the idle injection is to force the CPU to go to an
> +idle state for a specified time each control cycle, it provides
> +another way to control CPU power and heat in addition to
> +cpufreq. Ideally, if all CPUs of a cluster inject idle synchronously,
> +this cluster can get into the deepest idle state and achieve minimum
> +power consumption, but that will also increase system response latency
> +if we inject less than cpuidle latency.
I don't understand last sentence.
> +The mitigation begins with a maximum period value which decrease
decreases?
> +more cooling effect is requested. When the period duration is equal
> to
> +the idle duration, then we are in a situation the platform can’t
> +dissipate the heat enough and the mitigation fails. In this case
fast enough?
> +situation is considered critical and there is nothing to do. The idle
Nothing to do? Maybe power the system down?
> +The idle injection duration value must comply with the constraints:
> +
> +- It is lesser or equal to the latency we tolerate when the mitigation
less ... than the latency
> +Minimum period
> +--------------
> +
> +The idle injection duration being fixed, it is obvious the minimum
> +period can’t be lesser than that, otherwise we will be scheduling the
less.
> +Practically, if the running power is lesses than the targeted power,
less.
> +However, in this demonstration we ignore three aspects:
> +
> + * The static leakage is not defined here, we can introduce it in the
> + equation but assuming it will be zero most of the time as it is
, but?
Best regards,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: [PATCH v3 2/4] soc: qcom: Add GENI based QUP Wrapper driver
From: Stephen Boyd @ 2018-03-06 21:56 UTC (permalink / raw)
To: Karthik Ramasubramanian, 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: <4ac213a6-081f-72c1-c9c8-6994786285c9@codeaurora.org>
Quoting Karthik Ramasubramanian (2018-03-02 16:58:23)
>
>
> On 3/2/2018 1:41 PM, Stephen Boyd wrote:
> > Quoting Karthikeyan Ramasubramanian (2018-02-27 17:38:07)
> >> +
> >> +/**
> >> + * 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.
Ok. Maybe it can also be a macro to get the u32 and then some more
macros on top of that to pick out the major/minor/step out of the u32
that you read.
> >
> >> +
> >> +/**
> >> + * 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.
Ok maybe we don't really need it then?
> >> + * 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?
Not that I'm aware of. It's really up to you to decide.
> >
> >> +
> >> + 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.
Ok, but that would basically be some code consolidation around picking a
bandwidth and enabling/disabling? It sounds like it could go into either
the serial interface drivers or into the runtime PM path of the wrapper.
> >
> >> +
> >> +/**
> >> + * 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.
Hmm sounds like the round rate should happen on the parent of the
se_clk, and this wrapper DT binding should get the clk for the parent of
the se->clk to run round_rate() on. Then it could all be done in probe,
which sounds good.
> >> + 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.
Ok, thanks.
> >> +{
> >> + 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.
It would be a good idea to check with some other people on the dma_ops
suggestion. Maybe add the DMA mapping subsystem folks to help out here
DMA MAPPING HELPERS
M: Christoph Hellwig <hch@lst.de>
M: Marek Szyprowski <m.szyprowski@samsung.com>
R: Robin Murphy <robin.murphy@arm.com>
L: iommu@lists.linux-foundation.org
> >
> >> +
> >> +/* 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.
That's fine, but I mean to have struct geni_wrapper *wrapper, and then
struct geni_wrapper; in this file. Children won't know details and we
get slightly more type safety.
--
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-06 21:45 UTC (permalink / raw)
To: Karthik Ramasubramanian, 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, Girish Mahadevan, Sagar Dharia,
Doug Anderson
In-Reply-To: <f900dc96-0150-079d-bcd6-9d2e05f8fde1@codeaurora.org>
Quoting Karthik Ramasubramanian (2018-03-05 16:51:23)
> On 3/2/2018 3:11 PM, Stephen Boyd wrote:
> > Quoting Karthikeyan Ramasubramanian (2018-02-27 17:38:09)
> >
> >> + 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.
> Yes. Based on the baud rate of 115200 and the FIFO Depth & Width of (16
> * 32), the poll should not take more than 5 ms under the timeout scenario.
Sure, but I'm asking if this has any sort of timeout associated with it.
It looks to be a while loop that could possibly go forever?
> >> +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.
> If we fail to grab the lock, then there is another active writer. So
> trying to write to the fifo will put the hardware in bad state because
> writer has programmed the hardware to write 'x' number of words and this
> thread will over-write it with 'y' number of words. Also the data that
> you might see in the console might be garbled.
I suspect that because this is the serial console, and we want it to
always output stuff even when we're going down in flames, we may want to
ignore that case and just wait for the fifo to free up and then
overwrite the number of words that we want to output and push out more
characters.
I always get confused though because there seem to be lots of different
ways to do things in serial drivers and not too much clear documentation
that I've read describing what to do.
> >
> >> + spin_unlock_irqrestore(&uport->lock, flags);
> >> + }
> >> +}
[...]
> >> +
> >> + 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.
> The reason it is not added is because the hardware does not indicate
> when the break character occured. It can be any one of the FIFO words.
> The statistics is updated to give an idea that the break happened. We
> can add uart_handle_break() but it may not be at an accurate position
> for the above mentioned reason.
Sounds like the previous uart design. We would want uart_handle_break()
support for kgdb to work over serial. Do we still get an interrupt
signal that a break character is somewhere in the fifo? If we get that,
then does it also put a NUL (0) character into the fifo that we can
find? I had to do something like that before, where we detect the irq
and then we go through the fifo a character at a time to find the break
character that looks like a NUL, and then let sysrq core handle the
characters after that break character comes in.
> >
> >
> >> +}
> >> +
> >> +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.
> Based on my current expertise with the CCF, I will address this comment
> in a later patchset than the next one.
Ok. I've seen similar designs in some mmc drivers. For example, you can
look at drivers/mmc/host/meson-gx-mmc.c and see that they register a
clk_ops and then just start using that clk directly from the driver.
There are also some helper functions for dividers that would hopefully
make the job of calculating the best divider easier.
> >> +
> >> + 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.
> I think you mean parenthesis. I do not see casting here.
Yes! You got it.
> >> +#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.
> There were compiler warnings assigning a const void * to a void *. That
> is why I have the cast in place.
Oh. Yes you shouldn't cast away the const. Please make the compiler
warning go away without casting it away.
> >
> >
> > Also, I see some serial drivers do the mapping when the port is
> > requested. That can't be done here?
> By "when the port is requested" do you mean console's setup operation.
> It can be done, but given that it is a one-time operation I am not sure
> if it makes any difference between mapping here and there.
No. I meant the uart_ops::uart_request() function and also
uart_ops::config_port(). Take a look at msm_config_port() and
msm_request_port() for how it was done on pre-geni qcom uarts.
I suppose we should try to probe this as a module and run a getty on it
and then remove and probe the module a couple times after that.
That should shake out some bugs.
> >> +
> >> +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.
> The intention is to enable the console UART port usage as late as
> possible in the suspend cycle. Hence noirq variants. I will add this as
> a comment. Please let me know if the usage does not match the intention.
Hm.. Does no_console_suspend not work? Or not work well enough?
--
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 6/8] [PATCH 6/8] Documentation: hwmon: Add a document for PECI hwmon client driver
From: Jae Hyun Yoo @ 2018-03-06 21:08 UTC (permalink / raw)
To: Randy Dunlap, joel, andrew, arnd, gregkh, jdelvare, linux, benh,
andrew
Cc: linux-kernel, linux-doc, devicetree, linux-hwmon,
linux-arm-kernel, openbmc
In-Reply-To: <0e3750a0-d45b-368d-f5cb-d2cca6797ecb@infradead.org>
Hi Randy,
On 3/6/2018 12:28 PM, Randy Dunlap wrote:
> Hi,
>
> On 02/21/2018 08:16 AM, Jae Hyun Yoo wrote:
>
>> +temp<n>_label Provides DDR DIMM temperature if this label indicates
>> + 'DIMM #'.
>> +temp<n>_input Provides current temperature of the DDR DIMM.
>> +
>> +Note:
>> + DIMM temperature group will be appeared when the client CPU's BIOS
>
> will appear when
>
I'll fix this description as you suggested. Thanks a lot!
Jae
>> + completes memory training and testing.
>>
>
>
--
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 6/8] [PATCH 6/8] Documentation: hwmon: Add a document for PECI hwmon client driver
From: Randy Dunlap @ 2018-03-06 20:28 UTC (permalink / raw)
To: Jae Hyun Yoo, joel, andrew, arnd, gregkh, jdelvare, linux, benh,
andrew
Cc: linux-kernel, linux-doc, devicetree, linux-hwmon,
linux-arm-kernel, openbmc
In-Reply-To: <20180221161606.32247-7-jae.hyun.yoo@linux.intel.com>
Hi,
On 02/21/2018 08:16 AM, Jae Hyun Yoo wrote:
> +temp<n>_label Provides DDR DIMM temperature if this label indicates
> + 'DIMM #'.
> +temp<n>_input Provides current temperature of the DDR DIMM.
> +
> +Note:
> + DIMM temperature group will be appeared when the client CPU's BIOS
will appear when
> + completes memory training and testing.
>
--
~Randy
--
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/8] PECI device driver introduction
From: Jae Hyun Yoo @ 2018-03-06 19:21 UTC (permalink / raw)
To: Pavel Machek
Cc: joel, andrew, arnd, gregkh, jdelvare, linux, benh, andrew,
linux-kernel, linux-doc, devicetree, linux-hwmon,
linux-arm-kernel, openbmc
In-Reply-To: <20180306124014.GB13950@amd>
Hi Pavel,
Please see my answer inline.
On 3/6/2018 4:40 AM, Pavel Machek wrote:
> Hi!
>
>> Introduction of the Platform Environment Control Interface (PECI) bus
>> device driver. PECI is a one-wire bus interface that provides a
>> communication channel between Intel processor and chipset components to
>> external monitoring or control devices. PECI is designed to support the
>> following sideband functions:
>>
>> * Processor and DRAM thermal management
>> - Processor fan speed control is managed by comparing Digital Thermal
>> Sensor (DTS) thermal readings acquired via PECI against the
>> processor-specific fan speed control reference point, or TCONTROL.
>> Both TCONTROL and DTS thermal readings are accessible via the processor
>> PECI client. These variables are referenced to a common temperature,
>> the TCC activation point, and are both defined as negative offsets from
>> that reference.
>> - PECI based access to the processor package configuration space provides
>> a means for Baseboard Management Controllers (BMC) or other platform
>> management devices to actively manage the processor and memory power
>> and thermal features.
>>
>> * Platform Manageability
>> - Platform manageability functions including thermal, power, and error
>> monitoring. Note that platform 'power' management includes monitoring
>> and control for both the processor and DRAM subsystem to assist with
>> data center power limiting.
>> - PECI allows read access to certain error registers in the processor MSR
>> space and status monitoring registers in the PCI configuration space
>> within the processor and downstream devices.
>> - PECI permits writes to certain registers in the processor PCI
>> configuration space.
>>
>> * Processor Interface Tuning and Diagnostics
>> - Processor interface tuning and diagnostics capabilities
>> (Intel(c) Interconnect BIST). The processors Intel(c) Interconnect
>> Built In Self Test (Intel(c) IBIST) allows for infield diagnostic
>> capabilities in the Intel UPI and memory controller interfaces. PECI
>> provides a port to execute these diagnostics via its PCI Configuration
>> read and write capabilities.
>>
>> * Failure Analysis
>> - Output the state of the processor after a failure for analysis via
>> Crashdump.
>>
>> PECI uses a single wire for self-clocking and data transfer. The bus
>> requires no additional control lines. The physical layer is a self-clocked
>> one-wire bus that begins each bit with a driven, rising edge from an idle
>> level near zero volts. The duration of the signal driven high depends on
>> whether the bit value is a logic '0' or logic '1'. PECI also includes
>> variable data transfer rate established with every message. In this way,
>> it is highly flexible even though underlying logic is simple.
>>
>> The interface design was optimized for interfacing to Intel processor and
>> chipset components in both single processor and multiple processor
>> environments. The single wire interface provides low board routing
>> overhead for the multiple load connections in the congested routing area
>> near the processor and chipset components. Bus speed, error checking, and
>> low protocol overhead provides adequate link bandwidth and reliability to
>> transfer critical device operating conditions and configuration
>> information.
>>
>> This implementation provides the basic framework to add PECI extensions
>> to the Linux bus and device models. A hardware specific 'Adapter' driver
>> can be attached to the PECI bus to provide sideband functions described
>> above. It is also possible to access all devices on an adapter from
>> userspace through the /dev interface. A device specific 'Client' driver
>> also can be attached to the PECI bus so each processor client's features
>> can be supported by the 'Client' driver through an adapter connection in
>> the bus. This patch set includes Aspeed 24xx/25xx PECI driver and a generic
>> PECI hwmon driver as the first implementation for both adapter and client
>> drivers on the PECI bus framework.
>
> Ok, how does this interact with ACPI/SMM BIOS/Secure mode code? Does
> Linux _need_ to control the fan? Or is SMM BIOS capable of doing all
> the work itself and Linux has just read-only access for monitoring
> purposes?
>
This driver is not for local CPUs which this driver is running on.
Instead, this driver will be running on BMC (Baseboard Management
Controller) kernel which is separated from the server machine. In this
implementation, it provides just read-only access for monitoring the
server's CPU and DIMM temperatures remotely through a PECI connection.
The BMC can control fans according to the monitoring data if the BMC has
a fan control interface and feature, but it depends on baseboard
hardware and software designs.
Thanks,
Jae
> Pavel
>
> -- (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures)
> http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.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
* Re: [PATCH v2 2/8] [PATCH 2/8] Documentations: dt-bindings: Add a document of PECI adapter driver for Aspeed AST24xx/25xx SoCs
From: Jae Hyun Yoo @ 2018-03-06 19:05 UTC (permalink / raw)
To: Pavel Machek
Cc: joel, andrew, arnd, gregkh, jdelvare, linux, benh, andrew,
linux-kernel, linux-doc, devicetree, linux-hwmon,
linux-arm-kernel, openbmc
In-Reply-To: <20180306124002.GA13950@amd>
Hi Pavel,
Thanks for sharing your time on reviewing it. Please see my answers inline.
-Jae
On 3/6/2018 4:40 AM, Pavel Machek wrote:
> Hi!
>
>> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
>> ---
>> .../devicetree/bindings/peci/peci-aspeed.txt | 73 ++++++++++++++++++++++
>> 1 file changed, 73 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/peci/peci-aspeed.txt
>>
>> diff --git a/Documentation/devicetree/bindings/peci/peci-aspeed.txt b/Documentation/devicetree/bindings/peci/peci-aspeed.txt
>> new file mode 100644
>> index 000000000000..8a86f346d550
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/peci/peci-aspeed.txt
>> @@ -0,0 +1,73 @@
>> +Device tree configuration for PECI buses on the AST24XX and AST25XX SoCs.
>
> Are these SoCs x86-based?
>
Yes, these are ARM SoCs. Please see Andrew's answer as well.
>> +Required properties:
>> +- compatible
>> + "aspeed,ast2400-peci" or "aspeed,ast2500-peci"
>> + - aspeed,ast2400-peci: Aspeed AST2400 family PECI controller
>> + - aspeed,ast2500-peci: Aspeed AST2500 family PECI controller
>> +
>> +- reg
>> + Should contain PECI registers location and length.
>
> Other dts documents put it on one line, reg: Should contain ...
>
>> +- clock_frequency
>> + Should contain the operation frequency of PECI hardware module.
>> + 187500 ~ 24000000
>
> specify this is Hz?
>
I'll add a description. Thanks!
>> +- rd-sampling-point
>> + Read sampling point selection. The whole period of a bit time will be
>> + divided into 16 time frames. This value will determine which time frame
>> + this controller will sample PECI signal for data read back. Usually in
>> + the middle of a bit time is the best.
>
> English? "This value will determine when this controller"?
>
Could I change it like below?:
"This value will determine in which time frame this controller samples
PECI signal for data read back"
>> + 0 ~ 15 (default: 8)
>> +
>> +- cmd_timeout_ms
>> + Command timeout in units of ms.
>> + 1 ~ 60000 (default: 1000)
>> +
>> +Example:
>> + peci: peci@1e78b000 {
>> + compatible = "simple-bus";
>> + #address-cells = <1>;
>> + #size-cells = <1>;
>> + ranges = <0x0 0x1e78b000 0x60>;
>> +
>> + peci0: peci-bus@0 {
>> + compatible = "aspeed,ast2500-peci";
>> + reg = <0x0 0x60>;
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + interrupts = <15>;
>> + clocks = <&clk_clkin>;
>> + clock-frequency = <24000000>;
>> + msg-timing-nego = <1>;
>> + addr-timing-nego = <1>;
>> + rd-sampling-point = <8>;
>> + cmd-timeout-ms = <1000>;
>> + };
>> + };
>> \ No newline at end of file
>
--
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: add my name to kernel driver statement
From: Jonathan Corbet @ 2018-03-06 17:48 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: Aaro Koskinen, linux-doc, linux-kernel
In-Reply-To: <20180306174628.GA9333@kroah.com>
On Tue, 6 Mar 2018 09:46:28 -0800
Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> Jon, can you pick this up or do you need me to forward this to you?
I've got it, no worries.
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: add my name to kernel driver statement
From: Greg Kroah-Hartman @ 2018-03-06 17:46 UTC (permalink / raw)
To: Aaro Koskinen; +Cc: Jonathan Corbet, linux-doc, linux-kernel
In-Reply-To: <20180304201814.19641-1-aaro.koskinen@iki.fi>
On Sun, Mar 04, 2018 at 10:18:14PM +0200, Aaro Koskinen wrote:
> Add my name to kernel driver statement.
Thanks for doing this!
> Signed-off-by: Aaro Koskinen <aaro.koskinen@iki.fi>
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Jon, can you pick this up or do you need me to forward this to you?
thanks,
greg k-h
--
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] w1: fix w1_ds2438 documentation
From: Mauro Carvalho Chehab @ 2018-03-06 17:29 UTC (permalink / raw)
To: Mariusz Bialonczyk
Cc: Linux Media Mailing List, Mauro Carvalho Chehab, Evgeniy Polyakov,
Jonathan Corbet, linux-doc
In-Reply-To: <20180302075524.27868-1-manio@skyboo.net>
Em Fri, 2 Mar 2018 08:55:24 +0100
Mariusz Bialonczyk <manio@skyboo.net> escreveu:
> Signed-off-by: Mariusz Bialonczyk <manio@skyboo.net>
> ---
> Documentation/w1/slaves/w1_ds2438 | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Documentation/w1/slaves/w1_ds2438 b/Documentation/w1/slaves/w1_ds2438
> index b99f3674c5b4..e64f65a09387 100644
> --- a/Documentation/w1/slaves/w1_ds2438
> +++ b/Documentation/w1/slaves/w1_ds2438
> @@ -60,4 +60,4 @@ vad: general purpose A/D input (VAD)
> vdd: battery input (VDD)
>
> After the voltage conversion the value is returned as decimal ASCII.
> -Note: The value is in mV, so to get a volts the value has to be divided by 10.
> +Note: To get a volts the value has to be divided by 100.
Hmm... I've no idea why you sent this to linux-media and to me...
The proper maintainer seems to be Evgeniy:
./scripts/get_maintainer.pl -f Documentation/w1/slaves/w1_ds2438
Evgeniy Polyakov <zbr@ioremap.net> (maintainer:W1 DALLAS'S 1-WIRE BUS)
Jonathan Corbet <corbet@lwn.net> (maintainer:DOCUMENTATION)
Greg Kroah-Hartman <gregkh@linuxfoundation.org> (commit_signer:1/1=100%)
Mariusz Bialonczyk <manio@skyboo.net> (commit_signer:1/1=100%,authored:1/1=100%,added_lines:63/63=100%)
linux-doc@vger.kernel.org (open list:DOCUMENTATION)
linux-kernel@vger.kernel.org (open list)
Thanks,
Mauro
--
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 1/4] dt-bindings: soc: qcom: Add device tree binding for GENI SE
From: Karthik Ramasubramanian @ 2018-03-06 17:13 UTC (permalink / raw)
To: Rob Herring
Cc: Jonathan Corbet, Andy Gross, David Brown, Mark Rutland,
Wolfram Sang, Greg Kroah-Hartman, linux-doc, linux-arm-msm,
devicetree, Linux I2C, linux-serial, Jiri Slaby, evgreen,
acourbot, Sagar Dharia, Girish Mahadevan
In-Reply-To: <CAL_JsqJHSSsVs4Lut_f7Nk7NU1=NQnOvsyXz8g9-STrwO58GrA@mail.gmail.com>
On 3/6/2018 6:22 AM, Rob Herring wrote:
> On Mon, Mar 5, 2018 at 6:55 PM, Karthik Ramasubramanian
> <kramasub@codeaurora.org> wrote:
>>
>>
>> On 3/5/2018 4:58 PM, Rob Herring wrote:
>>>
>>> On Tue, Feb 27, 2018 at 06:38:06PM -0700, Karthikeyan Ramasubramanian
>>> wrote:
>>>>
>>>> Add device tree binding support for the QCOM GENI SE driver.
>>>>
>>>> Signed-off-by: Karthikeyan Ramasubramanian <kramasub@codeaurora.org>
>>>> Signed-off-by: Sagar Dharia <sdharia@codeaurora.org>
>>>> Signed-off-by: Girish Mahadevan <girishm@codeaurora.org>
>>>> ---
>>>> .../devicetree/bindings/soc/qcom/qcom,geni-se.txt | 89
>>>> ++++++++++++++++++++++
>>>> 1 file changed, 89 insertions(+)
>>>> create mode 100644
>>>> Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt
>>>> b/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt
>>>> new file mode 100644
>>>> index 0000000..fe6a0c0
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt
>>>> @@ -0,0 +1,89 @@
>>>> +Qualcomm Technologies, Inc. GENI Serial Engine QUP Wrapper Controller
>>>> +
>>>> +Generic Interface (GENI) based Qualcomm Universal Peripheral (QUP)
>>>> wrapper
>>>> +is a 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 GENI Serial Engine
>>>> QUP
>>>> +Wrapper controller is modeled as a node with zero or more child nodes
>>>> each
>>>> +representing a serial engine.
>>>> +
>>>> +Required properties:
>>>> +- compatible: Must be "qcom,geni-se-qup".
>>>> +- reg: Must contain QUP register address and length.
>>>> +- clock-names: Must contain "m-ahb" and "s-ahb".
>>>> +- clocks: AHB clocks needed by the device.
>>>> +
>>>> +Required properties if child node exists:
>>>> +- #address-cells: Must be <1> for Serial Engine Address
>>>> +- #size-cells: Must be <1> for Serial Engine Address
>>>> Size
>>>> +- ranges: Must be present
>>>> +
>>>> +Properties for children:
>>>> +
>>>> +A GENI based QUP wrapper controller node can contain 0 or more child
>>>> nodes
>>>> +representing serial devices. These serial devices can be a QCOM UART,
>>>> I2C
>>>> +controller, spi controller, or some combination of aforementioned
>>>> devices.
>>>
>>>
>>> s/spi/SPI/
>>>
>>> Where's the SPI binding?
>>
>> Since the patch series introduces UART and I2C drivers, I added the bindings
>> only for them. I thought about adding the SPI binding when the SPI
>> controller driver is introduced. Please let me know if you want me to add
>> the bindings for SPI in this patch series itself.
>
> There's no requirement to have the driver and I prefer bindings be as
> complete as possible.
Ok, I will add the bindings for SPI controller in the next posting.
>
> Rob
>
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
* Re: [PATCH] HID: logitech-hidpp: document sysfs interface
From: Jiri Kosina @ 2018-03-06 14:18 UTC (permalink / raw)
To: Aishwarya Pant
Cc: Benjamin Tissoires, linux-input, linux-kernel, Jonathan Corbet,
Greg KH, Julia Lawall, linux-doc
In-Reply-To: <20180302131653.GA24934@mordor.localdomain>
On Fri, 2 Mar 2018, Aishwarya Pant wrote:
> Descriptions have been collected from git commit logs.
Applied, thanks.
--
Jiri Kosina
SUSE Labs
--
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 1/4] dt-bindings: soc: qcom: Add device tree binding for GENI SE
From: Rob Herring @ 2018-03-06 13:22 UTC (permalink / raw)
To: Karthik Ramasubramanian
Cc: Jonathan Corbet, Andy Gross, David Brown, Mark Rutland,
Wolfram Sang, Greg Kroah-Hartman, linux-doc, linux-arm-msm,
devicetree, Linux I2C, linux-serial, Jiri Slaby, evgreen,
acourbot, Sagar Dharia, Girish Mahadevan
In-Reply-To: <af0d4155-c3bf-97cb-98b2-978093a30611@codeaurora.org>
On Mon, Mar 5, 2018 at 6:55 PM, Karthik Ramasubramanian
<kramasub@codeaurora.org> wrote:
>
>
> On 3/5/2018 4:58 PM, Rob Herring wrote:
>>
>> On Tue, Feb 27, 2018 at 06:38:06PM -0700, Karthikeyan Ramasubramanian
>> wrote:
>>>
>>> Add device tree binding support for the QCOM GENI SE driver.
>>>
>>> Signed-off-by: Karthikeyan Ramasubramanian <kramasub@codeaurora.org>
>>> Signed-off-by: Sagar Dharia <sdharia@codeaurora.org>
>>> Signed-off-by: Girish Mahadevan <girishm@codeaurora.org>
>>> ---
>>> .../devicetree/bindings/soc/qcom/qcom,geni-se.txt | 89
>>> ++++++++++++++++++++++
>>> 1 file changed, 89 insertions(+)
>>> create mode 100644
>>> Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt
>>> b/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt
>>> new file mode 100644
>>> index 0000000..fe6a0c0
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt
>>> @@ -0,0 +1,89 @@
>>> +Qualcomm Technologies, Inc. GENI Serial Engine QUP Wrapper Controller
>>> +
>>> +Generic Interface (GENI) based Qualcomm Universal Peripheral (QUP)
>>> wrapper
>>> +is a 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 GENI Serial Engine
>>> QUP
>>> +Wrapper controller is modeled as a node with zero or more child nodes
>>> each
>>> +representing a serial engine.
>>> +
>>> +Required properties:
>>> +- compatible: Must be "qcom,geni-se-qup".
>>> +- reg: Must contain QUP register address and length.
>>> +- clock-names: Must contain "m-ahb" and "s-ahb".
>>> +- clocks: AHB clocks needed by the device.
>>> +
>>> +Required properties if child node exists:
>>> +- #address-cells: Must be <1> for Serial Engine Address
>>> +- #size-cells: Must be <1> for Serial Engine Address
>>> Size
>>> +- ranges: Must be present
>>> +
>>> +Properties for children:
>>> +
>>> +A GENI based QUP wrapper controller node can contain 0 or more child
>>> nodes
>>> +representing serial devices. These serial devices can be a QCOM UART,
>>> I2C
>>> +controller, spi controller, or some combination of aforementioned
>>> devices.
>>
>>
>> s/spi/SPI/
>>
>> Where's the SPI binding?
>
> Since the patch series introduces UART and I2C drivers, I added the bindings
> only for them. I thought about adding the SPI binding when the SPI
> controller driver is introduced. Please let me know if you want me to add
> the bindings for SPI in this patch series itself.
There's no requirement to have the driver and I prefer bindings be as
complete as possible.
Rob
--
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 2/8] [PATCH 2/8] Documentations: dt-bindings: Add a document of PECI adapter driver for Aspeed AST24xx/25xx SoCs
From: Arnd Bergmann @ 2018-03-06 13:19 UTC (permalink / raw)
To: Pavel Machek
Cc: Andrew Lunn, Jae Hyun Yoo, Joel Stanley, Andrew Jeffery, gregkh,
Jean Delvare, Guenter Roeck, Benjamin Herrenschmidt,
Linux Kernel Mailing List, open list:DOCUMENTATION, DTML,
linux-hwmon, Linux ARM, OpenBMC Maillist
In-Reply-To: <20180306130551.GA16061@amd>
On Tue, Mar 6, 2018 at 2:05 PM, Pavel Machek <pavel@ucw.cz> wrote:
> On Tue 2018-03-06 13:54:16, Andrew Lunn wrote:
>> On Tue, Mar 06, 2018 at 01:40:02PM +0100, Pavel Machek wrote:
>> > Hi!
>> >
>> > > Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
>> > > ---
>> > > .../devicetree/bindings/peci/peci-aspeed.txt | 73 ++++++++++++++++++++++
>> > > 1 file changed, 73 insertions(+)
>> > > create mode 100644 Documentation/devicetree/bindings/peci/peci-aspeed.txt
>> > >
>> > > diff --git a/Documentation/devicetree/bindings/peci/peci-aspeed.txt b/Documentation/devicetree/bindings/peci/peci-aspeed.txt
>> > > new file mode 100644
>> > > index 000000000000..8a86f346d550
>> > > --- /dev/null
>> > > +++ b/Documentation/devicetree/bindings/peci/peci-aspeed.txt
>> > > @@ -0,0 +1,73 @@
>> > > +Device tree configuration for PECI buses on the AST24XX and AST25XX SoCs.
>> >
>> > Are these SoCs x86-based?
>>
>> ARM, as far as i can tell. If i get the architecture correct, these
>> are BMC, Board Management Controllers, looking after the main x86 CPU,
>> stopping it overheating, controlling the power supplies, remote
>> management, etc.
>
> Ok, so with x86 machine, I get arm-based one for free. I get it. Is
> user able to run his own kernel on the arm system, or is it locked
> down, TiVo style?
In the past, they were all locked down, the team submitting those
patches in working on changing that. Have a look for OpenBMC.
Arnd
--
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 2/8] [PATCH 2/8] Documentations: dt-bindings: Add a document of PECI adapter driver for Aspeed AST24xx/25xx SoCs
From: Pavel Machek @ 2018-03-06 13:05 UTC (permalink / raw)
To: Andrew Lunn
Cc: Jae Hyun Yoo, joel, andrew, arnd, gregkh, jdelvare, linux, benh,
linux-kernel, linux-doc, devicetree, linux-hwmon,
linux-arm-kernel, openbmc
In-Reply-To: <20180306125416.GD26143@lunn.ch>
[-- Attachment #1: Type: text/plain, Size: 1406 bytes --]
On Tue 2018-03-06 13:54:16, Andrew Lunn wrote:
> On Tue, Mar 06, 2018 at 01:40:02PM +0100, Pavel Machek wrote:
> > Hi!
> >
> > > Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
> > > ---
> > > .../devicetree/bindings/peci/peci-aspeed.txt | 73 ++++++++++++++++++++++
> > > 1 file changed, 73 insertions(+)
> > > create mode 100644 Documentation/devicetree/bindings/peci/peci-aspeed.txt
> > >
> > > diff --git a/Documentation/devicetree/bindings/peci/peci-aspeed.txt b/Documentation/devicetree/bindings/peci/peci-aspeed.txt
> > > new file mode 100644
> > > index 000000000000..8a86f346d550
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/peci/peci-aspeed.txt
> > > @@ -0,0 +1,73 @@
> > > +Device tree configuration for PECI buses on the AST24XX and AST25XX SoCs.
> >
> > Are these SoCs x86-based?
>
> ARM, as far as i can tell. If i get the architecture correct, these
> are BMC, Board Management Controllers, looking after the main x86 CPU,
> stopping it overheating, controlling the power supplies, remote
> management, etc.
Ok, so with x86 machine, I get arm-based one for free. I get it. Is
user able to run his own kernel on the arm system, or is it locked
down, TiVo style?
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: [PATCH v2 2/8] [PATCH 2/8] Documentations: dt-bindings: Add a document of PECI adapter driver for Aspeed AST24xx/25xx SoCs
From: Andrew Lunn @ 2018-03-06 12:54 UTC (permalink / raw)
To: Pavel Machek
Cc: Jae Hyun Yoo, joel, andrew, arnd, gregkh, jdelvare, linux, benh,
linux-kernel, linux-doc, devicetree, linux-hwmon,
linux-arm-kernel, openbmc
In-Reply-To: <20180306124002.GA13950@amd>
On Tue, Mar 06, 2018 at 01:40:02PM +0100, Pavel Machek wrote:
> Hi!
>
> > Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
> > ---
> > .../devicetree/bindings/peci/peci-aspeed.txt | 73 ++++++++++++++++++++++
> > 1 file changed, 73 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/peci/peci-aspeed.txt
> >
> > diff --git a/Documentation/devicetree/bindings/peci/peci-aspeed.txt b/Documentation/devicetree/bindings/peci/peci-aspeed.txt
> > new file mode 100644
> > index 000000000000..8a86f346d550
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/peci/peci-aspeed.txt
> > @@ -0,0 +1,73 @@
> > +Device tree configuration for PECI buses on the AST24XX and AST25XX SoCs.
>
> Are these SoCs x86-based?
ARM, as far as i can tell. If i get the architecture correct, these
are BMC, Board Management Controllers, looking after the main x86 CPU,
stopping it overheating, controlling the power supplies, remote
management, etc.
Andrew
--
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/8] PECI device driver introduction
From: Pavel Machek @ 2018-03-06 12:40 UTC (permalink / raw)
To: Jae Hyun Yoo
Cc: joel, andrew, arnd, gregkh, jdelvare, linux, benh, andrew,
linux-kernel, linux-doc, devicetree, linux-hwmon,
linux-arm-kernel, openbmc
In-Reply-To: <20180221161606.32247-1-jae.hyun.yoo@linux.intel.com>
[-- Attachment #1: Type: text/plain, Size: 4427 bytes --]
Hi!
> Introduction of the Platform Environment Control Interface (PECI) bus
> device driver. PECI is a one-wire bus interface that provides a
> communication channel between Intel processor and chipset components to
> external monitoring or control devices. PECI is designed to support the
> following sideband functions:
>
> * Processor and DRAM thermal management
> - Processor fan speed control is managed by comparing Digital Thermal
> Sensor (DTS) thermal readings acquired via PECI against the
> processor-specific fan speed control reference point, or TCONTROL.
> Both TCONTROL and DTS thermal readings are accessible via the processor
> PECI client. These variables are referenced to a common temperature,
> the TCC activation point, and are both defined as negative offsets from
> that reference.
> - PECI based access to the processor package configuration space provides
> a means for Baseboard Management Controllers (BMC) or other platform
> management devices to actively manage the processor and memory power
> and thermal features.
>
> * Platform Manageability
> - Platform manageability functions including thermal, power, and error
> monitoring. Note that platform 'power' management includes monitoring
> and control for both the processor and DRAM subsystem to assist with
> data center power limiting.
> - PECI allows read access to certain error registers in the processor MSR
> space and status monitoring registers in the PCI configuration space
> within the processor and downstream devices.
> - PECI permits writes to certain registers in the processor PCI
> configuration space.
>
> * Processor Interface Tuning and Diagnostics
> - Processor interface tuning and diagnostics capabilities
> (Intel(c) Interconnect BIST). The processors Intel(c) Interconnect
> Built In Self Test (Intel(c) IBIST) allows for infield diagnostic
> capabilities in the Intel UPI and memory controller interfaces. PECI
> provides a port to execute these diagnostics via its PCI Configuration
> read and write capabilities.
>
> * Failure Analysis
> - Output the state of the processor after a failure for analysis via
> Crashdump.
>
> PECI uses a single wire for self-clocking and data transfer. The bus
> requires no additional control lines. The physical layer is a self-clocked
> one-wire bus that begins each bit with a driven, rising edge from an idle
> level near zero volts. The duration of the signal driven high depends on
> whether the bit value is a logic '0' or logic '1'. PECI also includes
> variable data transfer rate established with every message. In this way,
> it is highly flexible even though underlying logic is simple.
>
> The interface design was optimized for interfacing to Intel processor and
> chipset components in both single processor and multiple processor
> environments. The single wire interface provides low board routing
> overhead for the multiple load connections in the congested routing area
> near the processor and chipset components. Bus speed, error checking, and
> low protocol overhead provides adequate link bandwidth and reliability to
> transfer critical device operating conditions and configuration
> information.
>
> This implementation provides the basic framework to add PECI extensions
> to the Linux bus and device models. A hardware specific 'Adapter' driver
> can be attached to the PECI bus to provide sideband functions described
> above. It is also possible to access all devices on an adapter from
> userspace through the /dev interface. A device specific 'Client' driver
> also can be attached to the PECI bus so each processor client's features
> can be supported by the 'Client' driver through an adapter connection in
> the bus. This patch set includes Aspeed 24xx/25xx PECI driver and a generic
> PECI hwmon driver as the first implementation for both adapter and client
> drivers on the PECI bus framework.
Ok, how does this interact with ACPI/SMM BIOS/Secure mode code? Does
Linux _need_ to control the fan? Or is SMM BIOS capable of doing all
the work itself and Linux has just read-only access for monitoring
purposes?
Pavel
-- (english) http://www.livejournal.com/~pavelmachek
(cesky, pictures)
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: [PATCH v2 2/8] [PATCH 2/8] Documentations: dt-bindings: Add a document of PECI adapter driver for Aspeed AST24xx/25xx SoCs
From: Pavel Machek @ 2018-03-06 12:40 UTC (permalink / raw)
To: Jae Hyun Yoo
Cc: joel, andrew, arnd, gregkh, jdelvare, linux, benh, andrew,
linux-kernel, linux-doc, devicetree, linux-hwmon,
linux-arm-kernel, openbmc
In-Reply-To: <20180221161606.32247-3-jae.hyun.yoo@linux.intel.com>
[-- Attachment #1: Type: text/plain, Size: 2389 bytes --]
Hi!
> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo@linux.intel.com>
> ---
> .../devicetree/bindings/peci/peci-aspeed.txt | 73 ++++++++++++++++++++++
> 1 file changed, 73 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/peci/peci-aspeed.txt
>
> diff --git a/Documentation/devicetree/bindings/peci/peci-aspeed.txt b/Documentation/devicetree/bindings/peci/peci-aspeed.txt
> new file mode 100644
> index 000000000000..8a86f346d550
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/peci/peci-aspeed.txt
> @@ -0,0 +1,73 @@
> +Device tree configuration for PECI buses on the AST24XX and AST25XX SoCs.
Are these SoCs x86-based?
> +Required properties:
> +- compatible
> + "aspeed,ast2400-peci" or "aspeed,ast2500-peci"
> + - aspeed,ast2400-peci: Aspeed AST2400 family PECI controller
> + - aspeed,ast2500-peci: Aspeed AST2500 family PECI controller
> +
> +- reg
> + Should contain PECI registers location and length.
Other dts documents put it on one line, reg: Should contain ...
> +- clock_frequency
> + Should contain the operation frequency of PECI hardware module.
> + 187500 ~ 24000000
specify this is Hz?
> +- rd-sampling-point
> + Read sampling point selection. The whole period of a bit time will be
> + divided into 16 time frames. This value will determine which time frame
> + this controller will sample PECI signal for data read back. Usually in
> + the middle of a bit time is the best.
English? "This value will determine when this controller"?
> + 0 ~ 15 (default: 8)
> +
> +- cmd_timeout_ms
> + Command timeout in units of ms.
> + 1 ~ 60000 (default: 1000)
> +
> +Example:
> + peci: peci@1e78b000 {
> + compatible = "simple-bus";
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges = <0x0 0x1e78b000 0x60>;
> +
> + peci0: peci-bus@0 {
> + compatible = "aspeed,ast2500-peci";
> + reg = <0x0 0x60>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + interrupts = <15>;
> + clocks = <&clk_clkin>;
> + clock-frequency = <24000000>;
> + msg-timing-nego = <1>;
> + addr-timing-nego = <1>;
> + rd-sampling-point = <8>;
> + cmd-timeout-ms = <1000>;
> + };
> + };
> \ No newline at end of file
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: [PATCH v3 2/2] kconfig: rename silentoldconfig to syncconfig
From: Masahiro Yamada @ 2018-03-06 9:39 UTC (permalink / raw)
To: Linux Kbuild mailing list
Cc: Sam Ravnborg, Ulf Magnusson, Randy Dunlap, Marc Herbert,
Masahiro Yamada, Linux Kernel Mailing List,
open list:DOCUMENTATION, Jonathan Corbet, Jeff Kirsher,
Michal Marek, intel-wired-lan
In-Reply-To: <1519886077-31914-2-git-send-email-yamada.masahiro@socionext.com>
2018-03-01 15:34 GMT+09:00 Masahiro Yamada <yamada.masahiro@socionext.com>:
> As commit cedd55d49dee ("kconfig: Remove silentoldconfig from help
> and docs; fix kconfig/conf's help") mentioned, 'silentoldconfig' is a
> historical misnomer. That commit removed it from help and docs since
> it is an internal interface. If so, it should be allowed to rename
> it to something more intuitive. 'syncconfig' is the one I came up
> with because it updates the .config if necessary, then synchronize
> include/generated/autoconf.h and include/config/* with it.
>
> You should not manually invoke 'silentoldcofig'. Display warning if
> used in case existing scripts are doing wrong.
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
>
> Changes in v3:
> - Fix Documentation/networking/i40e.txt
> - Display warning if silentoldconfig is used
>
> Changes in v2:
> - newly added
>
Both applied to linux-kbuild/kconfig.
--
Best Regards
Masahiro Yamada
--
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 1/4] dt-bindings: soc: qcom: Add device tree binding for GENI SE
From: Karthik Ramasubramanian @ 2018-03-06 0:55 UTC (permalink / raw)
To: Rob Herring
Cc: corbet, andy.gross, david.brown, mark.rutland, wsa, gregkh,
linux-doc, linux-arm-msm, devicetree, linux-i2c, linux-serial,
jslaby, evgreen, acourbot, Sagar Dharia, Girish Mahadevan
In-Reply-To: <20180305235831.i7kxups7khus2g5c@rob-hp-laptop>
On 3/5/2018 4:58 PM, Rob Herring wrote:
> On Tue, Feb 27, 2018 at 06:38:06PM -0700, Karthikeyan Ramasubramanian wrote:
>> Add device tree binding support for the QCOM GENI SE driver.
>>
>> Signed-off-by: Karthikeyan Ramasubramanian <kramasub@codeaurora.org>
>> Signed-off-by: Sagar Dharia <sdharia@codeaurora.org>
>> Signed-off-by: Girish Mahadevan <girishm@codeaurora.org>
>> ---
>> .../devicetree/bindings/soc/qcom/qcom,geni-se.txt | 89 ++++++++++++++++++++++
>> 1 file changed, 89 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt
>>
>> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt b/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt
>> new file mode 100644
>> index 0000000..fe6a0c0
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt
>> @@ -0,0 +1,89 @@
>> +Qualcomm Technologies, Inc. GENI Serial Engine QUP Wrapper Controller
>> +
>> +Generic Interface (GENI) based Qualcomm Universal Peripheral (QUP) wrapper
>> +is a 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 GENI Serial Engine QUP
>> +Wrapper controller is modeled as a node with zero or more child nodes each
>> +representing a serial engine.
>> +
>> +Required properties:
>> +- compatible: Must be "qcom,geni-se-qup".
>> +- reg: Must contain QUP register address and length.
>> +- clock-names: Must contain "m-ahb" and "s-ahb".
>> +- clocks: AHB clocks needed by the device.
>> +
>> +Required properties if child node exists:
>> +- #address-cells: Must be <1> for Serial Engine Address
>> +- #size-cells: Must be <1> for Serial Engine Address Size
>> +- ranges: Must be present
>> +
>> +Properties for children:
>> +
>> +A GENI based QUP wrapper controller node can contain 0 or more child nodes
>> +representing serial devices. These serial devices can be a QCOM UART, I2C
>> +controller, spi controller, or some combination of aforementioned devices.
>
> s/spi/SPI/
>
> Where's the SPI binding?
Since the patch series introduces UART and I2C drivers, I added the
bindings only for them. I thought about adding the SPI binding when the
SPI controller driver is introduced. Please let me know if you want me
to add the bindings for SPI in this patch series itself.
>
>> +Please refer below the child node definitions for the supported serial
>> +interface protocols.
>> +
>> +Qualcomm Technologies Inc. GENI Serial Engine based I2C Controller
>> +
>> +Required properties:
>> +- compatible: Must be "qcom,geni-i2c".
>> +- reg: Must contain QUP register address and length.
>> +- interrupts: Must contain I2C interrupt.
>> +- clock-names: Must contain "se".
>> +- clocks: Serial engine core clock needed by the device.
>> +- #address-cells: Must be <1> for i2c device address.
>> +- #size-cells: Must be <0> as i2c addresses have no size component.
>> +
>> +Optional property:
>> +- clock-frequency: Desired I2C bus clock frequency in Hz.
>> + When missing default to 400000Hz.
>> +
>> +Child nodes should conform to i2c bus binding as described in i2c.txt.
>> +
>> +Qualcomm Technologies Inc. GENI Serial Engine based UART Controller
>> +
>> +Required properties:
>> +- compatible: Must be "qcom,geni-debug-uart".
>> +- reg: Must contain UART register location and length.
>> +- interrupts: Must contain UART core interrupts.
>> +- clock-names: Must contain "se".
>> +- clocks: Serial engine core clock needed by the device.
>> +
>> +Example:
>> + geniqup@8c0000 {
>> + compatible = "qcom,geni-se-qup";
>> + reg = <0x8c0000 0x6000>;
>> + clock-names = "m-ahb", "s-ahb";
>> + clocks = <&clock_gcc GCC_QUPV3_WRAP_0_M_AHB_CLK>,
>> + <&clock_gcc GCC_QUPV3_WRAP_0_S_AHB_CLK>;
>> + #address-cells = <1>;
>> + #size-cells = <1>;
>> + ranges;
>> +
>> + i2c0: i2c@a94000 {
>> + compatible = "qcom,geni-i2c";
>> + reg = <0xa94000 0x4000>;
>> + interrupts = <GIC_SPI 358 IRQ_TYPE_LEVEL_HIGH>;
>> + clock-names = "se";
>> + clocks = <&clock_gcc GCC_QUPV3_WRAP0_S5_CLK>;
>> + pinctrl-names = "default", "sleep";
>> + pinctrl-0 = <&qup_1_i2c_5_active>;
>> + pinctrl-1 = <&qup_1_i2c_5_sleep>;
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> + };
>> +
>> + uart0: serial@a88000 {
>> + compatible = "qcom,geni-debug-uart";
>> + reg = <0xa88000 0x7000>;
>> + interrupts = <GIC_SPI 355 IRQ_TYPE_LEVEL_HIGH>;
>> + clock-names = "se";
>> + clocks = <&clock_gcc GCC_QUPV3_WRAP0_S0_CLK>;
>> + pinctrl-names = "default", "sleep";
>> + pinctrl-0 = <&qup_1_uart_3_active>;
>> + pinctrl-1 = <&qup_1_uart_3_sleep>;
>> + };
>> + }
>> --
>> Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>> a Linux Foundation Collaborative Project
>>
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
* Re: [PATCH v3 4/4] tty: serial: msm_geni_serial: Add serial driver support for GENI based QUP
From: Karthik Ramasubramanian @ 2018-03-06 0:51 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, Girish Mahadevan, Sagar Dharia,
Doug Anderson
In-Reply-To: <152002870571.108663.443363731684218377@swboyd.mtv.corp.google.com>
On 3/2/2018 3:11 PM, Stephen Boyd wrote:
> 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
> ?
Ok.
>
>> + 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.
Ok.
>
>> + 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?
Ok.
>
>> + bool port_setup;
>
> Maybe just 'setup'? Port is in the type already.
Ok.
>
>> + int (*handle_rx)(struct uart_port *uport,
>> + u32 rx_bytes, bool drop_rx);
>
> s/rx_bytes/bytes/
> s/drop_rx/drop/
Ok.
>
>> + unsigned int xmit_size;
>> + unsigned int cur_baud;
>
> s/cur//
Ok.
>
>> + 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/
Ok.
>
>> +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?
I will check and remove if not required.
>
> s/bit_// ?
Ok.
>
>> +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.
Ok. I will remove it and rely on DT always having alias.
>
>> +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?
Our current use-case does not need more than one instance. But more
instances can be added if desired.
>
>> +
>> +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.
Ok.
>
>> + 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 ^
>
I will fix it.
>> + */
>> + 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.
Ok.
>
>> + 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
Ok.
>
>> + int i;
>> + u32 bytes_to_send = count;
>> +
>> + for (i = 0; i < count; i++) {
>> + if (s[i] == '\n')
>> + new_line++;
>
> bytes_to_send++;
Ok.
>
>> + }
>> +
>> + bytes_to_send += new_line;
>
> Drop.
Ok.
>
>> + 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.
Yes. Based on the baud rate of 115200 and the FIFO Depth & Width of (16
* 32), the poll should not take more than 5 ms under the timeout scenario.
>
>> + 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.
Ok.
>
>> + 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.
If we fail to grab the lock, then there is another active writer. So
trying to write to the fifo will put the hardware in bad state because
writer has programmed the hardware to write 'x' number of words and this
thread will over-write it with 'y' number of words. Also the data that
you might see in the console might be garbled.
>
>> + 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.
Ok.
>
>> + i -= bytes;
>> + if (drop)
>> + continue;
>> + buf = (unsigned char *)&rx_fifo;
>
> So that this cast becomes unnecessary, and endian agnostic.
Ok.
>
>> +
>> + 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.
Ok.
>
>> + 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.
I will fix the comments.
>
>> + */
>> + 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/
Ok.
>
>> +{
>> + 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.
Ok.
>
>> + 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; ) {
Ok.
>
>> + 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.
The reason it is not added is because the hardware does not indicate
when the break character occured. It can be any one of the FIFO words.
The statistics is updated to give an idea that the break happened. We
can add uart_handle_break() but it may not be at an accurate position
for the above mentioned reason.
>
>> + }
>> +
>> + 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?
We haven't seen a failure yet. I can drop the check and rely on the fact
that the hardware is programmed correctly.
>
>> +
>> + 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?
Ok.
>
>> +}
>> +
>> +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.
Ok.
>
>> + 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.
Bjorn had a similar comment and there I mentioned that the writes are
required during early console setup as well. Since the popular vote is
towards inlining these writes, I will update it accordingly.
>
>> +}
>> +
>> +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.
Based on my current expertise with the CCF, I will address this comment
in a later patchset than the next one.
>
>> + 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.
I think you mean parenthesis. I do not see casting here.
>
>> +
>> + /* 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.
Ok.
>
>> + 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
Ok.
>
>> +{
>> + 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.
There were compiler warnings assigning a const void * to a void *. That
is why I have the cast in place.
>
>> + 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.
I will drop it.
>
>> + 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()
Ok.
>
>> + dev_err(&pdev->dev, "Err IO mapping serial iomem");
>
> No need for error message with devm_ioremap_resource()
Ok.
>
>> + return -ENOMEM;
>
> return PTR_ERR(..)
Ok.
>
> Also, I see some serial drivers do the mapping when the port is
> requested. That can't be done here?
By "when the port is requested" do you mean console's setup operation.
It can be done, but given that it is a one-time operation I am not sure
if it makes any difference between mapping here and there.
>
>> + }
>> + 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.
The intention is to enable the console UART port usage as late as
possible in the suspend cycle. Hence noirq variants. I will add this as
a comment. Please let me know if the usage does not match the intention.
>
>> +};
>> +
>> +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.
Ok.
>
>> +
>> + 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-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
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
* Re: [PATCH v3 1/4] dt-bindings: soc: qcom: Add device tree binding for GENI SE
From: Rob Herring @ 2018-03-05 23:58 UTC (permalink / raw)
To: Karthikeyan Ramasubramanian
Cc: corbet, andy.gross, david.brown, mark.rutland, wsa, gregkh,
linux-doc, linux-arm-msm, devicetree, linux-i2c, linux-serial,
jslaby, evgreen, acourbot, Sagar Dharia, Girish Mahadevan
In-Reply-To: <1519781889-16117-2-git-send-email-kramasub@codeaurora.org>
On Tue, Feb 27, 2018 at 06:38:06PM -0700, Karthikeyan Ramasubramanian wrote:
> Add device tree binding support for the QCOM GENI SE driver.
>
> Signed-off-by: Karthikeyan Ramasubramanian <kramasub@codeaurora.org>
> Signed-off-by: Sagar Dharia <sdharia@codeaurora.org>
> Signed-off-by: Girish Mahadevan <girishm@codeaurora.org>
> ---
> .../devicetree/bindings/soc/qcom/qcom,geni-se.txt | 89 ++++++++++++++++++++++
> 1 file changed, 89 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt
>
> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt b/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt
> new file mode 100644
> index 0000000..fe6a0c0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,geni-se.txt
> @@ -0,0 +1,89 @@
> +Qualcomm Technologies, Inc. GENI Serial Engine QUP Wrapper Controller
> +
> +Generic Interface (GENI) based Qualcomm Universal Peripheral (QUP) wrapper
> +is a 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 GENI Serial Engine QUP
> +Wrapper controller is modeled as a node with zero or more child nodes each
> +representing a serial engine.
> +
> +Required properties:
> +- compatible: Must be "qcom,geni-se-qup".
> +- reg: Must contain QUP register address and length.
> +- clock-names: Must contain "m-ahb" and "s-ahb".
> +- clocks: AHB clocks needed by the device.
> +
> +Required properties if child node exists:
> +- #address-cells: Must be <1> for Serial Engine Address
> +- #size-cells: Must be <1> for Serial Engine Address Size
> +- ranges: Must be present
> +
> +Properties for children:
> +
> +A GENI based QUP wrapper controller node can contain 0 or more child nodes
> +representing serial devices. These serial devices can be a QCOM UART, I2C
> +controller, spi controller, or some combination of aforementioned devices.
s/spi/SPI/
Where's the SPI binding?
> +Please refer below the child node definitions for the supported serial
> +interface protocols.
> +
> +Qualcomm Technologies Inc. GENI Serial Engine based I2C Controller
> +
> +Required properties:
> +- compatible: Must be "qcom,geni-i2c".
> +- reg: Must contain QUP register address and length.
> +- interrupts: Must contain I2C interrupt.
> +- clock-names: Must contain "se".
> +- clocks: Serial engine core clock needed by the device.
> +- #address-cells: Must be <1> for i2c device address.
> +- #size-cells: Must be <0> as i2c addresses have no size component.
> +
> +Optional property:
> +- clock-frequency: Desired I2C bus clock frequency in Hz.
> + When missing default to 400000Hz.
> +
> +Child nodes should conform to i2c bus binding as described in i2c.txt.
> +
> +Qualcomm Technologies Inc. GENI Serial Engine based UART Controller
> +
> +Required properties:
> +- compatible: Must be "qcom,geni-debug-uart".
> +- reg: Must contain UART register location and length.
> +- interrupts: Must contain UART core interrupts.
> +- clock-names: Must contain "se".
> +- clocks: Serial engine core clock needed by the device.
> +
> +Example:
> + geniqup@8c0000 {
> + compatible = "qcom,geni-se-qup";
> + reg = <0x8c0000 0x6000>;
> + clock-names = "m-ahb", "s-ahb";
> + clocks = <&clock_gcc GCC_QUPV3_WRAP_0_M_AHB_CLK>,
> + <&clock_gcc GCC_QUPV3_WRAP_0_S_AHB_CLK>;
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges;
> +
> + i2c0: i2c@a94000 {
> + compatible = "qcom,geni-i2c";
> + reg = <0xa94000 0x4000>;
> + interrupts = <GIC_SPI 358 IRQ_TYPE_LEVEL_HIGH>;
> + clock-names = "se";
> + clocks = <&clock_gcc GCC_QUPV3_WRAP0_S5_CLK>;
> + pinctrl-names = "default", "sleep";
> + pinctrl-0 = <&qup_1_i2c_5_active>;
> + pinctrl-1 = <&qup_1_i2c_5_sleep>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + };
> +
> + uart0: serial@a88000 {
> + compatible = "qcom,geni-debug-uart";
> + reg = <0xa88000 0x7000>;
> + interrupts = <GIC_SPI 355 IRQ_TYPE_LEVEL_HIGH>;
> + clock-names = "se";
> + clocks = <&clock_gcc GCC_QUPV3_WRAP0_S0_CLK>;
> + pinctrl-names = "default", "sleep";
> + pinctrl-0 = <&qup_1_uart_3_active>;
> + pinctrl-1 = <&qup_1_uart_3_sleep>;
> + };
> + }
> --
> 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
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