All of lore.kernel.org
 help / color / mirror / Atom feed
From: Bjorn Andersson <bjorn.andersson@sonymobile.com>
To: Josh Cartwright <joshc@codeaurora.org>
Cc: "linux-arm-msm@vger.kernel.org" <linux-arm-msm@vger.kernel.org>
Subject: Re: [PATCH RFC] WIP: mfd: add support for Qualcomm RPM
Date: Tue, 22 Apr 2014 10:53:32 -0700	[thread overview]
Message-ID: <20140422175330.GA25012@sonymobile.com> (raw)
In-Reply-To: <1397168269-6844-1-git-send-email-joshc@codeaurora.org>

On Thu 10 Apr 15:17 PDT 2014, Josh Cartwright wrote:

Hi Josh,

Thanks for posting this RFC.

> The Resource Power Manager (RPM) is responsible managing SoC-wide
> resources (clocks, regulators, etc) on MSM and other Qualcomm SoCs.
> This driver provides an implementation of the message-RAM-based
> communication protocol.
> 
> Note, this is a rewrite of the driver as it exists in the downstream
> tree[1], making a few simplifying assumptions to clean it up, and adding
> device tree support.
> 
> [1]: https://www.codeaurora.org/cgit/quic/la/kernel/msm/tree/arch/arm/mach-msm/rpm.c?h=msm-3.4
> 
> Signed-off-by: Josh Cartwright <joshc@codeaurora.org>
> ---
> This patch is intended to act as a starting point for discussions on how we
> should proceed going forward supporting RPM.  In particular, figuring out how
> to model RPM and it's controlled resources in device tree.

As you know I wrote up a simple driver for this to get regulators a few months
back, so I'm happy to see this discussion.

> 
> I've chosen a path where a subnode logically separates the RPM resources; it's
> intended each set of resources will be controlled by a single driver.  For
> example, an RPM-controlled regulator might consume two RPM_TYPE_REQ resources
> described in 'reg'.

I agree, having each subnode be a 1:1 for a resource exposed by the rpm driver
seems to be the most logical idea. I did an experiment where I made my rpm
driver a mfd with regulator cells, but fell back to just having generic
platform_drivers like you do.


>From the RPMs perspective each logical resource is represented by the following
properties:
1) A consecutive range of addresses used for updating a value (what you call
   REQ)
2) An offset in a bitmap used to indicate what resources are set by the urrent
   write
3) A consecutive range of addresses used for reading the status of a resource
   (what you call STATUS)

For the individual clock client we have:
  4 bytes of requested clock rate

For the individual regulator client we have:
  4 or 8 bytes, depending on regulator type, of a packed struct containing the
  various properties of the regulator.

For msm_bus the data is represented by 29 words, I'm not sure if there's some
additional logic so that not all of these needs to be updated at once, there is
still just one selector bit.

At least the clock and regulators are only ever interested in operating on
entire logical resources. It's arguably never correct to write only 4 bytes for
a ldo update.


The API must therefor describe the entirety of a logical resource, either via
passing a list of req's or by abstracting the rpm details out and simply
exposing logical resources.

> 
> Effectively, this pushes the "generic resource ID" -> "SoC-specific resource
> ID" mapping out of the large data tables that exist in msm-3.4 into the device
> tree.  An alternative approach would be to still maintain the SoC-specific
> tables, and have each node matched to it's resources using a unique compatible
> string.

In my solution I did choose to expose logical resources, that simply maps to a
lookup table where I store offset for writes and reds, the selector bit and the
resource's size.

The rpm doesn't care about the internal representation of the clients data and
the clients doesn't care about the internal representation of the rpm's data.

[snip]
> +Qualcomm Resource Power Manager (RPM)
> +
> +This driver is used to interface with Resource Power Manager (RPM).  The RPM is
> +responsible managing SoC-wide resources (clocks, regulators, etc) on MSM and
> +other Qualcomm chipsets.
> +
> +Required properties:
> +
> +- compatible: must be one of:
> +       "qcom,rpm-apq8064"
> +       "qcom,rpm-ipq8064"
> +
> +- reg: must contain two register specifiers, in the following order:
> +       specifier 0: RPM Message RAM
> +       specifier 1: IPC register
> +
> +- reg-names: must contain the following, in order:
> +       "msg_ram"
> +       "ipc"
> +
> +- interrupts: must contain the following three interrupt specifiers, in order:
> +       specifier 0: RPM Acknowledgement Interrupt
> +       specifier 1: Error Interrupt
> +       specifier 2: Wakeup interrupt
> +
> +- interrupt-names: must contain the following, in order:
> +       "ack"
> +       "err"
> +       "wakeup"
> +
> +- ipc-bit: bit written to the IPC register to notify RPM of a pending request
> +
> +- #address-cells: must be 3
> +       cell 0: offset in ACK and REQ register spaces corresponding to the register
> +       cell 1: type field, one of RPM_TYPE_REQ (0) or RPM_TYPE_STATUS (1)
> +       cell 2: indicates the selector bit to set when writing this register,
> +               this cell is ignored (and should be set to zero) when type is
> +               RPM_TYPE_STATUS

As you've showed below, this requires a common parser-function to be feasable.
The natural place is to put this in the rpm implementation, but I found it
strange to have a function in the rpm driver, taking a pointer to a child node
and operating on that.

> +
> +Example:
> +
> +       #include <dt-bindings/mfd/qcom_rpm.h>
> +
> +       rpm@108000 {
> +               compatible = "qcom,rpm-ipq8064";
> +               reg = <0x00108000 0x1000>,
> +                     <0x02011008 0x4>;
> +               reg-names = "msg_ram",
> +                           "ipc";
> +               interrupts = <GIC_SPI 19 0>,
> +                            <GIC_SPI 21 0>,
> +                            <GIC_SPI 22 0>;
> +               interrupt-names = "ack",
> +                                 "err",
> +                                 "wakeup";
> +               ipc-bit = <2>;
> +
> +               #address-cells = <3>;
> +               #size-cells = <0>;

This looks good.

> +
> +               subnode {
> +                       compatible = "...";
> +                       reg = <464 RPM_TYPE_REQ 30>,
> +                             <468 RPM_TYPE_REQ 30>,
> +                             <118 RPM_TYPE_STATUS 0>;
> +               };
> +       };

It could be argued that these 3 values (base + size) is representing how things
are connected and therefor should be represented in DT. But I believe this is a
leaking abstraction and that this should be considered internal properties of
the mfd.

With a mapping table in the rpm the clients would be exposed to a simple u32
defining the resource to be accessed, this removes the need for any special
parsing function:

               pm8921_ldo16 {
                       compatible = "qcom,rpm-pm8921-ldo"
                       reg = <PM8921_LDO16>;

		       regulator-xxx = <yyy>;
               };


[snip]
> +
> +static const __be32 *qcom_decode_reg_type(struct platform_device *pdev,
> +                                         unsigned int which, unsigned int type)
> +{
> +       const struct device_node *np = pdev->dev.of_node;
> +       const __be32 *cell;
> +       int sz;
> +
> +       cell = of_get_property(np, "reg", &sz);
> +       if (!cell)
> +               return ERR_PTR(-EINVAL);
> +
> +       sz /= 3 * sizeof(u32);
> +
> +       for (; sz--; cell += 3) {
> +
> +               if (be32_to_cpup(&cell[1]) != type)
> +                       continue;
> +
> +               if (!which--)
> +                       return cell;
> +
> +       }
> +
> +       return ERR_PTR(-ENOENT);
> +}
> +
> +int qcom_rpm_get_req(struct platform_device *pdev, unsigned int which,
> +                    struct qcom_rpm_req *req)
> +{
> +       const __be32 *cell;
> +       u32 sel_bit;
> +
> +       cell = qcom_decode_reg_type(pdev, which, RPM_TYPE_REQ);
> +       if (IS_ERR(cell))
> +               return PTR_ERR(cell);
> +
> +       req->offset = be32_to_cpup(&cell[0]);
> +
> +       sel_bit = be32_to_cpup(&cell[2]);
> +
> +       req->sel_reg  = sel_bit / 32;
> +       req->sel_mask = BIT(sel_bit % 32);
> +       return 0;
> +}
> +EXPORT_SYMBOL(qcom_rpm_get_req);

To me it's not clean to have a function in the middle of the rpm driver that's
supposed to operate on the clients pdev.

> +
> +const void __iomem *qcom_rpm_get_status(struct platform_device *pdev,
> +                                       unsigned int which)
> +{
> +       struct qcom_rpm *rpm = qcom_rpm_get(pdev);
> +       const __be32 *cell;
> +
> +       cell = qcom_decode_reg_type(pdev, which, RPM_TYPE_STATUS);
> +       if (IS_ERR(cell))
> +               return (const void __iomem *) cell;
> +
> +       return rpm->status + be32_to_cpup(&cell[0]);
> +}
> +EXPORT_SYMBOL(qcom_rpm_get_status);

I think you want to expose the current status, not just return a pointer that
the clients can deference at unknown times in the future.

I also find it inconsistent that the write function takes a reference to the
rpm device while the read function takes a reference to the client.

[snip]
> +#ifndef QCOM_RPM_H
> +#define QCOM_RPM_H
> +
> +#include <linux/platform_device.h>
> +
> +struct qcom_rpm;
> +
> +static inline struct qcom_rpm *qcom_rpm_get(struct platform_device *pdev)
> +{
> +       struct platform_device *parent;
> +
> +       parent = to_platform_device(pdev->dev.parent);
> +
> +       return platform_get_drvdata(parent);
> +}

Most drivers would hide this in the api, but I think this looks good. The
clients would use this to acquire a qcom_rpm pointer in their probe function,
that they later can used when calling the api.

> +
> +struct qcom_rpm_req {
> +       unsigned int offset;
> +       unsigned int sel_reg;
> +       u32 sel_mask;
> +};

In my view, this is internal matters of the rpm.

> +
> +enum qcom_rpm_context_mask {
> +       QCOM_RPM_CTX_SET_ACTIVE         = BIT(0),
> +       QCOM_RPM_CTX_SET_SLEEP          = BIT(1),
> +       QCOM_RPM_CTX_NOTIFICATION       = BIT(30),
> +       QCOM_RPM_CTX_REJECTED           = BIT(31),

These defines does not belong to the public interface.

> +};
> +
> +int qcom_rpm_write_ctx(struct qcom_rpm *rpm, enum qcom_rpm_context_mask ctx,
> +                      const struct qcom_rpm_req *req, u32 *data, size_t len);
> +
> +static inline int qcom_rpm_req_write(struct qcom_rpm *rpm,
> +                                    const struct qcom_rpm_req *req, u32 data)
> +{
> +       return qcom_rpm_write_ctx(rpm, QCOM_RPM_CTX_SET_ACTIVE, req, &data, 1);

Most resources does not have size 1; e.g. all ldos are size 2 (words) and
should be updated atomically.

For it to make sense to have the child list all req's this api must take a list
of reqs or the api should be that the client pass some base and then a size.
But as you don't have a mapping table or anything to compare this with it's not
possible to detect invalid writes.

> +}
> +
> +static inline int qcom_rpm_req_write_sleep(struct qcom_rpm *rpm,
> +                                          const struct qcom_rpm_req *req,
> +                                          u32 data)
> +{
> +       return qcom_rpm_write_ctx(rpm, QCOM_RPM_CTX_SET_SLEEP, req, &data, 1);
> +}
> +
> +int qcom_rpm_get_req(struct platform_device *pdev, unsigned int which,
> +                    struct qcom_rpm_req *req);
> +
> +const void __iomem *qcom_rpm_get_status(struct platform_device *pdev,
> +                                       unsigned int which);

I would like you to simplify this api to be:

/* pdev is the childs pdev */
struct qcom_rpm *qcom_rpm_get(struct platform_device *pdev);

int qcom_rpm_read(struct qcom_rpm *rpm, resource, void *data, size_t len);
int qcom_rpm_write(struct qcom_rpm *rpm, resource, void *data, size_t len);
int qcom_rpm_write_sleep(struct qcom_rpm *rpm, resource, void *data, size_t len);

The common write function is not supposed to be used by the clients, so this
would be moved into rpm.c


This api is creating an abstraction between what is rpm and what is client
data. Furthermore it holds for the SMD based rpm as well, that has the same
representation for at least clocks. So we have a potential for some re-use.

> +
> +#endif /* QCOM_RPM_H */
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> hosted by The Linux Foundation
> 

I'll post my code to the mailing list as well, for reference.

Regards,
Bjorn

      parent reply	other threads:[~2014-04-22 17:53 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-04-10 22:17 Josh Cartwright
2014-04-11 15:54 ` Kumar Gala
2014-04-22 17:53 ` Bjorn Andersson [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20140422175330.GA25012@sonymobile.com \
    --to=bjorn.andersson@sonymobile.com \
    --cc=joshc@codeaurora.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --subject='Re: [PATCH RFC] WIP: mfd: add support for Qualcomm RPM' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.