[PATCH 04/11] ARM: shmobile: lager legacy: Add MSIOF support

Hi Geert!

On Mon, Feb 24, 2014 at 5:25 PM, Geert Uytterhoeven
[off-list ref] wrote:

Hi Magnus,

On Mon, Feb 24, 2014 at 3:09 AM, Magnus Damm [off-list ref] wrote:

quoted

On Fri, Feb 21, 2014 at 1:18 AM, Geert Uytterhoeven
[off-list ref] wrote:

quoted

On Thu, Feb 20, 2014 at 4:48 PM, Magnus Damm [off-list ref] wrote:

quoted

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.

quoted

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.

quoted

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.

quoted

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