2014-02-17 14:08 GMT-08:00 Sergei Shtylyov <sergei.shtylyov@xxxxxxxxxxxxxxxxxx>:
> On 02/17/2014 11:48 PM, Florian Fainelli wrote:
>
>>>>> Add new init-regs field for of_phy nodes and make sure these
>>>>> get applied when the phy is configured.
>
>
>>>>> This allows any phy node in an fdt to initialise registers
>>>>> that may not be set as standard by the driver at initialisation
>>>>> time, such as LED controls.
>
>
>>>>> Signed-off-by: Ben Dooks <ben.dooks@xxxxxxxxxxxxxxx>
>
> [...]
>
>
>>>>> diff --git a/drivers/net/phy/phy_device.c
>>>>> b/drivers/net/phy/phy_device.c
>>>>> index 82514e7..6741cdb 100644
>>>>> --- a/drivers/net/phy/phy_device.c
>>>>> +++ b/drivers/net/phy/phy_device.c
>
>
>>> [...]
>
>
>>>>> @@ -532,6 +533,57 @@ static int phy_poll_reset(struct phy_device
>>>>> *phydev)
>>>>> return 0;
>>>>> }
>>>>>
>>>>> +#ifdef CONFIG_OF
>>>>> +static int of_phy_configure(struct phy_device *phydev)
>>>>> +{
>>>>> + struct device *dev = &phydev->dev;
>>>>> + struct device_node *of_node = dev->of_node;
>>>>> + struct property *prop;
>>>>> + const __be32 *ptr;
>>>>> + u32 reg, set, clear;
>>>>> + int len;
>>>>> + int val;
>
>
>>>> This does not belong in the generic PHY code unless we are very clear
>>>> on what we want to do, and how to do it, which I do not think we are
>>>> yet. What exactly is needed here:
>
>
>>>> - fixing up some design mistake?
>>>> - accounting for a specific board design?
>
>
>>> Kind of both. This was invented to defy the necessity of having
>>> platform
>>> fixup in the DT case (where there should be no board file to place it
>>> into).
>>> I have already described that platform fixup necessary on the Renesas
>>> Lager/Koelsch boards where the LED0 signat is connected to ETH_LINK
>>> signal
>>> on the SoC and the PHY reset sets the LED control bits to default 0 which
>>> means that LED0 will be LINK/ACTIVITY signal and thus blink on activity
>>> and
>>> cause ETH_LINK to bounce off/on after each packet.
>
>
>>>> In any case a PHY fixup would do the job for you.
>
>
>>> Not in any case. In case of DT we have no place for it, so should
>>> invent
>>> something involving DT.
>
>
>> How is DT different than any machine probing mechanism here?
>
>
> There supposed to be no board files. The purpose of DT is to get rid of
> the board files, at least on ARM.
>
>
>> The way to involve DT is to do the following:
>
>
>> if (of_machine_is_compatible("renesas,foo-board-with-broken-micrel-phy"))
>> phy_register_fixup(&foo_board_with_broken_micrel_phy);
>
>
> Where are you suggesting to place such code?
> arch/arm/mach-shmobile/setup-*.c?
Somewhere along those lines, I am not familiar at all with the SH
Mobile line of SoCs and how the DT/non-DT code used to look like.
Although I would be naively thinking that hooking this into the
init_machine() callback for the DT machine descriptor would do the
job.
>
>
>> If your machine compatible string does not allow you to uniquely
>> identify your machine, this is a DT problem, as this should really be
>> the case. If you do not want to add this code to wherever this is
>> relevant in arch/arm/mach-shmobile/board-*.c,
>
>
> There just should be no such file for DT case.
There is still a generic file which catches all SH Mobile machines and
registers some peripherals, as far as I look e.g; board-marzen.c that
one is still doing a bunch of platform_device_register_full() calls.
Even if the DT board file was extremely generic to the point where it
contains nothing, adding a custom init_machine() callback which
registers PHY fixups would not be too crazy.
>
>
>> neither is drivers/net/phy/phy_device.c this the place to add it.
>
>
> Hey, I wasn't arguing with that! :-)
>
>
>> Dealing with quirks applying to industry standard blocks is to update
>> the relevant driver, based on information provided by the specifically
>> affected systems. Failure to identify either of those correctly is a
>> problem that must not lead to a generic "let's override PHY registers
>> from DT" type of solution.
>
>
>> As usual, mechanism vs policy applies even more when DT is involved.
>
>
> Ah, so you're suggesting placing the fixup code in the driver itself?
> That's a bit strange for the platform specific code, but would do I guess...
PHY fixups are slightly different from say, traditional HW fixups,
what I meant here was that the general use case for quirks is:
- board code detects the faulty hardware and sets a flag that gets
passed down the relevant driver
- the relevant driver checks for this flag to enable such a thing
For the specific PHY devices, there are actually two ways to deal with this:
- register a PHY fixup somewhere (TBD where this somewhere is)
- set the phydev->dev_flags (just like what the TG3 driver does for instance
--
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
