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@xxxxxxxxxxxxxx> > --- > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
