linux-kernel - Re: [PATCH 04/11] ARM: shmobile: lager legacy: Add MSIOF support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANqRtoRLcac3x55N+VHK+n6RvcgT0v9==9mt1rhVda-zwCYTCg@mail.gmail.com>
Date:	Mon, 24 Feb 2014 11:09:50 +0900
From:	Magnus Damm <magnus.damm@...il.com>
To:	Geert Uytterhoeven <geert@...ux-m68k.org>
Cc:	Simon Horman <horms@...ge.net.au>,
	SH-Linux <linux-sh@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	linux-kernel <linux-kernel@...r.kernel.org>,
	Geert Uytterhoeven <geert+renesas@...ux-m68k.org>
Subject: Re: [PATCH 04/11] ARM: shmobile: lager legacy: Add MSIOF support

Hi Geert,

On Fri, Feb 21, 2014 at 1:18 AM, Geert Uytterhoeven
<geert@...ux-m68k.org> wrote:
> Hi Magnus,
>
> On Thu, Feb 20, 2014 at 4:48 PM, Magnus Damm <magnus.damm@...il.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/