Am 25.07.24 um 08:43 schrieb Vinod Koul:
> On 20-07-24, 16:19, Josua Mayer wrote:
>> Armada 380 has similar USB-2.0 PHYs as CP-110. The differences are:
>> - register base addresses
>> - gap between port registers
>> - number of ports: 388 has three, cp110 two
>> - device-mode mux bit refers to different ports
>> - syscon register's base address (offsets identical)
>> - armada-8k uses syscon for various drivers, a38x not
>>
>> Differentiation uses of_match_data with distinct compatible strings.
>>
>> Add support for Armada 380 PHYs by partially restructuting the driver:
>> - Port register pointers are moved to the per-port private data.
>> - Add armada-38x-specific compatible string and store enum value in
>> of_match_data for differentiation.
>> - Add support for optional regs usb-cfg and utmi-cfg, to be used instead
>> of syscon.
>>
>> Signed-off-by: Josua Mayer <josua@xxxxxxxxxxxxx>
>> ---
>> drivers/phy/marvell/phy-mvebu-cp110-utmi.c | 209 +++++++++++++++++++++++------
>> 1 file changed, 166 insertions(+), 43 deletions(-)
>>
>> diff --git a/drivers/phy/marvell/phy-mvebu-cp110-utmi.c b/drivers/phy/marvell/phy-mvebu-cp110-utmi.c
>> index 4922a5f3327d..4341923e85bc 100644
>> --- a/drivers/phy/marvell/phy-mvebu-cp110-utmi.c
>> +++ b/drivers/phy/marvell/phy-mvebu-cp110-utmi.c
>> @@ -19,7 +19,7 @@
>> #include <linux/usb/of.h>
>> #include <linux/usb/otg.h>
>>
>> -#define UTMI_PHY_PORTS 2
>> +#define UTMI_PHY_PORTS 3
>>
>> /* CP110 UTMI register macro definetions */
>> #define SYSCON_USB_CFG_REG 0x420
>> @@ -76,32 +76,44 @@
>> #define PLL_LOCK_DELAY_US 10000
>> #define PLL_LOCK_TIMEOUT_US 1000000
>>
>> -#define PORT_REGS(p) ((p)->priv->regs + (p)->id * 0x1000)
>> +enum mvebu_cp110_utmi_type {
>> + /* 0 is reserved to avoid clashing with NULL */
>> + A380_UTMI = 1,
>> + CP110_UTMI = 2,
>> +};
>> +
>> +struct mvebu_cp110_utmi_port;
> why forward declare and not move the structs instead?
Seemed like the smaller change / more readable as diff.
I can move the struct instead!
>
>>
>> /**
>> * struct mvebu_cp110_utmi - PHY driver data
>> *
>> - * @regs: PHY registers
>> + * @regs_usb: USB configuration register
>> * @syscon: Regmap with system controller registers
>> * @dev: device driver handle
>> * @ops: phy ops
>> + * @ports: phy object for each port
>> */
>> struct mvebu_cp110_utmi {
>> - void __iomem *regs;
>> + void __iomem *regs_usb;
>> struct regmap *syscon;
>> struct device *dev;
>> const struct phy_ops *ops;
>> + struct mvebu_cp110_utmi_port *ports[UTMI_PHY_PORTS];
>> };
>>
>> /**
>> * struct mvebu_cp110_utmi_port - PHY port data
>> *
>> + * @regs: PHY registers
>> + * @regs_cfg: PHY config register
>> * @priv: PHY driver data
>> * @id: PHY port ID
>> * @dr_mode: PHY connection: USB_DR_MODE_HOST or USB_DR_MODE_PERIPHERAL
>> */
>> struct mvebu_cp110_utmi_port {
>> struct mvebu_cp110_utmi *priv;
>> + void __iomem *regs;
>> + void __iomem *regs_cfg;
>> u32 id;
>> enum usb_dr_mode dr_mode;
>> };
>> @@ -118,47 +130,47 @@ static void mvebu_cp110_utmi_port_setup(struct mvebu_cp110_utmi_port *port)
>> * The crystal used for all platform boards is now 25MHz.
>> * See the functional specification for details.
>> */
>> - reg = readl(PORT_REGS(port) + UTMI_PLL_CTRL_REG);
>> + reg = readl(port->regs + UTMI_PLL_CTRL_REG);
> why not handle this as a preparatory patch for this? Helps in review
Will do!
>
>> reg &= ~(PLL_REFDIV_MASK | PLL_FBDIV_MASK | PLL_SEL_LPFR_MASK);
>> reg |= (PLL_REFDIV_VAL << PLL_REFDIV_OFFSET) |
>> (PLL_FBDIV_VAL << PLL_FBDIV_OFFSET);
>> - writel(reg, PORT_REGS(port) + UTMI_PLL_CTRL_REG);
>> + writel(reg, port->regs + UTMI_PLL_CTRL_REG);
>>
>> /* Impedance Calibration Threshold Setting */
>> - reg = readl(PORT_REGS(port) + UTMI_CAL_CTRL_REG);
>> + reg = readl(port->regs + UTMI_CAL_CTRL_REG);
>> reg &= ~IMPCAL_VTH_MASK;
>> reg |= IMPCAL_VTH_VAL << IMPCAL_VTH_OFFSET;
>> - writel(reg, PORT_REGS(port) + UTMI_CAL_CTRL_REG);
>> + writel(reg, port->regs + UTMI_CAL_CTRL_REG);
>>
>> /* Set LS TX driver strength coarse control */
>> - reg = readl(PORT_REGS(port) + UTMI_TX_CH_CTRL_REG);
>> + reg = readl(port->regs + UTMI_TX_CH_CTRL_REG);
>> reg &= ~TX_AMP_MASK;
>> reg |= TX_AMP_VAL << TX_AMP_OFFSET;
>> - writel(reg, PORT_REGS(port) + UTMI_TX_CH_CTRL_REG);
>> + writel(reg, port->regs + UTMI_TX_CH_CTRL_REG);
>>
>> /* Disable SQ and enable analog squelch detect */
>> - reg = readl(PORT_REGS(port) + UTMI_RX_CH_CTRL0_REG);
>> + reg = readl(port->regs + UTMI_RX_CH_CTRL0_REG);
>> reg &= ~SQ_DET_EN;
>> reg |= SQ_ANA_DTC_SEL;
>> - writel(reg, PORT_REGS(port) + UTMI_RX_CH_CTRL0_REG);
>> + writel(reg, port->regs + UTMI_RX_CH_CTRL0_REG);
>>
>> /*
>> * Set External squelch calibration number and
>> * enable the External squelch calibration
>> */
>> - reg = readl(PORT_REGS(port) + UTMI_RX_CH_CTRL1_REG);
>> + reg = readl(port->regs + UTMI_RX_CH_CTRL1_REG);
>> reg &= ~SQ_AMP_CAL_MASK;
>> reg |= (SQ_AMP_CAL_VAL << SQ_AMP_CAL_OFFSET) | SQ_AMP_CAL_EN;
>> - writel(reg, PORT_REGS(port) + UTMI_RX_CH_CTRL1_REG);
>> + writel(reg, port->regs + UTMI_RX_CH_CTRL1_REG);
>>
>> /*
>> * Set Control VDAT Reference Voltage - 0.325V and
>> * Control VSRC Reference Voltage - 0.6V
>> */
>> - reg = readl(PORT_REGS(port) + UTMI_CHGDTC_CTRL_REG);
>> + reg = readl(port->regs + UTMI_CHGDTC_CTRL_REG);
>> reg &= ~(VDAT_MASK | VSRC_MASK);
>> reg |= (VDAT_VAL << VDAT_OFFSET) | (VSRC_VAL << VSRC_OFFSET);
>> - writel(reg, PORT_REGS(port) + UTMI_CHGDTC_CTRL_REG);
>> + writel(reg, port->regs + UTMI_CHGDTC_CTRL_REG);
>> }
>>
>> static int mvebu_cp110_utmi_phy_power_off(struct phy *phy)
>> @@ -166,22 +178,38 @@ static int mvebu_cp110_utmi_phy_power_off(struct phy *phy)
>> struct mvebu_cp110_utmi_port *port = phy_get_drvdata(phy);
>> struct mvebu_cp110_utmi *utmi = port->priv;
>> int i;
>> + int reg;
>>
>> /* Power down UTMI PHY port */
>> - regmap_clear_bits(utmi->syscon, SYSCON_UTMI_CFG_REG(port->id),
>> - UTMI_PHY_CFG_PU_MASK);
>> + if (!IS_ERR(port->regs_cfg)) {
>> + reg = readl(port->regs_cfg);
>> + reg &= ~(UTMI_PHY_CFG_PU_MASK);
>> + writel(reg, port->regs_cfg);
>> + } else
>> + regmap_clear_bits(utmi->syscon, SYSCON_UTMI_CFG_REG(port->id),
>> + UTMI_PHY_CFG_PU_MASK);
> why are we doing both raw register read/write and regmap ops... that
> does not sound correct to me
Indeed.
The next question would be why for Armada 8K / CP110 syscon was chosen.
