On Tue, Jan 26, 2021 at 08:33:37AM +0100, Mike Looijmans wrote:
> The mdio_bus reset code first de-asserted the reset by allocating with
> GPIOD_OUT_LOW, then asserted and de-asserted again. In other words, if
> the reset signal defaulted to asserted, there'd be a short "spike"
> before the reset.
>
> Instead, directly assert the reset signal using GPIOD_OUT_HIGH, this
> removes the spike and also removes a line of code since the signal
> is already high.
Hi Mike
This however appears to remove the reset pulse, if the reset line was
already low to start with. Notice you left
fsleep(bus->reset_delay_us);
without any action before it? What are we now waiting for? Most data
sheets talk of a reset pulse. Take the reset line high, wait for some
time, take the reset low, wait for some time, and then start talking
to the PHY. I think with this patch, we have lost the guarantee of a
low to high transition.
Is this spike, followed by a pulse actually causing you problems? If
so, i would actually suggest adding another delay, to stretch the
spike. We have no control over the initial state of the reset line, it
is how the bootloader left it, we have to handle both states.
Andrew
On Tue, Jan 26, 2021 at 02:14:40PM +0100, Andrew Lunn wrote:
> On Tue, Jan 26, 2021 at 08:33:37AM +0100, Mike Looijmans wrote:
> > The mdio_bus reset code first de-asserted the reset by allocating with
> > GPIOD_OUT_LOW, then asserted and de-asserted again. In other words, if
> > the reset signal defaulted to asserted, there'd be a short "spike"
> > before the reset.
> >
> > Instead, directly assert the reset signal using GPIOD_OUT_HIGH, this
> > removes the spike and also removes a line of code since the signal
> > is already high.
>
> Hi Mike
>
> This however appears to remove the reset pulse, if the reset line was
> already low to start with. Notice you left
>
> fsleep(bus->reset_delay_us);
>
> without any action before it? What are we now waiting for? Most data
> sheets talk of a reset pulse. Take the reset line high, wait for some
> time, take the reset low, wait for some time, and then start talking
> to the PHY. I think with this patch, we have lost the guarantee of a
> low to high transition.
>
> Is this spike, followed by a pulse actually causing you problems? If
> so, i would actually suggest adding another delay, to stretch the
> spike. We have no control over the initial state of the reset line, it
> is how the bootloader left it, we have to handle both states.
Andrew, I don't get what you're saying.
Here is what happens depending on the pre-existing state of the
reset signal:
Reset (previously asserted): ~~~|_|~~~~|_______
Reset (previously deasserted): _____|~~~~|_______
^ ^ ^
A B C
At point A, the low going transition is because the reset line is
requested using GPIOD_OUT_LOW. If the line is successfully requested,
the first thing we do is set it high _without_ any delay. This is
point B. So, a glitch occurs between A and B.
We then fsleep() and finally set the GPIO low at point C.
Requesting the line using GPIOD_OUT_HIGH eliminates the A and B
transitions. Instead we get:
Reset (previously asserted) : ~~~~~~~~~~|______
Reset (previously deasserted): ____|~~~~~|______
^ ^
A C
Where A and C are the points described above in the code. Point B
has been eliminated.
Therefore, to me the patch looks entirely reasonable and correct.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
See below.
Met vriendelijke groet / kind regards,
Mike Looijmans
System Expert
TOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands
T: +31 (0) 499 33 69 69
E: [email protected]
W: http://www.topicproducts.com
Please consider the environment before printing this e-mail
On 26-01-2021 14:49, Russell King - ARM Linux admin wrote:
> On Tue, Jan 26, 2021 at 02:14:40PM +0100, Andrew Lunn wrote:
>> On Tue, Jan 26, 2021 at 08:33:37AM +0100, Mike Looijmans wrote:
>>> The mdio_bus reset code first de-asserted the reset by allocating with
>>> GPIOD_OUT_LOW, then asserted and de-asserted again. In other words, if
>>> the reset signal defaulted to asserted, there'd be a short "spike"
>>> before the reset.
>>>
>>> Instead, directly assert the reset signal using GPIOD_OUT_HIGH, this
>>> removes the spike and also removes a line of code since the signal
>>> is already high.
>> Hi Mike
>>
>> This however appears to remove the reset pulse, if the reset line was
>> already low to start with. Notice you left
>>
>> fsleep(bus->reset_delay_us);
>>
>> without any action before it? What are we now waiting for? Most data
>> sheets talk of a reset pulse. Take the reset line high, wait for some
>> time, take the reset low, wait for some time, and then start talking
>> to the PHY. I think with this patch, we have lost the guarantee of a
>> low to high transition.
>>
>> Is this spike, followed by a pulse actually causing you problems? If
>> so, i would actually suggest adding another delay, to stretch the
>> spike. We have no control over the initial state of the reset line, it
>> is how the bootloader left it, we have to handle both states.
> Andrew, I don't get what you're saying.
>
> Here is what happens depending on the pre-existing state of the
> reset signal:
>
> Reset (previously asserted): ~~~|_|~~~~|_______
> Reset (previously deasserted): _____|~~~~|_______
> ^ ^ ^
> A B C
>
> At point A, the low going transition is because the reset line is
> requested using GPIOD_OUT_LOW. If the line is successfully requested,
> the first thing we do is set it high _without_ any delay. This is
> point B. So, a glitch occurs between A and B.
>
> We then fsleep() and finally set the GPIO low at point C.
>
> Requesting the line using GPIOD_OUT_HIGH eliminates the A and B
> transitions. Instead we get:
>
> Reset (previously asserted) : ~~~~~~~~~~|______
> Reset (previously deasserted): ____|~~~~~|______
> ^ ^
> A C
>
> Where A and C are the points described above in the code. Point B
> has been eliminated.
>
> Therefore, to me the patch looks entirely reasonable and correct.
>
Thanks, excellent explanation.
As a bit of background, we were using a Marvell PHY where the datasheet
states that thou shallt not release the reset within 50 ms of power-up.
A pull-down on the active-low reset was thus added. Looking at the reset
signal with a scope revealed a short spike, visible only because it was
being controlled by an I2C GPIO expander. So it's indeed point "B" that
we wanted to eliminate.
--
Mike Looijmans
On Wed, 27 Jan 2021 08:08:29 +0100 Mike Looijmans wrote:
> > Andrew, I don't get what you're saying.
> >
> > Here is what happens depending on the pre-existing state of the
> > reset signal:
> >
> > Reset (previously asserted): ~~~|_|~~~~|_______
> > Reset (previously deasserted): _____|~~~~|_______
> > ^ ^ ^
> > A B C
> >
> > At point A, the low going transition is because the reset line is
> > requested using GPIOD_OUT_LOW. If the line is successfully requested,
> > the first thing we do is set it high _without_ any delay. This is
> > point B. So, a glitch occurs between A and B.
> >
> > We then fsleep() and finally set the GPIO low at point C.
> >
> > Requesting the line using GPIOD_OUT_HIGH eliminates the A and B
> > transitions. Instead we get:
> >
> > Reset (previously asserted) : ~~~~~~~~~~|______
> > Reset (previously deasserted): ____|~~~~~|______
> > ^ ^
> > A C
> >
> > Where A and C are the points described above in the code. Point B
> > has been eliminated.
> >
> > Therefore, to me the patch looks entirely reasonable and correct.
> >
> Thanks, excellent explanation.
>
> As a bit of background, we were using a Marvell PHY where the datasheet
> states that thou shallt not release the reset within 50 ms of power-up.
> A pull-down on the active-low reset was thus added. Looking at the reset
> signal with a scope revealed a short spike, visible only because it was
> being controlled by an I2C GPIO expander. So it's indeed point "B" that
> we wanted to eliminate.
This is all useful information - can we roll more of it into the commit
message? I'd think that calling out the part and the 50ms value could
make things more "concrete" for a reader down the line?
On Tue, Jan 26, 2021 at 01:49:38PM +0000, Russell King - ARM Linux admin wrote:
> On Tue, Jan 26, 2021 at 02:14:40PM +0100, Andrew Lunn wrote:
> > On Tue, Jan 26, 2021 at 08:33:37AM +0100, Mike Looijmans wrote:
> > > The mdio_bus reset code first de-asserted the reset by allocating with
> > > GPIOD_OUT_LOW, then asserted and de-asserted again. In other words, if
> > > the reset signal defaulted to asserted, there'd be a short "spike"
> > > before the reset.
> > >
> > > Instead, directly assert the reset signal using GPIOD_OUT_HIGH, this
> > > removes the spike and also removes a line of code since the signal
> > > is already high.
> >
> > Hi Mike
> >
> > This however appears to remove the reset pulse, if the reset line was
> > already low to start with. Notice you left
> >
> > fsleep(bus->reset_delay_us);
> >
> > without any action before it? What are we now waiting for? Most data
> > sheets talk of a reset pulse. Take the reset line high, wait for some
> > time, take the reset low, wait for some time, and then start talking
> > to the PHY. I think with this patch, we have lost the guarantee of a
> > low to high transition.
> >
> > Is this spike, followed by a pulse actually causing you problems? If
> > so, i would actually suggest adding another delay, to stretch the
> > spike. We have no control over the initial state of the reset line, it
> > is how the bootloader left it, we have to handle both states.
>
> Andrew, I don't get what you're saying.
>
> Here is what happens depending on the pre-existing state of the
> reset signal:
>
> Reset (previously asserted): ~~~|_|~~~~|_______
> Reset (previously deasserted): _____|~~~~|_______
> ^ ^ ^
> A B C
>
> At point A, the low going transition is because the reset line is
> requested using GPIOD_OUT_LOW. If the line is successfully requested,
> the first thing we do is set it high _without_ any delay. This is
> point B. So, a glitch occurs between A and B.
>
> We then fsleep() and finally set the GPIO low at point C.
>
> Requesting the line using GPIOD_OUT_HIGH eliminates the A and B
> transitions. Instead we get:
>
> Reset (previously asserted) : ~~~~~~~~~~|______
> Reset (previously deasserted): ____|~~~~~|______
> ^ ^
> A C
>
> Where A and C are the points described above in the code. Point B
> has been eliminated.
>
> Therefore, to me the patch looks entirely reasonable and correct.
I wonder if there are any PHYs which actually need a pulse? Would it
be better to have:
Reset (previously asserted): ~~~|____|~~~~|_______
Reset (previously deasserted): ________|~~~~|_______
^ ^ ^ ^
A B C D
Point D is where we actually start talking to the PHY. C-D is
reset-post-delay-us, and defaults to 0, but can be set via DT. B-C is
reset-delay-us, and defaults to 10us, but can be set via DT.
Currently A-B is '0', so we get the glitch. But should we make A-B the
same as B-C, so we get a real pulse?
Andrew
On Thu, Jan 28, 2021 at 01:00:57AM +0100, Andrew Lunn wrote:
> On Tue, Jan 26, 2021 at 01:49:38PM +0000, Russell King - ARM Linux admin wrote:
> > On Tue, Jan 26, 2021 at 02:14:40PM +0100, Andrew Lunn wrote:
> > > On Tue, Jan 26, 2021 at 08:33:37AM +0100, Mike Looijmans wrote:
> > > > The mdio_bus reset code first de-asserted the reset by allocating with
> > > > GPIOD_OUT_LOW, then asserted and de-asserted again. In other words, if
> > > > the reset signal defaulted to asserted, there'd be a short "spike"
> > > > before the reset.
> > > >
> > > > Instead, directly assert the reset signal using GPIOD_OUT_HIGH, this
> > > > removes the spike and also removes a line of code since the signal
> > > > is already high.
> > >
> > > Hi Mike
> > >
> > > This however appears to remove the reset pulse, if the reset line was
> > > already low to start with. Notice you left
> > >
> > > fsleep(bus->reset_delay_us);
> > >
> > > without any action before it? What are we now waiting for? Most data
> > > sheets talk of a reset pulse. Take the reset line high, wait for some
> > > time, take the reset low, wait for some time, and then start talking
> > > to the PHY. I think with this patch, we have lost the guarantee of a
> > > low to high transition.
> > >
> > > Is this spike, followed by a pulse actually causing you problems? If
> > > so, i would actually suggest adding another delay, to stretch the
> > > spike. We have no control over the initial state of the reset line, it
> > > is how the bootloader left it, we have to handle both states.
> >
> > Andrew, I don't get what you're saying.
> >
> > Here is what happens depending on the pre-existing state of the
> > reset signal:
> >
> > Reset (previously asserted): ~~~|_|~~~~|_______
> > Reset (previously deasserted): _____|~~~~|_______
> > ^ ^ ^
> > A B C
> >
> > At point A, the low going transition is because the reset line is
> > requested using GPIOD_OUT_LOW. If the line is successfully requested,
> > the first thing we do is set it high _without_ any delay. This is
> > point B. So, a glitch occurs between A and B.
> >
> > We then fsleep() and finally set the GPIO low at point C.
> >
> > Requesting the line using GPIOD_OUT_HIGH eliminates the A and B
> > transitions. Instead we get:
> >
> > Reset (previously asserted) : ~~~~~~~~~~|______
> > Reset (previously deasserted): ____|~~~~~|______
> > ^ ^
> > A C
> >
> > Where A and C are the points described above in the code. Point B
> > has been eliminated.
> >
> > Therefore, to me the patch looks entirely reasonable and correct.
>
> I wonder if there are any PHYs which actually need a pulse? Would it
> be better to have:
>
> Reset (previously asserted): ~~~|____|~~~~|_______
> Reset (previously deasserted): ________|~~~~|_______
> ^ ^ ^ ^
> A B C D
>
> Point D is where we actually start talking to the PHY. C-D is
> reset-post-delay-us, and defaults to 0, but can be set via DT. B-C is
> reset-delay-us, and defaults to 10us, but can be set via DT.
> Currently A-B is '0', so we get the glitch. But should we make A-B the
> same as B-C, so we get a real pulse?
I do not see any need for A-B - what is the reason for it? You will
find most datasheets talk about a clock must be active for some number
of clock cycles prior to the reset signal being released, or minimum
delay after power up before reset is released, or talking about a
minimum pulse width.
Note that looking at a few of the Marvell PHY datasheets, they require
a minimum reset pulse width of 10ms and between 5ms and 50ms before
the first access. AR8035 also talks about 10ms.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On Thu, Jan 28, 2021 at 12:25:55AM +0000, Russell King - ARM Linux admin wrote:
> On Thu, Jan 28, 2021 at 01:00:57AM +0100, Andrew Lunn wrote:
> > On Tue, Jan 26, 2021 at 01:49:38PM +0000, Russell King - ARM Linux admin wrote:
> > > On Tue, Jan 26, 2021 at 02:14:40PM +0100, Andrew Lunn wrote:
> > > > On Tue, Jan 26, 2021 at 08:33:37AM +0100, Mike Looijmans wrote:
> > > > > The mdio_bus reset code first de-asserted the reset by allocating with
> > > > > GPIOD_OUT_LOW, then asserted and de-asserted again. In other words, if
> > > > > the reset signal defaulted to asserted, there'd be a short "spike"
> > > > > before the reset.
> > > > >
> > > > > Instead, directly assert the reset signal using GPIOD_OUT_HIGH, this
> > > > > removes the spike and also removes a line of code since the signal
> > > > > is already high.
> > > >
> > > > Hi Mike
> > > >
> > > > This however appears to remove the reset pulse, if the reset line was
> > > > already low to start with. Notice you left
> > > >
> > > > fsleep(bus->reset_delay_us);
> > > >
> > > > without any action before it? What are we now waiting for? Most data
> > > > sheets talk of a reset pulse. Take the reset line high, wait for some
> > > > time, take the reset low, wait for some time, and then start talking
> > > > to the PHY. I think with this patch, we have lost the guarantee of a
> > > > low to high transition.
> > > >
> > > > Is this spike, followed by a pulse actually causing you problems? If
> > > > so, i would actually suggest adding another delay, to stretch the
> > > > spike. We have no control over the initial state of the reset line, it
> > > > is how the bootloader left it, we have to handle both states.
> > >
> > > Andrew, I don't get what you're saying.
> > >
> > > Here is what happens depending on the pre-existing state of the
> > > reset signal:
> > >
> > > Reset (previously asserted): ~~~|_|~~~~|_______
> > > Reset (previously deasserted): _____|~~~~|_______
> > > ^ ^ ^
> > > A B C
> > >
> > > At point A, the low going transition is because the reset line is
> > > requested using GPIOD_OUT_LOW. If the line is successfully requested,
> > > the first thing we do is set it high _without_ any delay. This is
> > > point B. So, a glitch occurs between A and B.
> > >
> > > We then fsleep() and finally set the GPIO low at point C.
> > >
> > > Requesting the line using GPIOD_OUT_HIGH eliminates the A and B
> > > transitions. Instead we get:
> > >
> > > Reset (previously asserted) : ~~~~~~~~~~|______
> > > Reset (previously deasserted): ____|~~~~~|______
> > > ^ ^
> > > A C
> > >
> > > Where A and C are the points described above in the code. Point B
> > > has been eliminated.
> > >
> > > Therefore, to me the patch looks entirely reasonable and correct.
> >
> > I wonder if there are any PHYs which actually need a pulse? Would it
> > be better to have:
> >
> > Reset (previously asserted): ~~~|____|~~~~|_______
> > Reset (previously deasserted): ________|~~~~|_______
> > ^ ^ ^ ^
> > A B C D
> >
> > Point D is where we actually start talking to the PHY. C-D is
> > reset-post-delay-us, and defaults to 0, but can be set via DT. B-C is
> > reset-delay-us, and defaults to 10us, but can be set via DT.
> > Currently A-B is '0', so we get the glitch. But should we make A-B the
> > same as B-C, so we get a real pulse?
>
> I do not see any need for A-B - what is the reason for it?
If level is all that matters, then it is not needed. If a PHY needs an
actual pulse, both a raising and a falling edge, we potentially don't
get the rising edge now.
But the datasheets you have looked at all seem to talk about level,
not pulse. So lets go with this.
Reviewed-by: Andrew Lunn <[email protected]>
Andrew
Met vriendelijke groet / kind regards,
Mike Looijmans
System Expert
TOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands
T: +31 (0) 499 33 69 69
E: [email protected]
W: http://www.topicproducts.com
Please consider the environment before printing this e-mail
On 28-01-2021 02:12, Andrew Lunn wrote:
> On Thu, Jan 28, 2021 at 12:25:55AM +0000, Russell King - ARM Linux admin wrote:
>> On Thu, Jan 28, 2021 at 01:00:57AM +0100, Andrew Lunn wrote:
>>> On Tue, Jan 26, 2021 at 01:49:38PM +0000, Russell King - ARM Linux admin wrote:
>>>> On Tue, Jan 26, 2021 at 02:14:40PM +0100, Andrew Lunn wrote:
>>>>> On Tue, Jan 26, 2021 at 08:33:37AM +0100, Mike Looijmans wrote:
>>>>>> The mdio_bus reset code first de-asserted the reset by allocating with
>>>>>> GPIOD_OUT_LOW, then asserted and de-asserted again. In other words, if
>>>>>> the reset signal defaulted to asserted, there'd be a short "spike"
>>>>>> before the reset.
>>>>>>
>>>>>> Instead, directly assert the reset signal using GPIOD_OUT_HIGH, this
>>>>>> removes the spike and also removes a line of code since the signal
>>>>>> is already high.
>>>>> Hi Mike
>>>>>
>>>>> This however appears to remove the reset pulse, if the reset line was
>>>>> already low to start with. Notice you left
>>>>>
>>>>> fsleep(bus->reset_delay_us);
>>>>>
>>>>> without any action before it? What are we now waiting for? Most data
>>>>> sheets talk of a reset pulse. Take the reset line high, wait for some
>>>>> time, take the reset low, wait for some time, and then start talking
>>>>> to the PHY. I think with this patch, we have lost the guarantee of a
>>>>> low to high transition.
>>>>>
>>>>> Is this spike, followed by a pulse actually causing you problems? If
>>>>> so, i would actually suggest adding another delay, to stretch the
>>>>> spike. We have no control over the initial state of the reset line, it
>>>>> is how the bootloader left it, we have to handle both states.
>>>> Andrew, I don't get what you're saying.
>>>>
>>>> Here is what happens depending on the pre-existing state of the
>>>> reset signal:
>>>>
>>>> Reset (previously asserted): ~~~|_|~~~~|_______
>>>> Reset (previously deasserted): _____|~~~~|_______
>>>> ^ ^ ^
>>>> A B C
>>>>
>>>> At point A, the low going transition is because the reset line is
>>>> requested using GPIOD_OUT_LOW. If the line is successfully requested,
>>>> the first thing we do is set it high _without_ any delay. This is
>>>> point B. So, a glitch occurs between A and B.
>>>>
>>>> We then fsleep() and finally set the GPIO low at point C.
>>>>
>>>> Requesting the line using GPIOD_OUT_HIGH eliminates the A and B
>>>> transitions. Instead we get:
>>>>
>>>> Reset (previously asserted) : ~~~~~~~~~~|______
>>>> Reset (previously deasserted): ____|~~~~~|______
>>>> ^ ^
>>>> A C
>>>>
>>>> Where A and C are the points described above in the code. Point B
>>>> has been eliminated.
>>>>
>>>> Therefore, to me the patch looks entirely reasonable and correct.
>>> I wonder if there are any PHYs which actually need a pulse? Would it
>>> be better to have:
>>>
>>> Reset (previously asserted): ~~~|____|~~~~|_______
>>> Reset (previously deasserted): ________|~~~~|_______
>>> ^ ^ ^ ^
>>> A B C D
>>>
>>> Point D is where we actually start talking to the PHY. C-D is
>>> reset-post-delay-us, and defaults to 0, but can be set via DT. B-C is
>>> reset-delay-us, and defaults to 10us, but can be set via DT.
>>> Currently A-B is '0', so we get the glitch. But should we make A-B the
>>> same as B-C, so we get a real pulse?
>> I do not see any need for A-B - what is the reason for it?
> If level is all that matters, then it is not needed. If a PHY needs an
> actual pulse, both a raising and a falling edge, we potentially don't
> get the rising edge now.
We only caught the "spike" because the reset GPIO was controlled by a
GPIO expander, so it took about a millisecond to toggle it. With a
"local" GPIO controller, the pulse duration would be below the
microsecond range and most PHYs would never see it.
> But the datasheets you have looked at all seem to talk about level,
> not pulse. So lets go with this.
>
> Reviewed-by: Andrew Lunn <[email protected]>
>
> Andrew
Just wondering, now, a v2 patch isn't needed? Or should I amend the
commit text?
--
Mike Looijmans
> > Reviewed-by: Andrew Lunn <[email protected]>
> >
> > Andrew
>
> Just wondering, now, a v2 patch isn't needed? Or should I amend the commit
> text?
Hi Mike
Take a look in patchwork.kernel.org. It has been flaky the last few
days. If the patch is not there, you definitively need to repost. If
you do find it, check what state it is in. That will tell you if it
needs reposting.
Andrew