[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH V4 2/2] sdhci: Split sdhci.h for public and inte
From: |
Sai Pavan Boddu |
Subject: |
Re: [Qemu-devel] [PATCH V4 2/2] sdhci: Split sdhci.h for public and internal device usage |
Date: |
Mon, 21 Sep 2015 16:35:21 +0000 |
> -----Original Message-----
> From: Peter Crosthwaite [mailto:address@hidden
> Sent: Monday, September 21, 2015 8:01 PM
> To: Sai Pavan Boddu
> Cc: address@hidden Developers; Stefan Hajnoczi; Peter Maydell;
> Alistair Francis; Edgar Iglesias; Sai Pavan Boddu
> Subject: Re: [PATCH V4 2/2] sdhci: Split sdhci.h for public and internal
> device
> usage
>
> On Mon, Sep 14, 2015 at 2:26 AM, Sai Pavan Boddu
> <address@hidden> wrote:
> > Split sdhci.h into sdhci-common.h(pubilc Version in include/) and
> > sdhci.h(internal version in hw/sd) base on register declarations and
> > object declaration.
> >
> > Signed-off-by: Sai Pavan Boddu <address@hidden>
> > ---
> > Changes for V4:
> > Remain the name of internal version of sdchi.h as same. And change
> > Re-Adding qemu-common.h header.
> > the one which is in includes/ to sdhci-common.h
> > Changes for V2:
> > Create new area in includes for sd. And move sdhci.h to same.
> > Changes for V3:
> > Split the headers to public and common.
> > ---
> > hw/sd/sdhci.c | 1 -
> > hw/sd/sdhci.h | 67 +-------------------------------
> > include/hw/sd/sdhci-common.h | 92
> ++++++++++++++++++++++++++++++++++++++++++++
>
> I think the split is good (and needed for SDHCI SoCification) but the
> names are inconsistent. The public header should trump the private
> when it comes to who gets the name "sdhci.h". So we should have
> include/hw/sd/sdhci.h with the SoC embeddable and
> hw/sd/sdhci-internal.h for the implementation private content. Check
> target-arm/cpu-internal.h vs target-arm/cpu.h for a similar concept.
[Sai Pavan ] This make sense, I will come up the same changes in V5
Thanks,
Sai Pavan
>
> Regards,
> Peter
>
> > 3 files changed, 94 insertions(+), 66 deletions(-)
> > create mode 100644 include/hw/sd/sdhci-common.h
> >
> > diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> > index e63367b..4860f41 100644
> > --- a/hw/sd/sdhci.c
> > +++ b/hw/sd/sdhci.c
> > @@ -28,7 +28,6 @@
> > #include "sysemu/dma.h"
> > #include "qemu/timer.h"
> > #include "qemu/bitops.h"
> > -
> > #include "sdhci.h"
> >
> > /* host controller debug messages */
> > diff --git a/hw/sd/sdhci.h b/hw/sd/sdhci.h
> > index a45593f..667c60c 100644
> > --- a/hw/sd/sdhci.h
> > +++ b/hw/sd/sdhci.h
> > @@ -21,14 +21,10 @@
> > * You should have received a copy of the GNU General Public License
> along
> > * with this program; if not, see <http://www.gnu.org/licenses/>.
> > */
> > -
> > #ifndef SDHCI_H
> > #define SDHCI_H
> >
> > -#include "qemu-common.h"
> > -#include "hw/pci/pci.h"
> > -#include "hw/sysbus.h"
> > -#include "hw/sd/sd.h"
> > +#include "hw/sd/sdhci-common.h"
> >
> > /* R/W SDMA System Address register 0x0 */
> > #define SDHC_SYSAD 0x00
> > @@ -231,65 +227,6 @@ enum {
> > sdhc_gap_write = 2 /* SDHC stopped at block gap during write
> operation */
> > };
> >
> > -/* SD/MMC host controller state */
> > -typedef struct SDHCIState {
> > - union {
> > - PCIDevice pcidev;
> > - SysBusDevice busdev;
> > - };
> > - SDState *card;
> > - MemoryRegion iomem;
> > -
> > - QEMUTimer *insert_timer; /* timer for 'changing' sd card. */
> > - QEMUTimer *transfer_timer;
> > - qemu_irq eject_cb;
> > - qemu_irq ro_cb;
> > - qemu_irq irq;
> > -
> > - uint32_t sdmasysad; /* SDMA System Address register */
> > - uint16_t blksize; /* Host DMA Buff Boundary and Transfer BlkSize
> > Reg
> */
> > - uint16_t blkcnt; /* Blocks count for current transfer */
> > - uint32_t argument; /* Command Argument Register */
> > - uint16_t trnmod; /* Transfer Mode Setting Register */
> > - uint16_t cmdreg; /* Command Register */
> > - uint32_t rspreg[4]; /* Response Registers 0-3 */
> > - uint32_t prnsts; /* Present State Register */
> > - uint8_t hostctl; /* Host Control Register */
> > - uint8_t pwrcon; /* Power control Register */
> > - uint8_t blkgap; /* Block Gap Control Register */
> > - uint8_t wakcon; /* WakeUp Control Register */
> > - uint16_t clkcon; /* Clock control Register */
> > - uint8_t timeoutcon; /* Timeout Control Register */
> > - uint8_t admaerr; /* ADMA Error Status Register */
> > - uint16_t norintsts; /* Normal Interrupt Status Register */
> > - uint16_t errintsts; /* Error Interrupt Status Register */
> > - uint16_t norintstsen; /* Normal Interrupt Status Enable Register */
> > - uint16_t errintstsen; /* Error Interrupt Status Enable Register */
> > - uint16_t norintsigen; /* Normal Interrupt Signal Enable Register */
> > - uint16_t errintsigen; /* Error Interrupt Signal Enable Register */
> > - uint16_t acmd12errsts; /* Auto CMD12 error status register */
> > - uint64_t admasysaddr; /* ADMA System Address Register */
> > -
> > - uint32_t capareg; /* Capabilities Register */
> > - uint32_t maxcurr; /* Maximum Current Capabilities Register */
> > - uint8_t *fifo_buffer; /* SD host i/o FIFO buffer */
> > - uint32_t buf_maxsz;
> > - uint16_t data_count; /* current element in FIFO buffer */
> > - uint8_t stopped_state;/* Current SDHC state */
> > - /* Buffer Data Port Register - virtual access point to R and W buffers
> > */
> > - /* Software Reset Register - always reads as 0 */
> > - /* Force Event Auto CMD12 Error Interrupt Reg - write only */
> > - /* Force Event Error Interrupt Register- write only */
> > - /* RO Host Controller Version Register always reads as 0x2401 */
> > -} SDHCIState;
> > -
> > extern const VMStateDescription sdhci_vmstate;
> >
> > -#define TYPE_PCI_SDHCI "sdhci-pci"
> > -#define PCI_SDHCI(obj) OBJECT_CHECK(SDHCIState, (obj),
> TYPE_PCI_SDHCI)
> > -
> > -#define TYPE_SYSBUS_SDHCI "generic-sdhci"
> > -#define SYSBUS_SDHCI(obj) \
> > - OBJECT_CHECK(SDHCIState, (obj), TYPE_SYSBUS_SDHCI)
> > -
> > -#endif /* SDHCI_H */
> > +#endif
> > diff --git a/include/hw/sd/sdhci-common.h b/include/hw/sd/sdhci-
> common.h
> > new file mode 100644
> > index 0000000..62c300b
> > --- /dev/null
> > +++ b/include/hw/sd/sdhci-common.h
> > @@ -0,0 +1,92 @@
> > +/*
> > + * SD Association Host Standard Specification v2.0 controller emulation
> > + *
> > + * Copyright (c) 2011 Samsung Electronics Co., Ltd.
> > + * Mitsyanko Igor <address@hidden>
> > + * Peter A.G. Crosthwaite <address@hidden>
> > + *
> > + * Based on MMC controller for Samsung S5PC1xx-based board emulation
> > + * by Alexey Merkulov and Vladimir Monakhov.
> > + *
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms of the GNU General Public License as published by the
> > + * Free Software Foundation; either version 2 of the License, or (at your
> > + * option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
> > + * See the GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU _General Public License
> along
> > + * with this program; if not, see <http://www.gnu.org/licenses/>.
> > + */
> > +
> > +#ifndef SDHCI_COMMON_H
> > +#define SDHCI_COMMON_H
> > +
> > +#include "qemu-common.h"
> > +#include "hw/pci/pci.h"
> > +#include "hw/sysbus.h"
> > +#include "hw/sd/sd.h"
> > +
> > +/* SD/MMC host controller state */
> > +typedef struct SDHCIState {
> > + union {
> > + PCIDevice pcidev;
> > + SysBusDevice busdev;
> > + };
> > + SDState *card;
> > + MemoryRegion iomem;
> > +
> > + QEMUTimer *insert_timer; /* timer for 'changing' sd card. */
> > + QEMUTimer *transfer_timer;
> > + qemu_irq eject_cb;
> > + qemu_irq ro_cb;
> > + qemu_irq irq;
> > +
> > + uint32_t sdmasysad; /* SDMA System Address register */
> > + uint16_t blksize; /* Host DMA Buff Boundary and Transfer BlkSize
> > Reg
> */
> > + uint16_t blkcnt; /* Blocks count for current transfer */
> > + uint32_t argument; /* Command Argument Register */
> > + uint16_t trnmod; /* Transfer Mode Setting Register */
> > + uint16_t cmdreg; /* Command Register */
> > + uint32_t rspreg[4]; /* Response Registers 0-3 */
> > + uint32_t prnsts; /* Present State Register */
> > + uint8_t hostctl; /* Host Control Register */
> > + uint8_t pwrcon; /* Power control Register */
> > + uint8_t blkgap; /* Block Gap Control Register */
> > + uint8_t wakcon; /* WakeUp Control Register */
> > + uint16_t clkcon; /* Clock control Register */
> > + uint8_t timeoutcon; /* Timeout Control Register */
> > + uint8_t admaerr; /* ADMA Error Status Register */
> > + uint16_t norintsts; /* Normal Interrupt Status Register */
> > + uint16_t errintsts; /* Error Interrupt Status Register */
> > + uint16_t norintstsen; /* Normal Interrupt Status Enable Register */
> > + uint16_t errintstsen; /* Error Interrupt Status Enable Register */
> > + uint16_t norintsigen; /* Normal Interrupt Signal Enable Register */
> > + uint16_t errintsigen; /* Error Interrupt Signal Enable Register */
> > + uint16_t acmd12errsts; /* Auto CMD12 error status register */
> > + uint64_t admasysaddr; /* ADMA System Address Register */
> > +
> > + uint32_t capareg; /* Capabilities Register */
> > + uint32_t maxcurr; /* Maximum Current Capabilities Register */
> > + uint8_t *fifo_buffer; /* SD host i/o FIFO buffer */
> > + uint32_t buf_maxsz;
> > + uint16_t data_count; /* current element in FIFO buffer */
> > + uint8_t stopped_state;/* Current SDHC state */
> > + /* Buffer Data Port Register - virtual access point to R and W buffers
> > */
> > + /* Software Reset Register - always reads as 0 */
> > + /* Force Event Auto CMD12 Error Interrupt Reg - write only */
> > + /* Force Event Error Interrupt Register- write only */
> > + /* RO Host Controller Version Register always reads as 0x2401 */
> > +} SDHCIState;
> > +
> > +#define TYPE_PCI_SDHCI "sdhci-pci"
> > +#define PCI_SDHCI(obj) OBJECT_CHECK(SDHCIState, (obj),
> TYPE_PCI_SDHCI)
> > +
> > +#define TYPE_SYSBUS_SDHCI "generic-sdhci"
> > +#define SYSBUS_SDHCI(obj) \
> > + OBJECT_CHECK(SDHCIState, (obj), TYPE_SYSBUS_SDHCI)
> > +
> > +#endif /* SDHCI_H */
> > --
> > 1.9.1
> >