[lkml]   [2014]   [Feb]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCH 04/11] ARM: shmobile: lager legacy: Add MSIOF support
Hi Geert!

On Mon, Feb 24, 2014 at 5:25 PM, Geert Uytterhoeven
<> wrote:
> Hi Magnus,
> On Mon, Feb 24, 2014 at 3:09 AM, Magnus Damm <> wrote:
>> On Fri, Feb 21, 2014 at 1:18 AM, Geert Uytterhoeven
>> <> wrote:
>>> On Thu, Feb 20, 2014 at 4:48 PM, Magnus Damm <> 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 (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

> 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. =)


/ magnus

 \ /
  Last update: 2014-02-24 12:01    [W:0.110 / U:2.296 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site