Re: [U-Boot] [RFC PATCH 00/20] SPI-NAND support
+Richard to comment on the MTD abstraction stuff and how uboot port of UBI might be impacted by some changes requested here. Hi Jagan, On Mon, 25 Jun 2018 13:59:37 +0530 Jagan Teki wrote: > > I've looked the code on the respective patches, look like most of the > code copy from Linux by adding __UBOOT__. I have no issue with Linux > copy but we need to structure the code according to U-Boot in the form > of driver-model (this series lack with that). > > Here are my suggestions, based the MTD work so-far > > First we need to design MTD driver-model which can capable to drive > one driver from each interface. (not converting all interface drivers > at once, that is taking more time and other issues) > > Like Linux MTD, U-Boot should have MTD dm for underlying flash devices > like nand, parallel nor, spinor etc. So to drive this theory with > driver model(with an example of block layer) mtd is common device > interaction for most of memory technology flashes like nand, > parallel nor, spinor etc, these are treated as interface types wrt > u-boot driver model. > > Once the respective interface driver bind happen, the uclass driver > will pass an 'interface type' to mtd layer to create device for it, > for example once spinor ULASS_SPI_NOR driver bind happen, the uclass > driver of spinor will pass MTD_IF_TYPE_SPI_NOR > interface type to create mtd device for spinor devices. > > So If we add this design to SPI-NAND changes, we need to implement > - MTD dm core that can driver all interfaces That's already what the MTD framework provides, and Miquel even added some stuff to integrate the MTD layer even further in the DM. It's probably not perfect yet, but the changes are, IMHO, going in the right direction. Now, if you're talking about the new MTD API that creates helper functions prefixed with dm_, sorry, but I don't see the point. We already have plenty of MTD users in u-boot, they all manipulate MTD objects and go through the standard MTD API to do that. What you suggest would make things messier for several reasons: 1/ we won't be able to easily port Linux code to u-boot. Look at the JFFS2 UBI support. They all use mtd_info objects. What's the point of changing that except making things harder to port. 2/ Not all MTD providers will be converted to the device model at once, so how do you plan to deal with that? 3/ What's the benefit of exposing yet another way to manipulate MTD devices? > - one driver for raw nand Unfortunately, that's not how it works right now, and clearly, we don't have time to work on this raw NAND rework right now. > - one driver for spinand I think that's already the case. > - spi-mem It's also what Miquel is doing in this series. > - convert fsl-qspi to spi-mem We're not targeting the fsl-qspi controller here but a simple SPI controller that is already upstreamed. But yes, the fsl-qspi driver will have to be patched to support the spi-mem interface at some point. > - implement command to handle This I don't get. What do you mean by "implement command to handle"? Are we talking about cmd/mtd.c? I think the work Miquel has done is already a good start, what's missing in there? > > For spi-nor interface design, we have an example code here[2] > > I've paused this [2] series because of dm conversion of spi-drivers > otherwise I need add legacy code like mmc-legacy.c, so if we really > move to spi-mem design and okay with above design. I will try to move > the current spi flash to add MTD driver-model so-that we can add > spi-mem, spi-nand on top of it or we can work together to convert them > all. Why can't we do things iteratively. I mean, if the long term goal is to convert everything to the driver model, then this patchset is going in the right direction: - addition of DM helpers to the MTD_UCLASS - addition of the spi-mem interface properly integrated in the DM model of the SPI framework - addition of a SPI NAND driver, again properly integrated in the DM - integration of DM-ready MTD drivers and old MTD drivers in a single view exposed by the cmd/mtd.c command set I'd really like to limit the scope of this development to these topics, which doesn't prevent you from converting other part of u-boot to the spi-mem approach (SPI NOR is one example). I hope you understand our concerns and the fact that what you're asking us to do as a dependency of getting SPI NAND support + cmd/mtd.c merged is way more than we can actually provide. Regards, Boris ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH 1/3] spi: mxc: Fix compilation problem of DM_SPI class driver
On Thu, Jun 21, 2018 at 2:21 AM, Michael Trimarchi wrote: > drivers/spi/mxc_spi.c:507: undefined reference to `dev_get_addr' > linux-ld.bfd: BFD (GNU Binutils) 2.29.1 assertion fail elf32-arm.c:9509 > > Signed-off-by: Michael Trimarchi > --- Applied to u-boot-spi/master ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [UBOOT PATCH] spi: zynq_qspi: Fixed incorrect return value error
On Thu, Jun 14, 2018 at 12:30 PM, Michal Simek wrote: > On 14.6.2018 08:46, Vipul Kumar wrote: >> This patch replaced "return 0" with "return status" to fix the >> incorrect return value error reported by the coverity. >> >> Signed-off-by: Vipul Kumar >> --- >> drivers/spi/zynq_qspi.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/spi/zynq_qspi.c b/drivers/spi/zynq_qspi.c >> index ee8796d..9ad1927 100644 >> --- a/drivers/spi/zynq_qspi.c >> +++ b/drivers/spi/zynq_qspi.c >> @@ -486,7 +486,7 @@ static int zynq_qspi_transfer(struct zynq_qspi_priv >> *priv) >> break; >> } >> >> - return 0; >> + return status; >> } >> >> static int zynq_qspi_claim_bus(struct udevice *dev) >> > > Reviewed-by: Michal Simek Applied to u-boot-spi/master ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [RFC PATCH 00/20] SPI-NAND support
Miquel and Brois, thanks for working on this. On Thu, Jun 7, 2018 at 2:11 PM, Miquel Raynal wrote: > Hi Jagan, > > On Thu, 7 Jun 2018 11:21:22 +0530, Jagan Teki > wrote: > >> + Boris >> + Suneel (who helped in DM MTD) >> + Siva, Michal (zynq qspi) >> >> On Wed, Jun 6, 2018 at 9:00 PM, Miquel Raynal >> wrote: >> > During the last months, Boris Brezillon shared his work to support >> > serial flashes within Linux. First, he delivered (and merged) a new >> > layer called spi-mem. He also initiated in Linux MTD subsystem the move >> > of all 'raw' NAND related code to a raw/ subdirectory, adding at the >> > same time a NAND core that would be shared with all NAND devices. Then, >> > he contributed a generic SPI-NAND driver, making use of this NAND core, >> > as well as some vendor code to drive a few chips. >> >> 1) Can you pointed us the Linux changes along with discussion thread >> about spi-mem, and spi-nand. > > Sure, you can have a look there: > > SPI-mem: > http://lists.infradead.org/pipermail/linux-mtd/2018-April/080225.html > > SPI-NAND: > http://lists.infradead.org/pipermail/linux-mtd/2018-May/081005.html This is really nice, look like this design handling any kind of controller features by abstracting spi core so-that controller hacks shouldn't touch the common core code. unfortunately I missed Linux ML during submission. > >> >> 2) If my understanding was correct, spi-mem is replacement for spi-nor >> controller drivers from driver/mtd/spi-nor in Linux. > > It is not 'exactly' a replacement for spi-nor controller drivers but > that's the idea. I suggest you to have a look at Boris' blog post about > it: > https://bootlin.com/blog/spi-mem-bringing-some-consistency-to-the-spi-memory-ecosystem/ > >> >> 3) If so is fsl_qspi spi-nor driver moves to drivers/spi area? yes >> then how does flash changes handled by spi-mem. > > This series does not migrate the SPI-NOR stack to spi-mem. It focuses > on SPI-NAND for now. And I don't understand the second sentence. > >> >> 4) we have zynq qspi controller which has extensive features like dual >> flash(stacked and parallel) does spi-mem support these flash specific >> changes? > > This controller is very specific and such support has not yet been > added. > >> >> 5) Better to send spi-mem and spi-nand changes separately, for better >> reviewing. > > It would mean sending 3 or 4 distinct series, I thought better to give > the whole picture but that's your choice. > >> >> 6) We have DM MTD layer in ML, better to send the changes on-top [1] >> >> [1] https://patchwork.ozlabs.org/project/uboot/list/?series=20450 > > This is great to see such effort being made to develop U-Boot > driver-model but is there a real need for yet another layer on top of > the MTD stack? > > I'm looking at mtd-uclass.c for instance, I don't get the need for a > mtd_dread() function, all the operations in the mtd_d*() helpers are > already handled by mtd/mtdcore.c, no? > > And the mtd_ops structure does not bring a lot. Should not we keep it > simple and avoid such intermediate layer? > > Also, there is the introduction of a spinor command (what about the > existing 'sf'?), which is exactly the opposite of what the MTD > abstraction would told us to do (thus, the 'mtd' generic command). I've looked the code on the respective patches, look like most of the code copy from Linux by adding __UBOOT__. I have no issue with Linux copy but we need to structure the code according to U-Boot in the form of driver-model (this series lack with that). Here are my suggestions, based the MTD work so-far First we need to design MTD driver-model which can capable to drive one driver from each interface. (not converting all interface drivers at once, that is taking more time and other issues) Like Linux MTD, U-Boot should have MTD dm for underlying flash devices like nand, parallel nor, spinor etc. So to drive this theory with driver model(with an example of block layer) mtd is common device interaction for most of memory technology flashes like nand, parallel nor, spinor etc, these are treated as interface types wrt u-boot driver model. Once the respective interface driver bind happen, the uclass driver will pass an 'interface type' to mtd layer to create device for it, for example once spinor ULASS_SPI_NOR driver bind happen, the uclass driver of spinor will pass MTD_IF_TYPE_SPI_NOR interface type to create mtd device for spinor devices. So If we add this design to SPI-NAND changes, we need to implement - MTD dm core that can driver all interfaces - one driver for raw nand - one driver for spinand - spi-mem - convert fsl-qspi to spi-mem - implement command to handle For spi-nor interface design, we have an example code here[2] I've paused this [2] series because of dm conversion of spi-drivers otherwise I need add legacy code like mmc-legacy.c, so if we really move to spi-mem design and okay with above design. I will try to move the current spi flash to add MTD driver-model so-that we
Re: [U-Boot] [PATCH v2 1/2] common: command: Use command_ret_t enum values instead of values
On 22.6.2018 21:28, Simon Glass wrote:
> Hi Michal,
>
> On 22 June 2018 at 00:33, Michal Simek wrote:
>>
>> On 21.6.2018 21:45, Simon Glass wrote:
>>> On 21 June 2018 at 06:58, Michal Simek wrote:
Use enum command_ret_t types in cmd_process_error().
Signed-off-by: Michal Simek
---
Changes in v2:
- Move adding RET_USAGE to separate patch.
common/command.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> Reviewed-by: Simon Glass
>>>
>>>
diff --git a/common/command.c b/common/command.c
index 52d47c133c3c..a4a8dc601acb 100644
--- a/common/command.c
+++ b/common/command.c
@@ -549,8 +549,8 @@ int cmd_process_error(cmd_tbl_t *cmdtp, int err)
{
if (err) {
printf("Command '%s' failed: Error %d\n", cmdtp->name,
err);
- return 1;
+ return CMD_RET_FAILURE;
}
- return 0;
+ return CMD_RET_SUCCESS;
>>>
>>> I actually thing 0 is fine here. That is the definition of success.
>>
>> and CMD_RET_SUCCESS has this 0 value too.
>>
>> maybe would be worth to also change return type to enum command_ret_t
>> as is done for cmd_process.
>>
>> For example ubi_remove_vol() in case of failure returs +ENODEV and others.
>> I thought that commands could return only 3 values convert by enum
>> command_ret_t. Or is it ok to return also different values?
>
> Commands should only return things from the enum. My point was that I
> find 'return CMD_RET_SUCCESS' to be a bit painful. We know the value
> is 0 and it is much shorter to read, so I prefer 'return 0' instead of
> 'return CMD_RET_SUCCESS'
>
> Also I like this:
>
> if (!xx)
>// we got an error
>
> and don't like this:
>
> if (xx != CMD_RET_SUCCESS)
>// we got an error
ok. Got what you meant.
Thanks,
Michal
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [PATCH v3 1/4] lib: fdtdec: Add new variable ram_start to global data
Hi, On 22.6.2018 21:28, Simon Glass wrote: > Hi, > > On 22 June 2018 at 01:41, Michal Simek wrote: >> Hi Simon, >> >> On 18.6.2018 08:18, Siva Durga Prasad Paladugu wrote: >>> Added new variable ram_start to global data structure for holding >>> the start address of first bank of RAM, and then use this ram_start >>> for calculating ram_top properly. This patch fixes the erroneous >>> calculation of ram_top incase of non zero ram start address. >>> This patch also renames fdtdec_setup_memory_size() to >>> fdtdec_setup_mem_size_start() as this routine now takes care >>> of memory size and start. >>> >>> Signed-off-by: Siva Durga Prasad Paladugu >>> Signed-off-by: Michal Simek >>> --- >>> Changes from v2: >>> - Used new varibale ram_start >>> - Rename fdtdec_setup_memory_size >>> >>> Changes from v1: >>> - None >>> --- >>> arch/arm/mach-mvebu/arm64-common.c | 2 +- >>> board/emulation/qemu-arm/qemu-arm.c | 2 +- >>> board/renesas/alt/alt.c | 2 +- >>> board/renesas/blanche/blanche.c | 2 +- >>> board/renesas/draak/draak.c | 2 +- >>> board/renesas/eagle/eagle.c | 2 +- >>> board/renesas/gose/gose.c| 2 +- >>> board/renesas/koelsch/koelsch.c | 2 +- >>> board/renesas/lager/lager.c | 2 +- >>> board/renesas/porter/porter.c| 2 +- >>> board/renesas/salvator-x/salvator-x.c| 2 +- >>> board/renesas/silk/silk.c| 2 +- >>> board/renesas/stout/stout.c | 2 +- >>> board/renesas/ulcb/ulcb.c| 2 +- >>> board/st/stm32f429-discovery/stm32f429-discovery.c | 2 +- >>> board/st/stm32f429-evaluation/stm32f429-evaluation.c | 2 +- >>> board/st/stm32f469-discovery/stm32f469-discovery.c | 2 +- >>> board/st/stm32h743-disco/stm32h743-disco.c | 2 +- >>> board/st/stm32h743-eval/stm32h743-eval.c | 2 +- >>> board/xilinx/zynq/board.c| 2 +- >>> board/xilinx/zynqmp/zynqmp.c | 2 +- >>> board/xilinx/zynqmp_r5/board.c | 2 +- >>> common/board_f.c | 4 ++-- >>> include/asm-generic/global_data.h| 1 + >>> include/fdtdec.h | 16 >>> +--- >>> lib/fdtdec.c | 3 ++- >>> tools/patman/func_test.py| 2 +- >>> tools/patman/test/-cover-letter.patch| 2 +- >>> ...orrect-cast-for-sandbox-in-fdtdec_setup_memory_.patch | 4 ++-- >>> tools/patman/test/test01.txt | 2 +- >>> 30 files changed, 41 insertions(+), 37 deletions(-) > > [...] > >> sjg: Do you see any issue with this patch? > > I think it is OK. Would like to get more people to look at it though. > >> >> I can't see the reason for fixing tools/patman but I will wait for you. > > Changing patman tests is OK with me. Keeps thing consistent. thanks Simon. Tom: Do you see any issue with this change? Marek: Can you please retest renesas boards that this patch is not breaking your boards? Thanks, Michal ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
