Re: [PATCH v2 00/10] Don't let i2c adapters declare I2C_CLASS_SPD support if they support I2C_CLASS_HWMON
On 24.11.2023 11:16, Heiner Kallweit wrote: > After removal of the legacy eeprom driver the only remaining I2C > client device driver supporting I2C_CLASS_SPD is jc42. Because this > driver also supports I2C_CLASS_HWMON, adapters don't have to > declare support for I2C_CLASS_SPD if they support I2C_CLASS_HWMON. > It's one step towards getting rid of I2C_CLASS_SPD mid-term. > > Series was created supported by Coccinelle and its splitpatch. > > v2: > - fix style issue in patch 4 > - add ack in patch 2 > - set proper subject prefix for all patches > > Signed-off-by: Heiner Kallweit > > --- > > drivers/i2c/busses/i2c-ali1535.c |2 +- > drivers/i2c/busses/i2c-ali1563.c |2 +- > drivers/i2c/busses/i2c-ali15x3.c |2 +- > drivers/i2c/busses/i2c-amd756.c |2 +- > drivers/i2c/busses/i2c-amd8111.c |2 +- > drivers/i2c/busses/i2c-elektor.c |2 +- > drivers/i2c/busses/i2c-gpio.c |2 +- > drivers/i2c/busses/i2c-ibm_iic.c |2 +- > drivers/i2c/busses/i2c-iop3xx.c |2 +- > drivers/i2c/busses/i2c-isch.c |2 +- > drivers/i2c/busses/i2c-kempld.c |3 +-- > drivers/i2c/busses/i2c-mlxcpld.c |2 +- > drivers/i2c/busses/i2c-nforce2.c |2 +- > drivers/i2c/busses/i2c-pasemi-pci.c |2 +- > drivers/i2c/busses/i2c-piix4.c|2 +- > drivers/i2c/busses/i2c-scmi.c |2 +- > drivers/i2c/busses/i2c-sh7760.c |2 +- > drivers/i2c/busses/i2c-sibyte.c |4 ++-- > drivers/i2c/busses/i2c-sis5595.c |2 +- > drivers/i2c/busses/i2c-sis630.c |2 +- > drivers/i2c/busses/i2c-sis96x.c |2 +- > drivers/i2c/busses/i2c-via.c |2 +- > drivers/i2c/busses/i2c-viapro.c |2 +- > drivers/i2c/busses/scx200_acb.c |2 +- > drivers/i2c/i2c-stub.c|2 +- > drivers/media/pci/netup_unidvb/netup_unidvb_i2c.c |2 +- > drivers/staging/greybus/i2c.c |2 +- > 27 files changed, 28 insertions(+), 29 deletions(-) This series and my other series are sitting idle in patchwork for 3 weeks now. AFAICS they have the needed ack's. Anything missing before they can be applied?
[PATCH v2 03/10] i2c: pasemi: Don't let i2c adapters declare I2C_CLASS_SPD support if they support I2C_CLASS_HWMON
After removal of the legacy eeprom driver the only remaining I2C client device driver supporting I2C_CLASS_SPD is jc42. Because this driver also supports I2C_CLASS_HWMON, adapters don't have to declare support for I2C_CLASS_SPD if they support I2C_CLASS_HWMON. It's one step towards getting rid of I2C_CLASS_SPD mid-term. Series was created supported by Coccinelle and its splitpatch. Signed-off-by: Heiner Kallweit --- drivers/i2c/busses/i2c-pasemi-pci.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/i2c/busses/i2c-pasemi-pci.c b/drivers/i2c/busses/i2c-pasemi-pci.c index cfc89e04e..77f90c743 100644 --- a/drivers/i2c/busses/i2c-pasemi-pci.c +++ b/drivers/i2c/busses/i2c-pasemi-pci.c @@ -56,7 +56,7 @@ static int pasemi_smb_pci_probe(struct pci_dev *dev, if (!smbus->ioaddr) return -EBUSY; - smbus->adapter.class = I2C_CLASS_HWMON | I2C_CLASS_SPD; + smbus->adapter.class = I2C_CLASS_HWMON; error = pasemi_i2c_common_probe(smbus); if (error) return error;
[PATCH v2 00/10] Don't let i2c adapters declare I2C_CLASS_SPD support if they support I2C_CLASS_HWMON
After removal of the legacy eeprom driver the only remaining I2C client device driver supporting I2C_CLASS_SPD is jc42. Because this driver also supports I2C_CLASS_HWMON, adapters don't have to declare support for I2C_CLASS_SPD if they support I2C_CLASS_HWMON. It's one step towards getting rid of I2C_CLASS_SPD mid-term. Series was created supported by Coccinelle and its splitpatch. v2: - fix style issue in patch 4 - add ack in patch 2 - set proper subject prefix for all patches Signed-off-by: Heiner Kallweit --- drivers/i2c/busses/i2c-ali1535.c |2 +- drivers/i2c/busses/i2c-ali1563.c |2 +- drivers/i2c/busses/i2c-ali15x3.c |2 +- drivers/i2c/busses/i2c-amd756.c |2 +- drivers/i2c/busses/i2c-amd8111.c |2 +- drivers/i2c/busses/i2c-elektor.c |2 +- drivers/i2c/busses/i2c-gpio.c |2 +- drivers/i2c/busses/i2c-ibm_iic.c |2 +- drivers/i2c/busses/i2c-iop3xx.c |2 +- drivers/i2c/busses/i2c-isch.c |2 +- drivers/i2c/busses/i2c-kempld.c |3 +-- drivers/i2c/busses/i2c-mlxcpld.c |2 +- drivers/i2c/busses/i2c-nforce2.c |2 +- drivers/i2c/busses/i2c-pasemi-pci.c |2 +- drivers/i2c/busses/i2c-piix4.c|2 +- drivers/i2c/busses/i2c-scmi.c |2 +- drivers/i2c/busses/i2c-sh7760.c |2 +- drivers/i2c/busses/i2c-sibyte.c |4 ++-- drivers/i2c/busses/i2c-sis5595.c |2 +- drivers/i2c/busses/i2c-sis630.c |2 +- drivers/i2c/busses/i2c-sis96x.c |2 +- drivers/i2c/busses/i2c-via.c |2 +- drivers/i2c/busses/i2c-viapro.c |2 +- drivers/i2c/busses/scx200_acb.c |2 +- drivers/i2c/i2c-stub.c|2 +- drivers/media/pci/netup_unidvb/netup_unidvb_i2c.c |2 +- drivers/staging/greybus/i2c.c |2 +- 27 files changed, 28 insertions(+), 29 deletions(-)
[PATCH 00/10] Don't let i2c adapters declare I2C_CLASS_SPD support if they support I2C_CLASS_HWMON
After removal of the legacy eeprom driver the only remaining I2C client device driver supporting I2C_CLASS_SPD is jc42. Because this driver also supports I2C_CLASS_HWMON, adapters don't have to declare support for I2C_CLASS_SPD if they support I2C_CLASS_HWMON. It's one step towards getting rid of I2C_CLASS_SPD mid-term. Series was created supported by Coccinelle and its splitpatch. Signed-off-by: Heiner Kallweit --- drivers/i2c/busses/i2c-ali1535.c |2 +- drivers/i2c/busses/i2c-ali1563.c |2 +- drivers/i2c/busses/i2c-ali15x3.c |2 +- drivers/i2c/busses/i2c-amd756.c |2 +- drivers/i2c/busses/i2c-amd8111.c |2 +- drivers/i2c/busses/i2c-elektor.c |2 +- drivers/i2c/busses/i2c-gpio.c |2 +- drivers/i2c/busses/i2c-ibm_iic.c |2 +- drivers/i2c/busses/i2c-iop3xx.c |2 +- drivers/i2c/busses/i2c-isch.c |2 +- drivers/i2c/busses/i2c-kempld.c |4 ++-- drivers/i2c/busses/i2c-mlxcpld.c |2 +- drivers/i2c/busses/i2c-nforce2.c |2 +- drivers/i2c/busses/i2c-pasemi-pci.c |2 +- drivers/i2c/busses/i2c-piix4.c|2 +- drivers/i2c/busses/i2c-scmi.c |2 +- drivers/i2c/busses/i2c-sh7760.c |2 +- drivers/i2c/busses/i2c-sibyte.c |4 ++-- drivers/i2c/busses/i2c-sis5595.c |2 +- drivers/i2c/busses/i2c-sis630.c |2 +- drivers/i2c/busses/i2c-sis96x.c |2 +- drivers/i2c/busses/i2c-via.c |2 +- drivers/i2c/busses/i2c-viapro.c |2 +- drivers/i2c/busses/scx200_acb.c |2 +- drivers/i2c/i2c-stub.c|2 +- drivers/media/pci/netup_unidvb/netup_unidvb_i2c.c |2 +- drivers/staging/greybus/i2c.c |2 +- 27 files changed, 29 insertions(+), 29 deletions(-)
[PATCH 03/10] drivers/i2c/busses/i2c-pasemi-pci.c: Don't let i2c adapters declare I2C_CLASS_SPD support if they support I2C_CLASS_HWMON
After removal of the legacy eeprom driver the only remaining I2C client device driver supporting I2C_CLASS_SPD is jc42. Because this driver also supports I2C_CLASS_HWMON, adapters don't have to declare support for I2C_CLASS_SPD if they support I2C_CLASS_HWMON. It's one step towards getting rid of I2C_CLASS_SPD mid-term. Series was created supported by Coccinelle and its splitpatch. Signed-off-by: Heiner Kallweit --- drivers/i2c/busses/i2c-pasemi-pci.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/i2c/busses/i2c-pasemi-pci.c b/drivers/i2c/busses/i2c-pasemi-pci.c index cfc89e04e..77f90c743 100644 --- a/drivers/i2c/busses/i2c-pasemi-pci.c +++ b/drivers/i2c/busses/i2c-pasemi-pci.c @@ -56,7 +56,7 @@ static int pasemi_smb_pci_probe(struct pci_dev *dev, if (!smbus->ioaddr) return -EBUSY; - smbus->adapter.class = I2C_CLASS_HWMON | I2C_CLASS_SPD; + smbus->adapter.class = I2C_CLASS_HWMON; error = pasemi_i2c_common_probe(smbus); if (error) return error;
Re: [net-next][bisected da41788] modules fails to build drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c:12199
On 23.08.2021 08:00, Abdul Haleem wrote:
> Greeting's
>
> Todays net-next branch fails to build on my powerpc box with below error
>
> CC [M] drivers/gpu/drm/virtio/virtgpu_trace_points.o
> CC [M] drivers/net/ethernet/atheros/atl1e/atl1e_ethtool.o
> drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c: In function
> ‘bnx2x_read_fwinfo’:
> drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c:12199:13: error: implicit
> declaration of function ‘pci_vpd_alloc’; did you mean ‘pci_pool_alloc’?
> [-Werror=implicit-function-declaration]
> vpd_data = pci_vpd_alloc(bp->pdev, _len);
> ^
> pci_pool_alloc
> drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c:12199:11: warning:
> assignment to ‘u8 *’ {aka ‘unsigned char *’} from ‘int’ makes pointer from
> integer without a cast [-Wint-conversion]
> vpd_data = pci_vpd_alloc(bp->pdev, _len);
> ^
> CC [M] drivers/scsi/qla2xxx/qla_nx.o
> CC [M] net/netfilter/xt_MASQUERADE.o
> CC [M] net/bluetooth/ecdh_helper.o
> CC [M] drivers/gpu/drm/amd/amdgpu/uvd_v5_0.o
> drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c:12203:9: error: implicit
> declaration of function ‘pci_vpd_find_ro_info_keyword’; did you mean
> ‘pci_vpd_find_info_keyword’? [-Werror=implicit-function-declaration]
> rodi = pci_vpd_find_ro_info_keyword(vpd_data, vpd_len,
> ^~~~
> pci_vpd_find_info_keyword
> CC [M] net/netfilter/xt_SECMARK.o
> CC [M] net/netfilter/xt_TPROXY.o
> CC [M] net/mac80211/driver-ops.o
>
>
> The recent changes to the code causing build fail is
>
> commit da417885a99d36036cc7d2778f94b846e6582434
> Author: Heiner Kallweit
> Date: Sun Aug 22 15:54:23 2021 +0200
>
> bnx2x: Search VPD with pci_vpd_find_ro_info_keyword()
> Use pci_vpd_find_ro_info_keyword() to search for keywords in VPD to
> simplify the code.
> str_id_reg and str_id_cap hold the same string and are used in the
> same
> comparison. This doesn't make sense, use one string str_id instead.
> Signed-off-by: Heiner Kallweit
> Signed-off-by: David S. Miller
>
> @Heiner, Could you please have a look, I am attaching the kernel config
>
This series was supposed to go via the PCI tree because it builds on patches
that are in the PCI tree but not in linux-next and net-next yet.
Re: [PATCH] spi: fsl-espi: Only process interrupts for expected events
On 04.09.2020 02:28, Chris Packham wrote:
> The SPIE register contains counts for the TX FIFO so any time the irq
> handler was invoked we would attempt to process the RX/TX fifos. Use the
> SPIM value to mask the events so that we only process interrupts that
> were expected.
>
> This was a latent issue exposed by commit 3282a3da25bd ("powerpc/64:
> Implement soft interrupt replay in C").
>
> Signed-off-by: Chris Packham
> Cc: sta...@vger.kernel.org
> ---
>
> Notes:
> I've tested this on a T2080RDB and a custom board using the T2081 SoC.
> With
> this change I don't see any spurious instances of the "Transfer done but
> SPIE_DON isn't set!" or "Transfer done but rx/tx fifo's aren't empty!"
> messages
> and the updates to spi flash are successful.
>
> I think this should go into the stable trees that contain 3282a3da25bd
> but I
> haven't added a Fixes: tag because I think 3282a3da25bd exposed the issue
> as
> opposed to causing it.
>
> drivers/spi/spi-fsl-espi.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/spi/spi-fsl-espi.c b/drivers/spi/spi-fsl-espi.c
> index 7e7c92cafdbb..cb120b68c0e2 100644
> --- a/drivers/spi/spi-fsl-espi.c
> +++ b/drivers/spi/spi-fsl-espi.c
> @@ -574,13 +574,14 @@ static void fsl_espi_cpu_irq(struct fsl_espi *espi, u32
> events)
> static irqreturn_t fsl_espi_irq(s32 irq, void *context_data)
> {
> struct fsl_espi *espi = context_data;
> - u32 events;
> + u32 events, mask;
>
> spin_lock(>lock);
>
> /* Get interrupt events(tx/rx) */
> events = fsl_espi_read_reg(espi, ESPI_SPIE);
> - if (!events) {
> + mask = fsl_espi_read_reg(espi, ESPI_SPIM);
> + if (!(events & mask)) {
> spin_unlock(>lock);
> return IRQ_NONE;
Sorry, I was on vacation and therefore couldn't comment earlier.
I'm fine with the change, just one thing could be improved IMO.
If we skip an unneeded interrupt now, then returning IRQ_NONE
causes reporting this interrupt as spurious. This isn't too nice
as spurious interrupts typically are seen as a problem indicator.
Therefore returning IRQ_HANDLED should be more appropriate.
This would just require a comment in the code explaining why we
do this, and why it can happen that we receive interrupts
we're not interested in.
> }
>
Re: fsl_espi errors on v5.7.15
On Fri 4. Sep 2020 at 01:58, Chris Packham <
chris.pack...@alliedtelesis.co.nz> wrote:
>
>
> On 1/09/20 6:14 pm, Nicholas Piggin wrote:
>
> > Excerpts from Chris Packham's message of September 1, 2020 11:25 am:
>
> >> On 1/09/20 12:33 am, Heiner Kallweit wrote:
>
> >>> On 30.08.2020 23:59, Chris Packham wrote:
>
> >>>> On 31/08/20 9:41 am, Heiner Kallweit wrote:
>
> >>>>> On 30.08.2020 23:00, Chris Packham wrote:
>
> >>>>>> On 31/08/20 12:30 am, Nicholas Piggin wrote:
>
> >>>>>>> Excerpts from Chris Packham's message of August 28, 2020 8:07 am:
>
> >>>>>>
>
> >>>>>>
>
> >>>>>>>>>>>> I've also now seen the RX FIFO not empty error on the T2080RDB
>
> >>>>>>>>>>>>
>
> >>>>>>>>>>>> fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set!
>
> >>>>>>>>>>>> fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set!
>
> >>>>>>>>>>>> fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set!
>
> >>>>>>>>>>>> fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set!
>
> >>>>>>>>>>>> fsl_espi ffe11.spi: Transfer done but rx/tx fifo's aren't
> empty!
>
> >>>>>>>>>>>> fsl_espi ffe11.spi: SPIE_RXCNT = 1, SPIE_TXCNT = 32
>
> >>>>>>>>>>>>
>
> >>>>>>>>>>>> With my current workaround of emptying the RX FIFO. It seems
>
> >>>>>>>>>>>> survivable. Interestingly it only ever seems to be 1 extra
> byte in the
>
> >>>>>>>>>>>> RX FIFO and it seems to be after either a READ_SR or a
> READ_FSR.
>
> >>>>>>>>>>>>
>
> >>>>>>>>>>>> fsl_espi ffe11.spi: tx 70
>
> >>>>>>>>>>>> fsl_espi ffe11.spi: rx 03
>
> >>>>>>>>>>>> fsl_espi ffe11.spi: Extra RX 00
>
> >>>>>>>>>>>> fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set!
>
> >>>>>>>>>>>> fsl_espi ffe11.spi: Transfer done but rx/tx fifo's aren't
> empty!
>
> >>>>>>>>>>>> fsl_espi ffe11.spi: SPIE_RXCNT = 1, SPIE_TXCNT = 32
>
> >>>>>>>>>>>> fsl_espi ffe11.spi: tx 05
>
> >>>>>>>>>>>> fsl_espi ffe11.spi: rx 00
>
> >>>>>>>>>>>> fsl_espi ffe11.spi: Extra RX 03
>
> >>>>>>>>>>>> fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set!
>
> >>>>>>>>>>>> fsl_espi ffe11.spi: Transfer done but rx/tx fifo's aren't
> empty!
>
> >>>>>>>>>>>> fsl_espi ffe11.spi: SPIE_RXCNT = 1, SPIE_TXCNT = 32
>
> >>>>>>>>>>>> fsl_espi ffe11.spi: tx 05
>
> >>>>>>>>>>>> fsl_espi ffe11.spi: rx 00
>
> >>>>>>>>>>>> fsl_espi ffe11.spi: Extra RX 03
>
> >>>>>>>>>>>>
>
> >>>>>>>>>>>> From all the Micron SPI-NOR datasheets I've got access
> to it is
>
> >>>>>>>>>>>> possible to continually read the SR/FSR. But I've no idea why
> it
>
> >>>>>>>>>>>> happens some times and not others.
>
> >>>>>>>>>>> So I think I've got a reproduction and I think I've bisected
> the problem
>
> >>>>>>>>>>> to commit 3282a3da25bd ("powerpc/64: Implement soft interrupt
> replay in
>
> >>>>>>>>>>> C"). My day is just finishing now so I haven't applied too
> much scrutiny
>
> >>>>>>>>>>> to this result. Given the various rabbit holes I've been down
> on this
>
> >>>>>>>>>>> issue already I'd take this information with a good degree of
> skepticism.
>
> >>>>>>>>>>>
>
> >>>>>>>>>> OK, so an easy test should be to re-test wit
Re: fsl_espi errors on v5.7.15
On 01.09.2020 04:52, Chris Packham wrote:
>
> On 1/09/20 12:33 am, Heiner Kallweit wrote:
>> On 30.08.2020 23:59, Chris Packham wrote:
>>> On 31/08/20 9:41 am, Heiner Kallweit wrote:
>>>> On 30.08.2020 23:00, Chris Packham wrote:
>>>>> On 31/08/20 12:30 am, Nicholas Piggin wrote:
>>>>>> Excerpts from Chris Packham's message of August 28, 2020 8:07 am:
>>>>>
>>>>>
>>>>>>>>>>> I've also now seen the RX FIFO not empty error on the T2080RDB
>>>>>>>>>>>
>>>>>>>>>>> fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set!
>>>>>>>>>>> fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set!
>>>>>>>>>>> fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set!
>>>>>>>>>>> fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set!
>>>>>>>>>>> fsl_espi ffe11.spi: Transfer done but rx/tx fifo's aren't empty!
>>>>>>>>>>> fsl_espi ffe11.spi: SPIE_RXCNT = 1, SPIE_TXCNT = 32
>>>>>>>>>>>
>>>>>>>>>>> With my current workaround of emptying the RX FIFO. It seems
>>>>>>>>>>> survivable. Interestingly it only ever seems to be 1 extra byte in
>>>>>>>>>>> the
>>>>>>>>>>> RX FIFO and it seems to be after either a READ_SR or a READ_FSR.
>>>>>>>>>>>
>>>>>>>>>>> fsl_espi ffe11.spi: tx 70
>>>>>>>>>>> fsl_espi ffe11.spi: rx 03
>>>>>>>>>>> fsl_espi ffe11.spi: Extra RX 00
>>>>>>>>>>> fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set!
>>>>>>>>>>> fsl_espi ffe11.spi: Transfer done but rx/tx fifo's aren't empty!
>>>>>>>>>>> fsl_espi ffe11.spi: SPIE_RXCNT = 1, SPIE_TXCNT = 32
>>>>>>>>>>> fsl_espi ffe11.spi: tx 05
>>>>>>>>>>> fsl_espi ffe11.spi: rx 00
>>>>>>>>>>> fsl_espi ffe11.spi: Extra RX 03
>>>>>>>>>>> fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set!
>>>>>>>>>>> fsl_espi ffe11.spi: Transfer done but rx/tx fifo's aren't empty!
>>>>>>>>>>> fsl_espi ffe11.spi: SPIE_RXCNT = 1, SPIE_TXCNT = 32
>>>>>>>>>>> fsl_espi ffe11.spi: tx 05
>>>>>>>>>>> fsl_espi ffe11.spi: rx 00
>>>>>>>>>>> fsl_espi ffe11.spi: Extra RX 03
>>>>>>>>>>>
>>>>>>>>>>> From all the Micron SPI-NOR datasheets I've got access to it is
>>>>>>>>>>> possible to continually read the SR/FSR. But I've no idea why it
>>>>>>>>>>> happens some times and not others.
>>>>>>>>>> So I think I've got a reproduction and I think I've bisected the
>>>>>>>>>> problem
>>>>>>>>>> to commit 3282a3da25bd ("powerpc/64: Implement soft interrupt replay
>>>>>>>>>> in
>>>>>>>>>> C"). My day is just finishing now so I haven't applied too much
>>>>>>>>>> scrutiny
>>>>>>>>>> to this result. Given the various rabbit holes I've been down on this
>>>>>>>>>> issue already I'd take this information with a good degree of
>>>>>>>>>> skepticism.
>>>>>>>>>>
>>>>>>>>> OK, so an easy test should be to re-test with a 5.4 kernel.
>>>>>>>>> It doesn't have yet the change you're referring to, and the fsl-espi
>>>>>>>>> driver
>>>>>>>>> is basically the same as in 5.7 (just two small changes in 5.7).
>>>>>>>> There's 6cc0c16d82f88 and maybe also other interrupt related patches
>>>>>>>> around this time that could affect book E, so it's good if that exact
>>>>>>>> patch is confirmed.
>>>>>>> My confirmation is basically that I can induce the issue in a 5.4 kernel
>>>>>>> by cherry-picking 3282a3da25bd. I'm also able to "
Re: fsl_espi errors on v5.7.15
On 30.08.2020 23:59, Chris Packham wrote:
>
> On 31/08/20 9:41 am, Heiner Kallweit wrote:
>> On 30.08.2020 23:00, Chris Packham wrote:
>>> On 31/08/20 12:30 am, Nicholas Piggin wrote:
>>>> Excerpts from Chris Packham's message of August 28, 2020 8:07 am:
>>>
>>>
>>>>>>>>> I've also now seen the RX FIFO not empty error on the T2080RDB
>>>>>>>>>
>>>>>>>>> fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set!
>>>>>>>>> fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set!
>>>>>>>>> fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set!
>>>>>>>>> fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set!
>>>>>>>>> fsl_espi ffe11.spi: Transfer done but rx/tx fifo's aren't empty!
>>>>>>>>> fsl_espi ffe11.spi: SPIE_RXCNT = 1, SPIE_TXCNT = 32
>>>>>>>>>
>>>>>>>>> With my current workaround of emptying the RX FIFO. It seems
>>>>>>>>> survivable. Interestingly it only ever seems to be 1 extra byte in the
>>>>>>>>> RX FIFO and it seems to be after either a READ_SR or a READ_FSR.
>>>>>>>>>
>>>>>>>>> fsl_espi ffe11.spi: tx 70
>>>>>>>>> fsl_espi ffe11.spi: rx 03
>>>>>>>>> fsl_espi ffe11.spi: Extra RX 00
>>>>>>>>> fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set!
>>>>>>>>> fsl_espi ffe11.spi: Transfer done but rx/tx fifo's aren't empty!
>>>>>>>>> fsl_espi ffe11.spi: SPIE_RXCNT = 1, SPIE_TXCNT = 32
>>>>>>>>> fsl_espi ffe11.spi: tx 05
>>>>>>>>> fsl_espi ffe11.spi: rx 00
>>>>>>>>> fsl_espi ffe11.spi: Extra RX 03
>>>>>>>>> fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set!
>>>>>>>>> fsl_espi ffe11.spi: Transfer done but rx/tx fifo's aren't empty!
>>>>>>>>> fsl_espi ffe11.spi: SPIE_RXCNT = 1, SPIE_TXCNT = 32
>>>>>>>>> fsl_espi ffe11.spi: tx 05
>>>>>>>>> fsl_espi ffe11.spi: rx 00
>>>>>>>>> fsl_espi ffe11.spi: Extra RX 03
>>>>>>>>>
>>>>>>>>>From all the Micron SPI-NOR datasheets I've got access to it is
>>>>>>>>> possible to continually read the SR/FSR. But I've no idea why it
>>>>>>>>> happens some times and not others.
>>>>>>>> So I think I've got a reproduction and I think I've bisected the
>>>>>>>> problem
>>>>>>>> to commit 3282a3da25bd ("powerpc/64: Implement soft interrupt replay in
>>>>>>>> C"). My day is just finishing now so I haven't applied too much
>>>>>>>> scrutiny
>>>>>>>> to this result. Given the various rabbit holes I've been down on this
>>>>>>>> issue already I'd take this information with a good degree of
>>>>>>>> skepticism.
>>>>>>>>
>>>>>>> OK, so an easy test should be to re-test with a 5.4 kernel.
>>>>>>> It doesn't have yet the change you're referring to, and the fsl-espi
>>>>>>> driver
>>>>>>> is basically the same as in 5.7 (just two small changes in 5.7).
>>>>>> There's 6cc0c16d82f88 and maybe also other interrupt related patches
>>>>>> around this time that could affect book E, so it's good if that exact
>>>>>> patch is confirmed.
>>>>> My confirmation is basically that I can induce the issue in a 5.4 kernel
>>>>> by cherry-picking 3282a3da25bd. I'm also able to "fix" the issue in
>>>>> 5.9-rc2 by reverting that one commit.
>>>>>
>>>>> I both cases it's not exactly a clean cherry-pick/revert so I also
>>>>> confirmed the bisection result by building at 3282a3da25bd (which sees
>>>>> the issue) and the commit just before (which does not).
>>>> Thanks for testing, that confirms it well.
>>>>
>>>> [snip patch]
>>>>
>>>>> I still saw the issue with this change applied. PPC_IRQ_SOFT_MASK_DEBUG
>>>>> didn't report anything (either with or with
Re: fsl_espi errors on v5.7.15
On 30.08.2020 23:00, Chris Packham wrote:
>
> On 31/08/20 12:30 am, Nicholas Piggin wrote:
>> Excerpts from Chris Packham's message of August 28, 2020 8:07 am:
>
>
>
>>> I've also now seen the RX FIFO not empty error on the T2080RDB
>>>
>>> fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set!
>>> fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set!
>>> fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set!
>>> fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set!
>>> fsl_espi ffe11.spi: Transfer done but rx/tx fifo's aren't empty!
>>> fsl_espi ffe11.spi: SPIE_RXCNT = 1, SPIE_TXCNT = 32
>>>
>>> With my current workaround of emptying the RX FIFO. It seems
>>> survivable. Interestingly it only ever seems to be 1 extra byte in the
>>> RX FIFO and it seems to be after either a READ_SR or a READ_FSR.
>>>
>>> fsl_espi ffe11.spi: tx 70
>>> fsl_espi ffe11.spi: rx 03
>>> fsl_espi ffe11.spi: Extra RX 00
>>> fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set!
>>> fsl_espi ffe11.spi: Transfer done but rx/tx fifo's aren't empty!
>>> fsl_espi ffe11.spi: SPIE_RXCNT = 1, SPIE_TXCNT = 32
>>> fsl_espi ffe11.spi: tx 05
>>> fsl_espi ffe11.spi: rx 00
>>> fsl_espi ffe11.spi: Extra RX 03
>>> fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set!
>>> fsl_espi ffe11.spi: Transfer done but rx/tx fifo's aren't empty!
>>> fsl_espi ffe11.spi: SPIE_RXCNT = 1, SPIE_TXCNT = 32
>>> fsl_espi ffe11.spi: tx 05
>>> fsl_espi ffe11.spi: rx 00
>>> fsl_espi ffe11.spi: Extra RX 03
>>>
>>> From all the Micron SPI-NOR datasheets I've got access to it is
>>> possible to continually read the SR/FSR. But I've no idea why it
>>> happens some times and not others.
>> So I think I've got a reproduction and I think I've bisected the problem
>> to commit 3282a3da25bd ("powerpc/64: Implement soft interrupt replay in
>> C"). My day is just finishing now so I haven't applied too much scrutiny
>> to this result. Given the various rabbit holes I've been down on this
>> issue already I'd take this information with a good degree of skepticism.
>>
> OK, so an easy test should be to re-test with a 5.4 kernel.
> It doesn't have yet the change you're referring to, and the fsl-espi
> driver
> is basically the same as in 5.7 (just two small changes in 5.7).
There's 6cc0c16d82f88 and maybe also other interrupt related patches
around this time that could affect book E, so it's good if that exact
patch is confirmed.
>>> My confirmation is basically that I can induce the issue in a 5.4 kernel
>>> by cherry-picking 3282a3da25bd. I'm also able to "fix" the issue in
>>> 5.9-rc2 by reverting that one commit.
>>>
>>> I both cases it's not exactly a clean cherry-pick/revert so I also
>>> confirmed the bisection result by building at 3282a3da25bd (which sees
>>> the issue) and the commit just before (which does not).
>> Thanks for testing, that confirms it well.
>>
>> [snip patch]
>>
>>> I still saw the issue with this change applied. PPC_IRQ_SOFT_MASK_DEBUG
>>> didn't report anything (either with or without the change above).
>> Okay, it was a bit of a shot in the dark. I still can't see what
>> else has changed.
>>
>> What would cause this, a lost interrupt? A spurious interrupt? Or
>> higher interrupt latency?
>>
>> I don't think the patch should cause significantly worse latency,
>> (it's supposed to be a bit better if anything because it doesn't set
>> up the full interrupt frame). But it's possible.
>
> My working theory is that the SPI_DON indication is all about the TX
> direction an now that the interrupts are faster we're hitting an error
> because there is still RX activity going on. Heiner disagrees with my
> interpretation of the SPI_DON indication and the fact that it doesn't
> happen every time does throw doubt on it.
>
It's right that the eSPI spec can be interpreted that SPI_DON refers to
TX only. However this wouldn't really make sense, because also for RX
we program the frame length, and therefore want to be notified once the
full frame was received. Also practical experience shows that SPI_DON
is set also after RX-only transfers.
Typical SPI NOR use case is that you write read command + start address,
followed by a longer read. If the TX-only interpretation would be right,
we'd always end up with SPI_DON not being set.
> I can't really explain the extra RX byte in the fifo. We know how many
> bytes to expect and we pull that many from the fifo so it's not as if
> we're missing an interrupt causing us to skip the last byte. I've been
> looking for some kind of off-by-one calculation but again if it were
> something like that it'd happen all the time.
>
Maybe it helps to know what value this extra byte in the FIFO has. Is it:
- a duplicate of the last read
Re: fsl_espi errors on v5.7.15
On 30.08.2020 14:30, Nicholas Piggin wrote: > Excerpts from Chris Packham's message of August 28, 2020 8:07 am: >> On 27/08/20 7:12 pm, Nicholas Piggin wrote: >>> Excerpts from Heiner Kallweit's message of August 26, 2020 4:38 pm: >>>> On 26.08.2020 08:07, Chris Packham wrote: >>>>> On 26/08/20 1:48 pm, Chris Packham wrote: >>>>>> On 26/08/20 10:22 am, Chris Packham wrote: >>>>>>> On 25/08/20 7:22 pm, Heiner Kallweit wrote: >>>>>>> >>>>>>> >>>>>>>> I've been staring at spi-fsl-espi.c for while now and I think I've >>>>>>>>> identified a couple of deficiencies that may or may not be related >>>>>>>>> to my >>>>>>>>> issue. >>>>>>>>> >>>>>>>>> First I think the 'Transfer done but SPIE_DON isn't set' message >>>>>>>>> can be >>>>>>>>> generated spuriously. In fsl_espi_irq() we read the ESPI_SPIE >>>>>>>>> register. >>>>>>>>> We also write back to it to clear the current events. We re-read it in >>>>>>>>> fsl_espi_cpu_irq() and complain when SPIE_DON is not set. But we can >>>>>>>>> naturally end up in that situation if we're doing a large read. >>>>>>>>> Consider >>>>>>>>> the messages for reading a block of data from a spi-nor chip >>>>>>>>> >>>>>>>>> tx = READ_OP + ADDR >>>>>>>>> rx = data >>>>>>>>> >>>>>>>>> We setup the transfer and pump out the tx_buf. The first interrupt >>>>>>>>> goes >>>>>>>>> off and ESPI_SPIE has SPIM_DON and SPIM_RXT set. We empty the rx fifo, >>>>>>>>> clear ESPI_SPIE and wait for the next interrupt. The next interrupt >>>>>>>>> fires and this time we have ESPI_SPIE with just SPIM_RXT set. This >>>>>>>>> continues until we've received all the data and we finish with >>>>>>>>> ESPI_SPIE >>>>>>>>> having only SPIM_RXT set. When we re-read it we complain that SPIE_DON >>>>>>>>> isn't set. >>>>>>>>> >>>>>>>>> The other deficiency is that we only get an interrupt when the >>>>>>>>> amount of >>>>>>>>> data in the rx fifo is above FSL_ESPI_RXTHR. If there are fewer than >>>>>>>>> FSL_ESPI_RXTHR left to be received we will never pull them out of >>>>>>>>> the fifo. >>>>>>>>> >>>>>>>> SPIM_DON will trigger an interrupt once the last characters have been >>>>>>>> transferred, and read the remaining characters from the FIFO. >>>>>>> The T2080RM that I have says the following about the DON bit >>>>>>> >>>>>>> "Last character was transmitted. The last character was transmitted >>>>>>> and a new command can be written for the next frame." >>>>>>> >>>>>>> That does at least seem to fit with my assertion that it's all about >>>>>>> the TX direction. But the fact that it doesn't happen all the time >>>>>>> throws some doubt on it. >>>>>>> >>>>>>>> I think the reason I'm seeing some variability is because of how fast >>>>>>>>> (or slow) the interrupts get processed and how fast the spi-nor >>>>>>>>> chip can >>>>>>>>> fill the CPUs rx fifo. >>>>>>>>> >>>>>>>> To rule out timing issues at high bus frequencies I initially asked >>>>>>>> for re-testing at lower frequencies. If you e.g. limit the bus to 1 MHz >>>>>>>> or even less, then timing shouldn't be an issue. >>>>>>> Yes I've currently got spi-max-frequency = <100>; in my dts. I >>>>>>> would also expect a slower frequency would fit my "DON is for TX" >>>>>>> narrative. >>>>>>>> Last relevant functional changes have been done almost 4 years ago. >>>>>>>> And yours is the first such report I see. So question is w
Re: fsl_espi errors on v5.7.15
On 26.08.2020 08:07, Chris Packham wrote: > > On 26/08/20 1:48 pm, Chris Packham wrote: >> >> On 26/08/20 10:22 am, Chris Packham wrote: >>> On 25/08/20 7:22 pm, Heiner Kallweit wrote: >>> >>> >>>> I've been staring at spi-fsl-espi.c for while now and I think I've >>>>> identified a couple of deficiencies that may or may not be related >>>>> to my >>>>> issue. >>>>> >>>>> First I think the 'Transfer done but SPIE_DON isn't set' message >>>>> can be >>>>> generated spuriously. In fsl_espi_irq() we read the ESPI_SPIE >>>>> register. >>>>> We also write back to it to clear the current events. We re-read it in >>>>> fsl_espi_cpu_irq() and complain when SPIE_DON is not set. But we can >>>>> naturally end up in that situation if we're doing a large read. >>>>> Consider >>>>> the messages for reading a block of data from a spi-nor chip >>>>> >>>>> tx = READ_OP + ADDR >>>>> rx = data >>>>> >>>>> We setup the transfer and pump out the tx_buf. The first interrupt >>>>> goes >>>>> off and ESPI_SPIE has SPIM_DON and SPIM_RXT set. We empty the rx fifo, >>>>> clear ESPI_SPIE and wait for the next interrupt. The next interrupt >>>>> fires and this time we have ESPI_SPIE with just SPIM_RXT set. This >>>>> continues until we've received all the data and we finish with >>>>> ESPI_SPIE >>>>> having only SPIM_RXT set. When we re-read it we complain that SPIE_DON >>>>> isn't set. >>>>> >>>>> The other deficiency is that we only get an interrupt when the >>>>> amount of >>>>> data in the rx fifo is above FSL_ESPI_RXTHR. If there are fewer than >>>>> FSL_ESPI_RXTHR left to be received we will never pull them out of >>>>> the fifo. >>>>> >>>> SPIM_DON will trigger an interrupt once the last characters have been >>>> transferred, and read the remaining characters from the FIFO. >>> >>> The T2080RM that I have says the following about the DON bit >>> >>> "Last character was transmitted. The last character was transmitted >>> and a new command can be written for the next frame." >>> >>> That does at least seem to fit with my assertion that it's all about >>> the TX direction. But the fact that it doesn't happen all the time >>> throws some doubt on it. >>> >>>> I think the reason I'm seeing some variability is because of how fast >>>>> (or slow) the interrupts get processed and how fast the spi-nor >>>>> chip can >>>>> fill the CPUs rx fifo. >>>>> >>>> To rule out timing issues at high bus frequencies I initially asked >>>> for re-testing at lower frequencies. If you e.g. limit the bus to 1 MHz >>>> or even less, then timing shouldn't be an issue. >>> Yes I've currently got spi-max-frequency = <100>; in my dts. I >>> would also expect a slower frequency would fit my "DON is for TX" >>> narrative. >>>> Last relevant functional changes have been done almost 4 years ago. >>>> And yours is the first such report I see. So question is what could >>>> be so >>>> special with your setup that it seems you're the only one being >>>> affected. >>>> The scenarios you describe are standard, therefore much more people >>>> should be affected in case of a driver bug. >>> Agreed. But even on my hardware (which may have a latent issue >>> despite being in the field for going on 5 years) the issue only >>> triggers under some fairly specific circumstances. >>>> You said that kernel config impacts how frequently the issue happens. >>>> Therefore question is what's the diff in kernel config, and how could >>>> the differences be related to SPI. >>> >>> It did seem to be somewhat random. Things like CONFIG_PREEMPT have an >>> impact but every time I found something that seemed to be having an >>> impact I've been able to disprove it. I actually think its about how >>> busy the system is which may or may not affect when we get round to >>> processing the interrupts. >>> >>> I have managed to get the 'Transfer done but SPIE_DON isn't set!' to >>> occur on the
Re: fsl_espi errors on v5.7.15
On 25.08.2020 05:54, Chris Packham wrote: > > On 25/08/20 10:04 am, Chris Packham wrote: >> >> On 20/08/20 9:08 am, Chris Packham wrote: >>> >>> On 19/08/20 6:15 pm, Heiner Kallweit wrote: >>>> On 19.08.2020 00:44, Chris Packham wrote: >>>>> Hi Again, >>>>> >>>>> On 17/08/20 9:09 am, Chris Packham wrote: >>>>> >>>>>> On 14/08/20 6:19 pm, Heiner Kallweit wrote: >>>>>>> On 14.08.2020 04:48, Chris Packham wrote: >>>>>>>> Hi, >>>>>>>> >>>>>>>> I'm seeing a problem with accessing spi-nor after upgrading a T2081 >>>>>>>> based system to linux v5.7.15 >>>>>>>> >>>>>>>> For this board u-boot and the u-boot environment live on spi-nor. >>>>>>>> >>>>>>>> When I use fw_setenv from userspace I get the following kernel logs >>>>>>>> >>>>>>>> # fw_setenv foo=1 >>>>>>>> fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set! >>>>>>>> fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set! >>>>>>>> fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set! >>>>>>>> fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set! >>>>>>>> fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set! >>>>>>>> fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set! >>>>>>>> fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set! >>>>>>>> fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set! >>>>>>>> fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set! >>>>>>>> fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set! >>>>>>>> fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set! >>>>>>>> fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set! >>>>>>>> fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set! >>>>>>>> fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set! >>>>>>>> fsl_espi ffe11.spi: Transfer done but rx/tx fifo's aren't >>>>>>>> empty! >>>>>>>> fsl_espi ffe11.spi: SPIE_RXCNT = 1, SPIE_TXCNT = 32 >>>>>>>> fsl_espi ffe11.spi: Transfer done but rx/tx fifo's aren't >>>>>>>> empty! >>>>>>>> fsl_espi ffe11.spi: SPIE_RXCNT = 1, SPIE_TXCNT = 32 >>>>>>>> fsl_espi ffe11.spi: Transfer done but rx/tx fifo's aren't >>>>>>>> empty! >>>>>>>> fsl_espi ffe11.spi: SPIE_RXCNT = 1, SPIE_TXCNT = 32 >>>>>>>> ... >>>>>>>> >>>>>>> This error reporting doesn't exist yet in 4.4. So you may have an >>>>>>> issue >>>>>>> under 4.4 too, it's just not reported. >>>>>>> Did you verify that under 4.4 fw_setenv actually has an effect? >>>>>> Just double checked and yes under 4.4 the setting does get saved. >>>>>>>> If I run fw_printenv (before getting it into a bad state) it is >>>>>>>> able to >>>>>>>> display the content of the boards u-boot environment. >>>>>>>> >>>>>>> This might indicate an issue with spi being locked. I've seen >>>>>>> related >>>>>>> questions, just use the search engine of your choice and check for >>>>>>> fw_setenv and locked. >>>>>> I'm running a version of fw_setenv which includes >>>>>> https://gitlab.denx.de/u-boot/u-boot/-/commit/db820159 so it >>>>>> shouldn't >>>>>> be locking things unnecessarily. >>>>>>>> If been unsuccessful in producing a setup for bisecting the >>>>>>>> issue. I do >>>>>>>> know the issue doesn't occur on the old 4.4.x based kernel but >>>>>>>> that's >>>>>>>> probably not much help. >>>>>>>> >>>>>>>> Any pointers on what the issue (and/or solution) might be. >>>>> I finally managed to get our board running with a vanilla kernel. With >>>>> corenet64_smp_defconfig
Re: fsl_espi errors on v5.7.15
On 19.08.2020 00:44, Chris Packham wrote: > Hi Again, > > On 17/08/20 9:09 am, Chris Packham wrote: > >> >> On 14/08/20 6:19 pm, Heiner Kallweit wrote: >>> On 14.08.2020 04:48, Chris Packham wrote: >>>> Hi, >>>> >>>> I'm seeing a problem with accessing spi-nor after upgrading a T2081 >>>> based system to linux v5.7.15 >>>> >>>> For this board u-boot and the u-boot environment live on spi-nor. >>>> >>>> When I use fw_setenv from userspace I get the following kernel logs >>>> >>>> # fw_setenv foo=1 >>>> fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set! >>>> fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set! >>>> fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set! >>>> fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set! >>>> fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set! >>>> fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set! >>>> fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set! >>>> fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set! >>>> fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set! >>>> fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set! >>>> fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set! >>>> fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set! >>>> fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set! >>>> fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set! >>>> fsl_espi ffe11.spi: Transfer done but rx/tx fifo's aren't empty! >>>> fsl_espi ffe11.spi: SPIE_RXCNT = 1, SPIE_TXCNT = 32 >>>> fsl_espi ffe11.spi: Transfer done but rx/tx fifo's aren't empty! >>>> fsl_espi ffe11.spi: SPIE_RXCNT = 1, SPIE_TXCNT = 32 >>>> fsl_espi ffe11.spi: Transfer done but rx/tx fifo's aren't empty! >>>> fsl_espi ffe11.spi: SPIE_RXCNT = 1, SPIE_TXCNT = 32 >>>> ... >>>> >>> This error reporting doesn't exist yet in 4.4. So you may have an issue >>> under 4.4 too, it's just not reported. >>> Did you verify that under 4.4 fw_setenv actually has an effect? >> Just double checked and yes under 4.4 the setting does get saved. >>>> If I run fw_printenv (before getting it into a bad state) it is able to >>>> display the content of the boards u-boot environment. >>>> >>> This might indicate an issue with spi being locked. I've seen related >>> questions, just use the search engine of your choice and check for >>> fw_setenv and locked. >> I'm running a version of fw_setenv which includes >> https://gitlab.denx.de/u-boot/u-boot/-/commit/db820159 so it shouldn't >> be locking things unnecessarily. >>>> If been unsuccessful in producing a setup for bisecting the issue. I do >>>> know the issue doesn't occur on the old 4.4.x based kernel but that's >>>> probably not much help. >>>> >>>> Any pointers on what the issue (and/or solution) might be. > > I finally managed to get our board running with a vanilla kernel. With > corenet64_smp_defconfig I occasionally see > > fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set! > > other than the message things seem to be working. > > With a custom defconfig I see > > fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set! > fsl_espi ffe11.spi: Transfer done but rx/tx fifo's aren't empty! > fsl_espi ffe11.spi: SPIE_RXCNT = 1, SPIE_TXCNT = 32 > ... > > and access to the spi-nor does not work until the board is reset. > > I'll try and pick apart the differences between the two defconfigs. > Also relevant may be: - Which dts are you using? - What's the spi-nor type, and at which frequency are you operating it? - Does the issue still happen if you lower the frequency?
Re: fsl_espi errors on v5.7.15
On 14.08.2020 04:48, Chris Packham wrote: > Hi, > > I'm seeing a problem with accessing spi-nor after upgrading a T2081 > based system to linux v5.7.15 > > For this board u-boot and the u-boot environment live on spi-nor. > > When I use fw_setenv from userspace I get the following kernel logs > > # fw_setenv foo=1 > fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set! > fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set! > fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set! > fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set! > fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set! > fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set! > fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set! > fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set! > fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set! > fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set! > fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set! > fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set! > fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set! > fsl_espi ffe11.spi: Transfer done but SPIE_DON isn't set! > fsl_espi ffe11.spi: Transfer done but rx/tx fifo's aren't empty! > fsl_espi ffe11.spi: SPIE_RXCNT = 1, SPIE_TXCNT = 32 > fsl_espi ffe11.spi: Transfer done but rx/tx fifo's aren't empty! > fsl_espi ffe11.spi: SPIE_RXCNT = 1, SPIE_TXCNT = 32 > fsl_espi ffe11.spi: Transfer done but rx/tx fifo's aren't empty! > fsl_espi ffe11.spi: SPIE_RXCNT = 1, SPIE_TXCNT = 32 > ... > This error reporting doesn't exist yet in 4.4. So you may have an issue under 4.4 too, it's just not reported. Did you verify that under 4.4 fw_setenv actually has an effect? > If I run fw_printenv (before getting it into a bad state) it is able to > display the content of the boards u-boot environment. > This might indicate an issue with spi being locked. I've seen related questions, just use the search engine of your choice and check for fw_setenv and locked. > If been unsuccessful in producing a setup for bisecting the issue. I do > know the issue doesn't occur on the old 4.4.x based kernel but that's > probably not much help. > > Any pointers on what the issue (and/or solution) might be. > > Thanks, > Chris > Heiner
Re: [PATCH v2 net-next] net: convert suitable drivers to use phy_do_ioctl_running
On 22.01.2020 05:04, Florian Fainelli wrote: > > > On 1/21/2020 1:09 PM, Heiner Kallweit wrote: >> Convert suitable drivers to use new helper phy_do_ioctl_running. >> >> Signed-off-by: Heiner Kallweit > The vast majority of drivers that you are converting use the following > convention: > > - !netif_running -> return -EINVAL > - !dev->phydev -> return -ENODEV > > so it may make sense to change the helper to accommodate the majority > here, not that I believe this is going to make much practical > difference, but if there were test cases that were specifically looking > for such an error code, they could be failing after this changeset. > Right, I also stumbled across the different error codes, mainly as you say -EINVAL. However there is no "wrong value", if netdev isn't running, then typically the PHY is not attached, and from a netdev point of view it's not there. So ENODEV seems to be best suited. In kernel code the changed return code doesn't make a difference, but yes, in theory there could be userspace programs checking for -EINVAL. However such userspace programs should check for ENODEV too anyway to cover the second check that already returns -ENODEV. > For bgmac.c, bcmgenet.c and cpmac.c: > > Acked-by: Florian Fainelli > > Whether you decide to spin another version or not. > Heiner
[PATCH v2 net-next] net: convert suitable drivers to use phy_do_ioctl_running
Convert suitable drivers to use new helper phy_do_ioctl_running.
Signed-off-by: Heiner Kallweit
---
v2: I forgot the netdev mailing list
---
drivers/net/ethernet/allwinner/sun4i-emac.c| 15 +--
drivers/net/ethernet/amd/au1000_eth.c | 13 +
drivers/net/ethernet/arc/emac_main.c | 14 +-
drivers/net/ethernet/broadcom/bgmac.c | 10 +-
drivers/net/ethernet/broadcom/genet/bcmgenet.c | 14 +-
drivers/net/ethernet/dnet.c| 15 +--
.../ethernet/freescale/fs_enet/fs_enet-main.c | 10 +-
drivers/net/ethernet/hisilicon/hisi_femac.c| 14 +-
drivers/net/ethernet/hisilicon/hns/hns_enet.c | 16 +---
drivers/net/ethernet/nxp/lpc_eth.c | 15 +--
drivers/net/ethernet/qualcomm/emac/emac.c | 14 +-
drivers/net/ethernet/renesas/sh_eth.c | 18 ++
drivers/net/ethernet/smsc/smsc911x.c | 11 +--
drivers/net/ethernet/smsc/smsc9420.c | 11 +--
drivers/net/ethernet/ti/cpmac.c| 12 +---
drivers/net/ethernet/toshiba/tc35815.c | 12 +---
drivers/net/ethernet/xilinx/ll_temac_main.c| 13 +
drivers/net/usb/ax88172a.c | 13 +
drivers/net/usb/lan78xx.c | 10 +-
19 files changed, 20 insertions(+), 230 deletions(-)
diff --git a/drivers/net/ethernet/allwinner/sun4i-emac.c
b/drivers/net/ethernet/allwinner/sun4i-emac.c
index 5ea806423..22cadfbee 100644
--- a/drivers/net/ethernet/allwinner/sun4i-emac.c
+++ b/drivers/net/ethernet/allwinner/sun4i-emac.c
@@ -207,19 +207,6 @@ static void emac_inblk_32bit(void __iomem *reg, void
*data, int count)
readsl(reg, data, round_up(count, 4) / 4);
}
-static int emac_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
-{
- struct phy_device *phydev = dev->phydev;
-
- if (!netif_running(dev))
- return -EINVAL;
-
- if (!phydev)
- return -ENODEV;
-
- return phy_mii_ioctl(phydev, rq, cmd);
-}
-
/* ethtool ops */
static void emac_get_drvinfo(struct net_device *dev,
struct ethtool_drvinfo *info)
@@ -791,7 +778,7 @@ static const struct net_device_ops emac_netdev_ops = {
.ndo_start_xmit = emac_start_xmit,
.ndo_tx_timeout = emac_timeout,
.ndo_set_rx_mode= emac_set_rx_mode,
- .ndo_do_ioctl = emac_ioctl,
+ .ndo_do_ioctl = phy_do_ioctl_running,
.ndo_validate_addr = eth_validate_addr,
.ndo_set_mac_address= emac_set_mac_address,
#ifdef CONFIG_NET_POLL_CONTROLLER
diff --git a/drivers/net/ethernet/amd/au1000_eth.c
b/drivers/net/ethernet/amd/au1000_eth.c
index 6acf5aa99..089a4fbc6 100644
--- a/drivers/net/ethernet/amd/au1000_eth.c
+++ b/drivers/net/ethernet/amd/au1000_eth.c
@@ -1053,23 +1053,12 @@ static void au1000_multicast_list(struct net_device
*dev)
writel(reg, >mac->control);
}
-static int au1000_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
-{
- if (!netif_running(dev))
- return -EINVAL;
-
- if (!dev->phydev)
- return -EINVAL; /* PHY not controllable */
-
- return phy_mii_ioctl(dev->phydev, rq, cmd);
-}
-
static const struct net_device_ops au1000_netdev_ops = {
.ndo_open = au1000_open,
.ndo_stop = au1000_close,
.ndo_start_xmit = au1000_tx,
.ndo_set_rx_mode= au1000_multicast_list,
- .ndo_do_ioctl = au1000_ioctl,
+ .ndo_do_ioctl = phy_do_ioctl_running,
.ndo_tx_timeout = au1000_tx_timeout,
.ndo_set_mac_address= eth_mac_addr,
.ndo_validate_addr = eth_validate_addr,
diff --git a/drivers/net/ethernet/arc/emac_main.c
b/drivers/net/ethernet/arc/emac_main.c
index 6f2c86778..17bda4e8c 100644
--- a/drivers/net/ethernet/arc/emac_main.c
+++ b/drivers/net/ethernet/arc/emac_main.c
@@ -781,18 +781,6 @@ static int arc_emac_set_address(struct net_device *ndev,
void *p)
return 0;
}
-static int arc_emac_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
-{
- if (!netif_running(dev))
- return -EINVAL;
-
- if (!dev->phydev)
- return -ENODEV;
-
- return phy_mii_ioctl(dev->phydev, rq, cmd);
-}
-
-
/**
* arc_emac_restart - Restart EMAC
* @ndev: Pointer to net_device structure.
@@ -857,7 +845,7 @@ static const struct net_device_ops arc_emac_netdev_ops = {
.ndo_set_mac_address= arc_emac_set_address,
.ndo_get_stats = arc_emac_stats,
.ndo_set_rx_mode= arc_emac_set_rx_mode,
- .ndo_do_ioctl = arc_emac_ioctl,
+ .ndo_do_ioctl = phy_do_ioctl_running,
#ifdef CONFIG_NET_POLL_CONTROLLER
[PATCH net-next] net: convert suitable drivers to use phy_do_ioctl_running
Convert suitable drivers to use new helper phy_do_ioctl_running.
Signed-off-by: Heiner Kallweit
---
drivers/net/ethernet/allwinner/sun4i-emac.c| 15 +--
drivers/net/ethernet/amd/au1000_eth.c | 13 +
drivers/net/ethernet/arc/emac_main.c | 14 +-
drivers/net/ethernet/broadcom/bgmac.c | 10 +-
drivers/net/ethernet/broadcom/genet/bcmgenet.c | 14 +-
drivers/net/ethernet/dnet.c| 15 +--
.../ethernet/freescale/fs_enet/fs_enet-main.c | 10 +-
drivers/net/ethernet/hisilicon/hisi_femac.c| 14 +-
drivers/net/ethernet/hisilicon/hns/hns_enet.c | 16 +---
drivers/net/ethernet/nxp/lpc_eth.c | 15 +--
drivers/net/ethernet/qualcomm/emac/emac.c | 14 +-
drivers/net/ethernet/renesas/sh_eth.c | 18 ++
drivers/net/ethernet/smsc/smsc911x.c | 11 +--
drivers/net/ethernet/smsc/smsc9420.c | 11 +--
drivers/net/ethernet/ti/cpmac.c| 12 +---
drivers/net/ethernet/toshiba/tc35815.c | 12 +---
drivers/net/ethernet/xilinx/ll_temac_main.c| 13 +
drivers/net/usb/ax88172a.c | 13 +
drivers/net/usb/lan78xx.c | 10 +-
19 files changed, 20 insertions(+), 230 deletions(-)
diff --git a/drivers/net/ethernet/allwinner/sun4i-emac.c
b/drivers/net/ethernet/allwinner/sun4i-emac.c
index 5ea806423..22cadfbee 100644
--- a/drivers/net/ethernet/allwinner/sun4i-emac.c
+++ b/drivers/net/ethernet/allwinner/sun4i-emac.c
@@ -207,19 +207,6 @@ static void emac_inblk_32bit(void __iomem *reg, void
*data, int count)
readsl(reg, data, round_up(count, 4) / 4);
}
-static int emac_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
-{
- struct phy_device *phydev = dev->phydev;
-
- if (!netif_running(dev))
- return -EINVAL;
-
- if (!phydev)
- return -ENODEV;
-
- return phy_mii_ioctl(phydev, rq, cmd);
-}
-
/* ethtool ops */
static void emac_get_drvinfo(struct net_device *dev,
struct ethtool_drvinfo *info)
@@ -791,7 +778,7 @@ static const struct net_device_ops emac_netdev_ops = {
.ndo_start_xmit = emac_start_xmit,
.ndo_tx_timeout = emac_timeout,
.ndo_set_rx_mode= emac_set_rx_mode,
- .ndo_do_ioctl = emac_ioctl,
+ .ndo_do_ioctl = phy_do_ioctl_running,
.ndo_validate_addr = eth_validate_addr,
.ndo_set_mac_address= emac_set_mac_address,
#ifdef CONFIG_NET_POLL_CONTROLLER
diff --git a/drivers/net/ethernet/amd/au1000_eth.c
b/drivers/net/ethernet/amd/au1000_eth.c
index 6acf5aa99..089a4fbc6 100644
--- a/drivers/net/ethernet/amd/au1000_eth.c
+++ b/drivers/net/ethernet/amd/au1000_eth.c
@@ -1053,23 +1053,12 @@ static void au1000_multicast_list(struct net_device
*dev)
writel(reg, >mac->control);
}
-static int au1000_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
-{
- if (!netif_running(dev))
- return -EINVAL;
-
- if (!dev->phydev)
- return -EINVAL; /* PHY not controllable */
-
- return phy_mii_ioctl(dev->phydev, rq, cmd);
-}
-
static const struct net_device_ops au1000_netdev_ops = {
.ndo_open = au1000_open,
.ndo_stop = au1000_close,
.ndo_start_xmit = au1000_tx,
.ndo_set_rx_mode= au1000_multicast_list,
- .ndo_do_ioctl = au1000_ioctl,
+ .ndo_do_ioctl = phy_do_ioctl_running,
.ndo_tx_timeout = au1000_tx_timeout,
.ndo_set_mac_address= eth_mac_addr,
.ndo_validate_addr = eth_validate_addr,
diff --git a/drivers/net/ethernet/arc/emac_main.c
b/drivers/net/ethernet/arc/emac_main.c
index 6f2c86778..17bda4e8c 100644
--- a/drivers/net/ethernet/arc/emac_main.c
+++ b/drivers/net/ethernet/arc/emac_main.c
@@ -781,18 +781,6 @@ static int arc_emac_set_address(struct net_device *ndev,
void *p)
return 0;
}
-static int arc_emac_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
-{
- if (!netif_running(dev))
- return -EINVAL;
-
- if (!dev->phydev)
- return -ENODEV;
-
- return phy_mii_ioctl(dev->phydev, rq, cmd);
-}
-
-
/**
* arc_emac_restart - Restart EMAC
* @ndev: Pointer to net_device structure.
@@ -857,7 +845,7 @@ static const struct net_device_ops arc_emac_netdev_ops = {
.ndo_set_mac_address= arc_emac_set_address,
.ndo_get_stats = arc_emac_stats,
.ndo_set_rx_mode= arc_emac_set_rx_mode,
- .ndo_do_ioctl = arc_emac_ioctl,
+ .ndo_do_ioctl = phy_do_ioctl_running,
#ifdef CONFIG_NET_POLL_CONTROLLER
.ndo_poll_controller= arc_emac_pol
Re: Freescale network device not activated on mpc8360 (kmeter1 board)
On 08.01.2020 12:53, Matteo Ghidoni wrote:
> Hi Heiner, thank you for the quick answer.
>
>>> Hi all,
>>>
>>> With the introduction of the following patch, we are facing an issue with
>> the activation of the Freescale network device (ucc_geth driver) on our
>> kmeter1 board based on a MPC8360:
>>
>> +Li as ucc_geth maintainer
>>
>> Can you describe the symptoms of the issue?
>
> I am trying to boot in NFS, but as soon as the boot process is finished there
> is no network connections between the board and the host.
>
>>>
>>> commit 124eee3f6955f7aa19b9e6ff5c9b6d37cb3d1e2c
>>> Author: Heiner Kallweit
>>> Date: Tue Sep 18 21:55:36 2018 +0200
>>>
>>> net: linkwatch: add check for netdevice being present to
>>> linkwatch_do_dev
>>>
>>> Based on my observations, just before trying to activate the device through
>> linkwatch_event, the controller wants to adjust the MAC configuration and in
>> order to achieve this it detaches the device. This avoids the activation of
>> the
>> net device.
>>>
>> It sounds a little bit odd to rely on an asynchronous linkwatch event here.
>> Can you give a call trace?
>
> Here is a call trace form the adjust_link function in the if condition at
> line 1644 (ucc_geth.c file):
>
> CPU: 0 PID: 35 Comm: kworker/0:1 Not tainted 5.4.8-dirty #19
> Workqueue: events_power_efficient phy_state_machine
> Call Trace:
> [cf88bde8] [c02ddca8] adjust_link+0x304/0x320 (unreliable)
> [cf88be28] [c02cbf3c] phy_check_link_status+0xe4/0xfc
> [cf88be48] [c02cccdc] phy_state_machine+0x44/0x170
> [cf88be78] [c00361a0] process_one_work+0x264/0x408
> [cf88bea8] [c00370f8] worker_thread+0x140/0x53c
> [cf88bef8] [c003d818] kthread+0xdc/0x108
> [cf88bf38] [c0010274] ret_from_kernel_thread+0x14/0x1c
>
> Here the call trace from the netif_carrier_on function just before the call
> to the linkwatch_fire_event function (line 498, sch_generic.c file):
>
> CPU: 0 PID: 35 Comm: kworker/0:1 Not tainted 5.4.8-dirty #20
> Workqueue: events_power_efficient phy_state_machine
> Call Trace:
> [cf88bde8] [c0352064] netif_carrier_on+0xc4/0xc8 (unreliable)
> [cf88be08] [c02cf4ec] phy_link_change+0x84/0xb4
> [cf88be28] [c02cbf3c] phy_check_link_status+0xe4/0xfc
> [cf88be48] [c02cccdc] phy_state_machine+0x44/0x170
> [cf88be78] [c00361a0] process_one_work+0x264/0x408
> [cf88bea8] [c00370f8] worker_thread+0x140/0x53c
> [cf88bef8] [c003d818] kthread+0xdc/0x108
> [cf88bf38] [c0010274] ret_from_kernel_thread+0x14/0x1c
>
> Moreover, I noticed that by adding the dump directly in the linkwatch_do_dev
> function (link_watch.c) the interface comes up correctly, because of the
> delay introduced by the dump_stack function.
>
> Here another log with some prints that maybe can help to understand the
> situation. The prints are placed just before calling the function mentioned
> in the second part of the message (hopefully this will not bring more
> confusion):
>
> <...>
> ubi0: available PEBs: 235, total reserved PEBs: 269, PEBs reserved for bad
> PEB handling: 0
> ubi0: background thread "ubi_bgt0d" started, PID 45
> # [phy_device.c] phy_link_change - calling netif_carrier_on
> (eth2)
> # [sched_generic.c] netif_carrier_on - calling
> linkwatch_fire_event (eth2)
> # [phy_device.c] phy_link_change - calling adjust_link (eth2)
> # [ucc_geth.c] adjust_link - calling ugeth_quiesce (detaching
> device) (eth2)
> # [link_watch.c] linkwatch_do_dev - checking for
> netif_device_present(eth2) => 0
> IP-Config: Guessing netmask 255.255.255.0
> IP-Config: Complete:
> device=eth2, hwaddr=00:e0:df:56:54:07, ipaddr=192.168.1.20,
> mask=255.255.255.0, gw=255.255.255.255
> host=kmeter1, domain=, nis-domain=(none)
> bootserver=192.168.1.100, rootserver=192.168.1.100, rootpath=
> # [ucc_geth.c] adjust_link - calling ugeth_activate
> (attaching device) (eth2)
> ucc_geth e0103200.ucc eth2: Link is Up - 100Mbps/Full - flow control off
> rpcbind: server 192.168.1.100 not responding, timed out
> rpcbind: server 192.168.1.100 not responding, timed out
>
> As mentioned, just before that the linkwatch checks for the net_device
> presence, this one is detached by the ucc_geth driver and reattached later.
>
Detaching the netdev was introduced with 08b5e1c91ce9
("ucc_geth: Fix netdev watchdog triggering on link changes").
Most likely detaching the netdev isn't the best way to fix the original issue.
If it's just about switching the watchdog off temporarily, then maybe
calling dev_watchdog_
Re: Freescale network device not activated on mpc8360 (kmeter1 board)
On 07.01.2020 14:05, Matteo Ghidoni wrote: > Hi all, > > With the introduction of the following patch, we are facing an issue with the > activation of the Freescale network device (ucc_geth driver) on our kmeter1 > board based on a MPC8360: +Li as ucc_geth maintainer Can you describe the symptoms of the issue? > > commit 124eee3f6955f7aa19b9e6ff5c9b6d37cb3d1e2c > Author: Heiner Kallweit > Date: Tue Sep 18 21:55:36 2018 +0200 > > net: linkwatch: add check for netdevice being present to linkwatch_do_dev > > Based on my observations, just before trying to activate the device through > linkwatch_event, the controller wants to adjust the MAC configuration and in > order to achieve this it detaches the device. This avoids the activation of > the net device. > It sounds a little bit odd to rely on an asynchronous linkwatch event here. Can you give a call trace? The driver is quite old and maybe some parts need to be improved. The referenced change is more than a year old and I'm not aware of any other problem with it. So it seems the change isn't wrong. > This is already happening with older versions (I checked with the v4.14.162) > and also there the situation is the same, but without the additional check in > the if condition the device is activated. > > I am currently working with the v5.4.8 kernel version, but the behavior > remains the same also with the latest v5.5-rc4. > > Any idea how to solve this? Any help is appreciated. > > Regards, > Matteo > Heiner
[PATCH 3/3] powerpc/fsl_soc: improve and simplify get_baudrate
Use of_property_read_u32 instead of the generic of_get_property to
simplify the code. In addition move the declaration of fs_baudrate
into get_baudrate because it's private to this function.
Signed-off-by: Heiner Kallweit <hkallwe...@gmail.com>
---
arch/powerpc/sysdev/fsl_soc.c | 10 ++
1 file changed, 2 insertions(+), 8 deletions(-)
diff --git a/arch/powerpc/sysdev/fsl_soc.c b/arch/powerpc/sysdev/fsl_soc.c
index d418424..0c542f1 100644
--- a/arch/powerpc/sysdev/fsl_soc.c
+++ b/arch/powerpc/sysdev/fsl_soc.c
@@ -136,10 +136,9 @@ u32 get_brgfreq(void)
EXPORT_SYMBOL(get_brgfreq);
-static u32 fs_baudrate = -1;
-
u32 get_baudrate(void)
{
+ static u32 fs_baudrate = -1;
struct device_node *node;
if (fs_baudrate != -1)
@@ -147,12 +146,7 @@ u32 get_baudrate(void)
node = of_find_node_by_type(NULL, "serial");
if (node) {
- int size;
- const unsigned int *prop = of_get_property(node,
- "current-speed", );
-
- if (prop)
- fs_baudrate = *prop;
+ of_property_read_u32(node, "current-speed", _baudrate);
of_node_put(node);
}
--
2.10.1
[PATCH 2/3] powerpc/fsl_soc: improve and simplify get_brgfreq
Use of_property_read_u32 instead of the generic of_get_property to
simplify the code. In addition move the declaration of brgfreq
into get_brgfreq because it's private to this function.
Signed-off-by: Heiner Kallweit <hkallwe...@gmail.com>
---
arch/powerpc/sysdev/fsl_soc.c | 24 +++-
1 file changed, 7 insertions(+), 17 deletions(-)
diff --git a/arch/powerpc/sysdev/fsl_soc.c b/arch/powerpc/sysdev/fsl_soc.c
index 48fc36b..d418424 100644
--- a/arch/powerpc/sysdev/fsl_soc.c
+++ b/arch/powerpc/sysdev/fsl_soc.c
@@ -100,23 +100,17 @@ EXPORT_SYMBOL(fsl_get_sys_freq);
#if defined(CONFIG_CPM2) || defined(CONFIG_QUICC_ENGINE) || defined(CONFIG_8xx)
-static u32 brgfreq = -1;
-
u32 get_brgfreq(void)
{
+ static u32 brgfreq = -1;
struct device_node *node;
- const unsigned int *prop;
- int size;
if (brgfreq != -1)
return brgfreq;
node = of_find_compatible_node(NULL, NULL, "fsl,cpm-brg");
if (node) {
- prop = of_get_property(node, "clock-frequency", );
- if (prop && size == 4)
- brgfreq = *prop;
-
+ of_property_read_u32(node, "clock-frequency", );
of_node_put(node);
return brgfreq;
}
@@ -129,15 +123,11 @@ u32 get_brgfreq(void)
node = of_find_node_by_type(NULL, "qe");
if (node) {
- prop = of_get_property(node, "brg-frequency", );
- if (prop && size == 4)
- brgfreq = *prop;
-
- if (brgfreq == -1 || brgfreq == 0) {
- prop = of_get_property(node, "bus-frequency", );
- if (prop && size == 4)
- brgfreq = *prop / 2;
- }
+ of_property_read_u32(node, "brg-frequency", );
+ if (brgfreq == -1 || !brgfreq)
+ if(!of_property_read_u32(node, "bus-frequency",
+ ))
+ brgfreq /= 2;
of_node_put(node);
}
--
2.10.1
[PATCH 1/3] powerpc/fsl_soc: improve and simplify fsl_get_sys_freq
Use of_property_read_u32 instead of the generic of_get_property to
simplify the code. In addition move the declaration of sysfreq
into fsl_get_sys_freq because it's private to this function.
Signed-off-by: Heiner Kallweit <hkallwe...@gmail.com>
---
arch/powerpc/sysdev/fsl_soc.c | 14 --
1 file changed, 4 insertions(+), 10 deletions(-)
diff --git a/arch/powerpc/sysdev/fsl_soc.c b/arch/powerpc/sysdev/fsl_soc.c
index d93056e..48fc36b 100644
--- a/arch/powerpc/sysdev/fsl_soc.c
+++ b/arch/powerpc/sysdev/fsl_soc.c
@@ -77,13 +77,10 @@ phys_addr_t get_immrbase(void)
EXPORT_SYMBOL(get_immrbase);
-static u32 sysfreq = -1;
-
u32 fsl_get_sys_freq(void)
{
+ static u32 sysfreq = -1;
struct device_node *soc;
- const u32 *prop;
- int size;
if (sysfreq != -1)
return sysfreq;
@@ -92,12 +89,9 @@ u32 fsl_get_sys_freq(void)
if (!soc)
return -1;
- prop = of_get_property(soc, "clock-frequency", );
- if (!prop || size != sizeof(*prop) || *prop == 0)
- prop = of_get_property(soc, "bus-frequency", );
-
- if (prop && size == sizeof(*prop))
- sysfreq = *prop;
+ of_property_read_u32(soc, "clock-frequency", );
+ if (sysfreq == -1 || !sysfreq)
+ of_property_read_u32(soc, "bus-frequency", );
of_node_put(soc);
return sysfreq;
--
2.10.1
[PATCH] powerpc/boot: fix boot on systems with uncompressed kernel image
This commit broke boot on systems with an uncompressed kernel image,
namely systems using a cuImage. On such systems the compressed boot
image (boot wrapper, uncompressed kernel image, ..) is decompressed
by u-boot already, therefore the boot wrapper code sees an
uncompressed kernel image.
The old decompression code silently assumed an uncompressed kernel
image if it found no valid gzip signature, whilst the new code
bailed out in this case.
Fix this by re-introducing such a fallback if no valid compressed
image is found.
Fixes: 1b7898ee276b ("Use the pre-boot decompression API")
Signed-off-by: Heiner Kallweit <hkallwe...@gmail.com>
---
arch/powerpc/boot/main.c | 18 --
1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/boot/main.c b/arch/powerpc/boot/main.c
index f7a184b..57d42d1 100644
--- a/arch/powerpc/boot/main.c
+++ b/arch/powerpc/boot/main.c
@@ -32,9 +32,16 @@ static struct addr_range prep_kernel(void)
void *addr = 0;
struct elf_info ei;
long len;
+ int uncompressed_image = 0;
- partial_decompress(vmlinuz_addr, vmlinuz_size,
+ len = partial_decompress(vmlinuz_addr, vmlinuz_size,
elfheader, sizeof(elfheader), 0);
+ /* assume uncompressed data if -1 is returned */
+ if (len == -1) {
+ uncompressed_image = 1;
+ memcpy(elfheader, vmlinuz_addr, sizeof(elfheader));
+ printf("No valid compressed data found, assume uncompressed
data\n\r");
+ }
if (!parse_elf64(elfheader, ) && !parse_elf32(elfheader, ))
fatal("Error: not a valid PPC32 or PPC64 ELF file!\n\r");
@@ -67,6 +74,13 @@ static struct addr_range prep_kernel(void)
"device tree\n\r");
}
+ if (uncompressed_image) {
+ memcpy(addr, vmlinuz_addr + ei.elfoffset, ei.loadsize);
+ printf("0x%lx bytes of uncompressed data copied\n\r",
+ ei.loadsize);
+ goto out;
+ }
+
/* Finally, decompress the kernel */
printf("Decompressing (0x%p <- 0x%p:0x%p)...\n\r", addr,
vmlinuz_addr, vmlinuz_addr+vmlinuz_size);
@@ -82,7 +96,7 @@ static struct addr_range prep_kernel(void)
len, ei.loadsize);
printf("Done! Decompressed 0x%lx bytes\n\r", len);
-
+out:
flush_cache(addr, ei.loadsize);
return (struct addr_range){addr, ei.memsize};
--
2.10.0
Re: Commit 1b7898ee276b "powerpc/boot: Use the pre-boot decompression API" breaks boot
Am 12.10.2016 um 06:26 schrieb Oliver O'Halloran: > On Tue, Oct 11, 2016 at 7:06 AM, Heiner Kallweit <hkallwe...@gmail.com> wrote: >>> IMHO in case of using cuboot no CONFIG_KERNEL_ config option >>> should be set and Makefile + code in arch/powerpc/boot should be able >>> to deal with this situation: >>> - don't copy and build the decompression stuff >>> - use an alternative version of prep_kernel() in main.c which doesn't >>> attempt to decompress the kernel image >>> >>> This should be a cleaner solution than probing the kernel image whether >>> it's compressed or not. >>> >> >> This would be the patch implementing the idea. Advantage is that all >> the unnecessary decompression code isn't built. Works fine for me. > > I don't think this approach is viable. The wrapper code is shared > among the various output image formats some of which *will* contain a > compressed kernel image so we can't simply remove the decompressor > from the wrapper. A random example I found in the makefile was > CONFIG_BAMBOO: > >> image-$(CONFIG_BAMBOO) += treeImage.bamboo cuImage.bamboo > > When building for this platform Kbuild will produce treeboot and a > cuboot image. Unlike uboot, Treeboot doesn't do any decompression so > the wrapper needs to decompress the kernel itself. The probing > solution more or less matches the old behaviour (which we know works) > so I think we should just stick with that. > > - Oliver > Indeed, I also figured that out later. As you say, then let's stick with re-introducing the probing. I'll send the patch for this. Heiner
Re: Commit 1b7898ee276b "powerpc/boot: Use the pre-boot decompression API" breaks boot
Am 10.10.2016 um 08:10 schrieb Heiner Kallweit:
> Am 10.10.2016 um 06:41 schrieb Michael Ellerman:
>> Heiner Kallweit <hkallwe...@gmail.com> writes:
>>
>>> Am 07.10.2016 um 21:26 schrieb Heiner Kallweit:
>>>> Am 07.10.2016 um 07:51 schrieb Oliver O'Halloran:
>>>>> Hi, Heiner
>>>>>
>>>>> Could you send me a copy of the kernel .config (or which defconfig)
>>>>> that you're using, the name of the HW platform that you're using and
>>>>> if possible the kernel image itself?
>>>>>
>>>>> Thanks,
>>>>> Oliver
>>>>>
>>>> Thanks for the quick reply. Attached are .config and cuImage.
>>>> HW is a TP-Link TL-WDR4900 WiFi router (P1014-based) running OpenWRT.
>>>>
>>> After further checking I think I found the issue. The old gunzip code
>>> handled uncompressed data transparently whilst the new one bails out
>>> if it doesn't find a proper gzip header.
>>> And in my case the actual kernel image is uncompressed.
>>> With the following patch the system boots fine again (at least for me).
>>
>> Thanks for testing and tracking it down.
>>
>> I wonder why the actual image is uncompressed? Or alternately why do we
>> tell uboot the image is compressed when it's not?
>>
> Uboot is provided with a compressed image, but what gets compressed is
> not the pure kernel image but the resulting image incl. boot wrapper code,
> see this part of the wrapper script:
>
> cuboot*)
> gzip -n -f -9 "$ofile"
> ${MKIMAGE} -A ppc -O linux -T kernel -C gzip -a "$base" -e "$entry" \
> $uboot_version -d "$ofile".gz "$ofile"
>
> And this resulting image is decompressed by uboot already during boot.
> Therefore the boot wrapper code sees an uncompressed kernel image.
>
> IMHO in case of using cuboot no CONFIG_KERNEL_ config option
> should be set and Makefile + code in arch/powerpc/boot should be able
> to deal with this situation:
> - don't copy and build the decompression stuff
> - use an alternative version of prep_kernel() in main.c which doesn't
> attempt to decompress the kernel image
>
> This should be a cleaner solution than probing the kernel image whether
> it's compressed or not.
>
This would be the patch implementing the idea. Advantage is that all
the unnecessary decompression code isn't built. Works fine for me.
---
arch/powerpc/boot/Makefile | 7 ++-
arch/powerpc/boot/main.c | 15 ---
2 files changed, 18 insertions(+), 4 deletions(-)
diff --git a/arch/powerpc/boot/Makefile b/arch/powerpc/boot/Makefile
index eae2dc8..1f18847 100644
--- a/arch/powerpc/boot/Makefile
+++ b/arch/powerpc/boot/Makefile
@@ -19,6 +19,7 @@
all: $(obj)/zImage
+compress-y := CONFIG_KERNEL_UNCOMPRESSED
compress-$(CONFIG_KERNEL_GZIP) := CONFIG_KERNEL_GZIP
compress-$(CONFIG_KERNEL_XZ) := CONFIG_KERNEL_XZ
@@ -95,12 +96,15 @@ libfdtheader := fdt.h libfdt.h libfdt_internal.h
$(addprefix $(obj)/,$(libfdt) libfdt-wrapper.o simpleboot.o epapr.o opal.o): \
$(addprefix $(obj)/,$(libfdtheader))
-src-wlib-y := string.S crt0.S crtsavres.S stdio.c decompress.c main.c \
+src-wlib-y := string.S crt0.S crtsavres.S stdio.c main.c \
$(libfdt) libfdt-wrapper.c \
ns16550.c serial.c simple_alloc.c div64.S util.S \
elf_util.c $(zlib-y) devtree.c stdlib.c \
oflib.c ofconsole.c cuboot.c mpsc.c cpm-serial.c \
uartlite.c mpc52xx-psc.c opal.c opal-calls.S
+ifneq ($(compress-y),CONFIG_KERNEL_UNCOMPRESSED)
+src-wlib-y += decompress.c
+endif
src-wlib-$(CONFIG_40x) += 4xx.c planetcore.c
src-wlib-$(CONFIG_44x) += 4xx.c ebony.c bamboo.c
src-wlib-$(CONFIG_8xx) += mpc8xx.c planetcore.c fsl-soc.c
@@ -226,6 +230,7 @@ CROSSWRAP := -C "$(CROSS_COMPILE)"
endif
endif
+compressor-y := none
compressor-$(CONFIG_KERNEL_GZIP) := gz
compressor-$(CONFIG_KERNEL_XZ) := xz
diff --git a/arch/powerpc/boot/main.c b/arch/powerpc/boot/main.c
index f7a184b..5a28c18 100644
--- a/arch/powerpc/boot/main.c
+++ b/arch/powerpc/boot/main.c
@@ -28,14 +28,19 @@ static struct addr_range prep_kernel(void)
{
char elfheader[256];
unsigned char *vmlinuz_addr = (unsigned char *)_vmlinux_start;
- unsigned long vmlinuz_size = _vmlinux_end - _vmlinux_start;
void *addr = 0;
struct elf_info ei;
+#ifndef CONFIG_KERNEL_UNCOMPRESSED
+ unsigned long vmlinuz_size = _vmlinux_end - _vmlinux_start;
long len;
+#endif
+#ifdef CONFIG_KERNEL_UNCOMPRESSED
+ memcpy(elfheader, vmlinuz_addr, sizeof(elfheader));
+#else
partial_decompress(vmlinuz_addr, vmlinuz_size,
Re: Commit 1b7898ee276b "powerpc/boot: Use the pre-boot decompression API" breaks boot
Am 10.10.2016 um 06:41 schrieb Michael Ellerman:
> Heiner Kallweit <hkallwe...@gmail.com> writes:
>
>> Am 07.10.2016 um 21:26 schrieb Heiner Kallweit:
>>> Am 07.10.2016 um 07:51 schrieb Oliver O'Halloran:
>>>> Hi, Heiner
>>>>
>>>> Could you send me a copy of the kernel .config (or which defconfig)
>>>> that you're using, the name of the HW platform that you're using and
>>>> if possible the kernel image itself?
>>>>
>>>> Thanks,
>>>> Oliver
>>>>
>>> Thanks for the quick reply. Attached are .config and cuImage.
>>> HW is a TP-Link TL-WDR4900 WiFi router (P1014-based) running OpenWRT.
>>>
>> After further checking I think I found the issue. The old gunzip code
>> handled uncompressed data transparently whilst the new one bails out
>> if it doesn't find a proper gzip header.
>> And in my case the actual kernel image is uncompressed.
>> With the following patch the system boots fine again (at least for me).
>
> Thanks for testing and tracking it down.
>
> I wonder why the actual image is uncompressed? Or alternately why do we
> tell uboot the image is compressed when it's not?
>
Uboot is provided with a compressed image, but what gets compressed is
not the pure kernel image but the resulting image incl. boot wrapper code,
see this part of the wrapper script:
cuboot*)
gzip -n -f -9 "$ofile"
${MKIMAGE} -A ppc -O linux -T kernel -C gzip -a "$base" -e "$entry" \
$uboot_version -d "$ofile".gz "$ofile"
And this resulting image is decompressed by uboot already during boot.
Therefore the boot wrapper code sees an uncompressed kernel image.
IMHO in case of using cuboot no CONFIG_KERNEL_ config option
should be set and Makefile + code in arch/powerpc/boot should be able
to deal with this situation:
- don't copy and build the decompression stuff
- use an alternative version of prep_kernel() in main.c which doesn't
attempt to decompress the kernel image
This should be a cleaner solution than probing the kernel image whether
it's compressed or not.
Rgds, Heiner
Re: Commit 1b7898ee276b "powerpc/boot: Use the pre-boot decompression API" breaks boot
Am 07.10.2016 um 21:26 schrieb Heiner Kallweit:
> Am 07.10.2016 um 07:51 schrieb Oliver O'Halloran:
>> Hi, Heiner
>>
>> Could you send me a copy of the kernel .config (or which defconfig)
>> that you're using, the name of the HW platform that you're using and
>> if possible the kernel image itself?
>>
>> Thanks,
>> Oliver
>>
> Thanks for the quick reply. Attached are .config and cuImage.
> HW is a TP-Link TL-WDR4900 WiFi router (P1014-based) running OpenWRT.
>
> Rgds, Heiner
>
After further checking I think I found the issue. The old gunzip code
handled uncompressed data transparently whilst the new one bails out
if it doesn't find a proper gzip header.
And in my case the actual kernel image is uncompressed.
With the following patch the system boots fine again (at least for me).
Rgds, Heiner
diff --git a/arch/powerpc/boot/main.c b/arch/powerpc/boot/main.c
index f7a184b..9796491 100644
--- a/arch/powerpc/boot/main.c
+++ b/arch/powerpc/boot/main.c
@@ -32,9 +32,16 @@ static struct addr_range prep_kernel(void)
void *addr = 0;
struct elf_info ei;
long len;
+ int uncompressed_image = 0;
- partial_decompress(vmlinuz_addr, vmlinuz_size,
+ len = partial_decompress(vmlinuz_addr, vmlinuz_size,
elfheader, sizeof(elfheader), 0);
+ /* assume uncompressed data if -1 is returned */
+ if (len == -1) {
+ uncompressed_image = 1;
+ memcpy(elfheader, vmlinuz_addr, sizeof(elfheader));
+ printf("No valid compressed data found, assume uncompressed
data\n\r");
+ }
if (!parse_elf64(elfheader, ) && !parse_elf32(elfheader, ))
fatal("Error: not a valid PPC32 or PPC64 ELF file!\n\r");
@@ -67,6 +74,12 @@ static struct addr_range prep_kernel(void)
"device tree\n\r");
}
+ if (uncompressed_image) {
+ memcpy(addr, vmlinuz_addr + ei.elfoffset, ei.loadsize);
+ printf("%ld bytes of uncompressed data copied\n\r",
ei.loadsize);
+ goto out;
+ }
+
/* Finally, decompress the kernel */
printf("Decompressing (0x%p <- 0x%p:0x%p)...\n\r", addr,
vmlinuz_addr, vmlinuz_addr+vmlinuz_size);
@@ -82,7 +95,7 @@ static struct addr_range prep_kernel(void)
len, ei.loadsize);
printf("Done! Decompressed 0x%lx bytes\n\r", len);
-
+out:
flush_cache(addr, ei.loadsize);
return (struct addr_range){addr, ei.memsize};
--
2.10.0
Commit 1b7898ee276b "powerpc/boot: Use the pre-boot decompression API" breaks boot
This commit results in boot stopping after the following messages. Any additional information I could provide? U-Boot 2010.12-svn19826 (Apr 24 2013 - 20:01:21) CPU: P1014, Version: 1.0, (0x80f10110) Core: E500, Version: 5.1, (0x80212151) Clock Configuration: CPU0:800 MHz, CCB:400 MHz, DDR:333.333 MHz (666.667 MT/s data rate) (Asynchronous), IFC:100 MHz L1:D-cache 32 kB enabled I-cache 32 kB enabled Board: P1014RDB SPI: ready DRAM: 128 MiB L2:256 KB enabled Using default environment PCIe1: Root Complex of mini PCIe Slot, x1, regs @ 0xffe0a000 01:00.0 - 168c:abcd - Network controller PCIe1: Bus 00 - 01 PCIe2: Root Complex of PCIe Slot, x1, regs @ 0xffe09000 03:00.0 - 168c:0033 - Network controller PCIe2: Bus 02 - 03 In:serial Out: serial Err: serial Net: initialization for Atheros AR8327/AR8328 eTSEC1 auto update firmware: is_auto_upload_firmware = 0! Autobooting in 1 seconds SF: Detected S25FL128S_64K with page size 256, total 16 MiB 16384 KiB S25FL128S_64K at 0:0 is now current device SPI flash read successful SPI flash read successful ## Booting kernel from Legacy Image at 0200 ... Image Name: Linux-4.9.0-rc0 Image Type: PowerPC Linux Kernel Image (gzip compressed) Data Size:2009549 Bytes = 1.9 MiB Load Address: 0100 Entry Point: 01000524 Verifying Checksum ... OK ## Flattened Device Tree blob at 0300 Booting using the fdt blob at 0x300 Uncompressing Kernel Image ... OK Loading Device Tree to 00ffa000, end 00ff ... OK
