Re: [PATCH v4 43/45] Add missed BCM2835 properties

[Peter Maydell]

On Fri, 8 Dec 2023 at 02:36, Sergey Kambalin <serg.oker@gmail.com> wrote:
>
> Signed-off-by: Sergey Kambalin <sergey.kambalin@auriga.com>
> ---
>  hw/misc/bcm2835_property.c           | 47 ++++++++++++++++++++++++++++
>  include/hw/arm/raspberrypi-fw-defs.h | 12 ++++++-
>  2 files changed, 58 insertions(+), 1 deletion(-)
>
> diff --git a/hw/misc/bcm2835_property.c b/hw/misc/bcm2835_property.c
> index ff55a4e2cd..dfeb793b3e 100644
> --- a/hw/misc/bcm2835_property.c
> +++ b/hw/misc/bcm2835_property.c
> @@ -19,6 +19,9 @@
>  #include "trace.h"
>  #include "hw/arm/raspi_platform.h"
>
> +#define RPI_EXP_GPIO_BASE       128

This define doesn't seem to be used in this patch.

> +#define VCHI_BUSADDR_SIZE       sizeof(uint32_t)
> +
>  /* https://github.com/raspberrypi/firmware/wiki/Mailbox-property-interface */
>
>  static void bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t 
> value)
> @@ -138,6 +141,13 @@ static void 
> bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
>              resplen = 8;
>              break;
>
> +        case RPI_FWREQ_GET_CLOCKS:
> +            /* TODO: add more clock IDs if needed */
> +            stl_le_phys(&s->dma_as, value + 12, 0);
> +            stl_le_phys(&s->dma_as, value + 16, RPI_FIRMWARE_ARM_CLK_ID);
> +            resplen = 8;
> +            break;
> +
>          case RPI_FWREQ_SET_CLOCK_RATE:
>          case RPI_FWREQ_SET_MAX_CLOCK_RATE:
>          case RPI_FWREQ_SET_MIN_CLOCK_RATE:
> @@ -276,6 +286,7 @@ static void 
> bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
>              stl_le_phys(&s->dma_as, value + 12, 0);
>              resplen = 4;
>              break;
> +
>          case RPI_FWREQ_FRAMEBUFFER_GET_NUM_DISPLAYS:
>              stl_le_phys(&s->dma_as, value + 12, 1);
>              resplen = 4;
> @@ -301,6 +312,42 @@ static void 
> bcm2835_property_mbox_push(BCM2835PropertyState *s, uint32_t value)
>                                      resplen);
>              break;
>
> +        case RPI_FWREQ_GET_THROTTLED:
> +            stl_le_phys(&s->dma_as, value + 12, 0);
> +            resplen = 4;
> +            break;
> +
> +        case RPI_FWREQ_FRAMEBUFFER_SET_PITCH:
> +            qemu_log_mask(LOG_UNIMP, "RPI_FWREQ_FRAMEBUFFER_SET_PITCH "
> +                              "is not implemented\n");
> +            break;
> +
> +        case RPI_FWREQ_GET_GPIO_CONFIG:
> +            qemu_log_mask(LOG_UNIMP, "RPI_FWREQ_GET_GPIO_CONFIG "
> +                          "is not implemented\n");
> +            break;
> +
> +        case RPI_FWREQ_SET_GPIO_CONFIG:
> +            qemu_log_mask(LOG_UNIMP, "RPI_FWREQ_SET_GPIO_CONFIG "
> +                          "is not implemented\n");
> +            break;
> +
> +        case RPI_FWREQ_GET_GPIO_STATE:
> +            qemu_log_mask(LOG_UNIMP, "RPI_FWREQ_GET_GPIO_STATE "
> +                          "is not implemented\n");
> +            break;
> +
> +        case RPI_FWREQ_SET_GPIO_STATE:
> +            qemu_log_mask(LOG_UNIMP, "RPI_FWREQ_SET_GPIO_STATE "
> +                          "is not implemented\n");
> +            break;

These don't do anything that isn't already covered by the
"default" case. The only reason to have a case specifically
for an unimplemented property is for where we need to at least
get the response length correct to avoid the guest falling over.
You'll note that all the other LOG_UNIMP cases in this switch
set resplen.

> +
> +        case RPI_FWREQ_VCHIQ_INIT:
> +            stl_le_phys(&s->dma_as,
> +                        value + offsetof(rpi_firmware_prop_request_t, 
> payload),
> +                        0);
> +            resplen = VCHI_BUSADDR_SIZE;
> +            break;
>          default:
>              qemu_log_mask(LOG_UNIMP,
>                            "bcm2835_property: unhandled tag 0x%08x\n", tag);
> diff --git a/include/hw/arm/raspberrypi-fw-defs.h 
> b/include/hw/arm/raspberrypi-fw-defs.h
> index 4551fe7450..ded7a22f02 100644
> --- a/include/hw/arm/raspberrypi-fw-defs.h
> +++ b/include/hw/arm/raspberrypi-fw-defs.h
> @@ -101,7 +101,6 @@ enum rpi_firmware_property_tag {
>      RPI_FWREQ_FRAMEBUFFER_GET_DISPLAY_ID =             0x00040016,
>      RPI_FWREQ_FRAMEBUFFER_SET_DISPLAY_NUM =            0x00048013,
>      RPI_FWREQ_FRAMEBUFFER_GET_NUM_DISPLAYS =           0x00040013,
> -    RPI_FWREQ_FRAMEBUFFER_GET_DISPLAY_SETTINGS =       0x00040014,

Why do we delete this ?

>      RPI_FWREQ_FRAMEBUFFER_TEST_PHYSICAL_WIDTH_HEIGHT = 0x00044003,
>      RPI_FWREQ_FRAMEBUFFER_TEST_VIRTUAL_WIDTH_HEIGHT =  0x00044004,
>      RPI_FWREQ_FRAMEBUFFER_TEST_DEPTH =                 0x00044005,
> @@ -160,4 +159,15 @@ enum rpi_firmware_clk_id {
>      RPI_FIRMWARE_NUM_CLK_ID,
>  };
>
> +struct rpi_firmware_property_tag_header {
> +    uint32_t tag;
> +    uint32_t buf_size;
> +    uint32_t req_resp_size;
> +};
> +
> +typedef struct rpi_firmware_prop_request {
> +    struct rpi_firmware_property_tag_header hdr;
> +    uint8_t payload[0];
> +} rpi_firmware_prop_request_t;
> +
>  #endif /* INCLUDE_HW_MISC_RASPBERRYPI_FW_DEFS_H_ */

thanks
-- PMM