Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757678AbbDVUvs (ORCPT ); Wed, 22 Apr 2015 16:51:48 -0400 Received: from arrakis.dune.hu ([78.24.191.176]:43343 "EHLO arrakis.dune.hu" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752314AbbDVUvq (ORCPT ); Wed, 22 Apr 2015 16:51:46 -0400 MIME-Version: 1.0 In-Reply-To: <20150422141721.D3F8C1A242E@localhost.localdomain> References: <20150422141721.D3F8C1A242E@localhost.localdomain> From: Jonas Gorski Date: Wed, 22 Apr 2015 22:50:56 +0200 Message-ID: Subject: Re: [PATCH v2] spi: fsl-spi: use of_iomap() to map parameter ram on CPM1 To: Christophe Leroy Cc: Mark Brown , "linux-kernel@vger.kernel.org" , linuxppc-dev@lists.ozlabs.org, linux-spi@vger.kernel.org Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4796 Lines: 128 Hi (again), as usual you only see issues *after* sending the email ... On Wed, Apr 22, 2015 at 4:17 PM, Christophe Leroy wrote: > On CPM2, the SPI parameter RAM is dynamically allocated in the > dualport RAM whereas in CPM1, it is statically allocated to a default > address with capability to relocate it somewhere else via the use of > CPM micropatch. The address of the parameter RAM is given by the boot > loader and expected to be mapped via of_iomap() > > In the current implementation, in function fsl_spi_cpm_get_pram() > there is a confusion between the SPI_BASE register and the base of the > SPI parameter RAM. Fortunatly, it is working properly with MPC866 and > MPC885 because they do set SPI_BASE, but on MPC860 and other old > MPC8xx that doesn't set SPI_BASE, pram_ofs is not properly set. > Also, the parameter RAM is not properly mapped with of_iomap() as it > should but still gets accessible by chance through the full RAM which > is mapped from somewhere else. > > This patch applies to the SPI driver the same principle as for the > CPM UART: when the CPM is of type CPM1, we simply do an of_iomap() of > the area provided via the device tree. > > Signed-off-by: Christophe Leroy > > --- > v2: Use devm_ioremap_resource() instead of_iomap() > > drivers/spi/spi-fsl-cpm.c | 35 ++++++++++++++++++----------------- > 1 file changed, 18 insertions(+), 17 deletions(-) > > diff --git a/drivers/spi/spi-fsl-cpm.c b/drivers/spi/spi-fsl-cpm.c > index e85ab1c..4e5c945 100644 > --- a/drivers/spi/spi-fsl-cpm.c > +++ b/drivers/spi/spi-fsl-cpm.c > @@ -23,6 +23,7 @@ > #include > #include > #include > +#include > > #include "spi-fsl-cpm.h" > #include "spi-fsl-lib.h" > @@ -264,17 +265,6 @@ static unsigned long fsl_spi_cpm_get_pram(struct mpc8xxx_spi *mspi) > if (mspi->flags & SPI_CPM2) { > pram_ofs = cpm_muram_alloc(SPI_PRAM_SIZE, 64); > out_be16(spi_base, pram_ofs); > - } else { > - struct spi_pram __iomem *pram = spi_base; > - u16 rpbase = in_be16(&pram->rpbase); > - > - /* Microcode relocation patch applied? */ > - if (rpbase) { > - pram_ofs = rpbase; > - } else { > - pram_ofs = cpm_muram_alloc(SPI_PRAM_SIZE, 64); > - out_be16(spi_base, pram_ofs); > - } > } > > iounmap(spi_base); > @@ -287,7 +277,6 @@ int fsl_spi_cpm_init(struct mpc8xxx_spi *mspi) > struct device_node *np = dev->of_node; > const u32 *iprop; > int size; > - unsigned long pram_ofs; > unsigned long bds_ofs; > > if (!(mspi->flags & SPI_CPM_MODE)) > @@ -314,8 +303,21 @@ int fsl_spi_cpm_init(struct mpc8xxx_spi *mspi) > } > } > > - pram_ofs = fsl_spi_cpm_get_pram(mspi); > - if (IS_ERR_VALUE(pram_ofs)) { > + if (mspi->flags & SPI_CPM1) { > + struct resource *res; > + > + res = platform_get_resource(to_platform_device(dev), > + IORESOURCE_MEM, 1); > + mspi->pram = devm_ioremap_resource(dev, res); > + } else { > + unsigned long pram_ofs = fsl_spi_cpm_get_pram(mspi); > + > + if (IS_ERR_VALUE(pram_ofs)) > + mspi->pram = NULL; > + else > + mspi->pram = cpm_muram_addr(pram_ofs); > + } > + if (mspi->pram == NULL) { devm_ioremap_resource will never return NULL; you either need to check with IS_ERR(), or do that after calling devm_ioremap_resource and setg pram to NULL in that case. > dev_err(dev, "can't allocate spi parameter ram\n"); > goto err_pram; > } > @@ -341,8 +343,6 @@ int fsl_spi_cpm_init(struct mpc8xxx_spi *mspi) > goto err_dummy_rx; > } > > - mspi->pram = cpm_muram_addr(pram_ofs); > - > mspi->tx_bd = cpm_muram_addr(bds_ofs); > mspi->rx_bd = cpm_muram_addr(bds_ofs + sizeof(*mspi->tx_bd)); > > @@ -370,7 +370,8 @@ err_dummy_rx: > err_dummy_tx: > cpm_muram_free(bds_ofs); > err_bds: > - cpm_muram_free(pram_ofs); > + if (!(mspi->flags & SPI_CPM1)) > + cpm_muram_free(cpm_muram_offset(mspi->pram)); > err_pram: > fsl_spi_free_dummy_rx(); > return -ENOMEM; Regards Jonas -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/