| Message ID | 1392907779-22053-5-git-send-email-geert@linux-m68k.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Geert, On Thu, Feb 20, 2014 at 11:49 PM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > From: Geert Uytterhoeven <geert+renesas@linux-m68k.org> > > Add MSIOF resources, platform data, platform device, pinctrl, and SPI > child device. > > - Platform device numbering is 1-based for compatibility with the BSP, > as QSPI uses zero. > - Only MSIOF1 is in use, and thus registered. Its bus contains a single > device (a Renesas R2A11302FT PMIC). > > Signed-off-by: Geert Uytterhoeven <geert+renesas@linux-m68k.org> > --- > arch/arm/mach-shmobile/board-lager.c | 64 ++++++++++++++++++++++++++++++---- > 1 file changed, 58 insertions(+), 6 deletions(-) > > diff --git a/arch/arm/mach-shmobile/board-lager.c b/arch/arm/mach-shmobile/board-lager.c > index f0104bfe544e..6062bcbda366 100644 > --- a/arch/arm/mach-shmobile/board-lager.c > +++ b/arch/arm/mach-shmobile/board-lager.c > @@ -55,6 +55,7 @@ > #include <linux/mtd/mtd.h> > #include <linux/spi/flash.h> > #include <linux/spi/rspi.h> > +#include <linux/spi/sh_msiof.h> > #include <linux/spi/spi.h> > #include <sound/rcar_snd.h> > #include <sound/simple_card.h> > @@ -287,6 +288,40 @@ static const struct platform_device_info ether_info __initconst = { > .dma_mask = DMA_BIT_MASK(32), > }; > > +/* MSIOF */ > +static const struct resource sh_msiof0_resources[] __initconst = { > + DEFINE_RES_MEM(0xe6e20000, 0x0064), > + DEFINE_RES_IRQ(gic_spi(156)), > +}; > + > +static const struct resource sh_msiof1_resources[] __initconst = { > + DEFINE_RES_MEM(0xe6e10000, 0x0064), > + DEFINE_RES_IRQ(gic_spi(157)), > +}; > + > +static const struct resource sh_msiof2_resources[] __initconst = { > + DEFINE_RES_MEM(0xe6e00000, 0x0064), > + DEFINE_RES_IRQ(gic_spi(158)), > +}; > + > +static const struct resource sh_msiof3_resources[] __initconst = { > + DEFINE_RES_MEM(0xe6c90000, 0x0064), > + DEFINE_RES_IRQ(gic_spi(159)), > +}; > + > +static const struct sh_msiof_spi_info sh_msiof_info __initconst = { > + .rx_fifo_override = 256, > + .num_chipselect = 1, > +}; > + > +#define r8a7790_register_msiof(idx) \ > + platform_device_register_resndata(&platform_bus, \ > + "spi_r8a7790_msiof", \ > + (idx+1), sh_msiof##idx##_resources, \ > + ARRAY_SIZE(sh_msiof##idx##_resources), \ > + &sh_msiof_info, \ > + sizeof(struct sh_msiof_spi_info)) Hi Geert, That for your efforts - it's good to see the MSIOF being integrated as well! I have one comment on this legacy board integration code. Since only MSIOF1 is used on Lager (correct me if i'm wrong), isn't it best to omit the unused resources from above? In case of DT I think it makes sense to define all channels in the SoC.dtsi and let the SoC-board.dts just enable the channels that are used. But in this case with legacy code I think we should keep thing simple and small and just enable the bits that are used on the particular board. The same obviously applies to the Koelsch legacy code as well. =) Thanks, / magnus
Hi Magnus, On Thu, Feb 20, 2014 at 4:48 PM, Magnus Damm <magnus.damm@gmail.com> wrote: >> +/* MSIOF */ >> +static const struct resource sh_msiof0_resources[] __initconst = { >> + DEFINE_RES_MEM(0xe6e20000, 0x0064), >> + DEFINE_RES_IRQ(gic_spi(156)), >> +}; >> + >> +static const struct resource sh_msiof1_resources[] __initconst = { >> + DEFINE_RES_MEM(0xe6e10000, 0x0064), >> + DEFINE_RES_IRQ(gic_spi(157)), >> +}; >> + >> +static const struct resource sh_msiof2_resources[] __initconst = { >> + DEFINE_RES_MEM(0xe6e00000, 0x0064), >> + DEFINE_RES_IRQ(gic_spi(158)), >> +}; >> + >> +static const struct resource sh_msiof3_resources[] __initconst = { >> + DEFINE_RES_MEM(0xe6c90000, 0x0064), >> + DEFINE_RES_IRQ(gic_spi(159)), >> +}; >> + >> +static const struct sh_msiof_spi_info sh_msiof_info __initconst = { >> + .rx_fifo_override = 256, >> + .num_chipselect = 1, >> +}; >> + >> +#define r8a7790_register_msiof(idx) \ >> + platform_device_register_resndata(&platform_bus, \ >> + "spi_r8a7790_msiof", \ >> + (idx+1), sh_msiof##idx##_resources, \ >> + ARRAY_SIZE(sh_msiof##idx##_resources), \ >> + &sh_msiof_info, \ >> + sizeof(struct sh_msiof_spi_info)) > > That for your efforts - it's good to see the MSIOF being integrated as > well! I have one comment on this legacy board integration code. > > Since only MSIOF1 is used on Lager (correct me if i'm wrong), isn't it > best to omit the unused resources from above? In case of DT I think it > makes sense to define all channels in the SoC.dtsi and let the > SoC-board.dts just enable the channels that are used. But in this case > with legacy code I think we should keep thing simple and small and > just enable the bits that are used on the particular board. > > The same obviously applies to the Koelsch legacy code as well. =) Note that while all resources are present, only MSIOF1 is registered on Lager (MSIOF0 on Koelsch). This is similar to i2c on Koelsch, which also has all resources, but only registers active devices. It's your preference, though, so I can adapt if you want. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Geert, On Fri, Feb 21, 2014 at 1:18 AM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > Hi Magnus, > > On Thu, Feb 20, 2014 at 4:48 PM, Magnus Damm <magnus.damm@gmail.com> wrote: >>> +/* MSIOF */ >>> +static const struct resource sh_msiof0_resources[] __initconst = { >>> + DEFINE_RES_MEM(0xe6e20000, 0x0064), >>> + DEFINE_RES_IRQ(gic_spi(156)), >>> +}; >>> + >>> +static const struct resource sh_msiof1_resources[] __initconst = { >>> + DEFINE_RES_MEM(0xe6e10000, 0x0064), >>> + DEFINE_RES_IRQ(gic_spi(157)), >>> +}; >>> + >>> +static const struct resource sh_msiof2_resources[] __initconst = { >>> + DEFINE_RES_MEM(0xe6e00000, 0x0064), >>> + DEFINE_RES_IRQ(gic_spi(158)), >>> +}; >>> + >>> +static const struct resource sh_msiof3_resources[] __initconst = { >>> + DEFINE_RES_MEM(0xe6c90000, 0x0064), >>> + DEFINE_RES_IRQ(gic_spi(159)), >>> +}; >>> + >>> +static const struct sh_msiof_spi_info sh_msiof_info __initconst = { >>> + .rx_fifo_override = 256, >>> + .num_chipselect = 1, >>> +}; >>> + >>> +#define r8a7790_register_msiof(idx) \ >>> + platform_device_register_resndata(&platform_bus, \ >>> + "spi_r8a7790_msiof", \ >>> + (idx+1), sh_msiof##idx##_resources, \ >>> + ARRAY_SIZE(sh_msiof##idx##_resources), \ >>> + &sh_msiof_info, \ >>> + sizeof(struct sh_msiof_spi_info)) >> >> That for your efforts - it's good to see the MSIOF being integrated as >> well! I have one comment on this legacy board integration code. >> >> Since only MSIOF1 is used on Lager (correct me if i'm wrong), isn't it >> best to omit the unused resources from above? In case of DT I think it >> makes sense to define all channels in the SoC.dtsi and let the >> SoC-board.dts just enable the channels that are used. But in this case >> with legacy code I think we should keep thing simple and small and >> just enable the bits that are used on the particular board. >> >> The same obviously applies to the Koelsch legacy code as well. =) > > Note that while all resources are present, only MSIOF1 is registered on > Lager (MSIOF0 on Koelsch). This is similar to i2c on Koelsch, which also > has all resources, but only registers active devices. Ok, I understand. Thanks for brining this to my attention. I'd like to avoid having unused resources so I'll cook up a patch to rework that myself. > It's your preference, though, so I can adapt if you want. Thanks. Please rework this patch to only register a single MSIOF channel. I think it makes sense to only enable hardware that is being used. Another question: How about "bus_num" and the platform device id mapping? I'd like them to be the same if possible, but you are having this "(idx+1)" bit in your code which I assume is to add offset for the QSPI bus. Regarding the PFC configuration, can you please double check that the PIN_MAP_MUX_GROUP_DEFAULT() is in sync with the platform device id? Is it the "bus_num" or the platform device id that is being used in case of SPI? Thanks! / magnus
Hi Magnus, On Mon, Feb 24, 2014 at 3:09 AM, Magnus Damm <magnus.damm@gmail.com> wrote: > On Fri, Feb 21, 2014 at 1:18 AM, Geert Uytterhoeven > <geert@linux-m68k.org> wrote: >> On Thu, Feb 20, 2014 at 4:48 PM, Magnus Damm <magnus.damm@gmail.com> wrote: >>> Since only MSIOF1 is used on Lager (correct me if i'm wrong), isn't it >>> best to omit the unused resources from above? In case of DT I think it >>> makes sense to define all channels in the SoC.dtsi and let the >>> SoC-board.dts just enable the channels that are used. But in this case >>> with legacy code I think we should keep thing simple and small and >>> just enable the bits that are used on the particular board. >>> >>> The same obviously applies to the Koelsch legacy code as well. =) >> >> Note that while all resources are present, only MSIOF1 is registered on >> Lager (MSIOF0 on Koelsch). This is similar to i2c on Koelsch, which also >> has all resources, but only registers active devices. > > Ok, I understand. Thanks for brining this to my attention. > > I'd like to avoid having unused resources so I'll cook up a patch to > rework that myself. > >> It's your preference, though, so I can adapt if you want. > > Thanks. > > Please rework this patch to only register a single MSIOF channel. I > think it makes sense to only enable hardware that is being used. Ehrm, I already register a single MSIOF channel only. Perhaps you mean't "remove the unused resources"? > Another question: How about "bus_num" and the platform device id > mapping? I'd like them to be the same if possible, but you are having > this "(idx+1)" bit in your code which I assume is to add offset for > the QSPI bus. "bus_num" is the SPI-specific numbering of SPI masters, which is filled in by spi-sh-msiof.c based on platform_device.id (i.e. the numeric suffix of e.g. "spi_r8a7790_msiof.1"). It's used for matching SPI slaves in spi_board_info with SPI masters. As QSPI ("qspi.0") has SPI bus_num 0, the MSIOF SPI masters use bus_num 1 to 4, hence the "idx+1", and the platform device names "spi_r8a7790_msiof.1" to "spi_r8a7790_msiof.4". (Can't spi-sh-msiof.c use "bus_num = pdev->id + 1", so we can have MSIOF0 as "spi_r8a7790_msiof.0"? No, as that would impact numbering on all SoCs with MSIOF.) With DT, all of this doesn't matter, and life is easier, as the SPI slaves are child nodes of the SPI masters and thus don't need numerical bus references. So MSIOF0 can be called "msiof0" there. We still have the offsets in the "spi%u" aliases, though. > Regarding the PFC configuration, can you please double check that the > PIN_MAP_MUX_GROUP_DEFAULT() is in sync with the platform device id? Is > it the "bus_num" or the platform device id that is being used in case > of SPI? "bus_num" is SPI-specific. Pinctrl uses the actual device's name: /** * struct pinctrl_map - boards/machines shall provide this map for devices * @dev_name: the name of the device using this specific mapping, the name * must be the same as in your struct device*. If this name is set to the * same name as the pin controllers own dev_name(), the map entry will be * hogged by the driver itself upon registration Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Geert! On Mon, Feb 24, 2014 at 5:25 PM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > Hi Magnus, > > On Mon, Feb 24, 2014 at 3:09 AM, Magnus Damm <magnus.damm@gmail.com> wrote: >> On Fri, Feb 21, 2014 at 1:18 AM, Geert Uytterhoeven >> <geert@linux-m68k.org> wrote: >>> On Thu, Feb 20, 2014 at 4:48 PM, Magnus Damm <magnus.damm@gmail.com> wrote: >>>> Since only MSIOF1 is used on Lager (correct me if i'm wrong), isn't it >>>> best to omit the unused resources from above? In case of DT I think it >>>> makes sense to define all channels in the SoC.dtsi and let the >>>> SoC-board.dts just enable the channels that are used. But in this case >>>> with legacy code I think we should keep thing simple and small and >>>> just enable the bits that are used on the particular board. >>>> >>>> The same obviously applies to the Koelsch legacy code as well. =) >>> >>> Note that while all resources are present, only MSIOF1 is registered on >>> Lager (MSIOF0 on Koelsch). This is similar to i2c on Koelsch, which also >>> has all resources, but only registers active devices. >> >> Ok, I understand. Thanks for brining this to my attention. >> >> I'd like to avoid having unused resources so I'll cook up a patch to >> rework that myself. >> >>> It's your preference, though, so I can adapt if you want. >> >> Thanks. >> >> Please rework this patch to only register a single MSIOF channel. I >> think it makes sense to only enable hardware that is being used. > > Ehrm, I already register a single MSIOF channel only. > Perhaps you mean't "remove the unused resources"? Yes, exactly. My apologies for the unclear reply. >> Another question: How about "bus_num" and the platform device id >> mapping? I'd like them to be the same if possible, but you are having >> this "(idx+1)" bit in your code which I assume is to add offset for >> the QSPI bus. > > "bus_num" is the SPI-specific numbering of SPI masters, which is filled > in by spi-sh-msiof.c based on platform_device.id (i.e. the numeric suffix > of e.g. "spi_r8a7790_msiof.1"). > It's used for matching SPI slaves in spi_board_info with SPI masters. > As QSPI ("qspi.0") has SPI bus_num 0, the MSIOF SPI masters use > bus_num 1 to 4, hence the "idx+1", and the platform device names > "spi_r8a7790_msiof.1" to "spi_r8a7790_msiof.4". > > (Can't spi-sh-msiof.c use "bus_num = pdev->id + 1", so we can have > MSIOF0 as "spi_r8a7790_msiof.0"? No, as that would impact numbering > on all SoCs with MSIOF.) Yeah, the bus number that is commonly used for SPI and I2C behaves like that so I agree with what you're saying. I guess historically we usually only have one I2C master and one SPI master which makes it easy to use direct mapping between bus num and pdev->id. Now on Lager we have multiple SPI masters (both QSPI and MSIOF unless I'm mistaken), so the question is how to allocate the ranges of bus_num for each SPI master. I believe your current allocation works well but I'm a bit confused by it I must confess. I'm used to one of the two schemes: 1) single master with pdev->id equals bus_num 2) compact board specific bus allocation I believe you introduce something similar to 1) but for two SPI masters which is totally fine! For some unknown reason I expected 2) with bus_num 0 for QSPI and bus_num 1 for MSIOF1, but I think your allocation scheme is reusable across multiple boards with the same SoC so I think your current code is better when I think about it a bit more. > With DT, all of this doesn't matter, and life is easier, as the SPI slaves > are child nodes of the SPI masters and thus don't need numerical bus > references. So MSIOF0 can be called "msiof0" there. > We still have the offsets in the "spi%u" aliases, though. Right all this goes away with DT which is nice. >> Regarding the PFC configuration, can you please double check that the >> PIN_MAP_MUX_GROUP_DEFAULT() is in sync with the platform device id? Is >> it the "bus_num" or the platform device id that is being used in case >> of SPI? > > "bus_num" is SPI-specific. Pinctrl uses the actual device's name: > > /** > * struct pinctrl_map - boards/machines shall provide this map for devices > * @dev_name: the name of the device using this specific mapping, the name > * must be the same as in your struct device*. If this name is set to the > * same name as the pin controllers own dev_name(), the map entry will be > * hogged by the driver itself upon registration Right. I was just confused seeing the pdev->id set to 2 on MSIOF1, but I now understand that it is your intentional design to have bus_num 0 as QSPI, bus_num1 as unused MSIOF0 and bus_num 2 as MSIOF1. It just takes some time for me to grasp. =) Cheers, / magnus
Hi Magnus, On Mon, Feb 24, 2014 at 9:44 AM, Magnus Damm <magnus.damm@gmail.com> wrote: >>> Another question: How about "bus_num" and the platform device id >>> mapping? I'd like them to be the same if possible, but you are having >>> this "(idx+1)" bit in your code which I assume is to add offset for >>> the QSPI bus. >> >> "bus_num" is the SPI-specific numbering of SPI masters, which is filled >> in by spi-sh-msiof.c based on platform_device.id (i.e. the numeric suffix >> of e.g. "spi_r8a7790_msiof.1"). >> It's used for matching SPI slaves in spi_board_info with SPI masters. >> As QSPI ("qspi.0") has SPI bus_num 0, the MSIOF SPI masters use >> bus_num 1 to 4, hence the "idx+1", and the platform device names >> "spi_r8a7790_msiof.1" to "spi_r8a7790_msiof.4". >> >> (Can't spi-sh-msiof.c use "bus_num = pdev->id + 1", so we can have >> MSIOF0 as "spi_r8a7790_msiof.0"? No, as that would impact numbering >> on all SoCs with MSIOF.) > > Yeah, the bus number that is commonly used for SPI and I2C behaves > like that so I agree with what you're saying. I guess historically we > usually only have one I2C master and one SPI master which makes it > easy to use direct mapping between bus num and pdev->id. > > Now on Lager we have multiple SPI masters (both QSPI and MSIOF unless > I'm mistaken), so the question is how to allocate the ranges of > bus_num for each SPI master. I believe your current allocation works > well but I'm a bit confused by it I must confess. > > I'm used to one of the two schemes: > 1) single master with pdev->id equals bus_num > 2) compact board specific bus allocation > > I believe you introduce something similar to 1) but for two SPI > masters which is totally fine! For some unknown reason I expected 2) > with bus_num 0 for QSPI and bus_num 1 for MSIOF1, but I think your > allocation scheme is reusable across multiple boards with the same SoC > so I think your current code is better when I think about it a bit > more. On our specific boards, we now have a sparse SPI bus numbering, but that's just because some MSIOF channels are not used. On other boards with the same SoC that may be different. > Right. I was just confused seeing the pdev->id set to 2 on MSIOF1, but > I now understand that it is your intentional design to have bus_num 0 > as QSPI, bus_num1 as unused MSIOF0 and bus_num 2 as MSIOF1. Actually that's too much credit for me: the bus numbering came from the BSP through Yoshii-san. > It just takes some time for me to grasp. =) Yes, we're too used to having one single type of each hardware type :-) Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
diff --git a/arch/arm/mach-shmobile/board-lager.c b/arch/arm/mach-shmobile/board-lager.c index f0104bfe544e..6062bcbda366 100644 --- a/arch/arm/mach-shmobile/board-lager.c +++ b/arch/arm/mach-shmobile/board-lager.c @@ -55,6 +55,7 @@ #include <linux/mtd/mtd.h> #include <linux/spi/flash.h> #include <linux/spi/rspi.h> +#include <linux/spi/sh_msiof.h> #include <linux/spi/spi.h> #include <sound/rcar_snd.h> #include <sound/simple_card.h> @@ -287,6 +288,40 @@ static const struct platform_device_info ether_info __initconst = { .dma_mask = DMA_BIT_MASK(32), }; +/* MSIOF */ +static const struct resource sh_msiof0_resources[] __initconst = { + DEFINE_RES_MEM(0xe6e20000, 0x0064), + DEFINE_RES_IRQ(gic_spi(156)), +}; + +static const struct resource sh_msiof1_resources[] __initconst = { + DEFINE_RES_MEM(0xe6e10000, 0x0064), + DEFINE_RES_IRQ(gic_spi(157)), +}; + +static const struct resource sh_msiof2_resources[] __initconst = { + DEFINE_RES_MEM(0xe6e00000, 0x0064), + DEFINE_RES_IRQ(gic_spi(158)), +}; + +static const struct resource sh_msiof3_resources[] __initconst = { + DEFINE_RES_MEM(0xe6c90000, 0x0064), + DEFINE_RES_IRQ(gic_spi(159)), +}; + +static const struct sh_msiof_spi_info sh_msiof_info __initconst = { + .rx_fifo_override = 256, + .num_chipselect = 1, +}; + +#define r8a7790_register_msiof(idx) \ + platform_device_register_resndata(&platform_bus, \ + "spi_r8a7790_msiof", \ + (idx+1), sh_msiof##idx##_resources, \ + ARRAY_SIZE(sh_msiof##idx##_resources), \ + &sh_msiof_info, \ + sizeof(struct sh_msiof_spi_info)) + /* SPI Flash memory (Spansion S25FL512SAGMFIG11 64Mb) */ static struct mtd_partition spi_flash_part[] = { /* Reserved for user loader program, read-only */ @@ -325,12 +360,19 @@ static const struct rspi_plat_data qspi_pdata __initconst = { static const struct spi_board_info spi_info[] __initconst = { { - .modalias = "m25p80", - .platform_data = &spi_flash_data, - .mode = SPI_MODE_0, - .max_speed_hz = 30000000, - .bus_num = 0, - .chip_select = 0, + .modalias = "m25p80", + .platform_data = &spi_flash_data, + .mode = SPI_MODE_0, + .max_speed_hz = 30000000, + .bus_num = 0, + .chip_select = 0, + }, { + .modalias = "r2a1130x", + .max_speed_hz = 6000000, + .chip_select = 0, + .bus_num = 2, + .mode = SPI_MODE_3, + .controller_data = (void *)-ENOENT, /* HW controlled CS */ }, }; @@ -703,6 +745,15 @@ static const struct pinctrl_map lager_pinctrl_map[] = { /* I2C2 */ PIN_MAP_MUX_GROUP_DEFAULT("i2c-rcar.2", "pfc-r8a7790", "i2c2", "i2c2"), + /* MSIOF1 */ + PIN_MAP_MUX_GROUP_DEFAULT("spi_r8a7790_msiof.2", "pfc-r8a7790", + "msiof1_clk", "msiof1"), + PIN_MAP_MUX_GROUP_DEFAULT("spi_r8a7790_msiof.2", "pfc-r8a7790", + "msiof1_sync", "msiof1"), + PIN_MAP_MUX_GROUP_DEFAULT("spi_r8a7790_msiof.2", "pfc-r8a7790", + "msiof1_rx", "msiof1"), + PIN_MAP_MUX_GROUP_DEFAULT("spi_r8a7790_msiof.2", "pfc-r8a7790", + "msiof1_tx", "msiof1"), /* QSPI */ PIN_MAP_MUX_GROUP_DEFAULT("qspi.0", "pfc-r8a7790", "qspi_ctrl", "qspi"), @@ -811,6 +862,7 @@ static void __init lager_add_standard_devices(void) qspi_resources, ARRAY_SIZE(qspi_resources), &qspi_pdata, sizeof(qspi_pdata)); + r8a7790_register_msiof(1); spi_register_board_info(spi_info, ARRAY_SIZE(spi_info)); platform_device_register_data(&platform_bus, "reg-fixed-voltage", fixed_regulator_idx++,