| Message ID | 3e4cf141-52a3-ef88-5e25-eb5c75b16186@cogentembedded.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Vignesh R |
| Headers | show |
| Series | [v2] mtd: hyperbus: add Renesas RPC-IF driver | expand |
On 13.05.2020 23:43, Sergei Shtylyov wrote: > Add the HyperFLash driver for the Renesas RPC-IF. It's the "front end" > driver using the "back end" APIs in the main driver to talk to the real > hardware. > > Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > > --- > This patch is against the 'mtd/next' branch of the MTD 'linux.git' repo. > Requires the RPC-IF main driver patch in order to build/work: > > [1] https://patchwork.kernel.org/patch/11283127/ Sorry, this was an old version of that driver. here's the new one: https://patchwork.kernel.org/patch/11521597/ [...] MBR, Sergei
Hello! On 05/13/2020 11:43 PM, Sergei Shtylyov wrote: > Add the HyperFLash driver for the Renesas RPC-IF. It's the "front end" > driver using the "back end" APIs in the main driver to talk to the real > hardware. > > Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > > --- > This patch is against the 'mtd/next' branch of the MTD 'linux.git' repo. > Requires the RPC-IF main driver patch in order to build/work: > > [1] https://patchwork.kernel.org/patch/11283127/ That patch has hit Linus' repo in the 5.8 timeframe, so it's high time that this driver is merged as well. MBR, Sergei
Hi Sergei, On 5/14/20 2:13 AM, Sergei Shtylyov wrote: > Add the HyperFLash driver for the Renesas RPC-IF. It's the "front end" > driver using the "back end" APIs in the main driver to talk to the real > hardware. > > Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > Mails to this ID is bouncing which meant I could not ask status of dependencies. > --- > This patch is against the 'mtd/next' branch of the MTD 'linux.git' repo. > Requires the RPC-IF main driver patch in order to build/work: > > [1] https://patchwork.kernel.org/patch/11283127/ > [...] > --- /dev/null > +++ linux/drivers/mtd/hyperbus/rpc-if.c > @@ -0,0 +1,165 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Linux driver for RPC-IF HyperFlash > + * > + * Copyright (C) 2019-2020 Cogent Embedded, Inc. > + */ > + > +#include <linux/err.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/mtd/hyperbus.h> > +#include <linux/mtd/mtd.h> > +#include <linux/mux/consumer.h> > +#include <linux/of.h> > +#include <linux/platform_device.h> > +#include <linux/types.h> > + > +#include <memory/renesas-rpc-if.h> > + > +/* FIXME: How to drop this? */ > +#ifndef CONFIG_MTD_CFI_BE_BYTE_SWAP > +#error Enable config "Flash cmd/query data swapping (BIG_ENDIAN_BYTE)" > +#endif select MTD_CFI_BE_BYTE_SWAP in Kconfig does not help? > + > + struct rpcif_hyperbus { WARNING: please, no spaces at the start of a line #73: FILE: drivers/mtd/hyperbus/rpc-if.c:25: + struct^Irpcif_hyperbus {$ > + struct rpcif rpc; > + struct hyperbus_ctlr ctlr; > + struct hyperbus_device hbdev; > +}; > + > +static const struct rpcif_op rpcif_op_tmpl = { > + .cmd = { > + .buswidth = 8, > + .ddr = true, > + }, > + .ocmd = { > + .buswidth = 8, > + .ddr = true, > + }, > + .addr = { > + .nbytes = 1, > + .buswidth = 8, > + .ddr = true, > + }, > + .data = { > + .buswidth = 8, > + .ddr = true, > + }, > +}; > + > +static u16 rpcif_hb_read16(struct hyperbus_device *hbdev, unsigned long addr) > +{ > + struct rpcif_hyperbus *hyperbus = > + container_of(hbdev, struct rpcif_hyperbus, hbdev); > + struct rpcif_op op = rpcif_op_tmpl; > + map_word data; > + > + op.cmd.opcode = 0x80; Here and elsewhere what does 0x80 represent? Maybe using a self describing macro would be helpful when assigning values to op.cmd.opcode? > + op.addr.val = addr >> 1; > + op.dummy.buswidth = 1; > + op.dummy.ncycles = 15; > + op.data.dir = RPCIF_DATA_IN; > + op.data.nbytes = 2; > + op.data.buf.in = &data; This code block seems like a good candidate to be converted to a macro/ template to avoid repetition. Template can initializes few values remain common across the driver and override others based on parameters passed. You could probably have one for write op and one for read op, if needed. [...] Regards Vignesh
On 14.09.2020 15:09, Vignesh Raghavendra wrote: > Hi Sergei, > > On 5/14/20 2:13 AM, Sergei Shtylyov wrote: >> Add the HyperFLash driver for the Renesas RPC-IF. It's the "front end" >> driver using the "back end" APIs in the main driver to talk to the real >> hardware. >> >> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> >> > > Mails to this ID is bouncing which meant I could not ask status of > dependencies. > >> --- >> This patch is against the 'mtd/next' branch of the MTD 'linux.git' repo. >> Requires the RPC-IF main driver patch in order to build/work: >> >> [1] https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.kernel.org%2Fpatch%2F11283127%2F&data=02%7C01%7Cdirk.behme%40de.bosch.com%7C67d24357a6644232518008d858af6cd0%7C0ae51e1907c84e4bbb6d648ee58410f4%7C0%7C0%7C637356857751715684&sdata=XDR3Nu15LcC3G5C4HZgb9UH8eNGWDpuxupf6zoXcM%2Fc%3D&reserved=0 >> > [...] >> --- /dev/null >> +++ linux/drivers/mtd/hyperbus/rpc-if.c >> @@ -0,0 +1,165 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Linux driver for RPC-IF HyperFlash >> + * >> + * Copyright (C) 2019-2020 Cogent Embedded, Inc. >> + */ >> + >> +#include <linux/err.h> >> +#include <linux/kernel.h> >> +#include <linux/module.h> >> +#include <linux/mtd/hyperbus.h> >> +#include <linux/mtd/mtd.h> >> +#include <linux/mux/consumer.h> >> +#include <linux/of.h> >> +#include <linux/platform_device.h> >> +#include <linux/types.h> >> + >> +#include <memory/renesas-rpc-if.h> >> + >> +/* FIXME: How to drop this? */ >> +#ifndef CONFIG_MTD_CFI_BE_BYTE_SWAP >> +#error Enable config "Flash cmd/query data swapping (BIG_ENDIAN_BYTE)" >> +#endif > > select MTD_CFI_BE_BYTE_SWAP in Kconfig does not help? If I remember correctly 'select MTD_CFI_BE_BYTE_SWAP' in Kconfig doesn't seem to work. Therefore the FIXME. I can't remember exactly the root cause for that any more, but maybe it was the 'bool' type of MTD_CFI_BE_BYTE_SWAP? If anybody has a nice solution for that, it would be welcome :) Best regards Dirk >> + >> + struct rpcif_hyperbus { > > WARNING: please, no spaces at the start of a line > #73: FILE: drivers/mtd/hyperbus/rpc-if.c:25: > + struct^Irpcif_hyperbus {$ > > > >> + struct rpcif rpc; >> + struct hyperbus_ctlr ctlr; >> + struct hyperbus_device hbdev; >> +}; >> + >> +static const struct rpcif_op rpcif_op_tmpl = { >> + .cmd = { >> + .buswidth = 8, >> + .ddr = true, >> + }, >> + .ocmd = { >> + .buswidth = 8, >> + .ddr = true, >> + }, >> + .addr = { >> + .nbytes = 1, >> + .buswidth = 8, >> + .ddr = true, >> + }, >> + .data = { >> + .buswidth = 8, >> + .ddr = true, >> + }, >> +}; >> + >> +static u16 rpcif_hb_read16(struct hyperbus_device *hbdev, unsigned long addr) >> +{ >> + struct rpcif_hyperbus *hyperbus = >> + container_of(hbdev, struct rpcif_hyperbus, hbdev); >> + struct rpcif_op op = rpcif_op_tmpl; >> + map_word data; >> + >> + op.cmd.opcode = 0x80; > > Here and elsewhere what does 0x80 represent? Maybe using a self > describing macro would be helpful when assigning values to op.cmd.opcode? > >> + op.addr.val = addr >> 1; >> + op.dummy.buswidth = 1; >> + op.dummy.ncycles = 15; >> + op.data.dir = RPCIF_DATA_IN; >> + op.data.nbytes = 2; >> + op.data.buf.in = &data; > > This code block seems like a good candidate to be converted to a macro/ > template to avoid repetition. Template can initializes few values remain > common across the driver and override others based on parameters passed. > You could probably have one for write op and one for read op, if needed. > > [...] > > Regards > Vignesh >
On 9/15/20 9:37 AM, Behme Dirk (CM/ESO2) wrote: > > > On 14.09.2020 15:09, Vignesh Raghavendra wrote: >> Hi Sergei, >> >> On 5/14/20 2:13 AM, Sergei Shtylyov wrote: >>> Add the HyperFLash driver for the Renesas RPC-IF. It's the "front end" >>> driver using the "back end" APIs in the main driver to talk to the real >>> hardware. >>> >>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> [...] >>> >>> >> [...] >>> --- /dev/null >>> +++ linux/drivers/mtd/hyperbus/rpc-if.c >>> @@ -0,0 +1,165 @@ >>> +// SPDX-License-Identifier: GPL-2.0 >>> +/* >>> + * Linux driver for RPC-IF HyperFlash >>> + * >>> + * Copyright (C) 2019-2020 Cogent Embedded, Inc. >>> + */ >>> + >>> +#include <linux/err.h> >>> +#include <linux/kernel.h> >>> +#include <linux/module.h> >>> +#include <linux/mtd/hyperbus.h> >>> +#include <linux/mtd/mtd.h> >>> +#include <linux/mux/consumer.h> >>> +#include <linux/of.h> >>> +#include <linux/platform_device.h> >>> +#include <linux/types.h> >>> + >>> +#include <memory/renesas-rpc-if.h> >>> + >>> +/* FIXME: How to drop this? */ >>> +#ifndef CONFIG_MTD_CFI_BE_BYTE_SWAP >>> +#error Enable config "Flash cmd/query data swapping (BIG_ENDIAN_BYTE)" >>> +#endif >> >> select MTD_CFI_BE_BYTE_SWAP in Kconfig does not help? > > > If I remember correctly 'select MTD_CFI_BE_BYTE_SWAP' in Kconfig doesn't > seem to work. Therefore the FIXME. I can't remember exactly the root > cause for that any more, but maybe it was the 'bool' type of > MTD_CFI_BE_BYTE_SWAP? > Ah, sorry MTD_CFI_BE_BYTE_SWAP is a choice and select does not work on choice menus. how about: > If anybody has a nice solution for that, it would be welcome :) Does this work: config RPCIF_HYPERBUS tristate "Renesas RPC-IF HyperBus driver" depends on RENESAS_RPCIF - select MTD_CFI_ADV_OPTIONS + depends on MTD_CFI_BE_BYTE_SWAP || COMPILE_TEST help This option includes Renesas RPC-IF HyperFlash support. Regards Vignesh [...]
On 15.09.2020 07:27, Vignesh Raghavendra wrote: > > > On 9/15/20 9:37 AM, Behme Dirk (CM/ESO2) wrote: >> >> >> On 14.09.2020 15:09, Vignesh Raghavendra wrote: >>> Hi Sergei, >>> >>> On 5/14/20 2:13 AM, Sergei Shtylyov wrote: >>>> Add the HyperFLash driver for the Renesas RPC-IF. It's the "front end" >>>> driver using the "back end" APIs in the main driver to talk to the real >>>> hardware. >>>> >>>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > [...] >>>> >>>> >>> [...] >>>> --- /dev/null >>>> +++ linux/drivers/mtd/hyperbus/rpc-if.c >>>> @@ -0,0 +1,165 @@ >>>> +// SPDX-License-Identifier: GPL-2.0 >>>> +/* >>>> + * Linux driver for RPC-IF HyperFlash >>>> + * >>>> + * Copyright (C) 2019-2020 Cogent Embedded, Inc. >>>> + */ >>>> + >>>> +#include <linux/err.h> >>>> +#include <linux/kernel.h> >>>> +#include <linux/module.h> >>>> +#include <linux/mtd/hyperbus.h> >>>> +#include <linux/mtd/mtd.h> >>>> +#include <linux/mux/consumer.h> >>>> +#include <linux/of.h> >>>> +#include <linux/platform_device.h> >>>> +#include <linux/types.h> >>>> + >>>> +#include <memory/renesas-rpc-if.h> >>>> + >>>> +/* FIXME: How to drop this? */ >>>> +#ifndef CONFIG_MTD_CFI_BE_BYTE_SWAP >>>> +#error Enable config "Flash cmd/query data swapping (BIG_ENDIAN_BYTE)" >>>> +#endif >>> >>> select MTD_CFI_BE_BYTE_SWAP in Kconfig does not help? >> >> >> If I remember correctly 'select MTD_CFI_BE_BYTE_SWAP' in Kconfig doesn't >> seem to work. Therefore the FIXME. I can't remember exactly the root >> cause for that any more, but maybe it was the 'bool' type of >> MTD_CFI_BE_BYTE_SWAP? >> > > Ah, sorry MTD_CFI_BE_BYTE_SWAP is a choice and select does not work on > choice menus. how about: > >> If anybody has a nice solution for that, it would be welcome :) > > Does this work: > > config RPCIF_HYPERBUS > tristate "Renesas RPC-IF HyperBus driver" > depends on RENESAS_RPCIF > - select MTD_CFI_ADV_OPTIONS > + depends on MTD_CFI_BE_BYTE_SWAP || COMPILE_TEST > help > This option includes Renesas RPC-IF HyperFlash support. Well, it at least makes the whole RPCIF_HYPERBUS option to vanish in case the (quite uncommon?) MTD_CFI_BE_BYTE_SWAP is not set, no? Or rephrased: How to make the potential user aware that MTD_CFI_BE_BYTE_SWAP needs to be enabled to make this option at least visible? From that point of view I feel the #error as given above more user friendly: RPCIF_HYPERBUS is there an can be enabled, but compiling is failing with a reasonable message if the dependencies are not met. Best regards Dirk
On 9/15/20 3:07 PM, Behme Dirk (CM/ESO2) wrote: > > > On 15.09.2020 07:27, Vignesh Raghavendra wrote: >> >> >> On 9/15/20 9:37 AM, Behme Dirk (CM/ESO2) wrote: >>> >>> >>> On 14.09.2020 15:09, Vignesh Raghavendra wrote: >>>> Hi Sergei, >>>> >>>> On 5/14/20 2:13 AM, Sergei Shtylyov wrote: >>>>> Add the HyperFLash driver for the Renesas RPC-IF. It's the "front >>>>> end" >>>>> driver using the "back end" APIs in the main driver to talk to the >>>>> real >>>>> hardware. >>>>> >>>>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> >> [...] >>>>> >>>>> >>>> [...] >>>>> --- /dev/null >>>>> +++ linux/drivers/mtd/hyperbus/rpc-if.c >>>>> @@ -0,0 +1,165 @@ >>>>> +// SPDX-License-Identifier: GPL-2.0 >>>>> +/* >>>>> + * Linux driver for RPC-IF HyperFlash >>>>> + * >>>>> + * Copyright (C) 2019-2020 Cogent Embedded, Inc. >>>>> + */ >>>>> + >>>>> +#include <linux/err.h> >>>>> +#include <linux/kernel.h> >>>>> +#include <linux/module.h> >>>>> +#include <linux/mtd/hyperbus.h> >>>>> +#include <linux/mtd/mtd.h> >>>>> +#include <linux/mux/consumer.h> >>>>> +#include <linux/of.h> >>>>> +#include <linux/platform_device.h> >>>>> +#include <linux/types.h> >>>>> + >>>>> +#include <memory/renesas-rpc-if.h> >>>>> + >>>>> +/* FIXME: How to drop this? */ >>>>> +#ifndef CONFIG_MTD_CFI_BE_BYTE_SWAP >>>>> +#error Enable config "Flash cmd/query data swapping >>>>> (BIG_ENDIAN_BYTE)" >>>>> +#endif >>>> >>>> select MTD_CFI_BE_BYTE_SWAP in Kconfig does not help? >>> >>> >>> If I remember correctly 'select MTD_CFI_BE_BYTE_SWAP' in Kconfig doesn't >>> seem to work. Therefore the FIXME. I can't remember exactly the root >>> cause for that any more, but maybe it was the 'bool' type of >>> MTD_CFI_BE_BYTE_SWAP? >>> >> >> Ah, sorry MTD_CFI_BE_BYTE_SWAP is a choice and select does not work on >> choice menus. how about: >> >>> If anybody has a nice solution for that, it would be welcome :) >> >> Does this work: >> >> config RPCIF_HYPERBUS >> tristate "Renesas RPC-IF HyperBus driver" >> depends on RENESAS_RPCIF >> - select MTD_CFI_ADV_OPTIONS >> + depends on MTD_CFI_BE_BYTE_SWAP || COMPILE_TEST >> help >> This option includes Renesas RPC-IF HyperFlash support. > > > Well, it at least makes the whole RPCIF_HYPERBUS option to vanish in > case the (quite uncommon?) MTD_CFI_BE_BYTE_SWAP is not set, no? Or > rephrased: How to make the potential user aware that > MTD_CFI_BE_BYTE_SWAP needs to be enabled to make this option at least > visible? User can search for RPCIF_HYPERBUS symbol (say in menuconfig) and dependencies will show up. > From that point of view I feel the #error as given above more > user friendly: RPCIF_HYPERBUS is there an can be enabled, but compiling > is failing with a reasonable message if the dependencies are not met. > No, this is uncommon.. If a driver needs a symbol to be enabled for it to work, then Kconfig entry should indicate so with "depends on" vs using "#error" clause. Using "select" to force enable all dependencies is not recommended either as it does not take care of dependencies of the symbol being selected. "select" is generally is used for symbols that are not user selectable (in menuconfig). Drivers along with required dependencies for a platform can be added to appropriate defconfig (See arch/*/configs/) to make end users job easy Regards Vignesh
Hello! On 09/14/2020 04:09 PM, Vignesh Raghavendra wrote: >> Add the HyperFLash driver for the Renesas RPC-IF. It's the "front end" >> driver using the "back end" APIs in the main driver to talk to the real >> hardware. >> >> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > > Mails to this ID is bouncing which meant I could not ask status of > dependencies. I no longer work for Cogent Embedded (this also means that I've lost the hardware access). The mail kept working for several months after my layoff though... >> --- >> This patch is against the 'mtd/next' branch of the MTD 'linux.git' repo. >> Requires the RPC-IF main driver patch in order to build/work: >> >> [1] https://patchwork.kernel.org/patch/11283127/ >> > [...] >> --- /dev/null >> +++ linux/drivers/mtd/hyperbus/rpc-if.c >> @@ -0,0 +1,165 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Linux driver for RPC-IF HyperFlash >> + * >> + * Copyright (C) 2019-2020 Cogent Embedded, Inc. >> + */ >> + >> +#include <linux/err.h> >> +#include <linux/kernel.h> >> +#include <linux/module.h> >> +#include <linux/mtd/hyperbus.h> >> +#include <linux/mtd/mtd.h> >> +#include <linux/mux/consumer.h> >> +#include <linux/of.h> >> +#include <linux/platform_device.h> >> +#include <linux/types.h> >> + >> +#include <memory/renesas-rpc-if.h> >> + >> +/* FIXME: How to drop this? */ >> +#ifndef CONFIG_MTD_CFI_BE_BYTE_SWAP >> +#error Enable config "Flash cmd/query data swapping (BIG_ENDIAN_BYTE)" >> +#endif > > select MTD_CFI_BE_BYTE_SWAP in Kconfig does not help? Alas, it doesn't work! >> + >> + struct rpcif_hyperbus { > > WARNING: please, no spaces at the start of a line > #73: FILE: drivers/mtd/hyperbus/rpc-if.c:25: > + struct^Irpcif_hyperbus {$ Ugh, must be a stray space... :-/ [...] >> +static const struct rpcif_op rpcif_op_tmpl = { >> + .cmd = { >> + .buswidth = 8, >> + .ddr = true, >> + }, >> + .ocmd = { >> + .buswidth = 8, >> + .ddr = true, >> + }, >> + .addr = { >> + .nbytes = 1, >> + .buswidth = 8, >> + .ddr = true, >> + }, >> + .data = { >> + .buswidth = 8, >> + .ddr = true, >> + }, >> +}; >> + >> +static u16 rpcif_hb_read16(struct hyperbus_device *hbdev, unsigned long addr) >> +{ >> + struct rpcif_hyperbus *hyperbus = >> + container_of(hbdev, struct rpcif_hyperbus, hbdev); >> + struct rpcif_op op = rpcif_op_tmpl; >> + map_word data; >> + >> + op.cmd.opcode = 0x80; > > Here and elsewhere what does 0x80 represent? This is bits 40..47 of the HyperBus command/address packet: thus 0x80 means memory read. > Maybe using a self > describing macro would be helpful when assigning values to op.cmd.opcode? It probably would but it apparently needs to be placed in your HyperBus header... >> + op.addr.val = addr >> 1; >> + op.dummy.buswidth = 1; >> + op.dummy.ncycles = 15; >> + op.data.dir = RPCIF_DATA_IN; >> + op.data.nbytes = 2; >> + op.data.buf.in = &data; > > This code block seems like a good candidate to be converted to a macro/ > template to avoid repetition. Template can initializes few values remain I'm already using a common template... > common across the driver and override others based on parameters passed. > You could probably have one for write op and one for read op, if needed. OK, I'll look into doing that... > [...] > > Regards > Vignesh MBR, Sergei
On 9/19/20 10:07 PM, Sergei Shtylyov wrote: > Hello! > > On 09/14/2020 04:09 PM, Vignesh Raghavendra wrote: > >>> Add the HyperFLash driver for the Renesas RPC-IF. It's the "front end" >>> driver using the "back end" APIs in the main driver to talk to the real >>> hardware. >>> >>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> >> [...] >>> +#include <linux/err.h> >>> +#include <linux/kernel.h> >>> +#include <linux/module.h> >>> +#include <linux/mtd/hyperbus.h> >>> +#include <linux/mtd/mtd.h> >>> +#include <linux/mux/consumer.h> >>> +#include <linux/of.h> >>> +#include <linux/platform_device.h> >>> +#include <linux/types.h> >>> + >>> +#include <memory/renesas-rpc-if.h> >>> + >>> +/* FIXME: How to drop this? */ >>> +#ifndef CONFIG_MTD_CFI_BE_BYTE_SWAP >>> +#error Enable config "Flash cmd/query data swapping (BIG_ENDIAN_BYTE)" >>> +#endif >> >> select MTD_CFI_BE_BYTE_SWAP in Kconfig does not help? > > Alas, it doesn't work! > As I corrected in another reply, why not use "depends on" like: config RPCIF_HYPERBUS tristate "Renesas RPC-IF HyperBus driver" depends on RENESAS_RPCIF - select MTD_CFI_ADV_OPTIONS + depends on MTD_CFI_BE_BYTE_SWAP || COMPILE_TEST help This option includes Renesas RPC-IF HyperFlash support. >>> + >>> + struct rpcif_hyperbus { >> >> WARNING: please, no spaces at the start of a line >> #73: FILE: drivers/mtd/hyperbus/rpc-if.c:25: >> + struct^Irpcif_hyperbus {$ > > Ugh, must be a stray space... :-/ > > [...] >>> +static const struct rpcif_op rpcif_op_tmpl = { >>> + .cmd = { >>> + .buswidth = 8, >>> + .ddr = true, >>> + }, >>> + .ocmd = { >>> + .buswidth = 8, >>> + .ddr = true, >>> + }, >>> + .addr = { >>> + .nbytes = 1, >>> + .buswidth = 8, >>> + .ddr = true, >>> + }, >>> + .data = { >>> + .buswidth = 8, >>> + .ddr = true, >>> + }, >>> +}; >>> + >>> +static u16 rpcif_hb_read16(struct hyperbus_device *hbdev, unsigned long addr) >>> +{ >>> + struct rpcif_hyperbus *hyperbus = >>> + container_of(hbdev, struct rpcif_hyperbus, hbdev); >>> + struct rpcif_op op = rpcif_op_tmpl; >>> + map_word data; >>> + >>> + op.cmd.opcode = 0x80; >> >> Here and elsewhere what does 0x80 represent? > > This is bits 40..47 of the HyperBus command/address packet: thus 0x80 means > memory read. > >> Maybe using a self >> describing macro would be helpful when assigning values to op.cmd.opcode? > > It probably would but it apparently needs to be placed in your HyperBus header... That's fine, Patches are welcome... > >>> + op.addr.val = addr >> 1; >>> + op.dummy.buswidth = 1; >>> + op.dummy.ncycles = 15; >>> + op.data.dir = RPCIF_DATA_IN; >>> + op.data.nbytes = 2; >>> + op.data.buf.in = &data; >> >> This code block seems like a good candidate to be converted to a macro/ >> template to avoid repetition. Template can initializes few values remain > > I'm already using a common template... > Yes, but I see there is scope to extend further as I pointed out below.. >> common across the driver and override others based on parameters passed. >> You could probably have one for write op and one for read op, if needed. > > OK, I'll look into doing that... > >> [...] >> >> Regards >> Vignesh > > MBR, Sergei >
Index: linux/drivers/mtd/hyperbus/Kconfig =================================================================== --- linux.orig/drivers/mtd/hyperbus/Kconfig +++ linux/drivers/mtd/hyperbus/Kconfig @@ -22,4 +22,11 @@ config HBMC_AM654 This is the driver for HyperBus controller on TI's AM65x and other SoCs +config RPCIF_HYPERBUS + tristate "Renesas RPC-IF HyperBus driver" + depends on RENESAS_RPCIF + select MTD_CFI_ADV_OPTIONS + help + This option includes Renesas RPC-IF HyperFlash support. + endif # MTD_HYPERBUS Index: linux/drivers/mtd/hyperbus/Makefile =================================================================== --- linux.orig/drivers/mtd/hyperbus/Makefile +++ linux/drivers/mtd/hyperbus/Makefile @@ -2,3 +2,4 @@ obj-$(CONFIG_MTD_HYPERBUS) += hyperbus-core.o obj-$(CONFIG_HBMC_AM654) += hbmc-am654.o +obj-$(CONFIG_RPCIF_HYPERBUS) += rpc-if.o Index: linux/drivers/mtd/hyperbus/rpc-if.c =================================================================== --- /dev/null +++ linux/drivers/mtd/hyperbus/rpc-if.c @@ -0,0 +1,165 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Linux driver for RPC-IF HyperFlash + * + * Copyright (C) 2019-2020 Cogent Embedded, Inc. + */ + +#include <linux/err.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/mtd/hyperbus.h> +#include <linux/mtd/mtd.h> +#include <linux/mux/consumer.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/types.h> + +#include <memory/renesas-rpc-if.h> + +/* FIXME: How to drop this? */ +#ifndef CONFIG_MTD_CFI_BE_BYTE_SWAP +#error Enable config "Flash cmd/query data swapping (BIG_ENDIAN_BYTE)" +#endif + + struct rpcif_hyperbus { + struct rpcif rpc; + struct hyperbus_ctlr ctlr; + struct hyperbus_device hbdev; +}; + +static const struct rpcif_op rpcif_op_tmpl = { + .cmd = { + .buswidth = 8, + .ddr = true, + }, + .ocmd = { + .buswidth = 8, + .ddr = true, + }, + .addr = { + .nbytes = 1, + .buswidth = 8, + .ddr = true, + }, + .data = { + .buswidth = 8, + .ddr = true, + }, +}; + +static u16 rpcif_hb_read16(struct hyperbus_device *hbdev, unsigned long addr) +{ + struct rpcif_hyperbus *hyperbus = + container_of(hbdev, struct rpcif_hyperbus, hbdev); + struct rpcif_op op = rpcif_op_tmpl; + map_word data; + + op.cmd.opcode = 0x80; + op.addr.val = addr >> 1; + op.dummy.buswidth = 1; + op.dummy.ncycles = 15; + op.data.dir = RPCIF_DATA_IN; + op.data.nbytes = 2; + op.data.buf.in = &data; + rpcif_prepare(&hyperbus->rpc, &op, NULL, NULL); + rpcif_manual_xfer(&hyperbus->rpc); + + return data.x[0]; +} + +static void rpcif_hb_write16(struct hyperbus_device *hbdev, unsigned long addr, + u16 data) +{ + struct rpcif_hyperbus *hyperbus = + container_of(hbdev, struct rpcif_hyperbus, hbdev); + struct rpcif_op op = rpcif_op_tmpl; + + op.cmd.opcode = 0; + op.addr.val = addr >> 1; + op.data.dir = RPCIF_DATA_OUT; + op.data.nbytes = 2; + op.data.buf.out = &data; + rpcif_prepare(&hyperbus->rpc, &op, NULL, NULL); + rpcif_manual_xfer(&hyperbus->rpc); +} + +static void rpcif_hb_copy_from(struct hyperbus_device *hbdev, void *to, + unsigned long from, ssize_t len) +{ + struct rpcif_hyperbus *hyperbus = + container_of(hbdev, struct rpcif_hyperbus, hbdev); + struct rpcif_op op = rpcif_op_tmpl; + + op.cmd.opcode = 0x80; + op.addr.val = from >> 1; + op.dummy.buswidth = 1; + op.dummy.ncycles = 15; + op.data.dir = RPCIF_DATA_IN; + op.data.nbytes = len; + op.data.buf.in = to; + rpcif_prepare(&hyperbus->rpc, &op, NULL, NULL); + + rpcif_dirmap_read(&hyperbus->rpc, from, len, to); +} + +static const struct hyperbus_ops rpcif_hb_ops = { + .read16 = rpcif_hb_read16, + .write16 = rpcif_hb_write16, + .copy_from = rpcif_hb_copy_from, +}; + +static int rpcif_hb_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct rpcif_hyperbus *hyperbus; + int error; + + hyperbus = devm_kzalloc(dev, sizeof(*hyperbus), GFP_KERNEL); + if (!hyperbus) + return -ENOMEM; + + rpcif_sw_init(&hyperbus->rpc, pdev->dev.parent); + + platform_set_drvdata(pdev, hyperbus); + + rpcif_enable_rpm(&hyperbus->rpc); + + rpcif_hw_init(&hyperbus->rpc, true); + + hyperbus->hbdev.map.size = hyperbus->rpc.size; + hyperbus->hbdev.map.virt = hyperbus->rpc.dirmap; + + hyperbus->ctlr.dev = dev; + hyperbus->ctlr.ops = &rpcif_hb_ops; + hyperbus->hbdev.ctlr = &hyperbus->ctlr; + hyperbus->hbdev.np = of_get_next_child(pdev->dev.parent->of_node, NULL); + error = hyperbus_register_device(&hyperbus->hbdev); + if (error) + rpcif_disable_rpm(&hyperbus->rpc); + + return error; +} + +static int rpcif_hb_remove(struct platform_device *pdev) +{ + struct rpcif_hyperbus *hyperbus = platform_get_drvdata(pdev); + int error = hyperbus_unregister_device(&hyperbus->hbdev); + struct rpcif *rpc = dev_get_drvdata(pdev->dev.parent); + + rpcif_disable_rpm(rpc); + return error; +} + +static struct platform_driver rpcif_platform_driver = { + .probe = rpcif_hb_probe, + .remove = rpcif_hb_remove, + .driver = { + .name = "rpc-if-hyperflash", + }, +}; + +module_platform_driver(rpcif_platform_driver); + +MODULE_DESCRIPTION("Renesas RPC-IF HyperFlash driver"); +MODULE_LICENSE("GPL v2");
Add the HyperFLash driver for the Renesas RPC-IF. It's the "front end" driver using the "back end" APIs in the main driver to talk to the real hardware. Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> --- This patch is against the 'mtd/next' branch of the MTD 'linux.git' repo. Requires the RPC-IF main driver patch in order to build/work: [1] https://patchwork.kernel.org/patch/11283127/ Changes in version 2: - added 'select MTD_CFI_ADV_OPTIONS' to the Kconfig entry, added #ifndef CONFIG_MTD_CFI_BE_BYTE_SWAP #error in the driver, and removed be16_to_cpu() call in the read16() method; - zeroed the target and burst type bits of the HyperBus command codes; - passed the address of 16-bit entity to rpcif_prepare() in the copy_from() method; - added an empty line between rpcif_prepare() and rpcif_drirmap_read() calls in the copy_from() method; - renamed rpcif_io_xfer() to rpcif_manual_xfer(); - removed the C++ style comments to rpcif_prepare() calls; - removed dev_err() call from the probe() method; - renamed the 'status' local variable to 'error' in the probe() method; - extended the driver copyright to this year. drivers/mtd/hyperbus/Kconfig | 7 + drivers/mtd/hyperbus/Makefile | 1 drivers/mtd/hyperbus/rpc-if.c | 165 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 173 insertions(+)