From mboxrd@z Thu Jan 1 00:00:00 1970 From: Simon Glass Date: Thu, 21 Nov 2019 06:50:17 -0700 Subject: [U-Boot] [PATCH v3 081/108] spi: ich: Add mmio_base to struct ich_spi_platdata In-Reply-To: References: <20191021033913.220758-22-sjg@chromium.org> <20191021033913.220758-76-sjg@chromium.org> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Bin, On Wed, 20 Nov 2019 at 05:52, Bin Meng wrote: > > Hi Simon, > > On Wed, Nov 20, 2019 at 1:53 AM Simon Glass wrote: > > > > Hi Bin, > > > > On Tue, 19 Nov 2019 at 06:36, Bin Meng wrote: > > > > > > Hi Simon, > > > > > > On Mon, Oct 21, 2019 at 11:40 AM Simon Glass wrote: > > > > > > > > It is useful to store the mmio base in platdata. It reduces the amount of > > > > casting needed. Update the code and move the struct to the C file at the > > > > same time, as we will need to use with of-platdata. > > > > > > > > Signed-off-by: Simon Glass > > > > --- > > > > > > > > Changes in v3: None > > > > Changes in v2: None > > > > > > > > drivers/spi/ich.c | 27 +++++++++++++-------------- > > > > drivers/spi/ich.h | 5 ----- > > > > 2 files changed, 13 insertions(+), 19 deletions(-) > > > > > > > > diff --git a/drivers/spi/ich.c b/drivers/spi/ich.c > > > > index e2bc77bbd58..7f73f096ecb 100644 > > > > --- a/drivers/spi/ich.c > > > > +++ b/drivers/spi/ich.c > > > > @@ -27,6 +27,12 @@ > > > > #define debug_trace(x, args...) > > > > #endif > > > > > > > > +struct ich_spi_platdata { > > > > + enum ich_version ich_version; /* Controller version, 7 or 9 */ > > > > + bool lockdown; /* lock down controller settings? */ > > > > + ulong mmio_base; /* Base of MMIO registers */ > > > > +}; > > > > + > > > > static u8 ich_readb(struct ich_spi_priv *priv, int reg) > > > > { > > > > u8 value = readb(priv->base + reg); > > > > @@ -466,16 +472,9 @@ static int ich_init_controller(struct udevice *dev, > > > > struct ich_spi_platdata *plat, > > > > struct ich_spi_priv *ctlr) > > > > { > > > > - ulong sbase_addr; > > > > - void *sbase; > > > > - > > > > - /* SBASE is similar */ > > > > - pch_get_spi_base(dev->parent, &sbase_addr); > > > > - sbase = (void *)sbase_addr; > > > > - debug("%s: sbase=%p\n", __func__, sbase); > > > > - > > > > + ctlr->base = (void *)plat->mmio_base; > > > > if (plat->ich_version == ICHV_7) { > > > > - struct ich7_spi_regs *ich7_spi = sbase; > > > > + struct ich7_spi_regs *ich7_spi = ctlr->base; > > > > > > > > ctlr->opmenu = offsetof(struct ich7_spi_regs, opmenu); > > > > ctlr->menubytes = sizeof(ich7_spi->opmenu); > > > > @@ -487,9 +486,8 @@ static int ich_init_controller(struct udevice *dev, > > > > ctlr->control = offsetof(struct ich7_spi_regs, spic); > > > > ctlr->bbar = offsetof(struct ich7_spi_regs, bbar); > > > > ctlr->preop = offsetof(struct ich7_spi_regs, preop); > > > > - ctlr->base = ich7_spi; > > > > } else if (plat->ich_version == ICHV_9) { > > > > - struct ich9_spi_regs *ich9_spi = sbase; > > > > + struct ich9_spi_regs *ich9_spi = ctlr->base; > > > > > > > > ctlr->opmenu = offsetof(struct ich9_spi_regs, opmenu); > > > > ctlr->menubytes = sizeof(ich9_spi->opmenu); > > > > @@ -504,7 +502,6 @@ static int ich_init_controller(struct udevice *dev, > > > > ctlr->preop = offsetof(struct ich9_spi_regs, preop); > > > > ctlr->bcr = offsetof(struct ich9_spi_regs, bcr); > > > > ctlr->pr = &ich9_spi->pr[0]; > > > > - ctlr->base = ich9_spi; > > > > } else { > > > > debug("ICH SPI: Unrecognised ICH version %d\n", > > > > plat->ich_version); > > > > @@ -515,8 +512,8 @@ static int ich_init_controller(struct udevice *dev, > > > > ctlr->max_speed = 20000000; > > > > if (plat->ich_version == ICHV_9 && ich9_can_do_33mhz(dev)) > > > > ctlr->max_speed = 33000000; > > > > - debug("ICH SPI: Version ID %d detected at %p, speed %ld\n", > > > > - plat->ich_version, ctlr->base, ctlr->max_speed); > > > > + debug("ICH SPI: Version ID %d detected at %lx, speed %ld\n", > > > > + plat->ich_version, plat->mmio_base, ctlr->max_speed); > > > > > > > > ich_set_bbar(ctlr, 0); > > > > > > > > @@ -601,6 +598,8 @@ static int ich_spi_ofdata_to_platdata(struct udevice *dev) > > > > plat->ich_version = dev_get_driver_data(dev); > > > > plat->lockdown = dev_read_bool(dev, "intel,spi-lock-down"); > > > > > > > > + pch_get_spi_base(dev->parent, &plat->mmio_base); > > > > > > In patch [77], spi: ich: Move the protection/lockdown code into a > > > function, we already removed the assumption of dev->parent, but used > > > priv->pch for the PCH device. > > > > Yes, this should use priv->pch, I think. > > > > > > > > > + > > > > return 0; > > > > } > > > > > > > > diff --git a/drivers/spi/ich.h b/drivers/spi/ich.h > > > > index 77057878a5d..623b2c547a6 100644 > > > > --- a/drivers/spi/ich.h > > > > +++ b/drivers/spi/ich.h > > > > @@ -168,11 +168,6 @@ enum ich_version { > > > > ICHV_9, > > > > }; > > > > > > > > -struct ich_spi_platdata { > > > > - enum ich_version ich_version; /* Controller version, 7 or 9 */ > > > > - bool lockdown; /* lock down controller settings? */ > > > > -}; > > > > > > I don't understand why moving this struct to C file? > > > > We need to add of-platdata to this struct and don't want to do that in > > a header file. It ends up begin: > > > > struct ich_spi_platdata { > > #if CONFIG_IS_ENABLED(OF_PLATDATA) > > struct dtd... ...; > > #endif > > enum ich_version ich_version; /* Controller version, 7 or 9 */ > > ... > > } > > > > All structs that use of-platdata should be in C files, I think. > > > > Is this hard requirement for drivers with of-platdata? Maybe we need > update of-plat.rst to better document this rule. Will add this in a new patch. Yes I think using of-platdata.h in a header file would be bad. Regards, Simon