Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1527751 > unrolled thread

Re: [PATCH 5/10] dt: bindings: Add bindings for Marvell Xenon SD Host Controller

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 5/10] dt: bindings: Add bindings for Marvell Xenon SD Host Controller Gregory CLEMENT <gregory.clement@free-electrons.com> - 2016-11-22 18:40 +0100
    Re: [PATCH 5/10] dt: bindings: Add bindings for Marvell Xenon SD Host Controller Ulf Hansson <ulf.hansson@linaro.org> - 2016-11-24 10:10 +0100
      Re: [PATCH 5/10] dt: bindings: Add bindings for Marvell Xenon SD Host Controller Arnd Bergmann <arnd@arndb.de> - 2016-11-24 10:20 +0100
        Re: [PATCH 5/10] dt: bindings: Add bindings for Marvell Xenon SD Host Controller Gregory CLEMENT <gregory.clement@free-electrons.com> - 2016-11-24 10:30 +0100
          Re: [PATCH 5/10] dt: bindings: Add bindings for Marvell Xenon SD Host Controller Arnd Bergmann <arnd@arndb.de> - 2016-11-24 10:40 +0100
            Re: [PATCH 5/10] dt: bindings: Add bindings for Marvell Xenon SD  Host Controller Thomas Petazzoni <thomas.petazzoni@free-electrons.com> - 2016-11-24 10:50 +0100
              Re: [PATCH 5/10] dt: bindings: Add bindings for Marvell Xenon SD Host Controller Arnd Bergmann <arnd@arndb.de> - 2016-11-24 11:10 +0100
            Re: [PATCH 5/10] dt: bindings: Add bindings for Marvell Xenon SD Host Controller Marcin Wojtas <mw@semihalf.com> - 2016-11-24 10:50 +0100
              Re: [PATCH 5/10] dt: bindings: Add bindings for Marvell Xenon SD  Host Controller Thomas Petazzoni <thomas.petazzoni@free-electrons.com> - 2016-11-24 11:20 +0100
                Re: [PATCH 5/10] dt: bindings: Add bindings for Marvell Xenon SD Host  Controller Ziji Hu <huziji@marvell.com> - 2016-11-24 11:40 +0100

#1527751 — Re: [PATCH 5/10] dt: bindings: Add bindings for Marvell Xenon SD Host Controller

Hi Rob,
 
 On jeu., nov. 10 2016, Ziji Hu <huziji@marvell.com> wrote:

[...]

>>> +
>>> +- reg:
>>> +  * For "marvell,xenon-sdhci", one register area for Xenon IP.
>>> +
>>> +  * For "marvell,armada-3700-sdhci", two register areas.
>>> +    The first one for Xenon IP register. The second one for the Armada 3700 SOC
>>> +    PHY PAD Voltage Control register.
>>> +    Please follow the examples with compatible "marvell,armada-3700-sdhci"
>>> +    in below.
>>> +    Please also check property marvell,pad-type in below.
>>> +
>>> +Optional Properties:
>>> +- marvell,xenon-slotno:
>> 
>> Multiple slots should be represented as child nodes IMO. I think some 
>> other bindings already do this.
>> 
>
> 	All the slots are entirely independent.
> 	I prefer to consider it as multiple independent SDHCs placed in
> 	a single IP, instead of that a IP contains multiple child slots.

It was indeed what I tried to show in my answer for the 1st version:
http://lists.infradead.org/pipermail/linux-arm-kernel/2016-October/461860.html

Maybe you missed it.

You also mentioned other bindings using child nodes, but for this one
we have one controller with only one set of register with multiple slots
(Atmel is an example). Here each slot have it own set of register.

Actually giving the fact that each slot is controlled by a different set
of register I wonder why the hardware can't also deduce the slot number
from the address register. For me it looks like an hardware bug but we
have to deal with it.

Do you still think we needchild node here?

>
> 	It is unlike the implementation which put multiple slots behind PCIe EP interface. sdhci-pci.c will handle each slot init one by one.
> 	If Xenon SDHC slots are represented as child nodes, there should also be a main entry in Xenon driver to init each child node one by one.
> 	In my very own opinion, it is inconvenient and unnecessary.


Gregory

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[toc] | [next] | [standalone]

#1529082

On 22 November 2016 at 18:23, Gregory CLEMENT
<gregory.clement@free-electrons.com> wrote:
> Hi Rob,
>
>  On jeu., nov. 10 2016, Ziji Hu <huziji@marvell.com> wrote:
>
> [...]
>
>>>> +
>>>> +- reg:
>>>> +  * For "marvell,xenon-sdhci", one register area for Xenon IP.
>>>> +
>>>> +  * For "marvell,armada-3700-sdhci", two register areas.
>>>> +    The first one for Xenon IP register. The second one for the Armada 3700 SOC
>>>> +    PHY PAD Voltage Control register.
>>>> +    Please follow the examples with compatible "marvell,armada-3700-sdhci"
>>>> +    in below.
>>>> +    Please also check property marvell,pad-type in below.
>>>> +
>>>> +Optional Properties:
>>>> +- marvell,xenon-slotno:
>>>
>>> Multiple slots should be represented as child nodes IMO. I think some
>>> other bindings already do this.
>>>
>>
>>       All the slots are entirely independent.
>>       I prefer to consider it as multiple independent SDHCs placed in
>>       a single IP, instead of that a IP contains multiple child slots.
>
> It was indeed what I tried to show in my answer for the 1st version:
> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-October/461860.html
>
> Maybe you missed it.
>
> You also mentioned other bindings using child nodes, but for this one
> we have one controller with only one set of register with multiple slots
> (Atmel is an example). Here each slot have it own set of register.
>
> Actually giving the fact that each slot is controlled by a different set
> of register I wonder why the hardware can't also deduce the slot number
> from the address register. For me it looks like an hardware bug but we
> have to deal with it.
>
> Do you still think we needchild node here?

Using child-nodes for slots like what's done in the atmel case, is
currently broken. I would recommend to avoid using child-nodes for
slots, if possible.

To give you some more background, currently the mmc core treats child
nodes as embedded non-removable cards or SDIO funcs. However, we can
change to make child-nodes also allowed to describe slots, but it
requires a specific compatible for "slots" and of course then we also
need to update the DT parsing of the child-nodes in the mmc core.

Documentation/devicetree/bindings/mmc/mmc.txt
Documentation/devicetree/bindings/mmc/mmc-card.txt

>
>>
>>       It is unlike the implementation which put multiple slots behind PCIe EP interface. sdhci-pci.c will handle each slot init one by one.
>>       If Xenon SDHC slots are represented as child nodes, there should also be a main entry in Xenon driver to init each child node one by one.
>>       In my very own opinion, it is inconvenient and unnecessary.
>

Kind regards
Uffe

[toc] | [prev] | [next] | [standalone]

#1529089

On Thursday, November 24, 2016 10:05:45 AM CET Ulf Hansson wrote:
> > You also mentioned other bindings using child nodes, but for this one
> > we have one controller with only one set of register with multiple slots
> > (Atmel is an example). Here each slot have it own set of register.
> >
> > Actually giving the fact that each slot is controlled by a different set
> > of register I wonder why the hardware can't also deduce the slot number
> > from the address register. For me it looks like an hardware bug but we
> > have to deal with it.
> >
> > Do you still think we needchild node here?
> 
> Using child-nodes for slots like what's done in the atmel case, is
> currently broken. I would recommend to avoid using child-nodes for
> slots, if possible.
> 
> To give you some more background, currently the mmc core treats child
> nodes as embedded non-removable cards or SDIO funcs. However, we can
> change to make child-nodes also allowed to describe slots, but it
> requires a specific compatible for "slots" and of course then we also
> need to update the DT parsing of the child-nodes in the mmc core.
> 
> Documentation/devicetree/bindings/mmc/mmc.txt
> Documentation/devicetree/bindings/mmc/mmc-card.txt

I don't see anything wrong with having child nodes for the slots
even with the current binding, under one condition:

The mmc.txt binding above must refer only to the child node, while
the parent node conceptually becomes a plain bus or MFD that
happens to encapsulate multiple MMC host controllers, and possibly
provides some shared registers to them.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1529097

Hi Arnd,
 
 On jeu., nov. 24 2016, Arnd Bergmann <arnd@arndb.de> wrote:

> On Thursday, November 24, 2016 10:05:45 AM CET Ulf Hansson wrote:
>> > You also mentioned other bindings using child nodes, but for this one
>> > we have one controller with only one set of register with multiple slots
>> > (Atmel is an example). Here each slot have it own set of register.
>> >
>> > Actually giving the fact that each slot is controlled by a different set
>> > of register I wonder why the hardware can't also deduce the slot number
>> > from the address register. For me it looks like an hardware bug but we
>> > have to deal with it.
>> >
>> > Do you still think we needchild node here?
>> 
>> Using child-nodes for slots like what's done in the atmel case, is
>> currently broken. I would recommend to avoid using child-nodes for
>> slots, if possible.
>> 
>> To give you some more background, currently the mmc core treats child
>> nodes as embedded non-removable cards or SDIO funcs. However, we can
>> change to make child-nodes also allowed to describe slots, but it
>> requires a specific compatible for "slots" and of course then we also
>> need to update the DT parsing of the child-nodes in the mmc core.
>> 
>> Documentation/devicetree/bindings/mmc/mmc.txt
>> Documentation/devicetree/bindings/mmc/mmc-card.txt
>
> I don't see anything wrong with having child nodes for the slots
> even with the current binding, under one condition:
>
> The mmc.txt binding above must refer only to the child node, while
> the parent node conceptually becomes a plain bus or MFD that
> happens to encapsulate multiple MMC host controllers, and possibly
> provides some shared registers to them.


I don't have an option for mmc in general, but using child node do not
fit at all the xenon controller.

For this controller each slot has its own set of register, so there is
no common ressource to share so no advantage to use it. Using child node
in our case will just make the code more complex for no benefit.

Gregory

>
> 	Arnd

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[toc] | [prev] | [next] | [standalone]

#1529111

On Thursday, November 24, 2016 10:22:31 AM CET Gregory CLEMENT wrote:
> 
> I don't have an option for mmc in general, but using child node do not
> fit at all the xenon controller.
> 
> For this controller each slot has its own set of register, so there is
> no common ressource to share so no advantage to use it. Using child node
> in our case will just make the code more complex for no benefit.

If every slot has its own registers, what is it that makes up the
'controller'? It sounds to me that you just have to adjust the terminology
and talk about multiple controllers then, with one slot per controller.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1529112 — Re: [PATCH 5/10] dt: bindings: Add bindings for Marvell Xenon SD Host Controller

Hello,

On Thu, 24 Nov 2016 10:44:48 +0100, Gregory CLEMENT wrote:

> "A single Xenon IP can support multiple slots.
> Each slot acts as an independent SDHC. It owns independent resources, such
> as register sets clock and PHY.
> Each slot should have an independent device tree node."

I think this wording is still very confusing, and continues to cause
confusion.

We should just state that each Xenon controller supports a single slot,
and that's it.

The text still says "a single Xenon IP can support multiple slots",
which continues to cause confusion.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[toc] | [prev] | [next] | [standalone]

#1529126

On Thursday, November 24, 2016 10:48:58 AM CET Thomas Petazzoni wrote:
> Hello,
> 
> On Thu, 24 Nov 2016 10:44:48 +0100, Gregory CLEMENT wrote:
> 
> > "A single Xenon IP can support multiple slots.
> > Each slot acts as an independent SDHC. It owns independent resources, such
> > as register sets clock and PHY.
> > Each slot should have an independent device tree node."
> 
> I think this wording is still very confusing, and continues to cause
> confusion.
> 
> We should just state that each Xenon controller supports a single slot,
> and that's it.
> 
> The text still says "a single Xenon IP can support multiple slots",
> which continues to cause confusion.

Agreed. Ideally we'd find out why exactly the slot number must
be used for accessing some of the registers to have a better
explanation to put in there, aside from stating that only one
slot is supported but the number must be set.

Could it be that this is some form of pinmuxing, i.e. that each
controller could in theory be used for any of the slots but you
have to pick one of them?

	Arnd

[toc] | [prev] | [next] | [standalone]

#1529114

Hi Gregory,

2016-11-24 10:44 GMT+01:00 Gregory CLEMENT <gregory.clement@free-electrons.com>:
> Hi Arnd,
>
>  On jeu., nov. 24 2016, Arnd Bergmann <arnd@arndb.de> wrote:
>
>> On Thursday, November 24, 2016 10:22:31 AM CET Gregory CLEMENT wrote:
>>>
>>> I don't have an option for mmc in general, but using child node do not
>>> fit at all the xenon controller.
>>>
>>> For this controller each slot has its own set of register, so there is
>>> no common ressource to share so no advantage to use it. Using child node
>>> in our case will just make the code more complex for no benefit.
>>
>> If every slot has its own registers, what is it that makes up the
>> 'controller'? It sounds to me that you just have to adjust the terminology
>> and talk about multiple controllers then, with one slot per controller.
>>
>
> I agree and actually there were some words about in at the begining of
> the binding:
>
> "A single Xenon IP can support multiple slots.
> Each slot acts as an independent SDHC. It owns independent resources, such
> as register sets clock and PHY.
> Each slot should have an independent device tree node."
>
> All the confusion came from the fact that we still need to identify a
> slot ID. For an obscure reason the hardware can't guess the slot ID from
> the address register."
>

How about to avoid confusion, by simply renaming this number to
port-id/xenon-id or anything else but slot? I guess this may allow to
avoid some misunderstandings.

Best regards,
Marcin

[toc] | [prev] | [next] | [standalone]

#1529129 — Re: [PATCH 5/10] dt: bindings: Add bindings for Marvell Xenon SD Host Controller

Hello,

On Thu, 24 Nov 2016 10:49:23 +0100, Marcin Wojtas wrote:

> How about to avoid confusion, by simply renaming this number to
> port-id/xenon-id or anything else but slot? I guess this may allow to
> avoid some misunderstandings.

Agreed.

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[toc] | [prev] | [next] | [standalone]

#1529145 — Re: [PATCH 5/10] dt: bindings: Add bindings for Marvell Xenon SD Host Controller

Hi all,

On 2016/11/24 18:10, Thomas Petazzoni wrote:
> Hello,
> 
> On Thu, 24 Nov 2016 10:49:23 +0100, Marcin Wojtas wrote:
> 
>> How about to avoid confusion, by simply renaming this number to
>> port-id/xenon-id or anything else but slot? I guess this may allow to
>> avoid some misunderstandings.
> 
	We borrow the term "slot" from PCIe interface from SD spec.
	According to Appendix C in SD spec 3.0, slot means an independent set of register from the view of SW.

	I can avoid using "slot" and replace "slot index" with "sdhc-id".
	Thanks for the suggestions.

	Thank you.

Best regards,
Hu Ziji

> Agreed.
> 
> Thomas
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web