[PATCH] firmware: google: Enable s0ix logging by default
Many moons ago, support was added to the SMI handlers to log s0ix entry and exit. Early iterations of firmware on Apollo Lake correctly returned "unsupported" for this new command they did not recognize, but unfortunately also contained a quirk where this command would cause them to power down rather than resume from s0ix. Fixes for this quirk were pushed out long ago, so all APL devices still in the field should have updated firmware. As such, we no longer need to have the s0ix_logging_enable be opt-in, where every new platform has to add this to their kernel commandline parameters. Change it to be on by default. In theory we could remove the parameter altogether: updated versions of Chrome OS containing a kernel with this change would also be coupled with firmware that behaves properly with these commands. Eventually we should probably do that. For now, convert this to an opt-out parameter so there's an emergency valve for people who are deliberately running old firmware, or as an escape hatch in case of unforeseen regressions. Signed-off-by: Evan Green --- drivers/firmware/google/gsmi.c | 14 +- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/drivers/firmware/google/gsmi.c b/drivers/firmware/google/gsmi.c index 3d77f26c1e8c93..bb6e77ee3898c1 100644 --- a/drivers/firmware/google/gsmi.c +++ b/drivers/firmware/google/gsmi.c @@ -136,12 +136,16 @@ MODULE_PARM_DESC(spincount, "The number of loop iterations to use when using the spin handshake."); /* - * Platforms might not support S0ix logging in their GSMI handlers. In order to - * avoid any side-effects of generating an SMI for S0ix logging, use the S0ix - * related GSMI commands only for those platforms that explicitly enable this - * option. + * Some older platforms with Apollo Lake chipsets do not support S0ix logging + * in their GSMI handlers, and behaved poorly when resuming via power button + * press if the logging was attempted. Updated firmware with proper behavior + * has long since shipped, removing the need for this opt-in parameter. It + * now exists as an opt-out parameter for folks defiantly running old + * firmware, or unforeseen circumstances. After the change from opt-in to + * opt-out has baked sufficiently, this parameter should probably be removed + * entirely. */ -static bool s0ix_logging_enable; +static bool s0ix_logging_enable = true; module_param(s0ix_logging_enable, bool, 0600); static struct gsmi_buf *gsmi_buf_alloc(void) -- 2.29.2
Guarantee Loan Offer At 2% Interest Rate Apply Now!!!
-- Are you looking for a loan? How much do you need as a loan and the duration time you can pay it back? thanks, Mr. Green Rodriguez of Equity Loan Company.
Re: [RESEND PATCH v3 2/2] i2c: i2c-mux-gpio: Enable this driver in ACPI land
On Sun, Jan 17, 2021 at 3:54 AM Wolfram Sang wrote: > > On Wed, Nov 18, 2020 at 03:40:25PM -0800, Evan Green wrote: > > Enable i2c-mux-gpio devices to be defined via ACPI. The idle-state > > property translates directly to a fwnode_property_*() call. The child > > reg property translates naturally into _ADR in ACPI. > > > > The i2c-parent binding is a relic from the days when the bindings > > dictated that all direct children of an I2C controller had to be I2C > > devices. These days that's no longer required. The i2c-mux can sit as a > > direct child of its parent controller, which is where it makes the most > > sense from a hardware description perspective. For the ACPI > > implementation we'll assume that's always how the i2c-mux-gpio is > > instantiated. > > > > Signed-off-by: Evan Green > > Applied to for-next, thanks! The code Andy mentioned can still be > refactored later if new ACPI helpers appear in the future. Thanks Wolfram and Peter!
Re: [RESEND PATCH v3 2/2] i2c: i2c-mux-gpio: Enable this driver in ACPI land
On Tue, Jan 5, 2021 at 2:25 AM Wolfram Sang wrote:
>
> On Fri, Nov 20, 2020 at 10:59:12AM -0800, Evan Green wrote:
> > On Thu, Nov 19, 2020 at 7:24 AM Andy Shevchenko
> > wrote:
> > >
> > > On Thu, Nov 19, 2020 at 1:40 AM Evan Green wrote:
> > > >
> > > > Enable i2c-mux-gpio devices to be defined via ACPI. The idle-state
> > > > property translates directly to a fwnode_property_*() call. The child
> > > > reg property translates naturally into _ADR in ACPI.
> > > >
> > > > The i2c-parent binding is a relic from the days when the bindings
> > > > dictated that all direct children of an I2C controller had to be I2C
> > > > devices. These days that's no longer required. The i2c-mux can sit as a
> > > > direct child of its parent controller, which is where it makes the most
> > > > sense from a hardware description perspective. For the ACPI
> > > > implementation we'll assume that's always how the i2c-mux-gpio is
> > > > instantiated.
> > >
> > > ...
> > >
> > > > +#ifdef CONFIG_ACPI
> > > > +
> > > > +static int i2c_mux_gpio_get_acpi_adr(struct device *dev,
> > > > +struct fwnode_handle *fwdev,
> > > > +unsigned int *adr)
> > > > +
> > > > +{
> > > > + unsigned long long adr64;
> > > > + acpi_status status;
> > > > +
> > > > + status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwdev),
> > > > + METHOD_NAME__ADR,
> > > > + NULL, );
> > > > +
> > > > + if (!ACPI_SUCCESS(status)) {
> > > > + dev_err(dev, "Cannot get address\n");
> > > > + return -EINVAL;
> > > > + }
> > > > +
> > > > + *adr = adr64;
> > > > + if (*adr != adr64) {
> > > > + dev_err(dev, "Address out of range\n");
> > > > + return -ERANGE;
> > > > + }
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +#else
> > > > +
> > > > +static int i2c_mux_gpio_get_acpi_adr(struct device *dev,
> > > > +struct fwnode_handle *fwdev,
> > > > +unsigned int *adr)
> > > > +{
> > > > + return -EINVAL;
> > > > +}
> > > > +
> > > > +#endif
> > >
> > > I'm wondering if you may use acpi_find_child_device() here.
> > > Or is it a complementary function?
> >
> > I think it's complementary. The code above is "I have a device, I want
> > its _ADR". whereas acpi_find_child_device() is "I have an _ADR, I want
> > its device". I could flip things around to use this, but it would turn
> > the code from linear into quadratic. I'd have to scan each possible
> > address and call acpi_find_child_device() with that _ADR to see if
> > there's a child device there.
> >
> > >
> > > ...
> > >
> > > > + device_for_each_child_node(dev, child) {
> > > > + if (is_of_node(child)) {
> > > > + fwnode_property_read_u32(child, "reg", values +
> > > > i);
> > > > +
> > > > + } else if (is_acpi_node(child)) {
> > > > + rc = i2c_mux_gpio_get_acpi_adr(dev, child,
> > > > values + i);
> > > > + if (rc)
> > > > + return rc;
> > > > + }
> > > > +
> > > > i++;
> > > > }
> > >
> > > And for this I already told in two different threads with similar code
> > > that perhaps we need common helper that will check reg followed by
> > > _ADR.
> >
> > Oh, I'm not aware of those threads. I'd need some advice: I guess a
> > new fwnode_* API would make sense for this, but I had trouble coming
> > up with a generic interface. _ADR is just a blobbo 64 bit int, but
> > DT's "reg" is a little more flexible, having a length, and potentially
> > being an array. I suppose it would have to be something like:
> >
> > int fwnode_property_read_reg(const struct fwnode_handle *fwnode,
> > size_t index, uint64_t *addr, uint64_t
> > *len);
> >
> > But then ACPI would always return 0 for length, and only index 0 would
> > ever work? I'm worried I'm designing an API that's only useful to me.
> >
> > I tried to look around for other examples of this specific pattern of
> > _ADR then "reg", but struggled to turn up much.
> > -Evan
>
> Andy, is Evan's answer satisfying for you?
Can this be accepted as-is, or should I resend?
-Evan
Re: [RESEND PATCH v3 2/2] i2c: i2c-mux-gpio: Enable this driver in ACPI land
Very sorry to ping. Is there anything I can do to help get this reviewed?
-Evan
On Mon, Nov 30, 2020 at 11:11 AM Evan Green wrote:
>
> Hi Andy, Peter,
>
> On Fri, Nov 20, 2020 at 10:59 AM Evan Green wrote:
> >
> > On Thu, Nov 19, 2020 at 7:24 AM Andy Shevchenko
> > wrote:
> > >
> > > On Thu, Nov 19, 2020 at 1:40 AM Evan Green wrote:
> > > >
> > > > Enable i2c-mux-gpio devices to be defined via ACPI. The idle-state
> > > > property translates directly to a fwnode_property_*() call. The child
> > > > reg property translates naturally into _ADR in ACPI.
> > > >
> > > > The i2c-parent binding is a relic from the days when the bindings
> > > > dictated that all direct children of an I2C controller had to be I2C
> > > > devices. These days that's no longer required. The i2c-mux can sit as a
> > > > direct child of its parent controller, which is where it makes the most
> > > > sense from a hardware description perspective. For the ACPI
> > > > implementation we'll assume that's always how the i2c-mux-gpio is
> > > > instantiated.
> > >
> > > ...
> > >
> > > > +#ifdef CONFIG_ACPI
> > > > +
> > > > +static int i2c_mux_gpio_get_acpi_adr(struct device *dev,
> > > > +struct fwnode_handle *fwdev,
> > > > +unsigned int *adr)
> > > > +
> > > > +{
> > > > + unsigned long long adr64;
> > > > + acpi_status status;
> > > > +
> > > > + status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwdev),
> > > > + METHOD_NAME__ADR,
> > > > + NULL, );
> > > > +
> > > > + if (!ACPI_SUCCESS(status)) {
> > > > + dev_err(dev, "Cannot get address\n");
> > > > + return -EINVAL;
> > > > + }
> > > > +
> > > > + *adr = adr64;
> > > > + if (*adr != adr64) {
> > > > + dev_err(dev, "Address out of range\n");
> > > > + return -ERANGE;
> > > > + }
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +#else
> > > > +
> > > > +static int i2c_mux_gpio_get_acpi_adr(struct device *dev,
> > > > +struct fwnode_handle *fwdev,
> > > > +unsigned int *adr)
> > > > +{
> > > > + return -EINVAL;
> > > > +}
> > > > +
> > > > +#endif
> > >
> > > I'm wondering if you may use acpi_find_child_device() here.
> > > Or is it a complementary function?
> >
> > I think it's complementary. The code above is "I have a device, I want
> > its _ADR". whereas acpi_find_child_device() is "I have an _ADR, I want
> > its device". I could flip things around to use this, but it would turn
> > the code from linear into quadratic. I'd have to scan each possible
> > address and call acpi_find_child_device() with that _ADR to see if
> > there's a child device there.
> >
> > >
> > > ...
> > >
> > > > + device_for_each_child_node(dev, child) {
> > > > + if (is_of_node(child)) {
> > > > + fwnode_property_read_u32(child, "reg", values +
> > > > i);
> > > > +
> > > > + } else if (is_acpi_node(child)) {
> > > > + rc = i2c_mux_gpio_get_acpi_adr(dev, child,
> > > > values + i);
> > > > + if (rc)
> > > > + return rc;
> > > > + }
> > > > +
> > > > i++;
> > > > }
> > >
> > > And for this I already told in two different threads with similar code
> > > that perhaps we need common helper that will check reg followed by
> > > _ADR.
> >
> > Oh, I'm not aware of those threads. I'd need some advice: I guess a
> > new fwnode_* API would make sense for this, but I had trouble coming
> > up with a generic interface. _ADR is just a blobbo 64 bit int, but
> > DT's "reg" is a little more flexible, having a length, and potentially
> > being an array. I suppose it would have to be something like:
> >
> > int fwnode_property_read_reg(const struct fwnode_handle *fwnode,
> > size_t index, uint64_t *addr, uint64_t
> > *len);
> >
> > But then ACPI would always return 0 for length, and only index 0 would
> > ever work? I'm worried I'm designing an API that's only useful to me.
> >
> > I tried to look around for other examples of this specific pattern of
> > _ADR then "reg", but struggled to turn up much.
>
> Any thoughts on this?
>
> > -Evan
> >
> > >
> > > --
> > > With Best Regards,
> > > Andy Shevchenko
Re: [PATCH v3 2/4] remoteproc: sysmon: Expose the shutdown result
On Sat, Nov 21, 2020 at 9:43 PM Bjorn Andersson
wrote:
>
> A graceful shutdown of the Qualcomm remote processors where
> traditionally performed by invoking a shared memory state signal and
> waiting for the associated ack.
>
> This was later superseded by the "sysmon" mechanism, where some form of
> shared memory bus is used to send a "graceful shutdown request" message
> and one of more signals comes back to indicate its success.
>
> But when this newer mechanism is in effect the firmware is shut down by
> the time the older mechanism, implemented in the remoteproc drivers,
> attempts to perform a graceful shutdown - and as such it will never
> receive an ack back.
>
> This patch therefor track the success of the latest shutdown attempt in
> sysmon and exposes a new function in the API that the remoteproc driver
> can use to query the success and the necessity of invoking the older
> mechanism.
>
> Tested-by: Steev Klimaszewski
> Reviewed-by: Rishabh Bhatnagar
> Signed-off-by: Bjorn Andersson
> ---
>
> Change since v2:
> - None
>
> drivers/remoteproc/qcom_common.h | 6 +++
> drivers/remoteproc/qcom_sysmon.c | 82
> 2 files changed, 69 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/remoteproc/qcom_common.h
> b/drivers/remoteproc/qcom_common.h
> index dfc641c3a98b..8ba9052955bd 100644
> --- a/drivers/remoteproc/qcom_common.h
> +++ b/drivers/remoteproc/qcom_common.h
> @@ -51,6 +51,7 @@ struct qcom_sysmon *qcom_add_sysmon_subdev(struct rproc
> *rproc,
>const char *name,
>int ssctl_instance);
> void qcom_remove_sysmon_subdev(struct qcom_sysmon *sysmon);
> +bool qcom_sysmon_shutdown_acked(struct qcom_sysmon *sysmon);
> #else
> static inline struct qcom_sysmon *qcom_add_sysmon_subdev(struct rproc *rproc,
> const char *name,
> @@ -62,6 +63,11 @@ static inline struct qcom_sysmon
> *qcom_add_sysmon_subdev(struct rproc *rproc,
> static inline void qcom_remove_sysmon_subdev(struct qcom_sysmon *sysmon)
> {
> }
> +
> +static inline bool qcom_sysmon_shutdown_acked(struct qcom_sysmon *sysmon)
> +{
> + return false;
> +}
> #endif
>
> #endif
> diff --git a/drivers/remoteproc/qcom_sysmon.c
> b/drivers/remoteproc/qcom_sysmon.c
> index b37b111b15b3..a428b707a6de 100644
> --- a/drivers/remoteproc/qcom_sysmon.c
> +++ b/drivers/remoteproc/qcom_sysmon.c
> @@ -44,6 +44,7 @@ struct qcom_sysmon {
> struct mutex lock;
>
> bool ssr_ack;
> + bool shutdown_acked;
>
> struct qmi_handle qmi;
> struct sockaddr_qrtr ssctl;
> @@ -115,10 +116,13 @@ static void sysmon_send_event(struct qcom_sysmon
> *sysmon,
> /**
> * sysmon_request_shutdown() - request graceful shutdown of remote
> * @sysmon:sysmon context
> + *
> + * Return: boolean indicator of the remote processor acking the request
> */
> -static void sysmon_request_shutdown(struct qcom_sysmon *sysmon)
> +static bool sysmon_request_shutdown(struct qcom_sysmon *sysmon)
> {
> char *req = "ssr:shutdown";
> + bool acked = false;
> int ret;
>
> mutex_lock(>lock);
> @@ -141,9 +145,13 @@ static void sysmon_request_shutdown(struct qcom_sysmon
> *sysmon)
> if (!sysmon->ssr_ack)
> dev_err(sysmon->dev,
> "unexpected response to sysmon shutdown request\n");
> + else
> + acked = true;
>
> out_unlock:
> mutex_unlock(>lock);
> +
> + return acked;
> }
>
> static int sysmon_callback(struct rpmsg_device *rpdev, void *data, int count,
> @@ -297,14 +305,33 @@ static struct qmi_msg_handler qmi_indication_handler[]
> = {
> {}
> };
>
> +static bool ssctl_request_shutdown_wait(struct qcom_sysmon *sysmon)
> +{
> + int ret;
> +
> + ret = wait_for_completion_timeout(>shutdown_comp, 10 * HZ);
> + if (ret)
> + return true;
> +
> + ret = try_wait_for_completion(>ind_comp);
> + if (ret)
> + return true;
> +
> + dev_err(sysmon->dev, "timeout waiting for shutdown ack\n");
> + return false;
> +}
> +
> /**
> * ssctl_request_shutdown() - request shutdown via SSCTL QMI service
> * @sysmon:sysmon context
> + *
> + * Return: boolean indicator of the remote processor acking the request
> */
> -static void ssctl_request_shutdown(struct qcom_sysmon *sysmon)
> +static bool ssctl_request_shutdown(struct qcom_sysmon *sysmon)
> {
> struct ssctl_shutdown_resp resp;
> struct qmi_txn txn;
> + bool acked = false;
> int ret;
>
> reinit_completion(>ind_comp);
> @@ -312,7 +339,7 @@ static void ssctl_request_shutdown(struct qcom_sysmon
> *sysmon)
> ret = qmi_txn_init(>qmi, , ssctl_shutdown_resp_ei, );
> if (ret < 0) {
> dev_err(sysmon->dev, "failed to allocate QMI txn\n");
> - return;
> +
Re: [RESEND PATCH v3 2/2] i2c: i2c-mux-gpio: Enable this driver in ACPI land
Hi Andy, Peter,
On Fri, Nov 20, 2020 at 10:59 AM Evan Green wrote:
>
> On Thu, Nov 19, 2020 at 7:24 AM Andy Shevchenko
> wrote:
> >
> > On Thu, Nov 19, 2020 at 1:40 AM Evan Green wrote:
> > >
> > > Enable i2c-mux-gpio devices to be defined via ACPI. The idle-state
> > > property translates directly to a fwnode_property_*() call. The child
> > > reg property translates naturally into _ADR in ACPI.
> > >
> > > The i2c-parent binding is a relic from the days when the bindings
> > > dictated that all direct children of an I2C controller had to be I2C
> > > devices. These days that's no longer required. The i2c-mux can sit as a
> > > direct child of its parent controller, which is where it makes the most
> > > sense from a hardware description perspective. For the ACPI
> > > implementation we'll assume that's always how the i2c-mux-gpio is
> > > instantiated.
> >
> > ...
> >
> > > +#ifdef CONFIG_ACPI
> > > +
> > > +static int i2c_mux_gpio_get_acpi_adr(struct device *dev,
> > > +struct fwnode_handle *fwdev,
> > > +unsigned int *adr)
> > > +
> > > +{
> > > + unsigned long long adr64;
> > > + acpi_status status;
> > > +
> > > + status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwdev),
> > > + METHOD_NAME__ADR,
> > > + NULL, );
> > > +
> > > + if (!ACPI_SUCCESS(status)) {
> > > + dev_err(dev, "Cannot get address\n");
> > > + return -EINVAL;
> > > + }
> > > +
> > > + *adr = adr64;
> > > + if (*adr != adr64) {
> > > + dev_err(dev, "Address out of range\n");
> > > + return -ERANGE;
> > > + }
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +#else
> > > +
> > > +static int i2c_mux_gpio_get_acpi_adr(struct device *dev,
> > > +struct fwnode_handle *fwdev,
> > > +unsigned int *adr)
> > > +{
> > > + return -EINVAL;
> > > +}
> > > +
> > > +#endif
> >
> > I'm wondering if you may use acpi_find_child_device() here.
> > Or is it a complementary function?
>
> I think it's complementary. The code above is "I have a device, I want
> its _ADR". whereas acpi_find_child_device() is "I have an _ADR, I want
> its device". I could flip things around to use this, but it would turn
> the code from linear into quadratic. I'd have to scan each possible
> address and call acpi_find_child_device() with that _ADR to see if
> there's a child device there.
>
> >
> > ...
> >
> > > + device_for_each_child_node(dev, child) {
> > > + if (is_of_node(child)) {
> > > + fwnode_property_read_u32(child, "reg", values +
> > > i);
> > > +
> > > + } else if (is_acpi_node(child)) {
> > > + rc = i2c_mux_gpio_get_acpi_adr(dev, child, values
> > > + i);
> > > + if (rc)
> > > + return rc;
> > > + }
> > > +
> > > i++;
> > > }
> >
> > And for this I already told in two different threads with similar code
> > that perhaps we need common helper that will check reg followed by
> > _ADR.
>
> Oh, I'm not aware of those threads. I'd need some advice: I guess a
> new fwnode_* API would make sense for this, but I had trouble coming
> up with a generic interface. _ADR is just a blobbo 64 bit int, but
> DT's "reg" is a little more flexible, having a length, and potentially
> being an array. I suppose it would have to be something like:
>
> int fwnode_property_read_reg(const struct fwnode_handle *fwnode,
> size_t index, uint64_t *addr, uint64_t *len);
>
> But then ACPI would always return 0 for length, and only index 0 would
> ever work? I'm worried I'm designing an API that's only useful to me.
>
> I tried to look around for other examples of this specific pattern of
> _ADR then "reg", but struggled to turn up much.
Any thoughts on this?
> -Evan
>
> >
> > --
> > With Best Regards,
> > Andy Shevchenko
Re: [RESEND PATCH v3 2/2] i2c: i2c-mux-gpio: Enable this driver in ACPI land
On Thu, Nov 19, 2020 at 7:24 AM Andy Shevchenko
wrote:
>
> On Thu, Nov 19, 2020 at 1:40 AM Evan Green wrote:
> >
> > Enable i2c-mux-gpio devices to be defined via ACPI. The idle-state
> > property translates directly to a fwnode_property_*() call. The child
> > reg property translates naturally into _ADR in ACPI.
> >
> > The i2c-parent binding is a relic from the days when the bindings
> > dictated that all direct children of an I2C controller had to be I2C
> > devices. These days that's no longer required. The i2c-mux can sit as a
> > direct child of its parent controller, which is where it makes the most
> > sense from a hardware description perspective. For the ACPI
> > implementation we'll assume that's always how the i2c-mux-gpio is
> > instantiated.
>
> ...
>
> > +#ifdef CONFIG_ACPI
> > +
> > +static int i2c_mux_gpio_get_acpi_adr(struct device *dev,
> > +struct fwnode_handle *fwdev,
> > +unsigned int *adr)
> > +
> > +{
> > + unsigned long long adr64;
> > + acpi_status status;
> > +
> > + status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwdev),
> > + METHOD_NAME__ADR,
> > + NULL, );
> > +
> > + if (!ACPI_SUCCESS(status)) {
> > + dev_err(dev, "Cannot get address\n");
> > + return -EINVAL;
> > + }
> > +
> > + *adr = adr64;
> > + if (*adr != adr64) {
> > + dev_err(dev, "Address out of range\n");
> > + return -ERANGE;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +#else
> > +
> > +static int i2c_mux_gpio_get_acpi_adr(struct device *dev,
> > +struct fwnode_handle *fwdev,
> > +unsigned int *adr)
> > +{
> > + return -EINVAL;
> > +}
> > +
> > +#endif
>
> I'm wondering if you may use acpi_find_child_device() here.
> Or is it a complementary function?
I think it's complementary. The code above is "I have a device, I want
its _ADR". whereas acpi_find_child_device() is "I have an _ADR, I want
its device". I could flip things around to use this, but it would turn
the code from linear into quadratic. I'd have to scan each possible
address and call acpi_find_child_device() with that _ADR to see if
there's a child device there.
>
> ...
>
> > + device_for_each_child_node(dev, child) {
> > + if (is_of_node(child)) {
> > + fwnode_property_read_u32(child, "reg", values + i);
> > +
> > + } else if (is_acpi_node(child)) {
> > + rc = i2c_mux_gpio_get_acpi_adr(dev, child, values +
> > i);
> > + if (rc)
> > + return rc;
> > + }
> > +
> > i++;
> > }
>
> And for this I already told in two different threads with similar code
> that perhaps we need common helper that will check reg followed by
> _ADR.
Oh, I'm not aware of those threads. I'd need some advice: I guess a
new fwnode_* API would make sense for this, but I had trouble coming
up with a generic interface. _ADR is just a blobbo 64 bit int, but
DT's "reg" is a little more flexible, having a length, and potentially
being an array. I suppose it would have to be something like:
int fwnode_property_read_reg(const struct fwnode_handle *fwnode,
size_t index, uint64_t *addr, uint64_t *len);
But then ACPI would always return 0 for length, and only index 0 would
ever work? I'm worried I'm designing an API that's only useful to me.
I tried to look around for other examples of this specific pattern of
_ADR then "reg", but struggled to turn up much.
-Evan
>
> --
> With Best Regards,
> Andy Shevchenko
[RESEND PATCH v3 2/2] i2c: i2c-mux-gpio: Enable this driver in ACPI land
Enable i2c-mux-gpio devices to be defined via ACPI. The idle-state
property translates directly to a fwnode_property_*() call. The child
reg property translates naturally into _ADR in ACPI.
The i2c-parent binding is a relic from the days when the bindings
dictated that all direct children of an I2C controller had to be I2C
devices. These days that's no longer required. The i2c-mux can sit as a
direct child of its parent controller, which is where it makes the most
sense from a hardware description perspective. For the ACPI
implementation we'll assume that's always how the i2c-mux-gpio is
instantiated.
Signed-off-by: Evan Green
---
Changes in v3:
- Update commit message again (Peter)
- Added missing \n (Peter)
- adr64 overflow check (Peter)
- Don't initialize child (Peter)
- Limit scope of dev_handle (Peter)
Changes in v2:
- Make it compile properly when !CONFIG_ACPI (Randy)
- Update commit message regarding i2c-parent (Peter)
drivers/i2c/muxes/i2c-mux-gpio.c | 107 +++
1 file changed, 79 insertions(+), 28 deletions(-)
diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c b/drivers/i2c/muxes/i2c-mux-gpio.c
index caaa782b50d83..bac415a52b780 100644
--- a/drivers/i2c/muxes/i2c-mux-gpio.c
+++ b/drivers/i2c/muxes/i2c-mux-gpio.c
@@ -49,35 +49,85 @@ static int i2c_mux_gpio_deselect(struct i2c_mux_core *muxc,
u32 chan)
return 0;
}
-#ifdef CONFIG_OF
-static int i2c_mux_gpio_probe_dt(struct gpiomux *mux,
- struct platform_device *pdev)
+#ifdef CONFIG_ACPI
+
+static int i2c_mux_gpio_get_acpi_adr(struct device *dev,
+struct fwnode_handle *fwdev,
+unsigned int *adr)
+
+{
+ unsigned long long adr64;
+ acpi_status status;
+
+ status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwdev),
+ METHOD_NAME__ADR,
+ NULL, );
+
+ if (!ACPI_SUCCESS(status)) {
+ dev_err(dev, "Cannot get address\n");
+ return -EINVAL;
+ }
+
+ *adr = adr64;
+ if (*adr != adr64) {
+ dev_err(dev, "Address out of range\n");
+ return -ERANGE;
+ }
+
+ return 0;
+}
+
+#else
+
+static int i2c_mux_gpio_get_acpi_adr(struct device *dev,
+struct fwnode_handle *fwdev,
+unsigned int *adr)
+{
+ return -EINVAL;
+}
+
+#endif
+
+static int i2c_mux_gpio_probe_fw(struct gpiomux *mux,
+struct platform_device *pdev)
{
struct device *dev = >dev;
- struct device_node *np = pdev->dev.of_node;
- struct device_node *adapter_np, *child;
- struct i2c_adapter *adapter;
+ struct device_node *np = dev->of_node;
+ struct device_node *adapter_np;
+ struct i2c_adapter *adapter = NULL;
+ struct fwnode_handle *child;
unsigned *values;
- int i = 0;
+ int rc, i = 0;
+
+ if (is_of_node(dev->fwnode)) {
+ if (!np)
+ return -ENODEV;
- if (!np)
- return -ENODEV;
+ adapter_np = of_parse_phandle(np, "i2c-parent", 0);
+ if (!adapter_np) {
+ dev_err(>dev, "Cannot parse i2c-parent\n");
+ return -ENODEV;
+ }
+ adapter = of_find_i2c_adapter_by_node(adapter_np);
+ of_node_put(adapter_np);
+
+ } else if (is_acpi_node(dev->fwnode)) {
+ /*
+* In ACPI land the mux should be a direct child of the i2c
+* bus it muxes.
+*/
+ acpi_handle dev_handle = ACPI_HANDLE(dev->parent);
- adapter_np = of_parse_phandle(np, "i2c-parent", 0);
- if (!adapter_np) {
- dev_err(>dev, "Cannot parse i2c-parent\n");
- return -ENODEV;
+ adapter = i2c_acpi_find_adapter_by_handle(dev_handle);
}
- adapter = of_find_i2c_adapter_by_node(adapter_np);
- of_node_put(adapter_np);
+
if (!adapter)
return -EPROBE_DEFER;
mux->data.parent = i2c_adapter_id(adapter);
put_device(>dev);
- mux->data.n_values = of_get_child_count(np);
-
+ mux->data.n_values = device_get_child_node_count(dev);
values = devm_kcalloc(dev,
mux->data.n_values, sizeof(*mux->data.values),
GFP_KERNEL);
@@ -86,24 +136,25 @@ static int i2c_mux_gpio_probe_dt(struct gpiomux *mux,
return -ENOMEM;
}
- for_each_child_of_node(np, child) {
- of_property_read_u32(child, "reg", values + i);
+ device_for_each_child_node(dev, child) {
+ if (is_of_nod
[RESEND PATCH v3 1/2] i2c: i2c-mux-gpio: Factor out pdev->dev in _probe_dt()
Factor out >dev into a local variable in preparation for
the ACPI enablement of this function, which will utilize the variable
more.
Signed-off-by: Evan Green
---
Changes in v3:
- Introduced minor >dev to dev refactor (Peter)
drivers/i2c/muxes/i2c-mux-gpio.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c b/drivers/i2c/muxes/i2c-mux-gpio.c
index 4effe563e9e8d..caaa782b50d83 100644
--- a/drivers/i2c/muxes/i2c-mux-gpio.c
+++ b/drivers/i2c/muxes/i2c-mux-gpio.c
@@ -53,6 +53,7 @@ static int i2c_mux_gpio_deselect(struct i2c_mux_core *muxc,
u32 chan)
static int i2c_mux_gpio_probe_dt(struct gpiomux *mux,
struct platform_device *pdev)
{
+ struct device *dev = >dev;
struct device_node *np = pdev->dev.of_node;
struct device_node *adapter_np, *child;
struct i2c_adapter *adapter;
@@ -77,11 +78,11 @@ static int i2c_mux_gpio_probe_dt(struct gpiomux *mux,
mux->data.n_values = of_get_child_count(np);
- values = devm_kcalloc(>dev,
+ values = devm_kcalloc(dev,
mux->data.n_values, sizeof(*mux->data.values),
GFP_KERNEL);
if (!values) {
- dev_err(>dev, "Cannot allocate values array");
+ dev_err(dev, "Cannot allocate values array");
return -ENOMEM;
}
--
2.26.2
[RESEND PATCH v3 0/2] i2c: i2c-mux-gpio: Enable this driver in ACPI land
The i2c-mux-gpio driver is a handy driver to have in your bag of tricks, but it currently only works with DT-based firmware. Enable this driver on ACPI platforms as well. The first patch is a little dinky. Peter, if it turns out you'd rather just take this all as a single patch, feel free to squash the first patch into the second. Or I can resend a squashed patch if needed. Changes in v3: - Introduced minor >dev to dev refactor (Peter) - Update commit message again (Peter) - Added missing \n (Peter) - adr64 overflow check (Peter) - Don't initialize child (Peter) - Limit scope of dev_handle (Peter) Changes in v2: - Make it compile properly when !CONFIG_ACPI (Randy) - Update commit message regarding i2c-parent (Peter) Evan Green (2): i2c: i2c-mux-gpio: Factor out pdev->dev in _probe_dt() i2c: i2c-mux-gpio: Enable this driver in ACPI land drivers/i2c/muxes/i2c-mux-gpio.c | 112 ++- 1 file changed, 82 insertions(+), 30 deletions(-) -- 2.26.2
[PATCH v2] pinctrl: intel: Fix Jasperlake HOSTSW_OWN offset
GPIOs that attempt to use interrupts get thwarted with a message like:
"pin 161 cannot be used as IRQ" (for instance with SD_CD). This is because
the HOSTSW_OWN offset is incorrect, so every GPIO looks like it's
owned by ACPI.
Fixes: e278dcb7048b1 ("pinctrl: intel: Add Intel Jasper Lake pin controller
support")
Cc: sta...@vger.kernel.org
Signed-off-by: Evan Green
---
Changes in v2:
- Commit text rewording [Andy]
drivers/pinctrl/intel/pinctrl-jasperlake.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/pinctrl/intel/pinctrl-jasperlake.c
b/drivers/pinctrl/intel/pinctrl-jasperlake.c
index 9bd0e8e6310c3..283698cf0dc7d 100644
--- a/drivers/pinctrl/intel/pinctrl-jasperlake.c
+++ b/drivers/pinctrl/intel/pinctrl-jasperlake.c
@@ -16,7 +16,7 @@
#define JSL_PAD_OWN0x020
#define JSL_PADCFGLOCK 0x080
-#define JSL_HOSTSW_OWN 0x0b0
+#define JSL_HOSTSW_OWN 0x0c0
#define JSL_GPI_IS 0x100
#define JSL_GPI_IE 0x120
--
2.26.2
Re: [PATCH] pinctrl: intel: Fix Jasperlake hostown offset
On Wed, Nov 11, 2020 at 2:13 PM Andy Shevchenko
wrote:
>
> On Tue, Nov 10, 2020 at 02:49:49PM -0800, Evan Green wrote:
> > GPIOs that attempt to use interrupts get thwarted with a message like:
> > "pin 161 cannot be used as IRQ" (for instance with SD_CD). This is because
> > the JSL_HOSTSW_OWN offset is incorrect, so every GPIO looks like it's
>
> Simply HOSTSW_OWN, and same spelling in the subject, please.
>
> > owned by ACPI.
>
>
> > Signed-off-by: Evan Green
>
> > Fixes: e278dcb7048b1 ("pinctrl: intel: Add Intel Jasper Lake pin
> > controller support")
>
> It must be one line, and put it first in the tag block
>
>
> > Cc: sta...@vger.kernel.org
>
> This is second one...
>
> Fixes: ...
> Cc: ...
> SoB: ...
Thanks, will fix these things, spinning now.
-Evan
Re: [PATCH] pinctrl: intel: Fix Jasperlake hostown offset
On Tue, Nov 10, 2020 at 3:48 PM Andy Shevchenko wrote: > > > > On Wednesday, November 11, 2020, Evan Green wrote: >> >> GPIOs that attempt to use interrupts get thwarted with a message like: >> "pin 161 cannot be used as IRQ" (for instance with SD_CD). This is because >> the JSL_HOSTSW_OWN offset is incorrect, so every GPIO looks like it's >> owned by ACPI. > > > Funny, I have created a similar patch few hours ago. Are you sure this is > enough? In mine I have also padcfglock updated. But I have to confirm that, > that’s why I didn’t send it out. Oh weird! I didn't check padcfglock since it didn't happen to be involved in the bug I was tracking down. I was trying to clean out some skeletons in my kernel closet [1] and debugged it down to this. If you want to smash the two patches together I'm fine with that. Let me know, and CC me if you do post something. -Evan [1] https://chromium.googlesource.com/chromiumos/overlays/board-overlays/+/master/overlay-dedede/sys-kernel/chromeos-kernel-5_4/files/0001-CHROMIUM-pinctrl-intel-Allow-pin-as-IRQ-even-in-ACPI.patch
[PATCH] pinctrl: intel: Fix Jasperlake hostown offset
GPIOs that attempt to use interrupts get thwarted with a message like:
"pin 161 cannot be used as IRQ" (for instance with SD_CD). This is because
the JSL_HOSTSW_OWN offset is incorrect, so every GPIO looks like it's
owned by ACPI.
Signed-off-by: Evan Green
Fixes: e278dcb7048b1 ("pinctrl: intel: Add Intel Jasper Lake pin
controller support")
Cc: sta...@vger.kernel.org
---
drivers/pinctrl/intel/pinctrl-jasperlake.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/pinctrl/intel/pinctrl-jasperlake.c
b/drivers/pinctrl/intel/pinctrl-jasperlake.c
index 9bd0e8e6310c3..283698cf0dc7d 100644
--- a/drivers/pinctrl/intel/pinctrl-jasperlake.c
+++ b/drivers/pinctrl/intel/pinctrl-jasperlake.c
@@ -16,7 +16,7 @@
#define JSL_PAD_OWN0x020
#define JSL_PADCFGLOCK 0x080
-#define JSL_HOSTSW_OWN 0x0b0
+#define JSL_HOSTSW_OWN 0x0c0
#define JSL_GPI_IS 0x100
#define JSL_GPI_IE 0x120
--
2.26.2
Re: [PATCH] arm64: dts: qcom: sc7180-trogdor: Fixup modem memory region
On Tue, Oct 20, 2020 at 11:37 AM Evan Green wrote: > > On Thu, Oct 15, 2020 at 11:28 AM Sibi Sankar wrote: > > > > The modem firmware memory requirements vary between 32M/140M on > > no-lte/lte skus respectively, so fixup the modem memory region > > to reflect the requirements. > > > > Signed-off-by: Sibi Sankar > > Reviewed-by: Evan Green Did this land anywhere?
Re: [PATCH v3 3/4] nvmem: core: Add support for keepout regions
On Thu, Oct 29, 2020 at 5:08 AM Srinivas Kandagatla wrote: > > Thanks Evan for doing this, > > On 29/10/2020 00:28, Evan Green wrote: > > Introduce support into the nvmem core for arrays of register ranges > > that should not result in actual device access. For these regions a > > constant byte (repeated) is returned instead on read, and writes are > > quietly ignored and returned as successful. > > > > This is useful for instance if certain efuse regions are protected > > from access by Linux because they contain secret info to another part > > of the system (like an integrated modem). > > > > Signed-off-by: Evan Green > > Overall the patch looks good for me. > I have applied just this patch for more testing in next! > > I can pick up 1/4 and 4/4 once Rob's Ack/Reviews the patch! Thank you, Srini! -Evan
[PATCH v3 1/4] dt-bindings: nvmem: Add soc qfprom compatible strings
Add SoC-specific compatible strings so that data can be attached
to it in the driver.
Signed-off-by: Evan Green
---
Changes in v3:
- Fixed example (Doug and rob-bot)
Changes in v2:
- Add other soc compatible strings (Doug)
- Fix compatible string definition (Doug)
.../devicetree/bindings/nvmem/qcom,qfprom.yaml | 17 ++---
1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml
b/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml
index 1a18b6bab35e7..992777c90a0bf 100644
--- a/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml
+++ b/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml
@@ -14,7 +14,18 @@ allOf:
properties:
compatible:
-const: qcom,qfprom
+items:
+ - enum:
+ - qcom,apq8064-qfprom
+ - qcom,apq8084-qfprom
+ - qcom,msm8974-qfprom
+ - qcom,msm8916-qfprom
+ - qcom,msm8996-qfprom
+ - qcom,msm8998-qfprom
+ - qcom,qcs404-qfprom
+ - qcom,sc7180-qfprom
+ - qcom,sdm845-qfprom
+ - const: qcom,qfprom
reg:
# If the QFPROM is read-only OS image then only the corrected region
@@ -60,7 +71,7 @@ examples:
#size-cells = <2>;
efuse@784000 {
-compatible = "qcom,qfprom";
+compatible = "qcom,sc7180-qfprom", "qcom,qfprom";
reg = <0 0x00784000 0 0x8ff>,
<0 0x0078 0 0x7a0>,
<0 0x00782000 0 0x100>,
@@ -85,7 +96,7 @@ examples:
#size-cells = <2>;
efuse@784000 {
-compatible = "qcom,qfprom";
+compatible = "qcom,sdm845-qfprom", "qcom,qfprom";
reg = <0 0x00784000 0 0x8ff>;
#address-cells = <1>;
#size-cells = <1>;
--
2.26.2
[PATCH v3 2/4] arm64: dts: qcom: sc7180: Add soc-specific qfprom compat string
Add the soc-specific compatible string so that it can be matched
more specifically now that the driver cares which SoC it's on.
Signed-off-by: Evan Green
Reviewed-by: Douglas Anderson
---
(no changes since v1)
arch/arm64/boot/dts/qcom/sc7180.dtsi | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi
b/arch/arm64/boot/dts/qcom/sc7180.dtsi
index 6fcffd588a7d6..f5ef2cb6e68c2 100644
--- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
@@ -660,7 +660,7 @@ gcc: clock-controller@10 {
};
qfprom: efuse@784000 {
- compatible = "qcom,qfprom";
+ compatible = "qcom,sc7180-qfprom", "qcom,qfprom";
reg = <0 0x00784000 0 0x8ff>,
<0 0x0078 0 0x7a0>,
<0 0x00782000 0 0x100>,
--
2.26.2
[PATCH v3 3/4] nvmem: core: Add support for keepout regions
Introduce support into the nvmem core for arrays of register ranges
that should not result in actual device access. For these regions a
constant byte (repeated) is returned instead on read, and writes are
quietly ignored and returned as successful.
This is useful for instance if certain efuse regions are protected
from access by Linux because they contain secret info to another part
of the system (like an integrated modem).
Signed-off-by: Evan Green
---
Changes in v3:
- Use min()/max() macros instead of defining my own (Doug)
- Comment changes to indicate sorting (Doug)
- Add function to validate keepouts are proper (Doug)
Changes in v2:
- Introduced keepout regions into the core (Srini)
drivers/nvmem/core.c | 153 -
include/linux/nvmem-provider.h | 17
2 files changed, 166 insertions(+), 4 deletions(-)
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index a09ff8409f600..177f5bf27c6d5 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -34,6 +34,8 @@ struct nvmem_device {
struct bin_attributeeeprom;
struct device *base_dev;
struct list_headcells;
+ const struct nvmem_keepout *keepout;
+ unsigned intnkeepout;
nvmem_reg_read_treg_read;
nvmem_reg_write_t reg_write;
struct gpio_desc*wp_gpio;
@@ -66,8 +68,8 @@ static LIST_HEAD(nvmem_lookup_list);
static BLOCKING_NOTIFIER_HEAD(nvmem_notifier);
-static int nvmem_reg_read(struct nvmem_device *nvmem, unsigned int offset,
- void *val, size_t bytes)
+static int __nvmem_reg_read(struct nvmem_device *nvmem, unsigned int offset,
+ void *val, size_t bytes)
{
if (nvmem->reg_read)
return nvmem->reg_read(nvmem->priv, offset, val, bytes);
@@ -75,8 +77,8 @@ static int nvmem_reg_read(struct nvmem_device *nvmem,
unsigned int offset,
return -EINVAL;
}
-static int nvmem_reg_write(struct nvmem_device *nvmem, unsigned int offset,
- void *val, size_t bytes)
+static int __nvmem_reg_write(struct nvmem_device *nvmem, unsigned int offset,
+void *val, size_t bytes)
{
int ret;
@@ -90,6 +92,88 @@ static int nvmem_reg_write(struct nvmem_device *nvmem,
unsigned int offset,
return -EINVAL;
}
+static int nvmem_access_with_keepouts(struct nvmem_device *nvmem,
+ unsigned int offset, void *val,
+ size_t bytes, int write)
+{
+
+ unsigned int end = offset + bytes;
+ unsigned int kend, ksize;
+ const struct nvmem_keepout *keepout = nvmem->keepout;
+ const struct nvmem_keepout *keepoutend = keepout + nvmem->nkeepout;
+ int rc;
+
+ /*
+* Skip all keepouts before the range being accessed.
+* Keepouts are sorted.
+*/
+ while ((keepout < keepoutend) && (keepout->end <= offset))
+ keepout++;
+
+ while ((offset < end) && (keepout < keepoutend)) {
+ /* Access the valid portion before the keepout. */
+ if (offset < keepout->start) {
+ kend = min(end, keepout->start);
+ ksize = kend - offset;
+ if (write)
+ rc = __nvmem_reg_write(nvmem, offset, val,
ksize);
+ else
+ rc = __nvmem_reg_read(nvmem, offset, val,
ksize);
+
+ if (rc)
+ return rc;
+
+ offset += ksize;
+ val += ksize;
+ }
+
+ /*
+* Now we're aligned to the start of this keepout zone. Go
+* through it.
+*/
+ kend = min(end, keepout->end);
+ ksize = kend - offset;
+ if (!write)
+ memset(val, keepout->value, ksize);
+
+ val += ksize;
+ offset += ksize;
+ keepout++;
+ }
+
+ /*
+* If we ran out of keepouts but there's still stuff to do, send it
+* down directly
+*/
+ if (offset < end) {
+ ksize = end - offset;
+ if (write)
+ return __nvmem_reg_write(nvmem, offset, val, ksize);
+ else
+ return __nvmem_reg_read(nvmem, offset, val, ksize);
+ }
+
+ return 0;
+}
+
+static int nvmem_reg_read(struct nvmem_device *nvmem, unsigned int offset,
+ void *val, size_t bytes)
+{
+ if (!nvmem->nkeepout)
+ return __nvmem_reg_read(nvmem, offset, val, bytes);
+
+ return nvmem_access_with_keepouts(nvmem, offset, val, bytes, false);
+}
+
+static
[PATCH v3 4/4] nvmem: qfprom: Don't touch certain fuses
Some fuse ranges are protected by the XPU such that the AP cannot
access them. Attempting to do so causes an SError. Use the newly
introduced per-soc compatible string, and the newly introduced
nvmem keepout support to attach the set of regions
we should not access.
Signed-off-by: Evan Green
Reviewed-by: Douglas Anderson
---
(no changes since v2)
Changes in v2:
- Use new core support in qfprom (Srini)
drivers/nvmem/qfprom.c | 30 ++
1 file changed, 30 insertions(+)
diff --git a/drivers/nvmem/qfprom.c b/drivers/nvmem/qfprom.c
index 5e9e60e2e591d..6cace24dfbf73 100644
--- a/drivers/nvmem/qfprom.c
+++ b/drivers/nvmem/qfprom.c
@@ -12,6 +12,7 @@
#include
#include
#include
+#include
#include
/* Blow timer clock frequency in Mhz */
@@ -88,6 +89,28 @@ struct qfprom_touched_values {
u32 timer_val;
};
+/**
+ * struct qfprom_soc_compatible_data - Data matched against the SoC
+ * compatible string.
+ *
+ * @keepout: Array of keepout regions for this SoC.
+ * @nkeepout: Number of elements in the keepout array.
+ */
+struct qfprom_soc_compatible_data {
+ const struct nvmem_keepout *keepout;
+ unsigned int nkeepout;
+};
+
+static const struct nvmem_keepout sc7180_qfprom_keepout[] = {
+ {.start = 0x128, .end = 0x148},
+ {.start = 0x220, .end = 0x228}
+};
+
+static const struct qfprom_soc_compatible_data sc7180_qfprom = {
+ .keepout = sc7180_qfprom_keepout,
+ .nkeepout = ARRAY_SIZE(sc7180_qfprom_keepout)
+};
+
/**
* qfprom_disable_fuse_blowing() - Undo enabling of fuse blowing.
* @priv: Our driver data.
@@ -281,6 +304,7 @@ static int qfprom_probe(struct platform_device *pdev)
struct device *dev = >dev;
struct resource *res;
struct nvmem_device *nvmem;
+ const struct qfprom_soc_compatible_data *soc_data;
struct qfprom_priv *priv;
int ret;
@@ -299,6 +323,11 @@ static int qfprom_probe(struct platform_device *pdev)
econfig.priv = priv;
priv->dev = dev;
+ soc_data = device_get_match_data(dev);
+ if (soc_data) {
+ econfig.keepout = soc_data->keepout;
+ econfig.nkeepout = soc_data->nkeepout;
+ }
/*
* If more than one region is provided then the OS has the ability
@@ -354,6 +383,7 @@ static int qfprom_probe(struct platform_device *pdev)
static const struct of_device_id qfprom_of_match[] = {
{ .compatible = "qcom,qfprom",},
+ { .compatible = "qcom,sc7180-qfprom", .data = _qfprom},
{/* sentinel */},
};
MODULE_DEVICE_TABLE(of, qfprom_of_match);
--
2.26.2
[PATCH v3 0/4] nvmem: qfprom: Avoid untouchable regions
Certain fuses are protected by the XPU such that the AP cannot access them. Attempting to do so causes an SError. Introduce an SoC-specific compatible string, and introduce support into the nvmem core to avoid accessing specified regions. Then use those new elements in the qfprom driver to avoid SErrors when usermode accesses certain registers. Changes in v3: - Fixed example (Doug and rob-bot) - Use min()/max() macros instead of defining my own (Doug) - Comment changes to indicate sorting (Doug) - Add function to validate keepouts are proper (Doug) Changes in v2: - Add other soc compatible strings (Doug) - Fix compatible string definition (Doug) - Introduced keepout regions into the core (Srini) - Use new core support in qfprom (Srini) Evan Green (4): dt-bindings: nvmem: Add soc qfprom compatible strings arm64: dts: qcom: sc7180: Add soc-specific qfprom compat string nvmem: core: Add support for keepout regions nvmem: qfprom: Don't touch certain fuses .../bindings/nvmem/qcom,qfprom.yaml | 17 +- arch/arm64/boot/dts/qcom/sc7180.dtsi | 2 +- drivers/nvmem/core.c | 153 +- drivers/nvmem/qfprom.c| 30 include/linux/nvmem-provider.h| 17 ++ 5 files changed, 211 insertions(+), 8 deletions(-) -- 2.26.2
Re: [PATCH v2 1/4] dt-bindings: nvmem: Add soc qfprom compatible strings
On Wed, Oct 21, 2020 at 2:41 PM Doug Anderson wrote: > > Hi, > > On Fri, Oct 16, 2020 at 12:27 PM Evan Green wrote: > > > > Add SoC-specific compatible strings so that data can be attached > > to it in the driver. > > > > Signed-off-by: Evan Green > > --- > > > > Changes in v2: > > - Add other soc compatible strings (Doug) > > - Fix compatible string definition (Doug) > > > > .../devicetree/bindings/nvmem/qcom,qfprom.yaml | 13 - > > 1 file changed, 12 insertions(+), 1 deletion(-) > > > > diff --git a/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml > > b/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml > > index 1a18b6bab35e7..eb1440045aff1 100644 > > --- a/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml > > +++ b/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml > > @@ -14,7 +14,18 @@ allOf: > > > > properties: > >compatible: > > -const: qcom,qfprom > > +items: > > + - enum: > > + - qcom,apq8064-qfprom > > + - qcom,apq8084-qfprom > > + - qcom,msm8974-qfprom > > + - qcom,msm8916-qfprom > > + - qcom,msm8996-qfprom > > + - qcom,msm8998-qfprom > > + - qcom,qcs404-qfprom > > + - qcom,sc7180-qfprom > > + - qcom,sdm845-qfprom > > + - const: qcom,qfprom > > > >reg: > > # If the QFPROM is read-only OS image then only the corrected region > > As Rob's bot found, your example no longer matches your requirements. > It needs an SoC-specific string plus the "qcom,qfprom". It's always > good to try running "make dt_binding_check" to catch these sorts of > things. Thanks Doug, will do for the next spin! -Evan
Re: [PATCH v2 3/4] nvmem: core: Add support for keepout regions
On Wed, Oct 21, 2020 at 2:41 PM Doug Anderson wrote:
>
> Hi,
>
> On Fri, Oct 16, 2020 at 12:27 PM Evan Green wrote:
> >
> > Introduce support into the nvmem core for arrays of register ranges
> > that should not result in actual device access. For these regions a
> > constant byte (repeated) is returned instead on read, and writes are
> > quietly ignored and returned as successful.
> >
> > This is useful for instance if certain efuse regions are protected
> > from access by Linux because they contain secret info to another part
> > of the system (like an integrated modem).
> >
> > Signed-off-by: Evan Green
> > ---
> >
> > Changes in v2:
> > - Introduced keepout regions into the core (Srini)
> >
> > drivers/nvmem/core.c | 95 --
> > include/linux/nvmem-provider.h | 17 ++
> > 2 files changed, 108 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> > index a09ff8409f600..f7819c57f8828 100644
> > --- a/drivers/nvmem/core.c
> > +++ b/drivers/nvmem/core.c
> > @@ -19,6 +19,9 @@
> > #include
> > #include
> >
> > +#define MAX(a, b) ((a) > (b) ? (a) : (b))
> > +#define MIN(a, b) ((a) < (b) ? (a) : (b))
>
> Why not use min() / max() macros from include/linux/kernel.h?
>
Done
>
> > +static int nvmem_access_with_keepouts(struct nvmem_device *nvmem,
> > + unsigned int offset, void *val,
> > + size_t bytes, int write)
> > +{
> > +
> > + unsigned int end = offset + bytes;
> > + unsigned int kend, ksize;
> > + const struct nvmem_keepout *keepout = nvmem->keepout;
> > + const struct nvmem_keepout *keepoutend = keepout + nvmem->nkeepout;
> > + int rc;
> > +
> > + /* Skip everything before the range being accessed */
>
> nit: "Skip everything" => "Skip all keepouts"
>
> ...might not hurt to remind here that keepouts are sorted?
Done
>
>
> > + while ((keepout < keepoutend) && (keepout->end <= offset))
> > + keepout++;
> > +
> > + while ((offset < end) && (keepout < keepoutend)) {
> > +
>
> nit: remove blank line?
Done
>
>
> > @@ -647,6 +732,8 @@ struct nvmem_device *nvmem_register(const struct
> > nvmem_config *config)
> > nvmem->type = config->type;
> > nvmem->reg_read = config->reg_read;
> > nvmem->reg_write = config->reg_write;
> > + nvmem->keepout = config->keepout;
> > + nvmem->nkeepout = config->nkeepout;
>
> It seems like it might be worth adding something to validate that the
> ranges are sorted and return an error if they're not.
>
> Maybe worth validating (and documenting) that the keepouts won't cause
> us to violate "stride" and/or "word_size" ?
Done
>
>
> Everything above is just nits and other than them this looks like a
> nice change. BTW: this is the kind of thing that screams for unit
> testing, though that might be a bit too much of a yak to shave here?
It would be cool, but I'll leave that for another time. Thanks for the
review, Doug!
>
> -Doug
Re: [PATCH v3 2/2] i2c: i2c-mux-gpio: Enable this driver in ACPI land
On Mon, Oct 19, 2020 at 9:53 AM Evan Green wrote: > > On Sun, Oct 18, 2020 at 11:58 AM Andy Shevchenko > wrote: > > > > On Sat, Oct 17, 2020 at 8:30 AM Evan Green wrote: > > > > > > Enable i2c-mux-gpio devices to be defined via ACPI. The idle-state > > > property translates directly to a fwnode_property_*() call. The child > > > reg property translates naturally into _ADR in ACPI. > > > > > > The i2c-parent binding is a relic from the days when the bindings > > > dictated that all direct children of an I2C controller had to be I2C > > > devices. These days that's no longer required. The i2c-mux can sit as a > > > direct child of its parent controller, which is where it makes the most > > > sense from a hardware description perspective. For the ACPI > > > implementation we'll assume that's always how the i2c-mux-gpio is > > > instantiated. > > > > Can you tell me if the following is relevant to what you are looking for? > > https://elixir.bootlin.com/linux/latest/source/drivers/i2c/i2c-mux.c#L393 > > I don't think so, but let me know if I'm reading between the lines > incorrectly. > > The code you pointed to links the newly-minted fake i2c controller > back together with its ACPI node. This is important, since I think > that's how child I2C devices underneath the fake busses get populated > in ACPI land. But the paragraph above is discussing how to identify > the parent adapter (ie the real hardware) for an i2c-mux-gpio device. > > In DT-land, the i2c-mux-gpio floats at the top of the tree directly > under /, and then uses a phandle to point to where transactions should > be forwarded. I'm told the reason for this is historical limitations > with the DT bindings. Rather than trying to translate the phandle over > 1:1 into ACPI-land, I'm asserting that the mux device should live > underneath the adapter it wants to forward traffic to. Andy or Peter, Any other thoughts on this series? -Evan
Re: [PATCH] arm64: dts: qcom: sc7180-trogdor: Fixup modem memory region
On Thu, Oct 15, 2020 at 11:28 AM Sibi Sankar wrote: > > The modem firmware memory requirements vary between 32M/140M on > no-lte/lte skus respectively, so fixup the modem memory region > to reflect the requirements. > > Signed-off-by: Sibi Sankar Reviewed-by: Evan Green
Re: [PATCH v3 2/2] i2c: i2c-mux-gpio: Enable this driver in ACPI land
On Sun, Oct 18, 2020 at 11:58 AM Andy Shevchenko wrote: > > On Sat, Oct 17, 2020 at 8:30 AM Evan Green wrote: > > > > Enable i2c-mux-gpio devices to be defined via ACPI. The idle-state > > property translates directly to a fwnode_property_*() call. The child > > reg property translates naturally into _ADR in ACPI. > > > > The i2c-parent binding is a relic from the days when the bindings > > dictated that all direct children of an I2C controller had to be I2C > > devices. These days that's no longer required. The i2c-mux can sit as a > > direct child of its parent controller, which is where it makes the most > > sense from a hardware description perspective. For the ACPI > > implementation we'll assume that's always how the i2c-mux-gpio is > > instantiated. > > Can you tell me if the following is relevant to what you are looking for? > https://elixir.bootlin.com/linux/latest/source/drivers/i2c/i2c-mux.c#L393 I don't think so, but let me know if I'm reading between the lines incorrectly. The code you pointed to links the newly-minted fake i2c controller back together with its ACPI node. This is important, since I think that's how child I2C devices underneath the fake busses get populated in ACPI land. But the paragraph above is discussing how to identify the parent adapter (ie the real hardware) for an i2c-mux-gpio device. In DT-land, the i2c-mux-gpio floats at the top of the tree directly under /, and then uses a phandle to point to where transactions should be forwarded. I'm told the reason for this is historical limitations with the DT bindings. Rather than trying to translate the phandle over 1:1 into ACPI-land, I'm asserting that the mux device should live underneath the adapter it wants to forward traffic to. -Evan > > -- > With Best Regards, > Andy Shevchenko
[PATCH v3 0/2] i2c: i2c-mux-gpio: Enable this driver in ACPI land
The i2c-mux-gpio driver is a handy driver to have in your bag of tricks, but it currently only works with DT-based firmware. Enable this driver on ACPI platforms as well. The first patch is a little dinky. Peter, if it turns out you'd rather just take this all as a single patch, feel free to squash the first patch into the second. Or I can resend a squashed patch if needed. Changes in v3: - Introduced minor >dev to dev refactor (Peter) - Update commit message again (Peter) - Added missing \n (Peter) - adr64 overflow check (Peter) - Don't initialize child (Peter) - Limit scope of dev_handle (Peter) Changes in v2: - Make it compile properly when !CONFIG_ACPI (Randy) - Update commit message regarding i2c-parent (Peter) Evan Green (2): i2c: i2c-mux-gpio: Factor out pdev->dev in _probe_dt() i2c: i2c-mux-gpio: Enable this driver in ACPI land drivers/i2c/muxes/i2c-mux-gpio.c | 112 ++- 1 file changed, 82 insertions(+), 30 deletions(-) -- 2.26.2
[PATCH v3 1/2] i2c: i2c-mux-gpio: Factor out pdev->dev in _probe_dt()
Factor out >dev into a local variable in preparation for
the ACPI enablement of this function, which will utilize the variable
more.
Signed-off-by: Evan Green
---
Changes in v3:
- Introduced minor >dev to dev refactor (Peter)
drivers/i2c/muxes/i2c-mux-gpio.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c b/drivers/i2c/muxes/i2c-mux-gpio.c
index 4effe563e9e8d..caaa782b50d83 100644
--- a/drivers/i2c/muxes/i2c-mux-gpio.c
+++ b/drivers/i2c/muxes/i2c-mux-gpio.c
@@ -53,6 +53,7 @@ static int i2c_mux_gpio_deselect(struct i2c_mux_core *muxc,
u32 chan)
static int i2c_mux_gpio_probe_dt(struct gpiomux *mux,
struct platform_device *pdev)
{
+ struct device *dev = >dev;
struct device_node *np = pdev->dev.of_node;
struct device_node *adapter_np, *child;
struct i2c_adapter *adapter;
@@ -77,11 +78,11 @@ static int i2c_mux_gpio_probe_dt(struct gpiomux *mux,
mux->data.n_values = of_get_child_count(np);
- values = devm_kcalloc(>dev,
+ values = devm_kcalloc(dev,
mux->data.n_values, sizeof(*mux->data.values),
GFP_KERNEL);
if (!values) {
- dev_err(>dev, "Cannot allocate values array");
+ dev_err(dev, "Cannot allocate values array");
return -ENOMEM;
}
--
2.26.2
[PATCH v3 2/2] i2c: i2c-mux-gpio: Enable this driver in ACPI land
Enable i2c-mux-gpio devices to be defined via ACPI. The idle-state
property translates directly to a fwnode_property_*() call. The child
reg property translates naturally into _ADR in ACPI.
The i2c-parent binding is a relic from the days when the bindings
dictated that all direct children of an I2C controller had to be I2C
devices. These days that's no longer required. The i2c-mux can sit as a
direct child of its parent controller, which is where it makes the most
sense from a hardware description perspective. For the ACPI
implementation we'll assume that's always how the i2c-mux-gpio is
instantiated.
Signed-off-by: Evan Green
---
Changes in v3:
- Update commit message again (Peter)
- Added missing \n (Peter)
- adr64 overflow check (Peter)
- Don't initialize child (Peter)
- Limit scope of dev_handle (Peter)
Changes in v2:
- Make it compile properly when !CONFIG_ACPI (Randy)
- Update commit message regarding i2c-parent (Peter)
drivers/i2c/muxes/i2c-mux-gpio.c | 107 +++
1 file changed, 79 insertions(+), 28 deletions(-)
diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c b/drivers/i2c/muxes/i2c-mux-gpio.c
index caaa782b50d83..bac415a52b780 100644
--- a/drivers/i2c/muxes/i2c-mux-gpio.c
+++ b/drivers/i2c/muxes/i2c-mux-gpio.c
@@ -49,35 +49,85 @@ static int i2c_mux_gpio_deselect(struct i2c_mux_core *muxc,
u32 chan)
return 0;
}
-#ifdef CONFIG_OF
-static int i2c_mux_gpio_probe_dt(struct gpiomux *mux,
- struct platform_device *pdev)
+#ifdef CONFIG_ACPI
+
+static int i2c_mux_gpio_get_acpi_adr(struct device *dev,
+struct fwnode_handle *fwdev,
+unsigned int *adr)
+
+{
+ unsigned long long adr64;
+ acpi_status status;
+
+ status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwdev),
+ METHOD_NAME__ADR,
+ NULL, );
+
+ if (!ACPI_SUCCESS(status)) {
+ dev_err(dev, "Cannot get address\n");
+ return -EINVAL;
+ }
+
+ *adr = adr64;
+ if (*adr != adr64) {
+ dev_err(dev, "Address out of range\n");
+ return -ERANGE;
+ }
+
+ return 0;
+}
+
+#else
+
+static int i2c_mux_gpio_get_acpi_adr(struct device *dev,
+struct fwnode_handle *fwdev,
+unsigned int *adr)
+{
+ return -EINVAL;
+}
+
+#endif
+
+static int i2c_mux_gpio_probe_fw(struct gpiomux *mux,
+struct platform_device *pdev)
{
struct device *dev = >dev;
- struct device_node *np = pdev->dev.of_node;
- struct device_node *adapter_np, *child;
- struct i2c_adapter *adapter;
+ struct device_node *np = dev->of_node;
+ struct device_node *adapter_np;
+ struct i2c_adapter *adapter = NULL;
+ struct fwnode_handle *child;
unsigned *values;
- int i = 0;
+ int rc, i = 0;
+
+ if (is_of_node(dev->fwnode)) {
+ if (!np)
+ return -ENODEV;
- if (!np)
- return -ENODEV;
+ adapter_np = of_parse_phandle(np, "i2c-parent", 0);
+ if (!adapter_np) {
+ dev_err(>dev, "Cannot parse i2c-parent\n");
+ return -ENODEV;
+ }
+ adapter = of_find_i2c_adapter_by_node(adapter_np);
+ of_node_put(adapter_np);
+
+ } else if (is_acpi_node(dev->fwnode)) {
+ /*
+* In ACPI land the mux should be a direct child of the i2c
+* bus it muxes.
+*/
+ acpi_handle dev_handle = ACPI_HANDLE(dev->parent);
- adapter_np = of_parse_phandle(np, "i2c-parent", 0);
- if (!adapter_np) {
- dev_err(>dev, "Cannot parse i2c-parent\n");
- return -ENODEV;
+ adapter = i2c_acpi_find_adapter_by_handle(dev_handle);
}
- adapter = of_find_i2c_adapter_by_node(adapter_np);
- of_node_put(adapter_np);
+
if (!adapter)
return -EPROBE_DEFER;
mux->data.parent = i2c_adapter_id(adapter);
put_device(>dev);
- mux->data.n_values = of_get_child_count(np);
-
+ mux->data.n_values = device_get_child_node_count(dev);
values = devm_kcalloc(dev,
mux->data.n_values, sizeof(*mux->data.values),
GFP_KERNEL);
@@ -86,24 +136,25 @@ static int i2c_mux_gpio_probe_dt(struct gpiomux *mux,
return -ENOMEM;
}
- for_each_child_of_node(np, child) {
- of_property_read_u32(child, "reg", values + i);
+ device_for_each_child_node(dev, child) {
+ if (is_of_nod
[PATCH v2 4/4] nvmem: qfprom: Don't touch certain fuses
Some fuse ranges are protected by the XPU such that the AP cannot
access them. Attempting to do so causes an SError. Use the newly
introduced per-soc compatible string, and the newly introduced
nvmem keepout support to attach the set of regions
we should not access.
Signed-off-by: Evan Green
---
Changes in v2:
- Use new core support in qfprom (Srini)
drivers/nvmem/qfprom.c | 30 ++
1 file changed, 30 insertions(+)
diff --git a/drivers/nvmem/qfprom.c b/drivers/nvmem/qfprom.c
index 5e9e60e2e591d..6cace24dfbf73 100644
--- a/drivers/nvmem/qfprom.c
+++ b/drivers/nvmem/qfprom.c
@@ -12,6 +12,7 @@
#include
#include
#include
+#include
#include
/* Blow timer clock frequency in Mhz */
@@ -88,6 +89,28 @@ struct qfprom_touched_values {
u32 timer_val;
};
+/**
+ * struct qfprom_soc_compatible_data - Data matched against the SoC
+ * compatible string.
+ *
+ * @keepout: Array of keepout regions for this SoC.
+ * @nkeepout: Number of elements in the keepout array.
+ */
+struct qfprom_soc_compatible_data {
+ const struct nvmem_keepout *keepout;
+ unsigned int nkeepout;
+};
+
+static const struct nvmem_keepout sc7180_qfprom_keepout[] = {
+ {.start = 0x128, .end = 0x148},
+ {.start = 0x220, .end = 0x228}
+};
+
+static const struct qfprom_soc_compatible_data sc7180_qfprom = {
+ .keepout = sc7180_qfprom_keepout,
+ .nkeepout = ARRAY_SIZE(sc7180_qfprom_keepout)
+};
+
/**
* qfprom_disable_fuse_blowing() - Undo enabling of fuse blowing.
* @priv: Our driver data.
@@ -281,6 +304,7 @@ static int qfprom_probe(struct platform_device *pdev)
struct device *dev = >dev;
struct resource *res;
struct nvmem_device *nvmem;
+ const struct qfprom_soc_compatible_data *soc_data;
struct qfprom_priv *priv;
int ret;
@@ -299,6 +323,11 @@ static int qfprom_probe(struct platform_device *pdev)
econfig.priv = priv;
priv->dev = dev;
+ soc_data = device_get_match_data(dev);
+ if (soc_data) {
+ econfig.keepout = soc_data->keepout;
+ econfig.nkeepout = soc_data->nkeepout;
+ }
/*
* If more than one region is provided then the OS has the ability
@@ -354,6 +383,7 @@ static int qfprom_probe(struct platform_device *pdev)
static const struct of_device_id qfprom_of_match[] = {
{ .compatible = "qcom,qfprom",},
+ { .compatible = "qcom,sc7180-qfprom", .data = _qfprom},
{/* sentinel */},
};
MODULE_DEVICE_TABLE(of, qfprom_of_match);
--
2.26.2
[PATCH v2 2/4] arm64: dts: qcom: sc7180: Add soc-specific qfprom compat string
Add the soc-specific compatible string so that it can be matched
more specifically now that the driver cares which SoC it's on.
Signed-off-by: Evan Green
Reviewed-by: Douglas Anderson
---
(no changes since v1)
arch/arm64/boot/dts/qcom/sc7180.dtsi | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi
b/arch/arm64/boot/dts/qcom/sc7180.dtsi
index 6678f1e8e3958..f1f8bbc0b37bc 100644
--- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
@@ -660,7 +660,7 @@ gcc: clock-controller@10 {
};
qfprom: efuse@784000 {
- compatible = "qcom,qfprom";
+ compatible = "qcom,sc7180-qfprom", "qcom,qfprom";
reg = <0 0x00784000 0 0x8ff>,
<0 0x0078 0 0x7a0>,
<0 0x00782000 0 0x100>,
--
2.26.2
[PATCH v2 3/4] nvmem: core: Add support for keepout regions
Introduce support into the nvmem core for arrays of register ranges
that should not result in actual device access. For these regions a
constant byte (repeated) is returned instead on read, and writes are
quietly ignored and returned as successful.
This is useful for instance if certain efuse regions are protected
from access by Linux because they contain secret info to another part
of the system (like an integrated modem).
Signed-off-by: Evan Green
---
Changes in v2:
- Introduced keepout regions into the core (Srini)
drivers/nvmem/core.c | 95 --
include/linux/nvmem-provider.h | 17 ++
2 files changed, 108 insertions(+), 4 deletions(-)
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index a09ff8409f600..f7819c57f8828 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -19,6 +19,9 @@
#include
#include
+#define MAX(a, b) ((a) > (b) ? (a) : (b))
+#define MIN(a, b) ((a) < (b) ? (a) : (b))
+
struct nvmem_device {
struct module *owner;
struct device dev;
@@ -34,6 +37,8 @@ struct nvmem_device {
struct bin_attributeeeprom;
struct device *base_dev;
struct list_headcells;
+ const struct nvmem_keepout *keepout;
+ unsigned intnkeepout;
nvmem_reg_read_treg_read;
nvmem_reg_write_t reg_write;
struct gpio_desc*wp_gpio;
@@ -66,8 +71,8 @@ static LIST_HEAD(nvmem_lookup_list);
static BLOCKING_NOTIFIER_HEAD(nvmem_notifier);
-static int nvmem_reg_read(struct nvmem_device *nvmem, unsigned int offset,
- void *val, size_t bytes)
+static int __nvmem_reg_read(struct nvmem_device *nvmem, unsigned int offset,
+ void *val, size_t bytes)
{
if (nvmem->reg_read)
return nvmem->reg_read(nvmem->priv, offset, val, bytes);
@@ -75,8 +80,8 @@ static int nvmem_reg_read(struct nvmem_device *nvmem,
unsigned int offset,
return -EINVAL;
}
-static int nvmem_reg_write(struct nvmem_device *nvmem, unsigned int offset,
- void *val, size_t bytes)
+static int __nvmem_reg_write(struct nvmem_device *nvmem, unsigned int offset,
+void *val, size_t bytes)
{
int ret;
@@ -90,6 +95,86 @@ static int nvmem_reg_write(struct nvmem_device *nvmem,
unsigned int offset,
return -EINVAL;
}
+static int nvmem_access_with_keepouts(struct nvmem_device *nvmem,
+ unsigned int offset, void *val,
+ size_t bytes, int write)
+{
+
+ unsigned int end = offset + bytes;
+ unsigned int kend, ksize;
+ const struct nvmem_keepout *keepout = nvmem->keepout;
+ const struct nvmem_keepout *keepoutend = keepout + nvmem->nkeepout;
+ int rc;
+
+ /* Skip everything before the range being accessed */
+ while ((keepout < keepoutend) && (keepout->end <= offset))
+ keepout++;
+
+ while ((offset < end) && (keepout < keepoutend)) {
+
+ /* Access the valid portion before the keepout. */
+ if (offset < keepout->start) {
+ kend = MIN(end, keepout->start);
+ ksize = kend - offset;
+ if (write)
+ rc = __nvmem_reg_write(nvmem, offset, val,
ksize);
+ else
+ rc = __nvmem_reg_read(nvmem, offset, val,
ksize);
+
+ if (rc)
+ return rc;
+
+ offset += ksize;
+ val += ksize;
+ }
+
+ /*
+* Now we're aligned to the start of this keepout zone. Go
+* through it.
+*/
+ kend = MIN(end, keepout->end);
+ ksize = kend - offset;
+ if (!write)
+ memset(val, keepout->value, ksize);
+
+ val += ksize;
+ offset += ksize;
+ keepout++;
+ }
+
+ /*
+* If we ran out of keepouts but there's still stuff to do, send it
+* down directly
+*/
+ if (offset < end) {
+ ksize = end - offset;
+ if (write)
+ return __nvmem_reg_write(nvmem, offset, val, ksize);
+ else
+ return __nvmem_reg_read(nvmem, offset, val, ksize);
+ }
+
+ return 0;
+}
+
+static int nvmem_reg_read(struct nvmem_device *nvmem, unsigned int offset,
+ void *val, size_t bytes)
+{
+ if (!nvmem->nkeepout)
+ return __nvmem_reg_read(nvmem, offset, val, bytes);
+
+ return nvmem_access_with_keepouts(nvmem, offset, val, bytes, false);
+}
+
+s
[PATCH v2 1/4] dt-bindings: nvmem: Add soc qfprom compatible strings
Add SoC-specific compatible strings so that data can be attached to it in the driver. Signed-off-by: Evan Green --- Changes in v2: - Add other soc compatible strings (Doug) - Fix compatible string definition (Doug) .../devicetree/bindings/nvmem/qcom,qfprom.yaml | 13 - 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml b/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml index 1a18b6bab35e7..eb1440045aff1 100644 --- a/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml +++ b/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml @@ -14,7 +14,18 @@ allOf: properties: compatible: -const: qcom,qfprom +items: + - enum: + - qcom,apq8064-qfprom + - qcom,apq8084-qfprom + - qcom,msm8974-qfprom + - qcom,msm8916-qfprom + - qcom,msm8996-qfprom + - qcom,msm8998-qfprom + - qcom,qcs404-qfprom + - qcom,sc7180-qfprom + - qcom,sdm845-qfprom + - const: qcom,qfprom reg: # If the QFPROM is read-only OS image then only the corrected region -- 2.26.2
[PATCH v2 0/4] nvmem: qfprom: Avoid untouchable regions
Certain fuses are protected by the XPU such that the AP cannot access them. Attempting to do so causes an SError. Introduce an SoC-specific compatible string, and introduce support into the nvmem core to avoid accessing specified regions. Then use those new elements in the qfprom driver to avoid SErrors when usermode accesses certain registers. Changes in v2: - Add other soc compatible strings (Doug) - Fix compatible string definition (Doug) - Introduced keepout regions into the core (Srini) - Use new core support in qfprom (Srini) Evan Green (4): dt-bindings: nvmem: Add soc qfprom compatible strings arm64: dts: qcom: sc7180: Add soc-specific qfprom compat string nvmem: core: Add support for keepout regions nvmem: qfprom: Don't touch certain fuses .../bindings/nvmem/qcom,qfprom.yaml | 13 ++- arch/arm64/boot/dts/qcom/sc7180.dtsi | 2 +- drivers/nvmem/core.c | 95 ++- drivers/nvmem/qfprom.c| 30 ++ include/linux/nvmem-provider.h| 17 5 files changed, 151 insertions(+), 6 deletions(-) -- 2.26.2
Re: [PATCH v2] i2c: i2c-mux-gpio: Enable this driver in ACPI land
On Thu, Oct 15, 2020 at 3:23 AM Peter Rosin wrote:
>
> Hi!
>
> On 2020-10-15 03:02, Evan Green wrote:
> > Enable i2c-mux-gpio devices to be defined via ACPI. The idle-state
> > property translates directly to a fwnode_property_*() call. The child
> > reg property translates naturally into _ADR in ACPI.
> >
> > The i2c-parent binding is a relic from the days when all direct children
> > of an i2c controller in Linux had to be i2c devices. These days that
>
> I2C controller. I2C devices.
>
> I fail to see why this "relic" has to be explicitly blamed on Linux? In the
> beginning, the bindings for all I2C controllers (sometimes implicitely,
> sometimes explicitely) specified that all child nodes had to be I2C devices.
> The *bindings* were simply not as flexible before the i2c-bus subnode was
> invented only a few years ago. So, there are arguments that the "problem"
> was in DT-land and that Linux just followed suit.
Gotcha, that makes sense. I was probably reading between the lines
incorrectly in your previous reply. I'll blame it on the bindings :)
>
> > implementation detail has been worked out, so the i2c-mux can sit
> > as a direct child of its parent controller, which is where it makes the
> > most sense from a hardware description perspective. For the ACPI
> > implementation we'll assume that's always how the i2c-mux-gpio is
> > instantiated.
>
> There is potential to match this and make i2c-parent optional for the
> DT case and require it to be a child of its parent in such cases, if
> someone has the time/energy...
I won't plan to since I don't have a device like this, but yeah I
agree this would be a fine convention for DT to start following as
well.
>
> >
> > Signed-off-by: Evan Green
> > ---
> >
> > Changes in v2:
> > - Make it compile properly when !CONFIG_ACPI (Randy)
> > - Update commit message regarding i2c-parent (Peter)
> >
> > drivers/i2c/muxes/i2c-mux-gpio.c | 103 ++-
> > 1 file changed, 75 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c
> > b/drivers/i2c/muxes/i2c-mux-gpio.c
> > index 4effe563e9e8d..8e4008f4a9b5d 100644
> > --- a/drivers/i2c/muxes/i2c-mux-gpio.c
> > +++ b/drivers/i2c/muxes/i2c-mux-gpio.c
> > @@ -49,34 +49,80 @@ static int i2c_mux_gpio_deselect(struct i2c_mux_core
> > *muxc, u32 chan)
> > return 0;
> > }
> >
> > -#ifdef CONFIG_OF
> > -static int i2c_mux_gpio_probe_dt(struct gpiomux *mux,
> > - struct platform_device *pdev)
> > +#ifdef CONFIG_ACPI
> > +
> > +static int i2c_mux_gpio_get_acpi_adr(struct device *dev,
> > + struct fwnode_handle *fwdev,
> > + unsigned int *adr)
> > +
> > +{
> > + unsigned long long adr64;
> > + acpi_status status;
> > +
> > + status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwdev),
> > +METHOD_NAME__ADR,
> > +NULL, );
> > +
> > + if (!ACPI_SUCCESS(status)) {
> > + dev_err(dev, "Cannot get address");
>
> Missing trailing \n
Whoops.
>
> > + return -EINVAL;
> > + }
> > +
> > + *adr = adr64;
>
> Maybe this is too pedantic? Optional, ignore if I'm just being insane...
>
> if (*adr != adr64) {
> dev_err(dev, "Address out of range\n");
> return -EINVAL;
> }
>
Sure, will add.
> > + return 0;
> > +}
> > +
> > +#else
> > +
> > +static int i2c_mux_gpio_get_acpi_adr(struct device *dev,
> > + struct fwnode_handle *fwdev,
> > + unsigned int *adr)
> > +{
> > + return -EINVAL;
> > +}
> > +
> > +#endif
> > +
> > +static int i2c_mux_gpio_probe_fw(struct gpiomux *mux,
> > + struct platform_device *pdev)
> > {
> > - struct device_node *np = pdev->dev.of_node;
> > - struct device_node *adapter_np, *child;
> > - struct i2c_adapter *adapter;
> > + struct device *dev = >dev;
> > + struct device_node *np = dev->of_node;
> > + acpi_handle dev_handle;
>
> Remove the dev_handle declaration here...(push)...
>
> > + struct device_node *adapter_np;
> > + struct i2c_adapter *adapter = NULL;
> > + struct fwnode_handle *child = NULL;
>
> Why do you
[PATCH v2] i2c: i2c-mux-gpio: Enable this driver in ACPI land
Enable i2c-mux-gpio devices to be defined via ACPI. The idle-state
property translates directly to a fwnode_property_*() call. The child
reg property translates naturally into _ADR in ACPI.
The i2c-parent binding is a relic from the days when all direct children
of an i2c controller in Linux had to be i2c devices. These days that
implementation detail has been worked out, so the i2c-mux can sit
as a direct child of its parent controller, which is where it makes the
most sense from a hardware description perspective. For the ACPI
implementation we'll assume that's always how the i2c-mux-gpio is
instantiated.
Signed-off-by: Evan Green
---
Changes in v2:
- Make it compile properly when !CONFIG_ACPI (Randy)
- Update commit message regarding i2c-parent (Peter)
drivers/i2c/muxes/i2c-mux-gpio.c | 103 ++-
1 file changed, 75 insertions(+), 28 deletions(-)
diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c b/drivers/i2c/muxes/i2c-mux-gpio.c
index 4effe563e9e8d..8e4008f4a9b5d 100644
--- a/drivers/i2c/muxes/i2c-mux-gpio.c
+++ b/drivers/i2c/muxes/i2c-mux-gpio.c
@@ -49,34 +49,80 @@ static int i2c_mux_gpio_deselect(struct i2c_mux_core *muxc,
u32 chan)
return 0;
}
-#ifdef CONFIG_OF
-static int i2c_mux_gpio_probe_dt(struct gpiomux *mux,
- struct platform_device *pdev)
+#ifdef CONFIG_ACPI
+
+static int i2c_mux_gpio_get_acpi_adr(struct device *dev,
+struct fwnode_handle *fwdev,
+unsigned int *adr)
+
+{
+ unsigned long long adr64;
+ acpi_status status;
+
+ status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwdev),
+ METHOD_NAME__ADR,
+ NULL, );
+
+ if (!ACPI_SUCCESS(status)) {
+ dev_err(dev, "Cannot get address");
+ return -EINVAL;
+ }
+
+ *adr = adr64;
+ return 0;
+}
+
+#else
+
+static int i2c_mux_gpio_get_acpi_adr(struct device *dev,
+struct fwnode_handle *fwdev,
+unsigned int *adr)
+{
+ return -EINVAL;
+}
+
+#endif
+
+static int i2c_mux_gpio_probe_fw(struct gpiomux *mux,
+struct platform_device *pdev)
{
- struct device_node *np = pdev->dev.of_node;
- struct device_node *adapter_np, *child;
- struct i2c_adapter *adapter;
+ struct device *dev = >dev;
+ struct device_node *np = dev->of_node;
+ acpi_handle dev_handle;
+ struct device_node *adapter_np;
+ struct i2c_adapter *adapter = NULL;
+ struct fwnode_handle *child = NULL;
unsigned *values;
- int i = 0;
+ int rc, i = 0;
- if (!np)
- return -ENODEV;
+ if (is_of_node(dev->fwnode)) {
+ if (!np)
+ return -ENODEV;
- adapter_np = of_parse_phandle(np, "i2c-parent", 0);
- if (!adapter_np) {
- dev_err(>dev, "Cannot parse i2c-parent\n");
- return -ENODEV;
+ adapter_np = of_parse_phandle(np, "i2c-parent", 0);
+ if (!adapter_np) {
+ dev_err(>dev, "Cannot parse i2c-parent\n");
+ return -ENODEV;
+ }
+ adapter = of_find_i2c_adapter_by_node(adapter_np);
+ of_node_put(adapter_np);
+
+ } else if (is_acpi_node(dev->fwnode)) {
+ /*
+* In ACPI land the mux should be a direct child of the i2c
+* bus it muxes.
+*/
+ dev_handle = ACPI_HANDLE(dev->parent);
+ adapter = i2c_acpi_find_adapter_by_handle(dev_handle);
}
- adapter = of_find_i2c_adapter_by_node(adapter_np);
- of_node_put(adapter_np);
+
if (!adapter)
return -EPROBE_DEFER;
mux->data.parent = i2c_adapter_id(adapter);
put_device(>dev);
- mux->data.n_values = of_get_child_count(np);
-
+ mux->data.n_values = device_get_child_node_count(dev);
values = devm_kcalloc(>dev,
mux->data.n_values, sizeof(*mux->data.values),
GFP_KERNEL);
@@ -85,24 +131,25 @@ static int i2c_mux_gpio_probe_dt(struct gpiomux *mux,
return -ENOMEM;
}
- for_each_child_of_node(np, child) {
- of_property_read_u32(child, "reg", values + i);
+ device_for_each_child_node(dev, child) {
+ if (is_of_node(child)) {
+ fwnode_property_read_u32(child, "reg", values + i);
+
+ } else if (is_acpi_node(child)) {
+ rc = i2c_mux_gpio_get_acpi_adr(dev, child, values + i);
+ if (rc)
+
Re: [PATCH] i2c: i2c-mux-gpio: Enable this driver in ACPI land
On Sat, Oct 10, 2020 at 10:03 AM Peter Rosin wrote:
>
> Hi!
>
> On 2020-10-10 00:43, Evan Green wrote:
> > Enable i2c-mux-gpio devices to be defined via ACPI. The idle-state
> > property translates directly to a fwnode_property_*() call. The child
> > reg property translates naturally into _ADR in ACPI.
> >
> > The i2c-parent is a little trickier, since of's phandle definition
> > suggests the i2c mux could live in a completely different part of
> > the tree than its upstream i2c controller. For now in ACPI,
>
> This is so since the I2C gpio-mux predates the "i2c-bus" sub-node of
> I2C controllers. At that time *all* sub-nodes of I2C controllers
> represented I2C client device, IIUC. With that knowledge, you could
> perhaps rephrase the above?
Ah I see, so this part of the binding was defined to work around
implementation details of Linux. But now those details are worked out,
so porting that part to ACPI isn't necessary. I'll rephrase to that
effect.
>
> Also, a nit below.
>
> > just assume that the i2c-mux-gpio device will always be a direct
> > child of the i2c controller. If the additional flexibility of
> > defining muxes in wildly different locations from their parent
> > controllers is required, it can be added later.
> >
> > Signed-off-by: Evan Green
> > ---
> >
> > drivers/i2c/muxes/i2c-mux-gpio.c | 77 +---
> > 1 file changed, 50 insertions(+), 27 deletions(-)
> >
> > diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c
> > b/drivers/i2c/muxes/i2c-mux-gpio.c
> > index 4effe563e9e8d..f195e95e8a037 100644
> > --- a/drivers/i2c/muxes/i2c-mux-gpio.c
> > +++ b/drivers/i2c/muxes/i2c-mux-gpio.c
> > @@ -49,34 +49,46 @@ static int i2c_mux_gpio_deselect(struct i2c_mux_core
> > *muxc, u32 chan)
> > return 0;
> > }
> >
> > -#ifdef CONFIG_OF
> > -static int i2c_mux_gpio_probe_dt(struct gpiomux *mux,
> > - struct platform_device *pdev)
> > +static int i2c_mux_gpio_probe_fw(struct gpiomux *mux,
> > + struct platform_device *pdev)
> > {
> > - struct device_node *np = pdev->dev.of_node;
> > - struct device_node *adapter_np, *child;
> > - struct i2c_adapter *adapter;
> > + struct device *dev = >dev;
> > + struct device_node *np = dev->of_node;
> > + acpi_handle dev_handle;
> > + struct device_node *adapter_np;
> > + struct i2c_adapter *adapter = NULL;
> > + struct fwnode_handle *child = NULL;
> > unsigned *values;
> > int i = 0;
> >
> > - if (!np)
> > - return -ENODEV;
> > + if (is_of_node(dev->fwnode)) {
> > + if (!np)
> > + return -ENODEV;
> >
> > - adapter_np = of_parse_phandle(np, "i2c-parent", 0);
> > - if (!adapter_np) {
> > - dev_err(>dev, "Cannot parse i2c-parent\n");
> > - return -ENODEV;
> > + adapter_np = of_parse_phandle(np, "i2c-parent", 0);
> > + if (!adapter_np) {
> > + dev_err(>dev, "Cannot parse i2c-parent\n");
> > + return -ENODEV;
> > + }
> > + adapter = of_find_i2c_adapter_by_node(adapter_np);
> > + of_node_put(adapter_np);
> > +
> > + } else if (is_acpi_node(dev->fwnode)) {
> > + /*
> > + * In ACPI land the mux should be a direct child of the i2c
> > + * bus it muxes.
> > + */
> > + dev_handle = ACPI_HANDLE(dev->parent);
> > + adapter = i2c_acpi_find_adapter_by_handle(dev_handle);
> > }
> > - adapter = of_find_i2c_adapter_by_node(adapter_np);
> > - of_node_put(adapter_np);
> > +
> > if (!adapter)
> > return -EPROBE_DEFER;
> >
> > mux->data.parent = i2c_adapter_id(adapter);
> > put_device(>dev);
> >
> > - mux->data.n_values = of_get_child_count(np);
> > -
> > + mux->data.n_values = device_get_child_node_count(dev);
> > values = devm_kcalloc(>dev,
> > mux->data.n_values, sizeof(*mux->data.values),
> > GFP_KERNEL);
> > @@ -85,24 +97,35 @@ static int i2c_mux_gpio_probe_dt(struct gpiomux *mux,
> > return -ENOMEM;
> > }
> >
> > - for_each_child_of_node(np, chi
[PATCH] i2c: i2c-mux-gpio: Enable this driver in ACPI land
Enable i2c-mux-gpio devices to be defined via ACPI. The idle-state
property translates directly to a fwnode_property_*() call. The child
reg property translates naturally into _ADR in ACPI.
The i2c-parent is a little trickier, since of's phandle definition
suggests the i2c mux could live in a completely different part of
the tree than its upstream i2c controller. For now in ACPI,
just assume that the i2c-mux-gpio device will always be a direct
child of the i2c controller. If the additional flexibility of
defining muxes in wildly different locations from their parent
controllers is required, it can be added later.
Signed-off-by: Evan Green
---
drivers/i2c/muxes/i2c-mux-gpio.c | 77 +---
1 file changed, 50 insertions(+), 27 deletions(-)
diff --git a/drivers/i2c/muxes/i2c-mux-gpio.c b/drivers/i2c/muxes/i2c-mux-gpio.c
index 4effe563e9e8d..f195e95e8a037 100644
--- a/drivers/i2c/muxes/i2c-mux-gpio.c
+++ b/drivers/i2c/muxes/i2c-mux-gpio.c
@@ -49,34 +49,46 @@ static int i2c_mux_gpio_deselect(struct i2c_mux_core *muxc,
u32 chan)
return 0;
}
-#ifdef CONFIG_OF
-static int i2c_mux_gpio_probe_dt(struct gpiomux *mux,
- struct platform_device *pdev)
+static int i2c_mux_gpio_probe_fw(struct gpiomux *mux,
+struct platform_device *pdev)
{
- struct device_node *np = pdev->dev.of_node;
- struct device_node *adapter_np, *child;
- struct i2c_adapter *adapter;
+ struct device *dev = >dev;
+ struct device_node *np = dev->of_node;
+ acpi_handle dev_handle;
+ struct device_node *adapter_np;
+ struct i2c_adapter *adapter = NULL;
+ struct fwnode_handle *child = NULL;
unsigned *values;
int i = 0;
- if (!np)
- return -ENODEV;
+ if (is_of_node(dev->fwnode)) {
+ if (!np)
+ return -ENODEV;
- adapter_np = of_parse_phandle(np, "i2c-parent", 0);
- if (!adapter_np) {
- dev_err(>dev, "Cannot parse i2c-parent\n");
- return -ENODEV;
+ adapter_np = of_parse_phandle(np, "i2c-parent", 0);
+ if (!adapter_np) {
+ dev_err(>dev, "Cannot parse i2c-parent\n");
+ return -ENODEV;
+ }
+ adapter = of_find_i2c_adapter_by_node(adapter_np);
+ of_node_put(adapter_np);
+
+ } else if (is_acpi_node(dev->fwnode)) {
+ /*
+* In ACPI land the mux should be a direct child of the i2c
+* bus it muxes.
+*/
+ dev_handle = ACPI_HANDLE(dev->parent);
+ adapter = i2c_acpi_find_adapter_by_handle(dev_handle);
}
- adapter = of_find_i2c_adapter_by_node(adapter_np);
- of_node_put(adapter_np);
+
if (!adapter)
return -EPROBE_DEFER;
mux->data.parent = i2c_adapter_id(adapter);
put_device(>dev);
- mux->data.n_values = of_get_child_count(np);
-
+ mux->data.n_values = device_get_child_node_count(dev);
values = devm_kcalloc(>dev,
mux->data.n_values, sizeof(*mux->data.values),
GFP_KERNEL);
@@ -85,24 +97,35 @@ static int i2c_mux_gpio_probe_dt(struct gpiomux *mux,
return -ENOMEM;
}
- for_each_child_of_node(np, child) {
- of_property_read_u32(child, "reg", values + i);
+ device_for_each_child_node(dev, child) {
+ if (is_of_node(child)) {
+ fwnode_property_read_u32(child, "reg", values + i);
+
+ } else if (is_acpi_node(child)) {
+ unsigned long long adr;
+ acpi_status status;
+
+ status =
acpi_evaluate_integer(ACPI_HANDLE_FWNODE(child),
+ METHOD_NAME__ADR,
+ NULL, );
+ if (ACPI_SUCCESS(status)) {
+ *(values + i) = adr;
+
+ } else {
+ dev_err(dev, "Cannot get address");
+ return -EINVAL;
+ }
+ }
+
i++;
}
mux->data.values = values;
- if (of_property_read_u32(np, "idle-state", >data.idle))
+ if (fwnode_property_read_u32(dev->fwnode, "idle-state",
>data.idle))
mux->data.idle = I2C_MUX_GPIO_NO_IDLE;
return 0;
}
-#else
-static int i2c_mux_gpio_probe_dt(struct gpiomux *mux,
- struct platform_device *pdev)
-{
- return 0;
-}
-#endif
static int i2c_mux_gpio
Re: [PATCH 1/3] dt-bindings: nvmem: Add qcom,sc7180-qfprom compatible string
On Fri, Oct 2, 2020 at 3:20 PM Doug Anderson wrote: > > Hi, > > On Tue, Sep 29, 2020 at 1:58 PM Evan Green wrote: > > > > Add an SoC-specific compatible string so that data can be attached > > to it in the driver. > > > > Signed-off-by: Evan Green > > --- > > > > Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml > > b/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml > > index 59aca6d22ff9b..b16c8e6a8c23d 100644 > > --- a/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml > > +++ b/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml > > @@ -14,7 +14,9 @@ allOf: > > > > properties: > >compatible: > > -const: qcom,qfprom > > +enum: > > + - qcom,qfprom > > + - qcom,sc7180-qfprom > > You don't want either/or. You want both. At the time Srinivas didn't > see the point of having the SoC-specific compatible string here, but > now that we have a reason for it maybe he'll be convinced? IMO you > essentially want: > > items: > - enum: > - qcom,apq8064-qfprom > - qcom,apq8084-qfprom > - qcom,msm8974-qfprom > - qcom,msm8916-qfprom > - qcom,msm8996-qfprom > - qcom,msm8998-qfprom > - qcom,qcs404-qfprom > - qcom,sc7180-qfprom > - qcom,sdm845-qfprom > - const: qcom,qfprom > > For some context: > <https://lore.kernel.org/r/CAD=FV=WjvAWVmq3fTh=_f2p1dv+sxg1rv-cqzr8krghe8_w...@mail.gmail.com/> That makes sense, thanks Doug. Srini, do you want me to go fix up all the various device trees to add the soc-compatible string, or just sc7180? (Also, don't forget about my other question about whether you still want the keepout stuff in the core at the cost of added complexity). -Evan > > -Doug > > > > > >reg: > > # If the QFPROM is read-only OS image then only the corrected region > > -- > > 2.26.2 > >
Re: [PATCH 3/3] nvmem: qfprom: Don't touch certain fuses
On Thu, Oct 1, 2020 at 9:30 AM Srinivas Kandagatla wrote: > > > > On 01/10/2020 17:27, Evan Green wrote: > > On Thu, Oct 1, 2020 at 7:17 AM Srinivas Kandagatla > > wrote: > >> > >> Hi Evan, > >> > >> On 29/09/2020 21:58, Evan Green wrote: > >>> Some fuse ranges are protected by the XPU such that the AP cannot > >>> access them. Attempting to do so causes an SError. Use the newly > >>> introduced per-soc compatible string to attach the set of regions > >>> we should not access. Then tiptoe around those regions. > >>> > >> > >> This is a generic feature that can be used by any nvmem provider, can > >> you move this logic to nvmem core instead of having it in qfprom! > > > > Sure! I'd prefer to keep this data in the driver for now rather than > Ofcourse these can come from driver directly based on compatible! > > > trying to define DT bindings for the keepout zones. So then I'll pass > > in my keepout array via struct nvmem_config at registration time, and > > then the core can handle the keepout logic instead of qfprom.c. > > > > Yes, that is inline with what am thinking of as well! Oh no, I realized this isn't nearly as beautiful when I try to move it into the core. The low level read/write interface between the nvmem core and the driver is a range. So to move this into the core I'd have to implement all the overlap computation logic to potentially break up a read into several small reads in cases where there are many little keepout ranges. It was much simpler when I could just check each byte offset individually, and because I was doing it in this one rarely-used driver I could make that performance tradeoff without much penalty. I could do all range/overlap handling if you want, but it'll be a bigger change, and I worry my driver would be the only one to end up using it. What do you think? -Evan > > > 00srini > > -Evan > >
Re: [PATCH 3/3] nvmem: qfprom: Don't touch certain fuses
On Thu, Oct 1, 2020 at 7:17 AM Srinivas Kandagatla wrote: > > Hi Evan, > > On 29/09/2020 21:58, Evan Green wrote: > > Some fuse ranges are protected by the XPU such that the AP cannot > > access them. Attempting to do so causes an SError. Use the newly > > introduced per-soc compatible string to attach the set of regions > > we should not access. Then tiptoe around those regions. > > > > This is a generic feature that can be used by any nvmem provider, can > you move this logic to nvmem core instead of having it in qfprom! Sure! I'd prefer to keep this data in the driver for now rather than trying to define DT bindings for the keepout zones. So then I'll pass in my keepout array via struct nvmem_config at registration time, and then the core can handle the keepout logic instead of qfprom.c. -Evan
[PATCH 3/3] nvmem: qfprom: Don't touch certain fuses
Some fuse ranges are protected by the XPU such that the AP cannot
access them. Attempting to do so causes an SError. Use the newly
introduced per-soc compatible string to attach the set of regions
we should not access. Then tiptoe around those regions.
Signed-off-by: Evan Green
---
drivers/nvmem/qfprom.c | 55 +++---
1 file changed, 51 insertions(+), 4 deletions(-)
diff --git a/drivers/nvmem/qfprom.c b/drivers/nvmem/qfprom.c
index 5e9e60e2e591d..feea39ae52164 100644
--- a/drivers/nvmem/qfprom.c
+++ b/drivers/nvmem/qfprom.c
@@ -12,6 +12,7 @@
#include
#include
#include
+#include
#include
/* Blow timer clock frequency in Mhz */
@@ -51,6 +52,17 @@ struct qfprom_soc_data {
u32 qfprom_blow_set_freq;
};
+/**
+ * struct qfprom_keepout_region - registers to avoid touching.
+ *
+ * @start: The first byte offset to avoid.
+ * @end: One after the last byte offset to avoid.
+ */
+struct qfprom_keepout_region {
+ u32 start;
+ u32 end;
+};
+
/**
* struct qfprom_priv - structure holding qfprom attributes
*
@@ -63,6 +75,7 @@ struct qfprom_soc_data {
* @secclk: Clock supply.
* @vcc: Regulator supply.
* @soc_data: Data that for things that varies from SoC to SoC.
+ * @keepout: Fuse regions not to access, as they may cause SErrors.
*/
struct qfprom_priv {
void __iomem *qfpraw;
@@ -73,6 +86,7 @@ struct qfprom_priv {
struct clk *secclk;
struct regulator *vcc;
const struct qfprom_soc_data *soc_data;
+ const struct qfprom_keepout_region *keepout;
};
/**
@@ -88,6 +102,12 @@ struct qfprom_touched_values {
u32 timer_val;
};
+const struct qfprom_keepout_region sc7180_qfprom[] = {
+ {.start = 0x128, .end = 0x148},
+ {.start = 0x220, .end = 0x228},
+ {} /* Sentinal where start == end. */
+};
+
/**
* qfprom_disable_fuse_blowing() - Undo enabling of fuse blowing.
* @priv: Our driver data.
@@ -171,6 +191,23 @@ static int qfprom_enable_fuse_blowing(const struct
qfprom_priv *priv,
return ret;
}
+static int qfprom_check_reg(struct qfprom_priv *priv, unsigned int reg)
+{
+ const struct qfprom_keepout_region *keepout = priv->keepout;
+
+ if (!keepout)
+ return 1;
+
+ while (keepout->start != keepout->end) {
+ if ((reg >= keepout->start) && (reg < keepout->end))
+ return 0;
+
+ keepout++;
+ }
+
+ return 1;
+}
+
/**
* qfprom_efuse_reg_write() - Write to fuses.
* @context: Our driver data.
@@ -228,8 +265,10 @@ static int qfprom_reg_write(void *context, unsigned int
reg, void *_val,
goto exit_enabled_fuse_blowing;
}
- for (i = 0; i < words; i++)
- writel(value[i], priv->qfpraw + reg + (i * 4));
+ for (i = 0; i < words; i++) {
+ if (qfprom_check_reg(priv, reg + (i * 4)))
+ writel(value[i], priv->qfpraw + reg + (i * 4));
+ }
ret = readl_relaxed_poll_timeout(
priv->qfpconf + QFPROM_BLOW_STATUS_OFFSET,
@@ -257,8 +296,14 @@ static int qfprom_reg_read(void *context,
if (read_raw_data && priv->qfpraw)
base = priv->qfpraw;
- while (words--)
- *val++ = readb(base + reg + i++);
+ while (words--) {
+ if (qfprom_check_reg(priv, reg + i))
+ *val++ = readb(base + reg + i);
+ else
+ *val++ = 0;
+
+ i++;
+ }
return 0;
}
@@ -299,6 +344,7 @@ static int qfprom_probe(struct platform_device *pdev)
econfig.priv = priv;
priv->dev = dev;
+ priv->keepout = device_get_match_data(dev);
/*
* If more than one region is provided then the OS has the ability
@@ -354,6 +400,7 @@ static int qfprom_probe(struct platform_device *pdev)
static const struct of_device_id qfprom_of_match[] = {
{ .compatible = "qcom,qfprom",},
+ { .compatible = "qcom,sc7180-qfprom", .data = _qfprom},
{/* sentinel */},
};
MODULE_DEVICE_TABLE(of, qfprom_of_match);
--
2.26.2
[PATCH 0/3] nvmem: qfprom: Avoid untouchable regions
Certain fuses are protected by the XPU such that the AP cannot access them. Attempting to do so causes an SError. Introduce an SoC-specific compatible string, and then use that to determine which fuse regions to present as Read as Zero / Write Ignore. Evan Green (3): dt-bindings: nvmem: Add qcom,sc7180-qfprom compatible string arm64: dts: qcom: sc7180: Add soc-specific qfprom compat string nvmem: qfprom: Don't touch certain fuses .../bindings/nvmem/qcom,qfprom.yaml | 4 +- arch/arm64/boot/dts/qcom/sc7180.dtsi | 2 +- drivers/nvmem/qfprom.c| 55 +-- 3 files changed, 55 insertions(+), 6 deletions(-) -- 2.26.2
[PATCH 2/3] arm64: dts: qcom: sc7180: Add soc-specific qfprom compat string
Add the soc-specific compatible string so that it can be matched
more specifically now that the driver cares which SoC it's on.
Signed-off-by: Evan Green
---
arch/arm64/boot/dts/qcom/sc7180.dtsi | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi
b/arch/arm64/boot/dts/qcom/sc7180.dtsi
index 6678f1e8e3958..f1f8bbc0b37bc 100644
--- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
@@ -660,7 +660,7 @@ gcc: clock-controller@10 {
};
qfprom: efuse@784000 {
- compatible = "qcom,qfprom";
+ compatible = "qcom,sc7180-qfprom", "qcom,qfprom";
reg = <0 0x00784000 0 0x8ff>,
<0 0x0078 0 0x7a0>,
<0 0x00782000 0 0x100>,
--
2.26.2
[PATCH 1/3] dt-bindings: nvmem: Add qcom,sc7180-qfprom compatible string
Add an SoC-specific compatible string so that data can be attached to it in the driver. Signed-off-by: Evan Green --- Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml b/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml index 59aca6d22ff9b..b16c8e6a8c23d 100644 --- a/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml +++ b/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml @@ -14,7 +14,9 @@ allOf: properties: compatible: -const: qcom,qfprom +enum: + - qcom,qfprom + - qcom,sc7180-qfprom reg: # If the QFPROM is read-only OS image then only the corrected region -- 2.26.2
[RESEND PATCH] soc: qcom: smp2p: Safely acquire spinlock without IRQs
smp2p_update_bits() should disable interrupts when it acquires its
spinlock. This is important because without the _irqsave, a priority
inversion can occur.
This function is called both with interrupts enabled in
qcom_q6v5_request_stop(), and with interrupts disabled in
ipa_smp2p_panic_notifier(). IRQ handling of spinlocks should be
consistent to avoid the panic notifier deadlocking because it's
sitting on the thread that's already got the lock via _request_stop().
Found via lockdep.
Fixes: 50e99641413e7 ("soc: qcom: smp2p: Qualcomm Shared Memory Point to Point")
Signed-off-by: Evan Green
---
drivers/soc/qcom/smp2p.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/soc/qcom/smp2p.c b/drivers/soc/qcom/smp2p.c
index 07183d731d747..a9709aae54abb 100644
--- a/drivers/soc/qcom/smp2p.c
+++ b/drivers/soc/qcom/smp2p.c
@@ -318,15 +318,16 @@ static int qcom_smp2p_inbound_entry(struct qcom_smp2p
*smp2p,
static int smp2p_update_bits(void *data, u32 mask, u32 value)
{
struct smp2p_entry *entry = data;
+ unsigned long flags;
u32 orig;
u32 val;
- spin_lock(>lock);
+ spin_lock_irqsave(>lock, flags);
val = orig = readl(entry->value);
val &= ~mask;
val |= value;
writel(val, entry->value);
- spin_unlock(>lock);
+ spin_unlock_irqrestore(>lock, flags);
if (val != orig)
qcom_smp2p_kick(entry->smp2p);
--
2.26.2
Re: [PATCH] interconnect: qcom: osm-l3: Mark more structures const
On Mon, Sep 14, 2020 at 11:21 AM Stephen Boyd wrote: > > These structures aren't modified at runtime. Mark them const so they get > moved to read-only memory. We have to cast away const in one place when > we store into the data member of struct icc_node. This is paired with a > re-const of the data member when it is extracted in qcom_icc_set(). > > Cc: Sibi Sankar > Cc: Evan Green > Signed-off-by: Stephen Boyd Reviewed-by: Evan Green
Re: [PATCH] interconnect: Show bandwidth for disabled paths as zero in debugfs
On Wed, Jul 29, 2020 at 10:50 AM Matthias Kaehlcke wrote: > > For disabled paths the 'interconnect_summary' in debugfs currently shows > the orginally requested bandwidths. This is confusing, since the bandwidth > requests aren't active. Instead show the bandwidths for disabled > paths/requests as zero. > > Signed-off-by: Matthias Kaehlcke Looks good to me. I briefly mulled over the idea of showing the disabled flag as a separate column, but I can't really think of how that would be useful. Reviewed-by: Evan Green
Re: [PATCH] x86/hotplug: Silence APIC only after all irq's are migrated
On Mon, Aug 17, 2020 at 11:33 AM Raj, Ashok wrote:
>
> Hi Evan
>
> Some details below,
>
> On Mon, Aug 17, 2020 at 11:12:17AM -0700, Evan Green wrote:
> > Hi Ashok,
> > Thank you, Srikanth, and Sukumar for some very impressive debugging here.
> >
> > On Fri, Aug 14, 2020 at 2:38 PM Ashok Raj wrote:
> > >
> > > When offlining CPU's, fixup_irqs() migrates all interrupts away from the
> > > outgoing CPU to an online CPU. Its always possible the device sent an
> > > interrupt to the previous CPU destination. Pending interrupt bit in IRR in
> > > lapic identifies such interrupts. apic_soft_disable() will not capture any
> > > new interrupts in IRR. This causes interrupts from device to be lost
> > > during
> > > cpu offline. The issue was found when explicitly setting MSI affinity to a
> > > CPU and immediately offlining it. It was simple to recreate with a USB
> > > ethernet device and doing I/O to it while the CPU is offlined. Lost
> > > interrupts happen even when Interrupt Remapping is enabled.
> > >
> > > Current code does apic_soft_disable() before migrating interrupts.
> > >
> > > native_cpu_disable()
> > > {
> > > ...
> > > apic_soft_disable();
> > > cpu_disable_common();
> > > --> fixup_irqs(); // Too late to capture anything in IRR.
> > > }
> > >
> > > Just fliping the above call sequence seems to hit the IRR checks
> > > and the lost interrupt is fixed for both legacy MSI and when
> > > interrupt remapping is enabled.
> > >
> > >
> > > Fixes: 60dcaad5736f ("x86/hotplug: Silence APIC and NMI when CPU is dead")
> > > Link: https://lore.kernel.org/lkml/875zdarr4h@nanos.tec.linutronix.de/
> > > Signed-off-by: Ashok Raj
> > >
> > > To: linux-kernel@vger.kernel.org
> > > To: Thomas Gleixner
> > > Cc: Sukumar Ghorai
> > > Cc: Srikanth Nandamuri
> > > Cc: Evan Green
> > > Cc: Mathias Nyman
> > > Cc: Bjorn Helgaas
> > > Cc: sta...@vger.kernel.org
> > > ---
> > > arch/x86/kernel/smpboot.c | 11 +--
> > > 1 file changed, 9 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> > > index ffbd9a3d78d8..278cc9f92f2f 100644
> > > --- a/arch/x86/kernel/smpboot.c
> > > +++ b/arch/x86/kernel/smpboot.c
> > > @@ -1603,13 +1603,20 @@ int native_cpu_disable(void)
> > > if (ret)
> > > return ret;
> > >
> > > + cpu_disable_common();
> > > /*
> > > * Disable the local APIC. Otherwise IPI broadcasts will reach
> > > * it. It still responds normally to INIT, NMI, SMI, and SIPI
> > > -* messages.
> >
> > I'm slightly unclear about whether interrupts are disabled at the core
> > by this point or not. I followed native_cpu_disable() up to
> > __cpu_disable(), up to take_cpu_down(). This is passed into a call to
> > stop_machine_cpuslocked(), where interrupts get disabled at the core.
> > So unless there's another path, it seems like interrupts are always
> > disabled at the core by this point.
>
> local_irq_disable() just does cli() which allows interrupts to trickle
> in to the IRR bits, and once you do sti() things would flow back for
> normal interrupt processing.
>
>
> >
> > If interrupts are always disabled, then the comment above is a little
>
> Disable interrupts is different from disabling LAPIC. Once you do the
> apic_soft_disable(), there is nothing flowing into the LAPIC except
> for INIT, NMI, SMI and SIPI messages.
>
> This turns off the pipe for all other interrupts to enter LAPIC. Which
> is different from doing a cli().
I understand the distinction. I was mostly musing on the difference in
behavior your change causes if this function is entered with
interrupts enabled at the core (ie sti()). But I think it never is, so
that thought is moot.
I could never repro the issue reliably on comet lake after Thomas'
original fix. But I can still repro it easily on jasper lake. And this
patch fixes the issue for me on that platform. Thanks for the fix.
Reviewed-by: Evan Green
Tested-by: Evan Green
Re: [PATCH] x86/hotplug: Silence APIC only after all irq's are migrated
Hi Ashok,
Thank you, Srikanth, and Sukumar for some very impressive debugging here.
On Fri, Aug 14, 2020 at 2:38 PM Ashok Raj wrote:
>
> When offlining CPU's, fixup_irqs() migrates all interrupts away from the
> outgoing CPU to an online CPU. Its always possible the device sent an
> interrupt to the previous CPU destination. Pending interrupt bit in IRR in
> lapic identifies such interrupts. apic_soft_disable() will not capture any
> new interrupts in IRR. This causes interrupts from device to be lost during
> cpu offline. The issue was found when explicitly setting MSI affinity to a
> CPU and immediately offlining it. It was simple to recreate with a USB
> ethernet device and doing I/O to it while the CPU is offlined. Lost
> interrupts happen even when Interrupt Remapping is enabled.
>
> Current code does apic_soft_disable() before migrating interrupts.
>
> native_cpu_disable()
> {
> ...
> apic_soft_disable();
> cpu_disable_common();
> --> fixup_irqs(); // Too late to capture anything in IRR.
> }
>
> Just fliping the above call sequence seems to hit the IRR checks
> and the lost interrupt is fixed for both legacy MSI and when
> interrupt remapping is enabled.
>
>
> Fixes: 60dcaad5736f ("x86/hotplug: Silence APIC and NMI when CPU is dead")
> Link: https://lore.kernel.org/lkml/875zdarr4h@nanos.tec.linutronix.de/
> Signed-off-by: Ashok Raj
>
> To: linux-kernel@vger.kernel.org
> To: Thomas Gleixner
> Cc: Sukumar Ghorai
> Cc: Srikanth Nandamuri
> Cc: Evan Green
> Cc: Mathias Nyman
> Cc: Bjorn Helgaas
> Cc: sta...@vger.kernel.org
> ---
> arch/x86/kernel/smpboot.c | 11 +--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> index ffbd9a3d78d8..278cc9f92f2f 100644
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -1603,13 +1603,20 @@ int native_cpu_disable(void)
> if (ret)
> return ret;
>
> + cpu_disable_common();
> /*
> * Disable the local APIC. Otherwise IPI broadcasts will reach
> * it. It still responds normally to INIT, NMI, SMI, and SIPI
> -* messages.
I'm slightly unclear about whether interrupts are disabled at the core
by this point or not. I followed native_cpu_disable() up to
__cpu_disable(), up to take_cpu_down(). This is passed into a call to
stop_machine_cpuslocked(), where interrupts get disabled at the core.
So unless there's another path, it seems like interrupts are always
disabled at the core by this point.
If interrupts are always disabled, then the comment above is a little
obsolete, since we're not expecting to receive broadcast IPIs from
here on out anyway. We could clean up that comment in this change.
If there is a path to here where interrupts are still enabled at the
core, then we'd need to watch out, because this change now allows
broadcast IPIs to come in during cpu_disable_common(). That could be
bad. But I think that's not this case, so this should be ok.
> +* messages. Its important to do apic_soft_disable() after
> +* fixup_irqs(), because fixup_irqs() called from cpu_disable_common()
> +* depends on IRR being set. After apic_soft_disable() CPU preserves
> +* currently set IRR/ISR but new interrupts will not set IRR.
> +* This causes interrupts sent to outgoing cpu before completion
> +* of irq migration to be lost. Check SDM Vol 3 "10.4.7.2 Local
> +* APIC State after It Has been Software Disabled" section for more
> +* details.
> */
> apic_soft_disable();
> - cpu_disable_common();
>
> return 0;
> }
> --
> 2.13.6
>
Re: [PATCH 1/2] serial: qcom_geni_serial: Make kgdb work even if UART isn't console
On Fri, Jul 10, 2020 at 12:24 PM Doug Anderson wrote:
>
> Hi,
>
> On Fri, Jul 10, 2020 at 12:03 PM Evan Green wrote:
> >
> > On Fri, Jul 10, 2020 at 11:19 AM Doug Anderson
> > wrote:
> > >
> > > Hi,
> > >
> > > On Fri, Jul 10, 2020 at 10:39 AM Evan Green wrote:
> > > >
> > > > On Fri, Jun 26, 2020 at 1:01 PM Douglas Anderson
> > > > wrote:
> > > > >
> > > > > The geni serial driver had the rather sketchy hack in it where it
> > > > > would adjust the number of bytes per RX FIFO word from 4 down to 1 if
> > > > > it detected that CONFIG_CONSOLE_POLL was enabled (for kgdb) and this
> > > > > was a console port (defined by the kernel directing output to this
> > > > > port via the "console=" command line argument).
> > > > >
> > > > > The problem with that sketchy hack is that it's possible to run kgdb
> > > > > over a serial port even if it isn't used for console.
> > > > >
> > > > > Let's avoid the hack by simply handling the 4-bytes-per-FIFO word case
> > > > > for kdb. We'll have to have a (very small) cache but that should be
> > > > > fine.
> > > > >
> > > > > A nice side effect of this patch is that an agetty (or similar)
> > > > > running on this port is less likely to drop characters. We'll
> > > > > have roughly 4 times the RX FIFO depth than we used to now.
> > > > >
> > > > > NOTE: the character cache here isn't shared between the polling API
> > > > > and the non-polling API. That means that, technically, the polling
> > > > > API could eat a few extra bytes. This doesn't seem to pose a huge
> > > > > problem in reality because we'll only get several characters per FIFO
> > > > > word if those characters are all received at nearly the same time and
> > > > > we don't really expect non-kgdb characters to be sent to the same port
> > > > > as kgdb at the exact same time we're exiting kgdb.
> > > > >
> > > > > ALSO NOTE: we still have the sketchy hack for setting the number of
> > > > > bytes per TX FIFO word in place, but that one is less bad. kgdb
> > > > > doesn't have any problem with this because it always just sends 1 byte
> > > > > at a time and waits for it to finish. The TX FIFO hack is only really
> > > > > needed for console output. In any case, a future patch will remove
> > > > > that hack, too.
> > > > >
> > > > > Signed-off-by: Douglas Anderson
> > > > > ---
> > > > >
> > > > > drivers/tty/serial/qcom_geni_serial.c | 80
> > > > > ++-
> > > > > 1 file changed, 55 insertions(+), 25 deletions(-)
> > > > >
> > > > > diff --git a/drivers/tty/serial/qcom_geni_serial.c
> > > > > b/drivers/tty/serial/qcom_geni_serial.c
> > > > > index 0300867eab7a..4610e391e886 100644
> > > > > --- a/drivers/tty/serial/qcom_geni_serial.c
> > > > > +++ b/drivers/tty/serial/qcom_geni_serial.c
> > > > > @@ -103,11 +103,13 @@
> > > > > #define DEFAULT_IO_MACRO_IO2_IO3_MASK GENMASK(15, 4)
> > > > > #define IO_MACRO_IO2_IO3_SWAP 0x4640
> > > > >
> > > > > -#ifdef CONFIG_CONSOLE_POLL
> > > > > -#define CONSOLE_RX_BYTES_PW 1
> > > > > -#else
> > > > > -#define CONSOLE_RX_BYTES_PW 4
> > > > > -#endif
> > > > > +struct qcom_geni_private_data {
> > > > > + /* NOTE: earlycon port will have NULL here */
> > > > > + struct uart_driver *drv;
> > > > > +
> > > > > + u32 poll_cached_bytes;
> > > > > + unsigned int poll_cached_bytes_cnt;
> > > > > +};
> > > > >
> > > > > struct qcom_geni_serial_port {
> > > > > struct uart_port uport;
> > > > > @@ -129,6 +131,8 @@ struct qcom_geni_serial_port {
> > > > > int wakeup_irq;
> > > > > bool rx_tx_swap;
> > > > > bool cts_rts_swap;
> > > > > +
> > > > > + struct qcom_geni_private_data private_data;
> > > > > };
> > > > >
> > > > > static const struct uart_op
Re: [PATCH 2/2] serial: qcom_geni_serial: Always use 4 bytes per TX FIFO word
On Fri, Jul 10, 2020 at 11:28 AM Doug Anderson wrote:
>
> Hi,
>
> On Fri, Jul 10, 2020 at 10:46 AM Evan Green wrote:
> >
> > On Fri, Jun 26, 2020 at 1:01 PM Douglas Anderson
> > wrote:
> > >
> > > The geni serial driver had a rule that we'd only use 1 byte per FIFO
> > > word for the TX FIFO if we were being used for the serial console.
> > > This is ugly and a bit of a pain. It's not too hard to fix, so fix
> > > it.
> > >
> > > Signed-off-by: Douglas Anderson
> > > ---
> > >
> > > drivers/tty/serial/qcom_geni_serial.c | 57 +--
> > > 1 file changed, 37 insertions(+), 20 deletions(-)
> > >
> > > diff --git a/drivers/tty/serial/qcom_geni_serial.c
> > > b/drivers/tty/serial/qcom_geni_serial.c
> > > index 4610e391e886..583d903321b5 100644
> > > --- a/drivers/tty/serial/qcom_geni_serial.c
> > > +++ b/drivers/tty/serial/qcom_geni_serial.c
> > > @@ -103,12 +103,18 @@
> > > #define DEFAULT_IO_MACRO_IO2_IO3_MASK GENMASK(15, 4)
> > > #define IO_MACRO_IO2_IO3_SWAP 0x4640
> > >
> > > +/* We always configure 4 bytes per FIFO word */
> > > +#define BYTES_PER_FIFO_WORD4
> > > +
> > > struct qcom_geni_private_data {
> > > /* NOTE: earlycon port will have NULL here */
> > > struct uart_driver *drv;
> > >
> > > u32 poll_cached_bytes;
> > > unsigned int poll_cached_bytes_cnt;
> > > +
> > > + u32 write_cached_bytes;
> > > + unsigned int write_cached_bytes_cnt;
> > > };
> > >
> > > struct qcom_geni_serial_port {
> > > @@ -121,8 +127,6 @@ struct qcom_geni_serial_port {
> > > bool setup;
> > > int (*handle_rx)(struct uart_port *uport, u32 bytes, bool drop);
> > > unsigned int baud;
> > > - unsigned int tx_bytes_pw;
> > > - unsigned int rx_bytes_pw;
> > > void *rx_fifo;
> > > u32 loopback;
> > > bool brk;
> > > @@ -390,13 +394,25 @@ static void qcom_geni_serial_poll_put_char(struct
> > > uart_port *uport,
> > > #ifdef CONFIG_SERIAL_QCOM_GENI_CONSOLE
> > > static void qcom_geni_serial_wr_char(struct uart_port *uport, int ch)
> > > {
> > > - writel(ch, uport->membase + SE_GENI_TX_FIFOn);
> > > + struct qcom_geni_private_data *private_data = uport->private_data;
> > > +
> > > + private_data->write_cached_bytes =
> > > + (private_data->write_cached_bytes >> 8) | (ch << 24);
> > > + private_data->write_cached_bytes_cnt++;
> > > +
> > > + if (private_data->write_cached_bytes_cnt == BYTES_PER_FIFO_WORD) {
> > > + writel(private_data->write_cached_bytes,
> > > + uport->membase + SE_GENI_TX_FIFOn);
> > > + private_data->write_cached_bytes_cnt = 0;
> > > + }
> > > }
> > >
> > > static void
> > > __qcom_geni_serial_console_write(struct uart_port *uport, const char *s,
> > > unsigned int count)
> > > {
> > > + struct qcom_geni_private_data *private_data = uport->private_data;
> > > +
> > > int i;
> > > u32 bytes_to_send = count;
> > >
> > > @@ -431,6 +447,15 @@ __qcom_geni_serial_console_write(struct uart_port
> > > *uport, const char *s,
> > >
> > > SE_GENI_M_IRQ_CLEAR);
> > > i += chars_to_write;
> > > }
> > > +
> > > + if (private_data->write_cached_bytes_cnt) {
> > > + private_data->write_cached_bytes >>= BITS_PER_BYTE *
> > > + (BYTES_PER_FIFO_WORD -
> > > private_data->write_cached_bytes_cnt);
> > > + writel(private_data->write_cached_bytes,
> > > + uport->membase + SE_GENI_TX_FIFOn);
> > > + private_data->write_cached_bytes_cnt = 0;
> > > + }
> >
> > How does this not end up sending stray zeros? In other words, how does
> > the hardware know which bytes of this word are valid?
>
> We told it how many bytes we wanted to send in
> qcom_geni_serial_setup_tx(). If the total number of bytes being sent
> is not a multiple of the FIFO word size then it knows that the last
> word will be a partial and it'll extract just the number of needed
> bytes out of it.
Ohh right. Sounds good.
Reviewed-by: Evan Green
Re: [PATCH 1/2] serial: qcom_geni_serial: Make kgdb work even if UART isn't console
On Fri, Jul 10, 2020 at 11:19 AM Doug Anderson wrote:
>
> Hi,
>
> On Fri, Jul 10, 2020 at 10:39 AM Evan Green wrote:
> >
> > On Fri, Jun 26, 2020 at 1:01 PM Douglas Anderson
> > wrote:
> > >
> > > The geni serial driver had the rather sketchy hack in it where it
> > > would adjust the number of bytes per RX FIFO word from 4 down to 1 if
> > > it detected that CONFIG_CONSOLE_POLL was enabled (for kgdb) and this
> > > was a console port (defined by the kernel directing output to this
> > > port via the "console=" command line argument).
> > >
> > > The problem with that sketchy hack is that it's possible to run kgdb
> > > over a serial port even if it isn't used for console.
> > >
> > > Let's avoid the hack by simply handling the 4-bytes-per-FIFO word case
> > > for kdb. We'll have to have a (very small) cache but that should be
> > > fine.
> > >
> > > A nice side effect of this patch is that an agetty (or similar)
> > > running on this port is less likely to drop characters. We'll
> > > have roughly 4 times the RX FIFO depth than we used to now.
> > >
> > > NOTE: the character cache here isn't shared between the polling API
> > > and the non-polling API. That means that, technically, the polling
> > > API could eat a few extra bytes. This doesn't seem to pose a huge
> > > problem in reality because we'll only get several characters per FIFO
> > > word if those characters are all received at nearly the same time and
> > > we don't really expect non-kgdb characters to be sent to the same port
> > > as kgdb at the exact same time we're exiting kgdb.
> > >
> > > ALSO NOTE: we still have the sketchy hack for setting the number of
> > > bytes per TX FIFO word in place, but that one is less bad. kgdb
> > > doesn't have any problem with this because it always just sends 1 byte
> > > at a time and waits for it to finish. The TX FIFO hack is only really
> > > needed for console output. In any case, a future patch will remove
> > > that hack, too.
> > >
> > > Signed-off-by: Douglas Anderson
> > > ---
> > >
> > > drivers/tty/serial/qcom_geni_serial.c | 80 ++-
> > > 1 file changed, 55 insertions(+), 25 deletions(-)
> > >
> > > diff --git a/drivers/tty/serial/qcom_geni_serial.c
> > > b/drivers/tty/serial/qcom_geni_serial.c
> > > index 0300867eab7a..4610e391e886 100644
> > > --- a/drivers/tty/serial/qcom_geni_serial.c
> > > +++ b/drivers/tty/serial/qcom_geni_serial.c
> > > @@ -103,11 +103,13 @@
> > > #define DEFAULT_IO_MACRO_IO2_IO3_MASK GENMASK(15, 4)
> > > #define IO_MACRO_IO2_IO3_SWAP 0x4640
> > >
> > > -#ifdef CONFIG_CONSOLE_POLL
> > > -#define CONSOLE_RX_BYTES_PW 1
> > > -#else
> > > -#define CONSOLE_RX_BYTES_PW 4
> > > -#endif
> > > +struct qcom_geni_private_data {
> > > + /* NOTE: earlycon port will have NULL here */
> > > + struct uart_driver *drv;
> > > +
> > > + u32 poll_cached_bytes;
> > > + unsigned int poll_cached_bytes_cnt;
> > > +};
> > >
> > > struct qcom_geni_serial_port {
> > > struct uart_port uport;
> > > @@ -129,6 +131,8 @@ struct qcom_geni_serial_port {
> > > int wakeup_irq;
> > > bool rx_tx_swap;
> > > bool cts_rts_swap;
> > > +
> > > + struct qcom_geni_private_data private_data;
> > > };
> > >
> > > static const struct uart_ops qcom_geni_console_pops;
> > > @@ -264,8 +268,9 @@ static bool qcom_geni_serial_poll_bit(struct
> > > uart_port *uport,
> > > unsigned int baud;
> > > unsigned int fifo_bits;
> > > unsigned long timeout_us = 2;
> > > + struct qcom_geni_private_data *private_data = uport->private_data;
> > >
> > > - if (uport->private_data) {
> > > + if (private_data->drv) {
> > > port = to_dev_port(uport, uport);
> > > baud = port->baud;
> > > if (!baud)
> > > @@ -331,23 +336,42 @@ static void qcom_geni_serial_abort_rx(struct
> > > uart_port *uport)
> > > }
> > >
> > > #ifdef CONFIG_CONSOLE_POLL
> > > +
> > > static int qcom_geni_serial_get_char(struct uart_port *uport)
> > > {
> > >
Re: [PATCH 2/2] serial: qcom_geni_serial: Always use 4 bytes per TX FIFO word
On Fri, Jun 26, 2020 at 1:01 PM Douglas Anderson wrote:
>
> The geni serial driver had a rule that we'd only use 1 byte per FIFO
> word for the TX FIFO if we were being used for the serial console.
> This is ugly and a bit of a pain. It's not too hard to fix, so fix
> it.
>
> Signed-off-by: Douglas Anderson
> ---
>
> drivers/tty/serial/qcom_geni_serial.c | 57 +--
> 1 file changed, 37 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/tty/serial/qcom_geni_serial.c
> b/drivers/tty/serial/qcom_geni_serial.c
> index 4610e391e886..583d903321b5 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -103,12 +103,18 @@
> #define DEFAULT_IO_MACRO_IO2_IO3_MASK GENMASK(15, 4)
> #define IO_MACRO_IO2_IO3_SWAP 0x4640
>
> +/* We always configure 4 bytes per FIFO word */
> +#define BYTES_PER_FIFO_WORD4
> +
> struct qcom_geni_private_data {
> /* NOTE: earlycon port will have NULL here */
> struct uart_driver *drv;
>
> u32 poll_cached_bytes;
> unsigned int poll_cached_bytes_cnt;
> +
> + u32 write_cached_bytes;
> + unsigned int write_cached_bytes_cnt;
> };
>
> struct qcom_geni_serial_port {
> @@ -121,8 +127,6 @@ struct qcom_geni_serial_port {
> bool setup;
> int (*handle_rx)(struct uart_port *uport, u32 bytes, bool drop);
> unsigned int baud;
> - unsigned int tx_bytes_pw;
> - unsigned int rx_bytes_pw;
> void *rx_fifo;
> u32 loopback;
> bool brk;
> @@ -390,13 +394,25 @@ static void qcom_geni_serial_poll_put_char(struct
> uart_port *uport,
> #ifdef CONFIG_SERIAL_QCOM_GENI_CONSOLE
> static void qcom_geni_serial_wr_char(struct uart_port *uport, int ch)
> {
> - writel(ch, uport->membase + SE_GENI_TX_FIFOn);
> + struct qcom_geni_private_data *private_data = uport->private_data;
> +
> + private_data->write_cached_bytes =
> + (private_data->write_cached_bytes >> 8) | (ch << 24);
> + private_data->write_cached_bytes_cnt++;
> +
> + if (private_data->write_cached_bytes_cnt == BYTES_PER_FIFO_WORD) {
> + writel(private_data->write_cached_bytes,
> + uport->membase + SE_GENI_TX_FIFOn);
> + private_data->write_cached_bytes_cnt = 0;
> + }
> }
>
> static void
> __qcom_geni_serial_console_write(struct uart_port *uport, const char *s,
> unsigned int count)
> {
> + struct qcom_geni_private_data *private_data = uport->private_data;
> +
> int i;
> u32 bytes_to_send = count;
>
> @@ -431,6 +447,15 @@ __qcom_geni_serial_console_write(struct uart_port
> *uport, const char *s,
> SE_GENI_M_IRQ_CLEAR);
> i += chars_to_write;
> }
> +
> + if (private_data->write_cached_bytes_cnt) {
> + private_data->write_cached_bytes >>= BITS_PER_BYTE *
> + (BYTES_PER_FIFO_WORD -
> private_data->write_cached_bytes_cnt);
> + writel(private_data->write_cached_bytes,
> + uport->membase + SE_GENI_TX_FIFOn);
> + private_data->write_cached_bytes_cnt = 0;
> + }
How does this not end up sending stray zeros? In other words, how does
the hardware know which bytes of this word are valid?
Re: [PATCH 1/2] serial: qcom_geni_serial: Make kgdb work even if UART isn't console
On Fri, Jun 26, 2020 at 1:01 PM Douglas Anderson wrote:
>
> The geni serial driver had the rather sketchy hack in it where it
> would adjust the number of bytes per RX FIFO word from 4 down to 1 if
> it detected that CONFIG_CONSOLE_POLL was enabled (for kgdb) and this
> was a console port (defined by the kernel directing output to this
> port via the "console=" command line argument).
>
> The problem with that sketchy hack is that it's possible to run kgdb
> over a serial port even if it isn't used for console.
>
> Let's avoid the hack by simply handling the 4-bytes-per-FIFO word case
> for kdb. We'll have to have a (very small) cache but that should be
> fine.
>
> A nice side effect of this patch is that an agetty (or similar)
> running on this port is less likely to drop characters. We'll
> have roughly 4 times the RX FIFO depth than we used to now.
>
> NOTE: the character cache here isn't shared between the polling API
> and the non-polling API. That means that, technically, the polling
> API could eat a few extra bytes. This doesn't seem to pose a huge
> problem in reality because we'll only get several characters per FIFO
> word if those characters are all received at nearly the same time and
> we don't really expect non-kgdb characters to be sent to the same port
> as kgdb at the exact same time we're exiting kgdb.
>
> ALSO NOTE: we still have the sketchy hack for setting the number of
> bytes per TX FIFO word in place, but that one is less bad. kgdb
> doesn't have any problem with this because it always just sends 1 byte
> at a time and waits for it to finish. The TX FIFO hack is only really
> needed for console output. In any case, a future patch will remove
> that hack, too.
>
> Signed-off-by: Douglas Anderson
> ---
>
> drivers/tty/serial/qcom_geni_serial.c | 80 ++-
> 1 file changed, 55 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/tty/serial/qcom_geni_serial.c
> b/drivers/tty/serial/qcom_geni_serial.c
> index 0300867eab7a..4610e391e886 100644
> --- a/drivers/tty/serial/qcom_geni_serial.c
> +++ b/drivers/tty/serial/qcom_geni_serial.c
> @@ -103,11 +103,13 @@
> #define DEFAULT_IO_MACRO_IO2_IO3_MASK GENMASK(15, 4)
> #define IO_MACRO_IO2_IO3_SWAP 0x4640
>
> -#ifdef CONFIG_CONSOLE_POLL
> -#define CONSOLE_RX_BYTES_PW 1
> -#else
> -#define CONSOLE_RX_BYTES_PW 4
> -#endif
> +struct qcom_geni_private_data {
> + /* NOTE: earlycon port will have NULL here */
> + struct uart_driver *drv;
> +
> + u32 poll_cached_bytes;
> + unsigned int poll_cached_bytes_cnt;
> +};
>
> struct qcom_geni_serial_port {
> struct uart_port uport;
> @@ -129,6 +131,8 @@ struct qcom_geni_serial_port {
> int wakeup_irq;
> bool rx_tx_swap;
> bool cts_rts_swap;
> +
> + struct qcom_geni_private_data private_data;
> };
>
> static const struct uart_ops qcom_geni_console_pops;
> @@ -264,8 +268,9 @@ static bool qcom_geni_serial_poll_bit(struct uart_port
> *uport,
> unsigned int baud;
> unsigned int fifo_bits;
> unsigned long timeout_us = 2;
> + struct qcom_geni_private_data *private_data = uport->private_data;
>
> - if (uport->private_data) {
> + if (private_data->drv) {
> port = to_dev_port(uport, uport);
> baud = port->baud;
> if (!baud)
> @@ -331,23 +336,42 @@ static void qcom_geni_serial_abort_rx(struct uart_port
> *uport)
> }
>
> #ifdef CONFIG_CONSOLE_POLL
> +
> static int qcom_geni_serial_get_char(struct uart_port *uport)
> {
> - u32 rx_fifo;
> + struct qcom_geni_private_data *private_data = uport->private_data;
> u32 status;
> + u32 word_cnt;
> + int ret;
> +
> + if (!private_data->poll_cached_bytes_cnt) {
> + status = readl(uport->membase + SE_GENI_M_IRQ_STATUS);
> + writel(status, uport->membase + SE_GENI_M_IRQ_CLEAR);
>
> - status = readl(uport->membase + SE_GENI_M_IRQ_STATUS);
> - writel(status, uport->membase + SE_GENI_M_IRQ_CLEAR);
> + status = readl(uport->membase + SE_GENI_S_IRQ_STATUS);
> + writel(status, uport->membase + SE_GENI_S_IRQ_CLEAR);
>
> - status = readl(uport->membase + SE_GENI_S_IRQ_STATUS);
> - writel(status, uport->membase + SE_GENI_S_IRQ_CLEAR);
> + status = readl(uport->membase + SE_GENI_RX_FIFO_STATUS);
> + word_cnt = status & RX_FIFO_WC_MSK;
> + if (!word_cnt)
> + return NO_POLL_CHAR;
>
> - status = readl(uport->membase + SE_GENI_RX_FIFO_STATUS);
> - if (!(status & RX_FIFO_WC_MSK))
> - return NO_POLL_CHAR;
> + if (word_cnt == 1 && (status & RX_LAST))
I forget how the partial word snapping works. Are you sure you want
word_cnt == 1? I see qcom_geni_serial_handle_rx() looks at RX_LAST
independently as long as word_cnt != 0. I'm worried the hardware
allows one FIFO
Re: [PATCH v11] arm64: dts: qcom: sc7180: Add WCN3990 WLAN module device node
On Thu, May 21, 2020 at 9:19 AM Evan Green wrote: > > On Tue, May 19, 2020 at 8:57 PM Rakesh Pillai wrote: > > > > Add device node for the ath10k SNOC platform driver probe > > and add resources required for WCN3990 on sc7180 soc. > > > > Signed-off-by: Rakesh Pillai > > Reviewed-by: Evan Green Looks like this never landed anywhere. Is it blocked on something? -Evan
[PATCH] soc: qcom: smp2p: Safely acquire spinlock without IRQs
smp2p_update_bits() should disable interrupts when it acquires its
spinlock. This is important because without the _irqsave, a priority
inversion can occur.
This function is called both with interrupts enabled in
qcom_q6v5_request_stop(), and with interrupts disabled in
ipa_smp2p_panic_notifier(). IRQ handling of spinlocks should be
consistent to avoid the panic notifier deadlocking because it's
sitting on the thread that's already got the lock via _request_stop().
Found via lockdep.
Fixes: 50e99641413e7 ("soc: qcom: smp2p: Qualcomm Shared Memory Point to Point")
Signed-off-by: Evan Green
---
drivers/soc/qcom/smp2p.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/soc/qcom/smp2p.c b/drivers/soc/qcom/smp2p.c
index 07183d731d747..a9709aae54abb 100644
--- a/drivers/soc/qcom/smp2p.c
+++ b/drivers/soc/qcom/smp2p.c
@@ -318,15 +318,16 @@ static int qcom_smp2p_inbound_entry(struct qcom_smp2p
*smp2p,
static int smp2p_update_bits(void *data, u32 mask, u32 value)
{
struct smp2p_entry *entry = data;
+ unsigned long flags;
u32 orig;
u32 val;
- spin_lock(>lock);
+ spin_lock_irqsave(>lock, flags);
val = orig = readl(entry->value);
val &= ~mask;
val |= value;
writel(val, entry->value);
- spin_unlock(>lock);
+ spin_unlock_irqrestore(>lock, flags);
if (val != orig)
qcom_smp2p_kick(entry->smp2p);
--
2.24.1
Re: [PATCH] ath10k: Acquire tx_lock in tx error paths
On Mon, Jun 8, 2020 at 4:39 AM Kalle Valo wrote:
>
> Evan Green writes:
>
> > ath10k_htt_tx_free_msdu_id() has a lockdep assertion that htt->tx_lock
> > is held. Acquire the lock in a couple of error paths when calling that
> > function to ensure this condition is met.
> >
> > Fixes: 6421969f248fd ("ath10k: refactor tx pending management")
> > Fixes: e62ee5c381c59 ("ath10k: Add support for htt_data_tx_desc_64
> > descriptor")
>
> Fixes tag should be in one line, I fixed that in the pending branch.
Ah, got it. Thanks Kalle!
-Evan
[PATCH] ath10k: Acquire tx_lock in tx error paths
ath10k_htt_tx_free_msdu_id() has a lockdep assertion that htt->tx_lock
is held. Acquire the lock in a couple of error paths when calling that
function to ensure this condition is met.
Fixes: 6421969f248fd ("ath10k: refactor tx pending management")
Fixes: e62ee5c381c59 ("ath10k: Add support for htt_data_tx_desc_64
descriptor")
Signed-off-by: Evan Green
---
drivers/net/wireless/ath/ath10k/htt_tx.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/net/wireless/ath/ath10k/htt_tx.c
b/drivers/net/wireless/ath/ath10k/htt_tx.c
index e9d12ea708b62..e8c00af2cce1d 100644
--- a/drivers/net/wireless/ath/ath10k/htt_tx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_tx.c
@@ -1545,7 +1545,9 @@ static int ath10k_htt_tx_32(struct ath10k_htt *htt,
err_unmap_msdu:
dma_unmap_single(dev, skb_cb->paddr, msdu->len, DMA_TO_DEVICE);
err_free_msdu_id:
+ spin_lock_bh(>tx_lock);
ath10k_htt_tx_free_msdu_id(htt, msdu_id);
+ spin_unlock_bh(>tx_lock);
err:
return res;
}
@@ -1752,7 +1754,9 @@ static int ath10k_htt_tx_64(struct ath10k_htt *htt,
err_unmap_msdu:
dma_unmap_single(dev, skb_cb->paddr, msdu->len, DMA_TO_DEVICE);
err_free_msdu_id:
+ spin_lock_bh(>tx_lock);
ath10k_htt_tx_free_msdu_id(htt, msdu_id);
+ spin_unlock_bh(>tx_lock);
err:
return res;
}
--
2.24.1
Re: [PATCH 1/2] remoteproc: qcom: q6v5: Update running state before requesting stop
On Tue, Jun 2, 2020 at 10:29 PM Sibi Sankar wrote:
>
> Evan,
> Thanks for taking time to review
> the series.
>
> On 2020-06-02 23:14, Evan Green wrote:
> > On Tue, Jun 2, 2020 at 9:33 AM Sibi Sankar
> > wrote:
> >>
> >> Sometimes the stop triggers a watchdog rather than a stop-ack. Update
> >> the running state to false on requesting stop to skip the watchdog
> >> instead.
> >>
> >> Error Logs:
> >> $ echo stop > /sys/class/remoteproc/remoteproc0/state
> >> ipa 1e4.ipa: received modem stopping event
> >> remoteproc-modem: watchdog received: sys_m_smsm_mpss.c:291:APPS force
> >> stop
> >> qcom-q6v5-mss 408.remoteproc-modem: port failed halt
> >> ipa 1e4.ipa: received modem offline event
> >> remoteproc0: stopped remote processor 408.remoteproc-modem
> >>
> >> Fixes: 3b415c8fb263 ("remoteproc: q6v5: Extract common resource
> >> handling")
> >> Cc: sta...@vger.kernel.org
> >> Signed-off-by: Sibi Sankar
> >> ---
> >
> > Are you sure you want to tolerate this behavior from MSS? This is a
> > graceful shutdown, modem shouldn't have a problem completing the
> > proper handshake. If they do, isn't that a bug on the modem side?
>
> The graceful shutdown is achieved
> though sysmon (enabled using
> CONFIG_QCOM_SYSMON). When sysmon is
> enabled we get a shutdown-ack when we
> try to stop the modem, post which
> request stop is a basically a nop.
> Request stop is done to force stop
> the modem during failure cases (like
> rmtfs is not running and so on) and
> we do want to mask the wdog that we get
> during this scenario ( The locking
> already prevents the servicing of the
> wdog during shutdown, the check just
> prevents the scheduling of crash handler
> and err messages associated with it).
> Also this check was always present and
> was missed during common q6v5 resource
> helper migration, hence the unused
> running state in mss driver.
So you're saying that the intention of the ->running check already in
q6v5_wdog_interrupt() was to allow either the stop-ack or wdog
interrupt to complete the stop. This patch just fixes a regression
introduced during the refactor.
This patch seems ok to me then. It still sort of seems like a bug that
the modem responds arbitrarily in one of two ways, even to a "harsh"
shutdown request.
I wasn't aware of QCOM_SYSMON. Reading it now, It seems like kind of a
lot... do I really need all this? Can I get by with just remoteproc
stops?
Anyway, for this patch:
Reviewed-by: Evan Green
Re: [PATCH 1/2] remoteproc: qcom: q6v5: Update running state before requesting stop
On Tue, Jun 2, 2020 at 9:33 AM Sibi Sankar wrote:
>
> Sometimes the stop triggers a watchdog rather than a stop-ack. Update
> the running state to false on requesting stop to skip the watchdog
> instead.
>
> Error Logs:
> $ echo stop > /sys/class/remoteproc/remoteproc0/state
> ipa 1e4.ipa: received modem stopping event
> remoteproc-modem: watchdog received: sys_m_smsm_mpss.c:291:APPS force stop
> qcom-q6v5-mss 408.remoteproc-modem: port failed halt
> ipa 1e4.ipa: received modem offline event
> remoteproc0: stopped remote processor 408.remoteproc-modem
>
> Fixes: 3b415c8fb263 ("remoteproc: q6v5: Extract common resource handling")
> Cc: sta...@vger.kernel.org
> Signed-off-by: Sibi Sankar
> ---
Are you sure you want to tolerate this behavior from MSS? This is a
graceful shutdown, modem shouldn't have a problem completing the
proper handshake. If they do, isn't that a bug on the modem side?
I just worry this will mask real issues that happen during graceful shutdown.
-Evan
Re: [PATCH 2/2] remoteproc: qcom_q6v5_mss: Remove redundant running state
On Tue, Jun 2, 2020 at 9:33 AM Sibi Sankar wrote: > > Remove the redundant running state, as an equivalent is maintained in > the common q6v5 resource handling helpers. > > Signed-off-by: Sibi Sankar This variable was written to and never read, sigh. Thanks for cleaning it up. Reviewed-by: Evan Green
Re: [PATCH v11] arm64: dts: qcom: sc7180: Add WCN3990 WLAN module device node
On Tue, May 19, 2020 at 8:57 PM Rakesh Pillai wrote: > > Add device node for the ath10k SNOC platform driver probe > and add resources required for WCN3990 on sc7180 soc. > > Signed-off-by: Rakesh Pillai Reviewed-by: Evan Green
Re: [PATCH] arm64: dts: qcom: sc7180: Move mss node to the right place
On Wed, May 20, 2020 at 6:03 PM Stephen Boyd wrote:
>
> The modem node has an address of 408 and thus should come after tlmm
> and before gpu. Move the node to the right place to maintainer proper
> address sort order.
>
> Cc: Evan Green
> Cc: Sibi Sankar
> Fixes: e14a15eba89a ("arm64: dts: qcom: sc7180: Add Q6V5 MSS node")
> Signed-off-by: Stephen Boyd
Thanks for the cleanup.
Reviewed-by: Evan Green
Re: [PATCH v9] arm64: dts: qcom: sc7180: Add WCN3990 WLAN module device node
On Sun, May 17, 2020 at 3:47 AM Rakesh Pillai wrote:
>
> Add device node for the ath10k SNOC platform driver probe
> and add resources required for WCN3990 on sc7180 soc.
>
> Signed-off-by: Rakesh Pillai
> ---
> Changes from v8:
> - Removed the qcom,msa-fixed-perm
> ---
> arch/arm64/boot/dts/qcom/sc7180-idp.dts | 7 +++
> arch/arm64/boot/dts/qcom/sc7180.dtsi| 27 +++
> 2 files changed, 34 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> index 4e9149d..38b102e 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> +++ b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> @@ -389,6 +389,13 @@
> };
> };
>
> + {
> + status = "okay";
> + wifi-firmware {
> + iommus = <_smmu 0xc2 0x1>;
> + };
> +};
> +
> /* PINCTRL - additions to nodes defined in sc7180.dtsi */
>
> _clk {
> diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi
> b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> index f1280e0..dd4e095 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> @@ -63,6 +63,11 @@
> clock-frequency = <32764>;
> #clock-cells = <0>;
> };
> +
> + wlan_fw_mem: memory@9410 {
> + reg = <0 0x9410 0 0x20>;
> + no-map;
> + };
This node is not in the right place. Up through v8, this lived inside
reserved-memory. Here it seems to apply into the clocks {} node, which
is not the right spot.
> };
>
> reserved_memory: reserved-memory {
> @@ -944,6 +949,28 @@
> };
> };
>
> + wifi: wifi@1880 {
> + compatible = "qcom,wcn3990-wifi";
> + reg = <0 0x1880 0 0x80>;
> + reg-names = "membase";
> + iommus = <_smmu 0xc0 0x1>;
> + interrupts =
> + ,
> + ,
> + ,
> + ,
> + ,
> + ,
> + ,
> + ,
> + ,
> + ,
> + ,
> + ;
> + memory-region = <_fw_mem>;
Should any of the *-supply regulators be specified? Or are they all
board specific? Or just not needed? The example has these:
vdd-0.8-cx-mx-supply = <_l5>;
vdd-1.8-xo-supply = <_l7a_1p8>;
vdd-1.3-rfa-supply = <_l17a_1p3>;
vdd-3.3-ch0-supply = <_l25a_3p3>;
> + status = "disabled";
> + };
> +
> config_noc: interconnect@150 {
> compatible = "qcom,sc7180-config-noc";
> reg = <0 0x0150 0 0x28000>;
> --
> 2.7.4
Re: [PATCH v2 00/17] net: introduce Qualcomm IPA driver (UPDATED)
On Thu, Mar 5, 2020 at 8:28 PM Alex Elder wrote: > > This series presents the driver for the Qualcomm IP Accelerator (IPA). > > This is version 2 of this updated series. It includes the following > small changes since the previous version: > - Now based on net-next instead of v5.6-rc > - Config option now named CONFIG_QCOM_IPA > - Some minor cleanup in the GSI code > - Small change to replenish logic > - No longer depends on remoteproc bug fixes > What follows is the basically same explanation as was posted previously. > > -Alex > > I have posted earlier versions of this code previously, but it has > undergone quite a bit of development since the last time, so rather > than calling it "version 3" I'm just treating it as a new series > (indicating it's been updated in this message). The fast/data path > is the same as before. But the driver now (nearly) supports a > second platform, its transaction handling has been generalized > and improved, and modem activities are now handled in a more > unified way. > > This series is available (based on net-next in branch "ipa_updated-v2" > in this git repository: > https://git.linaro.org/people/alex.elder/linux.git > > The branch depends on other one other small patch that I sent out > for review earlier. > https://lore.kernel.org/lkml/20200306042302.17602-1-el...@linaro.org/ > I realize this is all already in (yay!), but it took me a long time to get around to fully reading this driver. I'll paste my notes here for posterity or possible future patches. Overall the driver seemed well documented and thoughtfully written. As someone who has seen the old downstream IPA driver (though I didn't look long as my brain started hurting), I greatly appreciate the work required by Alex to polish this all up. So firstly, thanks Alex! Onto the notes. There are a couple themes I noticed. The driver seems occasionally to be unnecessarily layer-caked. I noticed "could be inlined" as a common refrain in my feedback. There are also a couple places with hand-rolled refcounting, atomic exchanges, and odd mutexes. I haven't fully digested those to be able to know how to get rid of them, but I'll point them out as something that "doesn't smell quite right". Acronyms (for my own benefit): ee - execution environment ep - endpoint er - endpoint or route ID rt - resource type dcd - Dynamic clock division (request to GCC to turn you off) bcr - Backwards compatibility register comp - Core master port holb - ??? ipa_main.c: What is IPA_VALIDATION. Can this just be on always or removed? otherwise it will likely bit rot. I'd like to see this suspend_ref go away. ipa_reg.c can be inlined ipa_mem_init can be inlined. IPA_NOTIFY: Shouldn't CONFIG_IPA depend on IPA_NOTIFY? ipa_data.h Why are ipa_resource_src and ipa_resource_dst separate structures? maybe the extern globals at the bottom should just be moved into ipa_main.c ipa_endpoint.h Add a note for enum ipa_endpoint_name indicating who is TXing and RXing ipa_data-sc7180.c Where is IPA_ENDPOINT_MODEM_LAN_TX definition? ipa_clock.c IPA_CORE_CLOCK_RATE - Should probably be specified in DT as a fixed frequency rather than here in code. Interconnect bandwidths - Are these a function of the core clock rate? This may be fine for the initial version, but is there any way to derive the bandwidth requirement? ipa_interconnect_init_one - Probably best to just inline this ipa_clock_get_additional - Seems sketchy, would like to remove this Overall don't like the homebrew reference counting here. Would runtime PM help you do this? ipa_interrupt.h I'd like to get rid of ipa_interrupt_add and ipa_interrupt_remove. Seems like there's no need for these to be dynamically added, it's all one driver. ipa_interrupt.c Why does ipa_interrupt_setup() need to dynamically allocate the structure, can't we just embed it in struct ipa? Without the kzalloc, ipa_interrupt_setup() and ipa_interrupt_teardown() are simple enough they can probably be inlined (at least teardown for sure). Interrupt processing seems a little odd. What I would have expected is: Hard ISR reads pending bits, and immediately writes all pending bits to quiesce them. Save bitmask of pending bits, and send to the threaded handler. Threaded handler then reads and clears pending bits out, and acts on any. Fixes interrupt storm in ipa_isr() if an unexpected interrupt comes in but an expected interrupt is also pending. Avoids multiple register writes (one for each bit) in ipa_interrupt_process() Saves all the register reads in ipa_interrupt_process_all(). That additional read in the loop seems like it shouldn't be there either way. ipa_mem.h Is IPA_SHARED_MEM_SIZE supposed to be defined? It's mentioned in the comment. Comment says the number of canaries is the same for all IPA versions, but ipa_data-sdm845.c and ipa_data-sc7180.c seem to have different canary counts for IPA_MEM_UC_INFO? Should the number of canaries really be part of
Re: [PATCH 0/2] Drop all accesses to MPSS PERPH register space
On Wed, Apr 15, 2020 at 7:51 AM Sibi Sankar wrote: > > 7C retail devices using MSA based boot will result in a fuse combination > which will prevent accesses to MSS PERPH register space where the mpss > clocks and halt-nav reside. Hence requesting a halt-nav as part of the > SSR sequence will result in a NoC error. Issuing HALT NAV request and > turning on the mss clocks as part of SSR will no longer be required > since the modem firmware will have the necessary fixes to ensure that > there are no pending NAV DMA transactions thereby ensuring a smooth > SSR. > > Sibi Sankar (2): > dt-bindings: remoteproc: qcom: Replace halt-nav with spare-regs > remoteproc: qcom_q6v5_mss: Drop accesses to MPSS PERPH register space I haven't tested things in the "production" fuse configuration yet, but in my current configuration I've got a tree that's running the modem well. Tested-by: Evan Green
Re: [PATCH v2 2/2] remoteproc: qcom_q6v5_mss: Remove unused q6v5_da_to_va function
On Wed, Apr 15, 2020 at 12:16 AM Sibi Sankar wrote: > > Remove unsed q6v5_da_to_va function as the mss driver uses a per segment > dump function. > > Signed-off-by: Sibi Sankar I tested both patches (successfully), but for some reason this is the only one related to this series in my inbox. Tested-by: Evan Green
Re: [PATCH v2 0/7] Add PAS and MSA based Modem support
On Tue, Apr 21, 2020 at 7:32 AM Sibi Sankar wrote: > > Add PAS based modem support on SC7180 SoCs and update the device node to > support MSA based modem boot. > > V2: > * use memory-region to reference mba/mpss regions [Bjorn] > * move peripheral memory regions to the board dts [Bjorn] > * overload the base remoteproc_mpss node wherever possible [Bjorn] > * Pick up Bjorn's R-b > > Patch [1,2] - Add PAS based modem support > Patch [3,4] - use memory-region to reference mba/mpss regions > Patch [5] - Update reserved memory map > Patch [6,7] - Add PAS/MSA modem nodes > > Sibi Sankar (7): > dt-bindings: remoteproc: qcom: Add SC7180 MPSS support > remoteproc: qcom: pas: Add SC7180 Modem support > dt-bindings: remoteproc: qcom: Use memory-region to reference memory > remoteproc: qcom_q6v5_mss: Extract mba/mpss from memory-region > arm64: dts: qcom: sc7180: Update reserved memory map > arm64: dts: qcom: sc7180: Add Q6V5 MSS node > arm64: dts: qcom: sc7180: Update Q6V5 MSS node Tested-by: Evan Green
Re: [PATCH] Input: synaptics-rmi4 - Really fix attn_data use-after-free
On Mon, Apr 27, 2020 at 6:11 PM Dmitry Torokhov
wrote:
>
> On Mon, Apr 27, 2020 at 02:55:48PM -0700, Evan Green wrote:
> > Fix a use-after-free noticed by running with KASAN enabled. If
> > rmi_irq_fn() is run twice in a row, then rmi_f11_attention() (among
> > others) will end up reading from drvdata->attn_data.data, which was
> > freed and left dangling in rmi_irq_fn().
> >
> > Commit 55edde9fff1a ("Input: synaptics-rmi4 - prevent UAF reported by
> > KASAN") correctly identified and analyzed this bug. However the attempted
> > fix only NULLed out a local variable, missing the fact that
> > drvdata->attn_data is a struct, not a pointer.
> >
> > NULL out the correct pointer in the driver data to prevent the attention
> > functions from copying from it.
> >
> > Fixes: 55edde9fff1a ("Input: synaptics-rmi4 - prevent UAF reported by
> > KASAN")
> > Fixes: b908d3cd812a ("Input: synaptics-rmi4 - allow to add attention data")
> >
> > Signed-off-by: Evan Green
> >
> > Cc: sta...@vger.kernel.org
> > Cc: Nick Desaulniers
>
> Ugh, this is all kind of ugly, but I guess that's what we have now.
> Applied, thank you.
Thanks Dmitry. There are other bits that sketch me out in here as
well, but I didn't get a chance to really figure out if they're a
problem. We call rmi_process_interrupt_requests(), which results in
reads from that same attn_data.data pointer, in a few different
places. Some of those calls are outside the irq handling path, like
the in rmi_enable_irq() and rmi_enable_sensor(). Can they race with
the irq handling path? (Meaning they'd be doing those attn_data.data
reads as rmi_irq_fn() is kfreeing the data?) There are a smattering of
mutexes around, but I'm not sure if they're trying to protect this.
If I can find some time I'll try to submit a patch. Anyone is welcome
to beat me to it though.
-Evan
[RFC v2 3/4] dmaengine: sf-pdma: add platform DMA support for HiFive Unleashed A00
Add PDMA driver, sf-pdma, to enable DMA engine on HiFive Unleashed
Rev A00 board.
- Implement dmaengine APIs, support MEM_TO_MEM async copy.
- Tested by DMA Test client
- Supports 4 channels DMA, each channel has 1 done and 1 err
interrupt connected to platform-level interrupt controller (PLIC).
- Depends on DMA_ENGINE and DMA_VIRTUAL_CHANNELS
The datasheet is here:
https://static.dev.sifive.com/FU540-C000-v1.0.pdf
Follow the DMAengine controller doc,
"./Documentation/driver-api/dmaengine/provider.rst" to implement DMA
engine. And use the dma test client in doc,
"./Documentation/driver-api/dmaengine/dmatest.rst", to test.
Each DMA channel has separate HW regs and support done and error ISRs.
4 channels share 1 done and 1 err ISRs. There's no expander/arbitrator
in DMA HW.
-- --
||--< done 23 >--|ch 0|
||--< err 24 >--|| (dma0chan0)
|| --
|| --
||--< done 25 >--|ch 1|
||--< err 26 >--|| (dma0chan1)
|PLIC| --
|| --
||--< done 27 >--|ch 2|
||--< err 28 >--|| (dma0chan2)
|| --
|| --
||--< done 29 >--|ch 3|
||--< err 30 >--|| (dma0chan3)
-- --
Reviewed-by: Vinod Koul
Signed-off-by: Green Wan
Reported-by: kbuild test robot
Fixes: 31c3b98b5a01 ("dmaengine: sf-pdma: add platform DMA support for HiFive
Unleashed A00")
Signed-off-by: kbuild test robot
---
drivers/dma/Kconfig | 2 +
drivers/dma/Makefile | 1 +
drivers/dma/sf-pdma/Kconfig | 6 +
drivers/dma/sf-pdma/Makefile | 1 +
drivers/dma/sf-pdma/sf-pdma.c | 601 ++
drivers/dma/sf-pdma/sf-pdma.h | 124 +++
6 files changed, 735 insertions(+)
create mode 100644 drivers/dma/sf-pdma/Kconfig
create mode 100644 drivers/dma/sf-pdma/Makefile
create mode 100644 drivers/dma/sf-pdma/sf-pdma.c
create mode 100644 drivers/dma/sf-pdma/sf-pdma.h
diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
index 7af874b69ffb..03dc82094857 100644
--- a/drivers/dma/Kconfig
+++ b/drivers/dma/Kconfig
@@ -661,6 +661,8 @@ source "drivers/dma/qcom/Kconfig"
source "drivers/dma/dw/Kconfig"
+source "drivers/dma/sf-pdma/Kconfig"
+
source "drivers/dma/dw-edma/Kconfig"
source "drivers/dma/hsu/Kconfig"
diff --git a/drivers/dma/Makefile b/drivers/dma/Makefile
index f5ce8665e944..4bbd90563ede 100644
--- a/drivers/dma/Makefile
+++ b/drivers/dma/Makefile
@@ -28,6 +28,7 @@ obj-$(CONFIG_DMA_SUN4I) += sun4i-dma.o
obj-$(CONFIG_DMA_SUN6I) += sun6i-dma.o
obj-$(CONFIG_DW_AXI_DMAC) += dw-axi-dmac/
obj-$(CONFIG_DW_DMAC_CORE) += dw/
+obj-$(CONFIG_SF_PDMA) += sf-pdma/
obj-$(CONFIG_DW_EDMA) += dw-edma/
obj-$(CONFIG_EP93XX_DMA) += ep93xx_dma.o
obj-$(CONFIG_FSL_DMA) += fsldma.o
diff --git a/drivers/dma/sf-pdma/Kconfig b/drivers/dma/sf-pdma/Kconfig
new file mode 100644
index ..0e01a5728a79
--- /dev/null
+++ b/drivers/dma/sf-pdma/Kconfig
@@ -0,0 +1,6 @@
+config SF_PDMA
+ bool "Sifive PDMA controller driver"
+ select DMA_ENGINE
+ select DMA_VIRTUAL_CHANNELS
+ help
+ Support the SiFive PDMA controller.
diff --git a/drivers/dma/sf-pdma/Makefile b/drivers/dma/sf-pdma/Makefile
new file mode 100644
index ..764552ab8d0a
--- /dev/null
+++ b/drivers/dma/sf-pdma/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_SF_PDMA) += sf-pdma.o
diff --git a/drivers/dma/sf-pdma/sf-pdma.c b/drivers/dma/sf-pdma/sf-pdma.c
new file mode 100644
index ..973ed9d8cfa4
--- /dev/null
+++ b/drivers/dma/sf-pdma/sf-pdma.c
@@ -0,0 +1,601 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/**
+ * SiFive FU540 Platform DMA driver
+ * Copyright (C) 2019 SiFive
+ *
+ * Based partially on:
+ * - drivers/dma/fsl-edma.c
+ * - drivers/dma/dw-edma/
+ * - drivers/dma/pxa-dma.c
+ *
+ * See the following sources for further documentation:
+ * - Chapter 12 "Platform DMA Engine (PDMA)" of
+ * SiFive FU540-C000 v1.0
+ * https://static.dev.sifive.com/FU540-C000-v1.0.pdf
+ */
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include "sf-pdma.h"
+#include "../dmaengine.h"
+#include "../virt-dma.h"
+
+#define SIFIVE_PDMA_NAME "sf-pdma"
+
+#ifndef readq
+static inline unsigned long long readq(void __iomem *addr)
+{
+ return readl(addr) | (((unsigned long long)readl(addr + 4)) << 32LL);
+}
+#endif
+
+#ifndef writeq
+static inline void writeq(unsigned long long v, void __iomem *addr)
+{
+ writel(v & 0x, addr);
+ writel(v >> 32, addr + 4);
+}
+#endif
+
+static inline struct sf_
[RFC v2 4/4] MAINTAINERS: Add Green as SiFive PDMA driver maintainer
Update MAINTAINERS for SiFive PDMA driver. Signed-off-by: Green Wan --- MAINTAINERS | 6 ++ 1 file changed, 6 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index a69e6db80c79..62d5b249be65 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -14778,6 +14778,12 @@ F: drivers/media/usb/siano/ F: drivers/media/usb/siano/ F: drivers/media/mmc/siano/ +SIFIVE PDMA DRIVER +M: Green Wan +S: Maintained +F: drivers/dma/sf-pdma/ +F: Documentation/devicetree/bindings/dma/sifive,fu540-c000-pdma.yaml + SIFIVE DRIVERS M: Palmer Dabbelt M: Paul Walmsley -- 2.17.1
[RFC v2 2/4] riscv: dts: add support for PDMA device of HiFive Unleashed Rev A00
Add PDMA support to (arch/riscv/boot/dts/sifive/fu540-c000.dtsi)
Signed-off-by: Green Wan
---
arch/riscv/boot/dts/sifive/fu540-c000.dtsi | 7 +++
1 file changed, 7 insertions(+)
diff --git a/arch/riscv/boot/dts/sifive/fu540-c000.dtsi
b/arch/riscv/boot/dts/sifive/fu540-c000.dtsi
index afa43c7ea369..70a1891e7cd0 100644
--- a/arch/riscv/boot/dts/sifive/fu540-c000.dtsi
+++ b/arch/riscv/boot/dts/sifive/fu540-c000.dtsi
@@ -162,6 +162,13 @@
clocks = < PRCI_CLK_TLCLK>;
status = "disabled";
};
+ dma: dma@300 {
+ compatible = "sifive,fu540-c000-pdma";
+ reg = <0x0 0x300 0x0 0x8000>;
+ interrupt-parent = <>;
+ interrupts = <23 24 25 26 27 28 29 30>;
+ #dma-cells = <1>;
+ };
uart1: serial@10011000 {
compatible = "sifive,fu540-c000-uart", "sifive,uart0";
reg = <0x0 0x10011000 0x0 0x1000>;
--
2.17.1
[RFC v2 1/4] dt-bindings: dmaengine: sf-pdma: add bindins for SiFive PDMA
Add DT bindings document for Platform DMA(PDMA) driver of board,
HiFive Unleashed Rev A00.
Reviewed-by: Rob Herring
Reviewed-by: Pragnesh Patel
Signed-off-by: Green Wan
---
.../bindings/dma/sifive,fu540-c000-pdma.yaml | 55 +++
1 file changed, 55 insertions(+)
create mode 100644
Documentation/devicetree/bindings/dma/sifive,fu540-c000-pdma.yaml
diff --git a/Documentation/devicetree/bindings/dma/sifive,fu540-c000-pdma.yaml
b/Documentation/devicetree/bindings/dma/sifive,fu540-c000-pdma.yaml
new file mode 100644
index ..2ca3ddbe1ff4
--- /dev/null
+++ b/Documentation/devicetree/bindings/dma/sifive,fu540-c000-pdma.yaml
@@ -0,0 +1,55 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/dma/sifive,fu540-c000-pdma.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: SiFive Unleashed Rev C000 Platform DMA
+
+maintainers:
+ - Green Wan
+ - Palmer Debbelt
+ - Paul Walmsley
+
+description: |
+ Platform DMA is a DMA engine of SiFive Unleashed. It supports 4
+ channels. Each channel has 2 interrupts. One is for DMA done and
+ the other is for DME error.
+
+ In different SoC, DMA could be attached to different IRQ line.
+ DT file need to be changed to meet the difference. For technical
+ doc,
+
+ https://static.dev.sifive.com/FU540-C000-v1.0.pdf
+
+properties:
+ compatible:
+items:
+ - const: sifive,fu540-c000-pdma
+
+ reg:
+maxItems: 1
+
+ interrupts:
+minItems: 1
+maxItems: 8
+
+ '#dma-cells':
+const: 1
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - '#dma-cells'
+
+examples:
+ - |
+dma@300 {
+ compatible = "sifive,fu540-c000-pdma";
+ reg = <0x0 0x300 0x0 0x8000>;
+ interrupts = <23 24 25 26 27 28 29 30>;
+ #dma-cells = <1>;
+};
+
+...
--
2.17.1
[RFC v2 0/4] dmaengine: sf-pdma: Add platform dma driver
Add PDMA driver support for SiFive HiFive Unleashed RevA00 board. Mainly follows DMAengine controller doc[1] to implement and take other DMA drivers as reference. Such as - drivers/dma/fsl-edma.c - drivers/dma/dw-edma/ - drivers/dma/pxa-dma.c Using DMA test client[2] to test. Detailed datasheet is doc[3]. Driver supports: - 4 physical DMA channels, share same DONE and error interrupt handler. - Support MEM_TO_MEM - Tested by DMA test client - patches include DT Bindgins document and dts for fu450-c000 SoC. Separate dts patch for easier review and apply to different branch or SoC platform. - retry 1 time if DMA error occurs. [Reference Doc] [1] ./Documentation/driver-api/dmaengine/provider.rst [2] ./Documentation/driver-api/dmaengine/dmatest.rst [3] https://static.dev.sifive.com/FU540-C000-v1.0.pdf [Simple steps to test of DMA Test client] $ echo 1 > /sys/module/dmatest/parameters/iterations $ echo dma0chan0 > /sys/module/dmatest/parameters/channel $ echo dma0chan1 > /sys/module/dmatest/parameters/channel $ echo dma0chan2 > /sys/module/dmatest/parameters/channel $ echo dma0chan3 > /sys/module/dmatest/parameters/channel $ echo 1 > /sys/module/dmatest/parameters/run [Expected test result] [ 267.563323] dmatest: dma0chan0-copy0: summary 45629 tests, 0 failures 38769.01 iops 309661 KB/s (0) [ 267.572427] dmatest: dma0chan1-copy0: summary 45863 tests, 0 failures 40286.85 iops 321643 KB/s (0) [ 267.581392] dmatest: dma0chan2-copy0: summary 45975 tests, 0 failures 41178.48 iops 328740 KB/s (0) [ 267.590542] dmatest: dma0chan3-copy0: summary 44768 tests, 0 failures 38560.29 iops 307726 KB/s (0) Green Wan (4): dt-bindings: dmaengine: sf-pdma: add bindins for SiFive PDMA riscv: dts: add support for PDMA device of HiFive Unleashed Rev A00 dmaengine: sf-pdma: add platform DMA support for HiFive Unleashed A00 MAINTAINERS: Add Green as SiFive PDMA driver maintainer .../bindings/dma/sifive,fu540-c000-pdma.yaml | 55 ++ MAINTAINERS | 6 + arch/riscv/boot/dts/sifive/fu540-c000.dtsi| 7 + drivers/dma/Kconfig | 2 + drivers/dma/Makefile | 1 + drivers/dma/sf-pdma/Kconfig | 6 + drivers/dma/sf-pdma/Makefile | 1 + drivers/dma/sf-pdma/sf-pdma.c | 601 ++ drivers/dma/sf-pdma/sf-pdma.h | 124 9 files changed, 803 insertions(+) create mode 100644 Documentation/devicetree/bindings/dma/sifive,fu540-c000-pdma.yaml create mode 100644 drivers/dma/sf-pdma/Kconfig create mode 100644 drivers/dma/sf-pdma/Makefile create mode 100644 drivers/dma/sf-pdma/sf-pdma.c create mode 100644 drivers/dma/sf-pdma/sf-pdma.h base-commit: 4f5cafb5cb8471e54afdc9054d973535614f7675 -- 2.17.1
Re: [PATCH v2 0/2] Avoid regmap debugfs collisions in qcom llcc driver
On Wed, Oct 9, 2019 at 10:46 AM Bjorn Andersson
wrote:
>
> On Wed 09 Oct 09:01 PDT 2019, Evan Green wrote:
>
> > On Tue, Oct 8, 2019 at 6:58 PM Stephen Boyd wrote:
> > >
> > > Quoting Bjorn Andersson (2019-10-08 16:55:04)
> > > > On Tue 08 Oct 16:45 PDT 2019, Stephen Boyd wrote:
> > > > > @@ drivers/soc/qcom/llcc-slice.c
> > > > >
> > > > > static struct llcc_drv_data *drv_data = (void *) -EPROBE_DEFER;
> > > > >
> > > > > --static const struct regmap_config llcc_regmap_config = {
> > > > > +-static struct regmap_config llcc_regmap_config = {
> > > > > -.reg_bits = 32,
> > > > > -.reg_stride = 4,
> > > > > -.val_bits = 32,
> > > > > @@ drivers/soc/qcom/llcc-slice.c: static struct regmap
> > > > > *qcom_llcc_init_mmio(struct
> > > > > {
> > > > > struct resource *res;
> > > > > void __iomem *base;
> > > > > -+static struct regmap_config llcc_regmap_config = {
> > > > > ++struct regmap_config llcc_regmap_config = {
> > > >
> > > > Now that this isn't static I like the end result better. Not sure about
> > > > the need for splitting it in two patches, but if Evan is happy I'll take
> > > > it.
> > > >
> > >
> > > Well I split it into bug fix and micro-optimization so backport choices
> > > can be made. But yeah, I hope Evan is happy enough to provide a
> > > reviewed-by tag!
> >
> > It's definitely better without the static local since it no longer has
> > the cognitive trap, but I still don't really get why we're messing
> > with the global v. local aspect of it. We're now inconsistent with
> > every other caller of this function, and for what exactly? We've
> > traded some data space for a call to memset() and some instructions. I
> > would have thought anecdotally that memory was the cheaper thing (ie
> > cpu speeds stopped increasing awhile ago, but memory is still getting
> > cheaper).
> >
>
> The reason for making the structure local is because it's being modified
> per instance, meaning it would still work as long as
> qcom_llcc_init_mmio() is never called concurrently for two llcc
> instances. But the correctness outweighs the performance degradation of
> setting it up on the stack in my view.
>
I hadn't considered the concurrency aspect of the change, since I had
anchored myself on the static local. I'm convinced. Might be worth
mentioning that in the commit message.
For the series:
Reviewed-by: Evan Green
Re: [PATCH v2 0/2] Avoid regmap debugfs collisions in qcom llcc driver
On Tue, Oct 8, 2019 at 6:58 PM Stephen Boyd wrote:
>
> Quoting Bjorn Andersson (2019-10-08 16:55:04)
> > On Tue 08 Oct 16:45 PDT 2019, Stephen Boyd wrote:
> > > @@ drivers/soc/qcom/llcc-slice.c
> > >
> > > static struct llcc_drv_data *drv_data = (void *) -EPROBE_DEFER;
> > >
> > > --static const struct regmap_config llcc_regmap_config = {
> > > +-static struct regmap_config llcc_regmap_config = {
> > > -.reg_bits = 32,
> > > -.reg_stride = 4,
> > > -.val_bits = 32,
> > > @@ drivers/soc/qcom/llcc-slice.c: static struct regmap
> > > *qcom_llcc_init_mmio(struct
> > > {
> > > struct resource *res;
> > > void __iomem *base;
> > > -+static struct regmap_config llcc_regmap_config = {
> > > ++struct regmap_config llcc_regmap_config = {
> >
> > Now that this isn't static I like the end result better. Not sure about
> > the need for splitting it in two patches, but if Evan is happy I'll take
> > it.
> >
>
> Well I split it into bug fix and micro-optimization so backport choices
> can be made. But yeah, I hope Evan is happy enough to provide a
> reviewed-by tag!
It's definitely better without the static local since it no longer has
the cognitive trap, but I still don't really get why we're messing
with the global v. local aspect of it. We're now inconsistent with
every other caller of this function, and for what exactly? We've
traded some data space for a call to memset() and some instructions. I
would have thought anecdotally that memory was the cheaper thing (ie
cpu speeds stopped increasing awhile ago, but memory is still getting
cheaper).
But either way it's correct, so really it's fine if you ignore me :)
-Evan
Re: [PATCH v2 1/2] soc: qcom: llcc: Name regmaps to avoid collisions
On Tue, Oct 8, 2019 at 4:45 PM Stephen Boyd wrote:
>
> We'll end up with debugfs collisions if we don't give names to the
> regmaps created by this driver. Change the name of the config before
> registering it so we don't collide in debugfs.
>
> Fixes: 7f9c136216c7 ("soc: qcom: Add broadcast base for Last Level Cache
> Controller (LLCC)")
> Cc: Venkata Narendra Kumar Gutta
> Cc: Evan Green
> Signed-off-by: Stephen Boyd
Reviewed-by: Evan Green
[PATCH] Input: synaptics-rmi4 - Avoid processing unknown IRQs
rmi_process_interrupt_requests() calls handle_nested_irq() for each interrupt status bit it finds. If the irq domain mapping for this bit had not yet been set up, then it ends up calling handle_nested_irq(0), which causes a NULL pointer dereference. There's already code that masks the irq_status bits coming out of the hardware with current_irq_mask, presumably to avoid this situation. However current_irq_mask seems to more reflect the actual mask set in the hardware rather than the IRQs software has set up and registered for. For example, in rmi_driver_reset_handler(), the current_irq_mask is initialized based on what is read from the hardware. If the reset value of this mask enables IRQs that Linux has not set up yet, then we end up in this situation. There appears to be a third unused bitmask that used to serve this purpose, fn_irq_bits. Use that bitmask instead of current_irq_mask to avoid calling handle_nested_irq() on IRQs that have not yet been set up. Signed-off-by: Evan Green --- drivers/input/rmi4/rmi_driver.c | 6 +- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c index 772493b1f665..190b9974526b 100644 --- a/drivers/input/rmi4/rmi_driver.c +++ b/drivers/input/rmi4/rmi_driver.c @@ -146,7 +146,7 @@ static int rmi_process_interrupt_requests(struct rmi_device *rmi_dev) } mutex_lock(>irq_mutex); - bitmap_and(data->irq_status, data->irq_status, data->current_irq_mask, + bitmap_and(data->irq_status, data->irq_status, data->fn_irq_bits, data->irq_count); /* * At this point, irq_status has all bits that are set in the @@ -385,6 +385,8 @@ static int rmi_driver_set_irq_bits(struct rmi_device *rmi_dev, bitmap_copy(data->current_irq_mask, data->new_irq_mask, data->num_of_irq_regs); + bitmap_or(data->fn_irq_bits, data->fn_irq_bits, mask, data->irq_count); + error_unlock: mutex_unlock(>irq_mutex); return error; @@ -398,6 +400,8 @@ static int rmi_driver_clear_irq_bits(struct rmi_device *rmi_dev, struct device *dev = _dev->dev; mutex_lock(>irq_mutex); + bitmap_andnot(data->fn_irq_bits, + data->fn_irq_bits, mask, data->irq_count); bitmap_andnot(data->new_irq_mask, data->current_irq_mask, mask, data->irq_count); -- 2.21.0
Re: [PATCH] soc: qcom: llcc: Name regmaps to avoid collisions
On Fri, Oct 4, 2019 at 4:31 PM Stephen Boyd wrote:
>
> We'll end up with debugfs collisions if we don't give names to the
> regmaps created inside this driver. Copy the template config over into
> this function and give the regmap the same name as the resource name.
>
> Fixes: 7f9c136216c7 ("soc: qcom: Add broadcast base for Last Level Cache
> Controller (LLCC)")
> Cc: Venkata Narendra Kumar Gutta
> Cc: Evan Green
> Signed-off-by: Stephen Boyd
> ---
> drivers/soc/qcom/llcc-slice.c | 14 +++---
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/soc/qcom/llcc-slice.c b/drivers/soc/qcom/llcc-slice.c
> index 9090ea12eaf3..aa342938c403 100644
> --- a/drivers/soc/qcom/llcc-slice.c
> +++ b/drivers/soc/qcom/llcc-slice.c
> @@ -48,13 +48,6 @@
>
> static struct llcc_drv_data *drv_data = (void *) -EPROBE_DEFER;
>
> -static const struct regmap_config llcc_regmap_config = {
> - .reg_bits = 32,
> - .reg_stride = 4,
> - .val_bits = 32,
> - .fast_io = true,
> -};
> -
> /**
> * llcc_slice_getd - get llcc slice descriptor
> * @uid: usecase_id for the client
> @@ -314,6 +307,12 @@ static struct regmap *qcom_llcc_init_mmio(struct
> platform_device *pdev,
> {
> struct resource *res;
> void __iomem *base;
> + static struct regmap_config llcc_regmap_config = {
> + .reg_bits = 32,
> + .reg_stride = 4,
> + .val_bits = 32,
> + .fast_io = true,
> + };
Why did you move this to be a static local? I think it works, but it
makes it look like this is a local variable that's possibly used out
of scope. Maybe leave it as a global?
Re: [PATCH v3 3/5] memremap: Add support for read-only memory mappings
On Thu, Oct 3, 2019 at 11:56 AM Stephen Boyd wrote: > > Quoting Evan Green (2019-09-18 12:37:34) > > On Tue, Sep 10, 2019 at 9:09 AM Stephen Boyd wrote: > > > > > > @@ -53,6 +60,9 @@ static void *try_ram_remap(resource_size_t offset, > > > size_t size, > > > * mapping types will be attempted in the order listed below until one of > > > * them succeeds. > > > * > > > + * MEMREMAP_RO - establish a mapping whereby writes are ignored/rejected. > > > + * Attempts to map System RAM with this mapping type will fail. > > > > Why should attempts to map RAM with this flag fail? MEMREMAP_WB will > > allow RAM and quietly give you back the direct mapping, so it seems > > like at least some values in this function allow RAM. > > > > Oh, I see a comment below about "Enforce that this mapping is not > > aliasing System RAM". I guess this is worried about cache coloring? > > But is that a problem with RO mappings? I guess the RO mappings could > > get partially stale, so if the memory were being updated out from > > under you, you might see some updates but not others. Was that the > > rationale? > > Will Deacon, Dan Williams, and I talked about this RO flag at LPC and I > believe we decided to mostly get rid of the flags argument to this > function. The vast majority of callers pass MEMREMAP_WB, so I'll just > make that be the implementation default and support the flags for > encrpytion (MEMREMAP_ENC and MEMREMAP_DEC). There are a few callers that > pass MEMREMAP_WC or MEMREMAP_WT (and one that passes all of them), but I > believe those can be changed to MEMREMAP_WB and not care. There's also > the efi framebuffer code that matches the memory attributes in the EFI > memory map. I'm not sure what to do with that one to be quite honest. > Maybe EFI shouldn't care and just use whatever is already there in the > mapping? I would guess that the folks mapping things like framebuffers would care if their write-combined memory were changed to writeback. But I suppose the better authorities on that are already here, so if they think it's fine, I guess it's all good. Whatever logic is used to defend that would likely apply equally well to the EFI mappings. > > Either way, I'll introduce a memremap_ro() API that maps memory as read > only if possible and return a const void pointer as well. I'm debating > making that API fallback to memremap() if RO isn't supported for some > reason or can't work because we're remapping system memory but that > seems a little too nice when the caller could just as well decide to > fail if memory can't be mapped as read only. Sounds good. My small vote would be for the nicer fallback to memremap(). I can't think of a case where someone would rather not have their memory mapped at all than have it mapped writeable. -Evan
[PATCH v4 4/4] MAINTAINERS: Add Green as SiFive PDMA driver maintainer
Update MAINTAINERS for SiFive PDMA driver. Signed-off-by: Green Wan --- MAINTAINERS | 6 ++ 1 file changed, 6 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index 30a5b4028d2f..6c12da0a324d 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -14779,6 +14779,12 @@ F: drivers/media/usb/siano/ F: drivers/media/usb/siano/ F: drivers/media/mmc/siano/ +SIFIVE PDMA DRIVER +M: Green Wan +S: Maintained +F: drivers/dma/sf-pdma/ +F: Documentation/devicetree/bindings/dma/sifive,fu540-c000-pdma.yaml + SIFIVE DRIVERS M: Palmer Dabbelt M: Paul Walmsley -- 2.17.1
[PATCH v4 2/4] riscv: dts: add support for PDMA device of HiFive Unleashed Rev A00
Add PDMA support to (arch/riscv/boot/dts/sifive/fu540-c000.dtsi)
Signed-off-by: Green Wan
---
arch/riscv/boot/dts/sifive/fu540-c000.dtsi | 7 +++
1 file changed, 7 insertions(+)
diff --git a/arch/riscv/boot/dts/sifive/fu540-c000.dtsi
b/arch/riscv/boot/dts/sifive/fu540-c000.dtsi
index afa43c7ea369..70a1891e7cd0 100644
--- a/arch/riscv/boot/dts/sifive/fu540-c000.dtsi
+++ b/arch/riscv/boot/dts/sifive/fu540-c000.dtsi
@@ -162,6 +162,13 @@
clocks = < PRCI_CLK_TLCLK>;
status = "disabled";
};
+ dma: dma@300 {
+ compatible = "sifive,fu540-c000-pdma";
+ reg = <0x0 0x300 0x0 0x8000>;
+ interrupt-parent = <>;
+ interrupts = <23 24 25 26 27 28 29 30>;
+ #dma-cells = <1>;
+ };
uart1: serial@10011000 {
compatible = "sifive,fu540-c000-uart", "sifive,uart0";
reg = <0x0 0x10011000 0x0 0x1000>;
--
2.17.1
[PATCH v4 3/4] dmaengine: sf-pdma: add platform DMA support for HiFive Unleashed A00
Add PDMA driver, sf-pdma, to enable DMA engine on HiFive Unleashed
Rev A00 board.
- Implement dmaengine APIs, support MEM_TO_MEM async copy.
- Tested by DMA Test client
- Supports 4 channels DMA, each channel has 1 done and 1 err
interrupt connected to platform-level interrupt controller (PLIC).
- Depends on DMA_ENGINE and DMA_VIRTUAL_CHANNELS
The datasheet is here:
https://static.dev.sifive.com/FU540-C000-v1.0.pdf
Follow the DMAengine controller doc,
"./Documentation/driver-api/dmaengine/provider.rst" to implement DMA
engine. And use the dma test client in doc,
"./Documentation/driver-api/dmaengine/dmatest.rst", to test.
Each DMA channel has separate HW regs and support done and error ISRs.
4 channels share 1 done and 1 err ISRs. There's no expander/arbitrator
in DMA HW.
-- --
||--< done 23 >--|ch 0|
||--< err 24 >--|| (dma0chan0)
|| --
|| --
||--< done 25 >--|ch 1|
||--< err 26 >--|| (dma0chan1)
|PLIC| --
|| --
||--< done 27 >--|ch 2|
||--< err 28 >--|| (dma0chan2)
|| --
|| --
||--< done 29 >--|ch 3|
||--< err 30 >--| | (dma0chan3)
-- --
Signed-off-by: Green Wan
---
drivers/dma/Kconfig | 2 +
drivers/dma/Makefile | 1 +
drivers/dma/sf-pdma/Kconfig | 6 +
drivers/dma/sf-pdma/Makefile | 1 +
drivers/dma/sf-pdma/sf-pdma.c | 601 ++
drivers/dma/sf-pdma/sf-pdma.h | 124 +++
6 files changed, 735 insertions(+)
create mode 100644 drivers/dma/sf-pdma/Kconfig
create mode 100644 drivers/dma/sf-pdma/Makefile
create mode 100644 drivers/dma/sf-pdma/sf-pdma.c
create mode 100644 drivers/dma/sf-pdma/sf-pdma.h
diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
index 7af874b69ffb..03dc82094857 100644
--- a/drivers/dma/Kconfig
+++ b/drivers/dma/Kconfig
@@ -661,6 +661,8 @@ source "drivers/dma/qcom/Kconfig"
source "drivers/dma/dw/Kconfig"
+source "drivers/dma/sf-pdma/Kconfig"
+
source "drivers/dma/dw-edma/Kconfig"
source "drivers/dma/hsu/Kconfig"
diff --git a/drivers/dma/Makefile b/drivers/dma/Makefile
index f5ce8665e944..4bbd90563ede 100644
--- a/drivers/dma/Makefile
+++ b/drivers/dma/Makefile
@@ -28,6 +28,7 @@ obj-$(CONFIG_DMA_SUN4I) += sun4i-dma.o
obj-$(CONFIG_DMA_SUN6I) += sun6i-dma.o
obj-$(CONFIG_DW_AXI_DMAC) += dw-axi-dmac/
obj-$(CONFIG_DW_DMAC_CORE) += dw/
+obj-$(CONFIG_SF_PDMA) += sf-pdma/
obj-$(CONFIG_DW_EDMA) += dw-edma/
obj-$(CONFIG_EP93XX_DMA) += ep93xx_dma.o
obj-$(CONFIG_FSL_DMA) += fsldma.o
diff --git a/drivers/dma/sf-pdma/Kconfig b/drivers/dma/sf-pdma/Kconfig
new file mode 100644
index ..0e01a5728a79
--- /dev/null
+++ b/drivers/dma/sf-pdma/Kconfig
@@ -0,0 +1,6 @@
+config SF_PDMA
+ bool "Sifive PDMA controller driver"
+ select DMA_ENGINE
+ select DMA_VIRTUAL_CHANNELS
+ help
+ Support the SiFive PDMA controller.
diff --git a/drivers/dma/sf-pdma/Makefile b/drivers/dma/sf-pdma/Makefile
new file mode 100644
index ..764552ab8d0a
--- /dev/null
+++ b/drivers/dma/sf-pdma/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_SF_PDMA) += sf-pdma.o
diff --git a/drivers/dma/sf-pdma/sf-pdma.c b/drivers/dma/sf-pdma/sf-pdma.c
new file mode 100644
index ..70197ad95c1a
--- /dev/null
+++ b/drivers/dma/sf-pdma/sf-pdma.c
@@ -0,0 +1,601 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/**
+ * SiFive FU540 Platform DMA driver
+ * Copyright (C) 2019 SiFive
+ *
+ * Based partially on:
+ * - drivers/dma/fsl-edma.c
+ * - drivers/dma/dw-edma/
+ * - drivers/dma/pxa-dma.c
+ *
+ * See the following sources for further documentation:
+ * - Chapter 12 "Platform DMA Engine (PDMA)" of
+ * SiFive FU540-C000 v1.0
+ * https://static.dev.sifive.com/FU540-C000-v1.0.pdf
+ */
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include "sf-pdma.h"
+#include "../dmaengine.h"
+#include "../virt-dma.h"
+
+#define SIFIVE_PDMA_NAME "sf-pdma"
+
+#ifndef readq
+static inline unsigned long long readq(void __iomem *addr)
+{
+ return readl(addr) | (((unsigned long long)readl(addr + 4)) << 32LL);
+}
+#endif
+
+#ifndef writeq
+static inline void writeq(unsigned long long v, void __iomem *addr)
+{
+ writel(v & 0x, addr);
+ writel(v >> 32, addr + 4);
+}
+#endif
+
+static inline struct sf_pdma_chan *to_sf_pdma_chan(struct dma_chan *dchan)
+{
+ return container_of(dchan, struct sf_pdma_chan, vchan.chan);
+}
+
+static inline struct sf_pdma_desc *to_sf_pdma_desc(struct virt_
[PATCH v4 1/4] dt-bindings: dmaengine: sf-pdma: add bindins for SiFive PDMA
Add DT bindings document for Platform DMA(PDMA) driver of board,
HiFive Unleashed Rev A00.
Signed-off-by: Green Wan
---
.../bindings/dma/sifive,fu540-c000-pdma.yaml | 55 +++
1 file changed, 55 insertions(+)
create mode 100644
Documentation/devicetree/bindings/dma/sifive,fu540-c000-pdma.yaml
diff --git a/Documentation/devicetree/bindings/dma/sifive,fu540-c000-pdma.yaml
b/Documentation/devicetree/bindings/dma/sifive,fu540-c000-pdma.yaml
new file mode 100644
index ..2ca3ddbe1ff4
--- /dev/null
+++ b/Documentation/devicetree/bindings/dma/sifive,fu540-c000-pdma.yaml
@@ -0,0 +1,55 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/dma/sifive,fu540-c000-pdma.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: SiFive Unleashed Rev C000 Platform DMA
+
+maintainers:
+ - Green Wan
+ - Palmer Debbelt
+ - Paul Walmsley
+
+description: |
+ Platform DMA is a DMA engine of SiFive Unleashed. It supports 4
+ channels. Each channel has 2 interrupts. One is for DMA done and
+ the other is for DME error.
+
+ In different SoC, DMA could be attached to different IRQ line.
+ DT file need to be changed to meet the difference. For technical
+ doc,
+
+ https://static.dev.sifive.com/FU540-C000-v1.0.pdf
+
+properties:
+ compatible:
+items:
+ - const: sifive,fu540-c000-pdma
+
+ reg:
+maxItems: 1
+
+ interrupts:
+minItems: 1
+maxItems: 8
+
+ '#dma-cells':
+const: 1
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - '#dma-cells'
+
+examples:
+ - |
+dma@300 {
+ compatible = "sifive,fu540-c000-pdma";
+ reg = <0x0 0x300 0x0 0x8000>;
+ interrupts = <23 24 25 26 27 28 29 30>;
+ #dma-cells = <1>;
+};
+
+...
--
2.17.1
[PATCH v4 0/4] dmaengine: sf-pdma: Add platform dma driver
Add PDMA driver support for SiFive HiFive Unleashed RevA00 board. Mainly follows DMAengine controller doc[1] to implement and take other DMA drivers as reference. Such as - drivers/dma/fsl-edma.c - drivers/dma/dw-edma/ - drivers/dma/pxa-dma.c Using DMA test client[2] to test. Detailed datasheet is doc[3]. Driver supports: - 4 physical DMA channels, share same DONE and error interrupt handler. - Support MEM_TO_MEM - Tested by DMA test client - patches include DT Bindgins document and dts for fu450-c000 SoC. Separate dts patch for easier review and apply to different branch or SoC platform. - retry 1 time if DMA error occurs. [Reference Doc] [1] ./Documentation/driver-api/dmaengine/provider.rst [2] ./Documentation/driver-api/dmaengine/dmatest.rst [3] https://static.dev.sifive.com/FU540-C000-v1.0.pdf [Simple steps to test of DMA Test client] $ echo 1 > /sys/module/dmatest/parameters/iterations $ echo dma0chan0 > /sys/module/dmatest/parameters/channel $ echo dma0chan1 > /sys/module/dmatest/parameters/channel $ echo dma0chan2 > /sys/module/dmatest/parameters/channel $ echo dma0chan3 > /sys/module/dmatest/parameters/channel $ echo 1 > /sys/module/dmatest/parameters/run [Expected test result] [ 267.563323] dmatest: dma0chan0-copy0: summary 45629 tests, 0 failures 38769.01 iops 309661 KB/s (0) [ 267.572427] dmatest: dma0chan1-copy0: summary 45863 tests, 0 failures 40286.85 iops 321643 KB/s (0) [ 267.581392] dmatest: dma0chan2-copy0: summary 45975 tests, 0 failures 41178.48 iops 328740 KB/s (0) [ 267.590542] dmatest: dma0chan3-copy0: summary 44768 tests, 0 failures 38560.29 iops 307726 KB/s (0) Green Wan (4): dt-bindings: dmaengine: sf-pdma: add bindins for SiFive PDMA riscv: dts: add support for PDMA device of HiFive Unleashed Rev A00 dmaengine: sf-pdma: add platform DMA support for HiFive Unleashed A00 MAINTAINERS: Add Green as SiFive PDMA driver maintainer .../bindings/dma/sifive,fu540-c000-pdma.yaml | 55 ++ MAINTAINERS | 6 + arch/riscv/boot/dts/sifive/fu540-c000.dtsi| 7 + drivers/dma/Kconfig | 2 + drivers/dma/Makefile | 1 + drivers/dma/sf-pdma/Kconfig | 6 + drivers/dma/sf-pdma/Makefile | 1 + drivers/dma/sf-pdma/sf-pdma.c | 601 ++ drivers/dma/sf-pdma/sf-pdma.h | 124 9 files changed, 803 insertions(+) create mode 100644 Documentation/devicetree/bindings/dma/sifive,fu540-c000-pdma.yaml create mode 100644 drivers/dma/sf-pdma/Kconfig create mode 100644 drivers/dma/sf-pdma/Makefile create mode 100644 drivers/dma/sf-pdma/sf-pdma.c create mode 100644 drivers/dma/sf-pdma/sf-pdma.h -- 2.17.1
[PATCH v3] Input: atmel_mxt_ts - Disable IRQ across suspend
Across suspend and resume, we are seeing error messages like the following: atmel_mxt_ts i2c-PRP0001:00: __mxt_read_reg: i2c transfer failed (-121) atmel_mxt_ts i2c-PRP0001:00: Failed to read T44 and T5 (-121) This occurs because the driver leaves its IRQ enabled. Upon resume, there is an IRQ pending, but the interrupt is serviced before both the driver and the underlying I2C bus have been resumed. This causes EREMOTEIO errors. Disable the IRQ in suspend, and re-enable it on resume. If there are cases where the driver enters suspend with interrupts disabled, that's a bug we should fix separately. Signed-off-by: Evan Green --- Changes in v3: - Move enable_irq to the beginning of resume (Dmitry) Changes in v2: - Enable and disable unconditionally (Dmitry) drivers/input/touchscreen/atmel_mxt_ts.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c index 24c4b691b1c9..1627fcb27f35 100644 --- a/drivers/input/touchscreen/atmel_mxt_ts.c +++ b/drivers/input/touchscreen/atmel_mxt_ts.c @@ -3155,6 +3155,7 @@ static int __maybe_unused mxt_suspend(struct device *dev) mxt_stop(data); mutex_unlock(_dev->mutex); + disable_irq(data->irq); return 0; } @@ -3168,6 +3169,7 @@ static int __maybe_unused mxt_resume(struct device *dev) if (!input_dev) return 0; + enable_irq(data->irq); mutex_lock(_dev->mutex); if (input_dev->users) -- 2.21.0
Re: [PATCH v2] Input: atmel_mxt_ts - Disable IRQ across suspend
On Fri, Sep 27, 2019 at 5:16 PM Dmitry Torokhov wrote: > > Hi Evan, > > On Tue, Sep 24, 2019 at 02:52:38PM -0700, Evan Green wrote: > > Across suspend and resume, we are seeing error messages like the following: > > > > atmel_mxt_ts i2c-PRP0001:00: __mxt_read_reg: i2c transfer failed (-121) > > atmel_mxt_ts i2c-PRP0001:00: Failed to read T44 and T5 (-121) > > > > This occurs because the driver leaves its IRQ enabled. Upon resume, there > > is an IRQ pending, but the interrupt is serviced before both the driver and > > the underlying I2C bus have been resumed. This causes EREMOTEIO errors. > > > > Disable the IRQ in suspend, and re-enable it on resume. If there are cases > > where the driver enters suspend with interrupts disabled, that's a bug we > > should fix separately. > > > > Signed-off-by: Evan Green > > --- > > > > Changes in v2: > > - Enable and disable unconditionally (Dmitry) > > > > drivers/input/touchscreen/atmel_mxt_ts.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c > > b/drivers/input/touchscreen/atmel_mxt_ts.c > > index 24c4b691b1c9..a58092488679 100644 > > --- a/drivers/input/touchscreen/atmel_mxt_ts.c > > +++ b/drivers/input/touchscreen/atmel_mxt_ts.c > > @@ -3155,6 +3155,7 @@ static int __maybe_unused mxt_suspend(struct device > > *dev) > > mxt_stop(data); > > > > mutex_unlock(_dev->mutex); > > + disable_irq(data->irq); > > > > return 0; > > } > > @@ -3174,6 +3175,7 @@ static int __maybe_unused mxt_resume(struct device > > *dev) > > mxt_start(data); > > > > mutex_unlock(_dev->mutex); > > + enable_irq(data->irq); > > At least for older devices that do soft reset on resume we need > interrupts to already work when we call mxt_start(). > > In general, order of resume steps should mirror suspend. Ok, I can move the enable_irq up towards the top of resume. I was worried that a pending IRQ might not get handled correctly if mxt_start() hadn't been called yet. But if we need IRQs for mxt_start() to run anyway, then I guess it should be handled. -Evan
[PATCH v2] Input: atmel_mxt_ts - Disable IRQ across suspend
Across suspend and resume, we are seeing error messages like the following: atmel_mxt_ts i2c-PRP0001:00: __mxt_read_reg: i2c transfer failed (-121) atmel_mxt_ts i2c-PRP0001:00: Failed to read T44 and T5 (-121) This occurs because the driver leaves its IRQ enabled. Upon resume, there is an IRQ pending, but the interrupt is serviced before both the driver and the underlying I2C bus have been resumed. This causes EREMOTEIO errors. Disable the IRQ in suspend, and re-enable it on resume. If there are cases where the driver enters suspend with interrupts disabled, that's a bug we should fix separately. Signed-off-by: Evan Green --- Changes in v2: - Enable and disable unconditionally (Dmitry) drivers/input/touchscreen/atmel_mxt_ts.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c b/drivers/input/touchscreen/atmel_mxt_ts.c index 24c4b691b1c9..a58092488679 100644 --- a/drivers/input/touchscreen/atmel_mxt_ts.c +++ b/drivers/input/touchscreen/atmel_mxt_ts.c @@ -3155,6 +3155,7 @@ static int __maybe_unused mxt_suspend(struct device *dev) mxt_stop(data); mutex_unlock(_dev->mutex); + disable_irq(data->irq); return 0; } @@ -3174,6 +3175,7 @@ static int __maybe_unused mxt_resume(struct device *dev) mxt_start(data); mutex_unlock(_dev->mutex); + enable_irq(data->irq); return 0; } -- 2.21.0
[PATCH] Input: atmel_mxt_ts - Disable IRQ across suspend
Across suspend and resume, we are seeing error messages like the following:
atmel_mxt_ts i2c-PRP0001:00: __mxt_read_reg: i2c transfer failed (-121)
atmel_mxt_ts i2c-PRP0001:00: Failed to read T44 and T5 (-121)
This occurs because the driver leaves its IRQ enabled. Upon resume, there
is an IRQ pending, but the interrupt is serviced before both the driver and
the underlying I2C bus have been resumed. This causes EREMOTEIO errors.
Disable the IRQ in suspend, and re-enable it if it was previously enabled
in resume.
Signed-off-by: Evan Green
---
drivers/input/touchscreen/atmel_mxt_ts.c | 33 +++-
1 file changed, 27 insertions(+), 6 deletions(-)
diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c
b/drivers/input/touchscreen/atmel_mxt_ts.c
index 24c4b691b1c9..19cbcd9767e2 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -291,6 +291,7 @@ struct mxt_data {
u8 xsize;
u8 ysize;
bool in_bootloader;
+ bool irq_enabled;
u16 mem_size;
u8 t100_aux_ampl;
u8 t100_aux_area;
@@ -1185,11 +1186,23 @@ static int mxt_t6_command(struct mxt_data *data, u16
cmd_offset,
return 0;
}
+static void mxt_enable_irq(struct mxt_data *data)
+{
+ enable_irq(data->irq);
+ data->irq_enabled = true;
+}
+
+static void mxt_disable_irq(struct mxt_data *data)
+{
+ disable_irq(data->irq);
+ data->irq_enabled = false;
+}
+
static int mxt_acquire_irq(struct mxt_data *data)
{
int error;
- enable_irq(data->irq);
+ mxt_enable_irq(data);
error = mxt_process_messages_until_invalid(data);
if (error)
@@ -1205,7 +1218,7 @@ static int mxt_soft_reset(struct mxt_data *data)
dev_info(dev, "Resetting device\n");
- disable_irq(data->irq);
+ mxt_disable_irq(data);
reinit_completion(>reset_completion);
@@ -2807,7 +2820,7 @@ static int mxt_load_fw(struct device *dev, const char *fn)
mxt_free_input_device(data);
mxt_free_object_table(data);
} else {
- enable_irq(data->irq);
+ mxt_enable_irq(data);
}
reinit_completion(>bl_completion);
@@ -2882,7 +2895,7 @@ static int mxt_load_fw(struct device *dev, const char *fn)
data->in_bootloader = false;
disable_irq:
- disable_irq(data->irq);
+ mxt_disable_irq(data);
release_firmware:
release_firmware(fw);
return ret;
@@ -3101,7 +3114,7 @@ static int mxt_probe(struct i2c_client *client, const
struct i2c_device_id *id)
return error;
}
- disable_irq(client->irq);
+ mxt_disable_irq(data);
if (data->reset_gpio) {
msleep(MXT_RESET_GPIO_TIME);
@@ -3132,7 +3145,7 @@ static int mxt_remove(struct i2c_client *client)
{
struct mxt_data *data = i2c_get_clientdata(client);
- disable_irq(data->irq);
+ mxt_disable_irq(data);
sysfs_remove_group(>dev.kobj, _attr_group);
mxt_free_input_device(data);
mxt_free_object_table(data);
@@ -3156,6 +3169,11 @@ static int __maybe_unused mxt_suspend(struct device *dev)
mutex_unlock(_dev->mutex);
+ /*
+* Disable the IRQ directly since the boolean will be used to restore
+* the IRQ state on resume.
+*/
+ disable_irq(data->irq);
return 0;
}
@@ -3168,6 +3186,9 @@ static int __maybe_unused mxt_resume(struct device *dev)
if (!input_dev)
return 0;
+ if (data->irq_enabled)
+ enable_irq(data->irq);
+
mutex_lock(_dev->mutex);
if (input_dev->users)
--
2.21.0
[PATCH v3 3/3] dmaengine: sf-pdma: add platform DMA support for HiFive Unleashed A00
Link: https://www.kernel.org/doc/html/v4.17/driver-api/dmaengine/
Link: https://static.dev.sifive.com/FU540-C000-v1.0.pdf
Add PDMA driver, sf-pdma, to enable DMA engine on HiFive Unleashed
Rev A00 board.
- Implement dmaengine APIs, support MEM_TO_MEM async copy.
- Tested by DMA Test client
- Supports 4 channels DMA, each channel has 1 done and 1 err
interrupt connected to platform-level interrupt controller (PLIC).
- Depends on DMA_ENGINE and DMA_VIRTUAL_CHANNELS
Follow the DMAengine controller doc,
"./Documentation/driver-api/dmaengine/provider.rst" to implement DMA
engine. And use the dma test client in doc,
"./Documentation/driver-api/dmaengine/dmatest.rst", to test.
Each DMA channel has separate HW regs and support done and error ISRs.
4 channels share 1 done and 1 err ISRs. There's no expander/arbitrator
in DMA HW.
-- --
||--< done 23 >--|ch 0|
||--< err 24 >--|| (dma0chan0)
|| --
|| --
||--< done 25 >--|ch 1|
||--< err 26 >--|| (dma0chan1)
|PLIC| --
|| --
||--< done 27 >--|ch 2|
||--< err 28 >--|| (dma0chan2)
|| --
|| --
||--< done 29 >--|ch 3|
||--< err 30 >--| | (dma0chan3)
-- --
Signed-off-by: Green Wan
---
MAINTAINERS | 1 +
drivers/dma/Kconfig | 2 +
drivers/dma/Makefile | 1 +
drivers/dma/sf-pdma/Kconfig | 6 +
drivers/dma/sf-pdma/Makefile | 1 +
drivers/dma/sf-pdma/sf-pdma.c | 623 ++
drivers/dma/sf-pdma/sf-pdma.h | 124 +++
7 files changed, 758 insertions(+)
create mode 100644 drivers/dma/sf-pdma/Kconfig
create mode 100644 drivers/dma/sf-pdma/Makefile
create mode 100644 drivers/dma/sf-pdma/sf-pdma.c
create mode 100644 drivers/dma/sf-pdma/sf-pdma.h
diff --git a/MAINTAINERS b/MAINTAINERS
index d0caa09a479e..c5f0662c9106 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14594,6 +14594,7 @@ F: drivers/media/mmc/siano/
SIFIVE PDMA DRIVER
M: Green Wan
S: Maintained
+F: drivers/dma/sf-pdma/
F: Documentation/devicetree/bindings/dma/sifive,fu540-c000-pdma.yaml
SIFIVE DRIVERS
diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
index 413efef5fbb6..f05a928f5a3d 100644
--- a/drivers/dma/Kconfig
+++ b/drivers/dma/Kconfig
@@ -667,6 +667,8 @@ source "drivers/dma/qcom/Kconfig"
source "drivers/dma/dw/Kconfig"
+source "drivers/dma/sf-pdma/Kconfig"
+
source "drivers/dma/dw-edma/Kconfig"
source "drivers/dma/hsu/Kconfig"
diff --git a/drivers/dma/Makefile b/drivers/dma/Makefile
index 5bddf6f8790f..77a552d970ae 100644
--- a/drivers/dma/Makefile
+++ b/drivers/dma/Makefile
@@ -29,6 +29,7 @@ obj-$(CONFIG_DMA_SUN4I) += sun4i-dma.o
obj-$(CONFIG_DMA_SUN6I) += sun6i-dma.o
obj-$(CONFIG_DW_AXI_DMAC) += dw-axi-dmac/
obj-$(CONFIG_DW_DMAC_CORE) += dw/
+obj-$(CONFIG_SF_PDMA) += sf-pdma/
obj-$(CONFIG_DW_EDMA) += dw-edma/
obj-$(CONFIG_EP93XX_DMA) += ep93xx_dma.o
obj-$(CONFIG_FSL_DMA) += fsldma.o
diff --git a/drivers/dma/sf-pdma/Kconfig b/drivers/dma/sf-pdma/Kconfig
new file mode 100644
index ..0e01a5728a79
--- /dev/null
+++ b/drivers/dma/sf-pdma/Kconfig
@@ -0,0 +1,6 @@
+config SF_PDMA
+ bool "Sifive PDMA controller driver"
+ select DMA_ENGINE
+ select DMA_VIRTUAL_CHANNELS
+ help
+ Support the SiFive PDMA controller.
diff --git a/drivers/dma/sf-pdma/Makefile b/drivers/dma/sf-pdma/Makefile
new file mode 100644
index ..764552ab8d0a
--- /dev/null
+++ b/drivers/dma/sf-pdma/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_SF_PDMA) += sf-pdma.o
diff --git a/drivers/dma/sf-pdma/sf-pdma.c b/drivers/dma/sf-pdma/sf-pdma.c
new file mode 100644
index ..0ebf8a819811
--- /dev/null
+++ b/drivers/dma/sf-pdma/sf-pdma.c
@@ -0,0 +1,623 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/**
+ * SiFive FU540 Platform DMA driver
+ * Copyright (C) 2019 SiFive
+ *
+ * Based partially on:
+ * - drivers/dma/fsl-edma.c
+ * - drivers/dma/dw-edma/
+ * - drivers/dma/pxa-dma.c
+ *
+ * See the following sources for further documentation:
+ * - Chapter 12 "Platform DMA Engine (PDMA)" of
+ * SiFive FU540-C000 v1.0
+ * https://static.dev.sifive.com/FU540-C000-v1.0.pdf
+ */
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include "sf-pdma.h"
+#include "../dmaengine.h"
+#include "../virt-dma.h"
+
+#define SIFIVE_PDMA_NAME "sf-pdma"
+
+#ifndef readq
+static inline unsigned long long readq(void __iomem *addr)
+{
+ return readl(addr) | (((unsigned long long)readl(addr + 4)) <
[PATCH v3 2/3] riscv: dts: add support for PDMA device of HiFive Unleashed Rev A00
Add PDMA support to (arch/riscv/boot/dts/sifive/fu540-c000.dtsi)
Signed-off-by: Green Wan
---
arch/riscv/boot/dts/sifive/fu540-c000.dtsi | 7 +++
1 file changed, 7 insertions(+)
diff --git a/arch/riscv/boot/dts/sifive/fu540-c000.dtsi
b/arch/riscv/boot/dts/sifive/fu540-c000.dtsi
index 42b5ec223100..d3030d7fb45c 100644
--- a/arch/riscv/boot/dts/sifive/fu540-c000.dtsi
+++ b/arch/riscv/boot/dts/sifive/fu540-c000.dtsi
@@ -164,6 +164,13 @@
clocks = < PRCI_CLK_TLCLK>;
status = "disabled";
};
+ dma: dma@300 {
+ compatible = "sifive,fu540-c000-pdma";
+ reg = <0x0 0x300 0x0 0x8000>;
+ interrupt-parent = <>;
+ interrupts = <23 24 25 26 27 28 29 30>;
+ #dma-cells = <1>;
+ };
uart1: serial@10011000 {
compatible = "sifive,fu540-c000-uart", "sifive,uart0";
reg = <0x0 0x10011000 0x0 0x1000>;
--
2.17.1
[PATCH v3 1/3] dt-bindings: dmaengine: sf-pdma: add bindins for SiFive PDMA
Add DT bindings document for Platform DMA(PDMA) driver of board,
HiFive Unleashed Rev A00.
Signed-off-by: Green Wan
---
.../bindings/dma/sifive,fu540-c000-pdma.yaml | 55 +++
MAINTAINERS | 5 ++
2 files changed, 60 insertions(+)
create mode 100644
Documentation/devicetree/bindings/dma/sifive,fu540-c000-pdma.yaml
diff --git a/Documentation/devicetree/bindings/dma/sifive,fu540-c000-pdma.yaml
b/Documentation/devicetree/bindings/dma/sifive,fu540-c000-pdma.yaml
new file mode 100644
index ..3ed015f2b83a
--- /dev/null
+++ b/Documentation/devicetree/bindings/dma/sifive,fu540-c000-pdma.yaml
@@ -0,0 +1,55 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/dma/sifive,fu540-c000-pdma.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: SiFive Unleashed Rev C000 Platform DMA
+
+maintainers:
+ - Green Wan
+ - Palmer Debbelt
+ - Paul Walmsley
+
+description: |
+ Platform DMA is a DMA engine of SiFive Unleashed. It supports 4
+ channels. Each channel has 2 interrupts. One is for DMA done and
+ the other is for DME error.
+
+ In different SoC, DMA could be attached to different IRQ line.
+ DT file need to be changed to meet the difference. For technical
+ doc,
+
+ https://static.dev.sifive.com/FU540-C000-v1.0.pdf
+
+properties:
+ compatible:
+items:
+ - const: sifive,fu540-c000-pdma
+
+ reg:
+maxItems: 1
+
+ interrupts:
+minItems: 8
+maxItems: 8
+
+ '#dma-cells':
+const: 1
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - '#dma-cells'
+
+examples:
+ - |
+dma@300 {
+ compatible = "sifive,fu540-c000-pdma";
+ reg = <0x0 0x300 0x0 0x8000>;
+ interrupts = <23 24 25 26 27 28 29 30>;
+ #dma-cells = <1>;
+};
+
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index 49f75d1b7b51..d0caa09a479e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14591,6 +14591,11 @@ F: drivers/media/usb/siano/
F: drivers/media/usb/siano/
F: drivers/media/mmc/siano/
+SIFIVE PDMA DRIVER
+M: Green Wan
+S: Maintained
+F: Documentation/devicetree/bindings/dma/sifive,fu540-c000-pdma.yaml
+
SIFIVE DRIVERS
M: Palmer Dabbelt
M: Paul Walmsley
--
2.17.1
[PATCH v3 0/3] dmaengine: sf-pdma: Add platform dma driver
Add PDMA driver support for SiFive HiFive Unleashed RevA00 board. Mainly follows DMAengine controller doc[1] to implement and take other DMA drivers as reference. Such as - drivers/dma/fsl-edma.c - drivers/dma/dw-edma/ - drivers/dma/pxa-dma.c Using DMA test client[2] to test. Detailed datasheet is doc[3]. Driver supports: - 4 physical DMA channels, share same DONE and error interrupt handler. - Support MEM_TO_MEM - Tested by DMA test client - patches include DT Bindgins document and dts for fu450-c000 SoC. Separate dts patch for easier review and apply to different branch or SoC platform. - retry 1 time if DMA error occurs. [Reference Doc] [1] ./Documentation/driver-api/dmaengine/provider.rst [2] ./Documentation/driver-api/dmaengine/dmatest.rst [3] https://static.dev.sifive.com/FU540-C000-v1.0.pdf [Simple steps to test of DMA Test client] $ echo 1 > /sys/module/dmatest/parameters/iterations $ echo dma0chan0 > /sys/module/dmatest/parameters/channel $ echo dma0chan1 > /sys/module/dmatest/parameters/channel $ echo dma0chan2 > /sys/module/dmatest/parameters/channel $ echo dma0chan3 > /sys/module/dmatest/parameters/channel $ echo 1 > /sys/module/dmatest/parameters/run [Expected test result] [ 7756.975356] dmatest: dma0chan0-copy0: summary 11208260 tests, 0 failures 36207.82 iops 579325 KB/s (0) [ 7756.984093] dmatest: dma0chan1-copy0: summary 11206263 tests, 0 failures 36007.36 iops 576117 KB/s (0) [ 7756.993453] dmatest: dma0chan2-copy0: summary 10929638 tests, 0 failures 33984.39 iops 543750 KB/s (0) [ 7757.003008] dmatest: dma0chan3-copy0: summary 11204208 tests, 0 failures 35759.65 iops 572154 KB/s (0) Green Wan (3): dt-bindings: dmaengine: sf-pdma: add bindins for SiFive PDMA riscv: dts: add support for PDMA device of HiFive Unleashed Rev A00 dmaengine: sf-pdma: add platform DMA support for HiFive Unleashed A00 .../bindings/dma/sifive,fu540-c000-pdma.yaml | 55 ++ MAINTAINERS | 6 + arch/riscv/boot/dts/sifive/fu540-c000.dtsi| 7 + drivers/dma/Kconfig | 2 + drivers/dma/Makefile | 1 + drivers/dma/sf-pdma/Kconfig | 6 + drivers/dma/sf-pdma/Makefile | 1 + drivers/dma/sf-pdma/sf-pdma.c | 623 ++ drivers/dma/sf-pdma/sf-pdma.h | 124 9 files changed, 825 insertions(+) create mode 100644 Documentation/devicetree/bindings/dma/sifive,fu540-c000-pdma.yaml create mode 100644 drivers/dma/sf-pdma/Kconfig create mode 100644 drivers/dma/sf-pdma/Makefile create mode 100644 drivers/dma/sf-pdma/sf-pdma.c create mode 100644 drivers/dma/sf-pdma/sf-pdma.h -- 2.17.1
[PATCH v2 3/3] dmaengine: sf-pdma: add platform DMA support for HiFive Unleashed A00
Link: https://www.kernel.org/doc/html/v4.17/driver-api/dmaengine/
Link: https://static.dev.sifive.com/FU540-C000-v1.0.pdf
Add PDMA driver, sf-pdma, to enable DMA engine on HiFive Unleashed
Rev A00 board.
- Implement dmaengine APIs, support MEM_TO_MEM async copy.
- Tested by DMA Test client
- Supports 4 channels DMA, each channel has 1 done and 1 err
interrupt connected to platform-level interrupt controller (PLIC).
- Depends on DMA_ENGINE and DMA_VIRTUAL_CHANNELS
Follow the DMAengine controller doc,
"./Documentation/driver-api/dmaengine/provider.rst" to implement DMA
engine. And use the dma test client in doc,
"./Documentation/driver-api/dmaengine/dmatest.rst", to test.
Each DMA channel has separate HW regs and support done and error ISRs.
4 channels share 1 done and 1 err ISRs. There's no expander/arbitrator
in DMA HW.
-- --
||--< done 23 >--|ch 0|
||--< err 24 >--|| (dma0chan0)
|| --
|| --
||--< done 25 >--|ch 1|
||--< err 26 >--|| (dma0chan1)
|PLIC| --
|| --
||--< done 27 >--|ch 2|
||--< err 28 >--|| (dma0chan2)
|| --
|| --
||--< done 29 >--|ch 3|
||--< err 30 >--| | (dma0chan3)
-- --
Signed-off-by: Green Wan
---
MAINTAINERS | 1 +
drivers/dma/Kconfig | 2 +
drivers/dma/Makefile | 1 +
drivers/dma/sf-pdma/Kconfig | 6 +
drivers/dma/sf-pdma/Makefile | 1 +
drivers/dma/sf-pdma/sf-pdma.c | 623 ++
drivers/dma/sf-pdma/sf-pdma.h | 124 +++
7 files changed, 758 insertions(+)
create mode 100644 drivers/dma/sf-pdma/Kconfig
create mode 100644 drivers/dma/sf-pdma/Makefile
create mode 100644 drivers/dma/sf-pdma/sf-pdma.c
create mode 100644 drivers/dma/sf-pdma/sf-pdma.h
diff --git a/MAINTAINERS b/MAINTAINERS
index d0caa09a479e..c5f0662c9106 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14594,6 +14594,7 @@ F: drivers/media/mmc/siano/
SIFIVE PDMA DRIVER
M: Green Wan
S: Maintained
+F: drivers/dma/sf-pdma/
F: Documentation/devicetree/bindings/dma/sifive,fu540-c000-pdma.yaml
SIFIVE DRIVERS
diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
index 413efef5fbb6..f05a928f5a3d 100644
--- a/drivers/dma/Kconfig
+++ b/drivers/dma/Kconfig
@@ -667,6 +667,8 @@ source "drivers/dma/qcom/Kconfig"
source "drivers/dma/dw/Kconfig"
+source "drivers/dma/sf-pdma/Kconfig"
+
source "drivers/dma/dw-edma/Kconfig"
source "drivers/dma/hsu/Kconfig"
diff --git a/drivers/dma/Makefile b/drivers/dma/Makefile
index 5bddf6f8790f..77a552d970ae 100644
--- a/drivers/dma/Makefile
+++ b/drivers/dma/Makefile
@@ -29,6 +29,7 @@ obj-$(CONFIG_DMA_SUN4I) += sun4i-dma.o
obj-$(CONFIG_DMA_SUN6I) += sun6i-dma.o
obj-$(CONFIG_DW_AXI_DMAC) += dw-axi-dmac/
obj-$(CONFIG_DW_DMAC_CORE) += dw/
+obj-$(CONFIG_SF_PDMA) += sf-pdma/
obj-$(CONFIG_DW_EDMA) += dw-edma/
obj-$(CONFIG_EP93XX_DMA) += ep93xx_dma.o
obj-$(CONFIG_FSL_DMA) += fsldma.o
diff --git a/drivers/dma/sf-pdma/Kconfig b/drivers/dma/sf-pdma/Kconfig
new file mode 100644
index ..0e01a5728a79
--- /dev/null
+++ b/drivers/dma/sf-pdma/Kconfig
@@ -0,0 +1,6 @@
+config SF_PDMA
+ bool "Sifive PDMA controller driver"
+ select DMA_ENGINE
+ select DMA_VIRTUAL_CHANNELS
+ help
+ Support the SiFive PDMA controller.
diff --git a/drivers/dma/sf-pdma/Makefile b/drivers/dma/sf-pdma/Makefile
new file mode 100644
index ..764552ab8d0a
--- /dev/null
+++ b/drivers/dma/sf-pdma/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_SF_PDMA) += sf-pdma.o
diff --git a/drivers/dma/sf-pdma/sf-pdma.c b/drivers/dma/sf-pdma/sf-pdma.c
new file mode 100644
index ..0ebf8a819811
--- /dev/null
+++ b/drivers/dma/sf-pdma/sf-pdma.c
@@ -0,0 +1,623 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/**
+ * SiFive FU540 Platform DMA driver
+ * Copyright (C) 2019 SiFive
+ *
+ * Based partially on:
+ * - drivers/dma/fsl-edma.c
+ * - drivers/dma/dw-edma/
+ * - drivers/dma/pxa-dma.c
+ *
+ * See the following sources for further documentation:
+ * - Chapter 12 "Platform DMA Engine (PDMA)" of
+ * SiFive FU540-C000 v1.0
+ * https://static.dev.sifive.com/FU540-C000-v1.0.pdf
+ */
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include "sf-pdma.h"
+#include "../dmaengine.h"
+#include "../virt-dma.h"
+
+#define SIFIVE_PDMA_NAME "sf-pdma"
+
+#ifndef readq
+static inline unsigned long long readq(void __iomem *addr)
+{
+ return readl(addr) | (((unsigned long long)readl(addr + 4)) <
[PATCH v2 2/3] riscv: dts: add support for PDMA device of HiFive Unleashed Rev A00
Add PDMA support to (arch/riscv/boot/dts/sifive/fu540-c000.dtsi)
Signed-off-by: Green Wan
---
arch/riscv/boot/dts/sifive/fu540-c000.dtsi | 7 +++
1 file changed, 7 insertions(+)
diff --git a/arch/riscv/boot/dts/sifive/fu540-c000.dtsi
b/arch/riscv/boot/dts/sifive/fu540-c000.dtsi
index 42b5ec223100..d3030d7fb45c 100644
--- a/arch/riscv/boot/dts/sifive/fu540-c000.dtsi
+++ b/arch/riscv/boot/dts/sifive/fu540-c000.dtsi
@@ -164,6 +164,13 @@
clocks = < PRCI_CLK_TLCLK>;
status = "disabled";
};
+ dma: dma@300 {
+ compatible = "sifive,fu540-c000-pdma";
+ reg = <0x0 0x300 0x0 0x8000>;
+ interrupt-parent = <>;
+ interrupts = <23 24 25 26 27 28 29 30>;
+ #dma-cells = <1>;
+ };
uart1: serial@10011000 {
compatible = "sifive,fu540-c000-uart", "sifive,uart0";
reg = <0x0 0x10011000 0x0 0x1000>;
--
2.17.1
[PATCH v2 1/3] dt-bindings: dmaengine: sf-pdma: add bindins for SiFive PDMA
Add DT bindings document for Platform DMA(PDMA) driver of board,
HiFive Unleashed Rev A00.
Signed-off-by: Green Wan
---
.../bindings/dma/sifive,fu540-c000-pdma.yaml | 63 +++
MAINTAINERS | 5 ++
2 files changed, 68 insertions(+)
create mode 100644
Documentation/devicetree/bindings/dma/sifive,fu540-c000-pdma.yaml
diff --git a/Documentation/devicetree/bindings/dma/sifive,fu540-c000-pdma.yaml
b/Documentation/devicetree/bindings/dma/sifive,fu540-c000-pdma.yaml
new file mode 100644
index ..b5423f1cfcaf
--- /dev/null
+++ b/Documentation/devicetree/bindings/dma/sifive,fu540-c000-pdma.yaml
@@ -0,0 +1,63 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/dma/sifive,fu540-c000-pdma.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: SiFive Unleashed Rev C000 Platform DMA
+
+maintainers:
+ - Green Wan
+ - Palmer Debbelt
+ - Paul Walmsley
+
+description: |
+ Platform DMA is a DMA engine of SiFive Unleashed. It supports 4
+ channels. Each channel has 2 interrupts. One is for DMA done and
+ the other is for DME error.
+
+ In different SoC, DMA could be attached to different IRQ line.
+ DT file need to be changed to meet the difference. For technical
+ doc,
+
+ https://static.dev.sifive.com/FU540-C000-v1.0.pdf
+
+properties:
+ compatible:
+items:
+ - const: sifive,fu540-c000-pdma
+
+ reg:
+maxItems: 1
+
+ interrupts:
+minItems: 8
+maxItems: 8
+
+ interrupt-parent:
+description:
+ Interrupt parent must correspond to the name PLIC interrupt
+ controller, i.e. "plic0"
+maxItems: 1
+
+ '#dma-cells':
+const: 1
+
+required:
+ - compatible
+ - reg
+ - interrupt-parent
+ - interrupts
+ - '#dma-cells'
+
+examples:
+ - |
+dma@300 {
+ compatible = "sifive,fu540-c000-pdma";
+ reg = <0x0 0x300 0x0 0x8000>;
+ interrupt-parent = <>;
+ interrupts = <23 24 25 26 27 28 29 30>;
+ #dma-cells = <1>;
+};
+
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index 49f75d1b7b51..d0caa09a479e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14591,6 +14591,11 @@ F: drivers/media/usb/siano/
F: drivers/media/usb/siano/
F: drivers/media/mmc/siano/
+SIFIVE PDMA DRIVER
+M: Green Wan
+S: Maintained
+F: Documentation/devicetree/bindings/dma/sifive,fu540-c000-pdma.yaml
+
SIFIVE DRIVERS
M: Palmer Dabbelt
M: Paul Walmsley
--
2.17.1
[PATCH v2 0/3] dmaengine: sf-pdma: Add platform dma driver
Add PDMA driver support for SiFive HiFive Unleashed RevA00 board. Mainly follows DMAengine controller doc[1] to implement and take other DMA drivers as reference. Such as - drivers/dma/fsl-edma.c - drivers/dma/dw-edma/ - drivers/dma/pxa-dma.c Using DMA test client[2] to test. Detailed datasheet is doc[3]. Driver supports: - 4 physical DMA channels, share same DONE and error interrupt handler. - Support MEM_TO_MEM - Tested by DMA test client - patches include DT Bindgins document and dts for fu450-c000 SoC. Separate dts patch for easier review and apply to different branch or SoC platform. - retry 1 time if DMA error occurs. [Reference Doc] [1] ./Documentation/driver-api/dmaengine/provider.rst [2] ./Documentation/driver-api/dmaengine/dmatest.rst [3] https://static.dev.sifive.com/FU540-C000-v1.0.pdf [Simple steps to test of DMA Test client] $ echo 1 > /sys/module/dmatest/parameters/iterations $ echo dma0chan0 > /sys/module/dmatest/parameters/channel $ echo dma0chan1 > /sys/module/dmatest/parameters/channel $ echo dma0chan2 > /sys/module/dmatest/parameters/channel $ echo dma0chan3 > /sys/module/dmatest/parameters/channel $ echo 1 > /sys/module/dmatest/parameters/run Green Wan (3): dt-bindings: dmaengine: sf-pdma: add bindins for SiFive PDMA riscv: dts: add support for PDMA device of HiFive Unleashed Rev A00 dmaengine: sf-pdma: add platform DMA support for HiFive Unleashed A00 .../bindings/dma/sifive,fu540-c000-pdma.yaml | 63 ++ MAINTAINERS | 6 + arch/riscv/boot/dts/sifive/fu540-c000.dtsi| 7 + drivers/dma/Kconfig | 2 + drivers/dma/Makefile | 1 + drivers/dma/sf-pdma/Kconfig | 6 + drivers/dma/sf-pdma/Makefile | 1 + drivers/dma/sf-pdma/sf-pdma.c | 623 ++ drivers/dma/sf-pdma/sf-pdma.h | 124 9 files changed, 833 insertions(+) create mode 100644 Documentation/devicetree/bindings/dma/sifive,fu540-c000-pdma.yaml create mode 100644 drivers/dma/sf-pdma/Kconfig create mode 100644 drivers/dma/sf-pdma/Makefile create mode 100644 drivers/dma/sf-pdma/sf-pdma.c create mode 100644 drivers/dma/sf-pdma/sf-pdma.h -- 2.17.1
Re: [PATCH v3 5/5] soc: qcom: cmd-db: Map with read-only mappings
On Tue, Sep 10, 2019 at 9:09 AM Stephen Boyd wrote:
>
> The command DB is read-only already to the kernel because everything is
> const marked once we map it. Let's go one step further and try to map
> the memory as read-only in the page tables. This should make it harder
> for random code to corrupt the database and change the contents.
>
> Cc: Evan Green
> Cc: Rob Herring
> Cc: Bjorn Andersson
> Cc: Andy Gross
> Cc: Will Deacon
> Cc: Catalin Marinas
> Cc: Dan Williams
> Reviewed-by: Bjorn Andersson
> Signed-off-by: Stephen Boyd
> ---
> drivers/soc/qcom/cmd-db.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/soc/qcom/cmd-db.c b/drivers/soc/qcom/cmd-db.c
> index 10a34d26b753..6365e8260282 100644
> --- a/drivers/soc/qcom/cmd-db.c
> +++ b/drivers/soc/qcom/cmd-db.c
> @@ -240,7 +240,8 @@ static int cmd_db_dev_probe(struct platform_device *pdev)
> {
> int ret = 0;
>
> - cmd_db_header = devm_memremap_reserved_mem(>dev, MEMREMAP_WB);
> + cmd_db_header = devm_memremap_reserved_mem(>dev,
> + MEMREMAP_RO | MEMREMAP_WB);
It seems weird to have both flags, like: "It's read-only, but if it
ever did get written to somehow, make it writeback".
> if (IS_ERR(cmd_db_header)) {
> ret = PTR_ERR(cmd_db_header);
> cmd_db_header = NULL;
> --
> Sent by a computer through tubes
>
