[PATCH v2 4/7] staging: mt7621-pci: fix reset lines for each pcie port

[NeilBrown]

On Tue, Nov 27 2018, Dan Carpenter wrote:

> On Mon, Nov 26, 2018 at 08:57:09PM +0100, Sergio Paracuellos wrote:
>> On Mon, Nov 26, 2018 at 10:57 AM Dan Carpenter <dan.carpenter at oracle.com> wrote:
>> >
>> > On Sat, Nov 24, 2018 at 06:54:54PM +0100, Sergio Paracuellos wrote:
>> > > Depending of chip revision reset lines are inverted. It is also
>> > > necessary to read PCIE_FTS_NUM register before enabling the phy.
>> > > Hence update the code to achieve this.
>> > >
>> > > Fixes: 745eeeac68d7: "staging: mt7621-pci: factor out 'mt7621_pcie_enable_port'
>> > > function"
>> > > Reported-by: NeilBrown <neil at brown.name>
>> > >
>> > > Signed-off-by: Sergio Paracuellos <sergio.paracuellos at gmail.com>
>> > > ---
>> > >  drivers/staging/mt7621-pci/pci-mt7621.c | 38 +++++++++++++++++++++----
>> > >  1 file changed, 32 insertions(+), 6 deletions(-)
>> > >
>> > > diff --git a/drivers/staging/mt7621-pci/pci-mt7621.c b/drivers/staging/mt7621-pci/pci-mt7621.c
>> > > index ba81b34dc1b7..1b63706e129b 100644
>> > > --- a/drivers/staging/mt7621-pci/pci-mt7621.c
>> > > +++ b/drivers/staging/mt7621-pci/pci-mt7621.c
>> > > @@ -412,6 +412,33 @@ static void mt7621_enable_phy(struct mt7621_pcie_port *port)
>> > >       set_phy_for_ssc(port);
>> > >  }
>> > >
>> > > +static inline void mt7621_control_assert(struct mt7621_pcie_port *port)
>> > > +{
>> > > +     u32 chip_rev_id = rt_sysc_r32(MT7621_CHIP_REV_ID);
>> > > +
>> > > +     if ((chip_rev_id & 0xFFFF) == CHIP_REV_MT7621_E2)
>> > > +             reset_control_assert(port->pcie_rst);
>> > > +     else
>> > > +             reset_control_deassert(port->pcie_rst);
>> > > +}
>> > > +
>> > > +static inline void mt7621_control_deassert(struct mt7621_pcie_port *port)
>> > > +{
>> > > +     u32 chip_rev_id = rt_sysc_r32(MT7621_CHIP_REV_ID);
>> > > +
>> > > +     if ((chip_rev_id & 0xFFFF) == CHIP_REV_MT7621_E2)
>> > > +             reset_control_deassert(port->pcie_rst);
>> > > +     else
>> > > +             reset_control_assert(port->pcie_rst);
>> > > +}
>> >
>> > The commit message is very good that on some chips assert and deassert
>> > mean the opposite but I feel like this should be commented in the code
>> > as well or people reading this code will be very confused.
>> >
>> 
>> Ok, Dan. Agreed. I will add some comment in next series.
>> 
>> > Also it would be better if we could change this from a white list to a
>> > black list.  In other words, if they were to come out with new revs
>> > of the hardware, we should assume that assert means assert and deassert
>> > means deassert.
>> 
>> I understand what you are saying but I don't know how to handle those
>> white and black lists... Is there some kind of example where I can
>> take a look to handle this in a proper way?
>> 
>
> What I mean is flip it around so that it becomes:
>
> static inline void mt7621_control_deassert(struct mt7621_pcie_port *port)
> {
> 	u32 chip_rev_id = rt_sysc_r32(MT7621_CHIP_REV_ID);
>
> 	/* Some revisions have assert and deassert reversed.  */
> 	if (chip_in_list_of_reversed_revs) {

Unfortunately we don't have that list.
All we have comes from:
 https://github.com/mqmaker/linux/blob/master/arch/mips/ralink/pci.c#L97
which suggests that any MT7621 RAlink pci controller that doesn't have 0x0101 at
the end of the chip_id has inverted PCI resets.
We don't have a list of all MT7621 CHIP_IDs to produce a blacklist from.

The only other info we have is that 0x030103 is one particular MT7621
CHIP_ID that is inverted (the one that I own).

So while I agree that a black-list would be best, that facts are that we
don't have one.

Thanks,
NeilBrown


> 		reset_control_assert(port->pcie_rst);
> 		return;
> 	}
>
> 	reset_control_deassert(port->pcie_rst);
> }
>
> regards,
> dan carpenter
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/attachments/20181127/959ff271/attachment.asc>