[PATCH 0/4] firmware: arm_scmi: Extend SMC/HVC to support multiple channels

[PATCH 0/4] firmware: arm_scmi: Extend SMC/HVC to support multiple channels

[Sudeep Holla]

Hi,

While merging SMC/HVC transport to SCMI, I did a quick hack to extend
it to support multiple channels. I may be missing something obvious, if
not the changes required look simple. Some of them could be merged into
original patch, but since I am unable to test, I preferred to keep them
separate, easy to revert if things break.

Regards,
Sudeep
--

Sudeep Holla (4):
  firmware: arm_scmi: Make mutex channel specific
  firmware: arm_scmi: Drop empty stub for smc_mark_txdone
  firmware: arm_scmi: Check shmem property for channel availablity
  firmware: arm_scmi: Drop checking for shmem property in parent node

 drivers/firmware/arm_scmi/smc.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

--
2.17.1

[PATCH 0/4] firmware: arm_scmi: Extend SMC/HVC to support multiple channels

[Sudeep Holla]

Hi,

While merging SMC/HVC transport to SCMI, I did a quick hack to extend
it to support multiple channels. I may be missing something obvious, if
not the changes required look simple. Some of them could be merged into
original patch, but since I am unable to test, I preferred to keep them
separate, easy to revert if things break.

Regards,
Sudeep
--

Sudeep Holla (4):
  firmware: arm_scmi: Make mutex channel specific
  firmware: arm_scmi: Drop empty stub for smc_mark_txdone
  firmware: arm_scmi: Check shmem property for channel availablity
  firmware: arm_scmi: Drop checking for shmem property in parent node

 drivers/firmware/arm_scmi/smc.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

--
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/4] firmware: arm_scmi: Make mutex channel specific

[Sudeep Holla]

In order to support multiple SMC/HVC transport channels with associated
shared memory, it is better to maintain the mutex per channel instead of
existing global one.

Move the smc_mutex into the scmi_smc structure and also rename it to
shmem_lock which is more appropriate for it's use.

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/arm_scmi/smc.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/firmware/arm_scmi/smc.c b/drivers/firmware/arm_scmi/smc.c
index 336168e40f49..6dc8a88cc91b 100644
--- a/drivers/firmware/arm_scmi/smc.c
+++ b/drivers/firmware/arm_scmi/smc.c
@@ -27,11 +27,10 @@
 struct scmi_smc {
 	struct scmi_chan_info *cinfo;
 	struct scmi_shared_mem __iomem *shmem;
+	struct mutex shmem_lock;
 	u32 func_id;
 };
 
-static DEFINE_MUTEX(smc_mutex);
-
 static bool smc_chan_available(struct device *dev, int idx)
 {
 	return true;
@@ -78,6 +77,7 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
 
 	scmi_info->func_id = func_id;
 	scmi_info->cinfo = cinfo;
+	mutex_init(&scmi_info->shmem_lock);
 	cinfo->transport_info = scmi_info;
 
 	return 0;
@@ -102,14 +102,14 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
 	struct scmi_smc *scmi_info = cinfo->transport_info;
 	struct arm_smccc_res res;
 
-	mutex_lock(&smc_mutex);
+	mutex_lock(&scmi_info->shmem_lock);
 
 	shmem_tx_prepare(scmi_info->shmem, xfer);
 
 	arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, &res);
 	scmi_rx_callback(scmi_info->cinfo, shmem_read_header(scmi_info->shmem));
 
-	mutex_unlock(&smc_mutex);
+	mutex_unlock(&scmi_info->shmem_lock);
 
 	return res.a0;
 }
-- 
2.17.1

[PATCH 1/4] firmware: arm_scmi: Make mutex channel specific

[Sudeep Holla]

In order to support multiple SMC/HVC transport channels with associated
shared memory, it is better to maintain the mutex per channel instead of
existing global one.

Move the smc_mutex into the scmi_smc structure and also rename it to
shmem_lock which is more appropriate for it's use.

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/arm_scmi/smc.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/firmware/arm_scmi/smc.c b/drivers/firmware/arm_scmi/smc.c
index 336168e40f49..6dc8a88cc91b 100644
--- a/drivers/firmware/arm_scmi/smc.c
+++ b/drivers/firmware/arm_scmi/smc.c
@@ -27,11 +27,10 @@
 struct scmi_smc {
 	struct scmi_chan_info *cinfo;
 	struct scmi_shared_mem __iomem *shmem;
+	struct mutex shmem_lock;
 	u32 func_id;
 };
 
-static DEFINE_MUTEX(smc_mutex);
-
 static bool smc_chan_available(struct device *dev, int idx)
 {
 	return true;
@@ -78,6 +77,7 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
 
 	scmi_info->func_id = func_id;
 	scmi_info->cinfo = cinfo;
+	mutex_init(&scmi_info->shmem_lock);
 	cinfo->transport_info = scmi_info;
 
 	return 0;
@@ -102,14 +102,14 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
 	struct scmi_smc *scmi_info = cinfo->transport_info;
 	struct arm_smccc_res res;
 
-	mutex_lock(&smc_mutex);
+	mutex_lock(&scmi_info->shmem_lock);
 
 	shmem_tx_prepare(scmi_info->shmem, xfer);
 
 	arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, &res);
 	scmi_rx_callback(scmi_info->cinfo, shmem_read_header(scmi_info->shmem));
 
-	mutex_unlock(&smc_mutex);
+	mutex_unlock(&scmi_info->shmem_lock);
 
 	return res.a0;
 }
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/4] firmware: arm_scmi: Drop empty stub for smc_mark_txdone

[Sudeep Holla]

The scmi protocol core driver check for non NULL mark_txdone before
invoking the same. There is no need to provide a empty stub. SMC/HVC
calls are synchronous and the call return indicates the completion.

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/arm_scmi/smc.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/firmware/arm_scmi/smc.c b/drivers/firmware/arm_scmi/smc.c
index 6dc8a88cc91b..dd4b54c29679 100644
--- a/drivers/firmware/arm_scmi/smc.c
+++ b/drivers/firmware/arm_scmi/smc.c
@@ -114,10 +114,6 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
 	return res.a0;
 }
 
-static void smc_mark_txdone(struct scmi_chan_info *cinfo, int ret)
-{
-}
-
 static void smc_fetch_response(struct scmi_chan_info *cinfo,
 			       struct scmi_xfer *xfer)
 {
@@ -139,7 +135,6 @@ static struct scmi_transport_ops scmi_smc_ops = {
 	.chan_setup = smc_chan_setup,
 	.chan_free = smc_chan_free,
 	.send_message = smc_send_message,
-	.mark_txdone = smc_mark_txdone,
 	.fetch_response = smc_fetch_response,
 	.poll_done = smc_poll_done,
 };
-- 
2.17.1

[PATCH 2/4] firmware: arm_scmi: Drop empty stub for smc_mark_txdone

[Sudeep Holla]

The scmi protocol core driver check for non NULL mark_txdone before
invoking the same. There is no need to provide a empty stub. SMC/HVC
calls are synchronous and the call return indicates the completion.

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/arm_scmi/smc.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/firmware/arm_scmi/smc.c b/drivers/firmware/arm_scmi/smc.c
index 6dc8a88cc91b..dd4b54c29679 100644
--- a/drivers/firmware/arm_scmi/smc.c
+++ b/drivers/firmware/arm_scmi/smc.c
@@ -114,10 +114,6 @@ static int smc_send_message(struct scmi_chan_info *cinfo,
 	return res.a0;
 }
 
-static void smc_mark_txdone(struct scmi_chan_info *cinfo, int ret)
-{
-}
-
 static void smc_fetch_response(struct scmi_chan_info *cinfo,
 			       struct scmi_xfer *xfer)
 {
@@ -139,7 +135,6 @@ static struct scmi_transport_ops scmi_smc_ops = {
 	.chan_setup = smc_chan_setup,
 	.chan_free = smc_chan_free,
 	.send_message = smc_send_message,
-	.mark_txdone = smc_mark_txdone,
 	.fetch_response = smc_fetch_response,
 	.poll_done = smc_poll_done,
 };
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/4] firmware: arm_scmi: Check shmem property for channel availablity

[Sudeep Holla]

Instead of declaring the channel availabilty unconditionally, let us
check for the presence of "shmem" property and return the channel
availablity accordingly.

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/arm_scmi/smc.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/firmware/arm_scmi/smc.c b/drivers/firmware/arm_scmi/smc.c
index dd4b54c29679..5929c668dc1d 100644
--- a/drivers/firmware/arm_scmi/smc.c
+++ b/drivers/firmware/arm_scmi/smc.c
@@ -33,6 +33,11 @@ struct scmi_smc {
 
 static bool smc_chan_available(struct device *dev, int idx)
 {
+	struct device_node *np = of_parse_phandle(dev->of_node, "shmem", 0);
+	if (!np)
+		return false;
+
+	of_node_put(np);
 	return true;
 }
 
-- 
2.17.1

[PATCH 3/4] firmware: arm_scmi: Check shmem property for channel availablity

[Sudeep Holla]

Instead of declaring the channel availabilty unconditionally, let us
check for the presence of "shmem" property and return the channel
availablity accordingly.

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/arm_scmi/smc.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/firmware/arm_scmi/smc.c b/drivers/firmware/arm_scmi/smc.c
index dd4b54c29679..5929c668dc1d 100644
--- a/drivers/firmware/arm_scmi/smc.c
+++ b/drivers/firmware/arm_scmi/smc.c
@@ -33,6 +33,11 @@ struct scmi_smc {
 
 static bool smc_chan_available(struct device *dev, int idx)
 {
+	struct device_node *np = of_parse_phandle(dev->of_node, "shmem", 0);
+	if (!np)
+		return false;
+
+	of_node_put(np);
 	return true;
 }
 
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 4/4] firmware: arm_scmi: Drop checking for shmem property in parent node

[Sudeep Holla]

The scmi protocol core driver checks for the channel availability
before evaluating the shmem property. If the individual protocols
don't have separate channel assigned to them, the channel alloted
for the BASE protocol is reused automatically.

Therefore there is no need to check for the shmem property in the
parent node if it is absent in the child protocol node.

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/arm_scmi/smc.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/firmware/arm_scmi/smc.c b/drivers/firmware/arm_scmi/smc.c
index 5929c668dc1d..833e793b5391 100644
--- a/drivers/firmware/arm_scmi/smc.c
+++ b/drivers/firmware/arm_scmi/smc.c
@@ -60,8 +60,6 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
 		return -ENOMEM;
 
 	np = of_parse_phandle(cdev->of_node, "shmem", 0);
-	if (!np)
-		np = of_parse_phandle(dev->of_node, "shmem", 0);
 	ret = of_address_to_resource(np, 0, &res);
 	of_node_put(np);
 	if (ret) {
-- 
2.17.1

[PATCH 4/4] firmware: arm_scmi: Drop checking for shmem property in parent node

[Sudeep Holla]

The scmi protocol core driver checks for the channel availability
before evaluating the shmem property. If the individual protocols
don't have separate channel assigned to them, the channel alloted
for the BASE protocol is reused automatically.

Therefore there is no need to check for the shmem property in the
parent node if it is absent in the child protocol node.

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/arm_scmi/smc.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/firmware/arm_scmi/smc.c b/drivers/firmware/arm_scmi/smc.c
index 5929c668dc1d..833e793b5391 100644
--- a/drivers/firmware/arm_scmi/smc.c
+++ b/drivers/firmware/arm_scmi/smc.c
@@ -60,8 +60,6 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo, struct device *dev,
 		return -ENOMEM;
 
 	np = of_parse_phandle(cdev->of_node, "shmem", 0);
-	if (!np)
-		np = of_parse_phandle(dev->of_node, "shmem", 0);
 	ret = of_address_to_resource(np, 0, &res);
 	of_node_put(np);
 	if (ret) {
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH 0/4] firmware: arm_scmi: Extend SMC/HVC to support multiple channels

[Peng Fan]

> Subject: [PATCH 0/4] firmware: arm_scmi: Extend SMC/HVC to support
> multiple channels
> 
> Hi,
> 
> While merging SMC/HVC transport to SCMI, I did a quick hack to extend it to
> support multiple channels. I may be missing something obvious, if not the
> changes required look simple. Some of them could be merged into original
> patch, but since I am unable to test, I preferred to keep them separate, easy
> to revert if things break.

With patchset applied to your scmi tree, my test still work as before.

Tested-by: Peng Fan <peng.fan@nxp.com>

Thanks,
Peng.

> 
> Regards,
> Sudeep
> --
> 
> Sudeep Holla (4):
>   firmware: arm_scmi: Make mutex channel specific
>   firmware: arm_scmi: Drop empty stub for smc_mark_txdone
>   firmware: arm_scmi: Check shmem property for channel availablity
>   firmware: arm_scmi: Drop checking for shmem property in parent node
> 
>  drivers/firmware/arm_scmi/smc.c | 20 +++++++++-----------
>  1 file changed, 9 insertions(+), 11 deletions(-)
> 
> --
> 2.17.1

RE: [PATCH 0/4] firmware: arm_scmi: Extend SMC/HVC to support multiple channels

[Peng Fan]

> Subject: [PATCH 0/4] firmware: arm_scmi: Extend SMC/HVC to support
> multiple channels
> 
> Hi,
> 
> While merging SMC/HVC transport to SCMI, I did a quick hack to extend it to
> support multiple channels. I may be missing something obvious, if not the
> changes required look simple. Some of them could be merged into original
> patch, but since I am unable to test, I preferred to keep them separate, easy
> to revert if things break.

With patchset applied to your scmi tree, my test still work as before.

Tested-by: Peng Fan <peng.fan@nxp.com>

Thanks,
Peng.

> 
> Regards,
> Sudeep
> --
> 
> Sudeep Holla (4):
>   firmware: arm_scmi: Make mutex channel specific
>   firmware: arm_scmi: Drop empty stub for smc_mark_txdone
>   firmware: arm_scmi: Check shmem property for channel availablity
>   firmware: arm_scmi: Drop checking for shmem property in parent node
> 
>  drivers/firmware/arm_scmi/smc.c | 20 +++++++++-----------
>  1 file changed, 9 insertions(+), 11 deletions(-)
> 
> --
> 2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/4] firmware: arm_scmi: Extend SMC/HVC to support multiple channels

[Sudeep Holla]

Hi Peng,

On Tue, Mar 31, 2020 at 01:53:40PM +0000, Peng Fan wrote:
> > Subject: [PATCH 0/4] firmware: arm_scmi: Extend SMC/HVC to support
> > multiple channels
> >
> > Hi,
> >
> > While merging SMC/HVC transport to SCMI, I did a quick hack to extend it to
> > support multiple channels. I may be missing something obvious, if not the
> > changes required look simple. Some of them could be merged into original
> > patch, but since I am unable to test, I preferred to keep them separate, easy
> > to revert if things break.
>
> With patchset applied to your scmi tree, my test still work as before.
>
> Tested-by: Peng Fan <peng.fan@nxp.com>
>

Thanks for testing. If you can give it a quick review as it is very small
change, that would be great. I just want more eyes to look at it just to
make sure I am not missing something obvious.

--
Regards,
Sudeep

Re: [PATCH 0/4] firmware: arm_scmi: Extend SMC/HVC to support multiple channels

[Sudeep Holla]

Hi Peng,

On Tue, Mar 31, 2020 at 01:53:40PM +0000, Peng Fan wrote:
> > Subject: [PATCH 0/4] firmware: arm_scmi: Extend SMC/HVC to support
> > multiple channels
> >
> > Hi,
> >
> > While merging SMC/HVC transport to SCMI, I did a quick hack to extend it to
> > support multiple channels. I may be missing something obvious, if not the
> > changes required look simple. Some of them could be merged into original
> > patch, but since I am unable to test, I preferred to keep them separate, easy
> > to revert if things break.
>
> With patchset applied to your scmi tree, my test still work as before.
>
> Tested-by: Peng Fan <peng.fan@nxp.com>
>

Thanks for testing. If you can give it a quick review as it is very small
change, that would be great. I just want more eyes to look at it just to
make sure I am not missing something obvious.

--
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH 1/4] firmware: arm_scmi: Make mutex channel specific

[Peng Fan]

Hi Sudeep,

> Subject: [PATCH 1/4] firmware: arm_scmi: Make mutex channel specific
> 
> In order to support multiple SMC/HVC transport channels with associated
> shared memory, 

Does this mean each channel will have its own shared memory? Or
All channels share the same shared memory?

it is better to maintain the mutex per channel instead of
> existing global one.

If all channels shared the same memory, use per channel mutex lock
will not be able to prevent other channels accessing shared memory
at the same time.  

Thanks,
Peng.
> 
> Move the smc_mutex into the scmi_smc structure and also rename it to
> shmem_lock which is more appropriate for it's use.
> 
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  drivers/firmware/arm_scmi/smc.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/firmware/arm_scmi/smc.c
> b/drivers/firmware/arm_scmi/smc.c index 336168e40f49..6dc8a88cc91b
> 100644
> --- a/drivers/firmware/arm_scmi/smc.c
> +++ b/drivers/firmware/arm_scmi/smc.c
> @@ -27,11 +27,10 @@
>  struct scmi_smc {
>  	struct scmi_chan_info *cinfo;
>  	struct scmi_shared_mem __iomem *shmem;
> +	struct mutex shmem_lock;
>  	u32 func_id;
>  };
> 
> -static DEFINE_MUTEX(smc_mutex);
> -
>  static bool smc_chan_available(struct device *dev, int idx)  {
>  	return true;
> @@ -78,6 +77,7 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo,
> struct device *dev,
> 
>  	scmi_info->func_id = func_id;
>  	scmi_info->cinfo = cinfo;
> +	mutex_init(&scmi_info->shmem_lock);
>  	cinfo->transport_info = scmi_info;
> 
>  	return 0;
> @@ -102,14 +102,14 @@ static int smc_send_message(struct
> scmi_chan_info *cinfo,
>  	struct scmi_smc *scmi_info = cinfo->transport_info;
>  	struct arm_smccc_res res;
> 
> -	mutex_lock(&smc_mutex);
> +	mutex_lock(&scmi_info->shmem_lock);
> 
>  	shmem_tx_prepare(scmi_info->shmem, xfer);
> 
>  	arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, &res);
>  	scmi_rx_callback(scmi_info->cinfo,
> shmem_read_header(scmi_info->shmem));
> 
> -	mutex_unlock(&smc_mutex);
> +	mutex_unlock(&scmi_info->shmem_lock);
> 
>  	return res.a0;
>  }
> --
> 2.17.1

RE: [PATCH 1/4] firmware: arm_scmi: Make mutex channel specific

[Peng Fan]

Hi Sudeep,

> Subject: [PATCH 1/4] firmware: arm_scmi: Make mutex channel specific
> 
> In order to support multiple SMC/HVC transport channels with associated
> shared memory, 

Does this mean each channel will have its own shared memory? Or
All channels share the same shared memory?

it is better to maintain the mutex per channel instead of
> existing global one.

If all channels shared the same memory, use per channel mutex lock
will not be able to prevent other channels accessing shared memory
at the same time.  

Thanks,
Peng.
> 
> Move the smc_mutex into the scmi_smc structure and also rename it to
> shmem_lock which is more appropriate for it's use.
> 
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  drivers/firmware/arm_scmi/smc.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/firmware/arm_scmi/smc.c
> b/drivers/firmware/arm_scmi/smc.c index 336168e40f49..6dc8a88cc91b
> 100644
> --- a/drivers/firmware/arm_scmi/smc.c
> +++ b/drivers/firmware/arm_scmi/smc.c
> @@ -27,11 +27,10 @@
>  struct scmi_smc {
>  	struct scmi_chan_info *cinfo;
>  	struct scmi_shared_mem __iomem *shmem;
> +	struct mutex shmem_lock;
>  	u32 func_id;
>  };
> 
> -static DEFINE_MUTEX(smc_mutex);
> -
>  static bool smc_chan_available(struct device *dev, int idx)  {
>  	return true;
> @@ -78,6 +77,7 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo,
> struct device *dev,
> 
>  	scmi_info->func_id = func_id;
>  	scmi_info->cinfo = cinfo;
> +	mutex_init(&scmi_info->shmem_lock);
>  	cinfo->transport_info = scmi_info;
> 
>  	return 0;
> @@ -102,14 +102,14 @@ static int smc_send_message(struct
> scmi_chan_info *cinfo,
>  	struct scmi_smc *scmi_info = cinfo->transport_info;
>  	struct arm_smccc_res res;
> 
> -	mutex_lock(&smc_mutex);
> +	mutex_lock(&scmi_info->shmem_lock);
> 
>  	shmem_tx_prepare(scmi_info->shmem, xfer);
> 
>  	arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, &res);
>  	scmi_rx_callback(scmi_info->cinfo,
> shmem_read_header(scmi_info->shmem));
> 
> -	mutex_unlock(&smc_mutex);
> +	mutex_unlock(&scmi_info->shmem_lock);
> 
>  	return res.a0;
>  }
> --
> 2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH 3/4] firmware: arm_scmi: Check shmem property for channel availablity

[Peng Fan]

> Subject: [PATCH 3/4] firmware: arm_scmi: Check shmem property for channel
> availablity
> 
> Instead of declaring the channel availabilty unconditionally, let us check for
> the presence of "shmem" property and return the channel availablity
> accordingly.
> 
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  drivers/firmware/arm_scmi/smc.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/firmware/arm_scmi/smc.c
> b/drivers/firmware/arm_scmi/smc.c index dd4b54c29679..5929c668dc1d
> 100644
> --- a/drivers/firmware/arm_scmi/smc.c
> +++ b/drivers/firmware/arm_scmi/smc.c
> @@ -33,6 +33,11 @@ struct scmi_smc {
> 
>  static bool smc_chan_available(struct device *dev, int idx)  {
> +	struct device_node *np = of_parse_phandle(dev->of_node, "shmem", 0);
> +	if (!np)
> +		return false;
> +
> +	of_node_put(np);
>  	return true;
>  }

This is global shared mem:)

Reviewed-by: Peng Fan <peng.fan@nxp.com>

> 
> --
> 2.17.1

RE: [PATCH 3/4] firmware: arm_scmi: Check shmem property for channel availablity

[Peng Fan]

> Subject: [PATCH 3/4] firmware: arm_scmi: Check shmem property for channel
> availablity
> 
> Instead of declaring the channel availabilty unconditionally, let us check for
> the presence of "shmem" property and return the channel availablity
> accordingly.
> 
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  drivers/firmware/arm_scmi/smc.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/firmware/arm_scmi/smc.c
> b/drivers/firmware/arm_scmi/smc.c index dd4b54c29679..5929c668dc1d
> 100644
> --- a/drivers/firmware/arm_scmi/smc.c
> +++ b/drivers/firmware/arm_scmi/smc.c
> @@ -33,6 +33,11 @@ struct scmi_smc {
> 
>  static bool smc_chan_available(struct device *dev, int idx)  {
> +	struct device_node *np = of_parse_phandle(dev->of_node, "shmem", 0);
> +	if (!np)
> +		return false;
> +
> +	of_node_put(np);
>  	return true;
>  }

This is global shared mem:)

Reviewed-by: Peng Fan <peng.fan@nxp.com>

> 
> --
> 2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH 2/4] firmware: arm_scmi: Drop empty stub for smc_mark_txdone

[Peng Fan]

> Subject: [PATCH 2/4] firmware: arm_scmi: Drop empty stub for
> smc_mark_txdone
> 
> The scmi protocol core driver check for non NULL mark_txdone before
> invoking the same. There is no need to provide a empty stub. SMC/HVC calls
> are synchronous and the call return indicates the completion.
> 
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

Reviewed-by: Peng Fan <peng.fan@nxp.com>

> ---
>  drivers/firmware/arm_scmi/smc.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/drivers/firmware/arm_scmi/smc.c
> b/drivers/firmware/arm_scmi/smc.c index 6dc8a88cc91b..dd4b54c29679
> 100644
> --- a/drivers/firmware/arm_scmi/smc.c
> +++ b/drivers/firmware/arm_scmi/smc.c
> @@ -114,10 +114,6 @@ static int smc_send_message(struct scmi_chan_info
> *cinfo,
>  	return res.a0;
>  }
> 
> -static void smc_mark_txdone(struct scmi_chan_info *cinfo, int ret) -{ -}
> -
>  static void smc_fetch_response(struct scmi_chan_info *cinfo,
>  			       struct scmi_xfer *xfer)
>  {
> @@ -139,7 +135,6 @@ static struct scmi_transport_ops scmi_smc_ops = {
>  	.chan_setup = smc_chan_setup,
>  	.chan_free = smc_chan_free,
>  	.send_message = smc_send_message,
> -	.mark_txdone = smc_mark_txdone,
>  	.fetch_response = smc_fetch_response,
>  	.poll_done = smc_poll_done,
>  };
> --
> 2.17.1

RE: [PATCH 2/4] firmware: arm_scmi: Drop empty stub for smc_mark_txdone

[Peng Fan]

> Subject: [PATCH 2/4] firmware: arm_scmi: Drop empty stub for
> smc_mark_txdone
> 
> The scmi protocol core driver check for non NULL mark_txdone before
> invoking the same. There is no need to provide a empty stub. SMC/HVC calls
> are synchronous and the call return indicates the completion.
> 
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

Reviewed-by: Peng Fan <peng.fan@nxp.com>

> ---
>  drivers/firmware/arm_scmi/smc.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/drivers/firmware/arm_scmi/smc.c
> b/drivers/firmware/arm_scmi/smc.c index 6dc8a88cc91b..dd4b54c29679
> 100644
> --- a/drivers/firmware/arm_scmi/smc.c
> +++ b/drivers/firmware/arm_scmi/smc.c
> @@ -114,10 +114,6 @@ static int smc_send_message(struct scmi_chan_info
> *cinfo,
>  	return res.a0;
>  }
> 
> -static void smc_mark_txdone(struct scmi_chan_info *cinfo, int ret) -{ -}
> -
>  static void smc_fetch_response(struct scmi_chan_info *cinfo,
>  			       struct scmi_xfer *xfer)
>  {
> @@ -139,7 +135,6 @@ static struct scmi_transport_ops scmi_smc_ops = {
>  	.chan_setup = smc_chan_setup,
>  	.chan_free = smc_chan_free,
>  	.send_message = smc_send_message,
> -	.mark_txdone = smc_mark_txdone,
>  	.fetch_response = smc_fetch_response,
>  	.poll_done = smc_poll_done,
>  };
> --
> 2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH 4/4] firmware: arm_scmi: Drop checking for shmem property in parent node

[Peng Fan]

> -----Original Message-----
> From: Sudeep Holla <sudeep.holla@arm.com>
> Sent: 2020年3月28日 0:37
> To: linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org; Peng
> Fan <peng.fan@nxp.com>
> Cc: Sudeep Holla <sudeep.holla@arm.com>; f.fainelli@gmail.com;
> dl-linux-imx <linux-imx@nxp.com>
> Subject: [PATCH 4/4] firmware: arm_scmi: Drop checking for shmem property
> in parent node
> 
> The scmi protocol core driver checks for the channel availability before
> evaluating the shmem property. If the individual protocols don't have separate
> channel assigned to them, the channel alloted for the BASE protocol is reused
> automatically.
> 
> Therefore there is no need to check for the shmem property in the parent
> node if it is absent in the child protocol node.
> 
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

Reviewed-by: Peng Fan <peng.fan@nxp.com>

> ---
>  drivers/firmware/arm_scmi/smc.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/firmware/arm_scmi/smc.c
> b/drivers/firmware/arm_scmi/smc.c index 5929c668dc1d..833e793b5391
> 100644
> --- a/drivers/firmware/arm_scmi/smc.c
> +++ b/drivers/firmware/arm_scmi/smc.c
> @@ -60,8 +60,6 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo,
> struct device *dev,
>  		return -ENOMEM;
> 
>  	np = of_parse_phandle(cdev->of_node, "shmem", 0);
> -	if (!np)
> -		np = of_parse_phandle(dev->of_node, "shmem", 0);
>  	ret = of_address_to_resource(np, 0, &res);
>  	of_node_put(np);
>  	if (ret) {
> --
> 2.17.1

RE: [PATCH 4/4] firmware: arm_scmi: Drop checking for shmem property in parent node

[Peng Fan]

> -----Original Message-----
> From: Sudeep Holla <sudeep.holla@arm.com>
> Sent: 2020年3月28日 0:37
> To: linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org; Peng
> Fan <peng.fan@nxp.com>
> Cc: Sudeep Holla <sudeep.holla@arm.com>; f.fainelli@gmail.com;
> dl-linux-imx <linux-imx@nxp.com>
> Subject: [PATCH 4/4] firmware: arm_scmi: Drop checking for shmem property
> in parent node
> 
> The scmi protocol core driver checks for the channel availability before
> evaluating the shmem property. If the individual protocols don't have separate
> channel assigned to them, the channel alloted for the BASE protocol is reused
> automatically.
> 
> Therefore there is no need to check for the shmem property in the parent
> node if it is absent in the child protocol node.
> 
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

Reviewed-by: Peng Fan <peng.fan@nxp.com>

> ---
>  drivers/firmware/arm_scmi/smc.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/firmware/arm_scmi/smc.c
> b/drivers/firmware/arm_scmi/smc.c index 5929c668dc1d..833e793b5391
> 100644
> --- a/drivers/firmware/arm_scmi/smc.c
> +++ b/drivers/firmware/arm_scmi/smc.c
> @@ -60,8 +60,6 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo,
> struct device *dev,
>  		return -ENOMEM;
> 
>  	np = of_parse_phandle(cdev->of_node, "shmem", 0);
> -	if (!np)
> -		np = of_parse_phandle(dev->of_node, "shmem", 0);
>  	ret = of_address_to_resource(np, 0, &res);
>  	of_node_put(np);
>  	if (ret) {
> --
> 2.17.1

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/4] firmware: arm_scmi: Check shmem property for channel availablity

[Sudeep Holla]

On Wed, Apr 01, 2020 at 01:15:08AM +0000, Peng Fan wrote:
> > Subject: [PATCH 3/4] firmware: arm_scmi: Check shmem property for channel
> > availablity
> >
> > Instead of declaring the channel availabilty unconditionally, let us check for
> > the presence of "shmem" property and return the channel availablity
> > accordingly.
> >
> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > ---
> >  drivers/firmware/arm_scmi/smc.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/firmware/arm_scmi/smc.c
> > b/drivers/firmware/arm_scmi/smc.c index dd4b54c29679..5929c668dc1d
> > 100644
> > --- a/drivers/firmware/arm_scmi/smc.c
> > +++ b/drivers/firmware/arm_scmi/smc.c
> > @@ -33,6 +33,11 @@ struct scmi_smc {
> >
> >  static bool smc_chan_available(struct device *dev, int idx)  {
> > +	struct device_node *np = of_parse_phandle(dev->of_node, "shmem", 0);
> > +	if (!np)
> > +		return false;
> > +
> > +	of_node_put(np);
> >  	return true;
> >  }
>
> This is global shared mem:)
>

No, the dev pointer is not the parent node here but the child devices
unless I am reading it wrong. But yes global for the base protocol :)

> Reviewed-by: Peng Fan <peng.fan@nxp.com>
>

Thanks for this and other patches too.

--
Regards,
Sudeep

Re: [PATCH 3/4] firmware: arm_scmi: Check shmem property for channel availablity

[Sudeep Holla]

On Wed, Apr 01, 2020 at 01:15:08AM +0000, Peng Fan wrote:
> > Subject: [PATCH 3/4] firmware: arm_scmi: Check shmem property for channel
> > availablity
> >
> > Instead of declaring the channel availabilty unconditionally, let us check for
> > the presence of "shmem" property and return the channel availablity
> > accordingly.
> >
> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > ---
> >  drivers/firmware/arm_scmi/smc.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/firmware/arm_scmi/smc.c
> > b/drivers/firmware/arm_scmi/smc.c index dd4b54c29679..5929c668dc1d
> > 100644
> > --- a/drivers/firmware/arm_scmi/smc.c
> > +++ b/drivers/firmware/arm_scmi/smc.c
> > @@ -33,6 +33,11 @@ struct scmi_smc {
> >
> >  static bool smc_chan_available(struct device *dev, int idx)  {
> > +	struct device_node *np = of_parse_phandle(dev->of_node, "shmem", 0);
> > +	if (!np)
> > +		return false;
> > +
> > +	of_node_put(np);
> >  	return true;
> >  }
>
> This is global shared mem:)
>

No, the dev pointer is not the parent node here but the child devices
unless I am reading it wrong. But yes global for the base protocol :)

> Reviewed-by: Peng Fan <peng.fan@nxp.com>
>

Thanks for this and other patches too.

--
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/4] firmware: arm_scmi: Make mutex channel specific

[Sudeep Holla]

On Wed, Apr 01, 2020 at 01:12:37AM +0000, Peng Fan wrote:
> Hi Sudeep,
>
> > Subject: [PATCH 1/4] firmware: arm_scmi: Make mutex channel specific
> >
> > In order to support multiple SMC/HVC transport channels with associated
> > shared memory,
>
> Does this mean each channel will have its own shared memory? Or
> All channels share the same shared memory?
>

It depends on platform firmware and DT. If there is only one shmem at
the top level scmi node, all share that single channel. If some/all
protocols have their own channel, they there must be shmem entry in the
corresponding child node.

> it is better to maintain the mutex per channel instead of
> > existing global one.
>
> If all channels shared the same memory, use per channel mutex lock
> will not be able to prevent other channels accessing shared memory
> at the same time.
>

No we don't create channel per protocol. If they share, we just share
the channel pointer. Look at:

       if (!info->desc->ops->chan_available(dev, idx)) {
                cinfo = idr_find(idr, SCMI_PROTOCOL_BASE);
                if (unlikely(!cinfo)) /* Possible only if platform has no Rx */
                        return -EINVAL;
                goto idr_alloc;
        }

If a protocol doesn't have a dedicated channel, we just assign the base
protocol channel to it. We don't call chan_setup at all on that channel.
Your patch assumed so but the core driver never did that.

Hope this clarifies you doubt.

--
Regards,
Sudeep

Re: [PATCH 1/4] firmware: arm_scmi: Make mutex channel specific

[Sudeep Holla]

On Wed, Apr 01, 2020 at 01:12:37AM +0000, Peng Fan wrote:
> Hi Sudeep,
>
> > Subject: [PATCH 1/4] firmware: arm_scmi: Make mutex channel specific
> >
> > In order to support multiple SMC/HVC transport channels with associated
> > shared memory,
>
> Does this mean each channel will have its own shared memory? Or
> All channels share the same shared memory?
>

It depends on platform firmware and DT. If there is only one shmem at
the top level scmi node, all share that single channel. If some/all
protocols have their own channel, they there must be shmem entry in the
corresponding child node.

> it is better to maintain the mutex per channel instead of
> > existing global one.
>
> If all channels shared the same memory, use per channel mutex lock
> will not be able to prevent other channels accessing shared memory
> at the same time.
>

No we don't create channel per protocol. If they share, we just share
the channel pointer. Look at:

       if (!info->desc->ops->chan_available(dev, idx)) {
                cinfo = idr_find(idr, SCMI_PROTOCOL_BASE);
                if (unlikely(!cinfo)) /* Possible only if platform has no Rx */
                        return -EINVAL;
                goto idr_alloc;
        }

If a protocol doesn't have a dedicated channel, we just assign the base
protocol channel to it. We don't call chan_setup at all on that channel.
Your patch assumed so but the core driver never did that.

Hope this clarifies you doubt.

--
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH 1/4] firmware: arm_scmi: Make mutex channel specific

[Peng Fan]

> Subject: Re: [PATCH 1/4] firmware: arm_scmi: Make mutex channel specific
> 
> On Wed, Apr 01, 2020 at 01:12:37AM +0000, Peng Fan wrote:
> > Hi Sudeep,
> >
> > > Subject: [PATCH 1/4] firmware: arm_scmi: Make mutex channel specific
> > >
> > > In order to support multiple SMC/HVC transport channels with
> > > associated shared memory,
> >
> > Does this mean each channel will have its own shared memory? Or All
> > channels share the same shared memory?
> >
> 
> It depends on platform firmware and DT. If there is only one shmem at the top
> level scmi node, all share that single channel. If some/all protocols have their
> own channel, they there must be shmem entry in the corresponding child
> node.
> 
> > it is better to maintain the mutex per channel instead of
> > > existing global one.
> >
> > If all channels shared the same memory, use per channel mutex lock
> > will not be able to prevent other channels accessing shared memory at
> > the same time.
> >
> 
> No we don't create channel per protocol. If they share, we just share the
> channel pointer. Look at:
> 
>        if (!info->desc->ops->chan_available(dev, idx)) {
>                 cinfo = idr_find(idr, SCMI_PROTOCOL_BASE);
>                 if (unlikely(!cinfo)) /* Possible only if platform has no Rx */
>                         return -EINVAL;
>                 goto idr_alloc;
>         }
> 
> If a protocol doesn't have a dedicated channel, we just assign the base
> protocol channel to it. We don't call chan_setup at all on that channel.
> Your patch assumed so but the core driver never did that.
> 
> Hope this clarifies you doubt.

Yes. Thanks for the explainaiton.

Thanks,
Peng.

> 
> --
> Regards,
> Sudeep

RE: [PATCH 1/4] firmware: arm_scmi: Make mutex channel specific

[Peng Fan]

> Subject: Re: [PATCH 1/4] firmware: arm_scmi: Make mutex channel specific
> 
> On Wed, Apr 01, 2020 at 01:12:37AM +0000, Peng Fan wrote:
> > Hi Sudeep,
> >
> > > Subject: [PATCH 1/4] firmware: arm_scmi: Make mutex channel specific
> > >
> > > In order to support multiple SMC/HVC transport channels with
> > > associated shared memory,
> >
> > Does this mean each channel will have its own shared memory? Or All
> > channels share the same shared memory?
> >
> 
> It depends on platform firmware and DT. If there is only one shmem at the top
> level scmi node, all share that single channel. If some/all protocols have their
> own channel, they there must be shmem entry in the corresponding child
> node.
> 
> > it is better to maintain the mutex per channel instead of
> > > existing global one.
> >
> > If all channels shared the same memory, use per channel mutex lock
> > will not be able to prevent other channels accessing shared memory at
> > the same time.
> >
> 
> No we don't create channel per protocol. If they share, we just share the
> channel pointer. Look at:
> 
>        if (!info->desc->ops->chan_available(dev, idx)) {
>                 cinfo = idr_find(idr, SCMI_PROTOCOL_BASE);
>                 if (unlikely(!cinfo)) /* Possible only if platform has no Rx */
>                         return -EINVAL;
>                 goto idr_alloc;
>         }
> 
> If a protocol doesn't have a dedicated channel, we just assign the base
> protocol channel to it. We don't call chan_setup at all on that channel.
> Your patch assumed so but the core driver never did that.
> 
> Hope this clarifies you doubt.

Yes. Thanks for the explainaiton.

Thanks,
Peng.

> 
> --
> Regards,
> Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH 1/4] firmware: arm_scmi: Make mutex channel specific

[Peng Fan]

> Subject: [PATCH 1/4] firmware: arm_scmi: Make mutex channel specific
> 
> In order to support multiple SMC/HVC transport channels with associated
> shared memory, it is better to maintain the mutex per channel instead of
> existing global one.
> 
> Move the smc_mutex into the scmi_smc structure and also rename it to
> shmem_lock which is more appropriate for it's use.
> 
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

Reviewed-by: Peng Fan <peng.fan@nxp.com>

> ---
>  drivers/firmware/arm_scmi/smc.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/firmware/arm_scmi/smc.c
> b/drivers/firmware/arm_scmi/smc.c index 336168e40f49..6dc8a88cc91b
> 100644
> --- a/drivers/firmware/arm_scmi/smc.c
> +++ b/drivers/firmware/arm_scmi/smc.c
> @@ -27,11 +27,10 @@
>  struct scmi_smc {
>  	struct scmi_chan_info *cinfo;
>  	struct scmi_shared_mem __iomem *shmem;
> +	struct mutex shmem_lock;
>  	u32 func_id;
>  };
> 
> -static DEFINE_MUTEX(smc_mutex);
> -
>  static bool smc_chan_available(struct device *dev, int idx)  {
>  	return true;
> @@ -78,6 +77,7 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo,
> struct device *dev,
> 
>  	scmi_info->func_id = func_id;
>  	scmi_info->cinfo = cinfo;
> +	mutex_init(&scmi_info->shmem_lock);
>  	cinfo->transport_info = scmi_info;
> 
>  	return 0;
> @@ -102,14 +102,14 @@ static int smc_send_message(struct
> scmi_chan_info *cinfo,
>  	struct scmi_smc *scmi_info = cinfo->transport_info;
>  	struct arm_smccc_res res;
> 
> -	mutex_lock(&smc_mutex);
> +	mutex_lock(&scmi_info->shmem_lock);
> 
>  	shmem_tx_prepare(scmi_info->shmem, xfer);
> 
>  	arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, &res);
>  	scmi_rx_callback(scmi_info->cinfo,
> shmem_read_header(scmi_info->shmem));
> 
> -	mutex_unlock(&smc_mutex);
> +	mutex_unlock(&scmi_info->shmem_lock);
> 
>  	return res.a0;
>  }
> --
> 2.17.1

RE: [PATCH 1/4] firmware: arm_scmi: Make mutex channel specific

[Peng Fan]

> Subject: [PATCH 1/4] firmware: arm_scmi: Make mutex channel specific
> 
> In order to support multiple SMC/HVC transport channels with associated
> shared memory, it is better to maintain the mutex per channel instead of
> existing global one.
> 
> Move the smc_mutex into the scmi_smc structure and also rename it to
> shmem_lock which is more appropriate for it's use.
> 
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

Reviewed-by: Peng Fan <peng.fan@nxp.com>

> ---
>  drivers/firmware/arm_scmi/smc.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/firmware/arm_scmi/smc.c
> b/drivers/firmware/arm_scmi/smc.c index 336168e40f49..6dc8a88cc91b
> 100644
> --- a/drivers/firmware/arm_scmi/smc.c
> +++ b/drivers/firmware/arm_scmi/smc.c
> @@ -27,11 +27,10 @@
>  struct scmi_smc {
>  	struct scmi_chan_info *cinfo;
>  	struct scmi_shared_mem __iomem *shmem;
> +	struct mutex shmem_lock;
>  	u32 func_id;
>  };
> 
> -static DEFINE_MUTEX(smc_mutex);
> -
>  static bool smc_chan_available(struct device *dev, int idx)  {
>  	return true;
> @@ -78,6 +77,7 @@ static int smc_chan_setup(struct scmi_chan_info *cinfo,
> struct device *dev,
> 
>  	scmi_info->func_id = func_id;
>  	scmi_info->cinfo = cinfo;
> +	mutex_init(&scmi_info->shmem_lock);
>  	cinfo->transport_info = scmi_info;
> 
>  	return 0;
> @@ -102,14 +102,14 @@ static int smc_send_message(struct
> scmi_chan_info *cinfo,
>  	struct scmi_smc *scmi_info = cinfo->transport_info;
>  	struct arm_smccc_res res;
> 
> -	mutex_lock(&smc_mutex);
> +	mutex_lock(&scmi_info->shmem_lock);
> 
>  	shmem_tx_prepare(scmi_info->shmem, xfer);
> 
>  	arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, &res);
>  	scmi_rx_callback(scmi_info->cinfo,
> shmem_read_header(scmi_info->shmem));
> 
> -	mutex_unlock(&smc_mutex);
> +	mutex_unlock(&scmi_info->shmem_lock);
> 
>  	return res.a0;
>  }
> --
> 2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/4] firmware: arm_scmi: Make mutex channel specific

[Sudeep Holla]

On Wed, Apr 01, 2020 at 09:14:36AM +0000, Peng Fan wrote:
> > Subject: Re: [PATCH 1/4] firmware: arm_scmi: Make mutex channel specific
> >
> > On Wed, Apr 01, 2020 at 01:12:37AM +0000, Peng Fan wrote:
> > > Hi Sudeep,
> > >
> > > > Subject: [PATCH 1/4] firmware: arm_scmi: Make mutex channel specific
> > > >
> > > > In order to support multiple SMC/HVC transport channels with
> > > > associated shared memory,
> > >
> > > Does this mean each channel will have its own shared memory? Or All
> > > channels share the same shared memory?
> > >
> >
> > It depends on platform firmware and DT. If there is only one shmem at the top
> > level scmi node, all share that single channel. If some/all protocols have their
> > own channel, they there must be shmem entry in the corresponding child
> > node.
> >
> > > it is better to maintain the mutex per channel instead of
> > > > existing global one.
> > >
> > > If all channels shared the same memory, use per channel mutex lock
> > > will not be able to prevent other channels accessing shared memory at
> > > the same time.
> > >
> >
> > No we don't create channel per protocol. If they share, we just share the
> > channel pointer. Look at:
> >
> >        if (!info->desc->ops->chan_available(dev, idx)) {
> >                 cinfo = idr_find(idr, SCMI_PROTOCOL_BASE);
> >                 if (unlikely(!cinfo)) /* Possible only if platform has no Rx */
> >                         return -EINVAL;
> >                 goto idr_alloc;
> >         }
> >
> > If a protocol doesn't have a dedicated channel, we just assign the base
> > protocol channel to it. We don't call chan_setup at all on that channel.
> > Your patch assumed so but the core driver never did that.
> >
> > Hope this clarifies you doubt.
>
> Yes. Thanks for the explainaiton.
>

No worries, I should have seen this during initial review, just missed
few trivial things.

--
Regards,
Sudeep

Re: [PATCH 1/4] firmware: arm_scmi: Make mutex channel specific

[Sudeep Holla]

On Wed, Apr 01, 2020 at 09:14:36AM +0000, Peng Fan wrote:
> > Subject: Re: [PATCH 1/4] firmware: arm_scmi: Make mutex channel specific
> >
> > On Wed, Apr 01, 2020 at 01:12:37AM +0000, Peng Fan wrote:
> > > Hi Sudeep,
> > >
> > > > Subject: [PATCH 1/4] firmware: arm_scmi: Make mutex channel specific
> > > >
> > > > In order to support multiple SMC/HVC transport channels with
> > > > associated shared memory,
> > >
> > > Does this mean each channel will have its own shared memory? Or All
> > > channels share the same shared memory?
> > >
> >
> > It depends on platform firmware and DT. If there is only one shmem at the top
> > level scmi node, all share that single channel. If some/all protocols have their
> > own channel, they there must be shmem entry in the corresponding child
> > node.
> >
> > > it is better to maintain the mutex per channel instead of
> > > > existing global one.
> > >
> > > If all channels shared the same memory, use per channel mutex lock
> > > will not be able to prevent other channels accessing shared memory at
> > > the same time.
> > >
> >
> > No we don't create channel per protocol. If they share, we just share the
> > channel pointer. Look at:
> >
> >        if (!info->desc->ops->chan_available(dev, idx)) {
> >                 cinfo = idr_find(idr, SCMI_PROTOCOL_BASE);
> >                 if (unlikely(!cinfo)) /* Possible only if platform has no Rx */
> >                         return -EINVAL;
> >                 goto idr_alloc;
> >         }
> >
> > If a protocol doesn't have a dedicated channel, we just assign the base
> > protocol channel to it. We don't call chan_setup at all on that channel.
> > Your patch assumed so but the core driver never did that.
> >
> > Hope this clarifies you doubt.
>
> Yes. Thanks for the explainaiton.
>

No worries, I should have seen this during initial review, just missed
few trivial things.

--
Regards,
Sudeep

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel