[linux-sunxi] Re: [PATCH 2/3] mmc: sunxi: Set the 'New Timing' register for 8 bits DDR transfers

[Jean-Francois Moine]

On Fri, 29 Jul 2016 21:36:34 +0200
Maxime Ripard  wrote:

> On Thu, Jul 21, 2016 at 11:26:55AM +0200, Jean-Francois Moine wrote:
> > On Thu, 21 Jul 2016 10:56:15 +0200
> > Maxime Ripard  wrote:
> > 
> > > On Wed, Jul 20, 2016 at 08:16:28PM +0200, Jean-Francois Moine wrote:
> > > > The 'new timing mode' with 8 bits DDR works correctly when the NewTiming
> > > > register is set.
> > > 
> > > What does that mode brings to the table?
> > 
> > From my tests, the eMMC of the Banana Pi M3 (A83T) cannot work when the
> > new mode is not used.
> 
> That's odd. The one in the Pine64 seems to work just fine, and yet
> there's only the new mode on the A64.

I spent 2 weeks on this problem. You may try it by yourself.

-- 
Ken ar c'hentañ | ** Breizh ha Linux atav! **
Jef |   http://moinejf.free.fr/

-- 
You received this message because you are subscribed to the Google Groups 
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to linux-sunxi+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[linux-sunxi] Re: [PATCH 2/3] mmc: sunxi: Set the 'New Timing' register for 8 bits DDR transfers

[Jean-Francois Moine]

On Fri, 29 Jul 2016 21:17:30 +0200
Maxime Ripard  wrote:

> > > What happens if you actually want to set it to 100MHz?
> > 
> > There is no SDXC_CLK_100M in the mainline driver, and 100MHz is asked
> > only for 8 bits DDR at 50MHz.
> 
> You're missing the point.
> 
> clk_set_rate is supposed to apply a rate as close as possible as
> requested, there's no reason why you would request a rate twice as
> high as need.
> 
> You want to switch the clock from one mode to another, fine, create a
> new function for that. But don't hack an existing one.

I will not change the core clock stuff for this marginal case.
Flagging the clock as 'new mode capable' is enough.
Anyway, setting the 'new mode' bit to the clock divides the rate by
two, so, there is no reason to not use it.

-- 
Ken ar c'hentañ | ** Breizh ha Linux atav! **
Jef |   http://moinejf.free.fr/

-- 
You received this message because you are subscribed to the Google Groups 
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to linux-sunxi+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[linux-sunxi] Re: [PATCH v2 1/5] ethernet: add sun8i-emac driver

[Chen-Yu Tsai]

On Sat, Jul 30, 2016 at 1:25 AM, Maxime Ripard
 wrote:
> On Thu, Jul 28, 2016 at 04:57:34PM +0200, LABBE Corentin wrote:
>> > > +static int sun8i_mdio_write(struct mii_bus *bus, int phy_addr, int 
>> > > phy_reg,
>> > > + u16 data)
>> > > +{
>> > > + struct net_device *ndev = bus->priv;
>> > > + struct sun8i_emac_priv *priv = netdev_priv(ndev);
>> > > + u32 reg;
>> > > + int err;
>> > > +
>> > > + err = readl_poll_timeout(priv->base + SUN8I_EMAC_MDIO_CMD, reg,
>> > > +  !(reg & MDIO_CMD_MII_BUSY), 100, 1);
>> > > + if (err) {
>> > > + dev_err(priv->dev, "%s timeout %x\n", __func__, reg);
>> > > + return err;
>> > > + }
>> >
>> > Why the poll_timeout variant?
>> >
>> Because, in case of bad clock/reset/regulator setting, the value
>> expected to come could never be set.
>
> Ah, I missed that it was for a busy bit, my bad. However, you seem to
> be using that on several occasions, maybe you could turn that into a
> function?
>
>> > > +static void sun8i_emac_unset_syscon(struct net_device *ndev)
>> > > +{
>> > > + struct sun8i_emac_priv *priv = netdev_priv(ndev);
>> > > + u32 reg = 0;
>> > > +
>> > > + if (priv->variant == H3_EMAC)
>> > > + reg = H3_EPHY_DEFAULT_VALUE;
>> >
>> > Why do you need that?
>> >
>> For resetting the syscon to the factory default.
>
> Yes, but does it matter? Does it have any side effect? Is that
> register shared with another device?
>
> Otherwise, either it won't be used anymore, and you don't care, or you
> will reload the driver later, and the driver should work whatever
> state is programmed in there. In both cases, you don't need to reset
> that value.

The "default" setting also disables and powers down the internal PHY.
I think that's a good thing? The naming could be better.

>> > > +static irqreturn_t sun8i_emac_dma_interrupt(int irq, void *dev_id)
>> > > +{
>> > > + struct net_device *ndev = dev_id;
>> > > + struct sun8i_emac_priv *priv = netdev_priv(ndev);
>> > > + u32 v, u;
>> > > +
>> > > + v = readl(priv->base + SUN8I_EMAC_INT_STA);
>> > > +
>> > > + /* When this bit is asserted, a frame transmission is completed. */
>> > > + if (v & BIT(0)) {
>> > > + priv->estats.tx_int++;
>> > > + writel(0, priv->base + SUN8I_EMAC_INT_EN);
>> > > + napi_schedule(>napi);
>> > > + }
>> > > +
>> > > + /* When this bit is asserted, the TX DMA FSM is stopped. */
>> > > + if (v & BIT(1))
>> > > + priv->estats.tx_dma_stop++;
>> > > +
>> > > + /* When this asserted, the TX DMA can not acquire next TX descriptor
>> > > +  * and TX DMA FSM is suspended.
>> > > + */
>> > > + if (v & BIT(2))
>> > > + priv->estats.tx_dma_ua++;
>> > > +
>> > > + if (v & BIT(3))
>> > > + netif_dbg(priv, intr, ndev, "Unhandled interrupt TX 
>> > > TIMEOUT\n");
>> >
>> > Why do you enable that interrupt if you can't handle it?
>>
>> Some interrupt fire even when not enabled (like RX_BUF_UA_INT/TX_BUF_UA_INT)
>
> So the bits 9 and 2, respectively, in the interrupt enable register
> are useless?

Does it actually fire, i.e. pull the interrupt line on the GIC? Or is it just
the interrupt state showing an event? IIRC some other hardware blocks have this
behavior, such as the timer.

ChenYu

>> > And printing in the interrupt handler is a very bad idea.
>>
>> There are printed only when DEBUG is set, so not a problem ?
>
> It's always a problem, this adds a very significant latency and will
> fill the kernel log buffer at an insane rate, flushing out actual
> important messages, for no particular reason.
>> > > +
>> > > + return IRQ_HANDLED;
>> >
>> > The lack of spinlocks in there is quite worrying.
>> >
>>
>> The interrupt handler just do nothing harmfull if it race with itself.
>> Just stats, enabling NAPI etc..
>> Anyway, It miss a comment for that non-locking strategy
>
> The interrupt handler cannot race with itself. The interrupts will be
> masked on the local CPU and the interrupt can only be delivered to a
> single CPU (so, the one that the handler is currently running from).
>
>> > > +}
>> > > +
>> > > +static int sun8i_emac_probe(struct platform_device *pdev)
>> > > +{
>> > > + struct device_node *node = pdev->dev.of_node;
>> > > + struct sun8i_emac_priv *priv;
>> > > + struct net_device *ndev;
>> > > + struct resource *res;
>> > > + int ret;
>> > > +
>> > > + ret = dma_set_mask_and_coherent(>dev, DMA_BIT_MASK(32));
>> > > + if (ret) {
>> > > + dev_err(>dev, "No suitable DMA available\n");
>> > > + return ret;
>> > > + }
>> >
>> > Isn't that the default?
>> >
>> No, it is necessary on arm64 as apritzel requested.
>
> http://lxr.free-electrons.com/source/drivers/of/device.c#L93
>
> It seems to be shared between the two.
>
> Thanks!
> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com

-- 
You received this message because you are subscribed to the Google Groups 
"linux-sunxi" 

[linux-sunxi] Re: [PATCH 2/3] mmc: sunxi: Set the 'New Timing' register for 8 bits DDR transfers

[Maxime Ripard]

On Thu, Jul 21, 2016 at 11:26:55AM +0200, Jean-Francois Moine wrote:
> On Thu, 21 Jul 2016 10:56:15 +0200
> Maxime Ripard  wrote:
> 
> > On Wed, Jul 20, 2016 at 08:16:28PM +0200, Jean-Francois Moine wrote:
> > > The 'new timing mode' with 8 bits DDR works correctly when the NewTiming
> > > register is set.
> > 
> > What does that mode brings to the table?
> 
> From my tests, the eMMC of the Banana Pi M3 (A83T) cannot work when the
> new mode is not used.

That's odd. The one in the Pine64 seems to work just fine, and yet
there's only the new mode on the A64.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

-- 
You received this message because you are subscribed to the Google Groups 
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to linux-sunxi+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: PGP signature

[linux-sunxi] Re: [PATCH 2/3] mmc: sunxi: Set the 'New Timing' register for 8 bits DDR transfers

[Maxime Ripard]

On Thu, Jul 21, 2016 at 11:26:55AM +0200, Jean-Francois Moine wrote:
> On Thu, 21 Jul 2016 10:56:15 +0200
> Maxime Ripard  wrote:
> 
> > On Wed, Jul 20, 2016 at 08:16:28PM +0200, Jean-Francois Moine wrote:
> > > The 'new timing mode' with 8 bits DDR works correctly when the NewTiming
> > > register is set.
> > 
> > What does that mode brings to the table?
> 
> From my tests, the eMMC of the Banana Pi M3 (A83T) cannot work when the
> new mode is not used.
> 
> > > 
> > > Signed-off-by: Jean-Francois Moine 
> > > ---
> > > Note about the 'new timing mode'.
> > > 
> > > This patch assumes that, when the new mode is used, the clock driver
> > > sets the mode select in the MMC clock and multiplies the clock rate
> > > by 2:
> > > - MMC side:
> > >   - with a timing 8 bits DDR at 50MHz, the MMC driver calls
> > > clk_set_rate() with a rate 50*2 = 100MHz,
> > > - clock side:
> > >   - the clock driver sets the hardware MMC clock to 100*2 = 200MHz,
> > >   - setting the 'mode select' of the hardware MMC clock divides the
> > > rate by 2,
> > > - MMC side:
> > >   - setting the MMC clock divider register to 1 divides the rate by 2.
> > > So, the final rate is 50MHz.
> > 
> > What happens if you actually want to set it to 100MHz?
> 
> There is no SDXC_CLK_100M in the mainline driver, and 100MHz is asked
> only for 8 bits DDR at 50MHz.

You're missing the point.

clk_set_rate is supposed to apply a rate as close as possible as
requested, there's no reason why you would request a rate twice as
high as need.

You want to switch the clock from one mode to another, fine, create a
new function for that. But don't hack an existing one.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

-- 
You received this message because you are subscribed to the Google Groups 
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to linux-sunxi+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: PGP signature

[linux-sunxi] Re: [PATCH v2 3/5] ARM: sun8i: dt: Add DT bindings documentation for Allwinner sun8i-emac

[Maxime Ripard]

On Fri, Jul 29, 2016 at 10:15:19AM +0200, LABBE Corentin wrote:
> > > > > +See ethernet.txt in the same directory for generic bindings for 
> > > > > ethernet
> > > > > +controllers.
> > > > > +
> > > > > +The device node referenced by "phy" or "phy-handle" should be a 
> > > > > child node
> > > > > +of this node. See phy.txt for the generic PHY bindings.
> > > > > +
> > > > > +Optional properties:
> > > > > +- phy-supply: phandle to a regulator if the PHY needs one
> > > > > +- phy-io-supply: phandle to a regulator if the PHY needs a another 
> > > > > one for I/O.
> > > > > +  This is sometimes found with RGMII PHYs, which use a 
> > > > > second
> > > > > +  regulator for the lower I/O voltage.
> > > > > +- allwinner,tx-delay: The setting of the TX clock delay chain
> > > > > +- allwinner,rx-delay: The setting of the RX clock delay chain
> > > > 
> > > > In which unit? What is the default value?
> > > 
> > > The unit is unknown to me, but I have added a comment for the
> > > default and acceptable range value.
> > 
> > That's unfortunate. We'll see how the DT maintainers feel about that.
> > 
> 
> I have searched for txdelay in Documentation, and found a few driver
> that give the units (us/ps).
>
> But in that case, the value in ps/us must be found in a table
> indexed by the Xxdelay value.
>
> So the settings seems always a raw number, and for sun8i-emac
> nothing in user manual could help to find what each value is/related
> to.
> 
> So the good value is either found by "try and test" or "copy the
> value found in fex file".

What I meant was that, just like you found out already, most of the
time the properties should be in absolute units, so that it doesn't
depend on some clock rate most likely in that case.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

-- 
You received this message because you are subscribed to the Google Groups 
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to linux-sunxi+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: PGP signature

[linux-sunxi] Re: [PATCH v2 1/5] ethernet: add sun8i-emac driver

[Andre Przywara]

Hi,

On 25/07/16 20:54, Maxime Ripard wrote:
> On Wed, Jul 20, 2016 at 10:03:16AM +0200, LABBE Corentin wrote:
>> This patch add support for sun8i-emac ethernet MAC hardware.
>> It could be found in Allwinner H3/A83T/A64 SoCs.
>>
>> It supports 10/100/1000 Mbit/s speed with half/full duplex.
>> It can use an internal PHY (MII 10/100) or an external PHY
>> via RGMII/RMII.
>>
>> Signed-off-by: LABBE Corentin 
>> ---
>>  drivers/net/ethernet/allwinner/Kconfig  |   13 +
>>  drivers/net/ethernet/allwinner/Makefile |1 +
>>  drivers/net/ethernet/allwinner/sun8i-emac.c | 2129 
>> +++
>>  3 files changed, 2143 insertions(+)
>>  create mode 100644 drivers/net/ethernet/allwinner/sun8i-emac.c

...

>> diff --git a/drivers/net/ethernet/allwinner/sun8i-emac.c 
>> b/drivers/net/ethernet/allwinner/sun8i-emac.c
>> new file mode 100644
>> index 000..fc0c1dd
>> --- /dev/null
>> +++ b/drivers/net/ethernet/allwinner/sun8i-emac.c

...

>> +
>> +/* struct dma_desc - Structure of DMA descriptor used by the hardware
>> + * @status: Status of the frame written by HW, so RO for the
>> + *  driver (except for BIT(31) which is R/W)
>> + * @ctl: Information on the frame written by the driver (INT, len,...)
>> + * @buf_addr: physical address of the frame data
>> + * @next: physical address of next dma_desc
>> + */
>> +struct dma_desc {
>> +u32 status;
>> +u32 ctl;
>> +u32 buf_addr;
>> +u32 next;
>> +};
> 
> You should use the endian-aware variants here.

For the records: just doing the sparse annotation with __le32 here will
of course not be sufficient to make it work on BE kernels. I added
proper endianness conversion to all accesses to the descriptors and got
it to work with an arm64 big-endian kernel on the Pine64.

I put a patch here:
https://gist.github.com/apritzel/bc792c4dbbd8789f5f18aef538e8c440

This particular version is untested (though it compiles), since I just
adapted the working patch against the newer driver code and couldn't
test it yet.
I am not really an endianness expert, so don't know if there are smarter
ways to tackle this, if we should for instance provide access wrappers
to the DMA descriptor fields.

I will try to test this later today, if that works, feel free to merge
those changes into your driver.

Cheers,
Andre.

-- 
You received this message because you are subscribed to the Google Groups 
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to linux-sunxi+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[linux-sunxi] Re: [PATCH v2 1/5] ethernet: add sun8i-emac driver

[Arnd Bergmann]

On Thursday, July 28, 2016 3:18:26 PM CEST LABBE Corentin wrote:
> 
> I will reworked locking and it seems that no locking is necessary.
> I have added the following comment about the locking strategy:
> 
> /* Locking strategy:
>  * RX queue does not need any lock since only sun8i_emac_poll() access it.
>  * (All other RX modifiers (ringparam/ndo_stop) disable NAPI and so 
> sun8i_emac_poll())
>  * TX queue is handled by sun8i_emac_xmit(), sun8i_emac_complete_xmit() and 
> sun8i_emac_tx_timeout()
>  * (All other RX modifiers (ringparam/ndo_stop) disable NAPI and stop queue)
>  *
>  * sun8i_emac_xmit() could fire only once (netif_tx_lock)
>  * sun8i_emac_complete_xmit() could fire only once (called from NAPI)
>  * sun8i_emac_tx_timeout() could fire only once (netif_tx_lock) and couldnt
>  * race with sun8i_emac_xmit (due to netif_tx_lock) and with 
> sun8i_emac_complete_xmit which disable NAPI.
>  *
>  * So only sun8i_emac_xmit and sun8i_emac_complete_xmit could fire at the 
> same time.
>  * But they never could modify the same descriptors:
>  * - sun8i_emac_complete_xmit() will modify only descriptors with empty status
>  * - sun8i_emac_xmit() will modify only descriptors set to DCLEAN
>  * Proper memory barriers ensure that descriptor set to DCLEAN could not be
>  * modified latter by sun8i_emac_complete_xmit().
>  * */

Sounds good, the comment is certainly very helpful here.

Arnd

-- 
You received this message because you are subscribed to the Google Groups 
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to linux-sunxi+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.