| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 13 Feb 2014 10:29:55 +0000
From: Lee Jones <lee.jones@...aro.org>
To: Kishon Vijay Abraham I <kishon@...com>
Cc: linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
alexandre.torgue@...com
Subject: Re: [PATCH 4/4] phy: miphy365x: Provide support for the MiPHY356x
Generic PHY
> > The MiPHY365x is a Generic PHY which can serve various SATA or PCIe
> > devices. It has 2 ports which it can use for either; both SATA, both
>
> various SATA or PCIe devices in STMicroelectronics STiH41x SoC series?
To tell you the truth, I'm not sure if it is limited to ST's h/w, but
I think it can only be found there, so I'm happy to fixup.
<snip>
> > +config PHY_MIPHY365X
> > + tristate "STMicroelectronics MIPHY365X PHY driver for STiH41x series"
> > + depends on ARCH_STI
> > + depends on GENERIC_PHY
> depends on CONFIG_OF and HAS_IOMEM?
Sure, I'll fix.
<snip>
> > + * Copyright (C) 2014 STMicroelectronics
> > + *
> > + * STMicroelectronics PHY driver MiPHY365 (for SoC STiH416).
> > + *
> > + * Author: Alexandre Torgue <alexandre.torgue@...com>
>
> The author of this patch is not Alexandre Torgue?
The history of this driver is long and the authors are many. Alex
did the last internal over-haul and converted it to use Generic PHY. I
took Alex's driver and made significant changes in order to upstream.
<snip>
> > +#define HFC_TIMEOUT 50
> > +
> > +#define SYSCFG_2521 0x824
> > +#define SYSCFG_2522 0x828
> > +#define SYSCFG_PCIE_SATA_MASK BIT(1)
> > +#define SYSCFG_PCIE_SATA_POS 1
> > +
> > +/* MiPHY365x register definitiona */
> > +#define RESET_REG 0x00
> > +#define RST_PLL BIT(1)
>
> There are quite a few alignment problems with these macros. It needs
> to be fixed.
This is just Git playing up.
In reality everything is perfectly aligned and all using tabs.
<snip>
> > +/*
> > + * This function selects the system configuration,
> > + * either two SATA, one SATA and one PCIe, or two PCIe lanes.
> > + */
> > +static int miphy365x_set_path(struct miphy365x_phy *miphy_phy,
> > + struct miphy365x_dev *miphy_dev)
> > +{
> > + u8 config = miphy_phy->type | miphy_phy->port;
> > + u32 mask = SYSCFG_PCIE_SATA_MASK;
> > + u32 reg;
> > + bool sata;
> > +
> > + switch (config) {
> > + case MIPHY_SATA_PORT0:
> > + reg = SYSCFG_2521;
> > + sata = true;
>
> How do we configure PORT1 for SATA here? Do we really support all the system
> configuration?
Good spot eagle-eye!
Actually in the current version there is a h/w bug which only allows
the SATA_PORT0 and PCIE_PORT1 configuration. When a new version fixing
this is released I will add version detection to the driver and we can
support full intended configuration options.
<snip>
> > +static inline int miphy365x_phy_hfc_not_rdy(struct miphy365x_phy *miphy_phy,
> > + struct miphy365x_dev *miphy_dev)
> > +{
> > + int timeout = HFC_TIMEOUT;
> > + u8 mask = IDLL_RDY | PLL_RDY;
> > + u8 regval;
> > +
> > + do {
> > + regval = readb_relaxed(miphy_phy->base + STATUS_REG);
> > + usleep_range(2000, 2500);
>
> Any comment on how this delay value is obtained?
I don't have any specific comments, I believe the 2000us it taken from
the datasheet and the 2500 is us playing nice with the scheduler.
<snip>
> > +static inline int miphy365x_phy_rdy(struct miphy365x_phy *miphy_phy,
> > + struct miphy365x_dev *miphy_dev)
> > +{
> > + int timeout = HFC_TIMEOUT;
> > + u8 mask = mask = IDLL_RDY | PLL_RDY;
>
> just u8 mask = IDLL_RDY | PLL_RDY; would suffice.
Hmm... not sure how this slipped through - will fix.
> > + u8 regval;
> > +
> > + do {
> > + regval = readb_relaxed(miphy_phy->base + STATUS_REG);
> > + usleep_range(2000, 2500);
>
> same here.
As above.
<snip>
> > + mask = DES_BIT_LOCK | DES_SYMBOL_LOCK;
> > + while ((readb_relaxed(miphy_phy->base + COMP_CTRL1_REG) & mask) != mask)
> > + cpu_relax();
>
> Don't we need to break from here at some point if the LOCK's are never set?
I'm sure sure that's possible, but I will invesigate and fixup if req'd.
<snip>
> > +static int miphy365x_phy_power_on(struct phy *phy)
> > +{
> > + return 0;
> > +}
> > +
> > +static int miphy365x_phy_power_off(struct phy *phy)
> > +{
> > + return 0;
> > +}
>
> Both these empty functions can be removed.
You're right, I see the NULL checks, thanks.
<snip>
> > +static int miphy365x_phy_get_base_addr(struct platform_device *pdev,
> > + struct miphy365x_phy *phy, u8 port)
> > +{
> > + struct resource *res;
> > + char sata[16];
> > + char pcie[16];
>
> It can be done with a single variable ;-)
Right. :)
<snip>
> Phy provider register should be the last step in registering the PHY.
Okay, will fix, thanks.
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists