| Message ID | 20250313-pcc_fixes_updates-v3-11-019a4aa74d0f@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | mailbox: pcc: Fixes and cleanup/refactoring | expand |
On Thu, Mar 13, 2025 at 03:28:57PM +0000, Sudeep Holla wrote: > The PCC driver now handles mapping and unmapping of shared memory > areas as part of pcc_mbox_{request,free}_channel(). Without these before, > this xgene hwmon driver did handling of those mappings like several > other PCC mailbox client drivers. > > There were redundant operations, leading to unnecessary code. Maintaining > the consistency across these driver was harder due to scattered handling > of shmem. > > Just use the mapped shmem and remove all redundant operations from this > driver. > > Cc: Jean Delvare <jdelvare@suse.com> > Cc: Guenter Roeck <linux@roeck-us.net> > Cc: linux-hwmon@vger.kernel.org > Acked-by: Guenter Roeck <linux@roeck-us.net> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> Not that it matters, but I keep wondering: Why don't people use auxiliary devices for situations like this, and keep the subsystem code where it belongs ? I am not requesting that you do it, I just wonder why the mechanism isn't used. I would have thought that it would be perfect for situations like this, so I guess I must be missing something, and I'd like to understand what that something is. Thanks, Guenter
On Fri, Apr 11, 2025 at 07:15:22AM -0700, Guenter Roeck wrote: > On Thu, Mar 13, 2025 at 03:28:57PM +0000, Sudeep Holla wrote: > > The PCC driver now handles mapping and unmapping of shared memory > > areas as part of pcc_mbox_{request,free}_channel(). Without these before, > > this xgene hwmon driver did handling of those mappings like several > > other PCC mailbox client drivers. > > > > There were redundant operations, leading to unnecessary code. Maintaining > > the consistency across these driver was harder due to scattered handling > > of shmem. > > > > Just use the mapped shmem and remove all redundant operations from this > > driver. > > > > Cc: Jean Delvare <jdelvare@suse.com> > > Cc: Guenter Roeck <linux@roeck-us.net> > > Cc: linux-hwmon@vger.kernel.org > > Acked-by: Guenter Roeck <linux@roeck-us.net> > > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > > Not that it matters, but I keep wondering: Why don't people use auxiliary > devices for situations like this, and keep the subsystem code where it > belongs ? > Good question. I haven't used auxiliary devices but did looks at it recently when I stumbled across some x86 telemetry code just last week. I need to go and understand it better to see how it can be used here as I don't have much understanding ATM other than its uses in GPU and Audio subsystems. > I am not requesting that you do it, I just wonder why the mechanism isn't > used. I would have thought that it would be perfect for situations like > this, so I guess I must be missing something, and I'd like to understand > what that something is. > Not sure, just that no one spent time think about it and see what is missing if any and make it work.
diff --git a/drivers/hwmon/xgene-hwmon.c b/drivers/hwmon/xgene-hwmon.c index 7087197383c96cb97e4623f419afed01d4f3c716..ea350d4de902c4e6fc4de1cd54a8b75edfad1119 100644 --- a/drivers/hwmon/xgene-hwmon.c +++ b/drivers/hwmon/xgene-hwmon.c @@ -102,9 +102,6 @@ struct xgene_hwmon_dev { struct device *hwmon_dev; bool temp_critical_alarm; - - phys_addr_t comm_base_addr; - void *pcc_comm_addr; u64 usecs_lat; }; @@ -125,7 +122,8 @@ static u16 xgene_word_tst_and_clr(u16 *addr, u16 mask) static int xgene_hwmon_pcc_rd(struct xgene_hwmon_dev *ctx, u32 *msg) { - struct acpi_pcct_shared_memory *generic_comm_base = ctx->pcc_comm_addr; + struct acpi_pcct_shared_memory __iomem *generic_comm_base = + ctx->pcc_chan->shmem; u32 *ptr = (void *)(generic_comm_base + 1); int rc, i; u16 val; @@ -523,7 +521,8 @@ static void xgene_hwmon_rx_cb(struct mbox_client *cl, void *msg) static void xgene_hwmon_pcc_rx_cb(struct mbox_client *cl, void *msg) { struct xgene_hwmon_dev *ctx = to_xgene_hwmon_dev(cl); - struct acpi_pcct_shared_memory *generic_comm_base = ctx->pcc_comm_addr; + struct acpi_pcct_shared_memory __iomem *generic_comm_base = + ctx->pcc_chan->shmem; struct slimpro_resp_msg amsg; /* @@ -649,7 +648,6 @@ static int xgene_hwmon_probe(struct platform_device *pdev) } else { struct pcc_mbox_chan *pcc_chan; const struct acpi_device_id *acpi_id; - int version; acpi_id = acpi_match_device(pdev->dev.driver->acpi_match_table, &pdev->dev); @@ -658,8 +656,6 @@ static int xgene_hwmon_probe(struct platform_device *pdev) goto out_mbox_free; } - version = (int)acpi_id->driver_data; - if (device_property_read_u32(&pdev->dev, "pcc-channel", &ctx->mbox_idx)) { dev_err(&pdev->dev, "no pcc-channel property\n"); @@ -685,34 +681,6 @@ static int xgene_hwmon_probe(struct platform_device *pdev) goto out; } - /* - * This is the shared communication region - * for the OS and Platform to communicate over. - */ - ctx->comm_base_addr = pcc_chan->shmem_base_addr; - if (ctx->comm_base_addr) { - if (version == XGENE_HWMON_V2) - ctx->pcc_comm_addr = (void __force *)devm_ioremap(&pdev->dev, - ctx->comm_base_addr, - pcc_chan->shmem_size); - else - ctx->pcc_comm_addr = devm_memremap(&pdev->dev, - ctx->comm_base_addr, - pcc_chan->shmem_size, - MEMREMAP_WB); - } else { - dev_err(&pdev->dev, "Failed to get PCC comm region\n"); - rc = -ENODEV; - goto out; - } - - if (IS_ERR_OR_NULL(ctx->pcc_comm_addr)) { - dev_err(&pdev->dev, - "Failed to ioremap PCC comm region\n"); - rc = -ENOMEM; - goto out; - } - /* * pcc_chan->latency is just a Nominal value. In reality * the remote processor could be much slower to reply.
The PCC driver now handles mapping and unmapping of shared memory areas as part of pcc_mbox_{request,free}_channel(). Without these before, this xgene hwmon driver did handling of those mappings like several other PCC mailbox client drivers. There were redundant operations, leading to unnecessary code. Maintaining the consistency across these driver was harder due to scattered handling of shmem. Just use the mapped shmem and remove all redundant operations from this driver. Cc: Jean Delvare <jdelvare@suse.com> Cc: Guenter Roeck <linux@roeck-us.net> Cc: linux-hwmon@vger.kernel.org Acked-by: Guenter Roeck <linux@roeck-us.net> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> --- drivers/hwmon/xgene-hwmon.c | 40 ++++------------------------------------ 1 file changed, 4 insertions(+), 36 deletions(-)