Hi Sekhar,
[...]
>>> Also, can you keep this as a platform variable (with
>>> a 2.2 MHz default)? The frequency depends on the board,
>>> and although most boards work at 2.2 MHz, not having it
>>> as a platform variable will make adding a board with a
>>> different frequency requirement difficult.
>>
>> I am not quite convinced that you'll have too many boards deviate from
>> the 2MHz ballpark. That seems to be a nice and safe frequency that
>> works well across phys, socs, and boards.
>>
>> That said, if we see the need to override the bus frequency in future, I
>> am all for a patch at that time. As it stands, I don't see the value in
>> adding platform data definitions for a capability that is not going to
>> be used at present.
>
> Okay, but why regress on existing functionality?
I don't quite think that the prior existence of a "knob" is reason
enough to keep it around, considering that the knob is essentially
unused (below).
> That frequency is a function of board design anyway so having the
> driver hardcode that would not be correct.
The frequency _limit_ is a function of board design. However, if these
effects were being factored in, we would have had the mdio bus operating
at different frequencies on different boards.
This is clearly not the case at present. Every board defines the same
magic 2.2 MHz frequency - a "safe" relatively low frequency that works
well across boards.
In short, we currently have a knob for frequency control, but we don't
use board-specific numbers from characterization data (if available).
That said, what good is the knob? All that it does at present, is muddy
up the code with identical definitions on every board.
If anyone (in future) were to take up the effort of analytically
determining frequencies based on characterization data, adding a
minuscule piece of platform data code would likely be the very least of
his worries :-)
[...]
>>>> + struct {
>>>> + u32 access;
>>>> +#define USERACCESS_GO BIT(31)
>>>> +#define USERACCESS_WRITE BIT(30)
>>>> +#define USERACCESS_ACK BIT(29)
>>>> +#define USERACCESS_READ (0)
>>>> +#define USERACCESS_DATA (0xffff)
>>>> +
>>>> + u32 physel;
>>>> + } user[0];
>>>> +};
>>>
>>> Structure overlays are usually not preferred and
>>> is difficult to debug. Can you please keep the
>>> direct register access method that was being
>>> followed in the original driver?
>>
>> Agreed, will change since the multiple user modules are not being used
>> anyway.
>
> Hmm, just curious how is this related to 'multiple user (mdio?) modules'?
Initially, I considered the option of having the MDIO hardware monitor
the link status for "attached" phys, by programming the physel
registers. For this, the mdio control register's highest-user-channel
field defines the number of user channels available. Therefore, the
number of user channels could vary across socs - leading to this kind of
a data structure.
This whole approach turned out to be a bit of a problem because the
phydev layer needs raw mdio interrupts and not "link state change"
interrupts. Further, the mdio module interrupts are not necessarily
hooked up (on at least some socs).
Regards
Cyril.
_______________________________________________
Davinci-linux-open-source mailing list
[email protected]
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
