Hello Simon,
On 9/27/18 3:41 PM, Simon Glass wrote:
> Hi Cedric,
>
> On 10 September 2018 at 07:21, Cédric Le Goater <c...@kaod.org> wrote:
>> The Faraday ftgmac100 MAC controllers as found on the Aspeed SoCs have
>> some slight differences in the HW interface (End-Of-Rx/Tx-Ring
>> bits). Also include the Aspeed clock enablement.
>>
>> Signed-off-by: Cédric Le Goater <c...@kaod.org>
>> ---
>> drivers/net/ftgmac100.h | 5 +++
>> drivers/net/ftgmac100.c | 72 +++++++++++++++++++++++++++++++++++++----
>> 2 files changed, 71 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/net/ftgmac100.h b/drivers/net/ftgmac100.h
>> index 9a789e4d5bee..b8f99ddf48bc 100644
>> --- a/drivers/net/ftgmac100.h
>> +++ b/drivers/net/ftgmac100.h
>> @@ -129,6 +129,11 @@ struct ftgmac100 {
>> #define FTGMAC100_DMAFIFOS_RXDMA_REQ BIT(30)
>> #define FTGMAC100_DMAFIFOS_TXDMA_REQ BIT(31)
>>
>> +/*
>> + * Feature Register
>> + */
>> +#define FTGMAC100_REVR_NEW_MDIO BIT(31)
>> +
>> /*
>> * Receive buffer size register
>> */
>> diff --git a/drivers/net/ftgmac100.c b/drivers/net/ftgmac100.c
>> index 8d7bf5b9b351..3df48a82c1ad 100644
>> --- a/drivers/net/ftgmac100.c
>> +++ b/drivers/net/ftgmac100.c
>> @@ -27,6 +27,8 @@
>> /* PKTBUFSTX/PKTBUFSRX must both be power of 2 */
>> #define PKTBUFSTX 4
>>
>> +#define FTGMAC100_ASPEED_NR_CLKS 2
>> +
>> struct ftgmac100_data {
>> phys_addr_t iobase;
>>
>> @@ -40,6 +42,11 @@ struct ftgmac100_data {
>
> Comments on struct and members
ok.
>
>> struct mii_dev *bus;
>> u32 phy_mode;
>> u32 max_speed;
>> +
>> + struct clk clks[FTGMAC100_ASPEED_NR_CLKS];
>> + u32 rxdes0_edorr_mask;
>> + u32 txdes0_edotr_mask;
>> + bool is_aspeed;
>> };
>>
>> /*
>> @@ -115,9 +122,15 @@ static int ftgmac100_mdio_write(struct mii_dev *bus,
>> int phy_addr, int dev_addr,
>>
>> static int ftgmac100_mdio_init(struct ftgmac100_data *priv, int dev_id)
>> {
>> + struct ftgmac100 *ftgmac100 = (struct ftgmac100 *)priv->iobase;
>> struct mii_dev *bus;
>> int ret;
>>
>> + if (priv->is_aspeed) {
>
> Perhaps call this old_mdio_if ?
Well, this feature is related to the Aspeed socs. The old MDIO interface is used
by default so I think we don't have to force the value below.
However, we can imagine selecting the mdio interface, new or old, through a DT
property. I will follow your suggestion then.
>> + /* This driver supports the old MDIO interface */
>> + clrbits_le32(&ftgmac100->revr, FTGMAC100_REVR_NEW_MDIO);
>> + };
>> +
>> bus = mdio_alloc();
>> if (!bus)
>> return -ENOMEM;
>> @@ -246,13 +259,13 @@ static int ftgmac100_start(struct udevice *dev)
>> priv->txdes[i].txdes3 = 0;
>> priv->txdes[i].txdes0 = 0;
>> }
>> - priv->txdes[PKTBUFSTX - 1].txdes0 = FTGMAC100_TXDES0_EDOTR;
>> + priv->txdes[PKTBUFSTX - 1].txdes0 = priv->txdes0_edotr_mask;
>>
>> for (i = 0; i < PKTBUFSRX; i++) {
>> priv->rxdes[i].rxdes3 = (unsigned int)net_rx_packets[i];
>> priv->rxdes[i].rxdes0 = 0;
>> }
>> - priv->rxdes[PKTBUFSRX - 1].rxdes0 = FTGMAC100_RXDES0_EDORR;
>> + priv->rxdes[PKTBUFSRX - 1].rxdes0 = priv->rxdes0_edorr_mask;
>>
>> /* transmit ring */
>> writel((u32)priv->txdes, &ftgmac100->txr_badr);
>> @@ -378,7 +391,7 @@ static int ftgmac100_send(struct udevice *dev, void
>> *packet, int length)
>> flush_dcache_range(data_start, data_end);
>>
>> /* only one descriptor on TXBUF */
>> - curr_des->txdes0 &= FTGMAC100_TXDES0_EDOTR;
>> + curr_des->txdes0 &= priv->txdes0_edotr_mask;
>> curr_des->txdes0 |= FTGMAC100_TXDES0_FTS |
>> FTGMAC100_TXDES0_LTS |
>> FTGMAC100_TXDES0_TXBUF_SIZE(length) |
>> @@ -409,8 +422,11 @@ static int ftgmac100_write_hwaddr(struct udevice *dev)
>>
>> static int ftgmac100_ofdata_to_platdata(struct udevice *dev)
>> {
>> + struct ftgmac100_data *priv = dev_get_priv(dev);
>> struct eth_pdata *pdata = dev_get_platdata(dev);
>> const char *phy_mode;
>> + int ret;
>> + int i;
>>
>> pdata->iobase = devfdt_get_addr(dev);
>> pdata->phy_interface = -1;
>> @@ -424,13 +440,39 @@ static int ftgmac100_ofdata_to_platdata(struct udevice
>> *dev)
>>
>> pdata->max_speed = dev_read_u32_default(dev, "max-speed", 0);
>>
>> + if (device_is_compatible(dev, "aspeed,ast2500-mac")) {
>
> Should use dev_get_driver_data() here.
OK. I see.
>
>> + priv->rxdes0_edorr_mask = BIT(30);
>> + priv->txdes0_edotr_mask = BIT(30);
>> + priv->is_aspeed = true;
>> + } else {
>> + priv->rxdes0_edorr_mask = BIT(15);
>> + priv->txdes0_edotr_mask = BIT(15);
>> + }
>> +
>> + if (priv->is_aspeed) {
>> + for (i = 0; i < FTGMAC100_ASPEED_NR_CLKS; i++) {
>> + ret = clk_get_by_index(dev, i, &priv->clks[i]);
>> + if (ret) {
>> + dev_err(dev, "Failed to get clock: %d\n",
>> ret);
>> + goto out_clk_free;
>> + }
>> + }
>
> clk_get_bulk() ?
I should help indeed. I will take a look.
>
>> + }
>> +
>> return 0;
>> +
>> +out_clk_free:
>> + while (--i >= 0)
>> + clk_free(&priv->clks[i]);
>> +
>> + return ret;
>> }
>>
>> static int ftgmac100_probe(struct udevice *dev)
>> {
>> struct eth_pdata *pdata = dev_get_platdata(dev);
>> struct ftgmac100_data *priv = dev_get_priv(dev);
>> + int i;
>> int ret;
>>
>> priv->iobase = pdata->iobase;
>> @@ -438,19 +480,33 @@ static int ftgmac100_probe(struct udevice *dev)
>> priv->max_speed = pdata->max_speed;
>> priv->phyaddr = 0;
>>
>> + if (priv->is_aspeed) {
>
> Why does this depend on aspeed? Can the DT specify what clocks are
> needed and how many?
yes. it does.
I should be able to make this part more generic with clk_get_bulk() and merge
it in the initial patch covering the whole ftgmac100 family. I only have access
to an Aspeed one.
>
>> + for (i = 0; i < FTGMAC100_ASPEED_NR_CLKS; i++) {
>
> clk_get_bulk() ?
>
> Should use device tree to get number of clocks.
>
>> + ret = clk_enable(&priv->clks[i]);
>> + if (ret) {
>> + dev_err(dev, "Failed to enable clock: %d\n",
>> + ret);
>> + goto out_clk_release;
>> + }
>> + }
>> + }
>> +
>> ret = ftgmac100_mdio_init(priv, dev->seq);
>> if (ret) {
>> dev_err(dev, "Failed to initialize mdiobus: %d\n", ret);
>> - goto out;
>> + goto out_clk_release;
>> }
>>
>> ret = ftgmac100_phy_init(priv, dev);
>> if (ret) {
>> dev_err(dev, "Failed to initialize PHY: %d\n", ret);
>> - goto out;
>> + goto out_clk_release;
>> }
>>
>> -out:
>> +out_clk_release:
>> + if (ret && priv->is_aspeed)
>> + clk_release_all(priv->clks, FTGMAC100_ASPEED_NR_CLKS);
>> +
>> return ret;
>> }
>>
>> @@ -462,6 +518,9 @@ static int ftgmac100_remove(struct udevice *dev)
>> mdio_unregister(priv->bus);
>> mdio_free(priv->bus);
>>
>> + if (priv->is_aspeed)
>> + clk_release_all(priv->clks, FTGMAC100_ASPEED_NR_CLKS);
>> +
>> return 0;
>> }
>>
>> @@ -476,6 +535,7 @@ static const struct eth_ops ftgmac100_ops = {
>>
>> static const struct udevice_id ftgmac100_ids[] = {
>> { .compatible = "faraday,ftgmac100" },
>> + { .compatible = "aspeed,ast2500-mac" },
>
> Need .data = ASPEED here, or something like that
ok.
Thanks for the review.
C.
>
>> { }
>> };
>>
>> --
>> 2.17.1
>>
>
> Regards,
> Simon
>
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
