| Message ID | 20201223163247.28923-1-conor.dooley@microchip.com |
|---|---|
| Headers | show |
| Series | Add support for the PolarFire SoC system controller | expand |
Hello, I've added review comments below. Some of them might be more detailed than necessary, and reflect my opinion rather than something that must be fixed. Anyway, I hope my comments make sense. On Wed, Dec 23, 2020 at 04:32:55PM +0000, conor.dooley@microchip.com wrote: > From: Conor Dooley <conor.dooley@microchip.com> > > This driver adds support for the single mailbox channel of the MSS > system controller on the Microchip PolarFire SoC. > > Signed-off-by: Conor Dooley <conor.dooley@microchip.com> > --- [...] > +#define SERVICES_CR_OFFSET 0x50u > +#define SERVICES_SR_OFFSET 0x54u > +#define MAILBOX_REG_OFFSET 0x800u > +#define MSS_SYS_BUSY -EBUSY > +#define MSS_SYS_PARAM_ERR -EINVAL Defining your own aliases for -Esomething is rather uncommon. > +#define MSS_SYS_MAILBOX_DATA_OFFSET 0u > +#define SCB_MASK_WIDTH 16u > + > +/* SCBCTRL service control register */ > + > +#define SCB_CTRL_REQ (0) > +#define SCB_CTRL_REQ_MASK BIT(SCB_CTRL_REQ) > + > +#define SCB_CTRL_BUSY (1) > +#define SCB_CTRL_BUSY_MASK BIT(SCB_CTRL_BUSY) > + > +#define SCB_CTRL_ABORT (2) > +#define SCB_CTRL_ABORT_MASK BIT(SCB_CTRL_ABORT) > + > +#define SCB_CTRL_NOTIFY (3) > +#define SCB_CTRL_NOTIFY_MASK BIT(SCB_CTRL_NOTIFY) > + > +#define SCB_CTRL_POS (16) > +#define SCB_CTRL_MASK GENMASK(SCB_CTRL_POS + SCB_MASK_WIDTH, SCB_CTRL_POS) > + > +/* SCBCTRL service status registers */ registers -> register ? It seems to be only one status register. > + > +#define SCB_STATUS_REQ (0) > +#define SCB_STATUS_REQ_MASK BIT(SCB_STATUS_REQ) > + > +#define SCB_STATUS_BUSY (1) > +#define SCB_STATUS_BUSY_MASK BIT(SCB_STATUS_BUSY) > + > +#define SCB_STATUS_ABORT (2) > +#define SCB_STATUS_ABORT_MASK BIT(SCB_STATUS_ABORT) > + > +#define SCB_STATUS_NOTIFY (3) > +#define SCB_STATUS_NOTIFY_MASK BIT(SCB_STATUS_NOTIFY) > + > +#define SCB_STATUS_POS (16) > +#define SCB_STATUS_MASK GENMASK(SCB_STATUS_POS + SCB_MASK_WIDTH, SCB_STATUS_POS) > + > +struct mpfs_mbox { > + struct mbox_controller controller; > + struct device *dev; > + int irq; > + void __iomem *mailbox_base; > + void __iomem *int_reg; > + struct mbox_chan *chan; > + u16 response_size; > + u16 response_offset; > +}; > + > +static bool mpfs_mbox_busy(struct mpfs_mbox *mbox) > +{ > + u32 status; > + > + status = readl_relaxed(mbox->mailbox_base + SERVICES_SR_OFFSET); > + > + return status & SCB_STATUS_BUSY_MASK; > +} > + > +static struct mpfs_mbox *mbox_chan_to_mpfs_mbox(struct mbox_chan *chan) > +{ > + if (!chan) > + return NULL; > + > + return (struct mpfs_mbox *)chan->con_priv; > +} > + > +static int mpfs_mbox_send_data(struct mbox_chan *chan, void *data) > +{ > + u32 index; > + u32 *word_buf; > + u8 *byte_buf; > + u8 byte_off; > + u8 extra_bits; > + u8 i; > + u32 mailbox_val = 0u; > + u16 mbox_offset; > + u16 mbox_options_select; > + u32 mbox_tx_trigger; the mbox_ prefix seems unnecessary because the whole driver deals with a mailbox. Some of the variables are only used in if/for scopes below. I'd move their declaration down into these scopes, to make the outer scope of the function a little easier to understand. > + struct mpfs_mss_msg *msg = data; > + struct mpfs_mbox *mbox = mbox_chan_to_mpfs_mbox(chan); > + > + mbox_offset = msg->mailbox_offset; mbox_offset is only used once. The indirection of storing the value in another variable makes the code slightly slower to read, IMO. > + mbox->response_size = msg->response_size; > + mbox->response_offset = msg->response_offset; > + > + if (mpfs_mbox_busy(mbox)) > + return MSS_SYS_BUSY; > + > + mbox_options_select = ((mbox_offset << 7u) | (msg->cmd_opcode & 0x7fu)); > + mbox_tx_trigger = (((mbox_options_select << SCB_CTRL_POS) & > + SCB_CTRL_MASK) | SCB_CTRL_REQ_MASK | SCB_STATUS_NOTIFY_MASK); Slightly reformatted, you could save a few parentheses: mbox_options_select = (mbox_offset << 7u) | (msg->cmd_opcode & 0x7fu); mbox_tx_trigger = (mbox_options_select << SCB_CTRL_POS) & SCB_CTRL_MASK; mbox_tx_trigger |= SCB_CTRL_REQ_MASK | SCB_STATUS_NOTIFY_MASK; > + /* Code for MSS_SYS_PARAM_ERR is not implemented with this version of driver. */ What is the "code for MSS_SYS_PARAM_ERR" semantically? Input validation? > + writel_relaxed(0, mbox->int_reg); What does a write to mbox->int_reg do? Does it acknowledge an interrupt? It would be interesting to have an explaining comment here. > + > + if (msg->cmd_data_size) { > + byte_buf = msg->cmd_data; > + > + for (index = 0; index < (msg->cmd_data_size / 4); index++) > + writel_relaxed(word_buf[index], > + mbox->mailbox_base + MAILBOX_REG_OFFSET + index); word_buf is uninitialized. In mpfs_mbox_rx_data, you access the registers at mbox->mailbox_base + MAILBOX_REG_OFFSET in steps of four bytes, here you increment the offset in steps of one byte, because the index isn't scaled. This seems wrong. > + extra_bits = msg->cmd_data_size & 3; > + if (extra_bits) { > + byte_off = ALIGN_DOWN(msg->cmd_data_size, 4); > + byte_buf = msg->cmd_data + byte_off; > + > + mailbox_val = readl_relaxed(mbox->mailbox_base + > + MAILBOX_REG_OFFSET + index); > + > + for (i = 0u; i < extra_bits; i++) { > + mailbox_val &= ~(0xffu << (i * 8u)); > + mailbox_val |= (byte_buf[i] << (i * 8u)); > + } > + > + writel_relaxed(mailbox_val, > + mbox->mailbox_base + MAILBOX_REG_OFFSET + index); > + } > + } > + I'd move the calculation of mbox_tx_trigger down here, right before its use, because it's not necessary for the code above (if I'm reading it correctly). > + writel_relaxed(mbox_tx_trigger, mbox->mailbox_base + SERVICES_CR_OFFSET); > + > + return 0; > +} > + > +static inline bool mpfs_mbox_pending(struct mpfs_mbox *mbox) > +{ > + u32 status; > + > + status = readl_relaxed(mbox->mailbox_base + SERVICES_SR_OFFSET); > + > + return !(status & SCB_STATUS_BUSY_MASK); > +} > + > +static void mpfs_mbox_rx_data(struct mbox_chan *chan) > +{ > + struct mpfs_mbox *mbox = mbox_chan_to_mpfs_mbox(chan); > + u32 *data; > + u32 i; > + u32 response_limit; > + > + data = devm_kzalloc(mbox->dev, sizeof(*data) * mbox->response_size, GFP_ATOMIC); The use of devm_ seems bogus here, because the allocated data does not have the same lifetime as the device; it is allocated and freed for every RX event. However, please use kcalloc instead of kzalloc, so you don't have to do the multiplication manually. kcalloc checks for overflows. > + if (!data) > + dev_err(mbox->dev, "failed to assign memory for response\n", -ENOMEM); There is no format specifier for parameter -ENOMEM. In the !data case, the code will still run into the loop below, and probably cause a crash. > + > + response_limit = mbox->response_size + mbox->response_offset; > + if (mpfs_mbox_pending(mbox) && mbox->response_size > 0U) { > + for (i = mbox->response_offset; i < response_limit; i++) { > + data[i - mbox->response_offset] = > + readl_relaxed(mbox->mailbox_base + MAILBOX_REG_OFFSET + i * 0x4); > + } > + } It seems slightly easier to me to let the loop run from zero to mbox->response_size-1. Most importantly, it becomes easy to see that data is not accessed out of bounds. (But it's your choice) if (mpfs_mbox_pending(mbox)) { for (i = 0; i < mbox->response_size; i++) { data[i] = readl_relaxed(mbox->mailbox_base + MAILBOX_REG_OFFSET + (i + mbox->response_offset) * 0x4); } } > + > + mbox_chan_received_data(chan, (void *)data); > + devm_kfree(mbox->dev, data); > +} > + > +static irqreturn_t mpfs_mbox_inbox_isr(int irq, void *data) > +{ > + struct mbox_chan *chan = (struct mbox_chan *)data; This cast and the one at the end of mpfs_mbox_rx_data are somewhat uncessary, because C allows implicit conversion of void pointers to and from other pointer types. > + struct mpfs_mbox *mbox = mbox_chan_to_mpfs_mbox(chan); > + > + writel_relaxed(0, mbox->int_reg); > + > + mpfs_mbox_rx_data(chan); > + > + mbox_chan_txdone(chan, 0); > + return IRQ_HANDLED; > +} [...] > +++ b/include/soc/microchip/mpfs.h > @@ -0,0 +1,51 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * > + * Microchip PolarFire SoC (MPFS) mailbox This is mpfs.h, but the comment implies it's only for mailbox-related code, not for all of the Microchip PolarFire SoC. Is this intentional? Best regards, Jonathan Neuschäfer
On Wed, Dec 23, 2020 at 8:33 AM <conor.dooley@microchip.com> wrote: > > From: Conor Dooley <conor.dooley@microchip.com> > > This patch series adds support for the system controller on > the PolarFire SoC, using the mailbox framework. A Microchip directory > in the SoC subsystem has been created to hold the mailbox client > driver and will be used for future service drivers. > > This directory is included by the kconfig option: > CONFIG_SOC_MICROCHIP_POLARFIRE, so this patch series depends on Atish > Patra's PolarFire SoC support patches which introduce that option. > What version of clock driver are you using? I am waiting for an updated clock patch to post the next version of the basic soc support series. > It further depends on the MAINTAINERS entry created in the same series. > > Changes from v2: > * Further reworked dt bindings to satisfy errors and feedback > (hopefully phandle array is the correct type for the mboxes) > * Full maintainers entry moved to Atish's PFSoC support series, this series now only adds mailbox driver > * Converted config options from MPFS to POLARFIRE_SOC so they are more recognisable > * Further simplified driver code from feedback > > Changes from v1: > * Squashed header into first patch > * Fixed DT binding warnings & small fixes > * Cleaned up drivers from feedback > > Conor Dooley (5): > mbox: add polarfire soc system controller mailbox > dt-bindings: add bindings for polarfire soc mailbox > soc: add polarfire soc system controller > dt-bindings: add bindings for polarfire soc system controller > MAINTAINERS: add maintainers for polarfire soc mailbox driver > > .../mailbox/microchip,mailbox-mpfs.yaml | 47 +++ > .../microchip,mpfs_sys_controller.yaml | 34 +++ > MAINTAINERS | 1 + > drivers/mailbox/Kconfig | 12 + > drivers/mailbox/Makefile | 2 + > drivers/mailbox/mailbox-mpfs.c | 285 ++++++++++++++++++ > drivers/soc/Kconfig | 1 + > drivers/soc/Makefile | 1 + > drivers/soc/microchip/Kconfig | 10 + > drivers/soc/microchip/Makefile | 1 + > drivers/soc/microchip/mpfs-sys-controller.c | 127 ++++++++ > include/soc/microchip/mpfs.h | 51 ++++ > 12 files changed, 572 insertions(+) > create mode 100644 Documentation/devicetree/bindings/mailbox/microchip,mailbox-mpfs.yaml > create mode 100644 Documentation/devicetree/bindings/soc/microchip/microchip,mpfs_sys_controller.yaml > create mode 100644 drivers/mailbox/mailbox-mpfs.c > create mode 100644 drivers/soc/microchip/Kconfig > create mode 100644 drivers/soc/microchip/Makefile > create mode 100644 drivers/soc/microchip/mpfs-sys-controller.c > create mode 100644 include/soc/microchip/mpfs.h > > -- > 2.17.1 > > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv
On 1/6/21 7:25 PM, Atish Patra wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On Wed, Dec 23, 2020 at 8:33 AM <conor.dooley@microchip.com> wrote: >> From: Conor Dooley <conor.dooley@microchip.com> >> >> This patch series adds support for the system controller on >> the PolarFire SoC, using the mailbox framework. A Microchip directory >> in the SoC subsystem has been created to hold the mailbox client >> driver and will be used for future service drivers. >> >> This directory is included by the kconfig option: >> CONFIG_SOC_MICROCHIP_POLARFIRE, so this patch series depends on Atish >> Patra's PolarFire SoC support patches which introduce that option. >> > What version of clock driver are you using? I am waiting for an > updated clock patch to post the next > version of the basic soc support series. v2 clock driver should be available shortly. It should only require two changes in the basic soc support series (pfsoc -> mpfs naming changes). >> It further depends on the MAINTAINERS entry created in the same series. >> >> Changes from v2: >> * Further reworked dt bindings to satisfy errors and feedback >> (hopefully phandle array is the correct type for the mboxes) >> * Full maintainers entry moved to Atish's PFSoC support series, this series now only adds mailbox driver >> * Converted config options from MPFS to POLARFIRE_SOC so they are more recognisable >> * Further simplified driver code from feedback >> >> Changes from v1: >> * Squashed header into first patch >> * Fixed DT binding warnings & small fixes >> * Cleaned up drivers from feedback >> >> Conor Dooley (5): >> mbox: add polarfire soc system controller mailbox >> dt-bindings: add bindings for polarfire soc mailbox >> soc: add polarfire soc system controller >> dt-bindings: add bindings for polarfire soc system controller >> MAINTAINERS: add maintainers for polarfire soc mailbox driver >> >> .../mailbox/microchip,mailbox-mpfs.yaml | 47 +++ >> .../microchip,mpfs_sys_controller.yaml | 34 +++ >> MAINTAINERS | 1 + >> drivers/mailbox/Kconfig | 12 + >> drivers/mailbox/Makefile | 2 + >> drivers/mailbox/mailbox-mpfs.c | 285 ++++++++++++++++++ >> drivers/soc/Kconfig | 1 + >> drivers/soc/Makefile | 1 + >> drivers/soc/microchip/Kconfig | 10 + >> drivers/soc/microchip/Makefile | 1 + >> drivers/soc/microchip/mpfs-sys-controller.c | 127 ++++++++ >> include/soc/microchip/mpfs.h | 51 ++++ >> 12 files changed, 572 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/mailbox/microchip,mailbox-mpfs.yaml >> create mode 100644 Documentation/devicetree/bindings/soc/microchip/microchip,mpfs_sys_controller.yaml >> create mode 100644 drivers/mailbox/mailbox-mpfs.c >> create mode 100644 drivers/soc/microchip/Kconfig >> create mode 100644 drivers/soc/microchip/Makefile >> create mode 100644 drivers/soc/microchip/mpfs-sys-controller.c >> create mode 100644 include/soc/microchip/mpfs.h >> >> -- >> 2.17.1 >> >> >> _______________________________________________ >> linux-riscv mailing list >> linux-riscv@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-riscv > > > -- > Regards, > Atish Regards, Cyril
On 02/01/2021 13:01, Jonathan Neuschäfer wrote >Hello, > >I've added review comments below. Some of them might be more detailed >than necessary, and reflect my opinion rather than something that must >be fixed. Anyway, I hope my comments make sense. > the more detailed feedback the better in my book, if i dont mention it youll probably see in changed in the next day or two ... ... > > >> + /* Code for MSS_SYS_PARAM_ERR is not implemented with this version of driver. */ > >What is the "code for MSS_SYS_PARAM_ERR" semantically? Input validation? > >> + writel_relaxed(0, mbox->int_reg); > >What does a write to mbox->int_reg do? Does it acknowledge an interrupt? >It would be interesting to have an explaining comment here. both of these are hang overs from the bare metal driver and will be dropped >> + >> + if (msg->cmd_data_size) { >> + byte_buf = msg->cmd_data; >> + >> + for (index = 0; index < (msg->cmd_data_size / 4); index++) >> + writel_relaxed(word_buf[index], >> + mbox->mailbox_base + MAILBOX_REG_OFFSET + index); > >word_buf is uninitialized. > >In mpfs_mbox_rx_data, you access the registers at mbox->mailbox_base + >MAILBOX_REG_OFFSET in steps of four bytes, here you increment the offset >in steps of one byte, because the index isn't scaled. This seems wrong. > Thanks for catching this bug, both are related and were introduced during refactoring. The only dependent drivers implemented so far don't use the full sending functionality so it went unnoticed. ... > > >> + >> + mbox_chan_received_data(chan, (void *)data); >> + devm_kfree(mbox->dev, data); >> +} >> + >> +static irqreturn_t mpfs_mbox_inbox_isr(int irq, void *data) >> +{ >> + struct mbox_chan *chan = (struct mbox_chan *)data; > >This cast and the one at the end of mpfs_mbox_rx_data are somewhat >uncessary, because C allows implicit conversion of void pointers to and >from other pointer types. > true, i had put them in thinking it made it more clear, but on reflection it doesnt. ... >> +++ b/include/soc/microchip/mpfs.h >> @@ -0,0 +1,51 @@ >> +/* SPDX-License-Identifier: GPL-2.0 */ >> +/* >> + * >> + * Microchip PolarFire SoC (MPFS) mailbox > >This is mpfs.h, but the comment implies it's only for mailbox-related >code, not for all of the Microchip PolarFire SoC. Is this intentional? > yeah, while thats correct for now it wont remain that way for long. ill drop the mailbox reference > >Best regards, >Jonathan Neuschäfer
On Thu, Jan 21, 2021 at 12:46:42PM +0000, Conor.Dooley@microchip.com wrote: > On 02/01/2021 13:01, Jonathan Neuschäfer wrote > > >Hello, > > > >I've added review comments below. Some of them might be more detailed > >than necessary, and reflect my opinion rather than something that must > >be fixed. Anyway, I hope my comments make sense. > > > the more detailed feedback the better in my book, if i dont mention it youll probably see in changed in the next day or two Great. > >> +static irqreturn_t mpfs_mbox_inbox_isr(int irq, void *data) > >> +{ > >> + struct mbox_chan *chan = (struct mbox_chan *)data; > > > >This cast and the one at the end of mpfs_mbox_rx_data are somewhat > >uncessary, because C allows implicit conversion of void pointers to and > >from other pointer types. > > > > true, i had put them in thinking it made it more clear, but on reflection it doesnt. The main problem that I personally have with explicit pointer casts is that they are accepted by the compiler even when they would be a bad idea (casting from one non-void pointer type to another (incompatible) non-void pointer type), so they can hide a class of bugs. Thanks, Jonathan Neuschäfer
From: Conor Dooley <conor.dooley@microchip.com> This patch series adds support for the system controller on the PolarFire SoC, using the mailbox framework. A Microchip directory in the SoC subsystem has been created to hold the mailbox client driver and will be used for future service drivers. This directory is included by the kconfig option: CONFIG_SOC_MICROCHIP_POLARFIRE, so this patch series depends on Atish Patra's PolarFire SoC support patches which introduce that option. It further depends on the MAINTAINERS entry created in the same series. Changes from v2: * Further reworked dt bindings to satisfy errors and feedback (hopefully phandle array is the correct type for the mboxes) * Full maintainers entry moved to Atish's PFSoC support series, this series now only adds mailbox driver * Converted config options from MPFS to POLARFIRE_SOC so they are more recognisable * Further simplified driver code from feedback Changes from v1: * Squashed header into first patch * Fixed DT binding warnings & small fixes * Cleaned up drivers from feedback Conor Dooley (5): mbox: add polarfire soc system controller mailbox dt-bindings: add bindings for polarfire soc mailbox soc: add polarfire soc system controller dt-bindings: add bindings for polarfire soc system controller MAINTAINERS: add maintainers for polarfire soc mailbox driver .../mailbox/microchip,mailbox-mpfs.yaml | 47 +++ .../microchip,mpfs_sys_controller.yaml | 34 +++ MAINTAINERS | 1 + drivers/mailbox/Kconfig | 12 + drivers/mailbox/Makefile | 2 + drivers/mailbox/mailbox-mpfs.c | 285 ++++++++++++++++++ drivers/soc/Kconfig | 1 + drivers/soc/Makefile | 1 + drivers/soc/microchip/Kconfig | 10 + drivers/soc/microchip/Makefile | 1 + drivers/soc/microchip/mpfs-sys-controller.c | 127 ++++++++ include/soc/microchip/mpfs.h | 51 ++++ 12 files changed, 572 insertions(+) create mode 100644 Documentation/devicetree/bindings/mailbox/microchip,mailbox-mpfs.yaml create mode 100644 Documentation/devicetree/bindings/soc/microchip/microchip,mpfs_sys_controller.yaml create mode 100644 drivers/mailbox/mailbox-mpfs.c create mode 100644 drivers/soc/microchip/Kconfig create mode 100644 drivers/soc/microchip/Makefile create mode 100644 drivers/soc/microchip/mpfs-sys-controller.c create mode 100644 include/soc/microchip/mpfs.h