ARC Feedback Updates in Message protocol, Transport, Base, CPPC #84

pathakraul · 2024-12-25T04:04:27Z

pathakraul · 2024-12-25T04:06:39Z

pathakraul · 2024-12-25T04:10:28Z

Couple of recent comments are still pending from ARC to be taken care of. Which I am working on and will raise separate PR for that

avpatel · 2024-12-26T05:11:42Z

src/srvgrp-base.adoc

@@ -62,6 +62,15 @@ The following table lists the services in the BASE service group:
 | 0x08
 | BASE_SET_MSI
 | NORMAL_REQUEST
+
+| 0x09
+| BASE_GET_MSI


Missing BASE_SET_MSI

on line 62-64

the subject and description of commit is misleading as it does not give full information, I will update it before merging in the end

avpatel · 2024-12-26T05:22:55Z

src/srvgrp-base.adoc

+
+
+[#srvgrp_base_config_msi]
+==== Service: BASE_CONFIG_MSI (SERVICE_ID: 0x0A)


s/BASE_CONFIG_MSI/BASE_SET_MSI_TARGET/

Should we then rename BASE_SET_MSI to BASE_SET_MSI_STATE?. Otherwise with above rename it gets confusing IMO

avpatel · 2024-12-26T05:43:07Z

src/transport.adoc

+if `\((tail + 1) % (M - 2)) == head`.
+
+NOTE: The queues and queue state are shared between application processors, and
+due to mechanisms such as kexec and others that can spawn another OS from the


Use "OS / Firmware" instead of just "OS" because in the future some M-mode firmware may also implement kexec() style feature.

lftan · 2024-12-26T08:46:03Z

src/transport.adoc

@@ -275,7 +275,10 @@ enqueue the message.
 In an implementation, a queue is empty if the `head` == `tail` and a queue is full
 if `\((tail + 1) % (M - 2)) == head`.

-NOTE: 
+NOTE: The queues and queue state are shared between application processors, and


state -> states

lftan · 2024-12-26T08:49:45Z

src/message-protocol.adoc

-	0b100 - 0b111: Reserved for future use.
-
+----
+FLAGS[3]: Doorbell interrupt request.


Some field names use full capital letters but some are not.

From the start we gave names to some of the fields which were capitalized, some we didn't. I think lets make all non capitalized.

lftan · 2024-12-26T08:52:14Z

src/message-protocol.adoc

-selectively disable it so that the platform microcontroller in that case does
-not trigger a doorbell.
+If the doorbell interrupts are supported and enabled, the sender of the RPMI
+normal request message can set the `flags[3]: Doorbell interrupt request` bit to


should use FLAGS[3] , so that same with the field name above.
Apply the same for the rest of description below.

lftan · 2024-12-26T09:01:35Z

src/srvgrp-cppc.adoc


 | 8
 | DB_SETMASK_LOW
 | uint32
-| Lower 32-bit of doorbell set mask.
+| Lower 32-bit of doorbell set mask for Performance Request fast-channel. +
+Only the lower bits of this field equal to the doorbell register width must be used


this sentence a bit confusing.
Do you mean "Only the bits within this field corresponding to the doorbell register width should be used as the bitmask."?

The register width can be lower than the width of this field and in that case the bitmask is always equal to the register width from the LSB side.

I will try to format this line here and other place

lftan · 2024-12-26T09:05:53Z

src/srvgrp-base.adoc

+If the MSI support is available, the application processor can enable the
+MSI using this service. If the MSI is not supported, the appropriate error code
+is returned. After the MSI is enabled, the configurations such as MSI address and
+MSI data can be configured using another service `BASE_CONFIG_MSI`. The platform


Should call BASE_CONFIG_MSI , then only SET_MSI enable/disable?

I dont understand your question

lftan · 2024-12-26T09:07:58Z

src/srvgrp-base.adoc

+
+The application processor can also use this service to disable the MSI. Once
+disabled, if the application processor enables the MSI again then it must call
+`BASE_CONFIG_MSI` again to configure the MSI address and MSI data.


disable MSI will void the MSI address and data?

if that's the case, then can include set MSI address and data parameters to SET_MSI service.
Then don't need to have CONFIG_MSI service.

That another possible way agree, but there had been discussion on the ARC feedback issue to have separate enable/disable service along with configuration service.
With this approach the MSI configuration does not have a case of getting passed as invalid values

disable MSI will void the MSI address and data?

What happens with the MSI configuration is not necessary, Spec mandates that Configuration must be done again. What PuC does to address and data is not necessary

pathakraul · 2024-12-29T10:34:36Z

src/srvgrp-base.adoc

+| 1
+| STATE
+| uint32
+| MSI state


Having second thoughts to convert this into a bitfield. This will enable multiple combination of states to be returned. Otherwise currently we need to define a unique integer value for every combination of state. For example "Enable only" and "Enable and Configured".

I will go ahead and do that. Please comment it there are objections

Something like below -

STATE[31:2] : Reserved STATE[1]: 1: if MSI Target Configured/Valid, 0 otherwise STATE[0]: 1: if MSI Enabled, 0 MSI Disabled

Add new section meant for software implementaiton of shared memory queues which will cover the queue discovery and also the ARC feedback to to define the queue initialization details, queue enpty and full condition for implemenations as normative text Signed-off-by: Rahul Pathak <[email protected]>

…ke mechanisms ARC feedback was to highlight the possibility that some message may be delivered to the OS which are not intended for that OS can be ignored Signed-off-by: Rahul Pathak <[email protected]>

…quest Feedback from ARC suggested to change the name of flags[3] bit to Doorbell interrupt requested. Also, elaborate when the flags[3] bit is applicable and other behaviours Signed-off-by: Rahul Pathak <[email protected]>

As per ARC feedback the TOKEN described as sequence number is not right and its simply a integer number given to a RPMI message. Also describe in non-normative text that how TOKEN is used for different RPMI message types Signed-off-by: Rahul Pathak <[email protected]>

CPPC service group already supports the service to get the offsets of the fastchannels for each application processor and the requirement for groupping the fastchannels together is not necessary Signed-off-by: Rahul Pathak <[email protected]>

As per ARC feedback, explain about the fastchannel doorbell fields. Signed-off-by: Rahul Pathak <[email protected]>

pathakraul · 2024-12-30T12:28:43Z

@avpatel @lftan I have addressed your comments. Please check.

avpatel · 2024-12-30T15:39:45Z

src/transport.adoc

@@ -77,6 +77,9 @@ If the P2A doorbell is a MSI then the application processors must configure
 the MSI on the platform microcontroller side using the RPMI service defined by
 the `BASE` service group.

+The doorbell register must support write operation or a read-modify-write sequence


This is required for the A2P doorbell not P2A doorbell.

I have updated the text for A2P now

avpatel · 2024-12-30T17:11:06Z

src/srvgrp-base.adoc

+
+If the MSI support is available, the application processor can enable the
+MSI using this service. If the MSI is not supported, the appropriate error code
+is returned. After the MSI is enabled, the configurations such as MSI target


First time, the MSI target address is configured before enabling the MSI so this statement looks confusing.

I can add the text something like - The application processor can enable and configure the MSI in any order but both must be done to allow platform microcontroller to trigger the MSI doorbell

What do you think?

I have relaxed the requirement where enable and configure can be done in any order but both must be done.

avpatel · 2024-12-31T10:37:18Z

src/srvgrp-base.adoc

+is done by another defined service `BASE_SET_MSI_TARGET`.
+
+The application processor can also use this service to disable the MSI. Once
+disabled, if the application processor enables the MSI again then it must call


Application process can set MSI target any number of times it want. There is no correlation with MSI enabling/disabling.

Just drop this second sentence "Once disabled, ..."

avpatel · 2024-12-31T10:40:25Z

src/srvgrp-base.adoc

+platform microcontroller can use as a doorbell to the application processor. The
+application processor may first discover the MSI support via the
+`BASE_GET_ATTRIBUTES` service. The application processor can enable the MSI
+using `BASE_SET_MSI_STATE` and configure the target in any order but it must


As mentioned above, there is STATE required for MSI Target configuration.

pathakraul · 2024-12-31T11:27:23Z

src/srvgrp-base.adoc

+! [31:2]
+! _Reserved_ and must be `0`.
+
+! [1]


Ok, if we dont need to maintain any invalid MSI target configuration anytime then current is not needed. I will update

pathakraul · 2025-01-02T11:33:53Z

@avpatel Update the text as per the comments.

avpatel · 2025-01-02T14:54:55Z

src/srvgrp-base.adoc

+[#srvgrp_base_get_msi_state]
+==== Service: BASE_GET_MSI_STATE (SERVICE_ID: 0x09)
+This service is used to get the state of the MSI, whether the MSI is
+enabled/disabled or if the MSI is currently pending as the platform microcontroller


s/is currently pending as the platform/is currently pending and the platform/

avpatel · 2025-01-02T15:02:51Z

only one minor comment in src/srvgrp-base.adoc
otherwise it looks good to me.

As per ARC feedback the BASE service group must also have provision to disable the MSI. To support that and also for clarity, add separate services to set the MSI state which enable/disables the MSI, service to get the current MSI state and a service to configure the MSI target. Signed-off-by: Rahul Pathak <[email protected]>

Signed-off-by: Rahul Pathak <[email protected]>

avpatel reviewed Dec 26, 2024

View reviewed changes

lftan reviewed Dec 26, 2024

View reviewed changes

pathakraul commented Dec 29, 2024

View reviewed changes

pathakraul added 6 commits December 30, 2024 09:22

transport: arc: Add non-normative text for situations due to kexec li…

27aae00

…ke mechanisms ARC feedback was to highlight the possibility that some message may be delivered to the OS which are not intended for that OS can be ignored Signed-off-by: Rahul Pathak <[email protected]>

cppc: arc: Explain performance request doorbell details

dc105e0

As per ARC feedback, explain about the fastchannel doorbell fields. Signed-off-by: Rahul Pathak <[email protected]>

pathakraul force-pushed the rpathak_arc_review_79_v2 branch from 7303c48 to a5af049 Compare December 30, 2024 12:26

avpatel reviewed Dec 30, 2024

View reviewed changes

pathakraul force-pushed the rpathak_arc_review_79_v2 branch 2 times, most recently from 565c8f8 to 5738b7b Compare December 30, 2024 16:22

avpatel reviewed Dec 30, 2024

View reviewed changes

pathakraul force-pushed the rpathak_arc_review_79_v2 branch from 29ebe74 to 0b92002 Compare December 31, 2024 04:59

avpatel reviewed Dec 31, 2024

View reviewed changes

pathakraul commented Dec 31, 2024

View reviewed changes

pathakraul force-pushed the rpathak_arc_review_79_v2 branch from 0b92002 to e5db4fe Compare January 2, 2025 10:59

avpatel reviewed Jan 2, 2025

View reviewed changes

pathakraul added 4 commits January 2, 2025 20:35

lib: Formatting of table fields and improvements in spec

5da6937

Signed-off-by: Rahul Pathak <[email protected]>

transport: Specify the doorbell register width for A2P doorbell

76b503e

Signed-off-by: Rahul Pathak <[email protected]>

lib: Formatting improvements in whole spec

ddcfae7

Signed-off-by: Rahul Pathak <[email protected]>

pathakraul force-pushed the rpathak_arc_review_79_v2 branch from e5db4fe to ddcfae7 Compare January 2, 2025 15:08

pathakraul merged commit 0dca1ec into riscv-non-isa:main Jan 2, 2025
1 check passed

pathakraul deleted the rpathak_arc_review_79_v2 branch January 2, 2025 15:11



		[#srvgrp_base_config_msi]
		==== Service: BASE_CONFIG_MSI (SERVICE_ID: 0x0A)

ARC Feedback Updates in Message protocol, Transport, Base, CPPC #84

ARC Feedback Updates in Message protocol, Transport, Base, CPPC #84

Conversation

pathakraul commented Dec 25, 2024

pathakraul commented Dec 25, 2024 • edited Loading

pathakraul commented Dec 25, 2024

avpatel Dec 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pathakraul Dec 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lftan Dec 26, 2024 • edited Loading

Choose a reason for hiding this comment

pathakraul Dec 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pathakraul Dec 29, 2024 • edited Loading

Choose a reason for hiding this comment

pathakraul commented Dec 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pathakraul commented Jan 2, 2025

Choose a reason for hiding this comment

avpatel commented Jan 2, 2025

pathakraul commented Dec 25, 2024 •

edited

Loading

avpatel Dec 26, 2024 •

edited

Loading

pathakraul Dec 26, 2024 •

edited

Loading

lftan Dec 26, 2024 •

edited

Loading

pathakraul Dec 26, 2024 •

edited

Loading

pathakraul Dec 29, 2024 •

edited

Loading