On 22 February 2017 at 11:23, Clement Deschamps
<clement.descha...@antfield.fr> wrote:
> This adds the BCM2835 GPIO controller.
>
> It implements:
> - The 54 GPIOs as outputs (qemu_irq)
> - The SD controller selection via alternate function of GPIOs 48-53
>
> Signed-off-by: Clement Deschamps <clement.descha...@antfield.fr>
> ---
> hw/gpio/Makefile.objs | 1 +
> hw/gpio/bcm2835_gpio.c | 361
> +++++++++++++++++++++++++++++++++++++++++
> include/hw/gpio/bcm2835_gpio.h | 38 +++++
> 3 files changed, 400 insertions(+)
> create mode 100644 hw/gpio/bcm2835_gpio.c
> create mode 100644 include/hw/gpio/bcm2835_gpio.h
>
> diff --git a/hw/gpio/Makefile.objs b/hw/gpio/Makefile.objs
> index a43c7cf442..fa0a72e6d0 100644
> --- a/hw/gpio/Makefile.objs
> +++ b/hw/gpio/Makefile.objs
> @@ -7,3 +7,4 @@ common-obj-$(CONFIG_GPIO_KEY) += gpio_key.o
>
> obj-$(CONFIG_OMAP) += omap_gpio.o
> obj-$(CONFIG_IMX) += imx_gpio.o
> +obj-$(CONFIG_RASPI) += bcm2835_gpio.o
> diff --git a/hw/gpio/bcm2835_gpio.c b/hw/gpio/bcm2835_gpio.c
> new file mode 100644
> index 0000000000..a75f0ebb6b
> --- /dev/null
> +++ b/hw/gpio/bcm2835_gpio.c
> @@ -0,0 +1,361 @@
> +/*
> + * Raspberry Pi (BCM2835) GPIO Controller
> + *
> + * Copyright (c) 2017 Antfield SAS
> + *
> + * Authors:
> + * Clement Deschamps <clement.descha...@antfield.fr>
> + * Luc Michel <luc.mic...@antfield.fr>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/log.h"
> +#include "qemu/timer.h"
> +#include "qapi/error.h"
> +#include "hw/sysbus.h"
> +#include "hw/sd/sd.h"
> +#include "hw/gpio/bcm2835_gpio.h"
> +
> +#define GPFSEL0 0x00
> +#define GPFSEL1 0x04
> +#define GPFSEL2 0x08
> +#define GPFSEL3 0x0C
> +#define GPFSEL4 0x10
> +#define GPFSEL5 0x14
> +#define GPSET0 0x1C
> +#define GPSET1 0x20
> +#define GPCLR0 0x28
> +#define GPCLR1 0x2C
> +#define GPLEV0 0x34
> +#define GPLEV1 0x38
> +#define GPEDS0 0x40
> +#define GPEDS1 0x44
> +#define GPREN0 0x4C
> +#define GPREN1 0x50
> +#define GPFEN0 0x58
> +#define GPFEN1 0x5C
> +#define GPHEN0 0x64
> +#define GPHEN1 0x68
> +#define GPLEN0 0x70
> +#define GPLEN1 0x74
> +#define GPAREN0 0x7C
> +#define GPAREN1 0x80
> +#define GPAFEN0 0x88
> +#define GPAFEN1 0x8C
> +#define GPPUD 0x94
> +#define GPPUDCLK0 0x98
> +#define GPPUDCLK1 0x9C
> +
> +static uint32_t gpfsel_get(BCM2835GpioState *s, uint8_t reg)
> +{
> + int i;
> + uint32_t value = 0;
> + for (i = 0; i < 10; i++) {
> + uint32_t index = 10 * reg + i;
> + if (index < sizeof(s->fsel)) {
> + value |= (s->fsel[index] & 0x7) << (3 * i);
> + }
> + }
> + return value;
> +}
> +
> +static void sdbus_reparent_card(SDBus *from, SDBus *to)
> +{
> + BusChild *kid = QTAILQ_FIRST(&from->qbus.children);
> + if(kid == NULL) {
> + return;
> + }
> + SDState *card = SD_CARD(kid->child);
This inlining of get_card() suggests to me that we want this function
to be defined in hw/sd/core.c, rather than here.
> + sdbus_set_inserted(from, false);
> + object_unparent(OBJECT(kid));
> + qdev_set_parent_bus(DEVICE(card), &to->qbus);
> + sdbus_set_inserted(to, true);
I think we probably also want to call
sdbus_set_readonly(to, readonly)
to tell the destination controller the r/o state.
(compare sd_cardchange(), though here we'll need to use
sc->get_readonly() like sdbus_get_readonly() does).
Do we also need to call the SD card object's reset method?
A comment to the effect of:
/* We directly reparent the card object rather than implementing this
* as a hotpluggable connection because we don't want to expose SD cards
* to users as being hotpluggable, and we can get away with it in this
* limited use case. This could perhaps be implemented more cleanly in
* future by adding support to the hotplug infrastructure for "device
* can be hotplugged only via code, not by user".
*/
will also be helpful for alerting future readers to the fact
we're doing something pragmatic-but-not-ideal here.
> +}
> +
> +static void gpfsel_set(BCM2835GpioState *s, uint8_t reg, uint32_t value)
> +{
> + int i;
> + for (i = 0; i < 10; i++) {
> + uint32_t index = 10 * reg + i;
> + if (index < sizeof(s->fsel)) {
> + int fsel = (value >> (3 * i)) & 0x7;
> + s->fsel[index] = fsel;
> + }
> + }
> +
> + /* SD controller selection (48-53) */
> + if (s->sd_fsel != 0
> + && (s->fsel[48] == 0) /* SD_CLK_R */
> + && (s->fsel[49] == 0) /* SD_CMD_R */
> + && (s->fsel[50] == 0) /* SD_DATA0_R */
> + && (s->fsel[51] == 0) /* SD_DATA1_R */
> + && (s->fsel[52] == 0) /* SD_DATA2_R */
> + && (s->fsel[53] == 0) /* SD_DATA3_R */
> + ) {
> + /* SDHCI controller selected */
> + sdbus_reparent_card(&s->sdhost->sdbus, &s->sdhci->sdbus);
> + s->sd_fsel = 0;
> + } else if (s->sd_fsel != 4
> + && (s->fsel[48] == 4) /* SD_CLK_R */
> + && (s->fsel[49] == 4) /* SD_CMD_R */
> + && (s->fsel[50] == 4) /* SD_DATA0_R */
> + && (s->fsel[51] == 4) /* SD_DATA1_R */
> + && (s->fsel[52] == 4) /* SD_DATA2_R */
> + && (s->fsel[53] == 4) /* SD_DATA3_R */
> + ) {
> + /* SDHost controller selected */
> + sdbus_reparent_card(&s->sdhci->sdbus, &s->sdhost->sdbus);
It would I think be slightly neater to model this as the
GPIO object exposing an sdbus itself and only taking the
sdbus links from the two controller objects, rather than
taking links to the controllers themselves and grabbing the
sdbus fields out of them. But this isn't user-visible, so
I don't care too much. (We can always come back and tidy it
up later.)
Apart from my comments above about the card-reparenting function
this looks OK to me (and I'd like to squeeze it into 2.9 if we
can; deadline for "patches reviewed and in pull requests is very
near though, 28th).
thanks
-- PMM
