| Message ID | 20201021025507.51001-3-vadivel.muruganx.ramuthevar@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | spi: cadence-quadspi: Add QSPI controller support for Intel LGM SoC | expand |
Hi, On 21/10/20 10:55AM, Ramuthevar,Vadivel MuruganX wrote: > From: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com> > > On Intel Lightning Mountain(LGM) SoCs QSPI controller do not use > Direct Access Controller(DAC). > > This patch adds a quirk to disable the Direct Access Controller > for data transfer instead it uses indirect data transfer. > > Signed-off-by: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com> > --- > drivers/spi/spi-cadence-quadspi.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c > index d7b10c46fa70..3d017b484114 100644 > --- a/drivers/spi/spi-cadence-quadspi.c > +++ b/drivers/spi/spi-cadence-quadspi.c > @@ -1106,6 +1106,13 @@ static void cqspi_controller_init(struct cqspi_st *cqspi) > reg |= CQSPI_REG_CONFIG_ENB_DIR_ACC_CTRL; > writel(reg, cqspi->iobase + CQSPI_REG_CONFIG); > > + /* Disable direct access controller */ > + if (!cqspi->use_direct_mode) { > + reg = readl(cqspi->iobase + CQSPI_REG_CONFIG); > + reg &= ~CQSPI_REG_CONFIG_ENB_DIR_ACC_CTRL; > + writel(reg, cqspi->iobase + CQSPI_REG_CONFIG); > + } > + Do you really need to disable the DAC controller? cqspi_read() and cqspi_write() already check for cqspi->use_direct_mode and avoid using direct mode if it is false. While I don't think it would do any harm I'm curious what prompted you to do this instead of just setting the quirk like cdns_qspi does. Anyway, if you do insist on doing it, it does not make any sense to set a bit and then unset it immediately after. The datasheet I have says this bit resets to 1 so the block above the code you added should be removed. > cqspi_controller_enable(cqspi, 1); > } > > @@ -1388,6 +1395,10 @@ static const struct cqspi_driver_platdata am654_ospi = { > .quirks = CQSPI_NEEDS_WR_DELAY, > }; > > +static const struct cqspi_driver_platdata intel_lgm_qspi = { > + .quirks = CQSPI_DISABLE_DAC_MODE, > +}; > + > static const struct of_device_id cqspi_dt_ids[] = { > { > .compatible = "cdns,qspi-nor", > @@ -1403,6 +1414,7 @@ static const struct of_device_id cqspi_dt_ids[] = { > }, > { > .compatible = "intel,lgm-qspi", > + .data = &intel_lgm_qspi, > }, > { /* end of table */ } > };
Hi Pratyush, On 21/10/2020 11:17 pm, Pratyush Yadav wrote: > Hi, > > On 21/10/20 10:55AM, Ramuthevar,Vadivel MuruganX wrote: >> From: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com> >> >> On Intel Lightning Mountain(LGM) SoCs QSPI controller do not use >> Direct Access Controller(DAC). >> >> This patch adds a quirk to disable the Direct Access Controller >> for data transfer instead it uses indirect data transfer. >> >> Signed-off-by: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com> >> --- >> drivers/spi/spi-cadence-quadspi.c | 12 ++++++++++++ >> 1 file changed, 12 insertions(+) >> >> diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c >> index d7b10c46fa70..3d017b484114 100644 >> --- a/drivers/spi/spi-cadence-quadspi.c >> +++ b/drivers/spi/spi-cadence-quadspi.c >> @@ -1106,6 +1106,13 @@ static void cqspi_controller_init(struct cqspi_st *cqspi) >> reg |= CQSPI_REG_CONFIG_ENB_DIR_ACC_CTRL; >> writel(reg, cqspi->iobase + CQSPI_REG_CONFIG); >> >> + /* Disable direct access controller */ >> + if (!cqspi->use_direct_mode) { >> + reg = readl(cqspi->iobase + CQSPI_REG_CONFIG); >> + reg &= ~CQSPI_REG_CONFIG_ENB_DIR_ACC_CTRL; >> + writel(reg, cqspi->iobase + CQSPI_REG_CONFIG); >> + } >> + > > Do you really need to disable the DAC controller? cqspi_read() and > cqspi_write() already check for cqspi->use_direct_mode and avoid using > direct mode if it is false. While I don't think it would do any harm I'm > curious what prompted you to do this instead of just setting the quirk > like cdns_qspi does. > > Anyway, if you do insist on doing it, it does not make any sense to set > a bit and then unset it immediately after. The datasheet I have says > this bit resets to 1 so the block above the code you added should be > removed. Thank you for your review comments.. yes, we need this patch to disable DAC for our SoC to avoid any conflicts in future as well since Intel LGM SoC doesn't support DAC at all. Regards Vadivel > >> cqspi_controller_enable(cqspi, 1); >> } >> >> @@ -1388,6 +1395,10 @@ static const struct cqspi_driver_platdata am654_ospi = { >> .quirks = CQSPI_NEEDS_WR_DELAY, >> }; >> >> +static const struct cqspi_driver_platdata intel_lgm_qspi = { >> + .quirks = CQSPI_DISABLE_DAC_MODE, >> +}; >> + >> static const struct of_device_id cqspi_dt_ids[] = { >> { >> .compatible = "cdns,qspi-nor", >> @@ -1403,6 +1414,7 @@ static const struct of_device_id cqspi_dt_ids[] = { >> }, >> { >> .compatible = "intel,lgm-qspi", >> + .data = &intel_lgm_qspi, >> }, >> { /* end of table */ } >> }; >
On 22/10/20 10:17AM, Ramuthevar, Vadivel MuruganX wrote: > Hi Pratyush, > > On 21/10/2020 11:17 pm, Pratyush Yadav wrote: > > Hi, > > > > On 21/10/20 10:55AM, Ramuthevar,Vadivel MuruganX wrote: > > > From: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com> > > > > > > On Intel Lightning Mountain(LGM) SoCs QSPI controller do not use > > > Direct Access Controller(DAC). > > > > > > This patch adds a quirk to disable the Direct Access Controller > > > for data transfer instead it uses indirect data transfer. > > > > > > Signed-off-by: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com> > > > --- > > > drivers/spi/spi-cadence-quadspi.c | 12 ++++++++++++ > > > 1 file changed, 12 insertions(+) > > > > > > diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c > > > index d7b10c46fa70..3d017b484114 100644 > > > --- a/drivers/spi/spi-cadence-quadspi.c > > > +++ b/drivers/spi/spi-cadence-quadspi.c > > > @@ -1106,6 +1106,13 @@ static void cqspi_controller_init(struct cqspi_st *cqspi) > > > reg |= CQSPI_REG_CONFIG_ENB_DIR_ACC_CTRL; > > > writel(reg, cqspi->iobase + CQSPI_REG_CONFIG); > > > + /* Disable direct access controller */ > > > + if (!cqspi->use_direct_mode) { > > > + reg = readl(cqspi->iobase + CQSPI_REG_CONFIG); > > > + reg &= ~CQSPI_REG_CONFIG_ENB_DIR_ACC_CTRL; > > > + writel(reg, cqspi->iobase + CQSPI_REG_CONFIG); > > > + } > > > + > > > > Do you really need to disable the DAC controller? cqspi_read() and > > cqspi_write() already check for cqspi->use_direct_mode and avoid using > > direct mode if it is false. While I don't think it would do any harm I'm > > curious what prompted you to do this instead of just setting the quirk > > like cdns_qspi does. > > > > Anyway, if you do insist on doing it, it does not make any sense to set > > a bit and then unset it immediately after. The datasheet I have says > > this bit resets to 1 so the block above the code you added should be > > removed. > Thank you for your review comments.. > yes, we need this patch to disable DAC for our SoC to avoid any conflicts in > future as well since Intel LGM SoC doesn't support DAC at all. I'm not sure you got my point here. I understand that LGM SoCs don't support DAC. I'm not arguing if this _patch_ is needed. I'm arguing if this _hunk_ is needed. Does DAC mode need to be explicitly disabled here? Why will the check in cqspi_read() and cqspi_write() not be enough? My other point is that if you absolutely need to disable DAC mode, then instead of the code you have added, it would make more sense to do something like below in cqspi_controller_init(). Because the bit resets to 1 so the block of code to enable it is useless [0]. --- 8< --- diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c index d7ad8b198a11..d2c5d448a944 100644 --- a/drivers/spi/spi-cadence-quadspi.c +++ b/drivers/spi/spi-cadence-quadspi.c @@ -2156,10 +2156,12 @@ static void cqspi_controller_init(struct cqspi_st *cqspi) writel(cqspi->fifo_depth * cqspi->fifo_width / 8, cqspi->iobase + CQSPI_REG_INDIRECTWRWATERMARK); - /* Enable Direct Access Controller */ - reg = readl(cqspi->iobase + CQSPI_REG_CONFIG); - reg |= CQSPI_REG_CONFIG_ENB_DIR_ACC_CTRL; - writel(reg, cqspi->iobase + CQSPI_REG_CONFIG); + /* Disable Direct Access Controller */ + if (!cqspi->use_dac_mode) { + reg = readl(cqspi->iobase + CQSPI_REG_CONFIG); + reg &= ~CQSPI_REG_CONFIG_ENB_DIR_ACC_CTRL; + writel(reg, cqspi->iobase + CQSPI_REG_CONFIG); + } cqspi_controller_enable(cqspi, 1); } --- >8 --- Disclaimer: not tested at all. [0] Git blames Vignesh for that block of code added in a27f2eaf2b27. Vignesh, was this simply an oversight or was there any real reason to set the bit? > Regards > Vadivel > > > > > cqspi_controller_enable(cqspi, 1); > > > } > > > @@ -1388,6 +1395,10 @@ static const struct cqspi_driver_platdata am654_ospi = { > > > .quirks = CQSPI_NEEDS_WR_DELAY, > > > }; > > > +static const struct cqspi_driver_platdata intel_lgm_qspi = { > > > + .quirks = CQSPI_DISABLE_DAC_MODE, > > > +}; > > > + > > > static const struct of_device_id cqspi_dt_ids[] = { > > > { > > > .compatible = "cdns,qspi-nor", > > > @@ -1403,6 +1414,7 @@ static const struct of_device_id cqspi_dt_ids[] = { > > > }, > > > { > > > .compatible = "intel,lgm-qspi", > > > + .data = &intel_lgm_qspi, > > > }, > > > { /* end of table */ } > > > }; > >
Hi, On 22/10/2020 5:01 pm, Pratyush Yadav wrote: > On 22/10/20 10:17AM, Ramuthevar, Vadivel MuruganX wrote: >> Hi Pratyush, >> >> On 21/10/2020 11:17 pm, Pratyush Yadav wrote: >>> Hi, >>> >>> On 21/10/20 10:55AM, Ramuthevar,Vadivel MuruganX wrote: >>>> From: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com> >>>> >>>> On Intel Lightning Mountain(LGM) SoCs QSPI controller do not use >>>> Direct Access Controller(DAC). >>>> >>>> This patch adds a quirk to disable the Direct Access Controller >>>> for data transfer instead it uses indirect data transfer. >>>> >>>> Signed-off-by: Ramuthevar Vadivel Murugan <vadivel.muruganx.ramuthevar@linux.intel.com> >>>> --- >>>> drivers/spi/spi-cadence-quadspi.c | 12 ++++++++++++ >>>> 1 file changed, 12 insertions(+) >>>> >>>> diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c >>>> index d7b10c46fa70..3d017b484114 100644 >>>> --- a/drivers/spi/spi-cadence-quadspi.c >>>> +++ b/drivers/spi/spi-cadence-quadspi.c >>>> @@ -1106,6 +1106,13 @@ static void cqspi_controller_init(struct cqspi_st *cqspi) >>>> reg |= CQSPI_REG_CONFIG_ENB_DIR_ACC_CTRL; >>>> writel(reg, cqspi->iobase + CQSPI_REG_CONFIG); >>>> + /* Disable direct access controller */ >>>> + if (!cqspi->use_direct_mode) { >>>> + reg = readl(cqspi->iobase + CQSPI_REG_CONFIG); >>>> + reg &= ~CQSPI_REG_CONFIG_ENB_DIR_ACC_CTRL; >>>> + writel(reg, cqspi->iobase + CQSPI_REG_CONFIG); >>>> + } >>>> + >>> >>> Do you really need to disable the DAC controller? cqspi_read() and >>> cqspi_write() already check for cqspi->use_direct_mode and avoid using >>> direct mode if it is false. While I don't think it would do any harm I'm >>> curious what prompted you to do this instead of just setting the quirk >>> like cdns_qspi does. >>> >>> Anyway, if you do insist on doing it, it does not make any sense to set >>> a bit and then unset it immediately after. The datasheet I have says >>> this bit resets to 1 so the block above the code you added should be >>> removed. >> Thank you for your review comments.. >> yes, we need this patch to disable DAC for our SoC to avoid any conflicts in >> future as well since Intel LGM SoC doesn't support DAC at all. > > I'm not sure you got my point here. Got your point, thanks! I understand that LGM SoCs don't > support DAC. I'm not arguing if this _patch_ is needed. I'm arguing if > this _hunk_ is needed. Needed, my previous patches added DAC disabled in cqspi_read() and cqspi_write() function then Vignesh suggested me to move cqspi_controller_init() function part so I have add it now. you are saying that add hunk at the end of cqspi_controller_init(). that's also okay for me, anyhow DAC should be disabled at any case. Regards Vadivel Does DAC mode need to be explicitly disabled > here? Why will the check in cqspi_read() and cqspi_write() not be > enough? > > My other point is that if you absolutely need to disable DAC mode, then > instead of the code you have added, it would make more sense to do > something like below in cqspi_controller_init(). Because the bit resets > to 1 so the block of code to enable it is useless [0]. > > --- 8< --- > diff --git a/drivers/spi/spi-cadence-quadspi.c > b/drivers/spi/spi-cadence-quadspi.c > index d7ad8b198a11..d2c5d448a944 100644 > --- a/drivers/spi/spi-cadence-quadspi.c > +++ b/drivers/spi/spi-cadence-quadspi.c > @@ -2156,10 +2156,12 @@ static void cqspi_controller_init(struct cqspi_st *cqspi) > writel(cqspi->fifo_depth * cqspi->fifo_width / 8, > cqspi->iobase + CQSPI_REG_INDIRECTWRWATERMARK); > > - /* Enable Direct Access Controller */ > - reg = readl(cqspi->iobase + CQSPI_REG_CONFIG); > - reg |= CQSPI_REG_CONFIG_ENB_DIR_ACC_CTRL; > - writel(reg, cqspi->iobase + CQSPI_REG_CONFIG); > + /* Disable Direct Access Controller */ > + if (!cqspi->use_dac_mode) { > + reg = readl(cqspi->iobase + CQSPI_REG_CONFIG); > + reg &= ~CQSPI_REG_CONFIG_ENB_DIR_ACC_CTRL; > + writel(reg, cqspi->iobase + CQSPI_REG_CONFIG); > + } > > cqspi_controller_enable(cqspi, 1); > } > --- >8 --- > > Disclaimer: not tested at all. > > [0] Git blames Vignesh for that block of code added in a27f2eaf2b27. > Vignesh, was this simply an oversight or was there any real reason to > set the bit? > >> Regards >> Vadivel >>> >>>> cqspi_controller_enable(cqspi, 1); >>>> } >>>> @@ -1388,6 +1395,10 @@ static const struct cqspi_driver_platdata am654_ospi = { >>>> .quirks = CQSPI_NEEDS_WR_DELAY, >>>> }; >>>> +static const struct cqspi_driver_platdata intel_lgm_qspi = { >>>> + .quirks = CQSPI_DISABLE_DAC_MODE, >>>> +}; >>>> + >>>> static const struct of_device_id cqspi_dt_ids[] = { >>>> { >>>> .compatible = "cdns,qspi-nor", >>>> @@ -1403,6 +1414,7 @@ static const struct of_device_id cqspi_dt_ids[] = { >>>> }, >>>> { >>>> .compatible = "intel,lgm-qspi", >>>> + .data = &intel_lgm_qspi, >>>> }, >>>> { /* end of table */ } >>>> }; >>> >
diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-cadence-quadspi.c index d7b10c46fa70..3d017b484114 100644 --- a/drivers/spi/spi-cadence-quadspi.c +++ b/drivers/spi/spi-cadence-quadspi.c @@ -1106,6 +1106,13 @@ static void cqspi_controller_init(struct cqspi_st *cqspi) reg |= CQSPI_REG_CONFIG_ENB_DIR_ACC_CTRL; writel(reg, cqspi->iobase + CQSPI_REG_CONFIG); + /* Disable direct access controller */ + if (!cqspi->use_direct_mode) { + reg = readl(cqspi->iobase + CQSPI_REG_CONFIG); + reg &= ~CQSPI_REG_CONFIG_ENB_DIR_ACC_CTRL; + writel(reg, cqspi->iobase + CQSPI_REG_CONFIG); + } + cqspi_controller_enable(cqspi, 1); } @@ -1388,6 +1395,10 @@ static const struct cqspi_driver_platdata am654_ospi = { .quirks = CQSPI_NEEDS_WR_DELAY, }; +static const struct cqspi_driver_platdata intel_lgm_qspi = { + .quirks = CQSPI_DISABLE_DAC_MODE, +}; + static const struct of_device_id cqspi_dt_ids[] = { { .compatible = "cdns,qspi-nor", @@ -1403,6 +1414,7 @@ static const struct of_device_id cqspi_dt_ids[] = { }, { .compatible = "intel,lgm-qspi", + .data = &intel_lgm_qspi, }, { /* end of table */ } };