ppc/sata-fsl: orphan config value: CONFIG_MPC8315_DS

[Scott Wood]

On 05/01/2013 06:35:38 PM, Anthony Foiani wrote:
> Scott --
> 
> Thanks again for the quick reply.
> 
> On 05/01/2013 12:05 PM, Scott Wood wrote:
>> On 04/30/2013 09:06:56 PM, Anthony Foiani wrote:
>>> Instead of a new property name, I would instead check for my  
>>> specific board type (let's call it a foo-8315) in the top-level  
>>> compatible list?  So I'd change my devtree to have this top-level  
>>> compatible:
>>> 
>>> / {
>>>     compatible = "example,foo-8315", "fsl,mpc8315erdb";
>> 
>> It should really only have compatible = "example,foo-8315", since  
>> it's not 100% compatible with fsl,mpc8315erdb (at least due to this  
>> bug, but probably there are other differences as well).
> 
> Then I guess I don't understand the proper use of "compatible" (or is  
> the root node special?)

It's only special in that 100% compatibility is much less likely to be  
true of an entire system than of a particular component.

> E.g., the DTS for the "parent" board (MPC8315ERDB) has multiple  
> entries for the crypto "compatible" value:
> 
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/powerpc/boot/dts/mpc8315erdb.dts?id=refs/tags/v3.9#n286
> (or: *http://preview.tinyurl.com/btlqxgo* )
> 
> |		crypto at 30000 {
> 			compatible = "fsl,sec3.3", "fsl,sec3.1",  
> "fsl,sec3.0",
> 				     "fsl,sec2.4", "fsl,sec2.2",  
> "fsl,sec2.1",
> 				     "fsl,sec2.0";
> 			reg = <0x30000 0x10000>;|
> 
> I read this as meaning: "if you have to ask if a certain feature is  
> compatible with some 'foo', then this board provides that  
> compatibility".  Not as "if a value is in the compatibility flag,  
> then it is 100% compatible with that value".  (Although maybe that's  
> true in the case of the SEC, so perhaps that a bad example.)

AFAIK there is backwards compatibility with these SEC versions.  If  
not, they shouldn't be listed.

> For what it's worth, the upstream vendor did have a separate  
> root-node "compatible" value -- which called a board-specific  
> function in a board-specific C file, both of which were direct cut &  
> paste copies from the MPC8315ERDB function / file.  My gut instinct  
> is that this degree of duplication is unhealthy and incorrect, but if  
> my solution is considered abuse of the device tree, then I can try to  
> do it a different way next time.

It's quite possible to use the same C file for multiple similar boards  
with different compatibles.  This is done often, including  
mpc831x_erdb.c.

> Given those diffs, it didn't seem much of a stretch to use compatible  
> = "fsl,mpc8315erdb"

The criteria for claiming compatibility should be based in the hardware  
itself, not whether a particular file in Linux needs any changes.

>>>> Or do you mean that you would not set this on any board's device  
>>>> tree by default, and instead have users set it if they encounter  
>>>> problems?
>>> 
>>> No, I would expect to set it on all the boards, so using the  
>>> compatibility hack above would work.
>> 
>> You mean all the boards that have the bug, which doesn't include any  
>> upstream device tree, right?
> As mentioned above, my primary concern is the use of these cards in  
> the project/product I'm working on.  My answer has been to apply this  
> fix (and the matching change to the device tree I supply as a part of  
> the boot image).  I feel that I'm trying to do the right thing by  
> getting some of these changes publicly visible, but I fear that I'll  
> also have to go down the route of "not enough time or money to  
> properly upstream it".
> 
> "doesn't include upstream device tree" ... no, the device tree was  
> supplied with the original set of patches from the vendor.

I'm not saying that the device tree not being upstream is a problem --  
actually the opposite, that it means we don't have compatibility to  
maintain with an already-accepted device tree.

-Scott