From: Peng Fan <[email protected]>
The firmware itself might not mark channel free, so let's explicitly
mark it free when do initialization.
Also move struct scmi_shared_mem to common.h
Signed-off-by: Peng Fan <[email protected]>
---
drivers/firmware/arm_scmi/common.h | 19 +++++++++++++++++--
drivers/firmware/arm_scmi/mailbox.c | 2 ++
drivers/firmware/arm_scmi/shmem.c | 18 ------------------
3 files changed, 19 insertions(+), 20 deletions(-)
diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h
index fd091a4ccbff..5df262a564a4 100644
--- a/drivers/firmware/arm_scmi/common.h
+++ b/drivers/firmware/arm_scmi/common.h
@@ -211,8 +211,23 @@ extern const struct scmi_desc scmi_mailbox_desc;
void scmi_rx_callback(struct scmi_chan_info *cinfo, u32 msg_hdr);
void scmi_free_channel(struct scmi_chan_info *cinfo, struct idr *idr, int id);
-/* shmem related declarations */
-struct scmi_shared_mem;
+/*
+ * SCMI specification requires all parameters, message headers, return
+ * arguments or any protocol data to be expressed in little endian
+ * format only.
+ */
+struct scmi_shared_mem {
+ __le32 reserved;
+ __le32 channel_status;
+#define SCMI_SHMEM_CHAN_STAT_CHANNEL_ERROR BIT(1)
+#define SCMI_SHMEM_CHAN_STAT_CHANNEL_FREE BIT(0)
+ __le32 reserved1[2];
+ __le32 flags;
+#define SCMI_SHMEM_FLAG_INTR_ENABLED BIT(0)
+ __le32 length;
+ __le32 msg_header;
+ u8 msg_payload[0];
+};
void shmem_tx_prepare(struct scmi_shared_mem __iomem *shmem,
struct scmi_xfer *xfer);
diff --git a/drivers/firmware/arm_scmi/mailbox.c b/drivers/firmware/arm_scmi/mailbox.c
index 68ed58e2a47a..2d34bf6e94e2 100644
--- a/drivers/firmware/arm_scmi/mailbox.c
+++ b/drivers/firmware/arm_scmi/mailbox.c
@@ -104,6 +104,8 @@ static int mailbox_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
cinfo->transport_info = smbox;
smbox->cinfo = cinfo;
+ iowrite32(BIT(0), &smbox->shmem->channel_status);
+
return 0;
}
diff --git a/drivers/firmware/arm_scmi/shmem.c b/drivers/firmware/arm_scmi/shmem.c
index 02c6c7540f42..c9e334654ba3 100644
--- a/drivers/firmware/arm_scmi/shmem.c
+++ b/drivers/firmware/arm_scmi/shmem.c
@@ -11,24 +11,6 @@
#include "common.h"
-/*
- * SCMI specification requires all parameters, message headers, return
- * arguments or any protocol data to be expressed in little endian
- * format only.
- */
-struct scmi_shared_mem {
- __le32 reserved;
- __le32 channel_status;
-#define SCMI_SHMEM_CHAN_STAT_CHANNEL_ERROR BIT(1)
-#define SCMI_SHMEM_CHAN_STAT_CHANNEL_FREE BIT(0)
- __le32 reserved1[2];
- __le32 flags;
-#define SCMI_SHMEM_FLAG_INTR_ENABLED BIT(0)
- __le32 length;
- __le32 msg_header;
- u8 msg_payload[0];
-};
-
void shmem_tx_prepare(struct scmi_shared_mem __iomem *shmem,
struct scmi_xfer *xfer)
{
--
2.16.4
> Subject: Re: [PATCH 2/2] firmware: arm_scmi: mark channel free when init
>
> On Thu, Feb 06, 2020 at 08:57:26PM +0800, [email protected] wrote:
> > From: Peng Fan <[email protected]>
> >
> > The firmware itself might not mark channel free, so let's explicitly
> > mark it free when do initialization.
> >
> > Also move struct scmi_shared_mem to common.h
> >
> > Signed-off-by: Peng Fan <[email protected]>
> > ---
> > drivers/firmware/arm_scmi/common.h | 19 +++++++++++++++++--
> > drivers/firmware/arm_scmi/mailbox.c | 2 ++
> > drivers/firmware/arm_scmi/shmem.c | 18 ------------------
> > 3 files changed, 19 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/firmware/arm_scmi/common.h
> > b/drivers/firmware/arm_scmi/common.h
> > index fd091a4ccbff..5df262a564a4 100644
> > --- a/drivers/firmware/arm_scmi/common.h
> > +++ b/drivers/firmware/arm_scmi/common.h
> > @@ -211,8 +211,23 @@ extern const struct scmi_desc scmi_mailbox_desc;
> > void scmi_rx_callback(struct scmi_chan_info *cinfo, u32 msg_hdr);
> > void scmi_free_channel(struct scmi_chan_info *cinfo, struct idr *idr,
> > int id);
> >
> > -/* shmem related declarations */
> > -struct scmi_shared_mem;
> > +/*
> > + * SCMI specification requires all parameters, message headers,
> > +return
> > + * arguments or any protocol data to be expressed in little endian
> > + * format only.
> > + */
> > +struct scmi_shared_mem {
> > + __le32 reserved;
> > + __le32 channel_status;
> > +#define SCMI_SHMEM_CHAN_STAT_CHANNEL_ERROR BIT(1)
> > +#define SCMI_SHMEM_CHAN_STAT_CHANNEL_FREE BIT(0)
> > + __le32 reserved1[2];
> > + __le32 flags;
> > +#define SCMI_SHMEM_FLAG_INTR_ENABLED BIT(0)
> > + __le32 length;
> > + __le32 msg_header;
> > + u8 msg_payload[0];
> > +};
> >
> > void shmem_tx_prepare(struct scmi_shared_mem __iomem *shmem,
> > struct scmi_xfer *xfer);
> > diff --git a/drivers/firmware/arm_scmi/mailbox.c
> > b/drivers/firmware/arm_scmi/mailbox.c
> > index 68ed58e2a47a..2d34bf6e94e2 100644
> > --- a/drivers/firmware/arm_scmi/mailbox.c
> > +++ b/drivers/firmware/arm_scmi/mailbox.c
> > @@ -104,6 +104,8 @@ static int mailbox_chan_setup(struct
> scmi_chan_info *cinfo, struct device *dev,
> > cinfo->transport_info = smbox;
> > smbox->cinfo = cinfo;
> >
> > + iowrite32(BIT(0), &smbox->shmem->channel_status);
> > +
>
+arm list
> If we need this then we may need to put this as a function in shmem.c I am
> still not convinced if we can do this unconditionally, i.e. will that affect Rx
> channel if there's notification pending before we initialise. But we can deal
> with that later.
Per understanding, channel is specific to an agent, it could not be shared.
So the shmem binded to the channel will not be used by others.
Since this is the initialization process, the firmware might not init the shmem.
The shmem.c shmem_tx_prepare will spin until channel free, so I did the patch.
Otherwise it might spin forever.
I'll add a check as following
if (tx)
iowrite32(BIT(0), &smbox->shmem->channel_status);
I not find a good place to put this in shmem.c (:
>
> Also what about error fields ? I would rather clear it to 0, not just BIT(0)
Tx channel error should also be cleared, fix in v2.
Thanks,
Peng
>
> --
> Regards,
> Sudeep
On Fri, Feb 07, 2020 at 02:16:04AM +0000, Peng Fan wrote:
>
> > Subject: Re: [PATCH 2/2] firmware: arm_scmi: mark channel free when init
> >
> > On Thu, Feb 06, 2020 at 08:57:26PM +0800, [email protected] wrote:
> > > From: Peng Fan <[email protected]>
> > >
> > > The firmware itself might not mark channel free, so let's explicitly
> > > mark it free when do initialization.
> > >
> > > Also move struct scmi_shared_mem to common.h
> > >
> > > Signed-off-by: Peng Fan <[email protected]>
> > > ---
> > > drivers/firmware/arm_scmi/common.h | 19 +++++++++++++++++--
> > > drivers/firmware/arm_scmi/mailbox.c | 2 ++
> > > drivers/firmware/arm_scmi/shmem.c | 18 ------------------
> > > 3 files changed, 19 insertions(+), 20 deletions(-)
> > >
> > > diff --git a/drivers/firmware/arm_scmi/common.h
> > > b/drivers/firmware/arm_scmi/common.h
> > > index fd091a4ccbff..5df262a564a4 100644
> > > --- a/drivers/firmware/arm_scmi/common.h
> > > +++ b/drivers/firmware/arm_scmi/common.h
> > > @@ -211,8 +211,23 @@ extern const struct scmi_desc scmi_mailbox_desc;
> > > void scmi_rx_callback(struct scmi_chan_info *cinfo, u32 msg_hdr);
> > > void scmi_free_channel(struct scmi_chan_info *cinfo, struct idr *idr,
> > > int id);
> > >
> > > -/* shmem related declarations */
> > > -struct scmi_shared_mem;
> > > +/*
> > > + * SCMI specification requires all parameters, message headers,
> > > +return
> > > + * arguments or any protocol data to be expressed in little endian
> > > + * format only.
> > > + */
> > > +struct scmi_shared_mem {
> > > + __le32 reserved;
> > > + __le32 channel_status;
> > > +#define SCMI_SHMEM_CHAN_STAT_CHANNEL_ERROR BIT(1)
> > > +#define SCMI_SHMEM_CHAN_STAT_CHANNEL_FREE BIT(0)
> > > + __le32 reserved1[2];
> > > + __le32 flags;
> > > +#define SCMI_SHMEM_FLAG_INTR_ENABLED BIT(0)
> > > + __le32 length;
> > > + __le32 msg_header;
> > > + u8 msg_payload[0];
> > > +};
> > >
> > > void shmem_tx_prepare(struct scmi_shared_mem __iomem *shmem,
> > > struct scmi_xfer *xfer);
> > > diff --git a/drivers/firmware/arm_scmi/mailbox.c
> > > b/drivers/firmware/arm_scmi/mailbox.c
> > > index 68ed58e2a47a..2d34bf6e94e2 100644
> > > --- a/drivers/firmware/arm_scmi/mailbox.c
> > > +++ b/drivers/firmware/arm_scmi/mailbox.c
> > > @@ -104,6 +104,8 @@ static int mailbox_chan_setup(struct
> > scmi_chan_info *cinfo, struct device *dev,
> > > cinfo->transport_info = smbox;
> > > smbox->cinfo = cinfo;
> > >
> > > + iowrite32(BIT(0), &smbox->shmem->channel_status);
> > > +
> >
>
> +arm list
>
> > If we need this then we may need to put this as a function in shmem.c I am
> > still not convinced if we can do this unconditionally, i.e. will that affect Rx
> > channel if there's notification pending before we initialise. But we can deal
> > with that later.
>
> Per understanding, channel is specific to an agent, it could not be shared.
> So the shmem binded to the channel will not be used by others.
>
Yes
> Since this is the initialization process, the firmware might not init the shmem.
>
But, is there any particular reason for firmware not to ? It means platform
holds the channel and needs to release it to agent(OSPM) in this case after
initialisation.
> The shmem.c shmem_tx_prepare will spin until channel free, so I did the patch.
> Otherwise it might spin forever.
>
Yes I guessed that to be reason.
> I'll add a check as following
> if (tx)
> iowrite32(BIT(0), &smbox->shmem->channel_status);
>
Not yet, I need answers to above query.
> I not find a good place to put this in shmem.c (:
>
Least of the problem :), let us first agree if we have to have it.
> >
> > Also what about error fields ? I would rather clear it to 0, not just BIT(0)
>
> Tx channel error should also be cleared, fix in v2.
>
OK but wait for a while before you post for the discussion to end.
--
Regards,
Sudeep
> Subject: Re: [PATCH 2/2] firmware: arm_scmi: mark channel free when init
>
> On Fri, Feb 07, 2020 at 02:16:04AM +0000, Peng Fan wrote:
> >
> > > Subject: Re: [PATCH 2/2] firmware: arm_scmi: mark channel free when
> > > init
> > >
> > > On Thu, Feb 06, 2020 at 08:57:26PM +0800, [email protected] wrote:
> > > > From: Peng Fan <[email protected]>
> > > >
> > > > The firmware itself might not mark channel free, so let's
> > > > explicitly mark it free when do initialization.
> > > >
> > > > Also move struct scmi_shared_mem to common.h
> > > >
> > > > Signed-off-by: Peng Fan <[email protected]>
> > > > ---
> > > > drivers/firmware/arm_scmi/common.h | 19 +++++++++++++++++--
> > > > drivers/firmware/arm_scmi/mailbox.c | 2 ++
> > > > drivers/firmware/arm_scmi/shmem.c | 18 ------------------
> > > > 3 files changed, 19 insertions(+), 20 deletions(-)
> > > >
> > > > diff --git a/drivers/firmware/arm_scmi/common.h
> > > > b/drivers/firmware/arm_scmi/common.h
> > > > index fd091a4ccbff..5df262a564a4 100644
> > > > --- a/drivers/firmware/arm_scmi/common.h
> > > > +++ b/drivers/firmware/arm_scmi/common.h
> > > > @@ -211,8 +211,23 @@ extern const struct scmi_desc
> > > > scmi_mailbox_desc; void scmi_rx_callback(struct scmi_chan_info
> > > > *cinfo, u32 msg_hdr); void scmi_free_channel(struct scmi_chan_info
> > > > *cinfo, struct idr *idr, int id);
> > > >
> > > > -/* shmem related declarations */
> > > > -struct scmi_shared_mem;
> > > > +/*
> > > > + * SCMI specification requires all parameters, message headers,
> > > > +return
> > > > + * arguments or any protocol data to be expressed in little
> > > > +endian
> > > > + * format only.
> > > > + */
> > > > +struct scmi_shared_mem {
> > > > + __le32 reserved;
> > > > + __le32 channel_status;
> > > > +#define SCMI_SHMEM_CHAN_STAT_CHANNEL_ERROR BIT(1)
> > > > +#define SCMI_SHMEM_CHAN_STAT_CHANNEL_FREE BIT(0)
> > > > + __le32 reserved1[2];
> > > > + __le32 flags;
> > > > +#define SCMI_SHMEM_FLAG_INTR_ENABLED BIT(0)
> > > > + __le32 length;
> > > > + __le32 msg_header;
> > > > + u8 msg_payload[0];
> > > > +};
> > > >
> > > > void shmem_tx_prepare(struct scmi_shared_mem __iomem *shmem,
> > > > struct scmi_xfer *xfer);
> > > > diff --git a/drivers/firmware/arm_scmi/mailbox.c
> > > > b/drivers/firmware/arm_scmi/mailbox.c
> > > > index 68ed58e2a47a..2d34bf6e94e2 100644
> > > > --- a/drivers/firmware/arm_scmi/mailbox.c
> > > > +++ b/drivers/firmware/arm_scmi/mailbox.c
> > > > @@ -104,6 +104,8 @@ static int mailbox_chan_setup(struct
> > > scmi_chan_info *cinfo, struct device *dev,
> > > > cinfo->transport_info = smbox;
> > > > smbox->cinfo = cinfo;
> > > >
> > > > + iowrite32(BIT(0), &smbox->shmem->channel_status);
> > > > +
> > >
> >
> > +arm list
> >
> > > If we need this then we may need to put this as a function in
> > > shmem.c I am still not convinced if we can do this unconditionally,
> > > i.e. will that affect Rx channel if there's notification pending
> > > before we initialise. But we can deal with that later.
> >
> > Per understanding, channel is specific to an agent, it could not be shared.
> > So the shmem binded to the channel will not be used by others.
> >
>
> Yes
>
> > Since this is the initialization process, the firmware might not init the
> shmem.
> >
>
> But, is there any particular reason for firmware not to ? It means platform
> holds the channel and needs to release it to agent(OSPM) in this case after
> initialisation.
It is just my ATF not initialize the shmem area the leave the area with random
data.
So I think that some released buggy firmware might also has similar issue.
To support buggy firmware and bug-fixed firmware, I think it might be helpful
to init shmem in Linux side.
Thanks,
Peng.
>
> > The shmem.c shmem_tx_prepare will spin until channel free, so I did the
> patch.
> > Otherwise it might spin forever.
> >
>
> Yes I guessed that to be reason.
>
> > I'll add a check as following
> > if (tx)
> > iowrite32(BIT(0), &smbox->shmem->channel_status);
> >
>
> Not yet, I need answers to above query.
>
> > I not find a good place to put this in shmem.c (:
> >
>
> Least of the problem :), let us first agree if we have to have it.
>
> > >
> > > Also what about error fields ? I would rather clear it to 0, not
> > > just BIT(0)
> >
> > Tx channel error should also be cleared, fix in v2.
> >
>
> OK but wait for a while before you post for the discussion to end.
>
> --
> Regards,
> Sudeep
Hi Sudeep
> Subject: Re: [PATCH 2/2] firmware: arm_scmi: mark channel free when init
>
> On Fri, Feb 07, 2020 at 02:16:04AM +0000, Peng Fan wrote:
> >
> > > Subject: Re: [PATCH 2/2] firmware: arm_scmi: mark channel free when
> > > init
> > >
> > > On Thu, Feb 06, 2020 at 08:57:26PM +0800, [email protected] wrote:
> > > > From: Peng Fan <[email protected]>
> > > >
> > > > The firmware itself might not mark channel free, so let's
> > > > explicitly mark it free when do initialization.
> > > >
> > > > Also move struct scmi_shared_mem to common.h
> > > >
> > > > Signed-off-by: Peng Fan <[email protected]>
> > > > ---
> > > > drivers/firmware/arm_scmi/common.h | 19 +++++++++++++++++--
> > > > drivers/firmware/arm_scmi/mailbox.c | 2 ++
> > > > drivers/firmware/arm_scmi/shmem.c | 18 ------------------
> > > > 3 files changed, 19 insertions(+), 20 deletions(-)
> > > >
> > > > diff --git a/drivers/firmware/arm_scmi/common.h
> > > > b/drivers/firmware/arm_scmi/common.h
> > > > index fd091a4ccbff..5df262a564a4 100644
> > > > --- a/drivers/firmware/arm_scmi/common.h
> > > > +++ b/drivers/firmware/arm_scmi/common.h
> > > > @@ -211,8 +211,23 @@ extern const struct scmi_desc
> > > > scmi_mailbox_desc; void scmi_rx_callback(struct scmi_chan_info
> > > > *cinfo, u32 msg_hdr); void scmi_free_channel(struct scmi_chan_info
> > > > *cinfo, struct idr *idr, int id);
> > > >
> > > > -/* shmem related declarations */
> > > > -struct scmi_shared_mem;
> > > > +/*
> > > > + * SCMI specification requires all parameters, message headers,
> > > > +return
> > > > + * arguments or any protocol data to be expressed in little
> > > > +endian
> > > > + * format only.
> > > > + */
> > > > +struct scmi_shared_mem {
> > > > + __le32 reserved;
> > > > + __le32 channel_status;
> > > > +#define SCMI_SHMEM_CHAN_STAT_CHANNEL_ERROR BIT(1)
> > > > +#define SCMI_SHMEM_CHAN_STAT_CHANNEL_FREE BIT(0)
> > > > + __le32 reserved1[2];
> > > > + __le32 flags;
> > > > +#define SCMI_SHMEM_FLAG_INTR_ENABLED BIT(0)
> > > > + __le32 length;
> > > > + __le32 msg_header;
> > > > + u8 msg_payload[0];
> > > > +};
> > > >
> > > > void shmem_tx_prepare(struct scmi_shared_mem __iomem *shmem,
> > > > struct scmi_xfer *xfer);
> > > > diff --git a/drivers/firmware/arm_scmi/mailbox.c
> > > > b/drivers/firmware/arm_scmi/mailbox.c
> > > > index 68ed58e2a47a..2d34bf6e94e2 100644
> > > > --- a/drivers/firmware/arm_scmi/mailbox.c
> > > > +++ b/drivers/firmware/arm_scmi/mailbox.c
> > > > @@ -104,6 +104,8 @@ static int mailbox_chan_setup(struct
> > > scmi_chan_info *cinfo, struct device *dev,
> > > > cinfo->transport_info = smbox;
> > > > smbox->cinfo = cinfo;
> > > >
> > > > + iowrite32(BIT(0), &smbox->shmem->channel_status);
> > > > +
> > >
> >
> > +arm list
> >
> > > If we need this then we may need to put this as a function in
> > > shmem.c I am still not convinced if we can do this unconditionally,
> > > i.e. will that affect Rx channel if there's notification pending
> > > before we initialise. But we can deal with that later.
> >
> > Per understanding, channel is specific to an agent, it could not be shared.
> > So the shmem binded to the channel will not be used by others.
> >
>
> Yes
>
> > Since this is the initialization process, the firmware might not init the
> shmem.
> >
>
> But, is there any particular reason for firmware not to ? It means platform
> holds the channel and needs to release it to agent(OSPM) in this case after
> initialisation.
>
> > The shmem.c shmem_tx_prepare will spin until channel free, so I did the
> patch.
> > Otherwise it might spin forever.
> >
>
> Yes I guessed that to be reason.
>
> > I'll add a check as following
> > if (tx)
> > iowrite32(BIT(0), &smbox->shmem->channel_status);
> >
>
> Not yet, I need answers to above query.
After recheck the SCMI 2.0 spec,
Shared memory area:
This is an area of memory that is shared between the caller and the callee.
At any point in time, the shared memory is owned by the caller or the callee.
The ownership is reflected by a channel status word in the shared memory area.
For TX shared memory, it not explains who should initialize the channel status,
callee or the caller.
In my case, I not let firmware initialize the shared memory, so it might contain
garbage data and not able to wait channel free.
So I think add a check of tx and set channel to free when scmi driver probe
, this is valid.
Or update spec to explicitly show who should initialize channel?
Thanks,
Peng.
>
> > I not find a good place to put this in shmem.c (:
> >
>
> Least of the problem :), let us first agree if we have to have it.
>
> > >
> > > Also what about error fields ? I would rather clear it to 0, not
> > > just BIT(0)
> >
> > Tx channel error should also be cleared, fix in v2.
> >
>
> OK but wait for a while before you post for the discussion to end.
>
> --
> Regards,
> Sudeep